| Message ID | 20220409062449.3752252-1-zheyuma97@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one() | expand |
Zheyu Ma <zheyuma97@gmail.com> wrote: > The driver should release resources in reverse order, i.e., the > resources requested first should be released last, and the driver > should adjust the order of error handling code by this rule. > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> Can someone review this, please?
On Sat, 9 Apr 2022 at 02:25, Zheyu Ma <zheyuma97@gmail.com> wrote: > > The driver should release resources in reverse order, i.e., the > resources requested first should be released last, and the driver > should adjust the order of error handling code by this rule. > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> > --- > drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++----------- > 1 file changed, 16 insertions(+), 18 deletions(-) > [Skipped] > @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev, > out: > return err; > > - fail_unlock: > +fail_unlock: > mutex_unlock(&priv->action_mutex); > - fail: > +fail: > + pci_release_regions(pci_dev); > +fail_disable: > + pci_disable_device(pci_dev); We can't move these functions before the following block. > +fail_dev: > if (dev) { > if (registered >= 2) > unregister_netdev(dev); This block continues with a function call to ipw2100_hw_stop_adapter which assumes that device is still accessible via pci bus. > @@ -6334,11 +6335,8 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev, > > free_libipw(dev, 0); > } > - > +fail_iounmap: > pci_iounmap(pci_dev, ioaddr); > - > - pci_release_regions(pci_dev); > - pci_disable_device(pci_dev); > goto out; > } > > -- > 2.25.1 > Stanislav.
On Thu, Apr 14, 2022 at 2:40 AM Stanislav Yakovlev <stas.yakovlev@gmail.com> wrote: > > On Sat, 9 Apr 2022 at 02:25, Zheyu Ma <zheyuma97@gmail.com> wrote: > > > > The driver should release resources in reverse order, i.e., the > > resources requested first should be released last, and the driver > > should adjust the order of error handling code by this rule. > > > > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> > > --- > > drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++----------- > > 1 file changed, 16 insertions(+), 18 deletions(-) > > > [Skipped] > > > @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev, > > out: > > return err; > > > > - fail_unlock: > > +fail_unlock: > > mutex_unlock(&priv->action_mutex); > > - fail: > > +fail: > > + pci_release_regions(pci_dev); > > +fail_disable: > > + pci_disable_device(pci_dev); > We can't move these functions before the following block. > > > +fail_dev: > > if (dev) { > > if (registered >= 2) > > unregister_netdev(dev); > This block continues with a function call to ipw2100_hw_stop_adapter > which assumes that device is still accessible via pci bus. Thanks for your reminder, but the existing error handling does need to be revised, I got the following warning when the probing fails at pci_resource_flags(): [ 20.712160] WARNING: CPU: 1 PID: 462 at lib/iomap.c:44 pci_iounmap+0x40/0x50 [ 20.716583] RIP: 0010:pci_iounmap+0x40/0x50 [ 20.726342] <TASK> [ 20.726550] ipw2100_pci_init_one+0x101/0x1ee0 [ipw2100] Since I am not familiar with the ipw2100, could someone give me some advice to fix this. Thanks, Zheyu Ma
Hi Zheyu, On 18/04/2022, Zheyu Ma <zheyuma97@gmail.com> wrote: > On Thu, Apr 14, 2022 at 2:40 AM Stanislav Yakovlev > <stas.yakovlev@gmail.com> wrote: >> >> On Sat, 9 Apr 2022 at 02:25, Zheyu Ma <zheyuma97@gmail.com> wrote: >> > >> > The driver should release resources in reverse order, i.e., the >> > resources requested first should be released last, and the driver >> > should adjust the order of error handling code by this rule. >> > >> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> >> > --- >> > drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++----------- >> > 1 file changed, 16 insertions(+), 18 deletions(-) >> > >> [Skipped] >> >> > @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev >> > *pci_dev, >> > out: >> > return err; >> > >> > - fail_unlock: >> > +fail_unlock: >> > mutex_unlock(&priv->action_mutex); >> > - fail: >> > +fail: >> > + pci_release_regions(pci_dev); >> > +fail_disable: >> > + pci_disable_device(pci_dev); >> We can't move these functions before the following block. >> >> > +fail_dev: >> > if (dev) { >> > if (registered >= 2) >> > unregister_netdev(dev); >> This block continues with a function call to ipw2100_hw_stop_adapter >> which assumes that device is still accessible via pci bus. > > Thanks for your reminder, but the existing error handling does need to > be revised, I got the following warning when the probing fails at > pci_resource_flags(): > > [ 20.712160] WARNING: CPU: 1 PID: 462 at lib/iomap.c:44 > pci_iounmap+0x40/0x50 > [ 20.716583] RIP: 0010:pci_iounmap+0x40/0x50 > [ 20.726342] <TASK> > [ 20.726550] ipw2100_pci_init_one+0x101/0x1ee0 [ipw2100] > > Since I am not familiar with the ipw2100, could someone give me some > advice to fix this. Could you please rebuild the kernel with IPW2100_DEBUG config option enabled, rerun the test and post your results here? Also, please post the output of "lspci -v" here. Stanislav.
On Sat, Apr 23, 2022 at 3:25 AM Stanislav Yakovlev <stas.yakovlev@gmail.com> wrote: > > Hi Zheyu, > > On 18/04/2022, Zheyu Ma <zheyuma97@gmail.com> wrote: > > On Thu, Apr 14, 2022 at 2:40 AM Stanislav Yakovlev > > <stas.yakovlev@gmail.com> wrote: > >> > >> On Sat, 9 Apr 2022 at 02:25, Zheyu Ma <zheyuma97@gmail.com> wrote: > >> > > >> > The driver should release resources in reverse order, i.e., the > >> > resources requested first should be released last, and the driver > >> > should adjust the order of error handling code by this rule. > >> > > >> > Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> > >> > --- > >> > drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++----------- > >> > 1 file changed, 16 insertions(+), 18 deletions(-) > >> > > >> [Skipped] > >> > >> > @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev > >> > *pci_dev, > >> > out: > >> > return err; > >> > > >> > - fail_unlock: > >> > +fail_unlock: > >> > mutex_unlock(&priv->action_mutex); > >> > - fail: > >> > +fail: > >> > + pci_release_regions(pci_dev); > >> > +fail_disable: > >> > + pci_disable_device(pci_dev); > >> We can't move these functions before the following block. > >> > >> > +fail_dev: > >> > if (dev) { > >> > if (registered >= 2) > >> > unregister_netdev(dev); > >> This block continues with a function call to ipw2100_hw_stop_adapter > >> which assumes that device is still accessible via pci bus. > > > > Thanks for your reminder, but the existing error handling does need to > > be revised, I got the following warning when the probing fails at > > pci_resource_flags(): > > > > [ 20.712160] WARNING: CPU: 1 PID: 462 at lib/iomap.c:44 > > pci_iounmap+0x40/0x50 > > [ 20.716583] RIP: 0010:pci_iounmap+0x40/0x50 > > [ 20.726342] <TASK> > > [ 20.726550] ipw2100_pci_init_one+0x101/0x1ee0 [ipw2100] > > > > Since I am not familiar with the ipw2100, could someone give me some > > advice to fix this. > > Could you please rebuild the kernel with IPW2100_DEBUG config option > enabled, rerun the test and post your results here? Also, please post > the output of "lspci -v" here. Sorry for the late response. I have rebuilt the kernel with IPW2100_DEBUG, and got the following result: [ 29.469624] libipw: 802.11 data/management/control stack, git-1.1.13 [ 29.470034] libipw: Copyright (C) 2004-2005 Intel Corporation <jketreno@linux.intel.com> [ 29.477455] ipw2100: Intel(R) PRO/Wireless 2100 Network Driver, git-1.2.2 [ 29.477833] ipw2100: Copyright(c) 2003-2006 Intel Corporation [ 29.478197] ipw2100Error calling ioremap. [ 29.478398] ------------[ cut here ]------------ [ 29.478630] Bad IO access at port 0x0 () [ 29.478834] WARNING: CPU: 0 PID: 304 at lib/iomap.c:44 pci_iounmap+0x40/0x50 [ 29.481116] RIP: 0010:pci_iounmap+0x40/0x50 [ 29.485282] Call Trace: [ 29.485407] <TASK> [ 29.485514] ipw2100_pci_init_one+0x192/0x20c0 [ipw2100] [ 29.487496] local_pci_probe+0x13f/0x200 Actually, I made a special virtual device to test this driver, not real hardware, so the "lspci" results are irrelevant here. I injected a software fault in the pci_iomap() function to force the driver to fail here, and then the driver goto "fail" tag, and under the "fail" tag the pci_iounmap() is called, resulting in a crash. In fact such error handling is incorrect and we should not release resources that have not been requested yet. Thanks, Zheyu Ma
diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2100.c b/drivers/net/wireless/intel/ipw2x00/ipw2100.c index 2ace2b27ecad..b10d10660eb8 100644 --- a/drivers/net/wireless/intel/ipw2x00/ipw2100.c +++ b/drivers/net/wireless/intel/ipw2x00/ipw2100.c @@ -6166,7 +6166,7 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev, printk(KERN_WARNING DRV_NAME "Error calling ioremap.\n"); err = -EIO; - goto fail; + goto out; } /* allocate and initialize our net_device */ @@ -6175,36 +6175,33 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev, printk(KERN_WARNING DRV_NAME "Error calling ipw2100_alloc_device.\n"); err = -ENOMEM; - goto fail; + goto fail_iounmap; } + priv = libipw_priv(dev); + pci_set_master(pci_dev); + pci_set_drvdata(pci_dev, priv); + /* set up PCI mappings for device */ err = pci_enable_device(pci_dev); if (err) { printk(KERN_WARNING DRV_NAME "Error calling pci_enable_device.\n"); - return err; + goto fail_dev; } - priv = libipw_priv(dev); - - pci_set_master(pci_dev); - pci_set_drvdata(pci_dev, priv); - err = dma_set_mask(&pci_dev->dev, DMA_BIT_MASK(32)); if (err) { printk(KERN_WARNING DRV_NAME "Error calling pci_set_dma_mask.\n"); - pci_disable_device(pci_dev); - return err; + goto fail_disable; } err = pci_request_regions(pci_dev, DRV_NAME); if (err) { printk(KERN_WARNING DRV_NAME "Error calling pci_request_regions.\n"); - pci_disable_device(pci_dev); - return err; + goto fail_disable; } /* We disable the RETRY_TIMEOUT register (0x41) to keep @@ -6306,9 +6303,13 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev, out: return err; - fail_unlock: +fail_unlock: mutex_unlock(&priv->action_mutex); - fail: +fail: + pci_release_regions(pci_dev); +fail_disable: + pci_disable_device(pci_dev); +fail_dev: if (dev) { if (registered >= 2) unregister_netdev(dev); @@ -6334,11 +6335,8 @@ static int ipw2100_pci_init_one(struct pci_dev *pci_dev, free_libipw(dev, 0); } - +fail_iounmap: pci_iounmap(pci_dev, ioaddr); - - pci_release_regions(pci_dev); - pci_disable_device(pci_dev); goto out; }
The driver should release resources in reverse order, i.e., the resources requested first should be released last, and the driver should adjust the order of error handling code by this rule. Signed-off-by: Zheyu Ma <zheyuma97@gmail.com> --- drivers/net/wireless/intel/ipw2x00/ipw2100.c | 34 +++++++++----------- 1 file changed, 16 insertions(+), 18 deletions(-)