Re: [patch] libata passthrough - return register data from HDIO_* commands

Re: [patch] libata passthrough - return register data from HDIO_* commands

[John W. Linville]

On Mon, Aug 15, 2005 at 11:11:02PM +0100, Jon Escombe wrote:
> 
> >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...

> --- a/drivers/scsi/libata-scsi.c
> +++ b/drivers/scsi/libata-scsi.c

<snip>

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

> @@ -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;
 	}

> -	/* 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;
 
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.

> @@ -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.

>  	   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
-- 
John W. Linville
linville@tuxdriver.com

Re: [patch] libata passthrough - return register data from HDIO_* commands

[John W. Linville]

On Fri, Aug 19, 2005 at 03:06:27PM -0400, John W. Linville wrote:
> On Mon, Aug 15, 2005 at 11:11:02PM +0100, Jon Escombe wrote:
> > 
> > >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 <lists@dresco.co.uk>" is not a good
address.  I got a delivery failur notification email from his mail
server.  Hopefully Jon reads the lists... :-)

John
-- 
John W. Linville
linville@tuxdriver.com

Re: [patch] libata passthrough - return register data from HDIO_* commands

[Jon Escombe]

John W. Linville wrote:

>On Fri, Aug 19, 2005 at 03:06:27PM -0400, John W. Linville wrote:
>  
>
>>On Mon, Aug 15, 2005 at 11:11:02PM +0100, Jon Escombe wrote:
>>    
>>
>>>>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 <lists@dresco.co.uk>" 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)

Re: [patch] libata passthrough - return register data from HDIO_* commands

[John W. Linville]

On Fri, Aug 19, 2005 at 08:31:45PM +0100, Jon Escombe wrote:

> 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
-- 
John W. Linville
linville@tuxdriver.com

Re: [patch] libata passthrough - return register data from HDIO_* commands

[Jeff Garzik]

Jon Escombe wrote:
> 
>> 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).

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

	Jeff