NetBSD Problem Report #44144
From apb@cequrux.com Wed Nov 24 09:49:27 2010
Return-Path: <apb@cequrux.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
by www.NetBSD.org (Postfix) with ESMTP id E57C263B95F
for <gnats-bugs@gnats.NetBSD.org>; Wed, 24 Nov 2010 09:49:26 +0000 (UTC)
Message-Id: <20101124094541.C6101100AA74@apb-laptoy.apb.alt.za>
Date: Wed, 24 Nov 2010 09:45:41 +0000 (UTC)
From: apb@cequrux.com
Reply-To: apb@cequrux.com
To: gnats-bugs@gnats.NetBSD.org
Subject: ifconfig causes assertion failure in vfs_lookup.c
X-Send-Pr-Version: 3.95
>Number: 44144
>Category: kern
>Synopsis: ifconfig causes assertion failure in vfs_lookup.c
>Confidential: no
>Severity: critical
>Priority: high
>Responsible: kern-bug-people
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Wed Nov 24 09:50:00 +0000 2010
>Closed-Date: Mon Oct 07 04:57:35 +0000 2013
>Last-Modified: Mon Oct 07 04:57:35 +0000 2013
>Originator: Alan Barrett
>Release: NetBSD 5.99.40
>Organization:
Not much
>Environment:
NetBSD version: 5.99.40 (2010-11-23)
MACHINE: i386
MACHINE_ARCH: i386
Build platform: NetBSD 5.99.27 i386 (2010-04-18)
>Description:
kernel diagnostic assertion "bp->pb_pathcopyuses == 0" failed: file
.../src/sys/kern/vfs_lookup.c line 232
fatal breakpoint trap in supervisor mode
trap type 1 code 0 eip c0249094 cs 0 eflags 246 cr2 bb98a010 ilevel 6
stopped in pid 158.1 (ifconfig) at netbsd:breakpoint+0x4: popl %ebp
db{0}> bt
breakpoint ...
panic ...
ker_assert ...
pathbuf_destroy ...
firmware_open ...
wpi_init ...
ether_ioctl ...
ieee80211_ioctl ...
wpi_ioctl ...
in6_update_ifa ...
in6_ifattach ...
in6_if_up ...
ifioctl_common ...
wpi_ioctl ...
ifioctl ...
soo_ioctl ...
sys_ioctl ...
>How-To-Repeat:
I was running a new kernel (5.99.40) with an old userland (5.99.27).
I was running a script to configure the wpi0 network interface,
and the machine crashed. I think that the command that caused the crash
was "ifconfig wpi0 up".
In case it matters, I use init.chroot in such a way that the real root
is on /dev/md0a, but init chroots into the /cgd1a file system, which is
ffs+wapbl on cgd on wd.
>Fix:
>Release-Note:
>Audit-Trail:
Responsible-Changed-From-To: kern-bug-people->dholland
Responsible-Changed-By: dholland@NetBSD.org
Responsible-Changed-When: Wed, 24 Nov 2010 16:05:48 +0000
Responsible-Changed-Why:
my fault
From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/44144 CVS commit: src/sys/dev
Date: Wed, 24 Nov 2010 16:31:13 +0000
Module Name: src
Committed By: dholland
Date: Wed Nov 24 16:31:12 UTC 2010
Modified Files:
src/sys/dev: firmload.c
Log Message:
Call pathbuf_destroy exactly once, never twice. PR 44144
To generate a diff of this commit:
cvs rdiff -u -r1.15 -r1.16 src/sys/dev/firmload.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Wed, 24 Nov 2010 16:36:53 +0000
State-Changed-Why:
please try firmload.c 1.16
From: Alan Barrett <apb@cequrux.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Fri, 26 Nov 2010 17:06:19 +0200
On Wed, 24 Nov 2010, dholland@NetBSD.org wrote:
> please try firmload.c 1.16
Now I get "ERROR: wpi0: could not read firmware file" on the console
(printed by line 1150 of if_wpi.c), and the system enters ddb.
I have /usr/pkg/libdata/if_wpi/iwlwifi-3945.ucode but, because
this was a new kernel with an old userland, I do not have
/libdata/firmware/if_wpi/iwlwifi-3945.ucode.
Instead of entering ddb, I think the system should return the error in a
way that allows userland to print a sensible message.
--apb (Alan Barrett)
Responsible-Changed-From-To: dholland->kern-bug-people
Responsible-Changed-By: dholland@NetBSD.org
Responsible-Changed-When: Fri, 26 Nov 2010 21:45:53 +0000
Responsible-Changed-Why:
this part does not appear to be my fault
State-Changed-From-To: feedback->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Fri, 26 Nov 2010 21:45:53 +0000
State-Changed-Why:
it is now horking at a higher level.
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Fri, 26 Nov 2010 21:50:24 +0000
On Fri, Nov 26, 2010 at 03:10:07PM +0000, Alan Barrett wrote:
> On Wed, 24 Nov 2010, dholland@NetBSD.org wrote:
> > please try firmload.c 1.16
>
> Now I get "ERROR: wpi0: could not read firmware file" on the console
> (printed by line 1150 of if_wpi.c), and the system enters ddb.
>
> I have /usr/pkg/libdata/if_wpi/iwlwifi-3945.ucode but, because
> this was a new kernel with an old userland, I do not have
> /libdata/firmware/if_wpi/iwlwifi-3945.ucode.
That appears to be progress, anyway...
> Instead of entering ddb, I think the system should return the error in a
> way that allows userland to print a sensible message.
Certainly. Where's it entering ddb? The error return chain seems to be
correct up through wpi_init returning failure, none of the direct
calls from inside if_wpi check that return at all (which is perhaps
broken but not likely to land in ddb) and above that it's some
indirect call through ->if_init from the network stack, and there are
quite a few possible choices.
--
David A. Holland
dholland@netbsd.org
State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Tue, 30 Nov 2010 09:40:12 +0000
State-Changed-Why:
I asked a question on Friday.
From: Alan Barrett <apb@cequrux.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Thu, 23 Dec 2010 20:44:40 +0200
On Fri, 26 Nov 2010, David Holland wrote:
> > Now I get "ERROR: wpi0: could not read firmware file" on the console
> > (printed by line 1150 of if_wpi.c), and the system enters ddb.
> >
> > I have /usr/pkg/libdata/if_wpi/iwlwifi-3945.ucode but, because
> > this was a new kernel with an old userland, I do not have
> > /libdata/firmware/if_wpi/iwlwifi-3945.ucode.
>
> That appears to be progress, anyway...
Sorry, I was wrong about that. I have both
/usr/pkg/libdata/if_wpi/iwlwifi-3945.ucode and
/libdata/firmware/if_wpi/iwlwifi-3945.ucode in the chroot in which
ifconfig was running (which most of userland thinks of as the root file
system, through the use of sysctl init.chroot), but I don't have them
in what the kernel thinks of as the root file system (which is a small
memory disk). My old kernel (5.99.27) is able to load the firmware.
> > Instead of entering ddb, I think the system should return the error in a
> > way that allows userland to print a sensible message.
>
> Certainly. Where's it entering ddb?
$ sudo -s
sudo password:
# ifconfig wpi0 up
wpi0: ERROR: could not read firmware file
uvm_fault(0xcf02292c, 0, 2) -> 0xe
fatal page fault in supervisor mode
trap type 6 code 2 eip c010ce0acs 8 eflags 10246 cr2 0 ilevel 6
kernel: supervisor trap page fault, code=0
Stopped in pid 1173.1 (ifconfig) at netbsd:mutex_enter+0xa: lock cmpxchgl
%
ecx,0(%edx)
db{1}> bt
mutex_enter ...
vn_close ...
firmware_close ...
wpi_init ...
ether_ioctl ...
ieee80211_ioctl ...
wpi_ioctl ... at netbsd:wpi_ioctl+0x43
in6_update_ifa ...
in6_ifattach ...
in6_if_up ...
ifioctl_common ...
wpi_ioctl ... at netbsd:wpi_ioctl+0xdd
ifioctl ...
soo_ioctl ...
sys_ioctl ...
syscall ...
db{1}>
Yes, wpi_ioctl really does appear twice in that call chain.
--apb (Alan Barrett)
State-Changed-From-To: feedback->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 26 Dec 2010 07:36:24 +0000
State-Changed-Why:
feedback received
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: apb@cequrux.com
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Sun, 26 Dec 2010 07:38:26 +0000
On Thu, Dec 23, 2010 at 06:45:02PM +0000, Alan Barrett wrote:
> > Certainly. Where's it entering ddb?
>
> [...]
> db{1}> bt
> mutex_enter ...
> vn_close ...
> firmware_close ...
> wpi_init ...
Looks like garden-variety bad error branching. With luck this will fix
both that problem and a second bad error case:
Index: pci/if_wpi.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_wpi.c,v
retrieving revision 1.48
diff -u -p -r1.48 if_wpi.c
--- pci/if_wpi.c 15 Nov 2010 05:57:39 -0000 1.48
+++ pci/if_wpi.c 26 Dec 2010 07:36:14 -0000
@@ -1149,7 +1149,7 @@ wpi_cache_firmware(struct wpi_softc *sc)
/* load firmware image from disk */
if ((error = firmware_open("if_wpi","iwlwifi-3945.ucode", &fw) != 0)) {
aprint_error_dev(sc->sc_dev, "could not read firmware file\n");
- goto fail1;
+ goto fail0;
}
wpi_firmware_size = firmware_get_size(fw);
@@ -1168,7 +1168,7 @@ wpi_cache_firmware(struct wpi_softc *sc)
"truncated firmware header: %zu bytes\n",
wpi_firmware_size);
error = EINVAL;
- goto fail2;
+ goto fail1;
}
wpi_firmware_image = firmware_malloc(wpi_firmware_size);
@@ -1193,6 +1193,7 @@ fail2:
firmware_free(wpi_firmware_image, wpi_firmware_size);
fail1:
firmware_close(fw);
+fail0:
if (--wpi_firmware_users == 0)
firmware_free(wpi_firmware_image, wpi_firmware_size);
mutex_exit(&wpi_firmware_mutex);
--
David A. Holland
dholland@netbsd.org
From: Alan Barrett <apb@cequrux.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Sun, 26 Dec 2010 15:21:32 +0200
On Sun, 26 Dec 2010, David Holland wrote:
> Looks like garden-variety bad error branching. With luck this will fix
> both that problem and a second bad error case:
I applied that patch, and now I get this:
# ifconfig wpi0 up
wpi0: ERROR: could not read firmware file
panic: free: addr 0x0 not within kmem_map
[...]
db{0}> bt
breakpoint ...
panic ...
kern_free ...
wpi_init ... at netbsd:wpi_init+0x6fe
ether_ioctl ...
[...]
--apb (Alan Barrett)
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Sun, 26 Dec 2010 18:50:39 +0000
(not sent to gnats)
------
From: Alan Barrett <apb@cequrux.com>
To: kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org,
netbsd-bugs@NetBSD.org
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Sun, 26 Dec 2010 15:58:05 +0200
On Sun, 26 Dec 2010, Alan Barrett wrote:
> I applied that patch, and now I get this:
>
> # ifconfig wpi0 up
> wpi0: ERROR: could not read firmware file
> panic: free: addr 0x0 not within kmem_map
If I guard the firmware_free() call after label fail1 at the end of
the wpi_cache_firmware() function with "if (wpi_firmware_imaeg != NULL)"
then I get this:
# ifconfig wpi0 up
wpi0: ERROR: could not read firmware file
wpi0: ERROR: could not load firmware file
wpi0: cannot assign link-local address
wpi0: ERROR: could not read firmware file
wpi0: ERROR: could not load firmware file
wpi0: ERROR: could not read firmware file
wpi0: ERROR: could not load firmware file
wpi0: cannot assign link-local address
#
There's no panic, which is good, but I think that the first error should
have been handled in such a way that the other errors did not occur.
It would also be nice if the "could not read firmware file" message
included the file name.
--apb (Alan Barrett)
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Sun, 26 Dec 2010 19:10:37 +0000
On Sun, Dec 26, 2010 at 06:55:01PM +0000, David Holland wrote:
> If I guard the firmware_free() call after label fail1 at the end of
> the wpi_cache_firmware() function with "if (wpi_firmware_imaeg != NULL)"
> then I get this:
It'd probably also be good not to free the image twice in the fail2
case. (sheesh)
> # ifconfig wpi0 up
> wpi0: ERROR: could not read firmware file
> wpi0: ERROR: could not load firmware file
> wpi0: cannot assign link-local address
> wpi0: ERROR: could not read firmware file
> wpi0: ERROR: could not load firmware file
> wpi0: ERROR: could not read firmware file
> wpi0: ERROR: could not load firmware file
> wpi0: cannot assign link-local address
> #
>
> There's no panic, which is good, but I think that the first error should
> have been handled in such a way that the other errors did not occur.
Yeah, but that will take some restructuring; as things are it's going
to attempt wpi_cache_firmware every time it goes through wpi_init
until it succeeds, and each successive call is going to print a
message. Just suppressing the message after the first time seems bad,
because then if you try it once, move the firmware into place, then
try again it'll fail silently.
it looks like it's going to print the message every time it
> It would also be nice if the "could not read firmware file" message
> included the file name.
It doesn't look possible to get the path, but the name I think can be
arranged.
This patch should cover more of the bases. (But note that I don't have
one of these, so I haven't tested it at all...)
Index: pci/if_wpi.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_wpi.c,v
retrieving revision 1.48
diff -u -p -r1.48 if_wpi.c
--- pci/if_wpi.c 15 Nov 2010 05:57:39 -0000 1.48
+++ pci/if_wpi.c 26 Dec 2010 19:09:23 -0000
@@ -418,8 +418,12 @@ wpi_detach(device_t self, int flags __un
if (sc->fw_used) {
mutex_enter(&wpi_firmware_mutex);
- if (--wpi_firmware_users == 0)
+ if (--wpi_firmware_users == 0) {
+ KASSERT(wpi_firmware_image != NULL);
firmware_free(wpi_firmware_image, wpi_firmware_size);
+ wpi_firmware_image = NULL;
+ wpi_firmware_size = 0;
+ }
mutex_exit(&wpi_firmware_mutex);
}
@@ -1134,6 +1138,7 @@ wpi_load_microcode(struct wpi_softc *sc,
static int
wpi_cache_firmware(struct wpi_softc *sc)
{
+ static const char fwname[] = "iwlwifi-3945.ucode";
firmware_handle_t fw;
int error;
@@ -1147,9 +1152,10 @@ wpi_cache_firmware(struct wpi_softc *sc)
}
/* load firmware image from disk */
- if ((error = firmware_open("if_wpi","iwlwifi-3945.ucode", &fw) != 0)) {
- aprint_error_dev(sc->sc_dev, "could not read firmware file\n");
- goto fail1;
+ if ((error = firmware_open("if_wpi", fwname, &fw) != 0)) {
+ aprint_error_dev(sc->sc_dev,
+ "Could not open firmware file %s\n", fwname);
+ goto fail0;
}
wpi_firmware_size = firmware_get_size(fw);
@@ -1158,29 +1164,33 @@ wpi_cache_firmware(struct wpi_softc *sc)
WPI_FW_MAIN_TEXT_MAXSZ + WPI_FW_MAIN_DATA_MAXSZ +
WPI_FW_INIT_TEXT_MAXSZ + WPI_FW_INIT_DATA_MAXSZ +
WPI_FW_BOOT_TEXT_MAXSZ) {
- aprint_error_dev(sc->sc_dev, "invalid firmware file\n");
+ aprint_error_dev(sc->sc_dev, "Invalid firmware file %s\n",
+ fwname);
error = EFBIG;
goto fail1;
}
if (wpi_firmware_size < sizeof (struct wpi_firmware_hdr)) {
aprint_error_dev(sc->sc_dev,
- "truncated firmware header: %zu bytes\n",
- wpi_firmware_size);
+ "Truncated firmware header in %s: %zu bytes\n",
+ fwname, wpi_firmware_size);
error = EINVAL;
- goto fail2;
+ goto fail1;
}
wpi_firmware_image = firmware_malloc(wpi_firmware_size);
if (wpi_firmware_image == NULL) {
- aprint_error_dev(sc->sc_dev, "not enough memory to stock firmware\n");
+ aprint_error_dev(sc->sc_dev,
+ "Not enough memory for firmware %s\n", fwname);
error = ENOMEM;
goto fail1;
}
- if ((error = firmware_read(fw, 0, wpi_firmware_image, wpi_firmware_size)) != 0) {
- aprint_error_dev(sc->sc_dev, "can't get firmware\n");
- goto fail2;
+ error = firmware_read(fw, 0, wpi_firmware_image, wpi_firmware_size);
+ if (error != 0) {
+ aprint_error_dev(sc->sc_dev,
+ "Error reading firmware file %s\n", fwname);
+ goto fail1;
}
sc->fw_used = true;
@@ -1189,12 +1199,16 @@ wpi_cache_firmware(struct wpi_softc *sc)
return 0;
-fail2:
- firmware_free(wpi_firmware_image, wpi_firmware_size);
fail1:
firmware_close(fw);
- if (--wpi_firmware_users == 0)
+fail0:
+ --wpi_firmware_users;
+ KASSERT(wpi_firmware_users == 0);
+ if (wpi_firmware_image != NULL) {
firmware_free(wpi_firmware_image, wpi_firmware_size);
+ wpi_firmware_image = NULL;
+ wpi_firmware_size = 0;
+ }
mutex_exit(&wpi_firmware_mutex);
return error;
}
@@ -3092,7 +3106,8 @@ wpi_init(struct ifnet *ifp)
WPI_WRITE(sc, WPI_UCODE_CLR, WPI_RADIO_OFF);
if ((error = wpi_load_firmware(sc)) != 0) {
- aprint_error_dev(sc->sc_dev, "could not load firmware\n");
+ /* wpi_load_firmware already prints a message on error */
+ /*aprint_error_dev(sc->sc_dev, "could not load firmware\n");*/
goto fail1;
}
--
David A. Holland
dholland@netbsd.org
From: Alan Barrett <apb@cequrux.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Sun, 26 Dec 2010 22:39:54 +0200
On Sun, 26 Dec 2010, David Holland wrote:
> > # ifconfig wpi0 up
> > wpi0: ERROR: could not read firmware file
> > wpi0: ERROR: could not load firmware file
> > wpi0: cannot assign link-local address
> > wpi0: ERROR: could not read firmware file
> > wpi0: ERROR: could not load firmware file
> > wpi0: ERROR: could not read firmware file
> > wpi0: ERROR: could not load firmware file
> > wpi0: cannot assign link-local address
> > #
> >
> > There's no panic, which is good, but I think that the first error should
> > have been handled in such a way that the other errors did not occur.
>
> Yeah, but that will take some restructuring; as things are it's going
> to attempt wpi_cache_firmware every time it goes through wpi_init
> until it succeeds, and each successive call is going to print a
> message.
I guess I expected the first failure to cause an error to be returned to
userland. If I run "ifconfig wpi0 up" again, then it would be fine if
it printed another error message, but preferably only one each time.
> This patch should cover more of the bases. (But note that I don't have
> one of these, so I haven't tested it at all...)
Thank you; I'll try it tomorrow.
--apb (Alan Barrett)
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Sun, 26 Dec 2010 22:16:04 +0000
On Sun, Dec 26, 2010 at 08:45:02PM +0000, Alan Barrett wrote:
> > Yeah, but that will take some restructuring; as things are it's going
> > to attempt wpi_cache_firmware every time it goes through wpi_init
> > until it succeeds, and each successive call is going to print a
> > message.
>
> I guess I expected the first failure to cause an error to be returned to
> userland. If I run "ifconfig wpi0 up" again, then it would be fine if
> it printed another error message, but preferably only one each time.
Several of the internalcalls to wpi_init aren't checked for failure.
If you added those checks it might do what you want; it might also go
kablooey... I'm not overly familiar with network drivers in general
(let alone this one in particular) and I don't have a wpi myself, so I
don't really want to try tinkering blindly.
> > This patch should cover more of the bases. (But note that I don't have
> > one of these, so I haven't tested it at all...)
>
> Thank you; I'll try it tomorrow.
Let me know when/if I should commit...
--
David A. Holland
dholland@netbsd.org
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Mon, 30 May 2011 03:44:25 +0000
On Sun, Dec 26, 2010 at 10:20:04PM +0000, David Holland wrote:
> > Thank you; I'll try it tomorrow.
>
> Let me know when/if I should commit...
Slightly updated patch (the previous version now rejects):
Index: if_wpi.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_wpi.c,v
retrieving revision 1.49
diff -u -p -r1.49 if_wpi.c
--- if_wpi.c 2 Apr 2011 08:11:31 -0000 1.49
+++ if_wpi.c 30 May 2011 03:42:25 -0000
@@ -418,8 +418,12 @@ wpi_detach(device_t self, int flags __un
if (sc->fw_used) {
mutex_enter(&wpi_firmware_mutex);
- if (--wpi_firmware_users == 0)
+ if (--wpi_firmware_users == 0) {
+ KASSERT(wpi_firmware_image != NULL);
firmware_free(wpi_firmware_image, wpi_firmware_size);
+ wpi_firmware_image = NULL;
+ wpi_firmware_size = 0;
+ }
mutex_exit(&wpi_firmware_mutex);
}
@@ -1134,6 +1138,7 @@ wpi_load_microcode(struct wpi_softc *sc,
static int
wpi_cache_firmware(struct wpi_softc *sc)
{
+ static const char fwname[] = "iwlwifi-3945.ucode";
firmware_handle_t fw;
int error;
@@ -1147,9 +1152,10 @@ wpi_cache_firmware(struct wpi_softc *sc)
}
/* load firmware image from disk */
- if ((error = firmware_open("if_wpi","iwlwifi-3945.ucode", &fw)) != 0) {
- aprint_error_dev(sc->sc_dev, "could not read firmware file\n");
- goto fail1;
+ if ((error = firmware_open("if_wpi", fwname, &fw)) != 0) {
+ aprint_error_dev(sc->sc_dev,
+ "Could not open firmware file %s\n", fwname);
+ goto fail0;
}
wpi_firmware_size = firmware_get_size(fw);
@@ -1158,29 +1164,33 @@ wpi_cache_firmware(struct wpi_softc *sc)
WPI_FW_MAIN_TEXT_MAXSZ + WPI_FW_MAIN_DATA_MAXSZ +
WPI_FW_INIT_TEXT_MAXSZ + WPI_FW_INIT_DATA_MAXSZ +
WPI_FW_BOOT_TEXT_MAXSZ) {
- aprint_error_dev(sc->sc_dev, "invalid firmware file\n");
+ aprint_error_dev(sc->sc_dev, "Invalid firmware file %s\n",
+ fwname);
error = EFBIG;
goto fail1;
}
if (wpi_firmware_size < sizeof (struct wpi_firmware_hdr)) {
aprint_error_dev(sc->sc_dev,
- "truncated firmware header: %zu bytes\n",
- wpi_firmware_size);
+ "Truncated firmware header in %s: %zu bytes\n",
+ fwname, wpi_firmware_size);
error = EINVAL;
- goto fail2;
+ goto fail1;
}
wpi_firmware_image = firmware_malloc(wpi_firmware_size);
if (wpi_firmware_image == NULL) {
- aprint_error_dev(sc->sc_dev, "not enough memory to stock firmware\n");
+ aprint_error_dev(sc->sc_dev,
+ "Not enough memory for firmware %s\n", fwname);
error = ENOMEM;
goto fail1;
}
- if ((error = firmware_read(fw, 0, wpi_firmware_image, wpi_firmware_size)) != 0) {
- aprint_error_dev(sc->sc_dev, "can't get firmware\n");
- goto fail2;
+ error = firmware_read(fw, 0, wpi_firmware_image, wpi_firmware_size);
+ if (error != 0) {
+ aprint_error_dev(sc->sc_dev,
+ "Error reading firmware file %s\n", fwname);
+ goto fail1;
}
sc->fw_used = true;
@@ -1189,12 +1199,16 @@ wpi_cache_firmware(struct wpi_softc *sc)
return 0;
-fail2:
- firmware_free(wpi_firmware_image, wpi_firmware_size);
fail1:
firmware_close(fw);
- if (--wpi_firmware_users == 0)
+fail0:
+ --wpi_firmware_users;
+ KASSERT(wpi_firmware_users == 0);
+ if (wpi_firmware_image != NULL) {
firmware_free(wpi_firmware_image, wpi_firmware_size);
+ wpi_firmware_image = NULL;
+ wpi_firmware_size = 0;
+ }
mutex_exit(&wpi_firmware_mutex);
return error;
}
@@ -3092,7 +3106,8 @@ wpi_init(struct ifnet *ifp)
WPI_WRITE(sc, WPI_UCODE_CLR, WPI_RADIO_OFF);
if ((error = wpi_load_firmware(sc)) != 0) {
- aprint_error_dev(sc->sc_dev, "could not load firmware\n");
+ /* wpi_load_firmware already prints a message on error */
+ /*aprint_error_dev(sc->sc_dev, "could not load firmware\n");*/
goto fail1;
}
--
David A. Holland
dholland@netbsd.org
From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Wed, 22 Aug 2012 03:37:04 +0000
On Mon, May 30, 2011 at 03:45:02AM +0000, David Holland wrote:
>>> Thank you; I'll try it tomorrow.
>>
>> Let me know when/if I should commit...
>
> Slightly updated patch (the previous version now rejects):
Current version, now mostly cosmetic:
Index: if_wpi.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_wpi.c,v
retrieving revision 1.53
diff -u -p -r1.53 if_wpi.c
--- if_wpi.c 4 Aug 2012 03:52:46 -0000 1.53
+++ if_wpi.c 22 Aug 2012 03:33:17 -0000
@@ -417,8 +417,12 @@ wpi_detach(device_t self, int flags __un
if (sc->fw_used) {
mutex_enter(&wpi_firmware_mutex);
- if (--wpi_firmware_users == 0)
+ if (--wpi_firmware_users == 0) {
+ KASSERT(wpi_firmware_image != NULL);
firmware_free(wpi_firmware_image, wpi_firmware_size);
+ wpi_firmware_image = NULL;
+ wpi_firmware_size = 0;
+ }
mutex_exit(&wpi_firmware_mutex);
}
@@ -1133,6 +1137,7 @@ wpi_load_microcode(struct wpi_softc *sc,
static int
wpi_cache_firmware(struct wpi_softc *sc)
{
+ static const char fwname[] = "iwlwifi-3945.ucode";
firmware_handle_t fw;
int error;
@@ -1146,8 +1151,9 @@ wpi_cache_firmware(struct wpi_softc *sc)
}
/* load firmware image from disk */
- if ((error = firmware_open("if_wpi","iwlwifi-3945.ucode", &fw)) != 0) {
- aprint_error_dev(sc->sc_dev, "could not read firmware file\n");
+ if ((error = firmware_open("if_wpi", fwname, &fw)) != 0) {
+ aprint_error_dev(sc->sc_dev,
+ "Could not open firmware file %s\n", fwname);
goto fail0;
}
@@ -1157,29 +1163,33 @@ wpi_cache_firmware(struct wpi_softc *sc)
WPI_FW_MAIN_TEXT_MAXSZ + WPI_FW_MAIN_DATA_MAXSZ +
WPI_FW_INIT_TEXT_MAXSZ + WPI_FW_INIT_DATA_MAXSZ +
WPI_FW_BOOT_TEXT_MAXSZ) {
- aprint_error_dev(sc->sc_dev, "invalid firmware file\n");
+ aprint_error_dev(sc->sc_dev, "Invalid firmware file %s\n",
+ fwname);
error = EFBIG;
goto fail1;
}
if (wpi_firmware_size < sizeof (struct wpi_firmware_hdr)) {
aprint_error_dev(sc->sc_dev,
- "truncated firmware header: %zu bytes\n",
- wpi_firmware_size);
+ "Truncated firmware header in %s: %zu bytes\n",
+ fwname, wpi_firmware_size);
error = EINVAL;
- goto fail2;
+ goto fail1;
}
wpi_firmware_image = firmware_malloc(wpi_firmware_size);
if (wpi_firmware_image == NULL) {
- aprint_error_dev(sc->sc_dev, "not enough memory to stock firmware\n");
+ aprint_error_dev(sc->sc_dev,
+ "Not enough memory for firmware %s\n", fwname);
error = ENOMEM;
goto fail1;
}
- if ((error = firmware_read(fw, 0, wpi_firmware_image, wpi_firmware_size)) != 0) {
- aprint_error_dev(sc->sc_dev, "can't get firmware\n");
- goto fail2;
+ error = firmware_read(fw, 0, wpi_firmware_image, wpi_firmware_size);
+ if (error != 0) {
+ aprint_error_dev(sc->sc_dev,
+ "Error reading firmware file %s\n", fwname);
+ goto fail1;
}
sc->fw_used = true;
@@ -1188,13 +1198,16 @@ wpi_cache_firmware(struct wpi_softc *sc)
return 0;
-fail2:
- firmware_free(wpi_firmware_image, wpi_firmware_size);
fail1:
firmware_close(fw);
fail0:
wpi_firmware_users--;
KASSERT(wpi_firmware_users == 0);
+ if (wpi_firmware_image != NULL) {
+ firmware_free(wpi_firmware_image, wpi_firmware_size);
+ wpi_firmware_image = NULL;
+ wpi_firmware_size = 0;
+ }
mutex_exit(&wpi_firmware_mutex);
return error;
}
@@ -3092,7 +3105,8 @@ wpi_init(struct ifnet *ifp)
WPI_WRITE(sc, WPI_UCODE_CLR, WPI_RADIO_OFF);
if ((error = wpi_load_firmware(sc)) != 0) {
- aprint_error_dev(sc->sc_dev, "could not load firmware\n");
+ /* wpi_load_firmware already prints a message on error */
+ /*aprint_error_dev(sc->sc_dev, "could not load firmware\n");*/
goto fail1;
}
--
David A. Holland
dholland@netbsd.org
State-Changed-From-To: open->feedback
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Wed, 22 Aug 2012 03:43:38 +0000
State-Changed-Why:
riastradh committed a smaller but similar patch a couple weeks ago; please
give it a try.
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/44144 CVS commit: src/sys/dev/pci
Date: Sun, 25 Nov 2012 19:50:34 +0000
Module Name: src
Committed By: riastradh
Date: Sun Nov 25 19:50:34 UTC 2012
Modified Files:
src/sys/dev/pci: if_wpi.c
Log Message:
Rework firmware reference counting and error messages in wpi(4).
. Clarify the shared firmware abstraction in wpi_cached_firmware
and its new sibling wpi_release_firmware.
. Fix typo in wpa_cache_firmware error branch leading to free NULL.
. Fix leak in wpi_load_firmware error branch.
. Sprinkle some kasserts to executably document invariants.
. A little KNF here and there.
Based on a patch from dh in PR kern/44144.
To generate a diff of this commit:
cvs rdiff -u -r1.53 -r1.54 src/sys/dev/pci/if_wpi.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: "Jeff Rizzo" <riz@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/44144 CVS commit: [netbsd-6] src/sys/dev/pci
Date: Fri, 8 Feb 2013 19:37:38 +0000
Module Name: src
Committed By: riz
Date: Fri Feb 8 19:37:37 UTC 2013
Modified Files:
src/sys/dev/pci [netbsd-6]: if_wpi.c
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #784):
sys/dev/pci/if_wpi.c: revision 1.54
Rework firmware reference counting and error messages in wpi(4).
. Clarify the shared firmware abstraction in wpi_cached_firmware
and its new sibling wpi_release_firmware.
. Fix typo in wpa_cache_firmware error branch leading to free NULL.
. Fix leak in wpi_load_firmware error branch.
. Sprinkle some kasserts to executably document invariants.
. A little KNF here and there.
Based on a patch from dh in PR kern/44144.
To generate a diff of this commit:
cvs rdiff -u -r1.50.2.3 -r1.50.2.4 src/sys/dev/pci/if_wpi.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Alan Barrett <apb@cequrux.com>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/44144 (ifconfig causes assertion failure in vfs_lookup.c)
Date: Fri, 8 Feb 2013 22:43:02 +0200
On Mon, 30 May 2011, David Holland wrote:
> Slightly updated patch (the previous version now rejects):
I no longer have wpi hardware, so I can't test.
--apb (Alan Barrett)
State-Changed-From-To: feedback->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 07 Oct 2013 04:57:35 +0000
State-Changed-Why:
Submitter can't test.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home