[patch] datafab fix and unusual devices

[patch] datafab fix and unusual devices

[Andries.Brouwer]

datafab.c has an often-seen bug: the SCSI READ_CAPACITY command
does not need the number of sectors but the last sector.

I just tried the CF and SM parts of a 5-in-1 card reader.
The CF part works with US_PR_DATAFAB when the bug mentioned is fixed.
The SM part works with US_PR_SDDR55.
(Revision Number is 17.08 - that in case the 0000-ffff
should prove to be too optimistic.)

We still must discuss what setup to use for readers like this -
I have several of them - that require different drivers for
different LUNs. As it is now one has to compile usb-storage
twice, once with CONFIG_USB_STORAGE_DATAFAB defined and once
without, and remove one usb-storage.ko and insert the other
to go from CF to SM. (And that hangs with 2.6.4 so a reboot
is required..)

Andries

[Two years ago I had a very general setup, and when mdharm
didnt like it, a 2-lun setup, the minimal needed for the
devices I had at that time. They must still live in the
archives somewhere. I forgot all details so have no opinion today.]


diff -uprN -X /linux/dontdiff a/drivers/usb/storage/datafab.c b/drivers/usb/storage/datafab.c
--- a/drivers/usb/storage/datafab.c	2004-02-05 19:55:20.000000000 +0100
+++ b/drivers/usb/storage/datafab.c	2004-03-30 00:00:36.000000000 +0200
@@ -539,8 +539,8 @@ int datafab_transport(Scsi_Cmnd * srb, s
 			  info->sectors, info->ssize);
 
 		// build the reply
-		//
-		((u32 *) ptr)[0] = cpu_to_be32(info->sectors);
+		// we need the last sector, not the number of sectors
+		((u32 *) ptr)[0] = cpu_to_be32(info->sectors - 1);
 		((u32 *) ptr)[1] = cpu_to_be32(info->ssize);
 		usb_stor_set_xfer_buf(ptr, 8, srb);
 
diff -uprN -X /linux/dontdiff a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h
--- a/drivers/usb/storage/unusual_devs.h	2004-03-28 17:11:50.000000000 +0200
+++ b/drivers/usb/storage/unusual_devs.h	2004-03-29 23:59:47.000000000 +0200
@@ -710,6 +710,21 @@ UNUSUAL_DEV(  0x0bf6, 0xa001, 0x0100, 0x
 		0 ),
 #endif
 
+#ifdef CONFIG_USB_STORAGE_DATAFAB
+UNUSUAL_DEV( 0x0c0b, 0xa109, 0x0000, 0xffff,
+	       "Acomdata",
+	       "CF",
+	       US_SC_SCSI, US_PR_DATAFAB, NULL,
+	       US_FL_SINGLE_LUN ),
+#endif
+#ifdef CONFIG_USB_STORAGE_SDDR55
+UNUSUAL_DEV( 0x0c0b, 0xa109, 0x0000, 0xffff,
+	       "Acomdata",
+	       "SM",
+	       US_SC_SCSI, US_PR_SDDR55, NULL,
+	       US_FL_SINGLE_LUN ),
+#endif
+
 /* Submitted by Joris Struyve <joris@struyve.be> */
 UNUSUAL_DEV( 0x0d96, 0x410a, 0x0001, 0xffff,
 		"Medion",

Re: [patch] datafab fix and unusual devices

[Matthew Dharm]

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

On Tue, Mar 30, 2004 at 12:44:09AM +0200, Andries.Brouwer@cwi.nl wrote:
> datafab.c has an often-seen bug: the SCSI READ_CAPACITY command
> does not need the number of sectors but the last sector.

The first part of the patch (which fixes this bug) certainly looks good to
me for 2.6 -- we need to check that 2.4 doesn't also have the problem.

> I just tried the CF and SM parts of a 5-in-1 card reader.
> The CF part works with US_PR_DATAFAB when the bug mentioned is fixed.
> The SM part works with US_PR_SDDR55.
> (Revision Number is 17.08 - that in case the 0000-ffff
> should prove to be too optimistic.)
> 
> We still must discuss what setup to use for readers like this -
> I have several of them - that require different drivers for
> different LUNs. As it is now one has to compile usb-storage
> twice, once with CONFIG_USB_STORAGE_DATAFAB defined and once
> without, and remove one usb-storage.ko and insert the other
> to go from CF to SM. (And that hangs with 2.6.4 so a reboot
> is required..)

The second part of your patch I don't like (it seems to violate the
'principal of least suprise' to me).... but I'm also ready and willing to
consider a beter alternative.  What do you suggest?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

It was a new hope.
					-- Dust Puppy
User Friendly, 12/25/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch] datafab fix and unusual devices

[Andries Brouwer]

On Mon, Mar 29, 2004 at 03:15:08PM -0800, Matthew Dharm wrote:
> On Tue, Mar 30, 2004 at 12:44:09AM +0200, Andries.Brouwer@cwi.nl wrote:
> > datafab.c has an often-seen bug: the SCSI READ_CAPACITY command
> > does not need the number of sectors but the last sector.
> 
> The first part of the patch (which fixes this bug) certainly looks good to
> me for 2.6 -- we need to check that 2.4 doesn't also have the problem.
> 
> The second part of your patch I don't like (it seems to violate the
> 'principal of least suprise' to me).... but I'm also ready and willing to
> consider a beter alternative.  What do you suggest?

Well, the entire patch should be applied. Nothing wrong with it.

That will enable people to use (0x0c0b,0xa109) to read CF,
or to read SM, but not both. (Without the patch the device
does not work at all.)
The situation is precisely the same as that for (0x07c4,0xa109).

To do something better we need infrastructure that does not exist today,
at least not in the vanilla kernel. That is why I call for discussion.

The points are ordinary use and error recovery.
For ordinary use the main point seems to be the us->extra
pointer to private data. Since each driver needs private data,
a single pointer does not suffice.

Andries

Re: [patch] datafab fix and unusual devices

[Greg KH]

On Tue, Mar 30, 2004 at 12:44:09AM +0200, Andries.Brouwer@cwi.nl wrote:
> datafab.c has an often-seen bug: the SCSI READ_CAPACITY command
> does not need the number of sectors but the last sector.

Applied, thanks.

greg k-h