Re: [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation

[Vincent Mailhol <mailhol.vincent@wanadoo.fr>]

On 25/06/2025 at 14:47, Markus Elfring wrote:
>>>> it seems that Coccinelle missed many simplifications.
>>> Would such software transformations become better supported anyhow?
>> Maybe?
>>
>> I am not involved in the development of Coccinelle and thus, I don't have an answer.
> 
> This can be fine (in principle).
> 
> 
>> Nor do I have the time to read and understand the Coccinelle source code
>> to which you pointed me to.
> 
> I hope that more users can become interested in presented information.
> I would appreciate if knowledge representations can be improved also for
> better automatic data processing and corresponding transformations.

A real quick search shows me that this ucan driver is not an isolated case. Here
is an example:

https://elixir.bootlin.com/linux/v6.16-rc3/source/drivers/media/rc/imon.c#L2137-L2148

But it does not seem to occur so often either. So not sure what is the best: do
a manual hunt or write a coccinelle checker. I let you be judge here.

>> My stance is that such static analyzers should never be trusted 100%.
> 
> This is generally fine.
> 
> 
>> The output is more an indicator.
> 
> This is usual.
> 
> 
>>                       And in this present case, a quick review made it very
>> clear that Coccinelle saw one simplification but missed two other ones.
> 
> Would you find another source code adjustment (like the following) more appropriate?

Yes. What you are proposing below is aligned with my initial comments.

> diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
> index 07406daf7c88..6c6cee3895c6 100644
> --- a/drivers/net/can/usb/ucan.c
> +++ b/drivers/net/can/usb/ucan.c
> @@ -1352,17 +1352,12 @@ static int ucan_probe(struct usb_interface *intf,
>  	for (i = 0; i < iface_desc->desc.bNumEndpoints; i++) {
>  		ep = &iface_desc->endpoint[i].desc;
>  
> -		if (((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != 0) &&
> -		    ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
> -		     USB_ENDPOINT_XFER_BULK)) {
> +		if (usb_endpoint_dir_in(ep) && usb_endpoint_xfer_bulk(ep)) {
>  			/* In Endpoint */
                           ^^^^^^^^^^^
Maybe just remove this comment. After migrating to usb_endpoint_dir_in(ep), this
comment is just paraphrasing the code and has no more reasons to stay.

>  			in_ep_addr = ep->bEndpointAddress;
>  			in_ep_addr &= USB_ENDPOINT_NUMBER_MASK;
>  			in_ep_size = le16_to_cpu(ep->wMaxPacketSize);
> -		} else if (((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) ==
> -			    0) &&
> -			   ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
> -			    USB_ENDPOINT_XFER_BULK)) {
> +		} else if (usb_endpoint_dir_out(ep) && usb_endpoint_xfer_bulk(ep)) {
>  			/* Out Endpoint */
                           ^^^^^^^^^^^^
Same.

>  			out_ep_addr = ep->bEndpointAddress;
>  			out_ep_addr &= USB_ENDPOINT_NUMBER_MASK;
> 
> 
> Can the functions “usb_endpoint_is_bulk_in” and “usb_endpoint_is_bulk_out”
> be applied here?
> https://elixir.bootlin.com/linux/v6.16-rc3/source/include/uapi/linux/usb/ch9.h#L572-L595

Further simplification, nice :)

I didn't see that last one, so glad you found what seems to be the optimal solution!


Yours sincerely,
Vincent Mailhol