libata-passthrough problem.

libata-passthrough problem.

[James Courtier-Dutton]

I am using (at 12-Nov-2005)
git clone 
rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git

uname -r
2.6.15-rc1-gcd52d1ee

I have found that although smartctl correctly displays different 
information for /dev/sda and /dev/sdb,
hdparm does not.
hdparm displays the same info for /dev/sda and /dev/sdb
i.e. hdparm -I /dev/sdb actually returns the information about /dev/sda

See below for details.

Is this a bug in hdparm or libata-devel ?

James


# smartctl -a /dev/sda
smartctl version 5.33 [i686-pc-linux-gnu] Copyright (C) 2002-4 Bruce Allen
Home page is http://smartmontools.sourceforge.net/

Device: ATA      ST3300831AS      Version: 3.01

# smartctl -a /dev/sdb
smartctl version 5.33 [i686-pc-linux-gnu] Copyright (C) 2002-4 Bruce Allen
Home page is http://smartmontools.sourceforge.net/

Device: ATA      ST3160023AS      Version: 3.18

# hdparm -I /dev/sda

/dev/sda:

ATA device, with non-removable media
	Model Number:       ST3300831AS
	Serial Number:      3NF0A9CQ
	Firmware Revision:  3.01

...

# hdparm -I /dev/sdb

/dev/sdb:

ATA device, with non-removable media
	Model Number:       ST3300831AS
	Serial Number:      3NF0A9CQ
	Firmware Revision:  3.01

Re: libata-passthrough problem.

[Mark Lord]

James Courtier-Dutton wrote:
> I am using (at 12-Nov-2005)
> git clone rsync://rsync.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git
> uname -r
> 2.6.15-rc1-gcd52d1ee
> I have found that although smartctl correctly displays different 
> information for /dev/sda and /dev/sdb, hdparm does not.
> hdparm displays the same info for /dev/sda and /dev/sdb
> i.e. hdparm -I /dev/sdb actually returns the information about /dev/sda
> 
> See below for details.
> 
> Is this a bug in hdparm or libata-devel ?
...
> # hdparm -I /dev/sda
> /dev/sda:
> ATA device, with non-removable media
>     Model Number:       ST3300831AS
>     Serial Number:      3NF0A9CQ
>     Firmware Revision:  3.01
> ...
> # hdparm -I /dev/sdb
> /dev/sdb:
> ATA device, with non-removable media
>     Model Number:       ST3300831AS
>     Serial Number:      3NF0A9CQ
>     Firmware Revision:  3.01

That would be a bug in libata passthru.

The reason the two programs behave differently in this example,
is that they likely use different HDIO_* ioctls, and libata/passthru
must have a bug in setting the unit number for the HDIO_DRIVE_CMD call.

We know it cannot be an hdparm bug, because HDIO_DRIVE_CMD does not
have a unit# parameter (and it would not be valid for libata to
accept one, either!).  All it does is open the /dev/ node,
and issue that call.   Use strace to verify that the correct device
is indeed being opened.

Is there a tarball of the latest libata-dev-git available?
Givent that, I could probably sift through libata/passthru and find/fix
the bug, but keeping up with git technology is something I gave up
trying to do ages ago --> it's really not for the casual developer.

Cheers

Re: libata-passthrough problem.

[Jeff Garzik]

Mark Lord wrote:
> Is there a tarball of the latest libata-dev-git available?
> Givent that, I could probably sift through libata/passthru and find/fix
> the bug, but keeping up with git technology is something I gave up
> trying to do ages ago --> it's really not for the casual developer.

Just download 2.6.15-rc1.

	Jeff

Re: libata-passthrough problem.

[Mark Lord]

Jeff Garzik wrote:
>
> Just download 2.6.15-rc1.
>

Okay, got it.

Jeff:  I cannot find anywhere in libata code,
where the ATA unit number gets ORed into tf->device.

If this is not done, then no PATA slave devices will
be addressed correctly, even for regular I/O.

Where is setting that bit done?
I'm sure it must be in there, I just don't see it.

Cheers

Re: libata-passthrough problem.

[Mark Lord]

Speaking of which, these comments are wrong:

/**
  *      ata_tf_from_fis - Convert SATA FIS to ATA taskfile
  *      @fis: Buffer from which data will be input
  *      @tf: Taskfile to output
  *
  *      Converts a standard ATA taskfile to a Serial ATA
  *      FIS structure (Register - Host to Device).
...

The second comment should probably read something like:

  *      Converts a serial ATA FIS structure to a
  *      standard ATA taskfile.

Re: libata-passthrough problem.

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
> 
>>
>> Just download 2.6.15-rc1.
>>
> 
> Okay, got it.
> 
> Jeff:  I cannot find anywhere in libata code,
> where the ATA unit number gets ORed into tf->device.
> 
> If this is not done, then no PATA slave devices will
> be addressed correctly, even for regular I/O.
> 
> Where is setting that bit done?
> I'm sure it must be in there, I just don't see it.

ata_tf_init() in include/linux/libata.h.

I admit with guilt that I had to search around a bit for it, too :)

	Jeff

Re: libata-passthrough problem.

[Jeff Garzik]

Mark Lord wrote:
> Speaking of which, these comments are wrong:
> 
> /**
>  *      ata_tf_from_fis - Convert SATA FIS to ATA taskfile
>  *      @fis: Buffer from which data will be input
>  *      @tf: Taskfile to output
>  *
>  *      Converts a standard ATA taskfile to a Serial ATA
>  *      FIS structure (Register - Host to Device).
> ...
> 
> The second comment should probably read something like:
> 
>  *      Converts a serial ATA FIS structure to a
>  *      standard ATA taskfile.

submit a patch, pretty please...

	Jeff

Re: libata-passthrough problem.

[Mark Lord]

Jeff Garzik wrote:
>
> ata_tf_init() in include/linux/libata.h.
> 
> I admit with guilt that I had to search around a bit for it, too :)

Thanks, Jeff.

In that case, the bug with passthru is obvious:

static unsigned int
ata_scsi_pass_thru(struct ata_queued_cmd *qc, const u8 *scsicmd)
{
...
		tf->device = scsicmd[13];    <<--- always forces ATA "master" unit 0
...
		tf->device = scsicmd[13];    <<--- always forces ATA "master" unit 0
...


Those lines should each do this instead:

		tf->device = qc->dev->devno ? (scsicmd[13] |  ATA_DEV1)
					    : (scsicmd[13] & ~ATA_DEV1);

I'll see about a patch for that shortly.

Cheers

[PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 234 bytes --]

This patch forces a correct master/slave unit bit
when sending libata commands via SCSI passthru.
The bit is left as-is for ports which do not
support slave devices.

Compiles, not tested.

Signed-off-by: Mark Lord <mlord@pobox.com>


[-- Attachment #2: passthru.patch --]
[-- Type: text/x-patch, Size: 532 bytes --]

--- linux-2.6.15-rc1/drivers/scsi/libata-scsi.c.orig	2005-11-11 20:43:36.000000000 -0500
+++ linux/drivers/scsi/libata-scsi.c	2005-11-12 18:45:43.000000000 -0500
@@ -2276,6 +2276,12 @@
 		tf->device = scsicmd[8];
 		tf->command = scsicmd[9];
 	}
+	/*
+	 * If applicable, enforce a correct master/slave bit for this device:
+	 */
+	if ((qc->ap->flags & ATA_FLAG_SLAVE_POSS))
+		tf->command = qc->dev->devno ?
+			(tf->command | ATA_DEV1) : (tf->command & ~ATA_DEV1);
 
 	/*
 	 * Filter SET_FEATURES - XFER MODE command -- otherwise,

[PATCH 2.6.15-rc1] libata: fix comments on ata_tf_from_fis()

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 95 bytes --]

Fix description on comments for ata_tf_from_fis().

Signed-off-by: Mark Lord <mlord@pobox.com>

[-- Attachment #2: comments.patch --]
[-- Type: text/x-patch, Size: 473 bytes --]

--- linux-2.6.15-rc1/drivers/scsi/libata-core.c.orig	2005-11-11 20:43:36.000000000 -0500
+++ linux/drivers/scsi/libata-core.c	2005-11-12 18:46:40.000000000 -0500
@@ -532,8 +532,7 @@
  *	@fis: Buffer from which data will be input
  *	@tf: Taskfile to output
  *
- *	Converts a standard ATA taskfile to a Serial ATA
- *	FIS structure (Register - Host to Device).
+ *	Converts a serial ATA FIS structure to a standard ATA taskfile.
  *
  *	LOCKING:
  *	Inherited from caller.

Re: [PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Jeff Garzik]

Mark Lord wrote:
> This patch forces a correct master/slave unit bit
> when sending libata commands via SCSI passthru.
> The bit is left as-is for ports which do not
> support slave devices.
> 
> Compiles, not tested.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> 
> ------------------------------------------------------------------------
> 
> --- linux-2.6.15-rc1/drivers/scsi/libata-scsi.c.orig	2005-11-11 20:43:36.000000000 -0500
> +++ linux/drivers/scsi/libata-scsi.c	2005-11-12 18:45:43.000000000 -0500
> @@ -2276,6 +2276,12 @@
>  		tf->device = scsicmd[8];
>  		tf->command = scsicmd[9];
>  	}
> +	/*
> +	 * If applicable, enforce a correct master/slave bit for this device:
> +	 */
> +	if ((qc->ap->flags & ATA_FLAG_SLAVE_POSS))

kill the extra parens

> +		tf->command = qc->dev->devno ?
> +			(tf->command | ATA_DEV1) : (tf->command & ~ATA_DEV1);

tf->command?  ;-)

Also, see if you figure out a way to send patches inline, without MIME 
attachments.

	Jeff

Re: [PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Jeff Garzik]

Mark Lord wrote:
> This patch forces a correct master/slave unit bit
> when sending libata commands via SCSI passthru.
> The bit is left as-is for ports which do not
> support slave devices.
> 
> Compiles, not tested.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> 
> ------------------------------------------------------------------------
> 
> --- linux-2.6.15-rc1/drivers/scsi/libata-scsi.c.orig	2005-11-11 20:43:36.000000000 -0500
> +++ linux/drivers/scsi/libata-scsi.c	2005-11-12 18:45:43.000000000 -0500
> @@ -2276,6 +2276,12 @@
>  		tf->device = scsicmd[8];
>  		tf->command = scsicmd[9];
>  	}
> +	/*
> +	 * If applicable, enforce a correct master/slave bit for this device:
> +	 */
> +	if ((qc->ap->flags & ATA_FLAG_SLAVE_POSS))
> +		tf->command = qc->dev->devno ?
> +			(tf->command | ATA_DEV1) : (tf->command & ~ATA_DEV1);

also, I would think you'd want to ditch the ATA_FLAG_SLAVE_POSS test. 
Otherwise userspace could pass an invalid bit through unimpeded.

	Jeff

Re: [PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
> 
>> This patch forces a correct master/slave unit bit
>> when sending libata commands via SCSI passthru.
>> The bit is left as-is for ports which do not
>> support slave devices.
...
> also, I would think you'd want to ditch the ATA_FLAG_SLAVE_POSS test. 
> Otherwise userspace could pass an invalid bit through unimpeded.

No.  Since that bit is currently "undefined" for SATA,
vendors may find a use for it in vendor-specific commands.
Thus, pass it through unless the lower level drive says
that it supports SLAVES.

Cheers

Re: [PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
> 
>> +    if ((qc->ap->flags & ATA_FLAG_SLAVE_POSS))
> 
> kill the extra parens

No.  Those parens are needed, to help gcc distinguish between
a common programming error and intentional use of "&" rather than "&&".

 > +        tf->command = qc->dev->devno ?
 > +            (tf->command | ATA_DEV1) : (tf->command & ~ATA_DEV1);

But I could kill *those* extran parens, while I do s/command/device/ .

Cheers

Re: [PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Mark Lord]

Mark Lord wrote:
> This patch forces a correct master/slave unit bit
> when sending libata commands via SCSI passthru.
> The bit is left as-is for ports which do not
> support slave devices.

Revised version: apply bitfix to tf->device (duh!),
and nuke extra parens.

Compiles, not tested.

Signed-off-by: Mark Lord <mlord@pobox.com>


  ------------------------------------------------------------------------

--- linux-2.6.15-rc1/drivers/scsi/libata-scsi.c.orig	2005-11-11 20:43:36.000000000 -0500
+++ linux/drivers/scsi/libata-scsi.c	2005-11-12 18:45:43.000000000 -0500
@@ -2276,6 +2276,12 @@
  		tf->device = scsicmd[8];
  		tf->command = scsicmd[9];
  	}
+	/*
+	 * If slave is possible, enforce correct master/slave bit for this port:
+	 */
+	if ((qc->ap->flags & ATA_FLAG_SLAVE_POSS))
+		tf->device = qc->dev->devno ?
+			tf->device | ATA_DEV1 : tf->command & ~ATA_DEV1;

  	/*
  	 * Filter SET_FEATURES - XFER MODE command -- otherwise,

Re: [PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Mark Lord]

AGAIN, with *feeling*.
(I'm in the middle of a complete distro-upgrade on my machine here,
so my tools are only half functional now.  Fix any residual formatting
issues yourself, please.)

- fixed whitespace, fixed third instance of "s/command/device".

> Compiles, not tested.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- linux-2.6.15-rc1/drivers/scsi/libata-scsi.c.orig    2005-11-11 20:43:36.000000000 -0500
+++ linux/drivers/scsi/libata-scsi.c    2005-11-12 18:45:43.000000000 -0500
@@ -2276,6 +2276,12 @@
		tf->device = scsicmd[8];
		tf->command = scsicmd[9];
	}
+	/*
+	 * If slave is possible, enforce correct master/slave bit for this port:
+	 */
+	if ((qc->ap->flags & ATA_FLAG_SLAVE_POSS))
+	tf->device = qc->dev->devno ?
+		tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;

	/*
	 * Filter SET_FEATURES - XFER MODE command -- otherwise,

Re: [PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
> 
>> Mark Lord wrote:
>>
>>> +    if ((qc->ap->flags & ATA_FLAG_SLAVE_POSS))
>>
>>
>> kill the extra parens
> 
> 
> No.  Those parens are needed, to help gcc distinguish between
> a common programming error and intentional use of "&" rather than "&&".

How is this different from any other 'flags' test in libata?

	Jeff

Re: [PATCH 2.6.15-rc1] libata: fix passthru for slave devices

[Mark Lord]

Jeff Garzik wrote:
> Mark Lord wrote:
>
>> No.  Those parens are needed, to help gcc distinguish between
>> a common programming error and intentional use of "&" rather than "&&".
> 
> How is this different from any other 'flags' test in libata?

ACK.  Go for it.

Cheers

Re: libata-passthrough problem.

[James Courtier-Dutton]

Mark Lord wrote:
> Jeff Garzik wrote:
> 
>>
>> ata_tf_init() in include/linux/libata.h.
>>
>> I admit with guilt that I had to search around a bit for it, too :)
> 
> 
> Thanks, Jeff.
> 
> In that case, the bug with passthru is obvious:
> 
> static unsigned int
> ata_scsi_pass_thru(struct ata_queued_cmd *qc, const u8 *scsicmd)
> {
> ...
>         tf->device = scsicmd[13];    <<--- always forces ATA "master" 
> unit 0
> ...
>         tf->device = scsicmd[13];    <<--- always forces ATA "master" 
> unit 0
> ...
> 
> 
> Those lines should each do this instead:
> 
>         tf->device = qc->dev->devno ? (scsicmd[13] |  ATA_DEV1)
>                         : (scsicmd[13] & ~ATA_DEV1);
> 
> I'll see about a patch for that shortly.
> 
> Cheers
> 
> 

Please tell me when you have fixed this, so that I could try it.
Please tell me where the fix will go, git libata-dev or linux-2.6

James

Re: libata-passthrough problem.

[Mark Lord]

James,

This is the version that fixes the problem.
Compiles, untested, no extraneous parenthesis.

Please give it a try, and let us know how it performs for you.

Signed-off-by: Mark Lord <mlord@pobox.com>


--- linux-2.6.15-rc1/drivers/scsi/libata-scsi.c.orig    2005-11-11 20:43:36.000000000 -0500
+++ linux/drivers/scsi/libata-scsi.c    2005-11-12 18:45:43.000000000 -0500
@@ -2276,6 +2276,12 @@
		tf->device = scsicmd[8];
		tf->command = scsicmd[9];
	}
+	/*
+	 * If slave is possible, enforce correct master/slave bit for this port:
+	*/
+	if (qc->ap->flags & ATA_FLAG_SLAVE_POSS)
+		tf->device = qc->dev->devno ?
+			tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;

	/*
	 * Filter SET_FEATURES - XFER MODE command -- otherwise,

Re: libata-passthrough problem.

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 74 bytes --]

And again, as an attachment, for people with modern mail clients.

Cheers

[-- Attachment #2: passthru.patch --]
[-- Type: text/x-patch, Size: 526 bytes --]

--- linux-2.6.15-rc1/drivers/scsi/libata-scsi.c.orig	2005-11-11 20:43:36.000000000 -0500
+++ linux/drivers/scsi/libata-scsi.c	2005-11-12 18:45:43.000000000 -0500
@@ -2276,6 +2276,12 @@
 		tf->device = scsicmd[8];
 		tf->command = scsicmd[9];
 	}
+	/*
+	 * If slave is possible, enforce correct master/slave bit for this port:
+	 */
+	if (qc->ap->flags & ATA_FLAG_SLAVE_POSS)
+		tf->device = qc->dev->devno ?
+			tf->device | ATA_DEV1 : tf->device & ~ATA_DEV1;
 
 	/*
 	 * Filter SET_FEATURES - XFER MODE command -- otherwise,

Re: libata-passthrough problem.

[James Courtier-Dutton]

Mark Lord wrote:
> And again, as an attachment, for people with modern mail clients.
> 
> Cheers
> 

Thank you

I have tested the patch against 2.6.15-rc1, and it seems to work fine.
Notice the different model numbers and serial numbers below, when before 
they were identical.


# hdparm -I /dev/sda

/dev/sda:

ATA device, with non-removable media
	Model Number:       ST3300831AS
	Serial Number:      3NF0A9CQ
	Firmware Revision:  3.01

# hdparm -I /dev/sdb

/dev/sdb:

ATA device, with non-removable media
	Model Number:       ST3160023AS
	Serial Number:      3JS2ZRPZ
	Firmware Revision:  3.18