usb: gadget: storage: Make CD-ROM emulation work with Mac OS-X
v2 cover usb: gadget: storage: Make CD-ROM emulation work with Mac OS-X
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
From: Roger Quadros <[email protected]>
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.
--- drivers/usb/gadget/storage_common.c | 68 +++++++++++++++++++++++++++++++++++ 1 files changed, 68 insertions(+), 0 deletions(-)
drivers/usb/gadget/storage_common.c +68 -0
@@ -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 ; + } +} +/*-------------------------------------------------------------------------*/
Signature · 1 line
1.6.0.4
PATCH v2 2/3 usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X
From: Roger Quadros <[email protected]>
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.
--- drivers/usb/gadget/file_storage.c | 31 ++++++++++++++++++------------- 1 files changed, 18 insertions(+), 13 deletions(-)
drivers/usb/gadget/file_storage.c +17 -12
@@ -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 ;
Signature · 1 line
1.6.0.4
PATCH v2 3/3 usb: gadget: f_mass_storage: Make CD-ROM emulation work with Mac OS-X
From: Roger Quadros <[email protected]>
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.
--- drivers/usb/gadget/f_mass_storage.c | 31 ++++++++++++++++++------------- 1 files changed, 18 insertions(+), 13 deletions(-)
drivers/usb/gadget/f_mass_storage.c +17 -12
@@ -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);
Signature · 1 line
1.6.0.4
Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X
In reply to Roger QuadrosHi,
Quoted 21 lines · ext Roger Quadros
drivers/usb/gadget/file_storage.c +8
-8@@ -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 */
drivers/usb/gadget/file_storage.c static int do_read_toc(struct fsg_dev *fsg, struct fsg_buffhd *bh) · line 1723
Quoted 10 lines
drivers/usb/gadget/file_storage.c +6
-3- 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;Signature · 2 lines
regards, -roger
Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X
Quoted 7 lines · Roger Quadros
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 returnfsg->data_size_from_cmndinstead of actual TOC length?
No. Always return the actual TOC length.
Quoted 6 lines
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?
Quoted 10 lines
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
Quoted 11 lines · ext Alan Stern
Quoted 7 lines · Roger Quadros
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 returnfsg->data_size_from_cmndinstead of actual TOC length?No. Always return the actual TOC length.
OK.
Quoted 18 lines
Quoted 7 lines
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?
Quoted 10 lines
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;
Quoted 7 lines
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().
Signature · 2 lines
regards, -roger
Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X
Quoted 6 lines · Roger Quadros
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->residueto actual TOC length. indo_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(-)
drivers/usb/gadget/f_mass_storage.c +11 -43
@@ -1584,37 +1584,6 @@ static int wedge_bulk_in_endpoint(structreturn 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_commoif (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 ;
drivers/usb/gadget/file_storage.c +12 -41
67 lines not on this page · raw message
Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X
Quoted 21 lines · ext Alan Stern
Quoted 6 lines · Roger Quadros
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->residueto actual TOC length. indo_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.
Quoted 2 lines
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".
Quoted 2 lines
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.
Quoted 6 lines
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 :).
Quoted 15 lines
static int throw_away_data(struct fsg_common *common) { struct fsg_buffhd *bh;drivers/usb/gadget/f_mass_storage.c +4
-0@@ -1702,6 +1671,10 @@ static int finish_reply(struct fsg_commoif (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 ;
drivers/usb/gadget/f_mass_storage.c static int finish_reply(struct fsg_commo · line 1689
Quoted 10 lines
drivers/usb/gadget/f_mass_storage.c +4
-3@@ -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?
Quoted 2 lines
drivers/usb/gadget/f_mass_storage.c +1
-0+* don't halt the endpoint. Presumably nobody will mind.) */
Signature · 2 lines
regards, -roger
Re: [PATCH v2 2/3] usb: gadget: file_storage: Make CD-ROM emulation work with Mac OS-X
Quoted 11 lines · Roger Quadros
Quoted 5 lines
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
Quoted 2 lines
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.
Quoted 13 lines
drivers/usb/gadget/f_mass_storage.c static int finish_reply(struct fsg_commo · line 1689
Quoted 11 lines
drivers/usb/gadget/f_mass_storage.c +4
-3@@ -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.
Quoted 2 lines
drivers/usb/gadget/f_mass_storage.c static int finish_reply(struct fsg_commo · line 1691
Quoted 2 lines
drivers/usb/gadget/f_mass_storage.c +1
-0+* 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
Quoted 9 lines
Quoted 2 lines
Quoted 2 lines
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.
Quoted 5 lines · Roger Quadros
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?
Quoted 8 lines · Alan Stern
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:
drivers/usb/gadget/f_mass_storage.c static int finish_reply(struct fsg_commo · line 1677
Quoted 4 lines
drivers/usb/gadget/f_mass_storage.c +4
-0+/* 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...
Signature · 4 lines
Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michal "mina86" Nazarewicz (o o) ooo +-----<email/xmpp: [email protected]>-----ooO--(_)--Ooo--
Re: [PATCH v2 1/3] usb: gadget: storage: Add fsg_get_toc helper
In reply to Roger QuadrosQuoted 34 lines · Roger Quadros
From: Roger Quadros <[email protected]>
fsg_get_toccan be used by both storage gadgets to build a CD-ROM Table of Contents which can be used as data forREAD_TOCcommand. Currentlyfsg_get_tocsupports 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 <[email protected]>--- drivers/usb/gadget/storage_common.c | 68 +++++++++++++++++++++++++++++++++++ 1 files changed, 68 insertions(+), 0 deletions(-)drivers/usb/gadget/storage_common.c +12
-0@@ -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. +*/
drivers/usb/gadget/storage_common.c static void store_cdrom_address(u8 *dest, int msf, u32 addr) · line 679
Quoted 10 lines
drivers/usb/gadget/storage_common.c +10
-0+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.
Quoted 8 lines
drivers/usb/gadget/storage_common.c +8
-0+ 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 */
drivers/usb/gadget/storage_common.c static void store_cdrom_address(u8 *dest, int msf, u32 addr) · line 697
Quoted 10 lines
drivers/usb/gadget/storage_common.c +10
-0+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
Quoted 49 lines · ext Alan Stern
Quoted 34 lines · Roger Quadros
From: Roger Quadros<[email protected]>
fsg_get_toccan be used by both storage gadgets to build a CD-ROM Table of Contents which can be used as data forREAD_TOCcommand. Currentlyfsg_get_tocsupports 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<[email protected]>--- drivers/usb/gadget/storage_common.c | 68 +++++++++++++++++++++++++++++++++++ 1 files changed, 68 insertions(+), 0 deletions(-)drivers/usb/gadget/storage_common.c +12
-0@@ -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. +*/ drivers/usb/gadget/storage_common.c static void store_cdrom_address(u8 *dest, int msf, u32 addr) · line 679
Quoted 10 lines
drivers/usb/gadget/storage_common.c +10
-0+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:
Quoted 4 lines
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.
Quoted 14 lines
Quoted 14 lines
drivers/usb/gadget/storage_common.c +13
-0+ 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 :
Quoted 8 lines
drivers/usb/gadget/storage_common.c static void store_cdrom_address(u8 *dest, int msf, u32 addr) · line 697
Quoted 5 lines
drivers/usb/gadget/storage_common.c +5
-0+/* 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.
Signature · 2 lines
regards, -roger