On Fri, Jun 25, 2010 at 9:39 PM, Tejun Heo <htejun@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
