| Message ID | 20210304104515.35933-1-pawell@gli-login.cadence.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: gadget: uvc: add bInterval checking for HS mode | expand |
> >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
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
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
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;