From: Zheyu Ma <zheyuma97@gmail.com>
To: Stanislav Yakovlev <stas.yakovlev@gmail.com>
Cc: kvalo@kernel.org, "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
pabeni@redhat.com, wireless <linux-wireless@vger.kernel.org>,
netdev@vger.kernel.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one()
Date: Fri, 20 May 2022 11:46:38 +0800 [thread overview]
Message-ID: <CAMhUBjmbfYCiNvgVkC7x0QQLCxsttEX9CpHOK=N+Gt4YxUCu2Q@mail.gmail.com> (raw)
In-Reply-To: <CA++WF2MFwtKs8-uy+e_77P0ySsN8y6W_8+Z8AdxBKsutcYK-ig@mail.gmail.com>
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
prev parent reply other threads:[~2022-05-20 3:46 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-09 6:24 [PATCH] wireless: ipw2x00: Refine the error handling of ipw2100_pci_init_one() Zheyu Ma
2022-04-12 13:55 ` Kalle Valo
2022-04-13 18:39 ` Stanislav Yakovlev
2022-04-18 3:04 ` Zheyu Ma
2022-04-22 19:25 ` Stanislav Yakovlev
2022-05-20 3:46 ` Zheyu Ma [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMhUBjmbfYCiNvgVkC7x0QQLCxsttEX9CpHOK=N+Gt4YxUCu2Q@mail.gmail.com' \
--to=zheyuma97@gmail.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=stas.yakovlev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).