(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
--
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
