Date:	Tue, 29 Jan 2013 11:04:26 +0900
From:	Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.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>,
	Toshi Kani <toshi.kani@...com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] ACPI / scan: Introduce struct acpi_scan_handler

Hi Rafael,

2013/01/28 21:59, 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>
> ---
Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.com>

I have a comment. Please see below.

>   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;
> +}
> +
>   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;

acpi_scan_attach_hanlder() returns only 0 or -EINVAL.
How about just return AE_CTRL_DEPTH?

Thanks,
Yasuaki Ishimatsu

> +
> +	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-acpi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--
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