[PATCH RFC] support sata odd zero power

[PATCH RFC] support sata odd zero power

[su henry]

>From b221e73b802ccfe5338b575dc0ff3b687acdd6fa Mon Sep 17 00:00:00 2001
From: henry su <henry.su.ati@gmail.com>
Date: Fri, 25 Jun 2010 00:08:42 +0800
Subject: [PATCH RFC] support sata odd zero power

In order to extend the battery life of Mobile PC system, the host should remove
the power supply to the Optical Disc Drive (logical unit) when it detects the
logical unit with no media and tray closed, and the host starts the power
supply to the logical unit when it detects user action to the logical unit that
the power supply is omitted.

The patch evaluates the _PS3 method to remove the power supply to the ODD if
the host detects MEDIUM NOT PRESENT - TRAY CLOSED is reported by REQUEST SENSE
command for a Drawer, Tray or Pop-up type drive, or MEDIUM NOT PRESENT -
TRAY CLOSED for a slot/caddy type drive; on the other hand, the patch evaluates
the _PS0 method to restart the power supply to the ODD when the user
presses the
button on a tray type drive or inserts a CD to a slot/caddy type drive.

PLDS DS-8A5S, pansonic UJ892 and UJ897 are the ODD samples used to test
the patch on AMD SB800 platforms.

Signed-off-by: Henry Su <henry.su.ati@gmail.com>
---
 drivers/scsi/sr.c |  147 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 146 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 0a90abc..9b7389f 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -55,6 +55,7 @@
 #include <scsi/scsi_eh.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_ioctl.h>	/* For the door lock/unlock commands */
+#include <acpi/acpi_bus.h>

 #include "scsi_logging.h"
 #include "sr.h"
@@ -89,6 +90,19 @@ static struct scsi_driver sr_template = {
 	.done			= sr_done,
 };

+static struct sr_zpodd_device_info {
+	acpi_handle handle;
+	mechtype_t mechtype;
+
+#define SR_NO_MEDIA_TIMEOUT	(120*HZ)
+#define SR_IS_ZPODD		0x01
+#define SR_ZPODD_NO_DISK	0x02
+#define SR_ZPODD_NEED_EJECT	0x04
+
+	u32 flags;
+	unsigned long last_jiffies;
+} sr_zpodd_device;
+
 static unsigned long sr_index_bits[SR_DISKS / BITS_PER_LONG];
 static DEFINE_SPINLOCK(sr_index_lock);

@@ -205,6 +219,7 @@ static int sr_media_change(struct
cdrom_device_info *cdi, int slot)
 	struct scsi_cd *cd = cdi->handle;
 	int retval;
 	struct scsi_sense_hdr *sshdr;
+	int isvalid;

 	if (CDSL_CURRENT != slot) {
 		/* no changer support */
@@ -213,7 +228,59 @@ static int sr_media_change(struct
cdrom_device_info *cdi, int slot)

 	sshdr =  kzalloc(sizeof(*sshdr), GFP_KERNEL);
 	retval = sr_test_unit_ready(cd->device, sshdr);
-	if (retval || (scsi_sense_valid(sshdr) &&
+	isvalid = scsi_sense_valid(sshdr);
+
+	if (!(sr_zpodd_device.flags & SR_IS_ZPODD))
+		goto out_not_zpodd_device;
+
+	/* Remove the power supply to the drive with no media and
+	 * tray closed for tray/drawer/pop-up type drive,
+	 * or no media and tray open for caddy/slot type drive.
+	 */
+	switch (sr_zpodd_device.mechtype) {
+	case mechtype_caddy:
+		/* 3a/02(asc/ascq) means MEDIUM NOT PRESENT - TRAY OPEN */
+		if (isvalid && (sshdr->asc  == 0x3a &&
+				sshdr->ascq == 0x02)) {
+			if (!(sr_zpodd_device.flags & SR_ZPODD_NO_DISK)) {
+				sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
+				sr_zpodd_device.last_jiffies = jiffies;
+			} else if (time_after(jiffies,
+					sr_zpodd_device.last_jiffies +
+					SR_NO_MEDIA_TIMEOUT)) {
+				acpi_bus_set_power(sr_zpodd_device.handle,
+							ACPI_STATE_D3);
+			}
+		} else
+			sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
+		break;
+	default:
+		/*tray/drawer/pop-up type*/
+		/* 3a/01(asc/ascq) means MEDIUM NOT PRESENT - TRAY CLOSED */
+		if (isvalid && (sshdr->asc  == 0x3a &&
+				sshdr->ascq == 0x01)) {
+
+			/* Eject the tray for a tray type drive*/
+			if (sr_zpodd_device.flags & SR_ZPODD_NEED_EJECT) {
+				sr_tray_move(cdi, 1);
+				sr_zpodd_device.flags &= ~SR_ZPODD_NEED_EJECT;
+			} else if (!(sr_zpodd_device.flags &
+					SR_ZPODD_NO_DISK)) {
+				sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
+				sr_zpodd_device.last_jiffies = jiffies;
+			} else if (time_after(jiffies,
+					sr_zpodd_device.last_jiffies
+					+ SR_NO_MEDIA_TIMEOUT)) {
+				acpi_bus_set_power(sr_zpodd_device.handle,
+							ACPI_STATE_D3);
+			}
+		} else
+			sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
+		break;
+	}
+
+out_not_zpodd_device:
+	if (retval || (isvalid &&
 		       /* 0x3a is medium not present */
 		       sshdr->asc == 0x3a)) {
 		/* Media not present or unable to test, unit probably not
@@ -786,6 +853,7 @@ static void get_capabilities(struct scsi_cd *cd)
 	}

 	n = data.header_length + data.block_descriptor_length;
+	sr_zpodd_device.mechtype = buffer[n + 6] >> 5;
 	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
 	cd->readcd_known = 1;
 	cd->readcd_cdda = buffer[n + 5] & 0x01;
@@ -898,6 +966,73 @@ static int sr_remove(struct device *dev)
 	return 0;
 }

+/*
+ * Handle ACPI event notification when pressing the button on a
+ * tray type drive or inserting a CD to a caddy/slot type drive
+ */
+static void sr_handle_events(acpi_handle handle, u32 event, void *context)
+{
+	unsigned int state;
+
+	switch (event) {
+	/* FIXME: handle both wake and 0x80 notifications? */
+	case 0x02:
+	case 0x80:
+		acpi_bus_get_power(sr_zpodd_device.handle, &state);
+		if (state == ACPI_STATE_D3) {
+			sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
+
+			acpi_bus_set_power(sr_zpodd_device.handle,
+						ACPI_STATE_D0);
+			udelay(100);
+
+			/* Need to eject the tray after starting
+			 * the power supply to tray type ODD
+			 */
+			if (sr_zpodd_device.mechtype == mechtype_tray)
+				sr_zpodd_device.flags |= SR_ZPODD_NEED_EJECT;
+		}
+	}
+	return;
+}
+
+/*
+ * find the zero power odd device in ACPI namespace
+ * and install the notification handler for the device
+ */
+static acpi_status __init sr_find_zpodd_device(acpi_handle handle,
+		u32 lvl, void *context, void **rv)
+{
+	acpi_status status;
+	char name[ACPI_PATH_SEGMENT_LENGTH];
+	struct acpi_buffer buffer = { sizeof(name), &name };
+	acpi_handle *phandle = (acpi_handle *)context;
+
+	/* FIXME: search the device by object name or _HID? */
+	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
+
+	if (ACPI_SUCCESS(status) && !strncmp("ODDZ", name, sizeof(name) - 1)) {
+		status = acpi_install_notify_handler(handle,
+				ACPI_DEVICE_NOTIFY, sr_handle_events,
+				NULL);
+		if (ACPI_FAILURE(status))
+			printk("sr: failed to register notification handler\n");
+		else
+			sr_zpodd_device.flags |= SR_IS_ZPODD;
+
+		*phandle = handle;
+
+		/* returning non-zero causes the search to stop
+		 * and returns this value to the caller of
+		 * acpi_walk_namespace.
+		 */
+		
+		return 1;
+
+	} else
+		return 0;
+}
+
 static int __init init_sr(void)
 {
 	int rc;
@@ -909,11 +1044,21 @@ static int __init init_sr(void)
 	if (rc)
 		unregister_blkdev(SCSI_CDROM_MAJOR, "sr");

+	acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				ACPI_UINT32_MAX, sr_find_zpodd_device, NULL,
+				&sr_zpodd_device.handle, NULL);
+
 	return rc;
 }

 static void __exit exit_sr(void)
 {
+	if (sr_zpodd_device.flags & SR_IS_ZPODD)
+		acpi_remove_notify_handler(
+			   sr_zpodd_device.handle,
+			   ACPI_DEVICE_NOTIFY,
+			   sr_handle_events);
+
 	scsi_unregister_driver(&sr_template.gendrv);
 	unregister_blkdev(SCSI_CDROM_MAJOR, "sr");
 }
-- 
1.5.6.3

Re: [PATCH RFC] support sata odd zero power

[Tejun Heo]

(cc'ing James)

On 06/25/2010 12:15 PM, su henry wrote:
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -55,6 +55,7 @@
>  #include <scsi/scsi_eh.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_ioctl.h>	/* For the door lock/unlock commands */
> +#include <acpi/acpi_bus.h>
> 
>  #include "scsi_logging.h"
>  #include "sr.h"
> @@ -89,6 +90,19 @@ static struct scsi_driver sr_template = {
>  	.done			= sr_done,
>  };
> 
> +static struct sr_zpodd_device_info {
> +	acpi_handle handle;
> +	mechtype_t mechtype;
> +
> +#define SR_NO_MEDIA_TIMEOUT	(120*HZ)
> +#define SR_IS_ZPODD		0x01
> +#define SR_ZPODD_NO_DISK	0x02
> +#define SR_ZPODD_NEED_EJECT	0x04
> +
> +	u32 flags;

It's more customary to use unsigned int.

> +	unsigned long last_jiffies;
> +} sr_zpodd_device;

Can you please align fields?  Also, single struct for the whole
system?  I haven't read the spec but I don't think anybody would be
defining things like this system-wide.  Right?

> @@ -205,6 +219,7 @@ static int sr_media_change(struct
> cdrom_device_info *cdi, int slot)
>  	struct scsi_cd *cd = cdi->handle;
>  	int retval;
>  	struct scsi_sense_hdr *sshdr;
> +	int isvalid;

bool?

>  	if (CDSL_CURRENT != slot) {
>  		/* no changer support */
> @@ -213,7 +228,59 @@ static int sr_media_change(struct
> cdrom_device_info *cdi, int slot)
> 
>  	sshdr =  kzalloc(sizeof(*sshdr), GFP_KERNEL);
>  	retval = sr_test_unit_ready(cd->device, sshdr);
> -	if (retval || (scsi_sense_valid(sshdr) &&
> +	isvalid = scsi_sense_valid(sshdr);
> +
> +	if (!(sr_zpodd_device.flags & SR_IS_ZPODD))
> +		goto out_not_zpodd_device;
> +
> +	/* Remove the power supply to the drive with no media and
> +	 * tray closed for tray/drawer/pop-up type drive,
> +	 * or no media and tray open for caddy/slot type drive.
> +	 */
> +	switch (sr_zpodd_device.mechtype) {
> +	case mechtype_caddy:
> +		/* 3a/02(asc/ascq) means MEDIUM NOT PRESENT - TRAY OPEN */
> +		if (isvalid && (sshdr->asc  == 0x3a &&
> +				sshdr->ascq == 0x02)) {
> +			if (!(sr_zpodd_device.flags & SR_ZPODD_NO_DISK)) {
> +				sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
> +				sr_zpodd_device.last_jiffies = jiffies;
> +			} else if (time_after(jiffies,
> +					sr_zpodd_device.last_jiffies +
> +					SR_NO_MEDIA_TIMEOUT)) {
> +				acpi_bus_set_power(sr_zpodd_device.handle,
> +							ACPI_STATE_D3);
> +			}
> +		} else
> +			sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
> +		break;

This thing definitely belongs to struct scsi_cd.  What are the
synchronization rules here?  Also, what happens if async notification
is in use?  How does the timer get activated then?

> +	default:
> +		/*tray/drawer/pop-up type*/
> +		/* 3a/01(asc/ascq) means MEDIUM NOT PRESENT - TRAY CLOSED */
> +		if (isvalid && (sshdr->asc  == 0x3a &&
> +				sshdr->ascq == 0x01)) {
> +
> +			/* Eject the tray for a tray type drive*/
> +			if (sr_zpodd_device.flags & SR_ZPODD_NEED_EJECT) {
> +				sr_tray_move(cdi, 1);
> +				sr_zpodd_device.flags &= ~SR_ZPODD_NEED_EJECT;
> +			} else if (!(sr_zpodd_device.flags &
> +					SR_ZPODD_NO_DISK)) {
> +				sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
> +				sr_zpodd_device.last_jiffies = jiffies;
> +			} else if (time_after(jiffies,
> +					sr_zpodd_device.last_jiffies
> +					+ SR_NO_MEDIA_TIMEOUT)) {
> +				acpi_bus_set_power(sr_zpodd_device.handle,
> +							ACPI_STATE_D3);
> +			}
> +		} else
> +			sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
> +		break;
> +	}

It would probably be better to separate decision making and updating
states.

Thanks.

-- 
tejun

Re: [PATCH RFC] support sata odd zero power

[su henry]

On Fri, Jun 25, 2010 at 9:39 PM, Tejun Heo <htejun@gmail.com> wrote:
> (cc'ing James)
>
> On 06/25/2010 12:15 PM, su henry wrote:
>> --- a/drivers/scsi/sr.c
>> +++ b/drivers/scsi/sr.c
>> @@ -55,6 +55,7 @@
>>  #include <scsi/scsi_eh.h>
>>  #include <scsi/scsi_host.h>
>>  #include <scsi/scsi_ioctl.h> /* For the door lock/unlock commands */
>> +#include <acpi/acpi_bus.h>
>>
>>  #include "scsi_logging.h"
>>  #include "sr.h"
>> @@ -89,6 +90,19 @@ static struct scsi_driver sr_template = {
>>       .done                   = sr_done,
>>  };
>>
>> +static struct sr_zpodd_device_info {
>> +     acpi_handle handle;
>> +     mechtype_t mechtype;
>> +
>> +#define SR_NO_MEDIA_TIMEOUT  (120*HZ)
>> +#define SR_IS_ZPODD          0x01
>> +#define SR_ZPODD_NO_DISK     0x02
>> +#define SR_ZPODD_NEED_EJECT  0x04
>> +
>> +     u32 flags;
>
> It's more customary to use unsigned int.
>

Yes, it should be unsigned int.

>> +     unsigned long last_jiffies;
>> +} sr_zpodd_device;
>
> Can you please align fields?  Also, single struct for the whole
> system?  I haven't read the spec but I don't think anybody would be
> defining things like this system-wide.  Right?
>

My original though is to put this structure to cdrom_device_info, but
this structure should be firstly used by acpi_walk_namespace in
sr_init, and cdrom_device_info is allocated in sr_probe, that's why I
define a separate structure for the zero power odd.

>> @@ -205,6 +219,7 @@ static int sr_media_change(struct
>> cdrom_device_info *cdi, int slot)
>>       struct scsi_cd *cd = cdi->handle;
>>       int retval;
>>       struct scsi_sense_hdr *sshdr;
>> +     int isvalid;
>
> bool?
>

The return value of sr_test_unit_ready is int.

>>       if (CDSL_CURRENT != slot) {
>>               /* no changer support */
>> @@ -213,7 +228,59 @@ static int sr_media_change(struct
>> cdrom_device_info *cdi, int slot)
>>
>>       sshdr =  kzalloc(sizeof(*sshdr), GFP_KERNEL);
>>       retval = sr_test_unit_ready(cd->device, sshdr);
>> -     if (retval || (scsi_sense_valid(sshdr) &&
>> +     isvalid = scsi_sense_valid(sshdr);
>> +
>> +     if (!(sr_zpodd_device.flags & SR_IS_ZPODD))
>> +             goto out_not_zpodd_device;
>> +
>> +     /* Remove the power supply to the drive with no media and
>> +      * tray closed for tray/drawer/pop-up type drive,
>> +      * or no media and tray open for caddy/slot type drive.
>> +      */
>> +     switch (sr_zpodd_device.mechtype) {
>> +     case mechtype_caddy:
>> +             /* 3a/02(asc/ascq) means MEDIUM NOT PRESENT - TRAY OPEN */
>> +             if (isvalid && (sshdr->asc  == 0x3a &&
>> +                             sshdr->ascq == 0x02)) {
>> +                     if (!(sr_zpodd_device.flags & SR_ZPODD_NO_DISK)) {
>> +                             sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
>> +                             sr_zpodd_device.last_jiffies = jiffies;
>> +                     } else if (time_after(jiffies,
>> +                                     sr_zpodd_device.last_jiffies +
>> +                                     SR_NO_MEDIA_TIMEOUT)) {
>> +                             acpi_bus_set_power(sr_zpodd_device.handle,
>> +                                                     ACPI_STATE_D3);
>> +                     }
>> +             } else
>> +                     sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
>> +             break;
>
> This thing definitely belongs to struct scsi_cd.  What are the
> synchronization rules here?  Also, what happens if async notification
> is in use?  How does the timer get activated then?

This is a problem, any suggestions? Especially when the system goes to
S3/S4 state.

>
>> +     default:
>> +             /*tray/drawer/pop-up type*/
>> +             /* 3a/01(asc/ascq) means MEDIUM NOT PRESENT - TRAY CLOSED */
>> +             if (isvalid && (sshdr->asc  == 0x3a &&
>> +                             sshdr->ascq == 0x01)) {
>> +
>> +                     /* Eject the tray for a tray type drive*/
>> +                     if (sr_zpodd_device.flags & SR_ZPODD_NEED_EJECT) {
>> +                             sr_tray_move(cdi, 1);
>> +                             sr_zpodd_device.flags &= ~SR_ZPODD_NEED_EJECT;
>> +                     } else if (!(sr_zpodd_device.flags &
>> +                                     SR_ZPODD_NO_DISK)) {
>> +                             sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
>> +                             sr_zpodd_device.last_jiffies = jiffies;
>> +                     } else if (time_after(jiffies,
>> +                                     sr_zpodd_device.last_jiffies
>> +                                     + SR_NO_MEDIA_TIMEOUT)) {
>> +                             acpi_bus_set_power(sr_zpodd_device.handle,
>> +                                                     ACPI_STATE_D3);
>> +                     }
>> +             } else
>> +                     sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
>> +             break;
>> +     }
>
> It would probably be better to separate decision making and updating
> states.

When user pressing the button on a tray type drive,  driver should set
a flag used to eject the tray in sr_media_change, if we use that
states here, we should a one more state "NEED_EJECT" for drive status.

>
> Thanks.
>
> --
> tejun
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] support sata odd zero power

[Tejun Heo]

Hello,

On 06/28/2010 10:43 AM, su henry wrote:
>> Can you please align fields?  Also, single struct for the whole
>> system?  I haven't read the spec but I don't think anybody would be
>> defining things like this system-wide.  Right?
>>
> 
> My original though is to put this structure to cdrom_device_info, but
> this structure should be firstly used by acpi_walk_namespace in
> sr_init, and cdrom_device_info is allocated in sr_probe, that's why I
> define a separate structure for the zero power odd.

What prevents you from walking the acpi device tree from sr_probe()?
Or even if you need to walk it from sr_init(), you still need to store
the result and associate it with a specific cdrom device.  It is
something which is specific to single device.  You can't use single
global data structure for all devices like this.

>> bool?
> 
> The return value of sr_test_unit_ready is int.

I would still go for bool but it's a peripheral issue.

>> This thing definitely belongs to struct scsi_cd.  What are the
>> synchronization rules here?  Also, what happens if async notification
>> is in use?  How does the timer get activated then?
> 
> This is a problem, any suggestions? Especially when the system goes to
> S3/S4 state.

Associate with specific device and using timer should work.

>>> +     default:
>>> +             /*tray/drawer/pop-up type*/
>>> +             /* 3a/01(asc/ascq) means MEDIUM NOT PRESENT - TRAY CLOSED */
>>> +             if (isvalid && (sshdr->asc  == 0x3a &&
>>> +                             sshdr->ascq == 0x01)) {
>>> +
>>> +                     /* Eject the tray for a tray type drive*/
>>> +                     if (sr_zpodd_device.flags & SR_ZPODD_NEED_EJECT) {
>>> +                             sr_tray_move(cdi, 1);
>>> +                             sr_zpodd_device.flags &= ~SR_ZPODD_NEED_EJECT;
>>> +                     } else if (!(sr_zpodd_device.flags &
>>> +                                     SR_ZPODD_NO_DISK)) {
>>> +                             sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
>>> +                             sr_zpodd_device.last_jiffies = jiffies;
>>> +                     } else if (time_after(jiffies,
>>> +                                     sr_zpodd_device.last_jiffies
>>> +                                     + SR_NO_MEDIA_TIMEOUT)) {
>>> +                             acpi_bus_set_power(sr_zpodd_device.handle,
>>> +                                                     ACPI_STATE_D3);
>>> +                     }
>>> +             } else
>>> +                     sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
>>> +             break;
>>> +     }
>>
>> It would probably be better to separate decision making and updating
>> states.
> 
> When user pressing the button on a tray type drive,  driver should set
> a flag used to eject the tray in sr_media_change, if we use that
> states here, we should a one more state "NEED_EJECT" for drive status.

Oh, I meant the code.  ie. instead of

	switch (device type) {
	type a:
		something something
		break;
	default:
		something something slightly differently
		break;
	}

do something like the following,

	switch (device type) {
	type a:
		determine what to do
		break;
	default:
		determine what to do slightly differently;
		break;
	}
	do what has been determined;

Thanks.

-- 
tejun

Re: [PATCH RFC] support sata odd zero power

[su henry]

Hi tejun,

Thanks for the good suggestion, please see my comments inline.

On Mon, Jun 28, 2010 at 5:04 PM, Tejun Heo <htejun@gmail.com> wrote:
> Hello,
>
> On 06/28/2010 10:43 AM, su henry wrote:
>>> Can you please align fields?  Also, single struct for the whole
>>> system?  I haven't read the spec but I don't think anybody would be
>>> defining things like this system-wide.  Right?
>>>
>>
>> My original though is to put this structure to cdrom_device_info, but
>> this structure should be firstly used by acpi_walk_namespace in
>> sr_init, and cdrom_device_info is allocated in sr_probe, that's why I
>> define a separate structure for the zero power odd.
>
> What prevents you from walking the acpi device tree from sr_probe()?
> Or even if you need to walk it from sr_init(), you still need to store
> the result and associate it with a specific cdrom device.  It is
> something which is specific to single device.  You can't use single
> global data structure for all devices like this.
>

In order to make sure we walk the name space one time only.

Because when the host starts the power supply to ODD, the driver will
start from sr_probe. I think it unnecessary to walk the acpi name
space when driver probes the device, so I  walk the acpi name space
from sr_init.

>>> bool?
>>
>> The return value of sr_test_unit_ready is int.
>
> I would still go for bool but it's a peripheral issue.
>
>>> This thing definitely belongs to struct scsi_cd.  What are the
>>> synchronization rules here?  Also, what happens if async notification
>>> is in use?  How does the timer get activated then?
>>
>> This is a problem, any suggestions? Especially when the system goes to
>> S3/S4 state.
>
> Associate with specific device and using timer should work.
>

Considering the S3/S4 state, if we add a new timer for this, we should
also add the suspend/resume callbacks for the driver, and modify the
timer timeout(mod_timer) in the callback function.

>>>> +     default:
>>>> +             /*tray/drawer/pop-up type*/
>>>> +             /* 3a/01(asc/ascq) means MEDIUM NOT PRESENT - TRAY CLOSED */
>>>> +             if (isvalid && (sshdr->asc  == 0x3a &&
>>>> +                             sshdr->ascq == 0x01)) {
>>>> +
>>>> +                     /* Eject the tray for a tray type drive*/
>>>> +                     if (sr_zpodd_device.flags & SR_ZPODD_NEED_EJECT) {
>>>> +                             sr_tray_move(cdi, 1);
>>>> +                             sr_zpodd_device.flags &= ~SR_ZPODD_NEED_EJECT;
>>>> +                     } else if (!(sr_zpodd_device.flags &
>>>> +                                     SR_ZPODD_NO_DISK)) {
>>>> +                             sr_zpodd_device.flags |= SR_ZPODD_NO_DISK;
>>>> +                             sr_zpodd_device.last_jiffies = jiffies;
>>>> +                     } else if (time_after(jiffies,
>>>> +                                     sr_zpodd_device.last_jiffies
>>>> +                                     + SR_NO_MEDIA_TIMEOUT)) {
>>>> +                             acpi_bus_set_power(sr_zpodd_device.handle,
>>>> +                                                     ACPI_STATE_D3);
>>>> +                     }
>>>> +             } else
>>>> +                     sr_zpodd_device.flags &= ~SR_ZPODD_NO_DISK;
>>>> +             break;
>>>> +     }
>>>
>>> It would probably be better to separate decision making and updating
>>> states.
>>
>> When user pressing the button on a tray type drive,  driver should set
>> a flag used to eject the tray in sr_media_change, if we use that
>> states here, we should a one more state "NEED_EJECT" for drive status.
>
> Oh, I meant the code.  ie. instead of
>
>        switch (device type) {
>        type a:
>                something something
>                break;
>        default:
>                something something slightly differently
>                break;
>        }
>
> do something like the following,
>
>        switch (device type) {
>        type a:
>                determine what to do
>                break;
>        default:
>                determine what to do slightly differently;
>                break;
>        }
>        do what has been determined;
>

Thanks for the advice.

> Thanks.
>
> --
> tejun
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] support sata odd zero power

[Tejun Heo]

Helo,

On 06/28/2010 12:42 PM, su henry wrote:
>> What prevents you from walking the acpi device tree from sr_probe()?
>> Or even if you need to walk it from sr_init(), you still need to store
>> the result and associate it with a specific cdrom device.  It is
>> something which is specific to single device.  You can't use single
>> global data structure for all devices like this.
> 
> In order to make sure we walk the name space one time only.
> 
> Because when the host starts the power supply to ODD, the driver will
> start from sr_probe. I think it unnecessary to walk the acpi name
> space when driver probes the device, so I  walk the acpi name space
> from sr_init.

I don't think it would matter at all whether you walk the ACPI tree
once during boot or on every sr_probe().  sr_probe() isn't exactly hot
path nor is ACPI tree walking a very expensive operation.  Even if it
is expensive, the right thing to do is to walk it in sr_init(), store
the result and _associate_ it with the appropriate device once the
device is probed.  It isn't a system global property.  It simply can't
live in a single global data structure shared by every device.  I
mean, what happens if there are multiple devices w/ ODDZ support?

>>> This is a problem, any suggestions? Especially when the system goes to
>>> S3/S4 state.
>>
>> Associate with specific device and using timer should work.
> 
> Considering the S3/S4 state, if we add a new timer for this, we should
> also add the suspend/resume callbacks for the driver, and modify the
> timer timeout(mod_timer) in the callback function.

I don't see why that would be necessary but if so, yeah, sure.

Thanks.

-- 
tejun

Re: [PATCH RFC] support sata odd zero power

[James Bottomley]

On Fri, 2010-06-25 at 18:15 +0800, su henry wrote:
> From b221e73b802ccfe5338b575dc0ff3b687acdd6fa Mon Sep 17 00:00:00 2001
> From: henry su <henry.su.ati@gmail.com>
> Date: Fri, 25 Jun 2010 00:08:42 +0800
> Subject: [PATCH RFC] support sata odd zero power
> 
> In order to extend the battery life of Mobile PC system, the host should remove
> the power supply to the Optical Disc Drive (logical unit) when it detects the
> logical unit with no media and tray closed, and the host starts the power
> supply to the logical unit when it detects user action to the logical unit that
> the power supply is omitted.

> The patch evaluates the _PS3 method to remove the power supply to the ODD if
> the host detects MEDIUM NOT PRESENT - TRAY CLOSED is reported by REQUEST SENSE
> command for a Drawer, Tray or Pop-up type drive, or MEDIUM NOT PRESENT -
> TRAY CLOSED for a slot/caddy type drive; on the other hand, the patch evaluates
> the _PS0 method to restart the power supply to the ODD when the user
> presses the
> button on a tray type drive or inserts a CD to a slot/caddy type drive.
> 
> PLDS DS-8A5S, pansonic UJ892 and UJ897 are the ODD samples used to test
> the patch on AMD SB800 platforms.
> 
> Signed-off-by: Henry Su <henry.su.ati@gmail.com>

The first observation I'd have is a big meta one:  This all needs to
work on non ACPI systems ... given that it relies on unstubbed ACPI
functions, I don't think it does, does it?

What I'd suggest is confining it all to sr_acpi.c and hooking it in to
the test unit ready and init/exit ... that way the code only need be
present if ACPI is.

[...]
> @@ -786,6 +853,7 @@ static void get_capabilities(struct scsi_cd *cd)
>  	}
> 
>  	n = data.header_length + data.block_descriptor_length;
> +	sr_zpodd_device.mechtype = buffer[n + 6] >> 5;
>  	cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
>  	cd->readcd_known = 1;
>  	cd->readcd_cdda = buffer[n + 5] & 0x01;

This piece of data should probably be added to struct scsi_cd .. we
already use it in the routine anyway for printing out the tray type.

James

Re: [PATCH RFC] support sata odd zero power

[su henry]

On Fri, Jun 25, 2010 at 10:01 PM, James Bottomley
<James.Bottomley@suse.de> wrote:
> On Fri, 2010-06-25 at 18:15 +0800, su henry wrote:
>> From b221e73b802ccfe5338b575dc0ff3b687acdd6fa Mon Sep 17 00:00:00 2001
>> From: henry su <henry.su.ati@gmail.com>
>> Date: Fri, 25 Jun 2010 00:08:42 +0800
>> Subject: [PATCH RFC] support sata odd zero power
>>
>> In order to extend the battery life of Mobile PC system, the host should remove
>> the power supply to the Optical Disc Drive (logical unit) when it detects the
>> logical unit with no media and tray closed, and the host starts the power
>> supply to the logical unit when it detects user action to the logical unit that
>> the power supply is omitted.
>
>> The patch evaluates the _PS3 method to remove the power supply to the ODD if
>> the host detects MEDIUM NOT PRESENT - TRAY CLOSED is reported by REQUEST SENSE
>> command for a Drawer, Tray or Pop-up type drive, or MEDIUM NOT PRESENT -
>> TRAY CLOSED for a slot/caddy type drive; on the other hand, the patch evaluates
>> the _PS0 method to restart the power supply to the ODD when the user
>> presses the
>> button on a tray type drive or inserts a CD to a slot/caddy type drive.
>>
>> PLDS DS-8A5S, pansonic UJ892 and UJ897 are the ODD samples used to test
>> the patch on AMD SB800 platforms.
>>
>> Signed-off-by: Henry Su <henry.su.ati@gmail.com>
>
> The first observation I'd have is a big meta one:  This all needs to
> work on non ACPI systems ... given that it relies on unstubbed ACPI
> functions, I don't think it does, does it?
>
> What I'd suggest is confining it all to sr_acpi.c and hooking it in to
> the test unit ready and init/exit ... that way the code only need be
> present if ACPI is.

Yes, it only works on a systems that supports ACPI functions, and that
system BIOS provides the _PS0/_PS3 method to start/stop the power
supply.

Besides a new driver to support the zero power odd, another suggestion
is adding the "#ifdef CONFIG_ACPI" in sr.c to separate the code.

>
> [...]
>> @@ -786,6 +853,7 @@ static void get_capabilities(struct scsi_cd *cd)
>>       }
>>
>>       n = data.header_length + data.block_descriptor_length;
>> +     sr_zpodd_device.mechtype = buffer[n + 6] >> 5;
>>       cd->cdi.speed = ((buffer[n + 8] << 8) + buffer[n + 9]) / 176;
>>       cd->readcd_known = 1;
>>       cd->readcd_cdda = buffer[n + 5] & 0x01;
>
> This piece of data should probably be added to struct scsi_cd .. we
> already use it in the routine anyway for printing out the tray type.

Agree. It is fine to add the mechanism type to scsi_cd or cdrom_device_info.

>
> James
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] support sata odd zero power

[James Bottomley]

On Mon, 2010-06-28 at 15:35 +0800, su henry wrote:
> On Fri, Jun 25, 2010 at 10:01 PM, James Bottomley
> <James.Bottomley@suse.de> wrote:
> > On Fri, 2010-06-25 at 18:15 +0800, su henry wrote:
> >> From b221e73b802ccfe5338b575dc0ff3b687acdd6fa Mon Sep 17 00:00:00 2001
> >> From: henry su <henry.su.ati@gmail.com>
> >> Date: Fri, 25 Jun 2010 00:08:42 +0800
> >> Subject: [PATCH RFC] support sata odd zero power
> >>
> >> In order to extend the battery life of Mobile PC system, the host should remove
> >> the power supply to the Optical Disc Drive (logical unit) when it detects the
> >> logical unit with no media and tray closed, and the host starts the power
> >> supply to the logical unit when it detects user action to the logical unit that
> >> the power supply is omitted.
> >
> >> The patch evaluates the _PS3 method to remove the power supply to the ODD if
> >> the host detects MEDIUM NOT PRESENT - TRAY CLOSED is reported by REQUEST SENSE
> >> command for a Drawer, Tray or Pop-up type drive, or MEDIUM NOT PRESENT -
> >> TRAY CLOSED for a slot/caddy type drive; on the other hand, the patch evaluates
> >> the _PS0 method to restart the power supply to the ODD when the user
> >> presses the
> >> button on a tray type drive or inserts a CD to a slot/caddy type drive.
> >>
> >> PLDS DS-8A5S, pansonic UJ892 and UJ897 are the ODD samples used to test
> >> the patch on AMD SB800 platforms.
> >>
> >> Signed-off-by: Henry Su <henry.su.ati@gmail.com>
> >
> > The first observation I'd have is a big meta one:  This all needs to
> > work on non ACPI systems ... given that it relies on unstubbed ACPI
> > functions, I don't think it does, does it?
> >
> > What I'd suggest is confining it all to sr_acpi.c and hooking it in to
> > the test unit ready and init/exit ... that way the code only need be
> > present if ACPI is.
> 
> Yes, it only works on a systems that supports ACPI functions, and that
> system BIOS provides the _PS0/_PS3 method to start/stop the power
> supply.
> 
> Besides a new driver to support the zero power odd, another suggestion
> is adding the "#ifdef CONFIG_ACPI" in sr.c to separate the code.

I didn't say add a new driver, I said add a new file to the existing
driver ... it's already split into multiple files.  That way there's no
need to litter #ifdef CONFIG_ACPI to make this compile.

James

Re: [PATCH RFC] support sata odd zero power

[su henry]

On Mon, Jun 28, 2010 at 9:42 PM, James Bottomley
<James.Bottomley@suse.de> wrote:
> On Mon, 2010-06-28 at 15:35 +0800, su henry wrote:
>> On Fri, Jun 25, 2010 at 10:01 PM, James Bottomley
>> <James.Bottomley@suse.de> wrote:
>> > On Fri, 2010-06-25 at 18:15 +0800, su henry wrote:
>> >> From b221e73b802ccfe5338b575dc0ff3b687acdd6fa Mon Sep 17 00:00:00 2001
>> >> From: henry su <henry.su.ati@gmail.com>
>> >> Date: Fri, 25 Jun 2010 00:08:42 +0800
>> >> Subject: [PATCH RFC] support sata odd zero power
>> >>
>> >> In order to extend the battery life of Mobile PC system, the host should remove
>> >> the power supply to the Optical Disc Drive (logical unit) when it detects the
>> >> logical unit with no media and tray closed, and the host starts the power
>> >> supply to the logical unit when it detects user action to the logical unit that
>> >> the power supply is omitted.
>> >
>> >> The patch evaluates the _PS3 method to remove the power supply to the ODD if
>> >> the host detects MEDIUM NOT PRESENT - TRAY CLOSED is reported by REQUEST SENSE
>> >> command for a Drawer, Tray or Pop-up type drive, or MEDIUM NOT PRESENT -
>> >> TRAY CLOSED for a slot/caddy type drive; on the other hand, the patch evaluates
>> >> the _PS0 method to restart the power supply to the ODD when the user
>> >> presses the
>> >> button on a tray type drive or inserts a CD to a slot/caddy type drive.
>> >>
>> >> PLDS DS-8A5S, pansonic UJ892 and UJ897 are the ODD samples used to test
>> >> the patch on AMD SB800 platforms.
>> >>
>> >> Signed-off-by: Henry Su <henry.su.ati@gmail.com>
>> >
>> > The first observation I'd have is a big meta one:  This all needs to
>> > work on non ACPI systems ... given that it relies on unstubbed ACPI
>> > functions, I don't think it does, does it?
>> >
>> > What I'd suggest is confining it all to sr_acpi.c and hooking it in to
>> > the test unit ready and init/exit ... that way the code only need be
>> > present if ACPI is.
>>
>> Yes, it only works on a systems that supports ACPI functions, and that
>> system BIOS provides the _PS0/_PS3 method to start/stop the power
>> supply.
>>
>> Besides a new driver to support the zero power odd, another suggestion
>> is adding the "#ifdef CONFIG_ACPI" in sr.c to separate the code.
>
> I didn't say add a new driver, I said add a new file to the existing
> driver ... it's already split into multiple files.  That way there's no
> need to litter #ifdef CONFIG_ACPI to make this compile.
>

Yes, adding a new file(sr_acpi.c), just like the existing sr_vendor.c.

> James
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html