Re: [PATCH 2/2] ACPI / scan: Simplify container driver

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday, February 07, 2013 05:32:15 PM Yasuaki Ishimatsu wrote:
> Hi Rafael,
> 
> Sorry for late reply.

No problem, thank you for the review.

> 2013/02/04 8:47, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The only useful thing that the ACPI container driver does is to
> > install system notify handlers for all container and module device
> > objects it finds in the namespace.  The driver structure,
> > acpi_container_driver, and the data structures created by its
> > .add() callback are in fact not used by the driver, so remove
> > them entirely.
> >
> > It also makes a little sense to build that driver as a module,
> > so make it non-modular and add its initialization to the
> > namespace scanning code.
> >
> > In addition to that, make the namespace walk callback used for
> > installing the notify handlers more straightforward.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >   drivers/acpi/Kconfig     |    2
> >   drivers/acpi/container.c |  158 ++++++-----------------------------------------
> >   drivers/acpi/internal.h  |    5 +
> >   drivers/acpi/scan.c      |    1
> >   4 files changed, 30 insertions(+), 136 deletions(-)
> >
> > Index: test/drivers/acpi/container.c
> > ===================================================================
> > --- test.orig/drivers/acpi/container.c
> > +++ test/drivers/acpi/container.c
> > @@ -38,41 +38,15 @@
> >
> >   #define PREFIX "ACPI: "
> >
> > -#define ACPI_CONTAINER_DEVICE_NAME	"ACPI container device"
> > -#define ACPI_CONTAINER_CLASS		"container"
> > -
> > -#define INSTALL_NOTIFY_HANDLER		1
> > -#define UNINSTALL_NOTIFY_HANDLER	2
> > -
> >   #define _COMPONENT			ACPI_CONTAINER_COMPONENT
> >   ACPI_MODULE_NAME("container");
> >
> > -MODULE_AUTHOR("Anil S Keshavamurthy");
> > -MODULE_DESCRIPTION("ACPI container driver");
> > -MODULE_LICENSE("GPL");
> > -
> > -static int acpi_container_add(struct acpi_device *device);
> > -static int acpi_container_remove(struct acpi_device *device);
> > -
> >   static const struct acpi_device_id container_device_ids[] = {
> >   	{"ACPI0004", 0},
> >   	{"PNP0A05", 0},
> >   	{"PNP0A06", 0},
> >   	{"", 0},
> >   };
> > -MODULE_DEVICE_TABLE(acpi, container_device_ids);
> > -
> > -static struct acpi_driver acpi_container_driver = {
> > -	.name = "container",
> > -	.class = ACPI_CONTAINER_CLASS,
> > -	.ids = container_device_ids,
> > -	.ops = {
> > -		.add = acpi_container_add,
> > -		.remove = acpi_container_remove,
> > -		},
> > -};
> > -
> > -/*******************************************************************/
> >
> >   static int is_device_present(acpi_handle handle)
> >   {
> > @@ -92,49 +66,6 @@ static int is_device_present(acpi_handle
> >   	return ((sta & ACPI_STA_DEVICE_PRESENT) == ACPI_STA_DEVICE_PRESENT);
> >   }
> >
> > -static bool is_container_device(const char *hid)
> > -{
> > -	const struct acpi_device_id *container_id;
> > -
> > -	for (container_id = container_device_ids;
> > -	     container_id->id[0]; container_id++) {
> > -		if (!strcmp((char *)container_id->id, hid))
> > -			return true;
> > -	}
> > -
> > -	return false;
> > -}
> > -
> > -/*******************************************************************/
> > -static int acpi_container_add(struct acpi_device *device)
> > -{
> > -	struct acpi_container *container;
> > -
> > -	container = kzalloc(sizeof(struct acpi_container), GFP_KERNEL);
> > -	if (!container)
> > -		return -ENOMEM;
> > -
> > -	container->handle = device->handle;
> > -	strcpy(acpi_device_name(device), ACPI_CONTAINER_DEVICE_NAME);
> > -	strcpy(acpi_device_class(device), ACPI_CONTAINER_CLASS);
> > -	device->driver_data = container;
> > -
> > -	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device <%s> bid <%s>\n",
> > -			  acpi_device_name(device), acpi_device_bid(device)));
> > -
> > -	return 0;
> > -}
> > -
> > -static int acpi_container_remove(struct acpi_device *device)
> > -{
> > -	acpi_status status = AE_OK;
> > -	struct acpi_container *pc = NULL;
> > -
> > -	pc = acpi_driver_data(device);
> > -	kfree(pc);
> > -	return status;
> > -}
> > -
> >   static void container_notify_cb(acpi_handle handle, u32 type, void *context)
> >   {
> >   	struct acpi_device *device = NULL;
> > @@ -199,84 +130,41 @@ static void container_notify_cb(acpi_han
> >   	return;
> >   }
> >
> > -static acpi_status
> > -container_walk_namespace_cb(acpi_handle handle,
> > -			    u32 lvl, void *context, void **rv)
> > +static bool is_container(acpi_handle handle)
> >   {
> > -	char *hid = NULL;
> >   	struct acpi_device_info *info;
> > -	acpi_status status;
> > -	int *action = context;
> > +	bool ret = false;
> >
> > -	status = acpi_get_object_info(handle, &info);
> > -	if (ACPI_FAILURE(status)) {
> > -		return AE_OK;
> > -	}
> > -
> > -	if (info->valid & ACPI_VALID_HID)
> > -		hid = info->hardware_id.string;
> > -
> > -	if (hid == NULL) {
> > -		goto end;
> > -	}
> > +	if (ACPI_FAILURE(acpi_get_object_info(handle, &info)))
> > +		return false;
> >
> > -	if (!is_container_device(hid))
> > -		goto end;
> > +	if (info->valid & ACPI_VALID_HID) {
> > +		const struct acpi_device_id *id;
> >
> > -	switch (*action) {
> > -	case INSTALL_NOTIFY_HANDLER:
> > -		acpi_install_notify_handler(handle,
> > -					    ACPI_SYSTEM_NOTIFY,
> > -					    container_notify_cb, NULL);
> > -		break;
> > -	case UNINSTALL_NOTIFY_HANDLER:
> > -		acpi_remove_notify_handler(handle,
> > -					   ACPI_SYSTEM_NOTIFY,
> > -					   container_notify_cb);
> > -		break;
> > -	default:
> > -		break;
> > +		for (id = container_device_ids; id->id[0]; id++) {
> > +			ret = !strcmp((char *)id->id, info->hardware_id.string);
> > +			if (ret)
> > +				break;
> > +		}
> >   	}
> > -
> > -      end:
> >   	kfree(info);
> > -
> > -	return AE_OK;
> > +	return ret;
> >   }
> >
> > -static int __init acpi_container_init(void)
> > +static acpi_status acpi_container_register_notify_handler(acpi_handle handle,
> > +							  u32 lvl, void *ctxt,
> > +							  void **retv)
> >   {
> > -	int result = 0;
> > -	int action = INSTALL_NOTIFY_HANDLER;
> > -
> > -	result = acpi_bus_register_driver(&acpi_container_driver);
> > -	if (result < 0) {
> > -		return (result);
> > -	}
> > -
> > -	/* register notify handler to every container device */
> > -	acpi_walk_namespace(ACPI_TYPE_DEVICE,
> > -			    ACPI_ROOT_OBJECT,
> > -			    ACPI_UINT32_MAX,
> > -			    container_walk_namespace_cb, NULL, &action, NULL);
> > +	if (is_container(handle))
> > +		acpi_install_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
> > +					    container_notify_cb, NULL);
> >
> > -	return (0);
> > +	return AE_OK;
> >   }
> >
> > -static void __exit acpi_container_exit(void)
> > +void __init acpi_container_init(void)
> >   {
> > -	int action = UNINSTALL_NOTIFY_HANDLER;
> > -
> > -
> > -	acpi_walk_namespace(ACPI_TYPE_DEVICE,
> > -			    ACPI_ROOT_OBJECT,
> > -			    ACPI_UINT32_MAX,
> > -			    container_walk_namespace_cb, NULL, &action, NULL);
> > -
> > -	acpi_bus_unregister_driver(&acpi_container_driver);
> > -
> > -	return;
> > +	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
> > +			    acpi_container_register_notify_handler, NULL,
> > +			    NULL, NULL);
> >   }
> > -
> > -module_init(acpi_container_init);
> > -module_exit(acpi_container_exit);
> > Index: test/drivers/acpi/Kconfig
> > ===================================================================
> > --- test.orig/drivers/acpi/Kconfig
> > +++ test/drivers/acpi/Kconfig
> > @@ -334,7 +334,7 @@ config X86_PM_TIMER
> >   	  systems require this timer.
> >
> >   config ACPI_CONTAINER
> > -	tristate "Container and Module Devices (EXPERIMENTAL)"
> > +	bool "Container and Module Devices (EXPERIMENTAL)"
> >   	depends on EXPERIMENTAL
> >   	default (ACPI_HOTPLUG_MEMORY || ACPI_HOTPLUG_CPU || ACPI_HOTPLUG_IO)
> >   	help
> > Index: test/drivers/acpi/internal.h
> > ===================================================================
> > --- test.orig/drivers/acpi/internal.h
> > +++ test/drivers/acpi/internal.h
> > @@ -40,6 +40,11 @@ void acpi_memory_hotplug_init(void);
> >   #else
> >   static inline void acpi_memory_hotplug_init(void) {}
> >   #endif
> 
> > +#ifdef ACPI_CONTAINER
> 
> It should be CONFIG_ACPI_CONTAINER.

Totally correct.

> By this, acpi_container_init() do nothing. When I fix it and test the patch,
> the patch goes well.
> 
> If you update the patch, I'll test again.

This fix from Toshi Kani is necessary in addition to it, though

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

because FORCE_EJECT is never set as far as I can say.

Thanks,
Rafael



-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.