libata passthrough - return register data from HDIO_* commands

PATCH In review Jon Escombe 3 participants 7 messages Aug 13, 2005 – Aug 22, 2005

Subsystems (2)

SCSI libata

lore

Share & tools

lore

Jon Escombe

author

2005-08-13 17:38 UTC #0 lore raw

PATCH 1/1 libata passthrough - return register data from HDIO_* commands

Here is a first attempt at a patch to return register data from the libata passthrough HDIO ioctl handlers, I needed this as the ATA 'unload immediate' command returns the success in the lbal register. This patch applies on top of 2.6.12 and Jeffs 2.6.12-git4-passthru1.patch. (Apologies, but Thunderbird appears to have replaced the tabs with spaces).

One oddity is that the sr_result field is correctly being set in ata_gen_ata_desc_sense(), however the high word is different when we're back in the ioctl hander. I've coded round this for now by only checking the low word, but this needs more investigation.

Jeff, could this functionality be incorporated into the pasthrough patch when complete?

Regards,
Jon.

drivers/scsi/libata-scsi.c +16 -4

@@ -89,6 +89,7 @@
    u8 args[4], *argbuf = NULL;
    int argsize = 0;
    struct scsi_request *sreq;
+    unsigned char *sb, *desc;
 
    if (NULL == (void *)arg)
        return -EINVAL;

@@ -100,6 +101,9 @@
    if (!sreq)
        return -EINTR;
 
+    sb = sreq->sr_sense_buffer;
+    desc = sb + 8;
+
    memset(scsi_cmd, 0, sizeof(scsi_cmd));
 
    if (args[3]) {

@@ -109,12 +113,12 @@
            return -ENOMEM;
 
        scsi_cmd[1]  = (4 << 1); /* PIO Data-in */
-        scsi_cmd[2]  = 0x0e;     /* no off.line or cc, read from dev,
+        scsi_cmd[2]  = 0x2e;     /* no off.line, read from dev, request cc
                                    block count in sector count field */
        sreq->sr_data_direction = DMA_FROM_DEVICE;
    } else {
        scsi_cmd[1]  = (3 << 1); /* Non-data */
-        /* scsi_cmd[2] is already 0 -- no off.line, cc, or data xfer */
+        scsi_cmd[2]  = 0x20;     /* no off.line or data xfer, request check condtion */
        sreq->sr_data_direction = DMA_NONE;
    }

@@ -135,16 +139,24 @@
       from scsi_ioctl_send_command() for default case... */
    scsi_wait_req(sreq, scsi_cmd, argbuf, argsize, (10*HZ), 5);
 
-    if (sreq->sr_result) {
+    if ((sreq->sr_result & 0xFFFF) != SAM_STAT_CHECK_CONDITION) {
        rc = -EIO;
        goto error;
    }
 
-    /* Need code to retrieve data from check condition? */
+    /* Retrieve data from check condition */
+    args[1] = desc[3];
+    args[2] = desc[5];
+    args[3] = desc[7];
+    args[4] = desc[9];
+    args[5] = desc[11];
+    args[0] = desc[13];
 
    if ((argbuf)
     && copy_to_user((void *)(arg + sizeof(args)), argbuf, argsize))
        rc = -EFAULT;
+    if (copy_to_user(arg, args, sizeof(args)))
+        rc = -EFAULT;

error:

    scsi_release_request(sreq);

@@ -171,6 +183,7 @@

    u8 scsi_cmd[MAX_COMMAND_SIZE];
    u8 args[7];
    struct scsi_request *sreq;

+ unsigned char *sb, *desc;

    if (NULL == (void *)arg)
        return -EINVAL;
@@ -181,7 +194,7 @@
    memset(scsi_cmd, 0, sizeof(scsi_cmd));
    scsi_cmd[0]  = ATA_16;
    scsi_cmd[1]  = (3 << 1); /* Non-data */
-    /* scsi_cmd[2] is already 0 -- no off.line, cc, or data xfer */
+    scsi_cmd[2]  = 0x20;     /* no off.line, or data xfer, request cc */
    scsi_cmd[4]  = args[1];
    scsi_cmd[6]  = args[2];
    scsi_cmd[8]  = args[3];
@@ -195,18 +208,29 @@
        goto error;
    }

+ sb = sreq->sr_sense_buffer;
+ desc = sb + 8;
+

    sreq->sr_data_direction = DMA_NONE;
    /* Good values for timeout and retries?  Values below
       from scsi_ioctl_send_command() for default case... */
    scsi_wait_req(sreq, scsi_cmd, NULL, 0, (10*HZ), 5);

- if (sreq->sr_result) {

+    if ((sreq->sr_result & 0xFFFF) != SAM_STAT_CHECK_CONDITION) {
        rc = -EIO;
        goto error;
    }

-    /* Need code to retrieve data from check condition? */
+    /* Retrieve data from check condition */
+    args[1] = desc[3];
+    args[2] = desc[5];
+    args[3] = desc[7];
+    args[4] = desc[9];
+    args[5] = desc[11];
+    args[0] = desc[13];

+ if (copy_to_user(arg, args, sizeof(args)))
+ rc = -EFAULT;
error:

    scsi_release_request(sreq);
    return rc;

Jon Escombe

author

2005-08-15 22:12 UTC #1 lore raw

Quoted 6 lines

Here is a first attempt at a patch to return register data from the libata passthrough HDIO ioctl handlers, I needed this as the ATA 'unload immediate' command returns the success in the lbal register. This patch applies on top of 2.6.12 and Jeffs 2.6.12-git4-passthru1.patch. (Apologies, but Thunderbird appears to have replaced the tabs with spaces).
One oddity is that the sr_result field is correctly being set in ata_gen_ata_desc_sense(), however the high word is different when we're back in the ioctl hander. I've coded round this for now by only checking the low word, but this needs more investigation.
Jeff, could this functionality be incorporated into the pasthrough patch when complete?

I'd failed to realise that scsi_finish_command() sets the high byte of the result field to DRIVER_SENSE when there is sense data. Patch updated to reflect this...

Haven't had any feedback on the patch itself - but this now does what I wanted it do to. (I can't find a way to make Thunderbird retain tabs in the message body, so sending as an attachment).

Regards,
Jon.

______________________________________________________________
Email via Mailtraq4Free from Enstar (www.mailtraqdirect.co.uk)

drivers/scsi/libata-scsi.c +31 -7

@@ -89,6 +89,7 @@
 	u8 args[4], *argbuf = NULL;
 	int argsize = 0;
 	struct scsi_request *sreq;
+	unsigned char *sb, *desc;
 
 	if (NULL == (void *)arg)
 		return -EINVAL;

@@ -100,6 +101,9 @@
 	if (!sreq)
 		return -EINTR;
 
+	sb = sreq->sr_sense_buffer;
+	desc = sb + 8;
+
 	memset(scsi_cmd, 0, sizeof(scsi_cmd));
 
 	if (args[3]) {

@@ -109,12 +113,12 @@
 			return -ENOMEM;
 
 		scsi_cmd[1]  = (4 << 1); /* PIO Data-in */
-		scsi_cmd[2]  = 0x0e;     /* no off.line or cc, read from dev,
+		scsi_cmd[2]  = 0x2e;     /* no off.line, read from dev, request cc
 		                            block count in sector count field */
 		sreq->sr_data_direction = DMA_FROM_DEVICE;
 	} else {
 		scsi_cmd[1]  = (3 << 1); /* Non-data */
-		/* scsi_cmd[2] is already 0 -- no off.line, cc, or data xfer */
+		scsi_cmd[2]  = 0x20;     /* no off.line or data xfer, request check condtion */
 		sreq->sr_data_direction = DMA_NONE;
 	}

@@ -135,16 +139,24 @@
 	   from scsi_ioctl_send_command() for default case... */
 	scsi_wait_req(sreq, scsi_cmd, argbuf, argsize, (10*HZ), 5);
 
-	if (sreq->sr_result) {
+	if (!sreq->sr_result == ((DRIVER_SENSE << 24) + SAM_STAT_CHECK_CONDITION)) {
 		rc = -EIO;
 		goto error;
 	}
 
-	/* Need code to retrieve data from check condition? */
+	/* Retrieve data from check condition */
+	args[1] = desc[3];
+	args[2] = desc[5];
+	args[3] = desc[7];
+	args[4] = desc[9];
+	args[5] = desc[11];
+	args[0] = desc[13];
 
 	if ((argbuf)
 	 && copy_to_user((void *)(arg + sizeof(args)), argbuf, argsize))
 		rc = -EFAULT;
+	if (copy_to_user(arg, args, sizeof(args)))
+		rc = -EFAULT;
 error:
 	scsi_release_request(sreq);

@@ -171,6 +183,7 @@
 	u8 scsi_cmd[MAX_COMMAND_SIZE];
 	u8 args[7];
 	struct scsi_request *sreq;
+	unsigned char *sb, *desc;
 
 	if (NULL == (void *)arg)
 		return -EINVAL;

@@ -181,7 +194,7 @@
 	memset(scsi_cmd, 0, sizeof(scsi_cmd));
 	scsi_cmd[0]  = ATA_16;
 	scsi_cmd[1]  = (3 << 1); /* Non-data */
-	/* scsi_cmd[2] is already 0 -- no off.line, cc, or data xfer */
+	scsi_cmd[2]  = 0x20;     /* no off.line, or data xfer, request cc */
 	scsi_cmd[4]  = args[1];
 	scsi_cmd[6]  = args[2];
 	scsi_cmd[8]  = args[3];

@@ -195,18 +208,29 @@
 		goto error;
 	}
 
+	sb = sreq->sr_sense_buffer;
+	desc = sb + 8;
+
 	sreq->sr_data_direction = DMA_NONE;
 	/* Good values for timeout and retries?  Values below
 	   from scsi_ioctl_send_command() for default case... */
 	scsi_wait_req(sreq, scsi_cmd, NULL, 0, (10*HZ), 5);
 
-	if (sreq->sr_result) {
+	if (!sreq->sr_result == ((DRIVER_SENSE << 24) + SAM_STAT_CHECK_CONDITION)) {
 		rc = -EIO;
 		goto error;
 	}
 
-	/* Need code to retrieve data from check condition? */
+	/* Retrieve data from check condition */
+	args[1] = desc[3];
+	args[2] = desc[5];
+	args[3] = desc[7];
+	args[4] = desc[9];
+	args[5] = desc[11];
+	args[0] = desc[13];
 
+	if (copy_to_user(arg, args, sizeof(args)))
+		rc = -EFAULT;
 error:
 	scsi_release_request(sreq);
 	return rc;

John W. Linville

2005-08-19 19:09 UTC #2 lore raw

Quoted 8 lines · Jon Escombe

Quoted 4 lines
Here is a first attempt at a patch to return register data from the libata passthrough HDIO ioctl handlers, I needed this as the ATA 'unload immediate' command returns the success in the lbal register.
Haven't had any feedback on the patch itself - but this now does what I wanted it do to. (I can't find a way to make Thunderbird retain tabs in the message body, so sending as an attachment).

Grumble...this is why patches as attachments suck...grumble...manually copied the patch below...

Overall, I like the patch. I trust your assertion that it actually works... :-)

I have a few comments...

Quoted 2 lines

drivers/scsi/libata-scsi.c +0 -0

<snip>

drivers/scsi/libata-scsi.c line 109

Quoted 10 lines

drivers/scsi/libata-scsi.c +3 -0

@@ -100,6 +101,9 @@
 	if (!sreq)
 		return -EINTR;
 
+	sb = sreq->sr_sense_buffer;
+	desc = sb + 8;
+
 	memset(scsi_cmd, 0, sizeof(scsi_cmd));
 
 	if (args[3]) {

Can we move this hunk closer to where desc is actually used? It seems like that would be easier to read. I'd put those new lines just before the "Retrieve data from check condition" comment.

<snip>

drivers/scsi/libata-scsi.c line 145

Quoted 9 lines

drivers/scsi/libata-scsi.c +1 -1

@@ -135,16 +139,24 @@
 	   from scsi_ioctl_send_command() for default case... */
 	scsi_wait_req(sreq, scsi_cmd, argbuf, argsize, (10*HZ), 5);
 
-	if (sreq->sr_result) {
+	if (!sreq->sr_result == ((DRIVER_SENSE << 24) + SAM_STAT_CHECK_CONDITION)) {
 		rc = -EIO;
 		goto error;
 	}

scsi_debug.c has a definition like this:

static const int check_condition_result =

                (DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION;

Perhaps we could add something like that to the top of libata-scsi.c, (or some appropriate header file?) changing the if to read as:

@@ -135,16 +139,24 @@
from scsi_ioctl_send_command() for default case... */
scsi_wait_req(sreq, scsi_cmd, argbuf, argsize, (10*HZ), 5);

- if (sreq->sr_result) {
+ if (sreq->sr_result != check_condition_result) {
rc = -EIO;
goto error;
}

Quoted 4 lines

drivers/scsi/libata-scsi.c +3 -1

-	/* Need code to retrieve data from check condition? */
+	/* Retrieve data from check condition */
+	args[1] = desc[3];
+	args[2] = desc[5];

drivers/scsi/libata-scsi.c line 159

Quoted 10 lines

drivers/scsi/libata-scsi.c +6 -0

+	args[3] = desc[7];
+	args[4] = desc[9];
+	args[5] = desc[11];
+	args[0] = desc[13];
 
 	if ((argbuf)
 	 && copy_to_user((void *)(arg + sizeof(args)), argbuf, argsize))
 		rc = -EFAULT;
+	if (copy_to_user(arg, args, sizeof(args)))
+		rc = -EFAULT;

It seems more natural to put these lines _before_ the "if ((argbuf)" line. It's probably more likely to be nice to the cache that way as well.

drivers/scsi/libata-scsi.c line 213

Quoted 7 lines

drivers/scsi/libata-scsi.c +3 -0

@@ -195,18 +208,29 @@
 		goto error;
 	}
 
+	sb = sreq->sr_sense_buffer;
+	desc = sb + 8;
+

Same comment as above about moving these closer to the "Retrieve data from check condition" comment.

drivers/scsi/libata-scsi.c line 222

Quoted 8 lines

drivers/scsi/libata-scsi.c +1 -1

 	   from scsi_ioctl_send_command() for default case... */
 	scsi_wait_req(sreq, scsi_cmd, NULL, 0, (10*HZ), 5);
 
-	if (sreq->sr_result) {
+	if (!sreq->sr_result == ((DRIVER_SENSE << 24) + SAM_STAT_CHECK_CONDITION)) {
 		rc = -EIO;
 		goto error;
 	}

Same comment as before regarding check_condition_result static const int.

I'd be happy to ACK the patch with the changes outlined above.

Thanks!

John

Signature · 2 lines

John W. Linville
[email protected]

John W. Linville

2005-08-19 19:19 UTC #3 lore raw

Quoted 10 lines · John W. Linville

Quoted 4 lines · Jon Escombe
Quoted 4 lines
Here is a first attempt at a patch to return register data from the libata passthrough HDIO ioctl handlers, I needed this as the ATA 'unload immediate' command returns the success in the lbal register.
Overall, I like the patch. I trust your assertion that it actually works... :-)
I have a few comments...

Well, apparently "Jon Escombe <[email protected]>" is not a good address. I got a delivery failur notification email from his mail server. Hopefully Jon reads the lists... :-)

John

Signature · 2 lines

John W. Linville
[email protected]

Jon Escombe

author

2005-08-19 19:33 UTC #4 lore raw

Quoted 19 lines · John W. Linville

Quoted 9 lines · John W. Linville
Quoted 3 lines · Jon Escombe
Quoted 3 lines
Here is a first attempt at a patch to return register data from the libata passthrough HDIO ioctl handlers, I needed this as the ATA 'unload immediate' command returns the success in the lbal register.
Quoted 6 lines
Overall, I like the patch. I trust your assertion that it actually works... :-)
I have a few comments...

Quoted 7 lines

Well, apparently "Jon Escombe <[email protected]>" is not a good address. I got a delivery failur notification email from his mail server. Hopefully Jon reads the lists... :-)
John

Thanks, will take a look at the comments.

Not sure why the email bounced, but I will give you another email address so you could forward me the notification (if you still have it?)

Regards,
Jon.

______________________________________________________________
Email via Mailtraq4Free from Enstar (www.mailtraqdirect.co.uk)

John W. Linville

2005-08-19 20:12 UTC #5 lore raw

Quoted 2 lines · Jon Escombe

Not sure why the email bounced, but I will give you another email address so you could forward me the notification (if you still have it?)

Jon,

I sent the information to the other address you provided...and I got another rejection. I don't suppose you have a Gmail, Pobox, or other account?

John

Signature · 2 lines

John W. Linville
[email protected]

Jeff Garzik

2005-08-22 22:56 UTC #6 lore raw

In reply to Jon Escombe

Quoted 5 lines · Jon Escombe

Quoted 5 lines
Here is a first attempt at a patch to return register data from the libata passthrough HDIO ioctl handlers, I needed this as the ATA 'unload immediate' command returns the success in the lbal register. This patch applies on top of 2.6.12 and Jeffs 2.6.12-git4-passthru1.patch. (Apologies, but Thunderbird appears to have replaced the tabs with spaces).
One oddity is that the sr_result field is correctly being set in ata_gen_ata_desc_sense(), however the high word is different when we're back in the ioctl hander. I've coded round this for now by only checking the low word, but this needs more investigation.

Quoted 7 lines

Quoted 1 line
Jeff, could this functionality be incorporated into the pasthrough patch when complete?
I'd failed to realise that scsi_finish_command() sets the high byte of the result field to DRIVER_SENSE when there is sense data. Patch updated to reflect this...
Haven't had any feedback on the patch itself - but this now does what I wanted it do to. (I can't find a way to make Thunderbird retain tabs in the message body, so sending as an attachment).

Patch seems sane at first glance. I'll look over it in depth this week.

Jeff