Date:	Mon, 28 Jan 2013 19:35:39 -0700
From:	Toshi Kani <toshi.kani@...com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Matthew Garrett <matthew.garrett@...ula.com>,
	Yinghai Lu <yinghai@...nel.org>, Jiang Liu <liuj97@...il.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] ACPI / scan: Introduce struct acpi_scan_handler

On Mon, 2013-01-28 at 13:59 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> 
> Introduce struct acpi_scan_handler for representing objects that
> will do configuration tasks depending on ACPI device nodes'
> hardware IDs (HIDs).
> 
> Currently, those tasks are done either directly by the ACPI namespace
> scanning code or by ACPI device drivers designed specifically for
> this purpose.  None of the above is desirable, however, because
> doing that directly in the namespace scanning code makes that code
> overly complicated and difficult to follow and doing that in
> "special" device drivers leads to a great deal of confusion about
> their role and to confusing interactions with the driver core (for
> example, sysfs directories are created for those drivers, but they
> are completely unnecessary and only increase the kernel's memory
> footprint in vain).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>  Documentation/acpi/scan_handlers.txt |   77 +++++++++++++++++++++++++++++++++++
>  drivers/acpi/scan.c                  |   60 ++++++++++++++++++++++++---
>  include/acpi/acpi_bus.h              |   14 ++++++
>  3 files changed, 144 insertions(+), 7 deletions(-)
> 
> Index: test/include/acpi/acpi_bus.h
> ===================================================================
> --- test.orig/include/acpi/acpi_bus.h
> +++ test/include/acpi/acpi_bus.h
> @@ -84,6 +84,18 @@ struct acpi_driver;
>  struct acpi_device;
>  
>  /*
> + * ACPI Scan Handler
> + * -----------------
> + */
> +
> +struct acpi_scan_handler {
> +	const struct acpi_device_id *ids;
> +	struct list_head list_node;
> +	int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id);
> +	void (*detach)(struct acpi_device *dev);
> +};
> +
> +/*
>   * ACPI Driver
>   * -----------
>   */
> @@ -269,6 +281,7 @@ struct acpi_device {
>  	struct acpi_device_wakeup wakeup;
>  	struct acpi_device_perf performance;
>  	struct acpi_device_dir dir;
> +	struct acpi_scan_handler *handler;
>  	struct acpi_driver *driver;
>  	void *driver_data;
>  	struct device dev;
> @@ -382,6 +395,7 @@ int acpi_bus_receive_event(struct acpi_b
>  static inline int acpi_bus_generate_proc_event(struct acpi_device *device, u8 type, int data)
>  	{ return 0; }
>  #endif
> +int acpi_scan_add_handler(struct acpi_scan_handler *handler);
>  int acpi_bus_register_driver(struct acpi_driver *driver);
>  void acpi_bus_unregister_driver(struct acpi_driver *driver);
>  int acpi_bus_scan(acpi_handle handle);
> Index: test/drivers/acpi/scan.c
> ===================================================================
> --- test.orig/drivers/acpi/scan.c
> +++ test/drivers/acpi/scan.c
> @@ -53,6 +53,7 @@ static const struct acpi_device_id acpi_
>  static LIST_HEAD(acpi_device_list);
>  static LIST_HEAD(acpi_bus_id_list);
>  static DEFINE_MUTEX(acpi_scan_lock);
> +static LIST_HEAD(acpi_scan_handlers_list);
>  DEFINE_MUTEX(acpi_device_lock);
>  LIST_HEAD(acpi_wakeup_device_list);
>  
> @@ -62,6 +63,15 @@ struct acpi_device_bus_id{
>  	struct list_head node;
>  };
>  
> +int acpi_scan_add_handler(struct acpi_scan_handler *handler)
> +{
> +	if (!handler || !handler->attach)
> +		return -EINVAL;
> +
> +	list_add_tail(&handler->list_node, &acpi_scan_handlers_list);
> +	return 0;
> +}
> +
>  /*
>   * Creates hid/cid(s) string needed for modalias and uevent
>   * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get:
> @@ -1570,20 +1580,42 @@ static acpi_status acpi_bus_check_add(ac
>  	return AE_OK;
>  }
>  
> +static int acpi_scan_attach_handler(struct acpi_device *device)
> +{
> +	struct acpi_scan_handler *handler;
> +	int ret = 0;
> +
> +	list_for_each_entry(handler, &acpi_scan_handlers_list, list_node) {
> +		const struct acpi_device_id *id;
> +
> +		id = __acpi_match_device(device, handler->ids);
> +		if (!id)
> +			continue;
> +
> +		ret = handler->attach(device, id);
> +		if (ret > 0) {
> +			device->handler = handler;
> +			break;
> +		} else if (ret < 0) {
> +			break;
> +		}
> +	}
> +	return ret;
> +}

Now that we have full control over the attach logic, it would be great
if we can update it to match with the ACPI spec -- HID has priority over
CIDs, and CIDs are also listed in their priority.  For example, Device-X
has HID and CID.  In this case, this Device-X should be attached to
Handler-A since it supports HID.  The current logic simply chooses a
handler whichever registered before.  

Device-X: HID PNPID-A, CID PNPID-B
Handler-A: PNPID-A
Handler-B: PNPID-B

So, the attach logic should be something like:

list_for_each_entry(hwid, acpi_device->pnp.ids,) {
	list_for_each_entry(,&acpi_scan_handlers_list,)
		check if this handler supports a given hwid
}


Thanks,
-Toshi


> +
>  static acpi_status acpi_bus_device_attach(acpi_handle handle, u32 lvl_not_used,
>  					  void *not_used, void **ret_not_used)
>  {
>  	const struct acpi_device_id *id;
> -	acpi_status status = AE_OK;
>  	struct acpi_device *device;
>  	unsigned long long sta_not_used;
> -	int type_not_used;
> +	int ret;
>  
>  	/*
>  	 * 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))
> +	if (acpi_bus_type_and_status(handle, &ret, &sta_not_used))
>  		return AE_OK;
>  
>  	if (acpi_bus_get_device(handle, &device))
> @@ -1593,10 +1625,15 @@ static acpi_status acpi_bus_device_attac
>  	if (id) {
>  		/* This is a known good platform device. */
>  		acpi_create_platform_device(device, id->driver_data);
> -	} else if (device_attach(&device->dev) < 0) {
> -		status = AE_CTRL_DEPTH;
> +		return AE_OK;
>  	}
> -	return status;
> +
> +	ret = acpi_scan_attach_handler(device);
> +	if (ret)
> +		return ret > 0 ? AE_OK : AE_CTRL_DEPTH;
> +
> +	ret = device_attach(&device->dev);
> +	return ret >= 0 ? AE_OK : AE_CTRL_DEPTH;
>  }
>  
>  /**
> @@ -1639,8 +1676,17 @@ static acpi_status acpi_bus_device_detac
>  	struct acpi_device *device = NULL;
>  
>  	if (!acpi_bus_get_device(handle, &device)) {
> +		struct acpi_scan_handler *dev_handler = device->handler;
> +
>  		device->removal_type = ACPI_BUS_REMOVAL_EJECT;
> -		device_release_driver(&device->dev);
> +		if (dev_handler) {
> +			if (dev_handler->detach)
> +				dev_handler->detach(device);
> +
> +			device->handler = NULL;
> +		} else {
> +			device_release_driver(&device->dev);
> +		}
>  	}
>  	return AE_OK;
>  }
> Index: test/Documentation/acpi/scan_handlers.txt
> ===================================================================
> --- /dev/null
> +++ test/Documentation/acpi/scan_handlers.txt
> @@ -0,0 +1,77 @@
> +ACPI Scan Handlers
> +
> +Copyright (C) 2012, Intel Corporation
> +Author: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> +
> +During system initialization and ACPI-based device hot-add, the ACPI namespace
> +is scanned in search of device objects that generally represent various pieces
> +of hardware.  This causes a struct acpi_device object to be created and
> +registered with the driver core for every device object in the ACPI namespace
> +and the hierarchy of those struct acpi_device objects reflects the namespace
> +layout (i.e. parent device objects in the namespace are represented by parent
> +struct acpi_device objects and analogously for their children).  Those struct
> +acpi_device objects are referred to as "device nodes" in what follows, but they
> +should not be confused with struct device_node objects used by the Device Trees
> +parsing code (although their role is analogous to the role of those objects).
> +
> +During ACPI-based device hot-remove device nodes representing pieces of hardware
> +being removed are unregistered and deleted.
> +
> +The core ACPI namespace scanning code in drivers/acpi/scan.c carries out basic
> +initialization of device nodes, such as retrieving common configuration
> +information from the device objects represented by them and populating them with
> +appropriate data, but some of them require additional handling after they have
> +been registered.  For example, if the given device node represents a PCI host
> +bridge, its registration should cause the PCI bus under that bridge to be
> +enumerated and PCI devices on that bus to be registered with the driver core.
> +Similarly, if the device node represents a PCI interrupt link, it is necessary
> +to configure that link so that the kernel can use it.
> +
> +Those additional configuration tasks usually depend on the type of the hardware
> +component represented by the given device node which can be determined on the
> +basis of the device node's hardware ID (HID).  They are performed by objects
> +called ACPI scan handlers represented by the following structure:
> +
> +struct acpi_scan_handler {
> +	const struct acpi_device_id *ids;
> +	struct list_head list_node;
> +	int (*attach)(struct acpi_device *dev, const struct acpi_device_id *id);
> +	void (*detach)(struct acpi_device *dev);
> +};
> +
> +where ids is the list of IDs of device nodes the given handler is supposed to
> +take care of, list_node is the hook to the global list of ACPI scan handlers
> +maintained by the ACPI core and the .attach() and .detach() callbacks are
> +executed, respectively, after registration of new device nodes and before
> +unregistration of device nodes the handler attached to previously.
> +
> +The namespace scanning function, acpi_bus_scan(), first registers all of the
> +device nodes in the given namespace scope with the driver core.  Then, it tries
> +to match a scan handler against each of them using the ids arrays of the
> +available scan handlers.  If a matching scan handler is found, its .attach()
> +callback is executed for the given device node.  If that callback returns 1,
> +that means that the handler has claimed the device node and is now responsible
> +for carrying out any additional configuration tasks related to it.  It also will
> +be responsible for preparing the device node for unregistration in that case.
> +The device node's handler field is then populated with the address of the scan
> +handler that has claimed it.
> +
> +If the .attach() callback returns 0, it means that the device node is not
> +interesting to the given scan handler and may be matched against the next scan
> +handler in the list.  If it returns a (negative) error code, that means that
> +the namespace scan should be terminated due to a serious error.  The error code
> +returned should then reflect the type of the error.
> +
> +The namespace trimming function, acpi_bus_trim(), first executes .detach()
> +callbacks from the scan handlers of all device nodes in the given namespace
> +scope (if they have scan handlers).  Next, it unregisters all of the device
> +nodes in that scope.
> +
> +ACPI scan handlers can be added to the list maintained by the ACPI core with the
> +help of the acpi_scan_add_handler() function taking a pointer to the new scan
> +handler as an argument.  The order in which scan handlers are added to the list
> +is the order in which they are matched against device nodes during namespace
> +scans.
> +
> +All scan handles must be added to the list before acpi_bus_scan() is run for the
> +first time and they cannot be removed from it.
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists