* linux-next: dock_link_device is oopsy @ 2012-02-17 21:46 Hugh Dickins 2012-02-17 22:29 ` Holger Macht 0 siblings, 1 reply; 18+ messages in thread From: Hugh Dickins @ 2012-02-17 21:46 UTC (permalink / raw) To: Matthew Garrett; +Cc: Holger Macht, Jeff Garzik, Stephen Rothwell, linux-kernel Matthew, A linux-next oops at bootup in dock_link_device() tells me that you were not feeling well when you wrote that and dock_unlink_device(): I hope you're feeling better now and can rewrite them soon. I think my particular problem comes from dock_station_count 0, so kmalloc() returns NULL, which you don't check for. But those functions appear to be minor masterpieces of doing things in the wrong order, not checking allocation, not freeing resource on failure. I'm not attaching a proposed patch because, on that record, I bet when you look closer you'll find more wrong that I wouldn't know about. Let's hope this isn't diverted into an endless discussion of whether you ought to use kaalloc() or kballoc() or .... me, I'd just like to boot, and have put #if 0..#endif around everything in those functions. Hugh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-17 21:46 linux-next: dock_link_device is oopsy Hugh Dickins @ 2012-02-17 22:29 ` Holger Macht 2012-02-17 22:42 ` Hugh Dickins 0 siblings, 1 reply; 18+ messages in thread From: Holger Macht @ 2012-02-17 22:29 UTC (permalink / raw) To: Hugh Dickins Cc: Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Fr 17. Feb - 13:46:04, Hugh Dickins wrote: > Matthew, > > A linux-next oops at bootup in dock_link_device() tells me that you > were not feeling well when you wrote that and dock_unlink_device(): > I hope you're feeling better now and can rewrite them soon. Andrew Morton experienced a similar problem. What system are you using? I didn't encounter this problem with the systems I tested with. Do you actually have a /sys/devices/platform/dock.?/ directory with a file 'type' that contains 'dock_station'? > I think my particular problem comes from dock_station_count 0, > so kmalloc() returns NULL, which you don't check for. But those > functions appear to be minor masterpieces of doing things in the > wrong order, not checking allocation, not freeing resource on failure. > > I'm not attaching a proposed patch because, on that record, I bet when > you look closer you'll find more wrong that I wouldn't know about. > > Let's hope this isn't diverted into an endless discussion of whether > you ought to use kaalloc() or kballoc() or .... me, I'd just like to > boot, and have put #if 0..#endif around everything in those functions. Nevertheless, we should check for this error case or make sure we aren't able to run into it. Thanks, Holger ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-17 22:29 ` Holger Macht @ 2012-02-17 22:42 ` Hugh Dickins 2012-02-17 23:01 ` Holger Macht 0 siblings, 1 reply; 18+ messages in thread From: Hugh Dickins @ 2012-02-17 22:42 UTC (permalink / raw) To: Holger Macht Cc: Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Fri, 17 Feb 2012, Holger Macht wrote: > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote: > > Matthew, > > > > A linux-next oops at bootup in dock_link_device() tells me that you > > were not feeling well when you wrote that and dock_unlink_device(): > > I hope you're feeling better now and can rewrite them soon. > > Andrew Morton experienced a similar problem. What system are you using? > I didn't encounter this problem with the systems I tested with. The two systems I got that on were both 4-year-old Core2 Duo systems, one an HP quad desktop, one a Fujitsu-Siemens laptop. > > Do you actually have a /sys/devices/platform/dock.?/ directory with a > file 'type' that contains 'dock_station'? I'll have to report back on that this evening, I'm away from them now. Hugh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-17 22:42 ` Hugh Dickins @ 2012-02-17 23:01 ` Holger Macht 2012-02-17 23:49 ` Hugh Dickins 2012-02-18 7:52 ` Hugh Dickins 0 siblings, 2 replies; 18+ messages in thread From: Holger Macht @ 2012-02-17 23:01 UTC (permalink / raw) To: Hugh Dickins Cc: Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Fr 17. Feb - 14:42:31, Hugh Dickins wrote: > On Fri, 17 Feb 2012, Holger Macht wrote: > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote: > > > Matthew, > > > > > > A linux-next oops at bootup in dock_link_device() tells me that you > > > were not feeling well when you wrote that and dock_unlink_device(): > > > I hope you're feeling better now and can rewrite them soon. > > > > Andrew Morton experienced a similar problem. What system are you using? > > I didn't encounter this problem with the systems I tested with. > > The two systems I got that on were both 4-year-old Core2 Duo systems, > one an HP quad desktop, one a Fujitsu-Siemens laptop. Thanks for the information I think this is really independent from the fact if a laptop, or more precicely if a system with dock station/bay is used. > > > > > Do you actually have a /sys/devices/platform/dock.?/ directory with a > > file 'type' that contains 'dock_station'? > > I'll have to report back on that this evening, I'm away from them now. I actually guess that those systems don't have a /sys/devices/platform/dock.? directory at all, which is fine. I also think this will fix it, would be great if you could confirm this: acpi: Bail out when linking devices and there are no dock stations If dock_station_count is zero, we allocate zero memory and don't check this at future references. So bail out if there are actually no dock stations. Signed-off-by: Holger Macht <holger@homac.de> --- drivers/acpi/dock.c | 13 ++++++++++--- 1 files changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c index b5e4142..8641912 100644 --- a/drivers/acpi/dock.c +++ b/drivers/acpi/dock.c @@ -286,6 +286,9 @@ struct device **dock_link_device(acpi_handle handle) int ret, dock = 0; struct device **devices; + if (!dock_station_count) + return -ENODEV; + devices = kmalloc(dock_station_count * sizeof(struct device *), GFP_KERNEL); @@ -323,9 +326,13 @@ struct device **dock_unlink_device(acpi_handle handle) struct device *dev = acpi_get_physical_device(handle); struct dock_station *dock_station; int dock = 0; - struct device **devices = - kmalloc(dock_station_count * sizeof(struct device *), - GFP_KERNEL); + struct device **devices; + + if (!dock_station_count) + return -ENODEV; + + devices = kmalloc(dock_station_count * sizeof(struct device *), + GFP_KERNEL); if (!dev) return NULL; -- 1.7.7 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-17 23:01 ` Holger Macht @ 2012-02-17 23:49 ` Hugh Dickins 2012-02-18 11:14 ` Holger Macht 2012-02-18 7:52 ` Hugh Dickins 1 sibling, 1 reply; 18+ messages in thread From: Hugh Dickins @ 2012-02-17 23:49 UTC (permalink / raw) To: Holger Macht Cc: Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sat, 18 Feb 2012, Holger Macht wrote: > On Fr 17. Feb - 14:42:31, Hugh Dickins wrote: > > On Fri, 17 Feb 2012, Holger Macht wrote: > > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote: > > > > Matthew, > > > > > > > > A linux-next oops at bootup in dock_link_device() tells me that you > > > > were not feeling well when you wrote that and dock_unlink_device(): > > > > I hope you're feeling better now and can rewrite them soon. > > > > > > Andrew Morton experienced a similar problem. What system are you using? > > > I didn't encounter this problem with the systems I tested with. > > > > The two systems I got that on were both 4-year-old Core2 Duo systems, > > one an HP quad desktop, one a Fujitsu-Siemens laptop. > > Thanks for the information I think this is really independent from the > fact if a laptop, or more precicely if a system with dock station/bay is > used. > > > > > > > > > Do you actually have a /sys/devices/platform/dock.?/ directory with a > > > file 'type' that contains 'dock_station'? > > > > I'll have to report back on that this evening, I'm away from them now. > > I actually guess that those systems don't have a > /sys/devices/platform/dock.? directory at all, which is fine. > > I also think this will fix it, would be great if you could confirm this: > > acpi: Bail out when linking devices and there are no dock stations > > If dock_station_count is zero, we allocate zero memory and don't check > this at future references. So bail out if there are actually no dock > stations. > > Signed-off-by: Holger Macht <holger@homac.de> Certainly won't fix it as is (well, it shifts the crash over into kfree). This function is expected to return a pointer, not an error or success code. I've little doubt that returning NULL rather than -ENODEV there would fix the boot crash; and if you're in a hurry to fix up booting (understandable) then I suppose that would do for the moment. But it won't address the rest of the breakage and leakage in here, which I suspect will reveal more once Matthew has time to look. Hugh > --- > drivers/acpi/dock.c | 13 ++++++++++--- > 1 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c > index b5e4142..8641912 100644 > --- a/drivers/acpi/dock.c > +++ b/drivers/acpi/dock.c > @@ -286,6 +286,9 @@ struct device **dock_link_device(acpi_handle handle) > int ret, dock = 0; > struct device **devices; > > + if (!dock_station_count) > + return -ENODEV; > + > devices = kmalloc(dock_station_count * sizeof(struct device *), > GFP_KERNEL); > > @@ -323,9 +326,13 @@ struct device **dock_unlink_device(acpi_handle handle) > struct device *dev = acpi_get_physical_device(handle); > struct dock_station *dock_station; > int dock = 0; > - struct device **devices = > - kmalloc(dock_station_count * sizeof(struct device *), > - GFP_KERNEL); > + struct device **devices; > + > + if (!dock_station_count) > + return -ENODEV; > + > + devices = kmalloc(dock_station_count * sizeof(struct device *), > + GFP_KERNEL); > > if (!dev) > return NULL; > -- > 1.7.7 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-17 23:49 ` Hugh Dickins @ 2012-02-18 11:14 ` Holger Macht 2012-02-18 13:05 ` Hillf Danton 0 siblings, 1 reply; 18+ messages in thread From: Holger Macht @ 2012-02-18 11:14 UTC (permalink / raw) To: Hugh Dickins Cc: Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Fr 17. Feb - 15:49:02, Hugh Dickins wrote: > On Sat, 18 Feb 2012, Holger Macht wrote: > > On Fr 17. Feb - 14:42:31, Hugh Dickins wrote: > > > On Fri, 17 Feb 2012, Holger Macht wrote: > > > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote: > > > > > Matthew, > > > > > > > > > > A linux-next oops at bootup in dock_link_device() tells me that you > > > > > were not feeling well when you wrote that and dock_unlink_device(): > > > > > I hope you're feeling better now and can rewrite them soon. > > > > > > > > Andrew Morton experienced a similar problem. What system are you using? > > > > I didn't encounter this problem with the systems I tested with. > > > > > > The two systems I got that on were both 4-year-old Core2 Duo systems, > > > one an HP quad desktop, one a Fujitsu-Siemens laptop. > > > > Thanks for the information I think this is really independent from the > > fact if a laptop, or more precicely if a system with dock station/bay is > > used. > > > > > > > > > > > > > Do you actually have a /sys/devices/platform/dock.?/ directory with a > > > > file 'type' that contains 'dock_station'? > > > > > > I'll have to report back on that this evening, I'm away from them now. > > > > I actually guess that those systems don't have a > > /sys/devices/platform/dock.? directory at all, which is fine. > > > > I also think this will fix it, would be great if you could confirm this: > > > > acpi: Bail out when linking devices and there are no dock stations > > > > If dock_station_count is zero, we allocate zero memory and don't check > > this at future references. So bail out if there are actually no dock > > stations. > > > > Signed-off-by: Holger Macht <holger@homac.de> > > Certainly won't fix it as is (well, it shifts the crash over into kfree). > This function is expected to return a pointer, not an error or success > code. Oh well, too late for me yesterday... > > I've little doubt that returning NULL rather than -ENODEV there would fix > the boot crash; and if you're in a hurry to fix up booting (understandable) > then I suppose that would do for the moment. I really think this will basically do the trick. dock_(un)link_device() is called by ata_acpi_(un)bind_dock(), which in turn is called by ata_scsi_scan_host() unconditionally, thus not depending on the presence of a dock device. And dock_(un)link_device() simply misses the check if there is a dock/bay device. So how about that? acpi: Bail out when linking devices and there are no dock stations If dock_station_count is zero, we allocate zero memory and don't check this at future references. So bail out if there are actually no dock stations. Signed-off-by: Holger Macht <holger@homac.de> --- drivers/acpi/dock.c | 19 ++++++++++++++----- 1 files changed, 14 insertions(+), 5 deletions(-) diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c index b5e4142..0b3072c 100644 --- a/drivers/acpi/dock.c +++ b/drivers/acpi/dock.c @@ -281,11 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device); */ struct device **dock_link_device(acpi_handle handle) { - struct device *dev = acpi_get_physical_device(handle); + struct device *dev; struct dock_station *dock_station; int ret, dock = 0; struct device **devices; + if (!dock_station_count) + return NULL; + + dev = acpi_get_physical_device(handle); devices = kmalloc(dock_station_count * sizeof(struct device *), GFP_KERNEL); @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device); */ struct device **dock_unlink_device(acpi_handle handle) { - struct device *dev = acpi_get_physical_device(handle); + struct device *dev; struct dock_station *dock_station; int dock = 0; - struct device **devices = - kmalloc(dock_station_count * sizeof(struct device *), - GFP_KERNEL); + struct device **devices; + + if (!dock_station_count) + return NULL; + + dev = acpi_get_physical_device(handle); + devices = kmalloc(dock_station_count * sizeof(struct device *), + GFP_KERNEL); if (!dev) return NULL; -- 1.7.7 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 11:14 ` Holger Macht @ 2012-02-18 13:05 ` Hillf Danton 2012-02-18 13:26 ` Holger Macht 0 siblings, 1 reply; 18+ messages in thread From: Hillf Danton @ 2012-02-18 13:05 UTC (permalink / raw) To: Holger Macht Cc: Hugh Dickins, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sat, Feb 18, 2012 at 7:14 PM, Holger Macht <holger@homac.de> wrote: > On Fr 17. Feb - 15:49:02, Hugh Dickins wrote: >> On Sat, 18 Feb 2012, Holger Macht wrote: >> > On Fr 17. Feb - 14:42:31, Hugh Dickins wrote: >> > > On Fri, 17 Feb 2012, Holger Macht wrote: >> > > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote: >> > > > > Matthew, >> > > > > >> > > > > A linux-next oops at bootup in dock_link_device() tells me that you >> > > > > were not feeling well when you wrote that and dock_unlink_device(): >> > > > > I hope you're feeling better now and can rewrite them soon. >> > > > >> > > > Andrew Morton experienced a similar problem. What system are you using? >> > > > I didn't encounter this problem with the systems I tested with. >> > > >> > > The two systems I got that on were both 4-year-old Core2 Duo systems, >> > > one an HP quad desktop, one a Fujitsu-Siemens laptop. >> > >> > Thanks for the information I think this is really independent from the >> > fact if a laptop, or more precicely if a system with dock station/bay is >> > used. >> > >> > > >> > > > >> > > > Do you actually have a /sys/devices/platform/dock.?/ directory with a >> > > > file 'type' that contains 'dock_station'? >> > > >> > > I'll have to report back on that this evening, I'm away from them now. >> > >> > I actually guess that those systems don't have a >> > /sys/devices/platform/dock.? directory at all, which is fine. >> > >> > I also think this will fix it, would be great if you could confirm this: >> > >> > acpi: Bail out when linking devices and there are no dock stations >> > >> > If dock_station_count is zero, we allocate zero memory and don't check >> > this at future references. So bail out if there are actually no dock >> > stations. >> > >> > Signed-off-by: Holger Macht <holger@homac.de> >> >> Certainly won't fix it as is (well, it shifts the crash over into kfree). >> This function is expected to return a pointer, not an error or success >> code. > > Oh well, too late for me yesterday... > >> >> I've little doubt that returning NULL rather than -ENODEV there would fix >> the boot crash; and if you're in a hurry to fix up booting (understandable) >> then I suppose that would do for the moment. > > I really think this will basically do the trick. dock_(un)link_device() > is called by ata_acpi_(un)bind_dock(), which in turn is called by > ata_scsi_scan_host() unconditionally, thus not depending on the presence > of a dock device. And dock_(un)link_device() simply misses the check if > there is a dock/bay device. > > So how about that? > > acpi: Bail out when linking devices and there are no dock stations > > If dock_station_count is zero, we allocate zero memory and don't check > this at future references. So bail out if there are actually no dock > stations. > > Signed-off-by: Holger Macht <holger@homac.de> > --- > drivers/acpi/dock.c | 19 ++++++++++++++----- > 1 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c > index b5e4142..0b3072c 100644 > --- a/drivers/acpi/dock.c > +++ b/drivers/acpi/dock.c > @@ -281,11 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device); > */ > struct device **dock_link_device(acpi_handle handle) > { > - struct device *dev = acpi_get_physical_device(handle); > + struct device *dev; > struct dock_station *dock_station; > int ret, dock = 0; > struct device **devices; > > + if (!dock_station_count) > + return NULL; > + > + dev = acpi_get_physical_device(handle); > devices = kmalloc(dock_station_count * sizeof(struct device *), > GFP_KERNEL); > > @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device); > */ > struct device **dock_unlink_device(acpi_handle handle) > { > - struct device *dev = acpi_get_physical_device(handle); > + struct device *dev; > struct dock_station *dock_station; > int dock = 0; > - struct device **devices = > - kmalloc(dock_station_count * sizeof(struct device *), > - GFP_KERNEL); > + struct device **devices; > + > + if (!dock_station_count) > + return NULL; > + > + dev = acpi_get_physical_device(handle); > + devices = kmalloc(dock_station_count * sizeof(struct device *), > + GFP_KERNEL); > If bail out in this way, another patch looks needed to fix up mem leakage :-( > if (!dev) > return NULL; > -- > 1.7.7 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 13:05 ` Hillf Danton @ 2012-02-18 13:26 ` Holger Macht 2012-02-18 13:37 ` Hillf Danton 0 siblings, 1 reply; 18+ messages in thread From: Holger Macht @ 2012-02-18 13:26 UTC (permalink / raw) To: Hillf Danton Cc: Hugh Dickins, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sa 18. Feb - 21:05:18, Hillf Danton wrote: > On Sat, Feb 18, 2012 at 7:14 PM, Holger Macht <holger@homac.de> wrote: > > So how about that? > > > > acpi: Bail out when linking devices and there are no dock stations > > > > If dock_station_count is zero, we allocate zero memory and don't check > > this at future references. So bail out if there are actually no dock > > stations. > > > > Signed-off-by: Holger Macht <holger@homac.de> > > --- > > drivers/acpi/dock.c | 19 ++++++++++++++----- > > 1 files changed, 14 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c > > index b5e4142..0b3072c 100644 > > --- a/drivers/acpi/dock.c > > +++ b/drivers/acpi/dock.c > > @@ -281,11 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device); > > */ > > struct device **dock_link_device(acpi_handle handle) > > { > > - struct device *dev = acpi_get_physical_device(handle); > > + struct device *dev; > > struct dock_station *dock_station; > > int ret, dock = 0; > > struct device **devices; > > > > + if (!dock_station_count) > > + return NULL; > > + > > + dev = acpi_get_physical_device(handle); > > devices = kmalloc(dock_station_count * sizeof(struct device *), > > GFP_KERNEL); > > > > @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device); > > */ > > struct device **dock_unlink_device(acpi_handle handle) > > { > > - struct device *dev = acpi_get_physical_device(handle); > > + struct device *dev; > > struct dock_station *dock_station; > > int dock = 0; > > - struct device **devices = > > - kmalloc(dock_station_count * sizeof(struct device *), > > - GFP_KERNEL); > > + struct device **devices; > > + > > + if (!dock_station_count) > > + return NULL; > > + > > + dev = acpi_get_physical_device(handle); > > + devices = kmalloc(dock_station_count * sizeof(struct device *), > > + GFP_KERNEL); > > > > If bail out in this way, another patch looks needed to fix up > mem leakage :-( Sorry if I'm a little slow...but where is the leakage? The function doesn't allocate anything before bailing out in the dock_station_count check. And the rerun value should be freed by the caller. Please point me in the right direction. Thanks, Holger ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 13:26 ` Holger Macht @ 2012-02-18 13:37 ` Hillf Danton 2012-02-18 14:04 ` Holger Macht 0 siblings, 1 reply; 18+ messages in thread From: Hillf Danton @ 2012-02-18 13:37 UTC (permalink / raw) To: Holger Macht Cc: Hugh Dickins, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sat, Feb 18, 2012 at 9:26 PM, Holger Macht <holger@homac.de> wrote: > On Sa 18. Feb - 21:05:18, Hillf Danton wrote: >> On Sat, Feb 18, 2012 at 7:14 PM, Holger Macht <holger@homac.de> wrote: >> > So how about that? >> > >> > acpi: Bail out when linking devices and there are no dock stations >> > >> > If dock_station_count is zero, we allocate zero memory and don't check >> > this at future references. So bail out if there are actually no dock >> > stations. >> > >> > Signed-off-by: Holger Macht <holger@homac.de> >> > --- >> > drivers/acpi/dock.c | 19 ++++++++++++++----- >> > 1 files changed, 14 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c >> > index b5e4142..0b3072c 100644 >> > --- a/drivers/acpi/dock.c >> > +++ b/drivers/acpi/dock.c >> > @@ -281,11 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device); >> > */ >> > struct device **dock_link_device(acpi_handle handle) >> > { >> > - struct device *dev = acpi_get_physical_device(handle); >> > + struct device *dev; >> > struct dock_station *dock_station; >> > int ret, dock = 0; >> > struct device **devices; >> > >> > + if (!dock_station_count) >> > + return NULL; >> > + >> > + dev = acpi_get_physical_device(handle); >> > devices = kmalloc(dock_station_count * sizeof(struct device *), >> > GFP_KERNEL); >> > >> > @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device); >> > */ >> > struct device **dock_unlink_device(acpi_handle handle) >> > { >> > - struct device *dev = acpi_get_physical_device(handle); >> > + struct device *dev; >> > struct dock_station *dock_station; >> > int dock = 0; >> > - struct device **devices = >> > - kmalloc(dock_station_count * sizeof(struct device *), >> > - GFP_KERNEL); >> > + struct device **devices; >> > + >> > + if (!dock_station_count) >> > + return NULL; >> > + >> > + dev = acpi_get_physical_device(handle); >> > + devices = kmalloc(dock_station_count * sizeof(struct device *), >> > + GFP_KERNEL); >> > >> >> If bail out in this way, another patch looks needed to fix up >> mem leakage :-( > > Sorry if I'm a little slow...but where is the leakage? The function > doesn't allocate anything before bailing out in the dock_station_count > check. And the rerun value should be freed by the caller. Please point > me in the right direction. > Hi Holger Lets check the following snippet from what you posted, > @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device); > */ > struct device **dock_unlink_device(acpi_handle handle) > { > - struct device *dev = acpi_get_physical_device(handle); > + struct device *dev; > struct dock_station *dock_station; > int dock = 0; > - struct device **devices = > - kmalloc(dock_station_count * sizeof(struct device *), > - GFP_KERNEL); > + struct device **devices; > + > + if (!dock_station_count) > + return NULL; > + > + dev = acpi_get_physical_device(handle); > + devices = kmalloc(dock_station_count * sizeof(struct device *), > + GFP_KERNEL); > > if (!dev) > return NULL; <=== here return without freeing the newly allocated devices after checking dock_station_count is not zero > -- > 1.7.7 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 13:37 ` Hillf Danton @ 2012-02-18 14:04 ` Holger Macht 2012-02-18 14:35 ` Hillf Danton 2012-02-18 18:46 ` Hugh Dickins 0 siblings, 2 replies; 18+ messages in thread From: Holger Macht @ 2012-02-18 14:04 UTC (permalink / raw) To: Hillf Danton Cc: Hugh Dickins, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sa 18. Feb - 21:37:18, Hillf Danton wrote: > On Sat, Feb 18, 2012 at 9:26 PM, Holger Macht <holger@homac.de> wrote: > Hi Holger > > Lets check the following snippet from what you posted, > > > @@ -320,12 +324,17 @@ EXPORT_SYMBOL_GPL(dock_link_device); > > */ > > struct device **dock_unlink_device(acpi_handle handle) > > { > > - struct device *dev = acpi_get_physical_device(handle); > > + struct device *dev; > > struct dock_station *dock_station; > > int dock = 0; > > - struct device **devices = > > - kmalloc(dock_station_count * sizeof(struct device *), > > - GFP_KERNEL); > > + struct device **devices; > > + > > + if (!dock_station_count) > > + return NULL; > > + > > + dev = acpi_get_physical_device(handle); > > + devices = kmalloc(dock_station_count * sizeof(struct device *), > > + GFP_KERNEL); > > > > if (!dev) > > return NULL; <=== here return without freeing the newly > allocated devices after checking > dock_station_count is not zero How about that one? acpi: Bail out when linking devices and there are no dock stations If dock_station_count is zero, we allocate zero memory and don't check this at future references. So bail out if there are actually no dock stations. Fix some possible memory leaks on the way. Signed-off-by: Holger Macht <holger@homac.de> --- drivers/acpi/dock.c | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c index b5e4142..b2e8730 100644 --- a/drivers/acpi/dock.c +++ b/drivers/acpi/dock.c @@ -281,14 +281,15 @@ EXPORT_SYMBOL_GPL(is_dock_device); */ struct device **dock_link_device(acpi_handle handle) { - struct device *dev = acpi_get_physical_device(handle); + struct device *dev; struct dock_station *dock_station; int ret, dock = 0; struct device **devices; - devices = kmalloc(dock_station_count * sizeof(struct device *), - GFP_KERNEL); + if (!dock_station_count) + return NULL; + dev = acpi_get_physical_device(handle); if (!dev) return NULL; @@ -297,6 +298,9 @@ struct device **dock_link_device(acpi_handle handle) return NULL; } + devices = kmalloc(dock_station_count * sizeof(struct device *), + GFP_KERNEL); + list_for_each_entry(dock_station, &dock_stations, sibling) { if (find_dock_dependent_device(dock_station, handle)) { ret = sysfs_create_link(&dock_station->dock_device->dev.kobj, @@ -320,13 +324,15 @@ EXPORT_SYMBOL_GPL(dock_link_device); */ struct device **dock_unlink_device(acpi_handle handle) { - struct device *dev = acpi_get_physical_device(handle); + struct device *dev; struct dock_station *dock_station; int dock = 0; - struct device **devices = - kmalloc(dock_station_count * sizeof(struct device *), - GFP_KERNEL); + struct device **devices; + + if (!dock_station_count) + return NULL; + dev = acpi_get_physical_device(handle); if (!dev) return NULL; @@ -335,6 +341,9 @@ struct device **dock_unlink_device(acpi_handle handle) return NULL; } + devices = kmalloc(dock_station_count * sizeof(struct device *), + GFP_KERNEL); + list_for_each_entry(dock_station, &dock_stations, sibling) { if (find_dock_dependent_device(dock_station, handle)) { sysfs_remove_link(&dock_station->dock_device->dev.kobj, -- 1.7.7 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 14:04 ` Holger Macht @ 2012-02-18 14:35 ` Hillf Danton 2012-02-18 18:46 ` Hugh Dickins 1 sibling, 0 replies; 18+ messages in thread From: Hillf Danton @ 2012-02-18 14:35 UTC (permalink / raw) To: Holger Macht Cc: Hugh Dickins, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sat, Feb 18, 2012 at 10:04 PM, Holger Macht <holger@homac.de> wrote: > > How about that one? > Thanks for your prompt update :) Good weekend Hillf ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 14:04 ` Holger Macht 2012-02-18 14:35 ` Hillf Danton @ 2012-02-18 18:46 ` Hugh Dickins 2012-02-18 19:57 ` Holger Macht 1 sibling, 1 reply; 18+ messages in thread From: Hugh Dickins @ 2012-02-18 18:46 UTC (permalink / raw) To: Holger Macht Cc: Hillf Danton, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sat, 18 Feb 2012, Holger Macht wrote: > How about that one? It's more broken than that. Here's my attempt. It boots on the systems with dock_station_count 0, and it boots on my laptop with dock_station_count 2; but I don't actually have any docking station, so it still doesn't test very much (dock is 0 after the loop). I have no idea if what goes on in the loop is correct, but it looks to me as if (as predicted) there's further breakage, that it would have been writing beyond the end of what it allocated if I did have a docking station. Hugh [PATCH] dock: fix bootup oops and other dock_link breakage dock_link_device() and dock_unlink_device() should bail out early to avoid oops on zero-length kmalloc() when dock_station_count is 0. But isn't there an off-by-one in that kmalloc() length anyway? An extra NULL appended at the end suggests so. Rework the ordering with gotos on failure to fix several issues. And presumably dock_unlink_device() should be presenting the same interface as dock_link_device(), with NULL returned when none found. Signed-off-by: Hugh Dickins <hughd@google.com> --- drivers/acpi/dock.c | 69 +++++++++++++++++++++++++++++------------- 1 file changed, 49 insertions(+), 20 deletions(-) --- linux-next/drivers/acpi/dock.c 2012-02-17 08:02:12.280064984 -0800 +++ fixed/drivers/acpi/dock.c 2012-02-18 09:57:54.926244796 -0800 @@ -281,21 +281,25 @@ EXPORT_SYMBOL_GPL(is_dock_device); */ struct device **dock_link_device(acpi_handle handle) { - struct device *dev = acpi_get_physical_device(handle); + struct device *dev; struct dock_station *dock_station; int ret, dock = 0; struct device **devices; - devices = kmalloc(dock_station_count * sizeof(struct device *), - GFP_KERNEL); + if (!dock_station_count) + return NULL; - if (!dev) + if (is_dock(handle)) return NULL; - if (is_dock(handle)) { - put_device(dev); + dev = acpi_get_physical_device(handle); + if (!dev) return NULL; - } + + devices = kmalloc((dock_station_count + 1) * sizeof(struct device *), + GFP_KERNEL); + if (!devices) + goto put; list_for_each_entry(dock_station, &dock_stations, sibling) { if (find_dock_dependent_device(dock_station, handle)) { @@ -304,13 +308,23 @@ struct device **dock_link_device(acpi_ha WARN_ON(ret); devices[dock] = &dock_station->dock_device->dev; dock++; + if (dock == dock_station_count) + goto out; } } - if (!dock) - put_device(dev); + if (!dock) + goto free; +out: + /* Keep a reference to the device while the link exists */ devices[dock] = NULL; return devices; + +free: + kfree(devices); +put: + put_device(dev); + return NULL; } EXPORT_SYMBOL_GPL(dock_link_device); @@ -320,20 +334,25 @@ EXPORT_SYMBOL_GPL(dock_link_device); */ struct device **dock_unlink_device(acpi_handle handle) { - struct device *dev = acpi_get_physical_device(handle); + struct device *dev; struct dock_station *dock_station; int dock = 0; - struct device **devices = - kmalloc(dock_station_count * sizeof(struct device *), - GFP_KERNEL); + struct device **devices; - if (!dev) + if (!dock_station_count) return NULL; - if (is_dock(handle)) { - put_device(dev); + if (is_dock(handle)) return NULL; - } + + dev = acpi_get_physical_device(handle); + if (!dev) + return NULL; + + devices = kmalloc((dock_station_count + 1) * sizeof(struct device *), + GFP_KERNEL); + if (!devices) + goto put; list_for_each_entry(dock_station, &dock_stations, sibling) { if (find_dock_dependent_device(dock_station, handle)) { @@ -341,15 +360,25 @@ struct device **dock_unlink_device(acpi_ dev_name(dev)); devices[dock] = &dock_station->dock_device->dev; dock++; + if (dock == dock_station_count) + goto out; } } - /* An extra reference has been held while the link existed */ - if (dock) - put_device(dev); + if (!dock) + goto free; +out: + /* An extra reference has been held while the link existed */ + put_device(dev); put_device(dev); devices[dock] = NULL; return devices; + +free: + kfree(devices); +put: + put_device(dev); + return NULL; } EXPORT_SYMBOL_GPL(dock_unlink_device); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 18:46 ` Hugh Dickins @ 2012-02-18 19:57 ` Holger Macht 2012-02-18 21:03 ` Hugh Dickins 2012-02-21 22:24 ` Jeff Garzik 0 siblings, 2 replies; 18+ messages in thread From: Holger Macht @ 2012-02-18 19:57 UTC (permalink / raw) To: Hugh Dickins Cc: Hillf Danton, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sa 18. Feb - 10:46:04, Hugh Dickins wrote: > On Sat, 18 Feb 2012, Holger Macht wrote: > > How about that one? > > It's more broken than that. Here's my attempt. It boots on the > systems with dock_station_count 0, and it boots on my laptop with > dock_station_count 2; but I don't actually have any docking station, > so it still doesn't test very much (dock is 0 after the loop). Well, there doesn't have to actually exist a physical dock station (or bay device) for dock_station_count to be > 0. It just tells that the ACPI objects are present and thus the system is capable of it. So does this function actually also break on your laptop and you're getting the oops there, too? > I have no idea if what goes on in the loop is correct, but it looks > to me as if (as predicted) there's further breakage, that it would > have been writing beyond the end of what it allocated if I did have > a docking station. > > Hugh > > [PATCH] dock: fix bootup oops and other dock_link breakage > > dock_link_device() and dock_unlink_device() should bail out early > to avoid oops on zero-length kmalloc() when dock_station_count is 0. > > But isn't there an off-by-one in that kmalloc() length anyway? > An extra NULL appended at the end suggests so. > > Rework the ordering with gotos on failure to fix several issues. > > And presumably dock_unlink_device() should be presenting the same > interface as dock_link_device(), with NULL returned when none found. > > Signed-off-by: Hugh Dickins <hughd@google.com> Fine with me. Thanks, Holger ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 19:57 ` Holger Macht @ 2012-02-18 21:03 ` Hugh Dickins 2012-02-18 21:50 ` Holger Macht 2012-02-21 22:24 ` Jeff Garzik 1 sibling, 1 reply; 18+ messages in thread From: Hugh Dickins @ 2012-02-18 21:03 UTC (permalink / raw) To: Holger Macht Cc: Hillf Danton, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sat, 18 Feb 2012, Holger Macht wrote: > On Sa 18. Feb - 10:46:04, Hugh Dickins wrote: > > On Sat, 18 Feb 2012, Holger Macht wrote: > > > How about that one? > > > > It's more broken than that. Here's my attempt. It boots on the > > systems with dock_station_count 0, and it boots on my laptop with > > dock_station_count 2; but I don't actually have any docking station, > > so it still doesn't test very much (dock is 0 after the loop). > > Well, there doesn't have to actually exist a physical dock station (or > bay device) for dock_station_count to be > 0. It just tells that the > ACPI objects are present and thus the system is capable of it. > > So does this function actually also break on your laptop and you're > getting the oops there, too? It oopsed on the 4-year-old Fujitsu-Siemens laptop whose dock_station_count was 0. It did not oops on the new ThinkPad laptop whose dock_station_count is 2, but no docks were found: so the function would only have been leaking memory on that. If docks were found, then I suspect it could have been scribbling, but I cannot actually check if that's true (for all I know, dock_station_count may be always 1 bigger than the most that that double loop can discover); but at least the loop is now made safe against scribbling. Hugh ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 21:03 ` Hugh Dickins @ 2012-02-18 21:50 ` Holger Macht 0 siblings, 0 replies; 18+ messages in thread From: Holger Macht @ 2012-02-18 21:50 UTC (permalink / raw) To: Hugh Dickins Cc: Hillf Danton, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sa 18. Feb - 13:03:34, Hugh Dickins wrote: > On Sat, 18 Feb 2012, Holger Macht wrote: > > On Sa 18. Feb - 10:46:04, Hugh Dickins wrote: > > > On Sat, 18 Feb 2012, Holger Macht wrote: > > > > How about that one? > > > > > > It's more broken than that. Here's my attempt. It boots on the > > > systems with dock_station_count 0, and it boots on my laptop with > > > dock_station_count 2; but I don't actually have any docking station, > > > so it still doesn't test very much (dock is 0 after the loop). > > > > Well, there doesn't have to actually exist a physical dock station (or > > bay device) for dock_station_count to be > 0. It just tells that the > > ACPI objects are present and thus the system is capable of it. > > > > So does this function actually also break on your laptop and you're > > getting the oops there, too? > > It oopsed on the 4-year-old Fujitsu-Siemens laptop whose dock_station_count > was 0. It did not oops on the new ThinkPad laptop whose dock_station_count > is 2, but no docks were found: so the function would only have been leaking > memory on that. It should actually have successfully linked the dock to the device, if for instance you had a device in your bay. At least this is working fine with two Thinkpads I tested with. That's why I didn't encounter this problem at all before resubmitting the corresponding patch set. > If docks were found, then I suspect it could have been scribbling, but I > cannot actually check if that's true (for all I know, dock_station_count > may be always 1 bigger than the most that that double loop can discover); > but at least the loop is now made safe against scribbling. It was actually working fine multiple times, out of pure luck it seems. Regards, Holger ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-18 19:57 ` Holger Macht 2012-02-18 21:03 ` Hugh Dickins @ 2012-02-21 22:24 ` Jeff Garzik 2012-02-21 22:30 ` Holger Macht 1 sibling, 1 reply; 18+ messages in thread From: Jeff Garzik @ 2012-02-21 22:24 UTC (permalink / raw) To: Holger Macht Cc: Hugh Dickins, Hillf Danton, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On 02/18/2012 02:57 PM, Holger Macht wrote: > On Sa 18. Feb - 10:46:04, Hugh Dickins wrote: >> On Sat, 18 Feb 2012, Holger Macht wrote: >>> How about that one? >> >> It's more broken than that. Here's my attempt. It boots on the >> systems with dock_station_count 0, and it boots on my laptop with >> dock_station_count 2; but I don't actually have any docking station, >> so it still doesn't test very much (dock is 0 after the loop). > > Well, there doesn't have to actually exist a physical dock station (or > bay device) for dock_station_count to be> 0. It just tells that the > ACPI objects are present and thus the system is capable of it. > > So does this function actually also break on your laptop and you're > getting the oops there, too? > >> I have no idea if what goes on in the loop is correct, but it looks >> to me as if (as predicted) there's further breakage, that it would >> have been writing beyond the end of what it allocated if I did have >> a docking station. >> >> Hugh >> >> [PATCH] dock: fix bootup oops and other dock_link breakage >> >> dock_link_device() and dock_unlink_device() should bail out early >> to avoid oops on zero-length kmalloc() when dock_station_count is 0. >> >> But isn't there an off-by-one in that kmalloc() length anyway? >> An extra NULL appended at the end suggests so. >> >> Rework the ordering with gotos on failure to fix several issues. >> >> And presumably dock_unlink_device() should be presenting the same >> interface as dock_link_device(), with NULL returned when none found. >> >> Signed-off-by: Hugh Dickins<hughd@google.com> > > Fine with me. So, just to be clear, the preferred patch is Hugh's, and I should drop your earlier proposed fix found in this thread? And what about that warning? Need to fix up linux-next or temporarily drop this patchset from linux-next. Jeff ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-21 22:24 ` Jeff Garzik @ 2012-02-21 22:30 ` Holger Macht 0 siblings, 0 replies; 18+ messages in thread From: Holger Macht @ 2012-02-21 22:30 UTC (permalink / raw) To: Jeff Garzik Cc: Hugh Dickins, Hillf Danton, Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Tue 21. Feb - 17:24:24, Jeff Garzik wrote: > On 02/18/2012 02:57 PM, Holger Macht wrote: > >On Sa 18. Feb - 10:46:04, Hugh Dickins wrote: > >>On Sat, 18 Feb 2012, Holger Macht wrote: > >>>How about that one? > >> > >>It's more broken than that. Here's my attempt. It boots on the > >>systems with dock_station_count 0, and it boots on my laptop with > >>dock_station_count 2; but I don't actually have any docking station, > >>so it still doesn't test very much (dock is 0 after the loop). > > > >Well, there doesn't have to actually exist a physical dock station (or > >bay device) for dock_station_count to be> 0. It just tells that the > >ACPI objects are present and thus the system is capable of it. > > > >So does this function actually also break on your laptop and you're > >getting the oops there, too? > > > >>I have no idea if what goes on in the loop is correct, but it looks > >>to me as if (as predicted) there's further breakage, that it would > >>have been writing beyond the end of what it allocated if I did have > >>a docking station. > >> > >>Hugh > >> > >>[PATCH] dock: fix bootup oops and other dock_link breakage > >> > >>dock_link_device() and dock_unlink_device() should bail out early > >>to avoid oops on zero-length kmalloc() when dock_station_count is 0. > >> > >>But isn't there an off-by-one in that kmalloc() length anyway? > >>An extra NULL appended at the end suggests so. > >> > >>Rework the ordering with gotos on failure to fix several issues. > >> > >>And presumably dock_unlink_device() should be presenting the same > >>interface as dock_link_device(), with NULL returned when none found. > >> > >>Signed-off-by: Hugh Dickins<hughd@google.com> > > > >Fine with me. > > So, just to be clear, the preferred patch is Hugh's, and I should > drop your earlier proposed fix found in this thread? Correct. > And what about that warning? You mean the fix for the compile error when compiling with CONFIG_ACPI_DOCK=n? Here it is again: [PATCH] acpi: Fix compiler error when setting CONFIG_ACPI_DOCK=n When compiling with CONFIG_ACPI_DOCK=n, is_registered_hotplug_dock_device() needs to be defined Signed-off-by: Holger Macht <holger@homac.de> --- include/acpi/acpi_drivers.h | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h index 3c4e381..3319574 100644 --- a/include/acpi/acpi_drivers.h +++ b/include/acpi/acpi_drivers.h @@ -155,6 +155,10 @@ static inline int register_hotplug_dock_device(acpi_handle handle, static inline void unregister_hotplug_dock_device(acpi_handle handle) { } +static inline int is_registered_hotplug_dock_device(const struct acpi_dock_ops *ops) +{ + return 0; +} static inline struct device **dock_link_device(acpi_handle handle) { return NULL; -- 1.7.7 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: linux-next: dock_link_device is oopsy 2012-02-17 23:01 ` Holger Macht 2012-02-17 23:49 ` Hugh Dickins @ 2012-02-18 7:52 ` Hugh Dickins 1 sibling, 0 replies; 18+ messages in thread From: Hugh Dickins @ 2012-02-18 7:52 UTC (permalink / raw) To: Holger Macht Cc: Matthew Garrett, Jeff Garzik, Stephen Rothwell, linux-kernel, Andrew Morton On Sat, 18 Feb 2012, Holger Macht wrote: > On Fr 17. Feb - 14:42:31, Hugh Dickins wrote: > > On Fri, 17 Feb 2012, Holger Macht wrote: > > > On Fr 17. Feb - 13:46:04, Hugh Dickins wrote: > > > > Matthew, > > > > > > > > A linux-next oops at bootup in dock_link_device() tells me that you > > > > were not feeling well when you wrote that and dock_unlink_device(): > > > > I hope you're feeling better now and can rewrite them soon. > > > > > > Andrew Morton experienced a similar problem. What system are you using? > > > I didn't encounter this problem with the systems I tested with. > > > > The two systems I got that on were both 4-year-old Core2 Duo systems, > > one an HP quad desktop, one a Fujitsu-Siemens laptop. > > Thanks for the information I think this is really independent from the > fact if a laptop, or more precicely if a system with dock station/bay is > used. > > > > > > > > > Do you actually have a /sys/devices/platform/dock.?/ directory with a > > > file 'type' that contains 'dock_station'? > > > > I'll have to report back on that this evening, I'm away from them now. > > I actually guess that those systems don't have a > /sys/devices/platform/dock.? directory at all, which is fine. You are correct, no /sys/devices/platform/dock.? directory on those. > > I also think this will fix it, would be great if you could confirm this: > > acpi: Bail out when linking devices and there are no dock stations > > If dock_station_count is zero, we allocate zero memory and don't check > this at future references. So bail out if there are actually no dock > stations. > > Signed-off-by: Holger Macht <holger@homac.de> > --- > drivers/acpi/dock.c | 13 ++++++++++--- > 1 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c > index b5e4142..8641912 100644 > --- a/drivers/acpi/dock.c > +++ b/drivers/acpi/dock.c > @@ -286,6 +286,9 @@ struct device **dock_link_device(acpi_handle handle) > int ret, dock = 0; > struct device **devices; > > + if (!dock_station_count) > + return -ENODEV; If I change your -ENODEV to NULL (here and in unlink, though I didn't check if that gets called), yes, that "fixes" the crash at boot. But note that in each case you already did an acpi_get_physical_device: if the existing code was correct in this regard, then you ought to be doing a put_device(dev) before returning. Or better reorder it all. Hugh > + > devices = kmalloc(dock_station_count * sizeof(struct device *), > GFP_KERNEL); > > @@ -323,9 +326,13 @@ struct device **dock_unlink_device(acpi_handle handle) > struct device *dev = acpi_get_physical_device(handle); > struct dock_station *dock_station; > int dock = 0; > - struct device **devices = > - kmalloc(dock_station_count * sizeof(struct device *), > - GFP_KERNEL); > + struct device **devices; > + > + if (!dock_station_count) > + return -ENODEV; > + > + devices = kmalloc(dock_station_count * sizeof(struct device *), > + GFP_KERNEL); > > if (!dev) > return NULL; > -- > 1.7.7 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2012-02-21 22:30 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-17 21:46 linux-next: dock_link_device is oopsy Hugh Dickins 2012-02-17 22:29 ` Holger Macht 2012-02-17 22:42 ` Hugh Dickins 2012-02-17 23:01 ` Holger Macht 2012-02-17 23:49 ` Hugh Dickins 2012-02-18 11:14 ` Holger Macht 2012-02-18 13:05 ` Hillf Danton 2012-02-18 13:26 ` Holger Macht 2012-02-18 13:37 ` Hillf Danton 2012-02-18 14:04 ` Holger Macht 2012-02-18 14:35 ` Hillf Danton 2012-02-18 18:46 ` Hugh Dickins 2012-02-18 19:57 ` Holger Macht 2012-02-18 21:03 ` Hugh Dickins 2012-02-18 21:50 ` Holger Macht 2012-02-21 22:24 ` Jeff Garzik 2012-02-21 22:30 ` Holger Macht 2012-02-18 7:52 ` Hugh Dickins
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox