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
