LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH RESEND] SCSI not showing tray status correctly
@ 2007-10-25 22:30 Maarten Bressers
  2007-10-25 23:24 ` James Bottomley
  0 siblings, 1 reply; 4+ messages in thread
From: Maarten Bressers @ 2007-10-25 22:30 UTC (permalink / raw)
  To: linux-kernel; +Cc: linux-scsi, axboe, James.Bottomley, tasio, dsd

From: David Martin <tasio@tasio.net>

Greetings,

The following patch was submitted to the lkml in 2004 by David Martin
(http://lkml.org/lkml/2004/12/27/1). It wasn't accepted, but I was
unable to find a reason why, so I'm resending it now.

Without this patch the SCSI ioctl CDROM_DRIVE_STATUS always returns
CDS_TRAY_OPEN even if the tray is closed. This patch fixes that.
Gentoo bug report: http://bugs.gentoo.org/show_bug.cgi?id=196879

Signed-off by: Maarten Bressers <mbres@gentoo.org>

---

--- a/drivers/scsi/sr_ioctl.c	2007-10-09 22:31:38.000000000 +0200
+++ b/drivers/scsi/sr_ioctl.c	2007-10-25 22:57:21.000000000 +0200
@@ -310,6 +310,8 @@ int sr_lock_door(struct cdrom_device_inf
 
 int sr_drive_status(struct cdrom_device_info *cdi, int slot)
 {
+	struct media_event_desc med;
+
 	if (CDSL_CURRENT != slot) {
 		/* we have no changer support */
 		return -EINVAL;
@@ -317,7 +319,10 @@ int sr_drive_status(struct cdrom_device_
 	if (0 == test_unit_ready(cdi->handle))
 		return CDS_DISC_OK;
 
-	return CDS_TRAY_OPEN;
+	if (!cdrom_get_media_event(cdi, &med) && med.door_open)
+		return CDS_TRAY_OPEN;
+
+	return CDS_NO_DISC;
 }
 
 int sr_disk_status(struct cdrom_device_info *cdi)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RESEND] SCSI not showing tray status correctly
  2007-10-25 22:30 [PATCH RESEND] SCSI not showing tray status correctly Maarten Bressers
@ 2007-10-25 23:24 ` James Bottomley
  0 siblings, 0 replies; 4+ messages in thread
From: James Bottomley @ 2007-10-25 23:24 UTC (permalink / raw)
  To: Maarten Bressers; +Cc: linux-kernel, linux-scsi, axboe, tasio, dsd

On Thu, 2007-10-25 at 22:30 +0000, Maarten Bressers wrote:
> From: David Martin <tasio@tasio.net>
> 
> Greetings,
> 
> The following patch was submitted to the lkml in 2004 by David Martin
> (http://lkml.org/lkml/2004/12/27/1). It wasn't accepted, but I was
> unable to find a reason why, so I'm resending it now.

Well, sending SCSI patches to the kernel mailing list but not to
linux-scsi is usually a good enough reason.

> Without this patch the SCSI ioctl CDROM_DRIVE_STATUS always returns
> CDS_TRAY_OPEN even if the tray is closed. This patch fixes that.
> Gentoo bug report: http://bugs.gentoo.org/show_bug.cgi?id=196879
> 
> Signed-off by: Maarten Bressers <mbres@gentoo.org>
> 
> ---
> 
> --- a/drivers/scsi/sr_ioctl.c	2007-10-09 22:31:38.000000000 +0200
> +++ b/drivers/scsi/sr_ioctl.c	2007-10-25 22:57:21.000000000 +0200
> @@ -310,6 +310,8 @@ int sr_lock_door(struct cdrom_device_inf
>  
>  int sr_drive_status(struct cdrom_device_info *cdi, int slot)
>  {
> +	struct media_event_desc med;
> +
>  	if (CDSL_CURRENT != slot) {
>  		/* we have no changer support */
>  		return -EINVAL;
> @@ -317,7 +319,10 @@ int sr_drive_status(struct cdrom_device_
>  	if (0 == test_unit_ready(cdi->handle))
>  		return CDS_DISC_OK;
>  
> -	return CDS_TRAY_OPEN;
> +	if (!cdrom_get_media_event(cdi, &med) && med.door_open)
> +		return CDS_TRAY_OPEN;
> +
> +	return CDS_NO_DISC;
>  }
>  
>  int sr_disk_status(struct cdrom_device_info *cdi)

This patch is too simplistic.  ide-cd.c:ide_cdrom_drive_status() looks
to be a reasonable implementation.  However, the worry is that
GET_EVENT_NOTIFICATION is a MMC command; devices not conforming to MMC
won't support it.  In theory, they should just return ILLEGAL_REQUEST,
but USB devices have been known to crash when given commands they don't
understand.  How widely tested has this been (if it's been in Gentoo
since 2004, then it's probably widely tested enough)?

James



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RESEND] SCSI not showing tray status correctly
  2007-10-27 22:58 Maarten Bressers
@ 2007-10-31  3:32 ` James Bottomley
  0 siblings, 0 replies; 4+ messages in thread
From: James Bottomley @ 2007-10-31  3:32 UTC (permalink / raw)
  To: Maarten Bressers; +Cc: linux-kernel, linux-scsi, axboe, tasio, dsd

On Sat, 2007-10-27 at 22:58 +0000, Maarten Bressers wrote:
> > This patch is too simplistic.  ide-cd.c:ide_cdrom_drive_status() looks
> > to be a reasonable implementation.  However, the worry is that
> > GET_EVENT_NOTIFICATION is a MMC command; devices not conforming to MMC
> > won't support it.  In theory, they should just return ILLEGAL_REQUEST,
> > but USB devices have been known to crash when given commands they don't
> > understand.  How widely tested has this been (if it's been in Gentoo
> > since 2004, then it's probably widely tested enough)?
> >
> > James
> >
> This patch hasn't been tested at all, I'm sorry if I gave you that
> impression, it isn't in Gentoo's patches, it's just been brought to our
> attention in the bug I linked too.
> 
> I created another patch, based on your recommendations, and with a lot of
> help from Daniel Drake (dsd@gentoo.org), that's included below. Some
> changes are made to test_unit_ready() to be able to pass the sense data
> to sr_drive_status(). Currently, sr_do_ioctl() swallows this data and
> makes its own interpretations of sense codes, which isn't what we want
> here.
> 
> Is this what you had in mind? Do you think the possible problems with
> USB drives that you mentioned will prevent this patch from going in? 
> The patch has only been compile tested right now, we can do some real
> testing later, for now I'd just like to get your feedback on it.

In general it looks good ... I suppose the only way to test it is to
stick it in and see what happens.  However, there are a few rough edges.

> Maarten
> 
> ---
> 
> --- a/drivers/scsi/sr_ioctl.c	2007-10-26 22:40:41.000000000 +0200
> +++ b/drivers/scsi/sr_ioctl.c	2007-10-27 23:56:16.000000000 +0200
> @@ -275,16 +275,21 @@ int sr_do_ioctl(Scsi_CD *cd, struct pack
>  /* ---------------------------------------------------------------------- */
>  /* interface to cdrom.c                                                   */
>  
> -static int test_unit_ready(Scsi_CD *cd)
> +static int test_unit_ready(Scsi_CD *cd, struct request_sense *sense)
>  {
> -	struct packet_command cgc;
> +	struct scsi_device *SDev = cd->device;
> +	unsigned char cmd[CDROM_PACKET_SIZE] = { GPCMD_TEST_UNIT_READY };
> +	int result;
>  
> -	memset(&cgc, 0, sizeof(struct packet_command));
> -	cgc.cmd[0] = GPCMD_TEST_UNIT_READY;
> -	cgc.quiet = 1;
> -	cgc.data_direction = DMA_NONE;
> -	cgc.timeout = IOCTL_TIMEOUT;
> -	return sr_do_ioctl(cd, &cgc);
> +	if (!scsi_block_when_processing_errors(SDev))
> +		return -ENODEV;
> +
> +	memset(sense, 0, sizeof(*sense));
> +	result = scsi_execute(SDev, cmd, DMA_NONE,
> +			      NULL, 0, (char *)sense,
> +			      IOCTL_TIMEOUT, IOCTL_RETRIES, 0);
> +
> +	return driver_byte(result);

This bit isn't quite right ... this will return true if there's a
collected sense code and false otherwise (so it returns 0 on failure
that doesn't produce any sense information).

>  }
>  
>  int sr_tray_move(struct cdrom_device_info *cdi, int pos)
> @@ -310,14 +315,41 @@ int sr_lock_door(struct cdrom_device_inf
>  
>  int sr_drive_status(struct cdrom_device_info *cdi, int slot)
>  {
> +	struct request_sense sense;
> +	struct media_event_desc med;
> +
>  	if (CDSL_CURRENT != slot) {
>  		/* we have no changer support */
>  		return -EINVAL;
>  	}
> -	if (0 == test_unit_ready(cdi->handle))
> +	if ((0 == test_unit_ready(cdi->handle, &sense)) || 
> +	    sense.sense_key == UNIT_ATTENTION)

Unit attention won't necessarily mean the media is OK ... unit attention
can mean the media or tray status has changed.

>  		return CDS_DISC_OK;
>  
> -	return CDS_TRAY_OPEN;
> +	if (!cdrom_get_media_event(cdi, &med)) {
> +		if (med.media_present)
> +			return CDS_DISC_OK;
> +		else if (med.door_open)
> +			return CDS_TRAY_OPEN;
> +		else
> +			return CDS_NO_DISC;
> +	}
> +
> +	if (sense.sense_key == NOT_READY && sense.asc == 0x04 && sense.ascq == 0x04)

Actually, all asc 0x4 codes are some type of in progress operation,
format doesn't necessarily have to be treated specially.

> +		return CDS_DISC_OK;
> +
> +	/*
> +	 * If not using Mt Fuji extended media tray reports,
> +	 * just return TRAY_OPEN since ATAPI doesn't provide
> +	 * any other way to detect this...
> +	 */
> +	if (sense.sense_key == NOT_READY) {
> +		if (sense.asc == 0x3a && sense.ascq == 1)
> +			return CDS_NO_DISC;
> +		else
> +			return CDS_TRAY_OPEN;

This is also a bit odd.  asc 0x3a ascq 0x2 definitely means tray open,
but there are a lot of other not ready conditions that don't mean this.

> +	}
> +	return CDS_DRIVE_NOT_READY;
>  }
>  
>  int sr_disk_status(struct cdrom_device_info *cdi)

James



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RESEND] SCSI not showing tray status correctly
@ 2007-10-27 22:58 Maarten Bressers
  2007-10-31  3:32 ` James Bottomley
  0 siblings, 1 reply; 4+ messages in thread
From: Maarten Bressers @ 2007-10-27 22:58 UTC (permalink / raw)
  To: James.Bottomley; +Cc: linux-kernel, linux-scsi, axboe, tasio, dsd

> This patch is too simplistic.  ide-cd.c:ide_cdrom_drive_status() looks
> to be a reasonable implementation.  However, the worry is that
> GET_EVENT_NOTIFICATION is a MMC command; devices not conforming to MMC
> won't support it.  In theory, they should just return ILLEGAL_REQUEST,
> but USB devices have been known to crash when given commands they don't
> understand.  How widely tested has this been (if it's been in Gentoo
> since 2004, then it's probably widely tested enough)?
>
> James
>
This patch hasn't been tested at all, I'm sorry if I gave you that
impression, it isn't in Gentoo's patches, it's just been brought to our
attention in the bug I linked too.

I created another patch, based on your recommendations, and with a lot of
help from Daniel Drake (dsd@gentoo.org), that's included below. Some
changes are made to test_unit_ready() to be able to pass the sense data
to sr_drive_status(). Currently, sr_do_ioctl() swallows this data and
makes its own interpretations of sense codes, which isn't what we want
here.

Is this what you had in mind? Do you think the possible problems with
USB drives that you mentioned will prevent this patch from going in? 
The patch has only been compile tested right now, we can do some real
testing later, for now I'd just like to get your feedback on it.

Maarten

---

--- a/drivers/scsi/sr_ioctl.c	2007-10-26 22:40:41.000000000 +0200
+++ b/drivers/scsi/sr_ioctl.c	2007-10-27 23:56:16.000000000 +0200
@@ -275,16 +275,21 @@ int sr_do_ioctl(Scsi_CD *cd, struct pack
 /* ---------------------------------------------------------------------- */
 /* interface to cdrom.c                                                   */
 
-static int test_unit_ready(Scsi_CD *cd)
+static int test_unit_ready(Scsi_CD *cd, struct request_sense *sense)
 {
-	struct packet_command cgc;
+	struct scsi_device *SDev = cd->device;
+	unsigned char cmd[CDROM_PACKET_SIZE] = { GPCMD_TEST_UNIT_READY };
+	int result;
 
-	memset(&cgc, 0, sizeof(struct packet_command));
-	cgc.cmd[0] = GPCMD_TEST_UNIT_READY;
-	cgc.quiet = 1;
-	cgc.data_direction = DMA_NONE;
-	cgc.timeout = IOCTL_TIMEOUT;
-	return sr_do_ioctl(cd, &cgc);
+	if (!scsi_block_when_processing_errors(SDev))
+		return -ENODEV;
+
+	memset(sense, 0, sizeof(*sense));
+	result = scsi_execute(SDev, cmd, DMA_NONE,
+			      NULL, 0, (char *)sense,
+			      IOCTL_TIMEOUT, IOCTL_RETRIES, 0);
+
+	return driver_byte(result);
 }
 
 int sr_tray_move(struct cdrom_device_info *cdi, int pos)
@@ -310,14 +315,41 @@ int sr_lock_door(struct cdrom_device_inf
 
 int sr_drive_status(struct cdrom_device_info *cdi, int slot)
 {
+	struct request_sense sense;
+	struct media_event_desc med;
+
 	if (CDSL_CURRENT != slot) {
 		/* we have no changer support */
 		return -EINVAL;
 	}
-	if (0 == test_unit_ready(cdi->handle))
+	if ((0 == test_unit_ready(cdi->handle, &sense)) || 
+	    sense.sense_key == UNIT_ATTENTION)
 		return CDS_DISC_OK;
 
-	return CDS_TRAY_OPEN;
+	if (!cdrom_get_media_event(cdi, &med)) {
+		if (med.media_present)
+			return CDS_DISC_OK;
+		else if (med.door_open)
+			return CDS_TRAY_OPEN;
+		else
+			return CDS_NO_DISC;
+	}
+
+	if (sense.sense_key == NOT_READY && sense.asc == 0x04 && sense.ascq == 0x04)
+		return CDS_DISC_OK;
+
+	/*
+	 * If not using Mt Fuji extended media tray reports,
+	 * just return TRAY_OPEN since ATAPI doesn't provide
+	 * any other way to detect this...
+	 */
+	if (sense.sense_key == NOT_READY) {
+		if (sense.asc == 0x3a && sense.ascq == 1)
+			return CDS_NO_DISC;
+		else
+			return CDS_TRAY_OPEN;
+	}
+	return CDS_DRIVE_NOT_READY;
 }
 
 int sr_disk_status(struct cdrom_device_info *cdi)

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2007-10-31  3:32 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-10-25 22:30 [PATCH RESEND] SCSI not showing tray status correctly Maarten Bressers
2007-10-25 23:24 ` James Bottomley
2007-10-27 22:58 Maarten Bressers
2007-10-31  3:32 ` James Bottomley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).