| Message ID | 20221024135728.2894863-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: fealnx: fix missing pci_disable_device() | expand |
On Mon, Oct 24, 2022 at 09:57:28PM +0800, Yang Yingliang wrote: > pci_disable_device() need be called while module exiting, switch > to use pcim_enable(), pci_disable_device() and pci_release_regions() > will be called in pcim_release() while unbinding device. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/net/ethernet/fealnx.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c > index ed18450fd2cc..fb139f295b67 100644 > --- a/drivers/net/ethernet/fealnx.c > +++ b/drivers/net/ethernet/fealnx.c > @@ -494,7 +494,7 @@ static int fealnx_init_one(struct pci_dev *pdev, > > option = card_idx < MAX_UNITS ? options[card_idx] : 0; > > - i = pci_enable_device(pdev); > + i = pcim_enable_device(pdev); > if (i) return i; > pci_set_master(pdev); > > @@ -670,7 +670,6 @@ static int fealnx_init_one(struct pci_dev *pdev, > err_out_unmap: > pci_iounmap(pdev, ioaddr); > err_out_res: > - pci_release_regions(pdev); > return err; > } > > @@ -689,7 +688,6 @@ static void fealnx_remove_one(struct pci_dev *pdev) > unregister_netdev(dev); > pci_iounmap(pdev, np->mem); > free_netdev(dev); > - pci_release_regions(pdev); > } else > printk(KERN_ERR "fealnx: remove for unknown device\n"); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This path is not possible. > } > -- > 2.25.1 >
On Mon, Oct 24, 2022 at 09:57:28PM +0800, Yang Yingliang wrote: > pci_disable_device() need be called while module exiting, switch > to use pcim_enable(), pci_disable_device() and pci_release_regions() > will be called in pcim_release() while unbinding device. I didn't understand the description at all, maybe people in CC will. Most likely, you wanted something like this: diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c index ed18450fd2cc..78107dd4aa57 100644 --- a/drivers/net/ethernet/fealnx.c +++ b/drivers/net/ethernet/fealnx.c @@ -690,6 +690,7 @@ static void fealnx_remove_one(struct pci_dev *pdev) pci_iounmap(pdev, np->mem); free_netdev(dev); pci_release_regions(pdev); + pci_disable_device(pdev); } else printk(KERN_ERR "fealnx: remove for unknown device\n"); > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/net/ethernet/fealnx.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c > index ed18450fd2cc..fb139f295b67 100644 > --- a/drivers/net/ethernet/fealnx.c > +++ b/drivers/net/ethernet/fealnx.c > @@ -494,7 +494,7 @@ static int fealnx_init_one(struct pci_dev *pdev, > > option = card_idx < MAX_UNITS ? options[card_idx] : 0; > > - i = pci_enable_device(pdev); > + i = pcim_enable_device(pdev); > if (i) return i; > pci_set_master(pdev); > > @@ -670,7 +670,6 @@ static int fealnx_init_one(struct pci_dev *pdev, > err_out_unmap: > pci_iounmap(pdev, ioaddr); > err_out_res: > - pci_release_regions(pdev); > return err; > } > > @@ -689,7 +688,6 @@ static void fealnx_remove_one(struct pci_dev *pdev) > unregister_netdev(dev); > pci_iounmap(pdev, np->mem); > free_netdev(dev); > - pci_release_regions(pdev); > } else > printk(KERN_ERR "fealnx: remove for unknown device\n"); > } > -- > 2.25.1 >
On 2022/10/25 15:47, Leon Romanovsky wrote: > On Mon, Oct 24, 2022 at 09:57:28PM +0800, Yang Yingliang wrote: >> pci_disable_device() need be called while module exiting, switch >> to use pcim_enable(), pci_disable_device() and pci_release_regions() >> will be called in pcim_release() while unbinding device. > I didn't understand the description at all, maybe people in CC will. > Most likely, you wanted something like this: After using pcim_enable_device(), pcim_release() will be called while unbinding device, pcim_release() calls both pci_release_regions() and pci_disable_device(). Thanks, Yang > > diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c > index ed18450fd2cc..78107dd4aa57 100644 > --- a/drivers/net/ethernet/fealnx.c > +++ b/drivers/net/ethernet/fealnx.c > @@ -690,6 +690,7 @@ static void fealnx_remove_one(struct pci_dev *pdev) > pci_iounmap(pdev, np->mem); > free_netdev(dev); > pci_release_regions(pdev); > + pci_disable_device(pdev); > } else > printk(KERN_ERR "fealnx: remove for unknown device\n"); > > >> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> drivers/net/ethernet/fealnx.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c >> index ed18450fd2cc..fb139f295b67 100644 >> --- a/drivers/net/ethernet/fealnx.c >> +++ b/drivers/net/ethernet/fealnx.c >> @@ -494,7 +494,7 @@ static int fealnx_init_one(struct pci_dev *pdev, >> >> option = card_idx < MAX_UNITS ? options[card_idx] : 0; >> >> - i = pci_enable_device(pdev); >> + i = pcim_enable_device(pdev); >> if (i) return i; >> pci_set_master(pdev); >> >> @@ -670,7 +670,6 @@ static int fealnx_init_one(struct pci_dev *pdev, >> err_out_unmap: >> pci_iounmap(pdev, ioaddr); >> err_out_res: >> - pci_release_regions(pdev); >> return err; >> } >> >> @@ -689,7 +688,6 @@ static void fealnx_remove_one(struct pci_dev *pdev) >> unregister_netdev(dev); >> pci_iounmap(pdev, np->mem); >> free_netdev(dev); >> - pci_release_regions(pdev); >> } else >> printk(KERN_ERR "fealnx: remove for unknown device\n"); >> } >> -- >> 2.25.1 >> > .
On Tue, Oct 25, 2022 at 05:01:58PM +0800, Yang Yingliang wrote: > > On 2022/10/25 15:47, Leon Romanovsky wrote: > > On Mon, Oct 24, 2022 at 09:57:28PM +0800, Yang Yingliang wrote: > > > pci_disable_device() need be called while module exiting, switch > > > to use pcim_enable(), pci_disable_device() and pci_release_regions() > > > will be called in pcim_release() while unbinding device. > > I didn't understand the description at all, maybe people in CC will. > > Most likely, you wanted something like this: > After using pcim_enable_device(), pcim_release() will be called while > unbinding > device, pcim_release() calls both pci_release_regions() and > pci_disable_device(). I'm not sure that you can mix managed with unmanaged PCI APIs. Thanks > > Thanks, > Yang > > > > diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c > > index ed18450fd2cc..78107dd4aa57 100644 > > --- a/drivers/net/ethernet/fealnx.c > > +++ b/drivers/net/ethernet/fealnx.c > > @@ -690,6 +690,7 @@ static void fealnx_remove_one(struct pci_dev *pdev) > > pci_iounmap(pdev, np->mem); > > free_netdev(dev); > > pci_release_regions(pdev); > > + pci_disable_device(pdev); > > } else > > printk(KERN_ERR "fealnx: remove for unknown device\n"); > > > > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > --- > > > drivers/net/ethernet/fealnx.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c > > > index ed18450fd2cc..fb139f295b67 100644 > > > --- a/drivers/net/ethernet/fealnx.c > > > +++ b/drivers/net/ethernet/fealnx.c > > > @@ -494,7 +494,7 @@ static int fealnx_init_one(struct pci_dev *pdev, > > > option = card_idx < MAX_UNITS ? options[card_idx] : 0; > > > - i = pci_enable_device(pdev); > > > + i = pcim_enable_device(pdev); > > > if (i) return i; > > > pci_set_master(pdev); > > > @@ -670,7 +670,6 @@ static int fealnx_init_one(struct pci_dev *pdev, > > > err_out_unmap: > > > pci_iounmap(pdev, ioaddr); > > > err_out_res: > > > - pci_release_regions(pdev); > > > return err; > > > } > > > @@ -689,7 +688,6 @@ static void fealnx_remove_one(struct pci_dev *pdev) > > > unregister_netdev(dev); > > > pci_iounmap(pdev, np->mem); > > > free_netdev(dev); > > > - pci_release_regions(pdev); > > > } else > > > printk(KERN_ERR "fealnx: remove for unknown device\n"); > > > } > > > -- > > > 2.25.1 > > > > > .
On 2022/10/25 18:27, Leon Romanovsky wrote: > On Tue, Oct 25, 2022 at 05:01:58PM +0800, Yang Yingliang wrote: >> On 2022/10/25 15:47, Leon Romanovsky wrote: >>> On Mon, Oct 24, 2022 at 09:57:28PM +0800, Yang Yingliang wrote: >>>> pci_disable_device() need be called while module exiting, switch >>>> to use pcim_enable(), pci_disable_device() and pci_release_regions() >>>> will be called in pcim_release() while unbinding device. >>> I didn't understand the description at all, maybe people in CC will. >>> Most likely, you wanted something like this: >> After using pcim_enable_device(), pcim_release() will be called while >> unbinding >> device, pcim_release() calls both pci_release_regions() and >> pci_disable_device(). > I'm not sure that you can mix managed with unmanaged PCI APIs. > > Thanks After pcim_enable_device() being called, the region_mask is set in __pci_request_region(). pcim_release() will call pci_release_region() because region_mask is set. Without device managed, the pci_release_region() and pci_disable_device() is called at end of remove() function which is called in device_remove(), and with managed, they are called in device_unbind_cleanup() which is called after device_remove(). So I think it's OK to use this management. Thanks, Yang > >> Thanks, >> Yang >>> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c >>> index ed18450fd2cc..78107dd4aa57 100644 >>> --- a/drivers/net/ethernet/fealnx.c >>> +++ b/drivers/net/ethernet/fealnx.c >>> @@ -690,6 +690,7 @@ static void fealnx_remove_one(struct pci_dev *pdev) >>> pci_iounmap(pdev, np->mem); >>> free_netdev(dev); >>> pci_release_regions(pdev); >>> + pci_disable_device(pdev); >>> } else >>> printk(KERN_ERR "fealnx: remove for unknown device\n"); >>> >>> >>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>>> --- >>>> drivers/net/ethernet/fealnx.c | 4 +--- >>>> 1 file changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c >>>> index ed18450fd2cc..fb139f295b67 100644 >>>> --- a/drivers/net/ethernet/fealnx.c >>>> +++ b/drivers/net/ethernet/fealnx.c >>>> @@ -494,7 +494,7 @@ static int fealnx_init_one(struct pci_dev *pdev, >>>> option = card_idx < MAX_UNITS ? options[card_idx] : 0; >>>> - i = pci_enable_device(pdev); >>>> + i = pcim_enable_device(pdev); >>>> if (i) return i; >>>> pci_set_master(pdev); >>>> @@ -670,7 +670,6 @@ static int fealnx_init_one(struct pci_dev *pdev, >>>> err_out_unmap: >>>> pci_iounmap(pdev, ioaddr); >>>> err_out_res: >>>> - pci_release_regions(pdev); >>>> return err; >>>> } >>>> @@ -689,7 +688,6 @@ static void fealnx_remove_one(struct pci_dev *pdev) >>>> unregister_netdev(dev); >>>> pci_iounmap(pdev, np->mem); >>>> free_netdev(dev); >>>> - pci_release_regions(pdev); >>>> } else >>>> printk(KERN_ERR "fealnx: remove for unknown device\n"); >>>> } >>>> -- >>>> 2.25.1 >>>> >>> . > .
On Tue, Oct 25, 2022 at 07:25:48PM +0800, Yang Yingliang wrote: > > On 2022/10/25 18:27, Leon Romanovsky wrote: > > On Tue, Oct 25, 2022 at 05:01:58PM +0800, Yang Yingliang wrote: > > > On 2022/10/25 15:47, Leon Romanovsky wrote: > > > > On Mon, Oct 24, 2022 at 09:57:28PM +0800, Yang Yingliang wrote: > > > > > pci_disable_device() need be called while module exiting, switch > > > > > to use pcim_enable(), pci_disable_device() and pci_release_regions() > > > > > will be called in pcim_release() while unbinding device. > > > > I didn't understand the description at all, maybe people in CC will. > > > > Most likely, you wanted something like this: > > > After using pcim_enable_device(), pcim_release() will be called while > > > unbinding > > > device, pcim_release() calls both pci_release_regions() and > > > pci_disable_device(). > > I'm not sure that you can mix managed with unmanaged PCI APIs. > > > > Thanks > After pcim_enable_device() being called, the region_mask is set in > __pci_request_region(). pcim_release() will call pci_release_region() > because region_mask is set. > > Without device managed, the pci_release_region() and pci_disable_device() > is called at end of remove() function which is called in device_remove(), > and > with managed, they are called in device_unbind_cleanup() which is called > after device_remove(). So I think it's OK to use this management. Even so, why do you think that call in device_unbind_cleanup() is the right one for such an old driver? Thanks > > Thanks, > Yang > > > > > Thanks, > > > Yang > > > > diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c > > > > index ed18450fd2cc..78107dd4aa57 100644 > > > > --- a/drivers/net/ethernet/fealnx.c > > > > +++ b/drivers/net/ethernet/fealnx.c > > > > @@ -690,6 +690,7 @@ static void fealnx_remove_one(struct pci_dev *pdev) > > > > pci_iounmap(pdev, np->mem); > > > > free_netdev(dev); > > > > pci_release_regions(pdev); > > > > + pci_disable_device(pdev); > > > > } else > > > > printk(KERN_ERR "fealnx: remove for unknown device\n"); > > > > > > > > > > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > > > --- > > > > > drivers/net/ethernet/fealnx.c | 4 +--- > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c > > > > > index ed18450fd2cc..fb139f295b67 100644 > > > > > --- a/drivers/net/ethernet/fealnx.c > > > > > +++ b/drivers/net/ethernet/fealnx.c > > > > > @@ -494,7 +494,7 @@ static int fealnx_init_one(struct pci_dev *pdev, > > > > > option = card_idx < MAX_UNITS ? options[card_idx] : 0; > > > > > - i = pci_enable_device(pdev); > > > > > + i = pcim_enable_device(pdev); > > > > > if (i) return i; > > > > > pci_set_master(pdev); > > > > > @@ -670,7 +670,6 @@ static int fealnx_init_one(struct pci_dev *pdev, > > > > > err_out_unmap: > > > > > pci_iounmap(pdev, ioaddr); > > > > > err_out_res: > > > > > - pci_release_regions(pdev); > > > > > return err; > > > > > } > > > > > @@ -689,7 +688,6 @@ static void fealnx_remove_one(struct pci_dev *pdev) > > > > > unregister_netdev(dev); > > > > > pci_iounmap(pdev, np->mem); > > > > > free_netdev(dev); > > > > > - pci_release_regions(pdev); > > > > > } else > > > > > printk(KERN_ERR "fealnx: remove for unknown device\n"); > > > > > } > > > > > -- > > > > > 2.25.1 > > > > > > > > > . > > .
diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c index ed18450fd2cc..fb139f295b67 100644 --- a/drivers/net/ethernet/fealnx.c +++ b/drivers/net/ethernet/fealnx.c @@ -494,7 +494,7 @@ static int fealnx_init_one(struct pci_dev *pdev, option = card_idx < MAX_UNITS ? options[card_idx] : 0; - i = pci_enable_device(pdev); + i = pcim_enable_device(pdev); if (i) return i; pci_set_master(pdev); @@ -670,7 +670,6 @@ static int fealnx_init_one(struct pci_dev *pdev, err_out_unmap: pci_iounmap(pdev, ioaddr); err_out_res: - pci_release_regions(pdev); return err; } @@ -689,7 +688,6 @@ static void fealnx_remove_one(struct pci_dev *pdev) unregister_netdev(dev); pci_iounmap(pdev, np->mem); free_netdev(dev); - pci_release_regions(pdev); } else printk(KERN_ERR "fealnx: remove for unknown device\n"); }
pci_disable_device() need be called while module exiting, switch to use pcim_enable(), pci_disable_device() and pci_release_regions() will be called in pcim_release() while unbinding device. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/net/ethernet/fealnx.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)