[PATCH v2 0/3] usb: gadget: storage: Make CD-ROM emulation work with Mac OS-X

[PATCH v2 0/3] usb: gadget: storage: Make CD-ROM emulation work with Mac OS-X

[Roger Quadros]

Hi,

Mac OS-X expects CD-ROM TOC in raw format (i.e. format:2). It also
sends the READ_TOC CDB in old style SFF8020i format. i.e. 2 format bits
are encoded in MSBs of CDB byte 9.

The first patch introduces a fsg_get_toc() helper which builds the
Table of Contents (TOC) that can be used for the READ_TOC command.
The remaining two patches simply make file_storage gadget and mass_storage
gadget use the new helper function.

Changes in v2:
- Review comments incorporated.

---
Roger Quadros (3):
  usb: gadget: storage: Add fsg_get_toc helper
  usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X
  usb: gadget: f_mass_storage: Make CD-ROM emulation work with Mac OS-X

 drivers/usb/gadget/f_mass_storage.c |   31 +++++++++-------
 drivers/usb/gadget/file_storage.c   |   31 +++++++++-------
 drivers/usb/gadget/storage_common.c |   68 +++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+), 26 deletions(-)

[PATCH v2 1/3] usb: gadget: storage: Add fsg_get_toc helper

[Roger Quadros]

From: Roger Quadros <roger.quadros@nokia.com>

fsg_get_toc can be used by both storage gadgets to build a CD-ROM
Table of Contents which can be used as data for READ_TOC command.
Currently fsg_get_toc supports two TOC formats, i.e.
format 0: Formatted TOC and format 2: Raw TOC.

Raw TOC is required for CD-ROM emulation to work with Mac OS-X.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/storage_common.c |   68 +++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index b015561..d47f148 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -655,6 +655,74 @@ static void store_cdrom_address(u8 *dest, int msf, u32 addr)
 	}
 }
 
+/**
+ * fsg_get_toc() - Builds a TOC with required format @format.
+ * @curlun: The LUN for which the TOC has to be built
+ * @msf: Min Sec Frame format or LBA format for address
+ * @format: TOC format code
+ * @buf: The buffer into which the TOC is built
+ *
+ * Builds a Table of Content which can be used as data for READ_TOC command.
+ * The TOC simulates a single session, single track CD-ROM mode 1 disc.
+ *
+ * Returns number of bytes written to @buf, -EINVAL if format not supported.
+ */
+static int fsg_get_toc(struct fsg_lun *curlun, int msf, int format, u8 *buf)
+{
+	int i, len;
+	switch (format) {
+	case 0:
+		/* Formatted TOC */
+		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
+		memset(buf, 0, len);
+		len -= 2;		/* TOC Length excludes length field */
+		buf[1] = len;		/* TOC data length */
+		buf[2] = 1;		/* First track number */
+		buf[3] = 1;		/* Last track number */
+		buf[5] = 0x16;		/* Data track, copying allowed */
+		buf[6] = 0x01;		/* Only track is number 1 */
+		store_cdrom_address(&buf[8], msf, 0);
+
+		buf[13] = 0x16;		/* Lead-out track is data */
+		buf[14] = 0xAA;		/* Lead-out track number */
+		store_cdrom_address(&buf[16], msf, curlun->num_sectors);
+		return len;
+		break;
+
+	case 2:
+		/* Raw TOC */
+		len = 4 + 3*11;		/* 4 byte header + 3 descriptors */
+		memset(buf, 0, len);	/* Header + A0, A1 & A2 descriptors */
+		len -= 2;		/* TOC Length excludes length field */
+		buf[1] = len;		/* TOC data length */
+		buf[2] = 1;		/* First complete session */
+		buf[3] = 1;		/* Last complete session */
+
+		buf += 4;
+		/* fill in A0, A1 and A2 points */
+		for (i = 0; i < 3; i++) {
+			buf[0] = 1;	/* Session number */
+			buf[1] = 0x16;	/* Data track, copying allowed */
+			/* 2 - Track number 0 ->  TOC */
+			buf[3] = 0xA0 + i; /* A0, A1, A2 point */
+			/* 4, 5, 6 - Min, sec, frame is zero */
+			buf[8] = 1;	/* Pmin: last track number */
+			buf += 11;	/* go to next track descriptor */
+		}
+		buf -= 11;		/* go back to A2 descriptor */
+
+		/* For A2, 7, 8, 9, 10 - zero, Pmin, Psec, Pframe of Lead out */
+		store_cdrom_address(&buf[7], msf, curlun->num_sectors);
+
+		return len;
+		break;
+	default:
+		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
+		return -EINVAL;
+		break;
+	}
+}
+
 
 /*-------------------------------------------------------------------------*/
 
-- 
1.6.0.4

[PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X

[Roger Quadros]

From: Roger Quadros <roger.quadros@nokia.com>

Mac OS-X expects CD-ROM TOC in raw format (i.e. format:2). It also
sends the READ_TOC CDB in old style SFF8020i format. i.e. 2 format bits
are encoded in MSBs of CDB byte 9.

This patch will enable CD-ROM emulation to work with Mac OS-X. Tested on
Mac OS X v10.6.3.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/file_storage.c |   31 ++++++++++++++++++-------------
 1 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index a6eacb5..03beb2e 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -1696,6 +1696,8 @@ static int do_read_toc(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 	int		msf = fsg->cmnd[1] & 0x02;
 	int		start_track = fsg->cmnd[6];
 	u8		*buf = (u8 *) bh->buf;
+	u8		format;
+	int		ret;
 
 	if ((fsg->cmnd[1] & ~0x02) != 0 ||		/* Mask away MSF */
 			start_track > 1) {
@@ -1703,18 +1705,21 @@ static int do_read_toc(struct fsg_dev *fsg, struct fsg_buffhd *bh)
 		return -EINVAL;
 	}
 
-	memset(buf, 0, 20);
-	buf[1] = (20-2);		/* TOC data length */
-	buf[2] = 1;			/* First track number */
-	buf[3] = 1;			/* Last track number */
-	buf[5] = 0x16;			/* Data track, copying allowed */
-	buf[6] = 0x01;			/* Only track is number 1 */
-	store_cdrom_address(&buf[8], msf, 0);
+	format = fsg->cmnd[2] & 0xf;
+	/*
+	 * Check if CDB is old style SFF-8020i
+	 * i.e. format is in 2 MSBs of byte 9
+	 * Mac OS-X host sends us this.
+	 */
+	if (format == 0)
+		format = (fsg->cmnd[9] >> 6) & 0x3;
 
-	buf[13] = 0x16;			/* Lead-out track is data */
-	buf[14] = 0xAA;			/* Lead-out track number */
-	store_cdrom_address(&buf[16], msf, curlun->num_sectors);
-	return 20;
+	ret = fsg_get_toc(curlun, msf, format, buf);
+	if (ret < 0) {
+		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
+		return -EINVAL;
+	}
+	return ret;
 }
 
 
@@ -2486,7 +2491,7 @@ static int do_scsi_command(struct fsg_dev *fsg)
 			goto unknown_cmnd;
 		fsg->data_size_from_cmnd = get_unaligned_be16(&fsg->cmnd[7]);
 		if ((reply = check_command(fsg, 10, DATA_DIR_TO_HOST,
-				(7<<6) | (1<<1), 1,
+				(0xf<<6) | (1<<1), 1,
 				"READ TOC")) == 0)
 			reply = do_read_toc(fsg, bh);
 		break;
-- 
1.6.0.4

[PATCH v2 3/3] usb: gadget: f_mass_storage: Make CD-ROM emulation work with Mac OS-X

[Roger Quadros]

From: Roger Quadros <roger.quadros@nokia.com>

Mac OS-X expects CD-ROM TOC in raw format (i.e. format:2). It also
sends the READ_TOC CDB in old style SFF8020i format. i.e. 2 format bits
are encoded in MSBs of CDB byte 9.

This patch will enable CD-ROM emulation to work with Mac OS-X. Tested on
Mac OS X v10.6.3.

Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
---
 drivers/usb/gadget/f_mass_storage.c |   31 ++++++++++++++++++-------------
 1 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 6d8e533..ef6c65a 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -1315,6 +1315,8 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
 	int		msf = common->cmnd[1] & 0x02;
 	int		start_track = common->cmnd[6];
 	u8		*buf = (u8 *)bh->buf;
+	u8		format;
+	int		ret;
 
 	if ((common->cmnd[1] & ~0x02) != 0 ||	/* Mask away MSF */
 			start_track > 1) {
@@ -1322,18 +1324,21 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
 		return -EINVAL;
 	}
 
-	memset(buf, 0, 20);
-	buf[1] = (20-2);		/* TOC data length */
-	buf[2] = 1;			/* First track number */
-	buf[3] = 1;			/* Last track number */
-	buf[5] = 0x16;			/* Data track, copying allowed */
-	buf[6] = 0x01;			/* Only track is number 1 */
-	store_cdrom_address(&buf[8], msf, 0);
+	format = common->cmnd[2] & 0xf;
+	/*
+	 * Check if CDB is old style SFF-8020i
+	 * i.e. format is in 2 MSBs of byte 9
+	 * Mac OS-X host sends us this.
+	 */
+	if (format == 0)
+		format = (common->cmnd[9] >> 6) & 0x3;
 
-	buf[13] = 0x16;			/* Lead-out track is data */
-	buf[14] = 0xAA;			/* Lead-out track number */
-	store_cdrom_address(&buf[16], msf, curlun->num_sectors);
-	return 20;
+	ret = fsg_get_toc(curlun, msf, format, buf);
+	if (ret < 0) {
+		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
+		return -EINVAL;
+	}
+	return ret;
 }
 
 static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -2103,7 +2108,7 @@ static int do_scsi_command(struct fsg_common *common)
 		common->data_size_from_cmnd =
 			get_unaligned_be16(&common->cmnd[7]);
 		reply = check_command(common, 10, DATA_DIR_TO_HOST,
-				      (7<<6) | (1<<1), 1,
+				      (0xf<<6) | (1<<1), 1,
 				      "READ TOC");
 		if (reply == 0)
 			reply = do_read_toc(common, bh);
-- 
1.6.0.4

Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X

[Roger Quadros]

Hi,

On 03/21/2011 12:59 PM, ext Roger Quadros wrote:
> @@ -1703,18 +1705,21 @@ static int do_read_toc(struct fsg_dev *fsg, struct fsg_buffhd *bh)
>   		return -EINVAL;
>   	}
>
> -	memset(buf, 0, 20);
> -	buf[1] = (20-2);		/* TOC data length */
> -	buf[2] = 1;			/* First track number */
> -	buf[3] = 1;			/* Last track number */
> -	buf[5] = 0x16;			/* Data track, copying allowed */
> -	buf[6] = 0x01;			/* Only track is number 1 */
> -	store_cdrom_address(&buf[8], msf, 0);
> +	format = fsg->cmnd[2]&  0xf;
> +	/*
> +	 * Check if CDB is old style SFF-8020i
> +	 * i.e. format is in 2 MSBs of byte 9
> +	 * Mac OS-X host sends us this.
> +	 */
> +	if (format == 0)
> +		format = (fsg->cmnd[9]>>  6)&  0x3;
>
> -	buf[13] = 0x16;			/* Lead-out track is data */
> -	buf[14] = 0xAA;			/* Lead-out track number */
> -	store_cdrom_address(&buf[16], msf, curlun->num_sectors);
> -	return 20;
> +	ret = fsg_get_toc(curlun, msf, format, buf);
> +	if (ret<  0) {
> +		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
> +		return -EINVAL;
> +	}
> +	return ret;
>   }


I have a question here.

The host can request us to send less or more than the actual TOC size, since it 
has no clue how big it is.
e.g. Linux host requests us to send only 12 bytes even though our formatted TOC 
length is 20. In this case should we return fsg->data_size_from_cmnd instead of 
actual TOC length?

e.g. Mac requests us to send 65534 bytes but our RAW TOC length is 37.
The file storage driver seems to be zero padding our data response. So we 
respond with 65534 bytes, 37 of TOC and remaining zero padded.

Can we do something like this to avoid unnecessary zero padded transfers?

         ret = fsg_get_toc(curlun, msf, format, buf);
         if (ret < 0) {
                 curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
                 return -EINVAL;
         } else if (ret > fsg->data_size_from_cmnd) {
                 ret = fsg->data_size_from_cmnd;
         } else {
                 fsg->residue = ret;
         }
         return ret;

-- 
regards,
-roger

Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X

[Alan Stern]

On Tue, 22 Mar 2011, Roger Quadros wrote:

> I have a question here.
> 
> The host can request us to send less or more than the actual TOC size, since it 
> has no clue how big it is.
> e.g. Linux host requests us to send only 12 bytes even though our formatted TOC 
> length is 20. In this case should we return fsg->data_size_from_cmnd instead of 
> actual TOC length?

No.  Always return the actual TOC length.

> e.g. Mac requests us to send 65534 bytes but our RAW TOC length is 37.
> The file storage driver seems to be zero padding our data response. So we 
> respond with 65534 bytes, 37 of TOC and remaining zero padded.
> 
> Can we do something like this to avoid unnecessary zero padded transfers?
> 
>          ret = fsg_get_toc(curlun, msf, format, buf);
>          if (ret < 0) {
>                  curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
>                  return -EINVAL;
>          } else if (ret > fsg->data_size_from_cmnd) {
>                  ret = fsg->data_size_from_cmnd;
>          } else {
>                  fsg->residue = ret;
>          }
>          return ret;

Not needed (and not correct).  The code at the end of do_scsi_command()  
already does this:

		reply = min((u32) reply, fsg->data_size_from_cmnd);
		...
		fsg->residue -= reply;

Alan Stern

Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X

[Roger Quadros]

On 03/22/2011 04:26 PM, ext Alan Stern wrote:
> On Tue, 22 Mar 2011, Roger Quadros wrote:
>
>> I have a question here.
>>
>> The host can request us to send less or more than the actual TOC size, since it
>> has no clue how big it is.
>> e.g. Linux host requests us to send only 12 bytes even though our formatted TOC
>> length is 20. In this case should we return fsg->data_size_from_cmnd instead of
>> actual TOC length?
>
> No.  Always return the actual TOC length.

OK.

>
>> e.g. Mac requests us to send 65534 bytes but our RAW TOC length is 37.
>> The file storage driver seems to be zero padding our data response. So we
>> respond with 65534 bytes, 37 of TOC and remaining zero padded.
>>
>> Can we do something like this to avoid unnecessary zero padded transfers?
>>
>>           ret = fsg_get_toc(curlun, msf, format, buf);
>>           if (ret<  0) {
>>                   curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
>>                   return -EINVAL;
>>           } else if (ret>  fsg->data_size_from_cmnd) {
>>                   ret = fsg->data_size_from_cmnd;
>>           } else {
>>                   fsg->residue = ret;
>>           }
>>           return ret;
>
> Not needed (and not correct).  The code at the end of do_scsi_command()
> already does this:
>
> 		reply = min((u32) reply, fsg->data_size_from_cmnd);
> 		...
> 		fsg->residue -= reply;
>

Oh yes, this explains why to return the actual TOC length in do_read_toc().

However, if host asked for data more than the TOC length then residue will be 
greater than zero and this will result in zero padded transfers.

Not a big issue but should we be fixing it?

one way could be to set fsg->residue to actual TOC length. in do_read_toc().

-- 
regards,
-roger

Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X

[Alan Stern]

On Tue, 22 Mar 2011, Roger Quadros wrote:

> However, if host asked for data more than the TOC length then residue will be 
> greater than zero and this will result in zero padded transfers.
> 
> Not a big issue but should we be fixing it?
> 
> one way could be to set fsg->residue to actual TOC length. in do_read_toc().

The current behavior is required by the the Bulk-Only Transport
specification.  The spec says (section 6.7.2, case (4) or (5)):

If the device intends to send less data than the host indicated, then:
    The device shall send the intended data.
	The device may send fill data to pad up to a total of 
	dCBWDataTransferLength.
	If the device actually transfers less data than the host indicated, 
	then:
	    The device may end the transfer with a short packet.
	    The device shall STALL the Bulk-In pipe.

You're loading the mass-storage gadget with the "stall=n" module
parameter, right?  Therefore the driver is not allowed to halt the
Bulk-In endpoint, and therefore the driver is not allowed to send less
data than the host indicated, which means the data has to be padded.

On the other hand, I don't think any implementations would get upset if 
we simply ended the transfer with a short packet instead of adhering 
strictly to the spec.

The patch below should do what you want.  I haven't tested it.

Alan Stern



 drivers/usb/gadget/f_mass_storage.c |   54 +++++++-----------------------------
 drivers/usb/gadget/file_storage.c   |   53 ++++++++---------------------------
 2 files changed, 23 insertions(+), 84 deletions(-)

Index: usb-2.6/drivers/usb/gadget/f_mass_storage.c
===================================================================
--- usb-2.6.orig/drivers/usb/gadget/f_mass_storage.c
+++ usb-2.6/drivers/usb/gadget/f_mass_storage.c
@@ -1584,37 +1584,6 @@ static int wedge_bulk_in_endpoint(struct
 	return rc;
 }
 
-static int pad_with_zeros(struct fsg_dev *fsg)
-{
-	struct fsg_buffhd	*bh = fsg->common->next_buffhd_to_fill;
-	u32			nkeep = bh->inreq->length;
-	u32			nsend;
-	int			rc;
-
-	bh->state = BUF_STATE_EMPTY;		/* For the first iteration */
-	fsg->common->usb_amount_left = nkeep + fsg->common->residue;
-	while (fsg->common->usb_amount_left > 0) {
-
-		/* Wait for the next buffer to be free */
-		while (bh->state != BUF_STATE_EMPTY) {
-			rc = sleep_thread(fsg->common);
-			if (rc)
-				return rc;
-		}
-
-		nsend = min(fsg->common->usb_amount_left, FSG_BUFLEN);
-		memset(bh->buf + nkeep, 0, nsend - nkeep);
-		bh->inreq->length = nsend;
-		bh->inreq->zero = 0;
-		start_transfer(fsg, fsg->bulk_in, bh->inreq,
-			       &bh->inreq_busy, &bh->state);
-		bh = fsg->common->next_buffhd_to_fill = bh->next;
-		fsg->common->usb_amount_left -= nsend;
-		nkeep = 0;
-	}
-	return 0;
-}
-
 static int throw_away_data(struct fsg_common *common)
 {
 	struct fsg_buffhd	*bh;
@@ -1702,6 +1671,10 @@ static int finish_reply(struct fsg_commo
 		if (common->data_size == 0) {
 			/* Nothing to send */
 
+		/* Don't know what to do if common->fsg is NULL */
+		} else if (!common->fsg) {
+			rc = -EIO;
+
 		/* If there's no residue, simply send the last buffer */
 		} else if (common->residue == 0) {
 			bh->inreq->zero = 0;
@@ -1710,24 +1683,19 @@ static int finish_reply(struct fsg_commo
 			common->next_buffhd_to_fill = bh->next;
 
 		/*
-		 * For Bulk-only, if we're allowed to stall then send the
-		 * short packet and halt the bulk-in endpoint.  If we can't
-		 * stall, pad out the remaining data with 0's.
+		 * For Bulk-only, mark the end of the data with a short
+		 * packet.  If we are allowed to stall, halt the bulk-in
+		 * endpoint.  (Note: This violates the Bulk-Only Transport
+		 * specification, which requires us to pad the data if we
+		 * don't halt the endpoint.  Presumably nobody will mind.)
 		 */
-		} else if (common->can_stall) {
+		} else {
 			bh->inreq->zero = 1;
 			if (!start_in_transfer(common, bh))
-				/* Don't know what to do if
-				 * common->fsg is NULL */
 				rc = -EIO;
 			common->next_buffhd_to_fill = bh->next;
-			if (common->fsg)
+			if (common->can_stall)
 				rc = halt_bulk_in_endpoint(common->fsg);
-		} else if (fsg_is_set(common)) {
-			rc = pad_with_zeros(common->fsg);
-		} else {
-			/* Don't know what to do if common->fsg is NULL */
-			rc = -EIO;
 		}
 		break;
 
Index: usb-2.6/drivers/usb/gadget/file_storage.c
===================================================================
--- usb-2.6.orig/drivers/usb/gadget/file_storage.c
+++ usb-2.6/drivers/usb/gadget/file_storage.c
@@ -1947,37 +1947,6 @@ static int wedge_bulk_in_endpoint(struct
 	return rc;
 }
 
-static int pad_with_zeros(struct fsg_dev *fsg)
-{
-	struct fsg_buffhd	*bh = fsg->next_buffhd_to_fill;
-	u32			nkeep = bh->inreq->length;
-	u32			nsend;
-	int			rc;
-
-	bh->state = BUF_STATE_EMPTY;		// For the first iteration
-	fsg->usb_amount_left = nkeep + fsg->residue;
-	while (fsg->usb_amount_left > 0) {
-
-		/* Wait for the next buffer to be free */
-		while (bh->state != BUF_STATE_EMPTY) {
-			rc = sleep_thread(fsg);
-			if (rc)
-				return rc;
-		}
-
-		nsend = min(fsg->usb_amount_left, (u32) mod_data.buflen);
-		memset(bh->buf + nkeep, 0, nsend - nkeep);
-		bh->inreq->length = nsend;
-		bh->inreq->zero = 0;
-		start_transfer(fsg, fsg->bulk_in, bh->inreq,
-				&bh->inreq_busy, &bh->state);
-		bh = fsg->next_buffhd_to_fill = bh->next;
-		fsg->usb_amount_left -= nsend;
-		nkeep = 0;
-	}
-	return 0;
-}
-
 static int throw_away_data(struct fsg_dev *fsg)
 {
 	struct fsg_buffhd	*bh;
@@ -2082,18 +2051,20 @@ static int finish_reply(struct fsg_dev *
 			}
 		}
 
-		/* For Bulk-only, if we're allowed to stall then send the
-		 * short packet and halt the bulk-in endpoint.  If we can't
-		 * stall, pad out the remaining data with 0's. */
+		/*
+		 * For Bulk-only, mark the end of the data with a short
+		 * packet.  If we are allowed to stall, halt the bulk-in
+		 * endpoint.  (Note: This violates the Bulk-Only Transport
+		 * specification, which requires us to pad the data if we
+		 * don't halt the endpoint.  Presumably nobody will mind.)
+		 */
 		else {
-			if (mod_data.can_stall) {
-				bh->inreq->zero = 1;
-				start_transfer(fsg, fsg->bulk_in, bh->inreq,
-						&bh->inreq_busy, &bh->state);
-				fsg->next_buffhd_to_fill = bh->next;
+			bh->inreq->zero = 1;
+			start_transfer(fsg, fsg->bulk_in, bh->inreq,
+					&bh->inreq_busy, &bh->state);
+			fsg->next_buffhd_to_fill = bh->next;
+			if (mod_data.can_stall)
 				rc = halt_bulk_in_endpoint(fsg);
-			} else
-				rc = pad_with_zeros(fsg);
 		}
 		break;
 

Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X

[Roger Quadros]

On 03/22/2011 05:13 PM, ext Alan Stern wrote:
> On Tue, 22 Mar 2011, Roger Quadros wrote:
>
>> However, if host asked for data more than the TOC length then residue will be
>> greater than zero and this will result in zero padded transfers.
>>
>> Not a big issue but should we be fixing it?
>>
>> one way could be to set fsg->residue to actual TOC length. in do_read_toc().
>
> The current behavior is required by the the Bulk-Only Transport
> specification.  The spec says (section 6.7.2, case (4) or (5)):
>
> If the device intends to send less data than the host indicated, then:
>      The device shall send the intended data.
> 	The device may send fill data to pad up to a total of
> 	dCBWDataTransferLength.
> 	If the device actually transfers less data than the host indicated,
> 	then:
> 	    The device may end the transfer with a short packet.
> 	    The device shall STALL the Bulk-In pipe.
>
> You're loading the mass-storage gadget with the "stall=n" module
> parameter, right?  Therefore the driver is not allowed to halt the

Yes i'm loading it with "stall=n".

> Bulk-In endpoint, and therefore the driver is not allowed to send less
> data than the host indicated, which means the data has to be padded.

Thanks. i got it now.

>
> On the other hand, I don't think any implementations would get upset if
> we simply ended the transfer with a short packet instead of adhering
> strictly to the spec.
>
> The patch below should do what you want.  I haven't tested it.

I tried your patch with the CD-ROM implementation and it works perfectly. I do 
not see the unnecessary zero padded transfers any more.

Do you think we should have this patch in? with the risk of not strictly 
adhering to spec for cases where controller cannot stall?

Maybe the term "controller cannot stall" itself does not adhere to bulk-only 
transport spec :).

>   static int throw_away_data(struct fsg_common *common)
>   {
>   	struct fsg_buffhd	*bh;
> @@ -1702,6 +1671,10 @@ static int finish_reply(struct fsg_commo
>   		if (common->data_size == 0) {
>   			/* Nothing to send */
>
> +		/* Don't know what to do if common->fsg is NULL */
> +		} else if (!common->fsg) {
> +			rc = -EIO;
> +
>   		/* If there's no residue, simply send the last buffer */
>   		} else if (common->residue == 0) {
>   			bh->inreq->zero = 0;
> @@ -1710,24 +1683,19 @@ static int finish_reply(struct fsg_commo
>   			common->next_buffhd_to_fill = bh->next;
>
>   		/*
> -		 * For Bulk-only, if we're allowed to stall then send the
> -		 * short packet and halt the bulk-in endpoint.  If we can't
> -		 * stall, pad out the remaining data with 0's.
> +		 * For Bulk-only, mark the end of the data with a short
> +		 * packet.  If we are allowed to stall, halt the bulk-in
> +		 * endpoint.  (Note: This violates the Bulk-Only Transport
> +		 * specification, which requires us to pad the data if we

violates the spec only if we are not allowed to stall (i.e. stall=n) right?

> +		 * don't halt the endpoint.  Presumably nobody will mind.)
>   		 */

-- 
regards,
-roger

Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X

[Alan Stern]

On Wed, 23 Mar 2011, Roger Quadros wrote:

> > On the other hand, I don't think any implementations would get upset if
> > we simply ended the transfer with a short packet instead of adhering
> > strictly to the spec.
> >
> > The patch below should do what you want.  I haven't tested it.
> 
> I tried your patch with the CD-ROM implementation and it works perfectly. I do 
> not see the unnecessary zero padded transfers any more.
> 
> Do you think we should have this patch in? with the risk of not strictly 
> adhering to spec for cases where controller cannot stall?

There already is another place where not stalling forces the driver to 
violate the spec.  I don't think this makes things much worse... but it 
is a significant change in behavior.

This affects Michal's driver too; we should ask his opinion.  Michal, 
in case you didn't see it, the proposed patch is here:

	http://marc.info/?l=linux-usb&m=130080683528607&w=2

> Maybe the term "controller cannot stall" itself does not adhere to bulk-only 
> transport spec :).

True enough.  However I believe USB flash drives behave this way: They 
don't stall and they don't pad their data.

> > @@ -1710,24 +1683,19 @@ static int finish_reply(struct fsg_commo
> >   			common->next_buffhd_to_fill = bh->next;
> >
> >   		/*
> > -		 * For Bulk-only, if we're allowed to stall then send the
> > -		 * short packet and halt the bulk-in endpoint.  If we can't
> > -		 * stall, pad out the remaining data with 0's.
> > +		 * For Bulk-only, mark the end of the data with a short
> > +		 * packet.  If we are allowed to stall, halt the bulk-in
> > +		 * endpoint.  (Note: This violates the Bulk-Only Transport
> > +		 * specification, which requires us to pad the data if we
> 
> violates the spec only if we are not allowed to stall (i.e. stall=n) right?

Right.

> > +		 * don't halt the endpoint.  Presumably nobody will mind.)
> >   		 */

Alan Stern

Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X

[Michal Nazarewicz]

>>> On the other hand, I don't think any implementations would get upset  
>>> if we simply ended the transfer with a short packet instead of
>>> adhering strictly to the spec.

> On Wed, 23 Mar 2011, Roger Quadros wrote:
>> I tried your patch with the CD-ROM implementation and it works  
>> perfectly. I do not see the unnecessary zero padded transfers
>> any more.
>>
>> Do you think we should have this patch in? with the risk of not strictly
>> adhering to spec for cases where controller cannot stall?

On Wed, 23 Mar 2011 16:17:32 +0100, Alan Stern wrote:
> There already is another place where not stalling forces the driver to
> violate the spec.  I don't think this makes things much worse... but it
> is a significant change in behavior.
>
> This affects Michal's driver too; we should ask his opinion.  Michal,
> in case you didn't see it, the proposed patch is here:
>
> 	http://marc.info/?l=linux-usb&m=130080683528607&w=2

Thanks.  I don't have original message so I'll reply here:

> +		/* Don't know what to do if common->fsg is NULL */
> +		} else if (!common->fsg) {
> +			rc = -EIO;
> +

I'd change that to "if (!fsg_is_set(common))".  fsg_is_set() prints out a  
warning
if common->fsg is not set and I think that's something we want.

(Come to think of it, this check should be done at the beginning of the  
function
simplifying the rest of the function, but that's unrelated, I'll do it  
myself I
guess.)

Other then that, looks good to me.

As for violating the spec, I don't think I have anything clever to add...

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH v2 1/3] usb: gadget: storage: Add fsg_get_toc helper

[Alan Stern]

On Mon, 21 Mar 2011, Roger Quadros wrote:

> From: Roger Quadros <roger.quadros@nokia.com>
> 
> fsg_get_toc can be used by both storage gadgets to build a CD-ROM
> Table of Contents which can be used as data for READ_TOC command.
> Currently fsg_get_toc supports two TOC formats, i.e.
> format 0: Formatted TOC and format 2: Raw TOC.
> 
> Raw TOC is required for CD-ROM emulation to work with Mac OS-X.
> 
> Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
> ---
>  drivers/usb/gadget/storage_common.c |   68 +++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
> index b015561..d47f148 100644
> --- a/drivers/usb/gadget/storage_common.c
> +++ b/drivers/usb/gadget/storage_common.c
> @@ -655,6 +655,74 @@ static void store_cdrom_address(u8 *dest, int msf, u32 addr)
>  	}
>  }
>  
> +/**
> + * fsg_get_toc() - Builds a TOC with required format @format.
> + * @curlun: The LUN for which the TOC has to be built
> + * @msf: Min Sec Frame format or LBA format for address
> + * @format: TOC format code
> + * @buf: The buffer into which the TOC is built
> + *
> + * Builds a Table of Content which can be used as data for READ_TOC command.
> + * The TOC simulates a single session, single track CD-ROM mode 1 disc.
> + *
> + * Returns number of bytes written to @buf, -EINVAL if format not supported.
> + */
> +static int fsg_get_toc(struct fsg_lun *curlun, int msf, int format, u8 *buf)
> +{
> +	int i, len;
> +	switch (format) {
> +	case 0:
> +		/* Formatted TOC */
> +		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
> +		memset(buf, 0, len);
> +		len -= 2;		/* TOC Length excludes length field */
> +		buf[1] = len;		/* TOC data length */

These two lines are wrong.  You should do:

		buf[1] = len - 2;	/* TOC data length excludes header */

The way it is now, the function ends up returning 18 instead of 20,
causing the wrong amount of data to be transmitted.

> +		buf[2] = 1;		/* First track number */
> +		buf[3] = 1;		/* Last track number */
> +		buf[5] = 0x16;		/* Data track, copying allowed */
> +		buf[6] = 0x01;		/* Only track is number 1 */
> +		store_cdrom_address(&buf[8], msf, 0);
> +
> +		buf[13] = 0x16;		/* Lead-out track is data */
> +		buf[14] = 0xAA;		/* Lead-out track number */
> +		store_cdrom_address(&buf[16], msf, curlun->num_sectors);
> +		return len;
> +		break;
> +
> +	case 2:
> +		/* Raw TOC */
> +		len = 4 + 3*11;		/* 4 byte header + 3 descriptors */
> +		memset(buf, 0, len);	/* Header + A0, A1 & A2 descriptors */
> +		len -= 2;		/* TOC Length excludes length field */
> +		buf[1] = len;		/* TOC data length */

Same thing here.  I'm surprised it worked during testing.  Did you
really check to see that the correct data was being sent?

Alan Stern

Re: [PATCH v2 1/3] usb: gadget: storage: Add fsg_get_toc helper

[Roger Quadros]

On 03/21/2011 04:20 PM, ext Alan Stern wrote:
> On Mon, 21 Mar 2011, Roger Quadros wrote:
>
>> From: Roger Quadros<roger.quadros@nokia.com>
>>
>> fsg_get_toc can be used by both storage gadgets to build a CD-ROM
>> Table of Contents which can be used as data for READ_TOC command.
>> Currently fsg_get_toc supports two TOC formats, i.e.
>> format 0: Formatted TOC and format 2: Raw TOC.
>>
>> Raw TOC is required for CD-ROM emulation to work with Mac OS-X.
>>
>> Signed-off-by: Roger Quadros<roger.quadros@nokia.com>
>> ---
>>   drivers/usb/gadget/storage_common.c |   68 +++++++++++++++++++++++++++++++++++
>>   1 files changed, 68 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
>> index b015561..d47f148 100644
>> --- a/drivers/usb/gadget/storage_common.c
>> +++ b/drivers/usb/gadget/storage_common.c
>> @@ -655,6 +655,74 @@ static void store_cdrom_address(u8 *dest, int msf, u32 addr)
>>   	}
>>   }
>>
>> +/**
>> + * fsg_get_toc() - Builds a TOC with required format @format.
>> + * @curlun: The LUN for which the TOC has to be built
>> + * @msf: Min Sec Frame format or LBA format for address
>> + * @format: TOC format code
>> + * @buf: The buffer into which the TOC is built
>> + *
>> + * Builds a Table of Content which can be used as data for READ_TOC command.
>> + * The TOC simulates a single session, single track CD-ROM mode 1 disc.
>> + *
>> + * Returns number of bytes written to @buf, -EINVAL if format not supported.
>> + */
>> +static int fsg_get_toc(struct fsg_lun *curlun, int msf, int format, u8 *buf)
>> +{
>> +	int i, len;
>> +	switch (format) {
>> +	case 0:
>> +		/* Formatted TOC */
>> +		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
>> +		memset(buf, 0, len);
>> +		len -= 2;		/* TOC Length excludes length field */
>> +		buf[1] = len;		/* TOC data length */
>
> These two lines are wrong.  You should do:
>
> 		buf[1] = len - 2;	/* TOC data length excludes header */
>
> The way it is now, the function ends up returning 18 instead of 20,
> causing the wrong amount of data to be transmitted.

Yes indeed.

>
>> +		buf[2] = 1;		/* First track number */
>> +		buf[3] = 1;		/* Last track number */
>> +		buf[5] = 0x16;		/* Data track, copying allowed */
>> +		buf[6] = 0x01;		/* Only track is number 1 */
>> +		store_cdrom_address(&buf[8], msf, 0);
>> +
>> +		buf[13] = 0x16;		/* Lead-out track is data */
>> +		buf[14] = 0xAA;		/* Lead-out track number */
>> +		store_cdrom_address(&buf[16], msf, curlun->num_sectors);
>> +		return len;
>> +		break;
>> +
>> +	case 2:
>> +		/* Raw TOC */
>> +		len = 4 + 3*11;		/* 4 byte header + 3 descriptors */
>> +		memset(buf, 0, len);	/* Header + A0, A1&  A2 descriptors */
>> +		len -= 2;		/* TOC Length excludes length field */
>> +		buf[1] = len;		/* TOC data length */
>
> Same thing here.  I'm surprised it worked during testing.  Did you
> really check to see that the correct data was being sent?

I too am surprised. I tried playing a few videos without problems.
After you pointed out the problem I ran a checksum on the files and they match!!

But thanks for pointing out, I'll send a new patch.

-- 
regards,
-roger