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

[Jack Pham <quic_jackp@quicinc.com>]

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