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

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

[Jack Pham]

Hi,

This is a resurrection of a *very* old patch [1] from 2011 that apparently
never landed upstream.

One of our customers is reporting exactly the failure described in $SUBJECT,
and mentioned that applying this change in their downstream kernel allows
their mass storage function to successfully connect to OS X hosts.  So
perhaps we can finally try to merge it, better late than never.

The original series was applicable to both the mass_storage and now-defunct
file_storage gadget functions.  So I've simply squashed the changes into a
single patch applicable to f_mass_storage.c

@Roger I've kept the authorship email from your original submission since
it was retained from "git am".  I'm not sure if it's ok/appropriate to update
it to your current @kernel.org address?

[1] https://lore.kernel.org/lkml/1302015569-9668-1-git-send-email-roger.quadros@nokia.com/T/#u

Roger Quadros (1):
  usb: gadget: f_mass_storage: Make CD-ROM emulation work with Mac OS-X

 drivers/usb/gadget/function/f_mass_storage.c | 73 +++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 12 deletions(-)

-- 
2.7.4

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

[Jack Pham]

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.

Change-Id: Ifdbe10e779bc8609a11eeab6ab6de549c6223fe2
Signed-off-by: Roger Quadros <roger.quadros@nokia.com>
Signed-off-by: Jack Pham <quic_jackp@quicinc.com>
---
 drivers/usb/gadget/function/f_mass_storage.c | 73 +++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 73a28f8..1f7f4dd6 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -1188,6 +1188,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		i, len;
 
 	if ((common->cmnd[1] & ~0x02) != 0 ||	/* Mask away MSF */
 			start_track > 1) {
@@ -1195,18 +1197,65 @@ 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;
+
+	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;
+
+	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);
 
-	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;
+		return len;
+
+	default:
+		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
+		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
+		return -EINVAL;
+	}
 }
 
 static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1933,7 +1982,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);
-- 
2.7.4

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

[Jack Pham]

On Sun, Jan 09, 2022 at 10:23:59PM -0800, Jack Pham wrote:
> 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.
> 
> Change-Id: Ifdbe10e779bc8609a11eeab6ab6de549c6223fe2

Argh, sorry about this! Will remove in V2.

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

[Jack Pham]

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>
Signed-off-by: Jack Pham <quic_jackp@quicinc.com>
---
v2: Removed Change-Id tag.

 drivers/usb/gadget/function/f_mass_storage.c | 73 +++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 73a28f8..1f7f4dd6 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -1188,6 +1188,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		i, len;
 
 	if ((common->cmnd[1] & ~0x02) != 0 ||	/* Mask away MSF */
 			start_track > 1) {
@@ -1195,18 +1197,65 @@ 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;
+
+	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;
+
+	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);
 
-	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;
+		return len;
+
+	default:
+		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
+		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
+		return -EINVAL;
+	}
 }
 
 static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
@@ -1933,7 +1982,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);
-- 
2.7.4

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

[Alan Stern]

On Sun, Jan 09, 2022 at 10:30:30PM -0800, Jack Pham wrote:
> 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>
> Signed-off-by: Jack Pham <quic_jackp@quicinc.com>
> ---
> v2: Removed Change-Id tag.
> 
>  drivers/usb/gadget/function/f_mass_storage.c | 73 +++++++++++++++++++++++-----
>  1 file changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 73a28f8..1f7f4dd6 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -1188,6 +1188,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		i, len;
>  
>  	if ((common->cmnd[1] & ~0x02) != 0 ||	/* Mask away MSF */
>  			start_track > 1) {
> @@ -1195,18 +1197,65 @@ 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;

Hmmm.  According to this part later on:

> @@ -1933,7 +1982,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");

common->cmnd[2] can never be anything other than 0.  So this computation 
and the test immediately below are pointless -- unless you change the 
argument to check_command().

Alan Stern

> +	/*
> +	 * 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;
> +
> +	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;
> +
> +	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);
>  
> -	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;
> +		return len;
> +
> +	default:
> +		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
> +		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
> +		return -EINVAL;
> +	}
>  }
>  
>  static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
> @@ -1933,7 +1982,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);
> -- 
> 2.7.4
> 

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

[Jack Pham]

Hi Alan,

On Wed, Jan 12, 2022 at 03:05:35PM -0500, Alan Stern wrote:
> On Sun, Jan 09, 2022 at 10:30:30PM -0800, Jack Pham wrote:
> > 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>
> > Signed-off-by: Jack Pham <quic_jackp@quicinc.com>
> > ---
> > v2: Removed Change-Id tag.
> > 
> >  drivers/usb/gadget/function/f_mass_storage.c | 73 +++++++++++++++++++++++-----
> >  1 file changed, 61 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> > index 73a28f8..1f7f4dd6 100644
> > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > @@ -1188,6 +1188,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		i, len;
> >  
> >  	if ((common->cmnd[1] & ~0x02) != 0 ||	/* Mask away MSF */
> >  			start_track > 1) {
> > @@ -1195,18 +1197,65 @@ 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;
> 
> Hmmm.  According to this part later on:
> 
> > @@ -1933,7 +1982,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");
> 
> common->cmnd[2] can never be anything other than 0.

Ah, that is true.  So to allow for cmnd[2] (as well as cmnd[9] as
intended by the patch) to be non-zero, the "mask" argument should rather
be:

 (0xf<<6) | (3<<1)

In other words a bitmask of 0x3c6, corresponding to command data bytes
1, 2, 6, 7, 8 and 9.  Is my understanding correct?

> So this computation and the test immediately below are pointless --
> unless you change the argument to check_command().

If you are referring to the "if (format == 0)" check, then I believe you
are right.

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

It seems this is the gist of the patch.  Without changing the mask
parameter to check_command() above, we know we can only reach here if
format = common->cmnd[2] is 0.  However this snippet is then reassigning
format from the upper bits of the 9th byte which could be non-zero, at
least in the case of MacOS.

So this patch does seem a bit incomplete as you point out and maybe
updating the mask as above should help to allow both fields to determine
the format for any non-zero TOC type.

But I was trying to confirm these details from the SFF-8020i spec as
mentioned in the original comment above.  I was only able to find a
draft copy [1] from a web search which stated:

	"Format field definition: When Format in Byte 2 is zero, then
	Byte 9 is used. Other values for this field are reserved for
	definition in MMC."

	Note: The Format field in Byte 9 is a vendor-specific area and
	will be removed in subsequent versions of this specification.
	Functionality is moving to Byte 2."

However when trying to look up the latest official release of SFF-8020
the SNIA website [2] lists it as having been expired and incorporated
into the SCSI MMC specification.  Consequently, I haven't yet been able
to look further into the SCSI MMC spec itself as it seems it is only
available for a fee from the ANSI/INCITS website [3].  I'm hoping you or
maybe somebody on this list might have more knowledge on these details.

So I'm left wondering whether the Format field in Byte 9 is even
standardized, or if it is a remnant of an older or possibly
draft/non-final specification.  Yet we clearly have a host that is
relying on this behavior, so there is utility in this patch.

FWIW, here are the raw bytes of the READ TOC request transaction as
issued by the MacOS host, obtained from a bus analyzer trace, which this
patch is purportedly fixing:

 55 53 42 43 19 00 00 00 FE FF 00 00 80 00 0A 43
                                              ^^
                                              cmnd[0] i.e. OpCode
 02 00 00 00 00 00 FF FE 80 00 00 00 00 00 00
    ^^                   ^^
    cmnd[2]==0           ||
                         cmnd[9], upper 2 bits == 0x2

Thanks,
Jack

[1] https://www.bswd.com/sff8020i.pdf
[2] https://www.snia.org/technology-communities/sff/specifications
[3] https://webstore.ansi.org/standards/incits/ansiincits4302007

> > +
> > +	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;
> > +
> > +	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);
> >  
> > -	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;
> > +		return len;
> > +
> > +	default:
> > +		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
> > +		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
> > +		return -EINVAL;
> > +	}
> >  }
> >  
> >  static int do_mode_sense(struct fsg_common *common, struct fsg_buffhd *bh)
> > @@ -1933,7 +1982,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);
> > -- 
> > 2.7.4
> > 

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

[Alan Stern]

On Wed, Jan 12, 2022 at 09:22:53PM -0800, Jack Pham wrote:
> Hi Alan,
> 
> On Wed, Jan 12, 2022 at 03:05:35PM -0500, Alan Stern wrote:
> > On Sun, Jan 09, 2022 at 10:30:30PM -0800, Jack Pham wrote:
> > > 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>
> > > Signed-off-by: Jack Pham <quic_jackp@quicinc.com>
> > > ---
> > > v2: Removed Change-Id tag.
> > > 
> > >  drivers/usb/gadget/function/f_mass_storage.c | 73 +++++++++++++++++++++++-----
> > >  1 file changed, 61 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> > > index 73a28f8..1f7f4dd6 100644
> > > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > > @@ -1188,6 +1188,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		i, len;
> > >  
> > >  	if ((common->cmnd[1] & ~0x02) != 0 ||	/* Mask away MSF */
> > >  			start_track > 1) {
> > > @@ -1195,18 +1197,65 @@ 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;
> > 
> > Hmmm.  According to this part later on:
> > 
> > > @@ -1933,7 +1982,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");
> > 
> > common->cmnd[2] can never be anything other than 0.
> 
> Ah, that is true.  So to allow for cmnd[2] (as well as cmnd[9] as
> intended by the patch) to be non-zero, the "mask" argument should rather
> be:
> 
>  (0xf<<6) | (3<<1)

Indeed.

> In other words a bitmask of 0x3c6, corresponding to command data bytes
> 1, 2, 6, 7, 8 and 9.  Is my understanding correct?

Yes, that's right.

> > So this computation and the test immediately below are pointless --
> > unless you change the argument to check_command().
> 
> If you are referring to the "if (format == 0)" check, then I believe you
> are right.
> 
> > > +	/*
> > > +	 * 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;
> 
> It seems this is the gist of the patch.  Without changing the mask
> parameter to check_command() above, we know we can only reach here if
> format = common->cmnd[2] is 0.  However this snippet is then reassigning
> format from the upper bits of the 9th byte which could be non-zero, at
> least in the case of MacOS.
> 
> So this patch does seem a bit incomplete as you point out and maybe
> updating the mask as above should help to allow both fields to determine
> the format for any non-zero TOC type.
> 
> But I was trying to confirm these details from the SFF-8020i spec as
> mentioned in the original comment above.  I was only able to find a
> draft copy [1] from a web search which stated:
> 
> 	"Format field definition: When Format in Byte 2 is zero, then
> 	Byte 9 is used. Other values for this field are reserved for
> 	definition in MMC."
> 
> 	Note: The Format field in Byte 9 is a vendor-specific area and
> 	will be removed in subsequent versions of this specification.
> 	Functionality is moving to Byte 2."
> 
> However when trying to look up the latest official release of SFF-8020
> the SNIA website [2] lists it as having been expired and incorporated
> into the SCSI MMC specification.  Consequently, I haven't yet been able
> to look further into the SCSI MMC spec itself as it seems it is only
> available for a fee from the ANSI/INCITS website [3].  I'm hoping you or
> maybe somebody on this list might have more knowledge on these details.

I thought I had a copy of the old SFF8020i spec somewhere, but now I 
can't find it.  :-(

> So I'm left wondering whether the Format field in Byte 9 is even
> standardized, or if it is a remnant of an older or possibly
> draft/non-final specification.  Yet we clearly have a host that is
> relying on this behavior, so there is utility in this patch.
> 
> FWIW, here are the raw bytes of the READ TOC request transaction as
> issued by the MacOS host, obtained from a bus analyzer trace, which this
> patch is purportedly fixing:
> 
>  55 53 42 43 19 00 00 00 FE FF 00 00 80 00 0A 43
>                                               ^^
>                                               cmnd[0] i.e. OpCode
>  02 00 00 00 00 00 FF FE 80 00 00 00 00 00 00
>     ^^                   ^^
>     cmnd[2]==0           ||
>                          cmnd[9], upper 2 bits == 0x2

Well, there are really just two things we need to worry about:

	The driver must continue to work properly on all the systems
	that are using it now.

	The driver should be able to work with Mac OS-X.

I think if you simply change the mask value then we'll be okay.

Alan Stern

> Thanks,
> Jack
> 
> [1] https://www.bswd.com/sff8020i.pdf
> [2] https://www.snia.org/technology-communities/sff/specifications
> [3] https://webstore.ansi.org/standards/incits/ansiincits4302007