[PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[Neal Liu]

Add read TOC with format 1 to support CD-ROM emulation with
Windows OS.
This patch is tested on Windows OS Server 2019.

Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
---
 drivers/usb/gadget/function/f_mass_storage.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
index 3a77bca0ebe1..9edf76c22605 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -1209,7 +1209,8 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
 
 	switch (format) {
 	case 0:
-		/* Formatted TOC */
+	case 1:
+		/* Formatted TOC, Session info */
 		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
 		memset(buf, 0, len);
 		buf[1] = len - 2;	/* TOC Length excludes length field */
@@ -1250,7 +1251,7 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
 		return len;
 
 	default:
-		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
+		/* PMA, ATIP, CD-TEXT not supported/required */
 		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
 		return -EINVAL;
 	}
-- 
2.25.1

Re: [PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[Greg Kroah-Hartman]

On Fri, Jun 24, 2022 at 10:19:16AM +0800, Neal Liu wrote:
> Add read TOC with format 1 to support CD-ROM emulation with

What is "TOC"?  What is "format 1"?

> Windows OS.

Which versions of Windows support this?

> This patch is tested on Windows OS Server 2019.

Does this fix a regression where we used to support this?  Or is this a
new feature of Windows?


> 
> Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> ---
>  drivers/usb/gadget/function/f_mass_storage.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 3a77bca0ebe1..9edf76c22605 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -1209,7 +1209,8 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
>  
>  	switch (format) {
>  	case 0:
> -		/* Formatted TOC */
> +	case 1:
> +		/* Formatted TOC, Session info */
>  		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
>  		memset(buf, 0, len);
>  		buf[1] = len - 2;	/* TOC Length excludes length field */
> @@ -1250,7 +1251,7 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
>  		return len;
>  
>  	default:
> -		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
> +		/* PMA, ATIP, CD-TEXT not supported/required */

So case 1 is "multi-session"?  Please document this properly.

thanks,

greg k-h

RE: [PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[Neal Liu]

> On Fri, Jun 24, 2022 at 10:19:16AM +0800, Neal Liu wrote:
> > Add read TOC with format 1 to support CD-ROM emulation with
> 
> What is "TOC"?  What is "format 1"?
> 
> > Windows OS.
> 
> Which versions of Windows support this?
> 
> > This patch is tested on Windows OS Server 2019.
> 
> Does this fix a regression where we used to support this?  Or is this a new
> feature of Windows?

We used to support this. I should add "Fixes" for the information.

> 
> 
> >
> > Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> > ---
> >  drivers/usb/gadget/function/f_mass_storage.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c
> > b/drivers/usb/gadget/function/f_mass_storage.c
> > index 3a77bca0ebe1..9edf76c22605 100644
> > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > @@ -1209,7 +1209,8 @@ static int do_read_toc(struct fsg_common
> > *common, struct fsg_buffhd *bh)
> >
> >  	switch (format) {
> >  	case 0:
> > -		/* Formatted TOC */
> > +	case 1:
> > +		/* Formatted TOC, Session info */
> >  		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
> >  		memset(buf, 0, len);
> >  		buf[1] = len - 2;	/* TOC Length excludes length field */
> > @@ -1250,7 +1251,7 @@ static int do_read_toc(struct fsg_common
> *common, struct fsg_buffhd *bh)
> >  		return len;
> >
> >  	default:
> > -		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
> > +		/* PMA, ATIP, CD-TEXT not supported/required */
> 
> So case 1 is "multi-session"?  Please document this properly.
> 

Okay, I'll revise it properly.

Re: [PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[Greg Kroah-Hartman]

On Fri, Jun 24, 2022 at 10:19:16AM +0800, Neal Liu wrote:
> Add read TOC with format 1 to support CD-ROM emulation with
> Windows OS.
> This patch is tested on Windows OS Server 2019.
> 
> Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> ---
>  drivers/usb/gadget/function/f_mass_storage.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 3a77bca0ebe1..9edf76c22605 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -1209,7 +1209,8 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
>  
>  	switch (format) {
>  	case 0:
> -		/* Formatted TOC */
> +	case 1:
> +		/* Formatted TOC, Session info */
>  		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
>  		memset(buf, 0, len);
>  		buf[1] = len - 2;	/* TOC Length excludes length field */
> @@ -1250,7 +1251,7 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
>  		return len;
>  
>  	default:
> -		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
> +		/* PMA, ATIP, CD-TEXT not supported/required */
>  		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
>  		return -EINVAL;
>  	}
> -- 
> 2.25.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[Alan Stern]

On Fri, Jun 24, 2022 at 10:19:16AM +0800, Neal Liu wrote:
> Add read TOC with format 1 to support CD-ROM emulation with
> Windows OS.
> This patch is tested on Windows OS Server 2019.
> 
> Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> ---
>  drivers/usb/gadget/function/f_mass_storage.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 3a77bca0ebe1..9edf76c22605 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -1209,7 +1209,8 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
>  
>  	switch (format) {
>  	case 0:
> -		/* Formatted TOC */
> +	case 1:
> +		/* Formatted TOC, Session info */

This comment is a little misleading.  0 is formatted TOC, and 1 is 
multi-session info.  The way you wrote it, it looks like the comment 
applies to both formats.  Do it like this:

	case 0:		/* Formatted TOC */
	case 1:		/* Multi-session info */

>  		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
>  		memset(buf, 0, len);
>  		buf[1] = len - 2;	/* TOC Length excludes length field */

It looks like you ignored one of the problems I pointed out in my 
earlier email:

> When format is 1, the driver is supposed to ignore the start_track
> value.  Your patch does not do this.

Please fix this.

Alan Stern

> @@ -1250,7 +1251,7 @@ static int do_read_toc(struct fsg_common *common, struct fsg_buffhd *bh)
>  		return len;
>  
>  	default:
> -		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
> +		/* PMA, ATIP, CD-TEXT not supported/required */
>  		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
>  		return -EINVAL;
>  	}
> -- 
> 2.25.1
> 

RE: [PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[Neal Liu]

> On Fri, Jun 24, 2022 at 10:19:16AM +0800, Neal Liu wrote:
> > Add read TOC with format 1 to support CD-ROM emulation with Windows
> > OS.
> > This patch is tested on Windows OS Server 2019.
> >
> > Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> > ---
> >  drivers/usb/gadget/function/f_mass_storage.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/gadget/function/f_mass_storage.c
> > b/drivers/usb/gadget/function/f_mass_storage.c
> > index 3a77bca0ebe1..9edf76c22605 100644
> > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > @@ -1209,7 +1209,8 @@ static int do_read_toc(struct fsg_common
> > *common, struct fsg_buffhd *bh)
> >
> >  	switch (format) {
> >  	case 0:
> > -		/* Formatted TOC */
> > +	case 1:
> > +		/* Formatted TOC, Session info */
> 
> This comment is a little misleading.  0 is formatted TOC, and 1 is
> multi-session info.  The way you wrote it, it looks like the comment applies to
> both formats.  Do it like this:
> 
> 	case 0:		/* Formatted TOC */
> 	case 1:		/* Multi-session info */
> 

Okay, looks more clear. I'll fix it in next patch.

> >  		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
> >  		memset(buf, 0, len);
> >  		buf[1] = len - 2;	/* TOC Length excludes length field */
> 
> It looks like you ignored one of the problems I pointed out in my earlier email:
> 
> > When format is 1, the driver is supposed to ignore the start_track
> > value.  Your patch does not do this.
> 
> Please fix this.
> 

Do you mean ignore the "start_track > 1" above?
Do this check only for format 0? Or format 0 & 2?

> 
> > @@ -1250,7 +1251,7 @@ static int do_read_toc(struct fsg_common
> *common, struct fsg_buffhd *bh)
> >  		return len;
> >
> >  	default:
> > -		/* Multi-session, PMA, ATIP, CD-TEXT not supported/required */
> > +		/* PMA, ATIP, CD-TEXT not supported/required */
> >  		curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
> >  		return -EINVAL;
> >  	}
> > --
> > 2.25.1
> >

Re: [PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[Alan Stern]

On Mon, Jun 27, 2022 at 02:56:40AM +0000, Neal Liu wrote:
> > On Fri, Jun 24, 2022 at 10:19:16AM +0800, Neal Liu wrote:
> > > Add read TOC with format 1 to support CD-ROM emulation with Windows
> > > OS.
> > > This patch is tested on Windows OS Server 2019.
> > >
> > > Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> > > ---
> > >  drivers/usb/gadget/function/f_mass_storage.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c
> > > b/drivers/usb/gadget/function/f_mass_storage.c
> > > index 3a77bca0ebe1..9edf76c22605 100644
> > > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > > @@ -1209,7 +1209,8 @@ static int do_read_toc(struct fsg_common
> > > *common, struct fsg_buffhd *bh)
> > >
> > >  	switch (format) {
> > >  	case 0:
> > > -		/* Formatted TOC */
> > > +	case 1:
> > > +		/* Formatted TOC, Session info */
> > 
> > This comment is a little misleading.  0 is formatted TOC, and 1 is
> > multi-session info.  The way you wrote it, it looks like the comment applies to
> > both formats.  Do it like this:
> > 
> > 	case 0:		/* Formatted TOC */
> > 	case 1:		/* Multi-session info */
> > 
> 
> Okay, looks more clear. I'll fix it in next patch.
> 
> > >  		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
> > >  		memset(buf, 0, len);
> > >  		buf[1] = len - 2;	/* TOC Length excludes length field */
> > 
> > It looks like you ignored one of the problems I pointed out in my earlier email:
> > 
> > > When format is 1, the driver is supposed to ignore the start_track
> > > value.  Your patch does not do this.
> > 
> > Please fix this.
> > 
> 
> Do you mean ignore the "start_track > 1" above?

Yes, that's what I mean.

> Do this check only for format 0? Or format 0 & 2?

According to the MMC spec, the Track/Session Number is ignored for 
format 1.  For format 0 it is valid as a Track Number, and for format 2 
it is valid as a Session Number.  Therefore it should be checked for 
formats 0 and 2 but not for format 1.

Alan Stern

RE: [PATCH v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS

[Neal Liu]

> On Mon, Jun 27, 2022 at 02:56:40AM +0000, Neal Liu wrote:
> > > On Fri, Jun 24, 2022 at 10:19:16AM +0800, Neal Liu wrote:
> > > > Add read TOC with format 1 to support CD-ROM emulation with
> > > > Windows OS.
> > > > This patch is tested on Windows OS Server 2019.
> > > >
> > > > Signed-off-by: Neal Liu <neal_liu@aspeedtech.com>
> > > > ---
> > > >  drivers/usb/gadget/function/f_mass_storage.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c
> > > > b/drivers/usb/gadget/function/f_mass_storage.c
> > > > index 3a77bca0ebe1..9edf76c22605 100644
> > > > --- a/drivers/usb/gadget/function/f_mass_storage.c
> > > > +++ b/drivers/usb/gadget/function/f_mass_storage.c
> > > > @@ -1209,7 +1209,8 @@ static int do_read_toc(struct fsg_common
> > > > *common, struct fsg_buffhd *bh)
> > > >
> > > >  	switch (format) {
> > > >  	case 0:
> > > > -		/* Formatted TOC */
> > > > +	case 1:
> > > > +		/* Formatted TOC, Session info */
> > >
> > > This comment is a little misleading.  0 is formatted TOC, and 1 is
> > > multi-session info.  The way you wrote it, it looks like the comment
> > > applies to both formats.  Do it like this:
> > >
> > > 	case 0:		/* Formatted TOC */
> > > 	case 1:		/* Multi-session info */
> > >
> >
> > Okay, looks more clear. I'll fix it in next patch.
> >
> > > >  		len = 4 + 2*8;		/* 4 byte header + 2 descriptors */
> > > >  		memset(buf, 0, len);
> > > >  		buf[1] = len - 2;	/* TOC Length excludes length field */
> > >
> > > It looks like you ignored one of the problems I pointed out in my earlier
> email:
> > >
> > > > When format is 1, the driver is supposed to ignore the start_track
> > > > value.  Your patch does not do this.
> > >
> > > Please fix this.
> > >
> >
> > Do you mean ignore the "start_track > 1" above?
> 
> Yes, that's what I mean.
> 
> > Do this check only for format 0? Or format 0 & 2?
> 
> According to the MMC spec, the Track/Session Number is ignored for format 1.
> For format 0 it is valid as a Track Number, and for format 2 it is valid as a
> Session Number.  Therefore it should be checked for formats 0 and 2 but not
> for format 1.
> 
> Alan Stern

Got it. I'll fix this in next patch, thanks for pointing out.