[PATCH 00/02] ACPI: sysfs eject support for container

[PATCH 00/02] ACPI: sysfs eject support for container

[Toshi Kani]

This patchset enables sysfs eject for ACPI scan drivers and
registers the ACPI container driver as an ACPI scan driver.
This patchset is the one I mentioned in my following reply.

Toshi

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

On Mon, 2013-02-04 at 00:47 +0100, 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.

I think the container driver needs to be registered as an ACPI scan
driver so that sysfs eject will continue to work for container devices,
such as ACPI0004:XX/eject.  Since the container driver does not support
ACPI eject notification (and we have been discussing how system device
hot-plug should work), this sysfs eject is the only way to eject a
container device at this point.  I will send an update patchset that
applies on top of this patch.

[PATCH 01/02] ACPI: sysfs eject support for ACPI scan handlers

[Toshi Kani]

Changed sysfs eject, acpi_eject_store(), to support ACPI scan handlers.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 drivers/acpi/scan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index cfd7a69..3ff632e 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -203,7 +203,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 		return -EINVAL;
 	}
 #ifndef FORCE_EJECT
-	if (acpi_device->driver == NULL) {
+	if (!acpi_device->driver && !acpi_device->handler) {
 		ret = -ENODEV;
 		goto err;
 	}

Re: [PATCH 01/02] ACPI: sysfs eject support for ACPI scan handlers

[Yasuaki Ishimatsu]

Hi Toshi,

2013/02/07 7:50, Toshi Kani wrote:
> Changed sysfs eject, acpi_eject_store(), to support ACPI scan handlers.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>   drivers/acpi/scan.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index cfd7a69..3ff632e 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -203,7 +203,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
>   		return -EINVAL;
>   	}
>   #ifndef FORCE_EJECT

> -	if (acpi_device->driver == NULL) {
> +	if (!acpi_device->driver && !acpi_device->handler) {

I don't understand the fix.

The if sentence becomes true, when both acpi_device->driver and acpi_device->handler
are NULL. It means that acpi_eject_store() runs if either acpi_device->driver or
acpi_device->handler has pointer. Is it O.K.?

I think it should be if (!acpi_device->driver || !acpi_device->handler).

Thanks,
Yasuaki Ishimatsu

>   		ret = -ENODEV;
>   		goto err;
>   	}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 01/02] ACPI: sysfs eject support for ACPI scan handlers

[Yasuaki Ishimatsu]

Hi Toshi,

2013/02/07 7:50, Toshi Kani wrote:
> Changed sysfs eject, acpi_eject_store(), to support ACPI scan handlers.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>   drivers/acpi/scan.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index cfd7a69..3ff632e 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -203,7 +203,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
>   		return -EINVAL;
>   	}
>   #ifndef FORCE_EJECT

> -	if (acpi_device->driver == NULL) {
> +	if (!acpi_device->driver && !acpi_device->handler) {

I don't understand the fix.

The if sentence becomes true, when both acpi_device->driver and acpi_device->handler
are NULL. It means that acpi_eject_store() runs if either acpi_device->driver or
acpi_device->handler has pointer. Is it O.K.?

I think it should be if (!acpi_device->driver || !acpi_device->handler).

Thanks,
Yasuaki Ishimatsu

>   		ret = -ENODEV;
>   		goto err;
>   	}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 01/02] ACPI: sysfs eject support for ACPI scan handlers

[Toshi Kani]

On Fri, 2013-02-08 at 09:50 +0900, Yasuaki Ishimatsu wrote:
> Hi Toshi,
> 
> 2013/02/07 7:50, Toshi Kani wrote:
> > Changed sysfs eject, acpi_eject_store(), to support ACPI scan handlers.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > ---
> >   drivers/acpi/scan.c |    2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index cfd7a69..3ff632e 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -203,7 +203,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
> >   		return -EINVAL;
> >   	}
> >   #ifndef FORCE_EJECT
> 
> > -	if (acpi_device->driver == NULL) {
> > +	if (!acpi_device->driver && !acpi_device->handler) {
> 
> I don't understand the fix.
> 
> The if sentence becomes true, when both acpi_device->driver and acpi_device->handler
> are NULL. It means that acpi_eject_store() runs if either acpi_device->driver or
> acpi_device->handler has pointer. Is it O.K.?

Yes.

> I think it should be if (!acpi_device->driver || !acpi_device->handler).

No, the condition has to be "&&" because an acpi_device is _either_
bound to an ACPI driver or an ACPI scan handler. 

Thanks,
-Toshi

Re: [PATCH 01/02] ACPI: sysfs eject support for ACPI scan handlers

[Yasuaki Ishimatsu]

2013/02/08 10:10, Toshi Kani wrote:
> On Fri, 2013-02-08 at 09:50 +0900, Yasuaki Ishimatsu wrote:
>> Hi Toshi,
>>
>> 2013/02/07 7:50, Toshi Kani wrote:
>>> Changed sysfs eject, acpi_eject_store(), to support ACPI scan handlers.
>>>
>>> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
>>> ---
>>>    drivers/acpi/scan.c |    2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>> index cfd7a69..3ff632e 100644
>>> --- a/drivers/acpi/scan.c
>>> +++ b/drivers/acpi/scan.c
>>> @@ -203,7 +203,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
>>>    		return -EINVAL;
>>>    	}
>>>    #ifndef FORCE_EJECT
>>
>>> -	if (acpi_device->driver == NULL) {
>>> +	if (!acpi_device->driver && !acpi_device->handler) {
>>
>> I don't understand the fix.
>>
>> The if sentence becomes true, when both acpi_device->driver and acpi_device->handler
>> are NULL. It means that acpi_eject_store() runs if either acpi_device->driver or
>> acpi_device->handler has pointer. Is it O.K.?
>
> Yes.
>
>> I think it should be if (!acpi_device->driver || !acpi_device->handler).
>
> No, the condition has to be "&&" because an acpi_device is _either_
> bound to an ACPI driver or an ACPI scan handler.

Thank you for you clarification. I understood it.

Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

Thanks,
Yasuaki Ishimatsu

>
> Thanks,
> -Toshi
>
>

Re: [PATCH 01/02] ACPI: sysfs eject support for ACPI scan handlers

[Yasuaki Ishimatsu]

2013/02/08 10:10, Toshi Kani wrote:
> On Fri, 2013-02-08 at 09:50 +0900, Yasuaki Ishimatsu wrote:
>> Hi Toshi,
>>
>> 2013/02/07 7:50, Toshi Kani wrote:
>>> Changed sysfs eject, acpi_eject_store(), to support ACPI scan handlers.
>>>
>>> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
>>> ---
>>>    drivers/acpi/scan.c |    2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>> index cfd7a69..3ff632e 100644
>>> --- a/drivers/acpi/scan.c
>>> +++ b/drivers/acpi/scan.c
>>> @@ -203,7 +203,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
>>>    		return -EINVAL;
>>>    	}
>>>    #ifndef FORCE_EJECT
>>
>>> -	if (acpi_device->driver == NULL) {
>>> +	if (!acpi_device->driver && !acpi_device->handler) {
>>
>> I don't understand the fix.
>>
>> The if sentence becomes true, when both acpi_device->driver and acpi_device->handler
>> are NULL. It means that acpi_eject_store() runs if either acpi_device->driver or
>> acpi_device->handler has pointer. Is it O.K.?
>
> Yes.
>
>> I think it should be if (!acpi_device->driver || !acpi_device->handler).
>
> No, the condition has to be "&&" because an acpi_device is _either_
> bound to an ACPI driver or an ACPI scan handler.

Thank you for you clarification. I understood it.

Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

Thanks,
Yasuaki Ishimatsu

>
> Thanks,
> -Toshi
>
>

Re: [PATCH 01/02] ACPI: sysfs eject support for ACPI scan handlers

[Toshi Kani]

On Fri, 2013-02-08 at 10:33 +0900, Yasuaki Ishimatsu wrote:
> 2013/02/08 10:10, Toshi Kani wrote:
> > On Fri, 2013-02-08 at 09:50 +0900, Yasuaki Ishimatsu wrote:
> >> Hi Toshi,
> >>
> >> 2013/02/07 7:50, Toshi Kani wrote:
> >>> Changed sysfs eject, acpi_eject_store(), to support ACPI scan handlers.
> >>>
> >>> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> >>> ---
> >>>    drivers/acpi/scan.c |    2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> >>> index cfd7a69..3ff632e 100644
> >>> --- a/drivers/acpi/scan.c
> >>> +++ b/drivers/acpi/scan.c
> >>> @@ -203,7 +203,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
> >>>    		return -EINVAL;
> >>>    	}
> >>>    #ifndef FORCE_EJECT
> >>
> >>> -	if (acpi_device->driver == NULL) {
> >>> +	if (!acpi_device->driver && !acpi_device->handler) {
> >>
> >> I don't understand the fix.
> >>
> >> The if sentence becomes true, when both acpi_device->driver and acpi_device->handler
> >> are NULL. It means that acpi_eject_store() runs if either acpi_device->driver or
> >> acpi_device->handler has pointer. Is it O.K.?
> >
> > Yes.
> >
> >> I think it should be if (!acpi_device->driver || !acpi_device->handler).
> >
> > No, the condition has to be "&&" because an acpi_device is _either_
> > bound to an ACPI driver or an ACPI scan handler.
> 
> Thank you for you clarification. I understood it.
> 
> Acked-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>

Thanks Yasuaki!
-Toshi


> 
> Thanks,
> Yasuaki Ishimatsu
> 
> >
> > Thanks,
> > -Toshi
> >
> >
> 
> 

[PATCH 02/02] ACPI: Register container drv as scan driver

[Toshi Kani]

Changed to register the ACPI container driver as an ACPI scan driver
so that sysfs eject of container devices works.

Fixed a typo in the acpi_container_init() prototype definition.
Removed struct acpi_container, which is no longer used.

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

This patch applies on top of the patch below.
https://lkml.org/lkml/2013/2/3/318

---
 drivers/acpi/container.c |   23 +++++++++++++++++++++++
 drivers/acpi/internal.h  |    2 +-
 include/acpi/container.h |    8 --------
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
index bcde1f3..f493180 100644
--- a/drivers/acpi/container.c
+++ b/drivers/acpi/container.c
@@ -41,6 +41,10 @@
 #define _COMPONENT			ACPI_CONTAINER_COMPONENT
 ACPI_MODULE_NAME("container");
 
+static int container_device_attach(struct acpi_device *device,
+				const struct acpi_device_id *not_used);
+static void container_device_detach(struct acpi_device *device);
+
 static const struct acpi_device_id container_device_ids[] = {
 	{"ACPI0004", 0},
 	{"PNP0A05", 0},
@@ -48,6 +52,12 @@ static const struct acpi_device_id container_device_ids[] = {
 	{"", 0},
 };
 
+static struct acpi_scan_handler container_device_handler = {
+	.ids = container_device_ids,
+	.attach = container_device_attach,
+	.detach = container_device_detach,
+};
+
 static int is_device_present(acpi_handle handle)
 {
 	acpi_handle temp;
@@ -66,6 +76,17 @@ static int is_device_present(acpi_handle handle)
 	return ((sta & ACPI_STA_DEVICE_PRESENT) == ACPI_STA_DEVICE_PRESENT);
 }
 
+static int container_device_attach(struct acpi_device *device,
+				const struct acpi_device_id *not_used)
+{
+	return 1;
+}
+
+static void container_device_detach(struct acpi_device *device)
+{
+	return;
+}
+
 static void container_notify_cb(acpi_handle handle, u32 type, void *context)
 {
 	struct acpi_device *device = NULL;
@@ -167,4 +188,6 @@ void __init acpi_container_init(void)
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX,
 			    acpi_container_register_notify_handler, NULL,
 			    NULL, NULL);
+
+	acpi_scan_add_handler(&container_device_handler);
 }
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 78fbf26..cbc0638 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -40,7 +40,7 @@ void acpi_memory_hotplug_init(void);
 #else
 static inline void acpi_memory_hotplug_init(void) {}
 #endif
-#ifdef ACPI_CONTAINER
+#ifdef CONFIG_ACPI_CONTAINER
 void acpi_container_init(void);
 #else
 static inline void acpi_container_init(void) {}
diff --git a/include/acpi/container.h b/include/acpi/container.h
index a703f14..0a464d2 100644
--- a/include/acpi/container.h
+++ b/include/acpi/container.h
@@ -1,12 +1,4 @@
 #ifndef __ACPI_CONTAINER_H
 #define __ACPI_CONTAINER_H
 
-#include <linux/kernel.h>
-
-struct acpi_container {
-	acpi_handle handle;
-	unsigned long sun;
-	int state;
-};
-
 #endif				/* __ACPI_CONTAINER_H */