[PATCH] ACPI / scan: Simplify ACPI driver probing

[PATCH] ACPI / scan: Simplify ACPI driver probing

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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@intel.com>
---

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
  *

Re: [PATCH] ACPI / scan: Simplify ACPI driver probing

[Aaron Lu]

On 06/09/2013 06:28 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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@intel.com>
> ---
> 
> 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
>   *
> 

Re: [PATCH] ACPI / scan: Simplify ACPI driver probing

[Aaron Lu]

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@intel.com>
>>
>> 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@intel.com>
>> ---
>>
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] ACPI / scan: Simplify ACPI driver probing

[Rafael J. Wysocki]

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@intel.com>
> >>
> >> 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@intel.com>
> >> ---
> >>
> >> 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.

Re: [PATCH] ACPI / scan: Simplify ACPI driver probing

[Aaron Lu]

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@intel.com>
>>>>
>>>> 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@intel.com>
>>>> ---
>>>>
>>>> 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

Re: [PATCH] ACPI / scan: Simplify ACPI driver probing

[Rafael J. Wysocki]

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 <rafael.j.wysocki@intel.com>
> >>>>
> >>>> 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@intel.com>
> >>>> ---
> >>>>
> >>>> 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.

[Update][PATCH] ACPI / scan: Simplify ACPI driver probing

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
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 <rafael.j.wysocki@intel.com>
---

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
  *

Re: [Update][PATCH] ACPI / scan: Simplify ACPI driver probing

[Aaron Lu]

On 06/10/2013 06:18 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 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 <rafael.j.wysocki@intel.com>
> ---
> 
> 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
>   *
>