* [PATCH v2] usb: gadget: f_sourcesink: fix function params handling
@ 2015-09-24 15:23 Robert Baldyga
2015-09-24 16:49 ` Felipe Balbi
0 siblings, 1 reply; 5+ messages in thread
From: Robert Baldyga @ 2015-09-24 15:23 UTC (permalink / raw)
To: balbi
Cc: gregkh, linux-usb, linux-kernel, andrzej.p, b.zolnierkie,
Robert Baldyga, stable
Move function parameters to struct f_sourcesink to make them per instance
instead of having them as global variables. Since we can have multiple
instances of USB function we also want to have separate set of parameters
for each instance.
Cc: <stable@vger.kernel.org> # 3.10+
Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
drivers/usb/gadget/function/f_sourcesink.c | 120 +++++++++++++++--------------
1 file changed, 62 insertions(+), 58 deletions(-)
diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
index 1353465..128abfb 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -50,6 +50,13 @@ struct f_sourcesink {
struct usb_ep *iso_in_ep;
struct usb_ep *iso_out_ep;
int cur_alt;
+
+ unsigned pattern;
+ unsigned isoc_interval;
+ unsigned isoc_maxpacket;
+ unsigned isoc_mult;
+ unsigned isoc_maxburst;
+ unsigned buflen;
};
static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
@@ -57,13 +64,6 @@ static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
return container_of(f, struct f_sourcesink, function);
}
-static unsigned pattern;
-static unsigned isoc_interval;
-static unsigned isoc_maxpacket;
-static unsigned isoc_mult;
-static unsigned isoc_maxburst;
-static unsigned buflen;
-
/*-------------------------------------------------------------------------*/
static struct usb_interface_descriptor source_sink_intf_alt0 = {
@@ -298,7 +298,9 @@ static struct usb_gadget_strings *sourcesink_strings[] = {
static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
{
- return alloc_ep_req(ep, len, buflen);
+ struct f_sourcesink *ss = ep->driver_data;
+
+ return alloc_ep_req(ep, len, ss->buflen);
}
void free_ep_req(struct usb_ep *ep, struct usb_request *req)
@@ -357,22 +359,22 @@ autoconf_fail:
goto autoconf_fail;
/* sanity check the isoc module parameters */
- if (isoc_interval < 1)
- isoc_interval = 1;
- if (isoc_interval > 16)
- isoc_interval = 16;
- if (isoc_mult > 2)
- isoc_mult = 2;
- if (isoc_maxburst > 15)
- isoc_maxburst = 15;
+ if (ss->isoc_interval < 1)
+ ss->isoc_interval = 1;
+ if (ss->isoc_interval > 16)
+ ss->isoc_interval = 16;
+ if (ss->isoc_mult > 2)
+ ss->isoc_mult = 2;
+ if (ss->isoc_maxburst > 15)
+ ss->isoc_maxburst = 15;
/* fill in the FS isoc descriptors from the module parameters */
- fs_iso_source_desc.wMaxPacketSize = isoc_maxpacket > 1023 ?
- 1023 : isoc_maxpacket;
- fs_iso_source_desc.bInterval = isoc_interval;
- fs_iso_sink_desc.wMaxPacketSize = isoc_maxpacket > 1023 ?
- 1023 : isoc_maxpacket;
- fs_iso_sink_desc.bInterval = isoc_interval;
+ fs_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket > 1023 ?
+ 1023 : ss->isoc_maxpacket;
+ fs_iso_source_desc.bInterval = ss->isoc_interval;
+ fs_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket > 1023 ?
+ 1023 : ss->isoc_maxpacket;
+ fs_iso_sink_desc.bInterval = ss->isoc_interval;
/* allocate iso endpoints */
ss->iso_in_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_source_desc);
@@ -394,8 +396,8 @@ no_iso:
ss_source_sink_descs[SS_ALT_IFC_1_OFFSET] = NULL;
}
- if (isoc_maxpacket > 1024)
- isoc_maxpacket = 1024;
+ if (ss->isoc_maxpacket > 1024)
+ ss->isoc_maxpacket = 1024;
/* support high speed hardware */
hs_source_desc.bEndpointAddress = fs_source_desc.bEndpointAddress;
@@ -406,15 +408,15 @@ no_iso:
* We assume that the user knows what they are doing and won't
* give parameters that their UDC doesn't support.
*/
- hs_iso_source_desc.wMaxPacketSize = isoc_maxpacket;
- hs_iso_source_desc.wMaxPacketSize |= isoc_mult << 11;
- hs_iso_source_desc.bInterval = isoc_interval;
+ hs_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket;
+ hs_iso_source_desc.wMaxPacketSize |= ss->isoc_mult << 11;
+ hs_iso_source_desc.bInterval = ss->isoc_interval;
hs_iso_source_desc.bEndpointAddress =
fs_iso_source_desc.bEndpointAddress;
- hs_iso_sink_desc.wMaxPacketSize = isoc_maxpacket;
- hs_iso_sink_desc.wMaxPacketSize |= isoc_mult << 11;
- hs_iso_sink_desc.bInterval = isoc_interval;
+ hs_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket;
+ hs_iso_sink_desc.wMaxPacketSize |= ss->isoc_mult << 11;
+ hs_iso_sink_desc.bInterval = ss->isoc_interval;
hs_iso_sink_desc.bEndpointAddress = fs_iso_sink_desc.bEndpointAddress;
/* support super speed hardware */
@@ -428,21 +430,21 @@ no_iso:
* We assume that the user knows what they are doing and won't
* give parameters that their UDC doesn't support.
*/
- ss_iso_source_desc.wMaxPacketSize = isoc_maxpacket;
- ss_iso_source_desc.bInterval = isoc_interval;
- ss_iso_source_comp_desc.bmAttributes = isoc_mult;
- ss_iso_source_comp_desc.bMaxBurst = isoc_maxburst;
- ss_iso_source_comp_desc.wBytesPerInterval =
- isoc_maxpacket * (isoc_mult + 1) * (isoc_maxburst + 1);
+ ss_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket;
+ ss_iso_source_desc.bInterval = ss->isoc_interval;
+ ss_iso_source_comp_desc.bmAttributes = ss->isoc_mult;
+ ss_iso_source_comp_desc.bMaxBurst = ss->isoc_maxburst;
+ ss_iso_source_comp_desc.wBytesPerInterval = ss->isoc_maxpacket *
+ (ss->isoc_mult + 1) * (ss->isoc_maxburst + 1);
ss_iso_source_desc.bEndpointAddress =
fs_iso_source_desc.bEndpointAddress;
- ss_iso_sink_desc.wMaxPacketSize = isoc_maxpacket;
- ss_iso_sink_desc.bInterval = isoc_interval;
- ss_iso_sink_comp_desc.bmAttributes = isoc_mult;
- ss_iso_sink_comp_desc.bMaxBurst = isoc_maxburst;
- ss_iso_sink_comp_desc.wBytesPerInterval =
- isoc_maxpacket * (isoc_mult + 1) * (isoc_maxburst + 1);
+ ss_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket;
+ ss_iso_sink_desc.bInterval = ss->isoc_interval;
+ ss_iso_sink_comp_desc.bmAttributes = ss->isoc_mult;
+ ss_iso_sink_comp_desc.bMaxBurst = ss->isoc_maxburst;
+ ss_iso_sink_comp_desc.wBytesPerInterval = ss->isoc_maxpacket *
+ (ss->isoc_mult + 1) * (ss->isoc_maxburst + 1);
ss_iso_sink_desc.bEndpointAddress = fs_iso_sink_desc.bEndpointAddress;
ret = usb_assign_descriptors(f, fs_source_sink_descs,
@@ -482,11 +484,11 @@ static int check_read_data(struct f_sourcesink *ss, struct usb_request *req)
struct usb_composite_dev *cdev = ss->function.config->cdev;
int max_packet_size = le16_to_cpu(ss->out_ep->desc->wMaxPacketSize);
- if (pattern == 2)
+ if (ss->pattern == 2)
return 0;
for (i = 0; i < req->actual; i++, buf++) {
- switch (pattern) {
+ switch (ss->pattern) {
/* all-zeroes has no synchronization issues */
case 0:
@@ -518,8 +520,9 @@ static void reinit_write_data(struct usb_ep *ep, struct usb_request *req)
unsigned i;
u8 *buf = req->buf;
int max_packet_size = le16_to_cpu(ep->desc->wMaxPacketSize);
+ struct f_sourcesink *ss = ep->driver_data;
- switch (pattern) {
+ switch (ss->pattern) {
case 0:
memset(req->buf, 0, req->length);
break;
@@ -549,7 +552,7 @@ static void source_sink_complete(struct usb_ep *ep, struct usb_request *req)
case 0: /* normal completion? */
if (ep == ss->out_ep) {
check_read_data(ss, req);
- if (pattern != 2)
+ if (ss->pattern != 2)
memset(req->buf, 0x55, req->length);
}
break;
@@ -598,15 +601,16 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in,
if (is_iso) {
switch (speed) {
case USB_SPEED_SUPER:
- size = isoc_maxpacket * (isoc_mult + 1) *
- (isoc_maxburst + 1);
+ size = ss->isoc_maxpacket *
+ (ss->isoc_mult + 1) *
+ (ss->isoc_maxburst + 1);
break;
case USB_SPEED_HIGH:
- size = isoc_maxpacket * (isoc_mult + 1);
+ size = ss->isoc_maxpacket * (ss->isoc_mult + 1);
break;
default:
- size = isoc_maxpacket > 1023 ?
- 1023 : isoc_maxpacket;
+ size = ss->isoc_maxpacket > 1023 ?
+ 1023 : ss->isoc_maxpacket;
break;
}
ep = is_in ? ss->iso_in_ep : ss->iso_out_ep;
@@ -622,7 +626,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in,
req->complete = source_sink_complete;
if (is_in)
reinit_write_data(ep, req);
- else if (pattern != 2)
+ else if (ss->pattern != 2)
memset(req->buf, 0x55, req->length);
status = usb_ep_queue(ep, req, GFP_ATOMIC);
@@ -859,12 +863,12 @@ static struct usb_function *source_sink_alloc_func(
ss_opts->refcnt++;
mutex_unlock(&ss_opts->lock);
- pattern = ss_opts->pattern;
- isoc_interval = ss_opts->isoc_interval;
- isoc_maxpacket = ss_opts->isoc_maxpacket;
- isoc_mult = ss_opts->isoc_mult;
- isoc_maxburst = ss_opts->isoc_maxburst;
- buflen = ss_opts->bulk_buflen;
+ ss->pattern = ss_opts->pattern;
+ ss->isoc_interval = ss_opts->isoc_interval;
+ ss->isoc_maxpacket = ss_opts->isoc_maxpacket;
+ ss->isoc_mult = ss_opts->isoc_mult;
+ ss->isoc_maxburst = ss_opts->isoc_maxburst;
+ ss->buflen = ss_opts->bulk_buflen;
ss->function.name = "source/sink";
ss->function.bind = sourcesink_bind;
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb: gadget: f_sourcesink: fix function params handling
2015-09-24 15:23 [PATCH v2] usb: gadget: f_sourcesink: fix function params handling Robert Baldyga
@ 2015-09-24 16:49 ` Felipe Balbi
2015-09-24 17:35 ` Krzysztof Opasiak
2015-10-14 8:38 ` Robert Baldyga
0 siblings, 2 replies; 5+ messages in thread
From: Felipe Balbi @ 2015-09-24 16:49 UTC (permalink / raw)
To: Robert Baldyga
Cc: balbi, gregkh, linux-usb, linux-kernel, andrzej.p, b.zolnierkie,
stable
[-- Attachment #1: Type: text/plain, Size: 9830 bytes --]
On Thu, Sep 24, 2015 at 05:23:09PM +0200, Robert Baldyga wrote:
> Move function parameters to struct f_sourcesink to make them per instance
> instead of having them as global variables. Since we can have multiple
> instances of USB function we also want to have separate set of parameters
> for each instance.
>
> Cc: <stable@vger.kernel.org> # 3.10+
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
why do you think this is stable material ? Looks like it's not
fixing anything to me; just implementing a brand new requirement.
> ---
> drivers/usb/gadget/function/f_sourcesink.c | 120 +++++++++++++++--------------
> 1 file changed, 62 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
> index 1353465..128abfb 100644
> --- a/drivers/usb/gadget/function/f_sourcesink.c
> +++ b/drivers/usb/gadget/function/f_sourcesink.c
> @@ -50,6 +50,13 @@ struct f_sourcesink {
> struct usb_ep *iso_in_ep;
> struct usb_ep *iso_out_ep;
> int cur_alt;
> +
> + unsigned pattern;
> + unsigned isoc_interval;
> + unsigned isoc_maxpacket;
> + unsigned isoc_mult;
> + unsigned isoc_maxburst;
> + unsigned buflen;
> };
>
> static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
> @@ -57,13 +64,6 @@ static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
> return container_of(f, struct f_sourcesink, function);
> }
>
> -static unsigned pattern;
> -static unsigned isoc_interval;
> -static unsigned isoc_maxpacket;
> -static unsigned isoc_mult;
> -static unsigned isoc_maxburst;
> -static unsigned buflen;
> -
> /*-------------------------------------------------------------------------*/
>
> static struct usb_interface_descriptor source_sink_intf_alt0 = {
> @@ -298,7 +298,9 @@ static struct usb_gadget_strings *sourcesink_strings[] = {
>
> static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
> {
> - return alloc_ep_req(ep, len, buflen);
> + struct f_sourcesink *ss = ep->driver_data;
> +
> + return alloc_ep_req(ep, len, ss->buflen);
> }
>
> void free_ep_req(struct usb_ep *ep, struct usb_request *req)
> @@ -357,22 +359,22 @@ autoconf_fail:
> goto autoconf_fail;
>
> /* sanity check the isoc module parameters */
> - if (isoc_interval < 1)
> - isoc_interval = 1;
> - if (isoc_interval > 16)
> - isoc_interval = 16;
> - if (isoc_mult > 2)
> - isoc_mult = 2;
> - if (isoc_maxburst > 15)
> - isoc_maxburst = 15;
> + if (ss->isoc_interval < 1)
> + ss->isoc_interval = 1;
> + if (ss->isoc_interval > 16)
> + ss->isoc_interval = 16;
> + if (ss->isoc_mult > 2)
> + ss->isoc_mult = 2;
> + if (ss->isoc_maxburst > 15)
> + ss->isoc_maxburst = 15;
>
> /* fill in the FS isoc descriptors from the module parameters */
> - fs_iso_source_desc.wMaxPacketSize = isoc_maxpacket > 1023 ?
> - 1023 : isoc_maxpacket;
> - fs_iso_source_desc.bInterval = isoc_interval;
> - fs_iso_sink_desc.wMaxPacketSize = isoc_maxpacket > 1023 ?
> - 1023 : isoc_maxpacket;
> - fs_iso_sink_desc.bInterval = isoc_interval;
> + fs_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket > 1023 ?
> + 1023 : ss->isoc_maxpacket;
> + fs_iso_source_desc.bInterval = ss->isoc_interval;
> + fs_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket > 1023 ?
> + 1023 : ss->isoc_maxpacket;
> + fs_iso_sink_desc.bInterval = ss->isoc_interval;
>
> /* allocate iso endpoints */
> ss->iso_in_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_source_desc);
> @@ -394,8 +396,8 @@ no_iso:
> ss_source_sink_descs[SS_ALT_IFC_1_OFFSET] = NULL;
> }
>
> - if (isoc_maxpacket > 1024)
> - isoc_maxpacket = 1024;
> + if (ss->isoc_maxpacket > 1024)
> + ss->isoc_maxpacket = 1024;
>
> /* support high speed hardware */
> hs_source_desc.bEndpointAddress = fs_source_desc.bEndpointAddress;
> @@ -406,15 +408,15 @@ no_iso:
> * We assume that the user knows what they are doing and won't
> * give parameters that their UDC doesn't support.
> */
> - hs_iso_source_desc.wMaxPacketSize = isoc_maxpacket;
> - hs_iso_source_desc.wMaxPacketSize |= isoc_mult << 11;
> - hs_iso_source_desc.bInterval = isoc_interval;
> + hs_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket;
> + hs_iso_source_desc.wMaxPacketSize |= ss->isoc_mult << 11;
> + hs_iso_source_desc.bInterval = ss->isoc_interval;
> hs_iso_source_desc.bEndpointAddress =
> fs_iso_source_desc.bEndpointAddress;
>
> - hs_iso_sink_desc.wMaxPacketSize = isoc_maxpacket;
> - hs_iso_sink_desc.wMaxPacketSize |= isoc_mult << 11;
> - hs_iso_sink_desc.bInterval = isoc_interval;
> + hs_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket;
> + hs_iso_sink_desc.wMaxPacketSize |= ss->isoc_mult << 11;
> + hs_iso_sink_desc.bInterval = ss->isoc_interval;
> hs_iso_sink_desc.bEndpointAddress = fs_iso_sink_desc.bEndpointAddress;
>
> /* support super speed hardware */
> @@ -428,21 +430,21 @@ no_iso:
> * We assume that the user knows what they are doing and won't
> * give parameters that their UDC doesn't support.
> */
> - ss_iso_source_desc.wMaxPacketSize = isoc_maxpacket;
> - ss_iso_source_desc.bInterval = isoc_interval;
> - ss_iso_source_comp_desc.bmAttributes = isoc_mult;
> - ss_iso_source_comp_desc.bMaxBurst = isoc_maxburst;
> - ss_iso_source_comp_desc.wBytesPerInterval =
> - isoc_maxpacket * (isoc_mult + 1) * (isoc_maxburst + 1);
> + ss_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket;
> + ss_iso_source_desc.bInterval = ss->isoc_interval;
> + ss_iso_source_comp_desc.bmAttributes = ss->isoc_mult;
> + ss_iso_source_comp_desc.bMaxBurst = ss->isoc_maxburst;
> + ss_iso_source_comp_desc.wBytesPerInterval = ss->isoc_maxpacket *
> + (ss->isoc_mult + 1) * (ss->isoc_maxburst + 1);
> ss_iso_source_desc.bEndpointAddress =
> fs_iso_source_desc.bEndpointAddress;
>
> - ss_iso_sink_desc.wMaxPacketSize = isoc_maxpacket;
> - ss_iso_sink_desc.bInterval = isoc_interval;
> - ss_iso_sink_comp_desc.bmAttributes = isoc_mult;
> - ss_iso_sink_comp_desc.bMaxBurst = isoc_maxburst;
> - ss_iso_sink_comp_desc.wBytesPerInterval =
> - isoc_maxpacket * (isoc_mult + 1) * (isoc_maxburst + 1);
> + ss_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket;
> + ss_iso_sink_desc.bInterval = ss->isoc_interval;
> + ss_iso_sink_comp_desc.bmAttributes = ss->isoc_mult;
> + ss_iso_sink_comp_desc.bMaxBurst = ss->isoc_maxburst;
> + ss_iso_sink_comp_desc.wBytesPerInterval = ss->isoc_maxpacket *
> + (ss->isoc_mult + 1) * (ss->isoc_maxburst + 1);
> ss_iso_sink_desc.bEndpointAddress = fs_iso_sink_desc.bEndpointAddress;
>
> ret = usb_assign_descriptors(f, fs_source_sink_descs,
> @@ -482,11 +484,11 @@ static int check_read_data(struct f_sourcesink *ss, struct usb_request *req)
> struct usb_composite_dev *cdev = ss->function.config->cdev;
> int max_packet_size = le16_to_cpu(ss->out_ep->desc->wMaxPacketSize);
>
> - if (pattern == 2)
> + if (ss->pattern == 2)
> return 0;
>
> for (i = 0; i < req->actual; i++, buf++) {
> - switch (pattern) {
> + switch (ss->pattern) {
>
> /* all-zeroes has no synchronization issues */
> case 0:
> @@ -518,8 +520,9 @@ static void reinit_write_data(struct usb_ep *ep, struct usb_request *req)
> unsigned i;
> u8 *buf = req->buf;
> int max_packet_size = le16_to_cpu(ep->desc->wMaxPacketSize);
> + struct f_sourcesink *ss = ep->driver_data;
>
> - switch (pattern) {
> + switch (ss->pattern) {
> case 0:
> memset(req->buf, 0, req->length);
> break;
> @@ -549,7 +552,7 @@ static void source_sink_complete(struct usb_ep *ep, struct usb_request *req)
> case 0: /* normal completion? */
> if (ep == ss->out_ep) {
> check_read_data(ss, req);
> - if (pattern != 2)
> + if (ss->pattern != 2)
> memset(req->buf, 0x55, req->length);
> }
> break;
> @@ -598,15 +601,16 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in,
> if (is_iso) {
> switch (speed) {
> case USB_SPEED_SUPER:
> - size = isoc_maxpacket * (isoc_mult + 1) *
> - (isoc_maxburst + 1);
> + size = ss->isoc_maxpacket *
> + (ss->isoc_mult + 1) *
> + (ss->isoc_maxburst + 1);
> break;
> case USB_SPEED_HIGH:
> - size = isoc_maxpacket * (isoc_mult + 1);
> + size = ss->isoc_maxpacket * (ss->isoc_mult + 1);
> break;
> default:
> - size = isoc_maxpacket > 1023 ?
> - 1023 : isoc_maxpacket;
> + size = ss->isoc_maxpacket > 1023 ?
> + 1023 : ss->isoc_maxpacket;
> break;
> }
> ep = is_in ? ss->iso_in_ep : ss->iso_out_ep;
> @@ -622,7 +626,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in,
> req->complete = source_sink_complete;
> if (is_in)
> reinit_write_data(ep, req);
> - else if (pattern != 2)
> + else if (ss->pattern != 2)
> memset(req->buf, 0x55, req->length);
>
> status = usb_ep_queue(ep, req, GFP_ATOMIC);
> @@ -859,12 +863,12 @@ static struct usb_function *source_sink_alloc_func(
> ss_opts->refcnt++;
> mutex_unlock(&ss_opts->lock);
>
> - pattern = ss_opts->pattern;
> - isoc_interval = ss_opts->isoc_interval;
> - isoc_maxpacket = ss_opts->isoc_maxpacket;
> - isoc_mult = ss_opts->isoc_mult;
> - isoc_maxburst = ss_opts->isoc_maxburst;
> - buflen = ss_opts->bulk_buflen;
> + ss->pattern = ss_opts->pattern;
> + ss->isoc_interval = ss_opts->isoc_interval;
> + ss->isoc_maxpacket = ss_opts->isoc_maxpacket;
> + ss->isoc_mult = ss_opts->isoc_mult;
> + ss->isoc_maxburst = ss_opts->isoc_maxburst;
> + ss->buflen = ss_opts->bulk_buflen;
>
> ss->function.name = "source/sink";
> ss->function.bind = sourcesink_bind;
> --
> 1.9.1
>
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb: gadget: f_sourcesink: fix function params handling
2015-09-24 16:49 ` Felipe Balbi
@ 2015-09-24 17:35 ` Krzysztof Opasiak
2015-10-14 8:38 ` Robert Baldyga
1 sibling, 0 replies; 5+ messages in thread
From: Krzysztof Opasiak @ 2015-09-24 17:35 UTC (permalink / raw)
To: balbi, Robert Baldyga
Cc: gregkh, linux-usb, linux-kernel, andrzej.p, b.zolnierkie, stable
On 09/24/2015 06:49 PM, Felipe Balbi wrote:
> On Thu, Sep 24, 2015 at 05:23:09PM +0200, Robert Baldyga wrote:
>> Move function parameters to struct f_sourcesink to make them per instance
>> instead of having them as global variables. Since we can have multiple
>> instances of USB function we also want to have separate set of parameters
>> for each instance.
>>
>> Cc: <stable@vger.kernel.org> # 3.10+
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>
> why do you think this is stable material ? Looks like it's not
> fixing anything to me; just implementing a brand new requirement.
>
Please see my answer in loopback patch[1]. It's totally the same case.
Both those functions have the same bug.
Footnotes:
1 - http://marc.info/?l=linux-usb&m=144311466118170&w=2
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb: gadget: f_sourcesink: fix function params handling
2015-09-24 16:49 ` Felipe Balbi
2015-09-24 17:35 ` Krzysztof Opasiak
@ 2015-10-14 8:38 ` Robert Baldyga
2015-10-14 13:38 ` Felipe Balbi
1 sibling, 1 reply; 5+ messages in thread
From: Robert Baldyga @ 2015-10-14 8:38 UTC (permalink / raw)
To: balbi; +Cc: gregkh, linux-usb, linux-kernel, andrzej.p, b.zolnierkie, stable
Hi Felipe,
On 09/24/2015 06:49 PM, Felipe Balbi wrote:
> On Thu, Sep 24, 2015 at 05:23:09PM +0200, Robert Baldyga wrote:
>> Move function parameters to struct f_sourcesink to make them per instance
>> instead of having them as global variables. Since we can have multiple
>> instances of USB function we also want to have separate set of parameters
>> for each instance.
>>
>> Cc: <stable@vger.kernel.org> # 3.10+
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>
> why do you think this is stable material ? Looks like it's not
> fixing anything to me; just implementing a brand new requirement.
I will not insist on stable backporting of this patch, as it's actually
not very important fix (especially in case of sourcesink function).
Should I resend this patch without CC:stable?
Thanks,
Robert
>
>> ---
>> drivers/usb/gadget/function/f_sourcesink.c | 120 +++++++++++++++--------------
>> 1 file changed, 62 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
>> index 1353465..128abfb 100644
>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>> @@ -50,6 +50,13 @@ struct f_sourcesink {
>> struct usb_ep *iso_in_ep;
>> struct usb_ep *iso_out_ep;
>> int cur_alt;
>> +
>> + unsigned pattern;
>> + unsigned isoc_interval;
>> + unsigned isoc_maxpacket;
>> + unsigned isoc_mult;
>> + unsigned isoc_maxburst;
>> + unsigned buflen;
>> };
>>
>> static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
>> @@ -57,13 +64,6 @@ static inline struct f_sourcesink *func_to_ss(struct usb_function *f)
>> return container_of(f, struct f_sourcesink, function);
>> }
>>
>> -static unsigned pattern;
>> -static unsigned isoc_interval;
>> -static unsigned isoc_maxpacket;
>> -static unsigned isoc_mult;
>> -static unsigned isoc_maxburst;
>> -static unsigned buflen;
>> -
>> /*-------------------------------------------------------------------------*/
>>
>> static struct usb_interface_descriptor source_sink_intf_alt0 = {
>> @@ -298,7 +298,9 @@ static struct usb_gadget_strings *sourcesink_strings[] = {
>>
>> static inline struct usb_request *ss_alloc_ep_req(struct usb_ep *ep, int len)
>> {
>> - return alloc_ep_req(ep, len, buflen);
>> + struct f_sourcesink *ss = ep->driver_data;
>> +
>> + return alloc_ep_req(ep, len, ss->buflen);
>> }
>>
>> void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>> @@ -357,22 +359,22 @@ autoconf_fail:
>> goto autoconf_fail;
>>
>> /* sanity check the isoc module parameters */
>> - if (isoc_interval < 1)
>> - isoc_interval = 1;
>> - if (isoc_interval > 16)
>> - isoc_interval = 16;
>> - if (isoc_mult > 2)
>> - isoc_mult = 2;
>> - if (isoc_maxburst > 15)
>> - isoc_maxburst = 15;
>> + if (ss->isoc_interval < 1)
>> + ss->isoc_interval = 1;
>> + if (ss->isoc_interval > 16)
>> + ss->isoc_interval = 16;
>> + if (ss->isoc_mult > 2)
>> + ss->isoc_mult = 2;
>> + if (ss->isoc_maxburst > 15)
>> + ss->isoc_maxburst = 15;
>>
>> /* fill in the FS isoc descriptors from the module parameters */
>> - fs_iso_source_desc.wMaxPacketSize = isoc_maxpacket > 1023 ?
>> - 1023 : isoc_maxpacket;
>> - fs_iso_source_desc.bInterval = isoc_interval;
>> - fs_iso_sink_desc.wMaxPacketSize = isoc_maxpacket > 1023 ?
>> - 1023 : isoc_maxpacket;
>> - fs_iso_sink_desc.bInterval = isoc_interval;
>> + fs_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket > 1023 ?
>> + 1023 : ss->isoc_maxpacket;
>> + fs_iso_source_desc.bInterval = ss->isoc_interval;
>> + fs_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket > 1023 ?
>> + 1023 : ss->isoc_maxpacket;
>> + fs_iso_sink_desc.bInterval = ss->isoc_interval;
>>
>> /* allocate iso endpoints */
>> ss->iso_in_ep = usb_ep_autoconfig(cdev->gadget, &fs_iso_source_desc);
>> @@ -394,8 +396,8 @@ no_iso:
>> ss_source_sink_descs[SS_ALT_IFC_1_OFFSET] = NULL;
>> }
>>
>> - if (isoc_maxpacket > 1024)
>> - isoc_maxpacket = 1024;
>> + if (ss->isoc_maxpacket > 1024)
>> + ss->isoc_maxpacket = 1024;
>>
>> /* support high speed hardware */
>> hs_source_desc.bEndpointAddress = fs_source_desc.bEndpointAddress;
>> @@ -406,15 +408,15 @@ no_iso:
>> * We assume that the user knows what they are doing and won't
>> * give parameters that their UDC doesn't support.
>> */
>> - hs_iso_source_desc.wMaxPacketSize = isoc_maxpacket;
>> - hs_iso_source_desc.wMaxPacketSize |= isoc_mult << 11;
>> - hs_iso_source_desc.bInterval = isoc_interval;
>> + hs_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket;
>> + hs_iso_source_desc.wMaxPacketSize |= ss->isoc_mult << 11;
>> + hs_iso_source_desc.bInterval = ss->isoc_interval;
>> hs_iso_source_desc.bEndpointAddress =
>> fs_iso_source_desc.bEndpointAddress;
>>
>> - hs_iso_sink_desc.wMaxPacketSize = isoc_maxpacket;
>> - hs_iso_sink_desc.wMaxPacketSize |= isoc_mult << 11;
>> - hs_iso_sink_desc.bInterval = isoc_interval;
>> + hs_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket;
>> + hs_iso_sink_desc.wMaxPacketSize |= ss->isoc_mult << 11;
>> + hs_iso_sink_desc.bInterval = ss->isoc_interval;
>> hs_iso_sink_desc.bEndpointAddress = fs_iso_sink_desc.bEndpointAddress;
>>
>> /* support super speed hardware */
>> @@ -428,21 +430,21 @@ no_iso:
>> * We assume that the user knows what they are doing and won't
>> * give parameters that their UDC doesn't support.
>> */
>> - ss_iso_source_desc.wMaxPacketSize = isoc_maxpacket;
>> - ss_iso_source_desc.bInterval = isoc_interval;
>> - ss_iso_source_comp_desc.bmAttributes = isoc_mult;
>> - ss_iso_source_comp_desc.bMaxBurst = isoc_maxburst;
>> - ss_iso_source_comp_desc.wBytesPerInterval =
>> - isoc_maxpacket * (isoc_mult + 1) * (isoc_maxburst + 1);
>> + ss_iso_source_desc.wMaxPacketSize = ss->isoc_maxpacket;
>> + ss_iso_source_desc.bInterval = ss->isoc_interval;
>> + ss_iso_source_comp_desc.bmAttributes = ss->isoc_mult;
>> + ss_iso_source_comp_desc.bMaxBurst = ss->isoc_maxburst;
>> + ss_iso_source_comp_desc.wBytesPerInterval = ss->isoc_maxpacket *
>> + (ss->isoc_mult + 1) * (ss->isoc_maxburst + 1);
>> ss_iso_source_desc.bEndpointAddress =
>> fs_iso_source_desc.bEndpointAddress;
>>
>> - ss_iso_sink_desc.wMaxPacketSize = isoc_maxpacket;
>> - ss_iso_sink_desc.bInterval = isoc_interval;
>> - ss_iso_sink_comp_desc.bmAttributes = isoc_mult;
>> - ss_iso_sink_comp_desc.bMaxBurst = isoc_maxburst;
>> - ss_iso_sink_comp_desc.wBytesPerInterval =
>> - isoc_maxpacket * (isoc_mult + 1) * (isoc_maxburst + 1);
>> + ss_iso_sink_desc.wMaxPacketSize = ss->isoc_maxpacket;
>> + ss_iso_sink_desc.bInterval = ss->isoc_interval;
>> + ss_iso_sink_comp_desc.bmAttributes = ss->isoc_mult;
>> + ss_iso_sink_comp_desc.bMaxBurst = ss->isoc_maxburst;
>> + ss_iso_sink_comp_desc.wBytesPerInterval = ss->isoc_maxpacket *
>> + (ss->isoc_mult + 1) * (ss->isoc_maxburst + 1);
>> ss_iso_sink_desc.bEndpointAddress = fs_iso_sink_desc.bEndpointAddress;
>>
>> ret = usb_assign_descriptors(f, fs_source_sink_descs,
>> @@ -482,11 +484,11 @@ static int check_read_data(struct f_sourcesink *ss, struct usb_request *req)
>> struct usb_composite_dev *cdev = ss->function.config->cdev;
>> int max_packet_size = le16_to_cpu(ss->out_ep->desc->wMaxPacketSize);
>>
>> - if (pattern == 2)
>> + if (ss->pattern == 2)
>> return 0;
>>
>> for (i = 0; i < req->actual; i++, buf++) {
>> - switch (pattern) {
>> + switch (ss->pattern) {
>>
>> /* all-zeroes has no synchronization issues */
>> case 0:
>> @@ -518,8 +520,9 @@ static void reinit_write_data(struct usb_ep *ep, struct usb_request *req)
>> unsigned i;
>> u8 *buf = req->buf;
>> int max_packet_size = le16_to_cpu(ep->desc->wMaxPacketSize);
>> + struct f_sourcesink *ss = ep->driver_data;
>>
>> - switch (pattern) {
>> + switch (ss->pattern) {
>> case 0:
>> memset(req->buf, 0, req->length);
>> break;
>> @@ -549,7 +552,7 @@ static void source_sink_complete(struct usb_ep *ep, struct usb_request *req)
>> case 0: /* normal completion? */
>> if (ep == ss->out_ep) {
>> check_read_data(ss, req);
>> - if (pattern != 2)
>> + if (ss->pattern != 2)
>> memset(req->buf, 0x55, req->length);
>> }
>> break;
>> @@ -598,15 +601,16 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in,
>> if (is_iso) {
>> switch (speed) {
>> case USB_SPEED_SUPER:
>> - size = isoc_maxpacket * (isoc_mult + 1) *
>> - (isoc_maxburst + 1);
>> + size = ss->isoc_maxpacket *
>> + (ss->isoc_mult + 1) *
>> + (ss->isoc_maxburst + 1);
>> break;
>> case USB_SPEED_HIGH:
>> - size = isoc_maxpacket * (isoc_mult + 1);
>> + size = ss->isoc_maxpacket * (ss->isoc_mult + 1);
>> break;
>> default:
>> - size = isoc_maxpacket > 1023 ?
>> - 1023 : isoc_maxpacket;
>> + size = ss->isoc_maxpacket > 1023 ?
>> + 1023 : ss->isoc_maxpacket;
>> break;
>> }
>> ep = is_in ? ss->iso_in_ep : ss->iso_out_ep;
>> @@ -622,7 +626,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in,
>> req->complete = source_sink_complete;
>> if (is_in)
>> reinit_write_data(ep, req);
>> - else if (pattern != 2)
>> + else if (ss->pattern != 2)
>> memset(req->buf, 0x55, req->length);
>>
>> status = usb_ep_queue(ep, req, GFP_ATOMIC);
>> @@ -859,12 +863,12 @@ static struct usb_function *source_sink_alloc_func(
>> ss_opts->refcnt++;
>> mutex_unlock(&ss_opts->lock);
>>
>> - pattern = ss_opts->pattern;
>> - isoc_interval = ss_opts->isoc_interval;
>> - isoc_maxpacket = ss_opts->isoc_maxpacket;
>> - isoc_mult = ss_opts->isoc_mult;
>> - isoc_maxburst = ss_opts->isoc_maxburst;
>> - buflen = ss_opts->bulk_buflen;
>> + ss->pattern = ss_opts->pattern;
>> + ss->isoc_interval = ss_opts->isoc_interval;
>> + ss->isoc_maxpacket = ss_opts->isoc_maxpacket;
>> + ss->isoc_mult = ss_opts->isoc_mult;
>> + ss->isoc_maxburst = ss_opts->isoc_maxburst;
>> + ss->buflen = ss_opts->bulk_buflen;
>>
>> ss->function.name = "source/sink";
>> ss->function.bind = sourcesink_bind;
>> --
>> 1.9.1
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] usb: gadget: f_sourcesink: fix function params handling
2015-10-14 8:38 ` Robert Baldyga
@ 2015-10-14 13:38 ` Felipe Balbi
0 siblings, 0 replies; 5+ messages in thread
From: Felipe Balbi @ 2015-10-14 13:38 UTC (permalink / raw)
To: Robert Baldyga
Cc: gregkh, linux-usb, linux-kernel, andrzej.p, b.zolnierkie, stable
[-- Attachment #1: Type: text/plain, Size: 932 bytes --]
Hi,
Robert Baldyga <r.baldyga@samsung.com> writes:
> Hi Felipe,
>
> On 09/24/2015 06:49 PM, Felipe Balbi wrote:
>> On Thu, Sep 24, 2015 at 05:23:09PM +0200, Robert Baldyga wrote:
>>> Move function parameters to struct f_sourcesink to make them per instance
>>> instead of having them as global variables. Since we can have multiple
>>> instances of USB function we also want to have separate set of parameters
>>> for each instance.
>>>
>>> Cc: <stable@vger.kernel.org> # 3.10+
>>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>>
>> why do you think this is stable material ? Looks like it's not
>> fixing anything to me; just implementing a brand new requirement.
>
> I will not insist on stable backporting of this patch, as it's actually
> not very important fix (especially in case of sourcesink function).
>
> Should I resend this patch without CC:stable?
please, thank you.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-10-14 13:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-24 15:23 [PATCH v2] usb: gadget: f_sourcesink: fix function params handling Robert Baldyga
2015-09-24 16:49 ` Felipe Balbi
2015-09-24 17:35 ` Krzysztof Opasiak
2015-10-14 8:38 ` Robert Baldyga
2015-10-14 13:38 ` Felipe Balbi
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).