From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
+
+ acpi_dev = to_acpi_device(dev);
+ acpi_drv = to_acpi_driver(dev->driver);
+ if (!acpi_drv->ops.add)
+ return -ENOSYS;
+
+ ret = acpi_drv->ops.add(acpi_dev);
+ if (ret)
+ return ret;
+
+ acpi_dev->driver = acpi_drv;
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "Driver [%s] successfully bound to device [%s]\n",
+ acpi_drv->name, acpi_dev->pnp.bus_id));
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Found driver [%s] for device [%s]\n",
- acpi_drv->name, acpi_dev->pnp.bus_id));
- get_device(dev);
+ 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;
+ }
}
- return ret;
+
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found driver [%s] for device [%s]\n",
+ acpi_drv->name, acpi_dev->pnp.bus_id));
+ get_device(dev);
+ return 0;
}
static int acpi_device_remove(struct device * dev)
@@ -1114,41 +1127,6 @@ static void acpi_device_unregister(struc
Driver Management
-------------------------------------------------------------------------- */
/**
- * acpi_bus_driver_init - add a device to a driver
- * @device: the device to add and initialize
- * @driver: driver for the device
- *
- * Used to initialize a device via its device driver. Called whenever a
- * driver is bound to a device. Invokes the driver's add() ops.
- */
-static int
-acpi_bus_driver_init(struct acpi_device *device, struct acpi_driver *driver)
-{
- int result = 0;
-
- if (!device || !driver)
- return -EINVAL;
-
- if (!driver->ops.add)
- return -ENOSYS;
-
- result = driver->ops.add(device);
- if (result)
- return result;
-
- device->driver = driver;
-
- /*
- * TBD - Configuration Management: Assign resources to device based
- * upon possible configuration and currently allocated resources.
- */
-
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Driver successfully bound to device\n"));
- return 0;
-}
-
-/**
* acpi_bus_register_driver - register a driver with the ACPI bus
* @driver: driver being registered
*
On 06/09/2013 06:28 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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?
Thanks,
Aaron
> +
> + acpi_dev = to_acpi_device(dev);
> + acpi_drv = to_acpi_driver(dev->driver);
> + if (!acpi_drv->ops.add)
> + return -ENOSYS;
> +
> + ret = acpi_drv->ops.add(acpi_dev);
> + if (ret)
> + return ret;
> +
> + acpi_dev->driver = acpi_drv;
> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> + "Driver [%s] successfully bound to device [%s]\n",
> + acpi_drv->name, acpi_dev->pnp.bus_id));
>
> - ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> - "Found driver [%s] for device [%s]\n",
> - acpi_drv->name, acpi_dev->pnp.bus_id));
> - get_device(dev);
> + 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;
> + }
> }
> - return ret;
> +
> + ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found driver [%s] for device [%s]\n",
> + acpi_drv->name, acpi_dev->pnp.bus_id));
> + get_device(dev);
> + return 0;
> }
>
> static int acpi_device_remove(struct device * dev)
> @@ -1114,41 +1127,6 @@ static void acpi_device_unregister(struc
> Driver Management
> -------------------------------------------------------------------------- */
> /**
> - * acpi_bus_driver_init - add a device to a driver
> - * @device: the device to add and initialize
> - * @driver: driver for the device
> - *
> - * Used to initialize a device via its device driver. Called whenever a
> - * driver is bound to a device. Invokes the driver's add() ops.
> - */
> -static int
> -acpi_bus_driver_init(struct acpi_device *device, struct acpi_driver *driver)
> -{
> - int result = 0;
> -
> - if (!device || !driver)
> - return -EINVAL;
> -
> - if (!driver->ops.add)
> - return -ENOSYS;
> -
> - result = driver->ops.add(device);
> - if (result)
> - return result;
> -
> - device->driver = driver;
> -
> - /*
> - * TBD - Configuration Management: Assign resources to device based
> - * upon possible configuration and currently allocated resources.
> - */
> -
> - ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> - "Driver successfully bound to device\n"));
> - return 0;
> -}
> -
> -/**
> * acpi_bus_register_driver - register a driver with the ACPI bus
> * @driver: driver being registered
> *
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>
>> 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?
BTW, I also tested the patch on a desktop and two laptops, no problems
found. Feel free to add my tested-by tag.
Thanks,
Aaron
>
> Thanks,
> Aaron
>
>> +
>> + acpi_dev = to_acpi_device(dev);
>> + acpi_drv = to_acpi_driver(dev->driver);
>> + if (!acpi_drv->ops.add)
>> + return -ENOSYS;
>> +
>> + ret = acpi_drv->ops.add(acpi_dev);
>> + if (ret)
>> + return ret;
>> +
>> + acpi_dev->driver = acpi_drv;
>> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> + "Driver [%s] successfully bound to device [%s]\n",
>> + acpi_drv->name, acpi_dev->pnp.bus_id));
>>
>> - ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> - "Found driver [%s] for device [%s]\n",
>> - acpi_drv->name, acpi_dev->pnp.bus_id));
>> - get_device(dev);
>> + 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;
>> + }
>> }
>> - return ret;
>> +
>> + ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found driver [%s] for device [%s]\n",
>> + acpi_drv->name, acpi_dev->pnp.bus_id));
>> + get_device(dev);
>> + return 0;
>> }
>>
>> static int acpi_device_remove(struct device * dev)
>> @@ -1114,41 +1127,6 @@ static void acpi_device_unregister(struc
>> Driver Management
>> -------------------------------------------------------------------------- */
>> /**
>> - * acpi_bus_driver_init - add a device to a driver
>> - * @device: the device to add and initialize
>> - * @driver: driver for the device
>> - *
>> - * Used to initialize a device via its device driver. Called whenever a
>> - * driver is bound to a device. Invokes the driver's add() ops.
>> - */
>> -static int
>> -acpi_bus_driver_init(struct acpi_device *device, struct acpi_driver *driver)
>> -{
>> - int result = 0;
>> -
>> - if (!device || !driver)
>> - return -EINVAL;
>> -
>> - if (!driver->ops.add)
>> - return -ENOSYS;
>> -
>> - result = driver->ops.add(device);
>> - if (result)
>> - return result;
>> -
>> - device->driver = driver;
>> -
>> - /*
>> - * TBD - Configuration Management: Assign resources to device based
>> - * upon possible configuration and currently allocated resources.
>> - */
>> -
>> - ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> - "Driver successfully bound to device\n"));
>> - return 0;
>> -}
>> -
>> -/**
>> * acpi_bus_register_driver - register a driver with the ACPI bus
>> * @driver: driver being registered
>> *
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >>
> >> 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?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
From: Rafael J. Wysocki <[email protected]>
Subject: ACPI / scan: Simplify ACPI driver probing
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 and, moreover,
'device' cannot be NULL then, because acpi_device_probe() is called
via really_probe() (which also sets dev->driver for that matter).
For these reasons, drop acpi_bus_driver_init() altogether and move
the remaining code from it directly into acpi_device_probe().
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
It's in the bleeding-edge branch of the linux-pm.git tree already.
Thanks,
Rafael
---
drivers/acpi/scan.c | 81 +++++++++++++++++-----------------------------------
1 file changed, 27 insertions(+), 54 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -933,32 +933,40 @@ 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)
+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);
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 (!acpi_drv->ops.add)
+ return -ENOSYS;
+
+ ret = acpi_drv->ops.add(acpi_dev);
+ if (ret)
+ return ret;
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Found driver [%s] for device [%s]\n",
- acpi_drv->name, acpi_dev->pnp.bus_id));
- get_device(dev);
+ acpi_dev->driver = acpi_drv;
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "Driver [%s] successfully bound to device [%s]\n",
+ acpi_drv->name, acpi_dev->pnp.bus_id));
+
+ 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;
+ }
}
- return ret;
+
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found driver [%s] for device [%s]\n",
+ acpi_drv->name, acpi_dev->pnp.bus_id));
+ get_device(dev);
+ return 0;
}
static int acpi_device_remove(struct device * dev)
@@ -1114,41 +1122,6 @@ static void acpi_device_unregister(struc
Driver Management
-------------------------------------------------------------------------- */
/**
- * acpi_bus_driver_init - add a device to a driver
- * @device: the device to add and initialize
- * @driver: driver for the device
- *
- * Used to initialize a device via its device driver. Called whenever a
- * driver is bound to a device. Invokes the driver's add() ops.
- */
-static int
-acpi_bus_driver_init(struct acpi_device *device, struct acpi_driver *driver)
-{
- int result = 0;
-
- if (!device || !driver)
- return -EINVAL;
-
- if (!driver->ops.add)
- return -ENOSYS;
-
- result = driver->ops.add(device);
- if (result)
- return result;
-
- device->driver = driver;
-
- /*
- * TBD - Configuration Management: Assign resources to device based
- * upon possible configuration and currently allocated resources.
- */
-
- ACPI_DEBUG_PRINT((ACPI_DB_INFO,
- "Driver successfully bound to device\n"));
- return 0;
-}
-
-/**
* acpi_bus_register_driver - register a driver with the ACPI bus
* @driver: driver being registered
*
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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
On Monday, June 10, 2013 09:28:58 PM Aaron Lu wrote:
> 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 <[email protected]>
> >>>>
> >>>> 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 <[email protected]>
> >>>> ---
> >>>>
> >>>> 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.
That won't hurt, but it's 3.11 material anyway, so it's going to get some
testing (hopefully).
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 06/10/2013 06:18 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
> Subject: ACPI / scan: Simplify ACPI driver probing
>
> 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 and, moreover,
> 'device' cannot be NULL then, because acpi_device_probe() is called
> via really_probe() (which also sets dev->driver for that matter).
>
> For these reasons, drop acpi_bus_driver_init() altogether and move
> the remaining code from it directly into acpi_device_probe().
Tested on one desktop and two laptops, no problems found.
Thanks,
Aaron
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> It's in the bleeding-edge branch of the linux-pm.git tree already.
>
> Thanks,
> Rafael
>
> ---
> drivers/acpi/scan.c | 81 +++++++++++++++++-----------------------------------
> 1 file changed, 27 insertions(+), 54 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -933,32 +933,40 @@ 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)
> +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);
> 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 (!acpi_drv->ops.add)
> + return -ENOSYS;
> +
> + ret = acpi_drv->ops.add(acpi_dev);
> + if (ret)
> + return ret;
>
> - ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> - "Found driver [%s] for device [%s]\n",
> - acpi_drv->name, acpi_dev->pnp.bus_id));
> - get_device(dev);
> + acpi_dev->driver = acpi_drv;
> + ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> + "Driver [%s] successfully bound to device [%s]\n",
> + acpi_drv->name, acpi_dev->pnp.bus_id));
> +
> + 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;
> + }
> }
> - return ret;
> +
> + ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found driver [%s] for device [%s]\n",
> + acpi_drv->name, acpi_dev->pnp.bus_id));
> + get_device(dev);
> + return 0;
> }
>
> static int acpi_device_remove(struct device * dev)
> @@ -1114,41 +1122,6 @@ static void acpi_device_unregister(struc
> Driver Management
> -------------------------------------------------------------------------- */
> /**
> - * acpi_bus_driver_init - add a device to a driver
> - * @device: the device to add and initialize
> - * @driver: driver for the device
> - *
> - * Used to initialize a device via its device driver. Called whenever a
> - * driver is bound to a device. Invokes the driver's add() ops.
> - */
> -static int
> -acpi_bus_driver_init(struct acpi_device *device, struct acpi_driver *driver)
> -{
> - int result = 0;
> -
> - if (!device || !driver)
> - return -EINVAL;
> -
> - if (!driver->ops.add)
> - return -ENOSYS;
> -
> - result = driver->ops.add(device);
> - if (result)
> - return result;
> -
> - device->driver = driver;
> -
> - /*
> - * TBD - Configuration Management: Assign resources to device based
> - * upon possible configuration and currently allocated resources.
> - */
> -
> - ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> - "Driver successfully bound to device\n"));
> - return 0;
> -}
> -
> -/**
> * acpi_bus_register_driver - register a driver with the ACPI bus
> * @driver: driver being registered
> *
>