libata-passthrough problem.

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

...

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

Just download 2.6.15-rc1.

Jeff

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

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

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.

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

Fix description on comments for ata_tf_from_fis().

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

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

ACK. Go for it.

Cheers

...

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

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

Compiles, not tested.

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

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

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

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

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

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

Jeff

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

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.

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

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

Cheers

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

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