diff mbox

[rev.2,1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers

Message ID 4082842.31rfDviYHK@vostro.rjw.lan (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Rafael Wysocki Dec. 18, 2012, 1:48 a.m. UTC
On Monday, December 17, 2012 05:08:17 PM Toshi Kani wrote:
> On Thu, 2012-12-13 at 23:17 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> 
> (snip)
> 
> >  struct acpi_device_ops {
> > Index: linux/drivers/acpi/scan.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/scan.c
> > +++ linux/drivers/acpi/scan.c
> > @@ -494,7 +494,8 @@ static int acpi_bus_match(struct device
> >  	struct acpi_device *acpi_dev = to_acpi_device(dev);
> >  	struct acpi_driver *acpi_drv = to_acpi_driver(drv);
> >  
> > -	return !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
> > +	return acpi_dev->bus_ops.acpi_op_match
> > +		&& !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
> >  }
> >  
> >  static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> > @@ -1418,6 +1419,17 @@ static int acpi_bus_remove(struct acpi_d
> >  	return 0;
> >  }
> >  
> > +/*
> > + * acpi_hot_add_bind - Bind _ADR-based devices on hot-add.
> > + * @device: ACPI device node to bind.
> > + */
> > +static void acpi_hot_add_bind(struct acpi_device *device)
> > +{
> > +	if (device->flags.bus_address
> > +	    && device->parent && device->parent->ops.bind)
> > +		device->parent->ops.bind(device);
> > +}
> > +
> >  static int acpi_add_single_object(struct acpi_device **child,
> >  				  acpi_handle handle, int type,
> >  				  unsigned long long sta,
> > @@ -1490,13 +1502,8 @@ static int acpi_add_single_object(struct
> >  
> >  	result = acpi_device_register(device);
> >  
> > -	/*
> > -	 * Bind _ADR-Based Devices when hot add
> > -	 */
> > -	if (device->flags.bus_address) {
> > -		if (device->parent && device->parent->ops.bind)
> > -			device->parent->ops.bind(device);
> > -	}
> 
> I think the original code above is hot-add only because ops.bind is not
> set at boot since the acpi_pci driver has not been registered yet.  It
> seems that acpi_pci_bridge_scan() called from acpi_pci_root_add() takes
> care of the binding.

Ah, I see the problem.  During boot the PCI root bridge driver is not present
yet when all struct acpi_device "devices" are registered, so their parents'
.bind() callbacks are all empty, so the code above has no effect.

But say we're doing a PCI root bridge hotplug, in which case the driver is
present, so acpi_pci_bind() will be executed both from acpi_pci_bridge_scan()
and from here, won't it?

OK, this needs to be addressed.

> This brings me a question for acpi_bus_probe_start() below...
> 
> 
> > +	if (device->bus_ops.acpi_op_match)
> > +		acpi_hot_add_bind(device);
> >  
> >  end:
> >  	if (!result) {
> > @@ -1522,6 +1529,7 @@ static void acpi_bus_add_power_resource(
> >  	struct acpi_bus_ops ops = {
> >  		.acpi_op_add = 1,
> >  		.acpi_op_start = 1,
> > +		.acpi_op_match = 1,
> >  	};
> >  	struct acpi_device *device = NULL;
> >  
> > @@ -1574,9 +1582,9 @@ static acpi_status acpi_bus_check_add(ac
> >  				      void *context, void **return_value)
> >  {
> >  	struct acpi_bus_ops *ops = context;
> > +	struct acpi_device *device = NULL;
> >  	int type;
> >  	unsigned long long sta;
> > -	struct acpi_device *device;
> >  	acpi_status status;
> >  	int result;
> >  
> > @@ -1596,52 +1604,86 @@ static acpi_status acpi_bus_check_add(ac
> >  		return AE_CTRL_DEPTH;
> >  	}
> >  
> > -	/*
> > -	 * We may already have an acpi_device from a previous enumeration.  If
> > -	 * so, we needn't add it again, but we may still have to start it.
> > -	 */
> > -	device = NULL;
> >  	acpi_bus_get_device(handle, &device);
> >  	if (ops->acpi_op_add && !device) {
> > -		acpi_add_single_object(&device, handle, type, sta, ops);
> > -		/* Is the device a known good platform device? */
> > -		if (device
> > -		    && !acpi_match_device_ids(device, acpi_platform_device_ids))
> > -			acpi_create_platform_device(device);
> > -	}
> > -
> > -	if (!device)
> > -		return AE_CTRL_DEPTH;
> > +		struct acpi_bus_ops add_ops = *ops;
> >  
> > -	if (ops->acpi_op_start && !(ops->acpi_op_add)) {
> > -		status = acpi_start_single_object(device);
> > -		if (ACPI_FAILURE(status))
> > +		add_ops.acpi_op_match = 0;
> > +		acpi_add_single_object(&device, handle, type, sta, &add_ops);
> > +		if (!device)
> >  			return AE_CTRL_DEPTH;
> > +
> > +		device->bus_ops.acpi_op_match = 1;
> >  	}
> >  
> >  	if (!*return_value)
> >  		*return_value = device;
> > +
> >  	return AE_OK;
> >  }
> >  
> > +static acpi_status acpi_bus_probe_start(acpi_handle handle, u32 lvl,
> > +					void *context, void **not_used)
> > +{
> > +	struct acpi_bus_ops *ops = context;
> > +	acpi_status status = AE_OK;
> > +	struct acpi_device *device;
> > +	unsigned long long sta_not_used;
> > +	int type_not_used;
> > +
> > +	/*
> > +	 * Ignore errors ignored by acpi_bus_check_add() to avoid terminating
> 
> "ignore" seems duplicated. 
 
It is not.  This is supposed to mean that the errors previously ignored by
acpi_bus_check_add() should be ignored here as well.

> > +	 * namespace walks prematurely.
> > +	 */
> > +	if (acpi_bus_type_and_status(handle, &type_not_used, &sta_not_used))
> > +		return AE_OK;
> > +
> > +	if (acpi_bus_get_device(handle, &device))
> > +		return AE_CTRL_DEPTH;
> > +
> > +	if (ops->acpi_op_add) {
> > +		if (!acpi_match_device_ids(device, acpi_platform_device_ids)) {
> > +			/* This is a known good platform device. */
> > +			acpi_create_platform_device(device);
> > +		} else {
> > +			int ret = device_attach(&device->dev);
> > +			acpi_hot_add_bind(device);
> 
> Since acpi_pci_root_add() is called by device_attach(), I think this
> acpi_hot_add_bind() calls .bind() of a device at boot since its .bind()
> may be set.  Is that correct?  If so, how does it coordinate with the
> bind procedure in acpi_pci_bridge_scan()?

It actually doesn't.

However, the $subject patch doesn't change this particular aspect of the
original behavior, because with it applied the PCI root bridge driver is still
not present when the device_attach() above is executed for all objects in the
given namespace scope, so the .bind() callbacks should all be empty.  In other
words, it doesn't change the boot case.

It also reproduces the original behavior in the hotplug case which may not be
correct.  Patch [2/6], however, kind of changes the boot case into the hotplug
case and things start to get ugly.

Well, what about calling acpi_hot_add_bind() from acpi_bus_check_add(),
right after doing the acpi_add_single_object()?  It would avoid calling
acpi_pci_bind() twice for the same device during root bridge hotplug too,
because in that case acpi_pci_root_add() will be called after all of these
acpi_hot_add_bind() calls.  At the same time if a single device is
hot-added and its parent happens to have .bind() set, it will be run
from acpi_bus_check_add().

Updated patch is appended.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI: Separate adding ACPI device objects from probing ACPI drivers

Split the ACPI namespace scanning for devices into two passes, such
that struct acpi_device objects are registerd in the first pass
without probing ACPI drivers and the drivers are probed against them
directly in the second pass.

There are two main reasons for doing that.

First, the ACPI PCI root bridge driver's .add() routine,
acpi_pci_root_add(), causes struct pci_dev objects to be created for
all PCI devices under the given root bridge.  Usually, there are
corresponding ACPI device nodes in the ACPI namespace for some of
those devices and therefore there should be "companion" struct
acpi_device objects to attach those struct pci_dev objects to.  These
struct acpi_device objects should exist when the corresponding
struct pci_dev objects are created, but that is only guaranteed
during boot and not during hotplug.  This leads to a number of
functional differences between the boot and the hotplug cases which
are not strictly necessary and make the code more complicated.

For example, this forces the ACPI PCI root bridge driver to defer the
registration of the just created struct pci_dev objects and to use a
special .start() callback routine, acpi_pci_root_start(), to make
sure that all of the "companion" struct acpi_device objects will be
present at PCI devices registration time during hotplug.

If those differences can be eliminated, we will be able to
consolidate the boot and hotplug code paths for the enumeration and
registration of PCI devices and to reduce the complexity of that
code quite a bit.

The second reason is that, in general, it should be possible to
resolve conflicts of resources assigned by the BIOS to different
devices represented by ACPI namespace nodes before any drivers bind
to them and before they are attached to "companion" objects
representing physical devices (such as struct pci_dev).  However, for
this purpose we first need to enumerate all ACPI device nodes in the
given namespace scope.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c     |  103 +++++++++++++++++++++++++++++++++---------------
 include/acpi/acpi_bus.h |    1 
 2 files changed, 73 insertions(+), 31 deletions(-)

Comments

Toshi Kani Dec. 18, 2012, 4:10 p.m. UTC | #1
On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
> On Monday, December 17, 2012 05:08:17 PM Toshi Kani wrote:
> > On Thu, 2012-12-13 at 23:17 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > 
> > (snip)
> > 
> > >  struct acpi_device_ops {
> > > Index: linux/drivers/acpi/scan.c
> > > ===================================================================
> > > --- linux.orig/drivers/acpi/scan.c
> > > +++ linux/drivers/acpi/scan.c
> > > @@ -494,7 +494,8 @@ static int acpi_bus_match(struct device
> > >  	struct acpi_device *acpi_dev = to_acpi_device(dev);
> > >  	struct acpi_driver *acpi_drv = to_acpi_driver(drv);
> > >  
> > > -	return !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
> > > +	return acpi_dev->bus_ops.acpi_op_match
> > > +		&& !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
> > >  }
> > >  
> > >  static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> > > @@ -1418,6 +1419,17 @@ static int acpi_bus_remove(struct acpi_d
> > >  	return 0;
> > >  }
> > >  
> > > +/*
> > > + * acpi_hot_add_bind - Bind _ADR-based devices on hot-add.
> > > + * @device: ACPI device node to bind.
> > > + */
> > > +static void acpi_hot_add_bind(struct acpi_device *device)
> > > +{
> > > +	if (device->flags.bus_address
> > > +	    && device->parent && device->parent->ops.bind)
> > > +		device->parent->ops.bind(device);
> > > +}
> > > +
> > >  static int acpi_add_single_object(struct acpi_device **child,
> > >  				  acpi_handle handle, int type,
> > >  				  unsigned long long sta,
> > > @@ -1490,13 +1502,8 @@ static int acpi_add_single_object(struct
> > >  
> > >  	result = acpi_device_register(device);
> > >  
> > > -	/*
> > > -	 * Bind _ADR-Based Devices when hot add
> > > -	 */
> > > -	if (device->flags.bus_address) {
> > > -		if (device->parent && device->parent->ops.bind)
> > > -			device->parent->ops.bind(device);
> > > -	}
> > 
> > I think the original code above is hot-add only because ops.bind is not
> > set at boot since the acpi_pci driver has not been registered yet.  It
> > seems that acpi_pci_bridge_scan() called from acpi_pci_root_add() takes
> > care of the binding.
> 
> Ah, I see the problem.  During boot the PCI root bridge driver is not present
> yet when all struct acpi_device "devices" are registered, so their parents'
> .bind() callbacks are all empty, so the code above has no effect.
> 
> But say we're doing a PCI root bridge hotplug, in which case the driver is
> present, so acpi_pci_bind() will be executed both from acpi_pci_bridge_scan()
> and from here, won't it?

Right.


> OK, this needs to be addressed.
> 
> > This brings me a question for acpi_bus_probe_start() below...
> > 
> > 
> > > +	if (device->bus_ops.acpi_op_match)
> > > +		acpi_hot_add_bind(device);
> > >  
> > >  end:
> > >  	if (!result) {
> > > @@ -1522,6 +1529,7 @@ static void acpi_bus_add_power_resource(
> > >  	struct acpi_bus_ops ops = {
> > >  		.acpi_op_add = 1,
> > >  		.acpi_op_start = 1,
> > > +		.acpi_op_match = 1,
> > >  	};
> > >  	struct acpi_device *device = NULL;
> > >  
> > > @@ -1574,9 +1582,9 @@ static acpi_status acpi_bus_check_add(ac
> > >  				      void *context, void **return_value)
> > >  {
> > >  	struct acpi_bus_ops *ops = context;
> > > +	struct acpi_device *device = NULL;
> > >  	int type;
> > >  	unsigned long long sta;
> > > -	struct acpi_device *device;
> > >  	acpi_status status;
> > >  	int result;
> > >  
> > > @@ -1596,52 +1604,86 @@ static acpi_status acpi_bus_check_add(ac
> > >  		return AE_CTRL_DEPTH;
> > >  	}
> > >  
> > > -	/*
> > > -	 * We may already have an acpi_device from a previous enumeration.  If
> > > -	 * so, we needn't add it again, but we may still have to start it.
> > > -	 */
> > > -	device = NULL;
> > >  	acpi_bus_get_device(handle, &device);
> > >  	if (ops->acpi_op_add && !device) {
> > > -		acpi_add_single_object(&device, handle, type, sta, ops);
> > > -		/* Is the device a known good platform device? */
> > > -		if (device
> > > -		    && !acpi_match_device_ids(device, acpi_platform_device_ids))
> > > -			acpi_create_platform_device(device);
> > > -	}
> > > -
> > > -	if (!device)
> > > -		return AE_CTRL_DEPTH;
> > > +		struct acpi_bus_ops add_ops = *ops;
> > >  
> > > -	if (ops->acpi_op_start && !(ops->acpi_op_add)) {
> > > -		status = acpi_start_single_object(device);
> > > -		if (ACPI_FAILURE(status))
> > > +		add_ops.acpi_op_match = 0;
> > > +		acpi_add_single_object(&device, handle, type, sta, &add_ops);
> > > +		if (!device)
> > >  			return AE_CTRL_DEPTH;
> > > +
> > > +		device->bus_ops.acpi_op_match = 1;
> > >  	}
> > >  
> > >  	if (!*return_value)
> > >  		*return_value = device;
> > > +
> > >  	return AE_OK;
> > >  }
> > >  
> > > +static acpi_status acpi_bus_probe_start(acpi_handle handle, u32 lvl,
> > > +					void *context, void **not_used)
> > > +{
> > > +	struct acpi_bus_ops *ops = context;
> > > +	acpi_status status = AE_OK;
> > > +	struct acpi_device *device;
> > > +	unsigned long long sta_not_used;
> > > +	int type_not_used;
> > > +
> > > +	/*
> > > +	 * Ignore errors ignored by acpi_bus_check_add() to avoid terminating
> > 
> > "ignore" seems duplicated. 
>  
> It is not.  This is supposed to mean that the errors previously ignored by
> acpi_bus_check_add() should be ignored here as well.

Oh, I see.  Thanks for the clarification.


> > > +	 * namespace walks prematurely.
> > > +	 */
> > > +	if (acpi_bus_type_and_status(handle, &type_not_used, &sta_not_used))
> > > +		return AE_OK;
> > > +
> > > +	if (acpi_bus_get_device(handle, &device))
> > > +		return AE_CTRL_DEPTH;
> > > +
> > > +	if (ops->acpi_op_add) {
> > > +		if (!acpi_match_device_ids(device, acpi_platform_device_ids)) {
> > > +			/* This is a known good platform device. */
> > > +			acpi_create_platform_device(device);
> > > +		} else {
> > > +			int ret = device_attach(&device->dev);
> > > +			acpi_hot_add_bind(device);
> > 
> > Since acpi_pci_root_add() is called by device_attach(), I think this
> > acpi_hot_add_bind() calls .bind() of a device at boot since its .bind()
> > may be set.  Is that correct?  If so, how does it coordinate with the
> > bind procedure in acpi_pci_bridge_scan()?
> 
> It actually doesn't.
> 
> However, the $subject patch doesn't change this particular aspect of the
> original behavior, because with it applied the PCI root bridge driver is still
> not present when the device_attach() above is executed for all objects in the
> given namespace scope, so the .bind() callbacks should all be empty.  In other
> words, it doesn't change the boot case.
> 
> It also reproduces the original behavior in the hotplug case which may not be
> correct.  Patch [2/6], however, kind of changes the boot case into the hotplug
> case and things start to get ugly.

Yes, I was concerned with the behavior when patch [2/6] applied.  It is
actually a good thing that this hotplug issue is now exposed in the boot
path.  This means that boot and hot-add paths are consistent.  So, we
just need to fix it.


> Well, what about calling acpi_hot_add_bind() from acpi_bus_check_add(),
> right after doing the acpi_add_single_object()?  It would avoid calling
> acpi_pci_bind() twice for the same device during root bridge hotplug too,
> because in that case acpi_pci_root_add() will be called after all of these
> acpi_hot_add_bind() calls.  At the same time if a single device is
> hot-added and its parent happens to have .bind() set, it will be run
> from acpi_bus_check_add().

I may be missing something here, but in case of root bridge hot-add, I
think acpi_pci_root_add() still calls .bind() after acpi_bus_check_add()
called it.  So, it's called twice, isn't it? 

We need to decide which module is responsible for calling .bind().  I
think it should be the ACPI scan module, not the ACPI PCI root bridge
driver, because:

 - bind() needs to be called when _ADR device is added.  The ACPI scan
module can scan any devices, while the PCI root driver can only scan
when it is added.
 - acpi_bus_remove() calls unbind() at hot-remove.  The same module
should be responsible for both bind() and unbind() handling.
 - It is cleaner to keep struct acpi_device_ops interface to be called
by the ACPI core.

So, I would propose the following changes.

 - Move the acpi_hot_add_bind() call back to the original place after
the device_attach() call.
 - Rename the name of acpi_hot_add_bind() to something like
acpi_bind_adr_device() since it is no longer hot-add only (and is
specific to _ADR devices).
 - Create its pair function, acpi_unbind_adr_device(), which is called
from acpi_bus_remove().  When a constructor interface is introduced, its
destructor should be introduced as well. 
 - Remove the binding procedure from acpi_pci_root_add().  This should
be done in patch [2/6].

Thanks,
-Toshi

--
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
Yinghai Lu Dec. 18, 2012, 6:59 p.m. UTC | #2
On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani <toshi.kani@hp.com> wrote:
> On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
>
> So, I would propose the following changes.
>
>  - Move the acpi_hot_add_bind() call back to the original place after
> the device_attach() call.
>  - Rename the name of acpi_hot_add_bind() to something like
> acpi_bind_adr_device() since it is no longer hot-add only (and is
> specific to _ADR devices).
>  - Create its pair function, acpi_unbind_adr_device(), which is called
> from acpi_bus_remove().  When a constructor interface is introduced, its
> destructor should be introduced as well.
>  - Remove the binding procedure from acpi_pci_root_add().  This should
> be done in patch [2/6].

i think we should put jiang four patches before Rafael's patches.

http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug
--
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
Toshi Kani Dec. 18, 2012, 8:48 p.m. UTC | #3
On Tue, 2012-12-18 at 10:59 -0800, Yinghai Lu wrote:
> On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani <toshi.kani@hp.com> wrote:
> > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
> >
> > So, I would propose the following changes.
> >
> >  - Move the acpi_hot_add_bind() call back to the original place after
> > the device_attach() call.
> >  - Rename the name of acpi_hot_add_bind() to something like
> > acpi_bind_adr_device() since it is no longer hot-add only (and is
> > specific to _ADR devices).
> >  - Create its pair function, acpi_unbind_adr_device(), which is called
> > from acpi_bus_remove().  When a constructor interface is introduced, its
> > destructor should be introduced as well.
> >  - Remove the binding procedure from acpi_pci_root_add().  This should
> > be done in patch [2/6].
> 
> i think we should put jiang four patches before Rafael's patches.
> 
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug

Thanks for the pointer!  Oh, I did not know that pci_dev gets removed
before acpi_bus_trim() is called...  I looked at Jiang's last two
patches for the bind update, and they look good to me.  

Thanks,
-Toshi

--
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
Yinghai Lu Dec. 18, 2012, 9:11 p.m. UTC | #4
On Tue, Dec 18, 2012 at 12:48 PM, Toshi Kani <toshi.kani@hp.com> wrote:
> On Tue, 2012-12-18 at 10:59 -0800, Yinghai Lu wrote:
>> On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani <toshi.kani@hp.com> wrote:
>> > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
>> >
>> > So, I would propose the following changes.
>> >
>> >  - Move the acpi_hot_add_bind() call back to the original place after
>> > the device_attach() call.
>> >  - Rename the name of acpi_hot_add_bind() to something like
>> > acpi_bind_adr_device() since it is no longer hot-add only (and is
>> > specific to _ADR devices).
>> >  - Create its pair function, acpi_unbind_adr_device(), which is called
>> > from acpi_bus_remove().  When a constructor interface is introduced, its
>> > destructor should be introduced as well.
>> >  - Remove the binding procedure from acpi_pci_root_add().  This should
>> > be done in patch [2/6].
>>
>> i think we should put jiang four patches before Rafael's patches.
>>
>> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug
>
> Thanks for the pointer!  Oh, I did not know that pci_dev gets removed
> before acpi_bus_trim() is called...

yes. that is
http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=8b4b836d8c56c290bc80ffa0b08b91fb3fe38867

> I looked at Jiang's last two
> patches for the bind update, and they look good to me.
>
> Thanks,
> -Toshi
>
--
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
Rafael Wysocki Dec. 18, 2012, 9:57 p.m. UTC | #5
On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote:
> On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
> > On Monday, December 17, 2012 05:08:17 PM Toshi Kani wrote:
> > > On Thu, 2012-12-13 at 23:17 +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > 
> > > (snip)
> > > 
> > > >  struct acpi_device_ops {
> > > > Index: linux/drivers/acpi/scan.c
> > > > ===================================================================
> > > > --- linux.orig/drivers/acpi/scan.c
> > > > +++ linux/drivers/acpi/scan.c
> > > > @@ -494,7 +494,8 @@ static int acpi_bus_match(struct device
> > > >  	struct acpi_device *acpi_dev = to_acpi_device(dev);
> > > >  	struct acpi_driver *acpi_drv = to_acpi_driver(drv);
> > > >  
> > > > -	return !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
> > > > +	return acpi_dev->bus_ops.acpi_op_match
> > > > +		&& !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
> > > >  }
> > > >  
> > > >  static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> > > > @@ -1418,6 +1419,17 @@ static int acpi_bus_remove(struct acpi_d
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/*
> > > > + * acpi_hot_add_bind - Bind _ADR-based devices on hot-add.
> > > > + * @device: ACPI device node to bind.
> > > > + */
> > > > +static void acpi_hot_add_bind(struct acpi_device *device)
> > > > +{
> > > > +	if (device->flags.bus_address
> > > > +	    && device->parent && device->parent->ops.bind)
> > > > +		device->parent->ops.bind(device);
> > > > +}
> > > > +
> > > >  static int acpi_add_single_object(struct acpi_device **child,
> > > >  				  acpi_handle handle, int type,
> > > >  				  unsigned long long sta,
> > > > @@ -1490,13 +1502,8 @@ static int acpi_add_single_object(struct
> > > >  
> > > >  	result = acpi_device_register(device);
> > > >  
> > > > -	/*
> > > > -	 * Bind _ADR-Based Devices when hot add
> > > > -	 */
> > > > -	if (device->flags.bus_address) {
> > > > -		if (device->parent && device->parent->ops.bind)
> > > > -			device->parent->ops.bind(device);
> > > > -	}
> > > 
> > > I think the original code above is hot-add only because ops.bind is not
> > > set at boot since the acpi_pci driver has not been registered yet.  It
> > > seems that acpi_pci_bridge_scan() called from acpi_pci_root_add() takes
> > > care of the binding.
> > 
> > Ah, I see the problem.  During boot the PCI root bridge driver is not present
> > yet when all struct acpi_device "devices" are registered, so their parents'
> > .bind() callbacks are all empty, so the code above has no effect.
> > 
> > But say we're doing a PCI root bridge hotplug, in which case the driver is
> > present, so acpi_pci_bind() will be executed both from acpi_pci_bridge_scan()
> > and from here, won't it?
> 
> Right.
> 
> 
> > OK, this needs to be addressed.
> > 
> > > This brings me a question for acpi_bus_probe_start() below...
> > > 
> > > 
> > > > +	if (device->bus_ops.acpi_op_match)
> > > > +		acpi_hot_add_bind(device);
> > > >  
> > > >  end:
> > > >  	if (!result) {
> > > > @@ -1522,6 +1529,7 @@ static void acpi_bus_add_power_resource(
> > > >  	struct acpi_bus_ops ops = {
> > > >  		.acpi_op_add = 1,
> > > >  		.acpi_op_start = 1,
> > > > +		.acpi_op_match = 1,
> > > >  	};
> > > >  	struct acpi_device *device = NULL;
> > > >  
> > > > @@ -1574,9 +1582,9 @@ static acpi_status acpi_bus_check_add(ac
> > > >  				      void *context, void **return_value)
> > > >  {
> > > >  	struct acpi_bus_ops *ops = context;
> > > > +	struct acpi_device *device = NULL;
> > > >  	int type;
> > > >  	unsigned long long sta;
> > > > -	struct acpi_device *device;
> > > >  	acpi_status status;
> > > >  	int result;
> > > >  
> > > > @@ -1596,52 +1604,86 @@ static acpi_status acpi_bus_check_add(ac
> > > >  		return AE_CTRL_DEPTH;
> > > >  	}
> > > >  
> > > > -	/*
> > > > -	 * We may already have an acpi_device from a previous enumeration.  If
> > > > -	 * so, we needn't add it again, but we may still have to start it.
> > > > -	 */
> > > > -	device = NULL;
> > > >  	acpi_bus_get_device(handle, &device);
> > > >  	if (ops->acpi_op_add && !device) {
> > > > -		acpi_add_single_object(&device, handle, type, sta, ops);
> > > > -		/* Is the device a known good platform device? */
> > > > -		if (device
> > > > -		    && !acpi_match_device_ids(device, acpi_platform_device_ids))
> > > > -			acpi_create_platform_device(device);
> > > > -	}
> > > > -
> > > > -	if (!device)
> > > > -		return AE_CTRL_DEPTH;
> > > > +		struct acpi_bus_ops add_ops = *ops;
> > > >  
> > > > -	if (ops->acpi_op_start && !(ops->acpi_op_add)) {
> > > > -		status = acpi_start_single_object(device);
> > > > -		if (ACPI_FAILURE(status))
> > > > +		add_ops.acpi_op_match = 0;
> > > > +		acpi_add_single_object(&device, handle, type, sta, &add_ops);
> > > > +		if (!device)
> > > >  			return AE_CTRL_DEPTH;
> > > > +
> > > > +		device->bus_ops.acpi_op_match = 1;
> > > >  	}
> > > >  
> > > >  	if (!*return_value)
> > > >  		*return_value = device;
> > > > +
> > > >  	return AE_OK;
> > > >  }
> > > >  
> > > > +static acpi_status acpi_bus_probe_start(acpi_handle handle, u32 lvl,
> > > > +					void *context, void **not_used)
> > > > +{
> > > > +	struct acpi_bus_ops *ops = context;
> > > > +	acpi_status status = AE_OK;
> > > > +	struct acpi_device *device;
> > > > +	unsigned long long sta_not_used;
> > > > +	int type_not_used;
> > > > +
> > > > +	/*
> > > > +	 * Ignore errors ignored by acpi_bus_check_add() to avoid terminating
> > > 
> > > "ignore" seems duplicated. 
> >  
> > It is not.  This is supposed to mean that the errors previously ignored by
> > acpi_bus_check_add() should be ignored here as well.
> 
> Oh, I see.  Thanks for the clarification.
> 
> 
> > > > +	 * namespace walks prematurely.
> > > > +	 */
> > > > +	if (acpi_bus_type_and_status(handle, &type_not_used, &sta_not_used))
> > > > +		return AE_OK;
> > > > +
> > > > +	if (acpi_bus_get_device(handle, &device))
> > > > +		return AE_CTRL_DEPTH;
> > > > +
> > > > +	if (ops->acpi_op_add) {
> > > > +		if (!acpi_match_device_ids(device, acpi_platform_device_ids)) {
> > > > +			/* This is a known good platform device. */
> > > > +			acpi_create_platform_device(device);
> > > > +		} else {
> > > > +			int ret = device_attach(&device->dev);
> > > > +			acpi_hot_add_bind(device);
> > > 
> > > Since acpi_pci_root_add() is called by device_attach(), I think this
> > > acpi_hot_add_bind() calls .bind() of a device at boot since its .bind()
> > > may be set.  Is that correct?  If so, how does it coordinate with the
> > > bind procedure in acpi_pci_bridge_scan()?
> > 
> > It actually doesn't.
> > 
> > However, the $subject patch doesn't change this particular aspect of the
> > original behavior, because with it applied the PCI root bridge driver is still
> > not present when the device_attach() above is executed for all objects in the
> > given namespace scope, so the .bind() callbacks should all be empty.  In other
> > words, it doesn't change the boot case.
> > 
> > It also reproduces the original behavior in the hotplug case which may not be
> > correct.  Patch [2/6], however, kind of changes the boot case into the hotplug
> > case and things start to get ugly.
> 
> Yes, I was concerned with the behavior when patch [2/6] applied.  It is
> actually a good thing that this hotplug issue is now exposed in the boot
> path.  This means that boot and hot-add paths are consistent.  So, we
> just need to fix it.
> 
> 
> > Well, what about calling acpi_hot_add_bind() from acpi_bus_check_add(),
> > right after doing the acpi_add_single_object()?  It would avoid calling
> > acpi_pci_bind() twice for the same device during root bridge hotplug too,
> > because in that case acpi_pci_root_add() will be called after all of these
> > acpi_hot_add_bind() calls.  At the same time if a single device is
> > hot-added and its parent happens to have .bind() set, it will be run
> > from acpi_bus_check_add().
> 
> I may be missing something here, but in case of root bridge hot-add, I
> think acpi_pci_root_add() still calls .bind() after acpi_bus_check_add()
> called it.  So, it's called twice, isn't it? 

No, it isn't.  The reason is that the .bind pointers of all devices below the
root bridge are populated from within acpi_pci_root_add() and are NULL before
it is called.  Therefore they are NULL when acpi_bus_check_add() checks them. 

Of course, I'm referring to this patch:

https://patchwork.kernel.org/patch/1889821/

After this and [2/6] (https://patchwork.kernel.org/patch/1884701/) have been
applied, it is quite easy to verify that acpi_pci_bind() is called exactly once
for each PCI device during boot (just add a debug printk to that function) and
the same should happen during root bridge hotplug.

> We need to decide which module is responsible for calling .bind().  I
> think it should be the ACPI scan module, not the ACPI PCI root bridge
> driver, because:
>  - bind() needs to be called when _ADR device is added.  The ACPI scan
> module can scan any devices, while the PCI root driver can only scan
> when it is added.
>  - acpi_bus_remove() calls unbind() at hot-remove.  The same module
> should be responsible for both bind() and unbind() handling.
>  - It is cleaner to keep struct acpi_device_ops interface to be called
> by the ACPI core.

I agree with that. :-)

Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all.

> So, I would propose the following changes.
> 
>  - Move the acpi_hot_add_bind() call back to the original place after
> the device_attach() call.
>  - Rename the name of acpi_hot_add_bind() to something like
> acpi_bind_adr_device() since it is no longer hot-add only (and is
> specific to _ADR devices).
>  - Create its pair function, acpi_unbind_adr_device(), which is called
> from acpi_bus_remove().  When a constructor interface is introduced, its
> destructor should be introduced as well. 
>  - Remove the binding procedure from acpi_pci_root_add().  This should
> be done in patch [2/6].

Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind()
somewhere else and removing those things altogether?

Rafael
Rafael Wysocki Dec. 18, 2012, 10:05 p.m. UTC | #6
On Tuesday, December 18, 2012 10:59:46 AM Yinghai Lu wrote:
> On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani <toshi.kani@hp.com> wrote:
> > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
> >
> > So, I would propose the following changes.
> >
> >  - Move the acpi_hot_add_bind() call back to the original place after
> > the device_attach() call.
> >  - Rename the name of acpi_hot_add_bind() to something like
> > acpi_bind_adr_device() since it is no longer hot-add only (and is
> > specific to _ADR devices).
> >  - Create its pair function, acpi_unbind_adr_device(), which is called
> > from acpi_bus_remove().  When a constructor interface is introduced, its
> > destructor should be introduced as well.
> >  - Remove the binding procedure from acpi_pci_root_add().  This should
> > be done in patch [2/6].
> 
> i think we should put jiang four patches before Rafael's patches.
> 
> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug

Actually, I have something more radical than that in mind. :-)

Namely, we don't need to call the wakeup-related stuff from acpi_pci_bind() and
acpi_pci_unbind().  It's only there, because I did't find a better place for it
when I added it.

If we can set the ACPI handles of PCI devices in pci_scan_device(), which isn't
too difficult to do (I actually have a patch for that and it's not too invasive),
we can easily move the wakeup enabling stuff to pci_pm_init() and wakeup
disabling somewhere near pci_release_capabilities().

Then we are only left with the _PRT setup in there, but that could be done
as soon as in pci_scan_device() too, if I'm not mistaken.

Thanks,
Rafael
Toshi Kani Dec. 18, 2012, 10:15 p.m. UTC | #7
On Tue, 2012-12-18 at 22:57 +0100, Rafael J. Wysocki wrote:
> On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote:
> > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
 :
> > We need to decide which module is responsible for calling .bind().  I
> > think it should be the ACPI scan module, not the ACPI PCI root bridge
> > driver, because:
> >  - bind() needs to be called when _ADR device is added.  The ACPI scan
> > module can scan any devices, while the PCI root driver can only scan
> > when it is added.
> >  - acpi_bus_remove() calls unbind() at hot-remove.  The same module
> > should be responsible for both bind() and unbind() handling.
> >  - It is cleaner to keep struct acpi_device_ops interface to be called
> > by the ACPI core.
> 
> I agree with that. :-)
> 
> Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all.
> 
> > So, I would propose the following changes.
> > 
> >  - Move the acpi_hot_add_bind() call back to the original place after
> > the device_attach() call.
> >  - Rename the name of acpi_hot_add_bind() to something like
> > acpi_bind_adr_device() since it is no longer hot-add only (and is
> > specific to _ADR devices).
> >  - Create its pair function, acpi_unbind_adr_device(), which is called
> > from acpi_bus_remove().  When a constructor interface is introduced, its
> > destructor should be introduced as well. 
> >  - Remove the binding procedure from acpi_pci_root_add().  This should
> > be done in patch [2/6].
> 
> Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind()
> somewhere else and removing those things altogether?

Sounds nice.  It will be bonus point if you can do that. :-)

Thanks,
-Toshi


--
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
Rafael Wysocki Dec. 18, 2012, 11 p.m. UTC | #8
On Tuesday, December 18, 2012 03:15:12 PM Toshi Kani wrote:
> On Tue, 2012-12-18 at 22:57 +0100, Rafael J. Wysocki wrote:
> > On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote:
> > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
>  :
> > > We need to decide which module is responsible for calling .bind().  I
> > > think it should be the ACPI scan module, not the ACPI PCI root bridge
> > > driver, because:
> > >  - bind() needs to be called when _ADR device is added.  The ACPI scan
> > > module can scan any devices, while the PCI root driver can only scan
> > > when it is added.
> > >  - acpi_bus_remove() calls unbind() at hot-remove.  The same module
> > > should be responsible for both bind() and unbind() handling.
> > >  - It is cleaner to keep struct acpi_device_ops interface to be called
> > > by the ACPI core.
> > 
> > I agree with that. :-)
> > 
> > Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all.
> > 
> > > So, I would propose the following changes.
> > > 
> > >  - Move the acpi_hot_add_bind() call back to the original place after
> > > the device_attach() call.
> > >  - Rename the name of acpi_hot_add_bind() to something like
> > > acpi_bind_adr_device() since it is no longer hot-add only (and is
> > > specific to _ADR devices).
> > >  - Create its pair function, acpi_unbind_adr_device(), which is called
> > > from acpi_bus_remove().  When a constructor interface is introduced, its
> > > destructor should be introduced as well. 
> > >  - Remove the binding procedure from acpi_pci_root_add().  This should
> > > be done in patch [2/6].
> > 
> > Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind()
> > somewhere else and removing those things altogether?
> 
> Sounds nice.  It will be bonus point if you can do that. :-)

I think I can, but I need a few more patches on top of what I've already posted
to do that.

I think that https://patchwork.kernel.org/patch/1889821/ and
https://patchwork.kernel.org/patch/1884701/ can stay as they are, since there's
some material on top of them already and I'll cut the new patches on top of all
that.  I'll repost the whole series some time later this week, stay tuned. :-)

Thanks,
Rafael
Bjorn Helgaas Dec. 18, 2012, 11:19 p.m. UTC | #9
On Tue, Dec 18, 2012 at 4:00 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Tuesday, December 18, 2012 03:15:12 PM Toshi Kani wrote:
>> On Tue, 2012-12-18 at 22:57 +0100, Rafael J. Wysocki wrote:
>> > On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote:
>> > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
>>  :
>> > > We need to decide which module is responsible for calling .bind().  I
>> > > think it should be the ACPI scan module, not the ACPI PCI root bridge
>> > > driver, because:
>> > >  - bind() needs to be called when _ADR device is added.  The ACPI scan
>> > > module can scan any devices, while the PCI root driver can only scan
>> > > when it is added.
>> > >  - acpi_bus_remove() calls unbind() at hot-remove.  The same module
>> > > should be responsible for both bind() and unbind() handling.
>> > >  - It is cleaner to keep struct acpi_device_ops interface to be called
>> > > by the ACPI core.
>> >
>> > I agree with that. :-)
>> >
>> > Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all.
>> >
>> > > So, I would propose the following changes.
>> > >
>> > >  - Move the acpi_hot_add_bind() call back to the original place after
>> > > the device_attach() call.
>> > >  - Rename the name of acpi_hot_add_bind() to something like
>> > > acpi_bind_adr_device() since it is no longer hot-add only (and is
>> > > specific to _ADR devices).
>> > >  - Create its pair function, acpi_unbind_adr_device(), which is called
>> > > from acpi_bus_remove().  When a constructor interface is introduced, its
>> > > destructor should be introduced as well.
>> > >  - Remove the binding procedure from acpi_pci_root_add().  This should
>> > > be done in patch [2/6].
>> >
>> > Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind()
>> > somewhere else and removing those things altogether?
>>
>> Sounds nice.  It will be bonus point if you can do that. :-)
>
> I think I can, but I need a few more patches on top of what I've already posted
> to do that.
>
> I think that https://patchwork.kernel.org/patch/1889821/ and
> https://patchwork.kernel.org/patch/1884701/ can stay as they are, since there's
> some material on top of them already and I'll cut the new patches on top of all
> that.  I'll repost the whole series some time later this week, stay tuned. :-)

I haven't follow this closely enough to give useful feedback, but I
trust that what you're doing is going in the right direction.

The only question I have right now is what I mentioned earlier on IRC,
namely, the idea of "binding" an ACPI handle or device to a pci_dev,
and whether there's a way to guarantee that the binding doesn't become
stale.  For example, if we bind pci_dev A to acpi_device B, I think we
essentially capture the pointer to B and store that pointer in A.
Obviously we want to know that the captured pointer in A remains valid
as long as A exists, but I don't know what assures us of that.

I don't think this is a new question; I have the same question about
the current code before your changes.  But it seems like you're
simplifying this area in a way that might make it easier to answer the
question.

Bjorn
--
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
Yinghai Lu Dec. 19, 2012, 1:57 a.m. UTC | #10
On Tue, Dec 18, 2012 at 2:05 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> i think we should put jiang four patches before Rafael's patches.
>>
>> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug
>
> Actually, I have something more radical than that in mind. :-)
>
> Namely, we don't need to call the wakeup-related stuff from acpi_pci_bind() and
> acpi_pci_unbind().  It's only there, because I did't find a better place for it
> when I added it.
>
> If we can set the ACPI handles of PCI devices in pci_scan_device(), which isn't
> too difficult to do (I actually have a patch for that and it's not too invasive),
> we can easily move the wakeup enabling stuff to pci_pm_init() and wakeup
> disabling somewhere near pci_release_capabilities().

good, let's see how acpi handles could be be passed to pci devices.

Yinghai
--
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
Rafael Wysocki Dec. 19, 2012, 11:13 a.m. UTC | #11
On Tuesday, December 18, 2012 04:19:55 PM Bjorn Helgaas wrote:
> On Tue, Dec 18, 2012 at 4:00 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Tuesday, December 18, 2012 03:15:12 PM Toshi Kani wrote:
> >> On Tue, 2012-12-18 at 22:57 +0100, Rafael J. Wysocki wrote:
> >> > On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote:
> >> > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote:
> >>  :
> >> > > We need to decide which module is responsible for calling .bind().  I
> >> > > think it should be the ACPI scan module, not the ACPI PCI root bridge
> >> > > driver, because:
> >> > >  - bind() needs to be called when _ADR device is added.  The ACPI scan
> >> > > module can scan any devices, while the PCI root driver can only scan
> >> > > when it is added.
> >> > >  - acpi_bus_remove() calls unbind() at hot-remove.  The same module
> >> > > should be responsible for both bind() and unbind() handling.
> >> > >  - It is cleaner to keep struct acpi_device_ops interface to be called
> >> > > by the ACPI core.
> >> >
> >> > I agree with that. :-)
> >> >
> >> > Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all.
> >> >
> >> > > So, I would propose the following changes.
> >> > >
> >> > >  - Move the acpi_hot_add_bind() call back to the original place after
> >> > > the device_attach() call.
> >> > >  - Rename the name of acpi_hot_add_bind() to something like
> >> > > acpi_bind_adr_device() since it is no longer hot-add only (and is
> >> > > specific to _ADR devices).
> >> > >  - Create its pair function, acpi_unbind_adr_device(), which is called
> >> > > from acpi_bus_remove().  When a constructor interface is introduced, its
> >> > > destructor should be introduced as well.
> >> > >  - Remove the binding procedure from acpi_pci_root_add().  This should
> >> > > be done in patch [2/6].
> >> >
> >> > Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind()
> >> > somewhere else and removing those things altogether?
> >>
> >> Sounds nice.  It will be bonus point if you can do that. :-)
> >
> > I think I can, but I need a few more patches on top of what I've already posted
> > to do that.
> >
> > I think that https://patchwork.kernel.org/patch/1889821/ and
> > https://patchwork.kernel.org/patch/1884701/ can stay as they are, since there's
> > some material on top of them already and I'll cut the new patches on top of all
> > that.  I'll repost the whole series some time later this week, stay tuned. :-)
> 
> I haven't follow this closely enough to give useful feedback, but I
> trust that what you're doing is going in the right direction.

Cool. :-)

> The only question I have right now is what I mentioned earlier on IRC,
> namely, the idea of "binding" an ACPI handle or device to a pci_dev,
> and whether there's a way to guarantee that the binding doesn't become
> stale.  For example, if we bind pci_dev A to acpi_device B, I think we
> essentially capture the pointer to B and store that pointer in A.
> Obviously we want to know that the captured pointer in A remains valid
> as long as A exists, but I don't know what assures us of that.

This really is a bit more complicated.

First, what we store in A is not a pointer to B, but rather the corresponding
ACPICA handle, which is guaranteed to live longer that B itself.

Second, we not only store the B's handle in A, but also a pointer to A in B
(in the physical_node_list list).  Moreover, we create the "firmware_node"
sysfs file under A and a "physical_node" sysfs file under B.

All of that becomes invalid when either A or B is removed without notice.

For the removal of A we have acpi_platform_notify() that calls
acpi_unbind_one() which kills all of that stuff, so as long as A is removed
earlier than B, we have no problems.

For the removal of B, however, we seem to assume that if the device is
ejectable, there will be an ACPI driver bound to B (ie. the struct acpi_device)
that will take care of the removal of the physical nodes associated with it
before B itself is removed.  At least that's my understanding of the current
code.  Moreover, I'm not sure if this assumption is universally satisfied.

> I don't think this is a new question; I have the same question about
> the current code before your changes.  But it seems like you're
> simplifying this area in a way that might make it easier to answer the
> question.

Well, my patches are not likely to make things worse in this area. :-)

Anyway, I think we should make acpi_bus_scan(), or even acpi_bus_add() after
my changes, mutually exclusive with acpi_bus_trim(), because that will ensure
that we won't be removing ACPI device objects while setting them up (or the
other way around).  That would require us to redesign some ACPI drivers first,
though, because they call acpi_bus_trim() in their initialization code paths
(I don't really think they should be doing that).

Moreover, I think we should make the ACPI core trigger the removal of all
physical nodes (As) associated with the given ACPI node (B) after calling
device_release_driver() in acpi_bus_remove().  That will ensure that all
physical nodes are gone along with all the binding-related stuff (thanks to
acpi_platform_notify()) before the ACPI node is removed.

Thanks,
Rafael
Rafael Wysocki Dec. 20, 2012, 1:45 a.m. UTC | #12
Hi all,

I've finally cut the patches removing acpi_pci_bind() and acpi_pci_unbind(),
so I'm kind of ready to post the entire series reworking the ACPI namespace
scanning code.

Patches [1-6/16] are essentially these ones:

https://patchwork.kernel.org/patch/1889821/
https://patchwork.kernel.org/patch/1876481/
https://patchwork.kernel.org/patch/1876531/
https://patchwork.kernel.org/patch/1876571/
https://patchwork.kernel.org/patch/1876511/
https://patchwork.kernel.org/patch/1876401/

but [2-6/16] have been rebased on top of the first one.

Patches [7-12/16] are essentially these:

https://patchwork.kernel.org/patch/1884721/
https://patchwork.kernel.org/patch/1884701/
https://patchwork.kernel.org/patch/1884761/
https://patchwork.kernel.org/patch/1884731/
https://patchwork.kernel.org/patch/1884751/
https://patchwork.kernel.org/patch/1884661/

but they have been rebased on top of https://patchwork.kernel.org/patch/1889821/.
I added Yinghai's ACKs to them tentatively, although they are a bit different
from the previous versions.  The difference is not too important, however,
because the following patches finally remove the acpi_pci_bind()/acpi_pci_unbind()
stuff:

[13/16] Add .setup() and .cleanup() callbacks to struct acpi_bus_type.
[14/16] Rework the setup and cleanup of ACPI/PCI device wakeup.
[15/16] Move the _PRT setup and cleanup code to pci-acpi.c.
[16/16] Drop ACPI device .bind() and .unbind() callbacks.

This is done a bit differently than I thought it would be, mostly because the
_PRT-related operations require the "subordinate" pointers of bridges to be
populated.  I think it may be possible to simplify this further if that
requirement can be removed (I haven't looked into that).

In fact, patches [13-15/16] do not essentially depend on [1-12/16], only the
last one does.

The patches are on top of my master branch and I'm going to rebase them when
v3.8-rc1 is out.

There is a git tree you can pull them from at:

http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new

It's v3.7 with my master branch merged and the new patches on top.


Thanks,
Rafael
Yinghai Lu Dec. 20, 2012, 2:06 a.m. UTC | #13
On Wed, Dec 19, 2012 at 5:45 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:

great!, the .bind and .unbind go away.

>
> The patches are on top of my master branch and I'm going to rebase them when
> v3.8-rc1 is out.

>
> There is a git tree you can pull them from at:
>
> http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new
>
> It's v3.7 with my master branch merged and the new patches on top.


can you make it base to today's Linus tree ?

otherwise I have to do some manual merge...

Bjorn change the _PRT for v.3.8...

Thanks

Yinghai
--
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
Toshi Kani Dec. 20, 2012, 9:07 p.m. UTC | #14
Hi Rafael,

The changes look very good.  It's much cleaner and consistent. :-)  For
the series:

Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks,
-Toshi


On Thu, 2012-12-20 at 02:45 +0100, Rafael J. Wysocki wrote:
> Hi all,
> 
> I've finally cut the patches removing acpi_pci_bind() and acpi_pci_unbind(),
> so I'm kind of ready to post the entire series reworking the ACPI namespace
> scanning code.
> 
> Patches [1-6/16] are essentially these ones:
> 
> https://patchwork.kernel.org/patch/1889821/
> https://patchwork.kernel.org/patch/1876481/
> https://patchwork.kernel.org/patch/1876531/
> https://patchwork.kernel.org/patch/1876571/
> https://patchwork.kernel.org/patch/1876511/
> https://patchwork.kernel.org/patch/1876401/
> 
> but [2-6/16] have been rebased on top of the first one.
> 
> Patches [7-12/16] are essentially these:
> 
> https://patchwork.kernel.org/patch/1884721/
> https://patchwork.kernel.org/patch/1884701/
> https://patchwork.kernel.org/patch/1884761/
> https://patchwork.kernel.org/patch/1884731/
> https://patchwork.kernel.org/patch/1884751/
> https://patchwork.kernel.org/patch/1884661/
> 
> but they have been rebased on top of https://patchwork.kernel.org/patch/1889821/.
> I added Yinghai's ACKs to them tentatively, although they are a bit different
> from the previous versions.  The difference is not too important, however,
> because the following patches finally remove the acpi_pci_bind()/acpi_pci_unbind()
> stuff:
> 
> [13/16] Add .setup() and .cleanup() callbacks to struct acpi_bus_type.
> [14/16] Rework the setup and cleanup of ACPI/PCI device wakeup.
> [15/16] Move the _PRT setup and cleanup code to pci-acpi.c.
> [16/16] Drop ACPI device .bind() and .unbind() callbacks.
> 
> This is done a bit differently than I thought it would be, mostly because the
> _PRT-related operations require the "subordinate" pointers of bridges to be
> populated.  I think it may be possible to simplify this further if that
> requirement can be removed (I haven't looked into that).
> 
> In fact, patches [13-15/16] do not essentially depend on [1-12/16], only the
> last one does.
> 
> The patches are on top of my master branch and I'm going to rebase them when
> v3.8-rc1 is out.
> 
> There is a git tree you can pull them from at:
> 
> http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new
> 
> It's v3.7 with my master branch merged and the new patches on top.
> 
> 
> Thanks,
> Rafael
> 
> 


--
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
Rafael Wysocki Dec. 20, 2012, 11:20 p.m. UTC | #15
On Thursday, December 20, 2012 02:07:27 PM Toshi Kani wrote:
> Hi Rafael,
> 
> The changes look very good.  It's much cleaner and consistent. :-)  For
> the series:
> 
> Acked-by: Toshi Kani <toshi.kani@hp.com>

Thanks!
Rafael Wysocki Dec. 21, 2012, 12:09 a.m. UTC | #16
On Wednesday, December 19, 2012 06:06:57 PM Yinghai Lu wrote:
> On Wed, Dec 19, 2012 at 5:45 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> great!, the .bind and .unbind go away.
> 
> >
> > The patches are on top of my master branch and I'm going to rebase them when
> > v3.8-rc1 is out.
> 
> >
> > There is a git tree you can pull them from at:
> >
> > http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new
> >
> > It's v3.7 with my master branch merged and the new patches on top.
> 
> 
> can you make it base to today's Linus tree ?

Sure.  Rebased and pushed out:

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-scan-next

Untested, though, so caveat emptor. :-)

> otherwise I have to do some manual merge...
> 
> Bjorn change the _PRT for v.3.8...

Well, not only that has changed.

Thanks,
Rafael
Yinghai Lu Dec. 21, 2012, 3:59 a.m. UTC | #17
On Thu, Dec 20, 2012 at 4:09 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, December 19, 2012 06:06:57 PM Yinghai Lu wrote:
>> On Wed, Dec 19, 2012 at 5:45 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> great!, the .bind and .unbind go away.
>>
>> >
>> > The patches are on top of my master branch and I'm going to rebase them when
>> > v3.8-rc1 is out.
>>
>> >
>> > There is a git tree you can pull them from at:
>> >
>> > http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new
>> >
>> > It's v3.7 with my master branch merged and the new patches on top.
>>
>>
>> can you make it base to today's Linus tree ?
>
> Sure.  Rebased and pushed out:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-scan-next
>

Thanks.

I rebased pci-root-bus-hotplug patchset on top it, and it works well.

So
Acked-by: Yinghai Lu <yinghai@kernel.org>
for the four new ones in your acpi_scan_next branch.

i put the updated patches in

git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git
for-pci-root-bus-3.9
--
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
Rafael Wysocki Dec. 21, 2012, 10:23 p.m. UTC | #18
On Thursday, December 20, 2012 07:59:15 PM Yinghai Lu wrote:
> On Thu, Dec 20, 2012 at 4:09 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, December 19, 2012 06:06:57 PM Yinghai Lu wrote:
> >> On Wed, Dec 19, 2012 at 5:45 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>
> >> great!, the .bind and .unbind go away.
> >>
> >> >
> >> > The patches are on top of my master branch and I'm going to rebase them when
> >> > v3.8-rc1 is out.
> >>
> >> >
> >> > There is a git tree you can pull them from at:
> >> >
> >> > http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new
> >> >
> >> > It's v3.7 with my master branch merged and the new patches on top.
> >>
> >>
> >> can you make it base to today's Linus tree ?
> >
> > Sure.  Rebased and pushed out:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-scan-next
> >
> 
> Thanks.
> 
> I rebased pci-root-bus-hotplug patchset on top it, and it works well.

Great, thanks for testing!

> So
> Acked-by: Yinghai Lu <yinghai@kernel.org>
> for the four new ones in your acpi_scan_next branch.

Thanks!

I'll rebase the whole series on top of v3.8-rc1 when it's out.

> i put the updated patches in
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git
> for-pci-root-bus-3.9

OK, thanks.

I'll have a look at that series next week.

Thanks,
Rafael
diff mbox

Patch

Index: linux/include/acpi/acpi_bus.h
===================================================================
--- linux.orig/include/acpi/acpi_bus.h
+++ linux/include/acpi/acpi_bus.h
@@ -98,6 +98,7 @@  typedef void (*acpi_op_notify) (struct a
 struct acpi_bus_ops {
 	u32 acpi_op_add:1;
 	u32 acpi_op_start:1;
+	u32 acpi_op_match:1;
 };
 
 struct acpi_device_ops {
Index: linux/drivers/acpi/scan.c
===================================================================
--- linux.orig/drivers/acpi/scan.c
+++ linux/drivers/acpi/scan.c
@@ -494,7 +494,8 @@  static int acpi_bus_match(struct device
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 	struct acpi_driver *acpi_drv = to_acpi_driver(drv);
 
-	return !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
+	return acpi_dev->bus_ops.acpi_op_match
+		&& !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
 }
 
 static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env)
@@ -1418,6 +1419,17 @@  static int acpi_bus_remove(struct acpi_d
 	return 0;
 }
 
+/*
+ * acpi_hot_add_bind - Bind _ADR-based devices on hot-add.
+ * @device: ACPI device node to bind.
+ */
+static void acpi_hot_add_bind(struct acpi_device *device)
+{
+	if (device->flags.bus_address
+	    && device->parent && device->parent->ops.bind)
+		device->parent->ops.bind(device);
+}
+
 static int acpi_add_single_object(struct acpi_device **child,
 				  acpi_handle handle, int type,
 				  unsigned long long sta,
@@ -1490,13 +1502,8 @@  static int acpi_add_single_object(struct
 
 	result = acpi_device_register(device);
 
-	/*
-	 * Bind _ADR-Based Devices when hot add
-	 */
-	if (device->flags.bus_address) {
-		if (device->parent && device->parent->ops.bind)
-			device->parent->ops.bind(device);
-	}
+	if (device->bus_ops.acpi_op_match)
+		acpi_hot_add_bind(device);
 
 end:
 	if (!result) {
@@ -1522,6 +1529,7 @@  static void acpi_bus_add_power_resource(
 	struct acpi_bus_ops ops = {
 		.acpi_op_add = 1,
 		.acpi_op_start = 1,
+		.acpi_op_match = 1,
 	};
 	struct acpi_device *device = NULL;
 
@@ -1574,9 +1582,9 @@  static acpi_status acpi_bus_check_add(ac
 				      void *context, void **return_value)
 {
 	struct acpi_bus_ops *ops = context;
+	struct acpi_device *device = NULL;
 	int type;
 	unsigned long long sta;
-	struct acpi_device *device;
 	acpi_status status;
 	int result;
 
@@ -1596,52 +1604,84 @@  static acpi_status acpi_bus_check_add(ac
 		return AE_CTRL_DEPTH;
 	}
 
-	/*
-	 * We may already have an acpi_device from a previous enumeration.  If
-	 * so, we needn't add it again, but we may still have to start it.
-	 */
-	device = NULL;
 	acpi_bus_get_device(handle, &device);
 	if (ops->acpi_op_add && !device) {
-		acpi_add_single_object(&device, handle, type, sta, ops);
-		/* Is the device a known good platform device? */
-		if (device
-		    && !acpi_match_device_ids(device, acpi_platform_device_ids))
-			acpi_create_platform_device(device);
-	}
-
-	if (!device)
-		return AE_CTRL_DEPTH;
+		struct acpi_bus_ops add_ops = *ops;
 
-	if (ops->acpi_op_start && !(ops->acpi_op_add)) {
-		status = acpi_start_single_object(device);
-		if (ACPI_FAILURE(status))
+		add_ops.acpi_op_match = 0;
+		acpi_add_single_object(&device, handle, type, sta, &add_ops);
+		if (!device)
 			return AE_CTRL_DEPTH;
+
+		device->bus_ops.acpi_op_match = 1;
+		acpi_hot_add_bind(device);
 	}
 
 	if (!*return_value)
 		*return_value = device;
+
 	return AE_OK;
 }
 
+static acpi_status acpi_bus_probe_start(acpi_handle handle, u32 lvl,
+					void *context, void **not_used)
+{
+	struct acpi_bus_ops *ops = context;
+	acpi_status status = AE_OK;
+	struct acpi_device *device;
+	unsigned long long sta_not_used;
+	int type_not_used;
+
+	/*
+	 * Ignore errors ignored by acpi_bus_check_add() to avoid terminating
+	 * namespace walks prematurely.
+	 */
+	if (acpi_bus_type_and_status(handle, &type_not_used, &sta_not_used))
+		return AE_OK;
+
+	if (acpi_bus_get_device(handle, &device))
+		return AE_CTRL_DEPTH;
+
+	if (ops->acpi_op_add) {
+		if (!acpi_match_device_ids(device, acpi_platform_device_ids)) {
+			/* This is a known good platform device. */
+			acpi_create_platform_device(device);
+		} else if (device_attach(&device->dev)) {
+			status = AE_CTRL_DEPTH;
+		}
+	} else if (ops->acpi_op_start) {
+		if (ACPI_FAILURE(acpi_start_single_object(device)))
+			status = AE_CTRL_DEPTH;
+	}
+	return status;
+}
+
 static int acpi_bus_scan(acpi_handle handle, struct acpi_bus_ops *ops,
 			 struct acpi_device **child)
 {
-	acpi_status status;
 	void *device = NULL;
+	acpi_status status;
+	int ret = -ENODEV;
 
 	status = acpi_bus_check_add(handle, 0, ops, &device);
 	if (ACPI_SUCCESS(status))
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
 				    acpi_bus_check_add, NULL, ops, &device);
 
+	if (!device)
+		goto out;
+
+	ret = 0;
+	status = acpi_bus_probe_start(handle, 0, ops, NULL);
+	if (ACPI_SUCCESS(status))
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_probe_start, NULL, ops, NULL);
+
+ out:
 	if (child)
 		*child = device;
 
-	if (device)
-		return 0;
-	else
-		return -ENODEV;
+	return ret;
 }
 
 /*
@@ -1752,6 +1792,7 @@  static int acpi_bus_scan_fixed(void)
 	memset(&ops, 0, sizeof(ops));
 	ops.acpi_op_add = 1;
 	ops.acpi_op_start = 1;
+	ops.acpi_op_match = 1;
 
 	/*
 	 * Enumerate all fixed-feature devices.