[PATCH] can: kvaser_usb: fix usb endpoints detection

[PATCH] can: kvaser_usb: fix usb endpoints detection

[Olivier Sobrie]

Some devices, like the Kvaser Memorator Professional, have several
bulk in endpoints. Only the first one found should be used by the
driver. The same holds for the bulk out endpoint.

Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
---
 drivers/net/can/usb/kvaser_usb.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 3b95465..bdbf5bf 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1554,13 +1554,14 @@ static void kvaser_usb_get_endpoints(const struct usb_interface *intf,
 
 	iface_desc = &intf->altsetting[0];
 
-	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
+	for (i = 0; (i < iface_desc->desc.bNumEndpoints) && (!*in || !*out);
+	     ++i) {
 		endpoint = &iface_desc->endpoint[i].desc;
 
-		if (usb_endpoint_is_bulk_in(endpoint))
+		if (!*in && usb_endpoint_is_bulk_in(endpoint))
 			*in = endpoint;
 
-		if (usb_endpoint_is_bulk_out(endpoint))
+		if (!*out && usb_endpoint_is_bulk_out(endpoint))
 			*out = endpoint;
 	}
 }
-- 
1.8.3.2

Re: [PATCH] can: kvaser_usb: fix usb endpoints detection

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On 10/27/2013 10:07 PM, Olivier Sobrie wrote:
> Some devices, like the Kvaser Memorator Professional, have several
> bulk in endpoints. Only the first one found should be used by the
> driver. The same holds for the bulk out endpoint.

Can you give reference where this information comes from? e.g.
documentation or another driver?

> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> ---
>  drivers/net/can/usb/kvaser_usb.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> index 3b95465..bdbf5bf 100644
> --- a/drivers/net/can/usb/kvaser_usb.c
> +++ b/drivers/net/can/usb/kvaser_usb.c
> @@ -1554,13 +1554,14 @@ static void kvaser_usb_get_endpoints(const struct usb_interface *intf,
>  
>  	iface_desc = &intf->altsetting[0];
>  
> -	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> +	for (i = 0; (i < iface_desc->desc.bNumEndpoints) && (!*in || !*out);
> +	     ++i) {
>  		endpoint = &iface_desc->endpoint[i].desc;
>  
> -		if (usb_endpoint_is_bulk_in(endpoint))
> +		if (!*in && usb_endpoint_is_bulk_in(endpoint))
>  			*in = endpoint;
>  
> -		if (usb_endpoint_is_bulk_out(endpoint))
> +		if (!*out && usb_endpoint_is_bulk_out(endpoint))
>  			*out = endpoint;

I personally would add a break or return here, together with a short
comment:

	/* only use first endpoint for in and out */
	if (*in && *out)
		return;

>  	}
>  }
> 

Thanks for taking care of the driver,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] can: kvaser_usb: fix usb endpoints detection

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]

On 10/28/2013 12:29 AM, Marc Kleine-Budde wrote:
> On 10/27/2013 10:07 PM, Olivier Sobrie wrote:
>> Some devices, like the Kvaser Memorator Professional, have several
>> bulk in endpoints. Only the first one found should be used by the
>> driver. The same holds for the bulk out endpoint.
> 
> Can you give reference where this information comes from? e.g.
> documentation or another driver?
> 
>> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
>> ---
>>  drivers/net/can/usb/kvaser_usb.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
>> index 3b95465..bdbf5bf 100644
>> --- a/drivers/net/can/usb/kvaser_usb.c
>> +++ b/drivers/net/can/usb/kvaser_usb.c
>> @@ -1554,13 +1554,14 @@ static void kvaser_usb_get_endpoints(const struct usb_interface *intf,
>>  
>>  	iface_desc = &intf->altsetting[0];
>>  
>> -	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
>> +	for (i = 0; (i < iface_desc->desc.bNumEndpoints) && (!*in || !*out);
>> +	     ++i) {
>>  		endpoint = &iface_desc->endpoint[i].desc;
>>  
>> -		if (usb_endpoint_is_bulk_in(endpoint))
>> +		if (!*in && usb_endpoint_is_bulk_in(endpoint))
>>  			*in = endpoint;
>>  
>> -		if (usb_endpoint_is_bulk_out(endpoint))
>> +		if (!*out && usb_endpoint_is_bulk_out(endpoint))
>>  			*out = endpoint;
> 
> I personally would add a break or return here, together with a short
> comment:
> 
> 	/* only use first endpoint for in and out */
> 	if (*in && *out)
> 		return;

With this break or return you can keep the for-loop as it is.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] can: kvaser_usb: fix usb endpoints detection

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

On 10/27/2013 10:07 PM, Olivier Sobrie wrote:
> Some devices, like the Kvaser Memorator Professional, have several
> bulk in endpoints. Only the first one found should be used by the
> driver. The same holds for the bulk out endpoint.

Is this a critical fix? Does the hardware work without this patch?
Should I add stable on Cc?

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [PATCH] can: kvaser_usb: fix usb endpoints detection

[Olivier Sobrie]

On Mon, Oct 28, 2013 at 12:29:31AM +0100, Marc Kleine-Budde wrote:
> On 10/27/2013 10:07 PM, Olivier Sobrie wrote:
> > Some devices, like the Kvaser Memorator Professional, have several
> > bulk in endpoints. Only the first one found should be used by the
> > driver. The same holds for the bulk out endpoint.
> 
> Can you give reference where this information comes from? e.g.
> documentation or another driver?

Someone using a Kvaser Memorator contacted me and said that its device was
not working. I found out that the usb endpoint used was not correct and I
saw in the Kvaser official driver (leaf) that the detection was made
similarly to what is done in the patch below.
So the reference is the "leaf" driver of Kvaser.

> 
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> >  drivers/net/can/usb/kvaser_usb.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> > index 3b95465..bdbf5bf 100644
> > --- a/drivers/net/can/usb/kvaser_usb.c
> > +++ b/drivers/net/can/usb/kvaser_usb.c
> > @@ -1554,13 +1554,14 @@ static void kvaser_usb_get_endpoints(const struct usb_interface *intf,
> >  
> >  	iface_desc = &intf->altsetting[0];
> >  
> > -	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> > +	for (i = 0; (i < iface_desc->desc.bNumEndpoints) && (!*in || !*out);
> > +	     ++i) {
> >  		endpoint = &iface_desc->endpoint[i].desc;
> >  
> > -		if (usb_endpoint_is_bulk_in(endpoint))
> > +		if (!*in && usb_endpoint_is_bulk_in(endpoint))
> >  			*in = endpoint;
> >  
> > -		if (usb_endpoint_is_bulk_out(endpoint))
> > +		if (!*out && usb_endpoint_is_bulk_out(endpoint))
> >  			*out = endpoint;
> 
> I personally would add a break or return here, together with a short
> comment:
> 
> 	/* only use first endpoint for in and out */
> 	if (*in && *out)
> 		return;
> 
> >  	}
> >  }
> > 

Fine for me. We can even maybe add a return code to the function and check
this return code intead of checking again (!dev->bulk_in || !dev->bulk_out).
By the way, the current return code (-ENOMEM) is maybe not appropriate in
case the endpoints are not found.

Thank you,

-- 
Olivier

Re: [PATCH] can: kvaser_usb: fix usb endpoints detection

[Olivier Sobrie]

On Mon, Oct 28, 2013 at 10:25:42AM +0100, Marc Kleine-Budde wrote:
> On 10/27/2013 10:07 PM, Olivier Sobrie wrote:
> > Some devices, like the Kvaser Memorator Professional, have several
> > bulk in endpoints. Only the first one found should be used by the
> > driver. The same holds for the bulk out endpoint.
> 
> Is this a critical fix? Does the hardware work without this patch?
> Should I add stable on Cc?

Yes it is critical for all devices like Kvaser Memorator Pro.
Without this patch these devices don't work.
So yes, you can add stable in cc.

Kr,

-- 
Olivier

Re: [PATCH] can: kvaser_usb: fix usb endpoints detection

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]

On 10/28/2013 01:06 PM, Olivier Sobrie wrote:
> On Mon, Oct 28, 2013 at 12:29:31AM +0100, Marc Kleine-Budde wrote:
>> On 10/27/2013 10:07 PM, Olivier Sobrie wrote:
>>> Some devices, like the Kvaser Memorator Professional, have several
>>> bulk in endpoints. Only the first one found should be used by the
>>> driver. The same holds for the bulk out endpoint.
>>
>> Can you give reference where this information comes from? e.g.
>> documentation or another driver?
> 
> Someone using a Kvaser Memorator contacted me and said that its device was
> not working. I found out that the usb endpoint used was not correct and I
> saw in the Kvaser official driver (leaf) that the detection was made
> similarly to what is done in the patch below.
> So the reference is the "leaf" driver of Kvaser.
> 
>>
>>> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
>>> ---
>>>  drivers/net/can/usb/kvaser_usb.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
>>> index 3b95465..bdbf5bf 100644
>>> --- a/drivers/net/can/usb/kvaser_usb.c
>>> +++ b/drivers/net/can/usb/kvaser_usb.c
>>> @@ -1554,13 +1554,14 @@ static void kvaser_usb_get_endpoints(const struct usb_interface *intf,
>>>  
>>>  	iface_desc = &intf->altsetting[0];
>>>  
>>> -	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
>>> +	for (i = 0; (i < iface_desc->desc.bNumEndpoints) && (!*in || !*out);
>>> +	     ++i) {
>>>  		endpoint = &iface_desc->endpoint[i].desc;
>>>  
>>> -		if (usb_endpoint_is_bulk_in(endpoint))
>>> +		if (!*in && usb_endpoint_is_bulk_in(endpoint))
>>>  			*in = endpoint;
>>>  
>>> -		if (usb_endpoint_is_bulk_out(endpoint))
>>> +		if (!*out && usb_endpoint_is_bulk_out(endpoint))
>>>  			*out = endpoint;
>>
>> I personally would add a break or return here, together with a short
>> comment:
>>
>> 	/* only use first endpoint for in and out */
>> 	if (*in && *out)
>> 		return;
>>
>>>  	}
>>>  }
>>>
> 
> Fine for me. We can even maybe add a return code to the function and check
> this return code intead of checking again (!dev->bulk_in || !dev->bulk_out).
> By the way, the current return code (-ENOMEM) is maybe not appropriate in
> case the endpoints are not found.

Good idea, have a look at v3.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]