On 06/10/2013 06:16 AM, Rafael J. Wysocki wrote:
> On Sunday, June 09, 2013 09:54:49 AM Aaron Lu wrote:
>> On 06/09/2013 09:19 AM, Aaron Lu wrote:
>>> On 06/09/2013 06:28 AM, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>>>
>>>> There is no particular reason why acpi_bus_driver_init() needs to be
>>>> a separate function and its location with respect to its only caller,
>>>> acpi_device_probe(), makes the code a bit difficult to follow.
>>>>
>>>> Besides, it doesn't really make sense to check if 'device' is not
>>>> NULL in acpi_bus_driver_init(), because we've already dereferenced
>>>> dev->driver in acpi_device_probe() at that point, so that check has
>>>> to be moved to acpi_device_probe() anyway.
>>>>
>>>> For these reasons, drop acpi_bus_driver_init() altogether and move
>>>> the code from it directly into acpi_device_probe().
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>>> ---
>>>>
>>>> Should apply on top of the bleeding-edge branch of the linux-pm.git tree.
>>>>
>>>> Thanks,
>>>> Rafael
>>>>
>>>> ---
>>>> drivers/acpi/scan.c | 88 +++++++++++++++++++---------------------------------
>>>> 1 file changed, 33 insertions(+), 55 deletions(-)
>>>>
>>>> Index: linux-pm/drivers/acpi/scan.c
>>>> ===================================================================
>>>> --- linux-pm.orig/drivers/acpi/scan.c
>>>> +++ linux-pm/drivers/acpi/scan.c
>>>> @@ -933,32 +933,45 @@ static void acpi_device_remove_notify_ha
>>>> acpi_device_notify);
>>>> }
>>>>
>>>> -static int acpi_bus_driver_init(struct acpi_device *, struct acpi_driver *);
>>>> static int acpi_device_probe(struct device * dev)
>>>> {
>>>> - struct acpi_device *acpi_dev = to_acpi_device(dev);
>>>> - struct acpi_driver *acpi_drv = to_acpi_driver(dev->driver);
>>>> + struct acpi_device *acpi_dev;
>>>> + struct acpi_driver *acpi_drv;
>>>> int ret;
>>>>
>>>> - ret = acpi_bus_driver_init(acpi_dev, acpi_drv);
>>>> - if (!ret) {
>>>> - if (acpi_drv->ops.notify) {
>>>> - ret = acpi_device_install_notify_handler(acpi_dev);
>>>> - if (ret) {
>>>> - if (acpi_drv->ops.remove)
>>>> - acpi_drv->ops.remove(acpi_dev);
>>>> - acpi_dev->driver = NULL;
>>>> - acpi_dev->driver_data = NULL;
>>>> - return ret;
>>>> - }
>>>> - }
>>>> + if (!dev || !dev->driver)
>>>> + return -EINVAL;
>>>
>>> Just out of curiosity, will dev ever be NULL in this function?
>>> This function is called in really_probe by dev->bus->probe after
>>> assigning dev->driver, so does the above check make any sense?
>
> Well, it makes sense as such, but it's not useful. :-)
>
>> BTW, I also tested the patch on a desktop and two laptops, no problems
>> found. Feel free to add my tested-by tag.
>
> I've modified the patch to remove that check and will post it again shortly.
> Can you please give the new version a run?
Actually, I added:
dev_info(dev, "%s: driver=%s\n", __func__, dev->driver->name);
before the if (!dev || !dev->driver) check while doing the tests to
verify my thoughts, so your new patch should also be fine on those
test systems, and my tested-by should still qualify.
It's national holiday here (6/10-6/12), but if you want to be sure, I
can do the tests on 6/13 when getting back to work.
Thanks,
Aaron
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
