* [PATCH] usb: gadget: uvc: add bInterval checking for HS mode @ 2021-03-04 10:45 Pawel Laszczak [not found] ` <CAL411-p4C4F9K--U=C+Yhu7Uy6GxOxwCHpXtdrXYMObLegFm9Q@mail.gmail.com> 0 siblings, 1 reply; 4+ messages in thread From: Pawel Laszczak @ 2021-03-04 10:45 UTC (permalink / raw) To: balbi, gregkh, linux-usb, linux-kernel; +Cc: laurent.pinchart, Pawel Laszczak From: Pawel Laszczak <pawell@cadence.com> Patch adds extra checking for bInterval passed by configfs. The 5.6.4 chapter of USB Specification (rev. 2.0) say: "A high-bandwidth endpoint must specify a period of 1x125 µs (i.e., a bInterval value of 1)." The issue was observed during testing UVC class on CV. I treat this change as improvement because we can control bInterval by configfs. Signed-off-by: Pawel Laszczak <pawell@cadence.com> --- drivers/usb/gadget/function/f_uvc.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 44b4352a2676..5d62720bb9e1 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -631,6 +631,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) cpu_to_le16(min(opts->streaming_maxpacket, 1023U)); uvc_fs_streaming_ep.bInterval = opts->streaming_interval; + /* A high-bandwidth endpoint must specify a bInterval value of 1 */ + if (max_packet_mult > 1) + uvc_hs_streaming_ep.bInterval = 1; + else + uvc_hs_streaming_ep.bInterval = opts->streaming_interval; + uvc_hs_streaming_ep.wMaxPacketSize = cpu_to_le16(max_packet_size | ((max_packet_mult - 1) << 11)); uvc_hs_streaming_ep.bInterval = opts->streaming_interval; -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
[parent not found: <CAL411-p4C4F9K--U=C+Yhu7Uy6GxOxwCHpXtdrXYMObLegFm9Q@mail.gmail.com>]
* RE: [PATCH] usb: gadget: uvc: add bInterval checking for HS mode [not found] ` <CAL411-p4C4F9K--U=C+Yhu7Uy6GxOxwCHpXtdrXYMObLegFm9Q@mail.gmail.com> @ 2021-03-08 8:28 ` Pawel Laszczak 2021-03-08 12:59 ` Pawel Laszczak 2021-03-10 5:45 ` Pawel Laszczak 0 siblings, 2 replies; 4+ messages in thread From: Pawel Laszczak @ 2021-03-08 8:28 UTC (permalink / raw) To: Peter Chen; +Cc: balbi, Greg Kroah-Hartman, USB list, lkml, laurent.pinchart > >On Fri, Mar 5, 2021 at 12:40 AM Pawel Laszczak <mailto:pawell@cadence.com> wrote: >From: Pawel Laszczak <mailto:pawell@cadence.com> > >Patch adds extra checking for bInterval passed by configfs. >The 5.6.4 chapter of USB Specification (rev. 2.0) say: >"A high-bandwidth endpoint must specify a period of 1x125 µs >(i.e., a bInterval value of 1)." > >The issue was observed during testing UVC class on CV. >I treat this change as improvement because we can control >bInterval by configfs. > >Signed-off-by: Pawel Laszczak <mailto:pawell@cadence.com> >--- > drivers/usb/gadget/function/f_uvc.c | 6 ++++++ > 1 file changed, 6 insertions(+) > >diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c >index 44b4352a2676..5d62720bb9e1 100644 >--- a/drivers/usb/gadget/function/f_uvc.c >+++ b/drivers/usb/gadget/function/f_uvc.c >@@ -631,6 +631,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) > cpu_to_le16(min(opts->streaming_maxpacket, 1023U)); > uvc_fs_streaming_ep.bInterval = opts->streaming_interval; > >+ /* A high-bandwidth endpoint must specify a bInterval value of 1 */ >+ if (max_packet_mult > 1) >+ uvc_hs_streaming_ep.bInterval = 1; >+ else >+ uvc_hs_streaming_ep.bInterval = opts->streaming_interval; >+ > >There is a "uvc_hs_streaming_ep.bInterval = opts->streaming_interval;" again at below code >Besides, the default value is 1 for opts->streaming_interval. What the real issue you observed >at CV test? > The issue occurs when I intentionally set: echo 3072 > functions/$FUNCTION/streaming_maxpacket echo 4 > functions/$FUNCTION/streaming_interval Then for CV CH9 TD 9.5: Endpoint Descriptor test it got: "(Mult = 2)Illegal high speed isochronous endpoint MaxPacketSize : 0x400 (USB: 1.2.78) A High speed Interrupt/Isochronous endpoint must have a MaxPacketSize between 683 and 1024 and bInterval value of 1 when the Mult value is two." For default value CV passed. Of course, I can fix it by changing streaming_interval, but I thought that it could be good to have protection against this issue. Especially since Usb 2 specification say that bInterval must be 1 for high bandwidth endpoints. Pawel >Peter > > uvc_hs_streaming_ep.wMaxPacketSize = > cpu_to_le16(max_packet_size | ((max_packet_mult - 1) << 11)); > uvc_hs_streaming_ep.bInterval = opts->streaming_interval; >-- >2.25.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] usb: gadget: uvc: add bInterval checking for HS mode 2021-03-08 8:28 ` Pawel Laszczak @ 2021-03-08 12:59 ` Pawel Laszczak 2021-03-10 5:45 ` Pawel Laszczak 1 sibling, 0 replies; 4+ messages in thread From: Pawel Laszczak @ 2021-03-08 12:59 UTC (permalink / raw) To: Peter Chen; +Cc: balbi, Greg Kroah-Hartman, USB list, lkml, laurent.pinchart Peter, You're right. This patch is wrong. I didn't remove the "uvc_hs_streaming_ep.bInterval" assignment. It has been fixed in v2. Pawel >> >>On Fri, Mar 5, 2021 at 12:40 AM Pawel Laszczak <mailto:pawell@cadence.com> wrote: >>From: Pawel Laszczak <mailto:pawell@cadence.com> >> >>Patch adds extra checking for bInterval passed by configfs. >>The 5.6.4 chapter of USB Specification (rev. 2.0) say: >>"A high-bandwidth endpoint must specify a period of 1x125 µs >>(i.e., a bInterval value of 1)." >> >>The issue was observed during testing UVC class on CV. >>I treat this change as improvement because we can control >>bInterval by configfs. >> >>Signed-off-by: Pawel Laszczak <mailto:pawell@cadence.com> >>--- >> drivers/usb/gadget/function/f_uvc.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >>diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c >>index 44b4352a2676..5d62720bb9e1 100644 >>--- a/drivers/usb/gadget/function/f_uvc.c >>+++ b/drivers/usb/gadget/function/f_uvc.c >>@@ -631,6 +631,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) >> cpu_to_le16(min(opts->streaming_maxpacket, 1023U)); >> uvc_fs_streaming_ep.bInterval = opts->streaming_interval; >> >>+ /* A high-bandwidth endpoint must specify a bInterval value of 1 */ >>+ if (max_packet_mult > 1) >>+ uvc_hs_streaming_ep.bInterval = 1; >>+ else >>+ uvc_hs_streaming_ep.bInterval = opts->streaming_interval; >>+ >> >>There is a "uvc_hs_streaming_ep.bInterval = opts->streaming_interval;" again at below code >>Besides, the default value is 1 for opts->streaming_interval. What the real issue you observed >>at CV test? >> > >The issue occurs when I intentionally set: > echo 3072 > functions/$FUNCTION/streaming_maxpacket > echo 4 > functions/$FUNCTION/streaming_interval > >Then for CV CH9 TD 9.5: Endpoint Descriptor test it got: >"(Mult = 2)Illegal high speed isochronous endpoint MaxPacketSize : 0x400 >(USB: 1.2.78) A High speed Interrupt/Isochronous endpoint must have a MaxPacketSize between >683 and 1024 and bInterval value of 1 when the Mult value is two." > >For default value CV passed. Of course, I can fix it by changing streaming_interval, but I thought that >it could be good to have protection against this issue. >Especially since Usb 2 specification say that bInterval must be 1 for high bandwidth endpoints. > >Pawel > >>Peter >> >> uvc_hs_streaming_ep.wMaxPacketSize = >> cpu_to_le16(max_packet_size | ((max_packet_mult - 1) << 11)); >> uvc_hs_streaming_ep.bInterval = opts->streaming_interval; >>-- >>2.25.1 Thanks, Pawel Laszczak ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] usb: gadget: uvc: add bInterval checking for HS mode 2021-03-08 8:28 ` Pawel Laszczak 2021-03-08 12:59 ` Pawel Laszczak @ 2021-03-10 5:45 ` Pawel Laszczak 1 sibling, 0 replies; 4+ messages in thread From: Pawel Laszczak @ 2021-03-10 5:45 UTC (permalink / raw) To: Peter Chen; +Cc: balbi, Greg Kroah-Hartman, USB list, lkml, laurent.pinchart Please check whether the problem occurs in this fragment of code: https://elixir.bootlin.com/linux/latest/source/drivers/usb/cdns3/gadget.c#L2569 zlp_buff is allocated with kzalloc. Pawel >>On Fri, Mar 5, 2021 at 12:40 AM Pawel Laszczak <mailto:pawell@cadence.com> wrote: >>From: Pawel Laszczak <mailto:pawell@cadence.com> >> >>Patch adds extra checking for bInterval passed by configfs. >>The 5.6.4 chapter of USB Specification (rev. 2.0) say: >>"A high-bandwidth endpoint must specify a period of 1x125 µs >>(i.e., a bInterval value of 1)." >> >>The issue was observed during testing UVC class on CV. >>I treat this change as improvement because we can control >>bInterval by configfs. >> >>Signed-off-by: Pawel Laszczak <mailto:pawell@cadence.com> >>--- >> drivers/usb/gadget/function/f_uvc.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >>diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c >>index 44b4352a2676..5d62720bb9e1 100644 >>--- a/drivers/usb/gadget/function/f_uvc.c >>+++ b/drivers/usb/gadget/function/f_uvc.c >>@@ -631,6 +631,12 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) >> cpu_to_le16(min(opts->streaming_maxpacket, 1023U)); >> uvc_fs_streaming_ep.bInterval = opts->streaming_interval; >> >>+ /* A high-bandwidth endpoint must specify a bInterval value of 1 */ >>+ if (max_packet_mult > 1) >>+ uvc_hs_streaming_ep.bInterval = 1; >>+ else >>+ uvc_hs_streaming_ep.bInterval = opts->streaming_interval; >>+ >> >>There is a "uvc_hs_streaming_ep.bInterval = opts->streaming_interval;" again at below code >>Besides, the default value is 1 for opts->streaming_interval. What the real issue you observed >>at CV test? >> > >The issue occurs when I intentionally set: > echo 3072 > functions/$FUNCTION/streaming_maxpacket > echo 4 > functions/$FUNCTION/streaming_interval > >Then for CV CH9 TD 9.5: Endpoint Descriptor test it got: >"(Mult = 2)Illegal high speed isochronous endpoint MaxPacketSize : 0x400 >(USB: 1.2.78) A High speed Interrupt/Isochronous endpoint must have a MaxPacketSize between >683 and 1024 and bInterval value of 1 when the Mult value is two." > >For default value CV passed. Of course, I can fix it by changing streaming_interval, but I thought that >it could be good to have protection against this issue. >Especially since Usb 2 specification say that bInterval must be 1 for high bandwidth endpoints. > >Pawel > >>Peter >> >> uvc_hs_streaming_ep.wMaxPacketSize = >> cpu_to_le16(max_packet_size | ((max_packet_mult - 1) << 11)); >> uvc_hs_streaming_ep.bInterval = opts->streaming_interval; >>-- >>2.25.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-10 5:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-04 10:45 [PATCH] usb: gadget: uvc: add bInterval checking for HS mode Pawel Laszczak [not found] ` <CAL411-p4C4F9K--U=C+Yhu7Uy6GxOxwCHpXtdrXYMObLegFm9Q@mail.gmail.com> 2021-03-08 8:28 ` Pawel Laszczak 2021-03-08 12:59 ` Pawel Laszczak 2021-03-10 5:45 ` Pawel Laszczak
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).