Before fe06fba292af5ed5c1c6ad9af3a9ef68da7a5088 the dock_station struct
was allocated and zero filled by kzalloc. Now it is allocated by
platform_device_register_data(), apparently via a kmemdup of a variable
on the stack?
Now we no longer init dock_station->flags to zero, and my x60 laptop was
lucky enough to always have DOCK_DOCKING set. So, I always saw -EBUSY
when I tried to undock.
With the patch below, I can dock and undock properly again. I double
checked that dock_station->flags was the only field that we were not
setting up.
This was just brute force guessing, I have no idea what this code does.
Signed-off-by: Chris Mason <[email protected]>
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index bbc2c13..0a449ad 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -944,6 +945,7 @@ static int dock_add(acpi_handle handle)
dock_station->handle = handle;
dock_station->dock_device = dd;
dock_station->last_dock_time = jiffies - HZ;
+ dock_station->flags = 0;
mutex_init(&dock_station->hp_lock);
spin_lock_init(&dock_station->dd_lock);
Hi Chris,
Thanks for catching this. Seems better to just memset the stack
variable before we kmemdup it in platform_deivce_register_data().
How about this instead?
---
From: Alex Chiang <[email protected]>
ACPI: dock: properly initialize local struct dock_station in dock_add()
Commit fe06fba2 (ACPI: dock: add struct dock_station * directly
to platform device data) changed dock_add() to use the
platform_device_register_data() API.
We passed that interface a stack variable, which is kmemdup'ed
and assigned to the device's platform_data pointer.
Unfortunately, whatever random garbage is in the stack variable
gets coped during the kmemdup, and that leads to broken behavior.
Explicitly zero out the structure before passing it to the API.
Cc: [email protected]
Reported-by: Chris Mason <[email protected]>
Signed-off-by: Alex Chiang <[email protected]>
---
Commit fe06fba2 was introduced in 2.6.32-rc5, so we need this fix
for the .32 stable series only.
---
diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
index bbc2c13..b2586f5 100644
--- a/drivers/acpi/dock.c
+++ b/drivers/acpi/dock.c
@@ -935,6 +935,7 @@ static int dock_add(acpi_handle handle)
struct platform_device *dd;
id = dock_station_count;
+ memset(&ds, 0, sizeof(ds));
dd = platform_device_register_data(NULL, "dock", id, &ds, sizeof(ds));
if (IS_ERR(dd))
return PTR_ERR(dd);
On Mon, Feb 01, 2010 at 10:35:18AM -0700, Alex Chiang wrote:
> Hi Chris,
>
> Thanks for catching this. Seems better to just memset the stack
> variable before we kmemdup it in platform_deivce_register_data().
>
> How about this instead?
[ memset instead of explicitly setting flags to zero ]
I didn't do the memset because lower down we init every other field, but
zeros are zeros really.
Looking at platform_device_register_data, you could even make ds
static...now that is sure to break in small and subtle ways later on ;)
-chris
* Chris Mason <[email protected]>:
> On Mon, Feb 01, 2010 at 10:35:18AM -0700, Alex Chiang wrote:
> > Hi Chris,
> >
> > Thanks for catching this. Seems better to just memset the stack
> > variable before we kmemdup it in platform_deivce_register_data().
> >
> > How about this instead?
>
> [ memset instead of explicitly setting flags to zero ]
>
> I didn't do the memset because lower down we init every other field, but
> zeros are zeros really.
Sure, zeros are zeros. I chose memset in case the struct grows in
the future and someone else forgets to initialize members to
zero.
I'll let Len decide which patch to take, it doesn't much matter
to me.
If he takes yours...
Acked-by: Alex Chiang <[email protected]>
> Looking at platform_device_register_data, you could even make
> ds static...now that is sure to break in small and subtle ways
> later on ;)
Ha, those are bugs that I don't have to have to fix in the
future. :)
cheers,
/ac
On Mon, Feb 01, 2010 at 10:35:18AM -0700, Alex Chiang wrote:
> Hi Chris,
>
> Thanks for catching this. Seems better to just memset the stack
> variable before we kmemdup it in platform_deivce_register_data().
>
I don't see either patch in rc8, I think we want to do something for
docking in 2.6.33...Len are you interested in picking up either of these
fixes?
[ full quote below ]
-chris
> How about this instead?
>
> ---
> From: Alex Chiang <[email protected]>
>
> ACPI: dock: properly initialize local struct dock_station in dock_add()
>
> Commit fe06fba2 (ACPI: dock: add struct dock_station * directly
> to platform device data) changed dock_add() to use the
> platform_device_register_data() API.
>
> We passed that interface a stack variable, which is kmemdup'ed
> and assigned to the device's platform_data pointer.
>
> Unfortunately, whatever random garbage is in the stack variable
> gets coped during the kmemdup, and that leads to broken behavior.
>
> Explicitly zero out the structure before passing it to the API.
>
> Cc: [email protected]
> Reported-by: Chris Mason <[email protected]>
> Signed-off-by: Alex Chiang <[email protected]>
> ---
> Commit fe06fba2 was introduced in 2.6.32-rc5, so we need this fix
> for the .32 stable series only.
>
> ---
> diff --git a/drivers/acpi/dock.c b/drivers/acpi/dock.c
> index bbc2c13..b2586f5 100644
> --- a/drivers/acpi/dock.c
> +++ b/drivers/acpi/dock.c
> @@ -935,6 +935,7 @@ static int dock_add(acpi_handle handle)
> struct platform_device *dd;
>
> id = dock_station_count;
> + memset(&ds, 0, sizeof(ds));
> dd = platform_device_register_data(NULL, "dock", id, &ds, sizeof(ds));
> if (IS_ERR(dd))
> return PTR_ERR(dd);
applied.
will apply to 2.6.32.stable too
thanks,
Len Brown, Intel Open Source Technology Center