Re: [PATCH] usb gadget: remove size limitation for storage cdrom

[Alan Stern]

On Mon, 9 Mar 2015, Dave Young wrote:

> On 03/08/15 at 11:29am, Alan Stern wrote:
> > On Sun, 8 Mar 2015, Dave Young wrote:
> > 
> > > I used usb cdrom emulation to play video dvd for my daughter, but I got below
> > > error:
> > > 
> > > [dave@darkstar tmp]$ cat /mnt/sr1/VIDEO_TS/VTS_01_5.VOB >/dev/null
> > > cat: /mnt/sr1/VIDEO_TS/VTS_01_5.VOB: Input/output error
> > > [dave@darkstar tmp]$ dmesg|tail -1
> > > [ 3349.371857] sr1: rw=0, want=8028824, limit=4607996
> > > 
> > > The assumption of cdrom size is not right, we can create data dvd large then
> > > 4G, also mkisofs can create an iso with only one blank directory, the size is
> > > less than 300 sectors. The size limit does not make sense, thus remove them. 
> > > 
> > > Signed-off-by: Dave Young <dyoung@xxxxxxxxxx>
> > > ---
> > >  drivers/usb/gadget/function/storage_common.c |    9 ---------
> > >  1 file changed, 9 deletions(-)
> > > 
> > > --- linux.orig/drivers/usb/gadget/function/storage_common.c
> > > +++ linux/drivers/usb/gadget/function/storage_common.c
> > > @@ -247,15 +247,6 @@ int fsg_lun_open(struct fsg_lun *curlun,
> > >  
> > >  	num_sectors = size >> blkbits; /* File size in logic-block-size blocks */
> > >  	min_sectors = 1;
> > > -	if (curlun->cdrom) {
> > > -		min_sectors = 300;	/* Smallest track is 300 frames */
> > > -		if (num_sectors >= 256*60*75) {
> > > -			num_sectors = 256*60*75 - 1;
> > > -			LINFO(curlun, "file too big: %s\n", filename);
> > > -			LINFO(curlun, "using only first %d blocks\n",
> > > -					(int) num_sectors);
> > > -		}
> > > -	}
> > >  	if (num_sectors < min_sectors) {
> > >  		LINFO(curlun, "file too small: %s\n", filename);
> > >  		rc = -ETOOSMALL;
> > 
> > NAK.  This patch is wrong, for a very simple reason.  You wrote:
> > 
> > > I used usb cdrom emulation to play video dvd for my daughter
> > 
> > and this demonstrates the error quite plainly.  You can't use _CD_ 
> > emulation to imitate a _DVD_ -- they are different sorts of media.  
> > Just think of what happens when you try playing a DVD on a CD player.
> > 
> > A more suitable approach would be to add DVD emulation to the driver.
> > 
> 
> They are both iso9660 images, aren't they? So from data image point
> of view there's no difference, it is not worth to create another mode
> for dvd data.

There's more to emulation than just the image.  We have to emulate the 
hardware's response to all the applicable commands.  CD players have a 
different command set from DVD players (or at least, different from DVD 
players when a DVD is inserted).  For example, see the definition in 
the MSF format for READ TOC command.

> We should not emulate cd drive to support different cd media type,
> it is far more better to support just iso9660 volume no matter how
> large the size is as long as it is fit for iso9660 spec.
> 
> OTOH, the size limitation is a bug:
> * isofs can be less than 300 sectors, the 300 sectors limitation is for
> music cd I think. Try mkisofs a blank directory and burn it.

You're thinking about this the wrong way.  We aren't emulating an 
iso9660 image; we are emulating a CD player.  (In principle we could 
even emulate an audio CD, but the driver doesn't support that.)

> * There's 99 minutes dics even for cdrom, see:
> http://en.wikipedia.org/wiki/CD-R 
> If we code to support different size discs, it will looks like a wrong way.

Look at the code you want to remove:

> > > -		if (num_sectors >= 256*60*75) {

That's 75 sectors/second * 60 seconds/minute * 256 minutes.  So the
driver already supports up to 256 minutes, which is way beyond the 99
minutes required by the spec.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/