Re: [PATCH RFC] support sata odd zero power

su henry
2010-06-28T08:43:34Z

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