| Message ID | 20220624021916.318067-1-neal_liu@aspeedtech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: gadget: f_mass_storage: Make CD-ROM emulation works with Windows OS | expand |
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
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
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 >
> 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.
> 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 > >
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
> 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.
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; }
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(-)