* [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
@ 2025-06-24 17:12 Markus Elfring
2025-06-24 17:29 ` Vincent Mailhol
0 siblings, 1 reply; 16+ messages in thread
From: Markus Elfring @ 2025-06-24 17:12 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Vincent Mailhol
Cc: LKML, kernel-janitors, Chen Ni
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 24 Jun 2025 19:05:04 +0200
Reuse existing functionality from usb_endpoint_type() instead of keeping
duplicate source code.
The source code was transformed by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/can/usb/ucan.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index 07406daf7c88..3c8dad8bcca4 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -1353,16 +1353,14 @@ static int ucan_probe(struct usb_interface *intf,
ep = &iface_desc->endpoint[i].desc;
if (((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != 0) &&
- ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
- USB_ENDPOINT_XFER_BULK)) {
+ usb_endpoint_type(ep) == USB_ENDPOINT_XFER_BULK) {
/* In Endpoint */
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)) {
+ usb_endpoint_type(ep) == USB_ENDPOINT_XFER_BULK) {
/* Out Endpoint */
out_ep_addr = ep->bEndpointAddress;
out_ep_addr &= USB_ENDPOINT_NUMBER_MASK;
--
2.50.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-24 17:12 [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation Markus Elfring
@ 2025-06-24 17:29 ` Vincent Mailhol
2025-06-24 18:28 ` Markus Elfring
2025-06-24 19:21 ` [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation Markus Elfring
0 siblings, 2 replies; 16+ messages in thread
From: Vincent Mailhol @ 2025-06-24 17:29 UTC (permalink / raw)
To: Markus Elfring
Cc: LKML, kernel-janitors, Chen Ni, linux-can, Marc Kleine-Budde
Hi Markus,
On 25/06/2025 at 02:12, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 24 Jun 2025 19:05:04 +0200
>
> Reuse existing functionality from usb_endpoint_type() instead of keeping
> duplicate source code.
>
> The source code was transformed by using the Coccinelle software.
Looking at the helpers in linux/usb/ch9.h, it seems that Coccinelle missed many
simplifications.
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Can you use the same e-mail address for signing and for sending the patch?
> ---
> drivers/net/can/usb/ucan.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
> index 07406daf7c88..3c8dad8bcca4 100644
> --- a/drivers/net/can/usb/ucan.c
> +++ b/drivers/net/can/usb/ucan.c
> @@ -1353,16 +1353,14 @@ static int ucan_probe(struct usb_interface *intf,
> ep = &iface_desc->endpoint[i].desc;
>
> if (((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != 0) &&
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is:
usb_endpoint_dir_in(ep)
> - ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
> - USB_ENDPOINT_XFER_BULK)) {
> + usb_endpoint_type(ep) == USB_ENDPOINT_XFER_BULK) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This is:
usb_endpoint_xfer_bulk(ep)
> /* In Endpoint */
After changing to usb_endpoint_dir_in(ep), this comment can be removed.
> 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)) {
> + usb_endpoint_type(ep) == USB_ENDPOINT_XFER_BULK) {
Similarly as above use:
usb_endpoint_dir_out(ep)
and:
usb_endpoint_xfer_bulk(ep)
> /* Out Endpoint */
> out_ep_addr = ep->bEndpointAddress;
> out_ep_addr &= USB_ENDPOINT_NUMBER_MASK;
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-24 17:29 ` Vincent Mailhol
@ 2025-06-24 18:28 ` Markus Elfring
2025-06-24 22:59 ` Vincent Mailhol
2025-06-24 19:21 ` [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation Markus Elfring
1 sibling, 1 reply; 16+ messages in thread
From: Markus Elfring @ 2025-06-24 18:28 UTC (permalink / raw)
To: Vincent Mailhol, linux-can
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde
> Looking at the helpers in linux/usb/ch9.h,
Please take another look at known source code mappings.
Passing code replacements by APIs (for SmPL)?
https://lore.kernel.org/cocci/481faa1d-7171-4657-8dc0-c37b153e6eaa@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2025-06/msg00044.html
> it seems that Coccinelle missed many simplifications.
Would such software transformations become better supported anyhow?
Regards,
Markus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-24 17:29 ` Vincent Mailhol
2025-06-24 18:28 ` Markus Elfring
@ 2025-06-24 19:21 ` Markus Elfring
2025-06-24 23:03 ` Vincent Mailhol
1 sibling, 1 reply; 16+ messages in thread
From: Markus Elfring @ 2025-06-24 19:21 UTC (permalink / raw)
To: Vincent Mailhol, linux-can
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde
>> +++ b/drivers/net/can/usb/ucan.c
>> @@ -1353,16 +1353,14 @@ static int ucan_probe(struct usb_interface *intf,
>> ep = &iface_desc->endpoint[i].desc;
>>
>> if (((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != 0) &&
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This is:
>
> usb_endpoint_dir_in(ep)
Can the check for a single value like “USB_DIR_IN” be really mapped to
an other value range?
https://elixir.bootlin.com/linux/v6.16-rc3/source/include/uapi/linux/usb/ch9.h#L495-L503
Regards,
Markus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-24 18:28 ` Markus Elfring
@ 2025-06-24 22:59 ` Vincent Mailhol
2025-06-25 5:47 ` [PATCH] " Markus Elfring
0 siblings, 1 reply; 16+ messages in thread
From: Vincent Mailhol @ 2025-06-24 22:59 UTC (permalink / raw)
To: Markus Elfring
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde, linux-can
On 25/06/2025 at 03:28, Markus Elfring wrote:
>> Looking at the helpers in linux/usb/ch9.h,
>
> Please take another look at known source code mappings.
>
> Passing code replacements by APIs (for SmPL)?
> https://lore.kernel.org/cocci/481faa1d-7171-4657-8dc0-c37b153e6eaa@web.de/
> https://sympa.inria.fr/sympa/arc/cocci/2025-06/msg00044.html
>
>
>> 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. Nor do I have the time to read and understand the Coccinelle source code
to which you pointed me to.
My stance is that such static analyzers should never be trusted 100%. The output
is more an indicator. And in this present case, a quick review made it very
clear that Coccinelle saw one simplification but missed two other ones.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-24 19:21 ` [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation Markus Elfring
@ 2025-06-24 23:03 ` Vincent Mailhol
0 siblings, 0 replies; 16+ messages in thread
From: Vincent Mailhol @ 2025-06-24 23:03 UTC (permalink / raw)
To: Markus Elfring
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde, linux-can
On 25/06/2025 at 04:21, Markus Elfring wrote:
>>> +++ b/drivers/net/can/usb/ucan.c
>>> @@ -1353,16 +1353,14 @@ static int ucan_probe(struct usb_interface *intf,
>>> ep = &iface_desc->endpoint[i].desc;
>>>
>>> if (((ep->bEndpointAddress & USB_ENDPOINT_DIR_MASK) != 0) &&
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This is:
>>
>> usb_endpoint_dir_in(ep)
>
> Can the check for a single value like “USB_DIR_IN” be really mapped to
> an other value range?
> https://elixir.bootlin.com/linux/v6.16-rc3/source/include/uapi/linux/usb/ch9.h#L495-L503
Look at the actual values of USB_ENDPOINT_DIR_MASK and USB_DIR_IN ;)
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-24 22:59 ` Vincent Mailhol
@ 2025-06-25 5:47 ` Markus Elfring
2025-06-25 15:51 ` Vincent Mailhol
0 siblings, 1 reply; 16+ messages in thread
From: Markus Elfring @ 2025-06-25 5:47 UTC (permalink / raw)
To: Vincent Mailhol, linux-can
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde
>>> 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.
> 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?
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 */
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 */
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
Regards,
Markus
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-25 5:47 ` [PATCH] " Markus Elfring
@ 2025-06-25 15:51 ` Vincent Mailhol
2025-06-25 16:23 ` Markus Elfring
0 siblings, 1 reply; 16+ messages in thread
From: Vincent Mailhol @ 2025-06-25 15:51 UTC (permalink / raw)
To: Markus Elfring
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde, linux-can
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-25 15:51 ` Vincent Mailhol
@ 2025-06-25 16:23 ` Markus Elfring
2025-06-26 2:47 ` Vincent Mailhol
0 siblings, 1 reply; 16+ messages in thread
From: Markus Elfring @ 2025-06-25 16:23 UTC (permalink / raw)
To: Vincent Mailhol, linux-can
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde
> 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
Thanks that you pointed another implementation detail out from
the function “imon_find_endpoints”.
> But it does not seem to occur so often either. So not sure what is the best:
> do a manual hunt
Unlikely.
I am unsure if such an aspect would become relevant for a code review
by other contributors.
> or write a coccinelle checker.
I would find it more convenient to achieve corresponding adjustments
to some degree with the help of such a development tool.
I constructed scripts for the semantic patch language accordingly.
>> 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!
I am unsure if the check reordering would be desirable for this function implementation.
Regards,
Markus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-25 16:23 ` Markus Elfring
@ 2025-06-26 2:47 ` Vincent Mailhol
2025-06-26 7:22 ` Markus Elfring
0 siblings, 1 reply; 16+ messages in thread
From: Vincent Mailhol @ 2025-06-26 2:47 UTC (permalink / raw)
To: Markus Elfring
Cc: linux-can, LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde
On Thu. 26 Jun. 2025 at 01:47, Markus Elfring <Markus.Elfring@web.de> wrote:
> > 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
>
> Thanks that you pointed another implementation detail out from
> the function “imon_find_endpoints”.
What I did here was simply to look at all the users of the
USB_ENDPOINT_DIR_MASK macro:
https://elixir.bootlin.com/linux/v6.16-rc3/C/ident/USB_ENDPOINT_DIR_MASK
and bingo, the very first user of that macro is the imon driver with a
true positive. I did not check the other drivers from the list, but
that is what I meant by the manual hunt: I believe that 15 minutes
would be enough to quickly check all those drivers. Of course, doing
it manually is a one time solution whereas adding the coccinelle
script is a long term solution. Also, I am just sharing my thoughts. I
am not trying to discourage you in any way, it is even the opposite:
such initiatives are really nice! Even if I do not participate in
these myself, I want to tell you my gratitude for your efforts!
> > But it does not seem to occur so often either. So not sure what is the best:
> > do a manual hunt
>
> Unlikely.
>
> I am unsure if such an aspect would become relevant for a code review
> by other contributors.
>
>
> > or write a coccinelle checker.
>
> I would find it more convenient to achieve corresponding adjustments
> to some degree with the help of such a development tool.
> I constructed scripts for the semantic patch language accordingly.
>
>
> >> 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!
> I am unsure if the check reordering would be desirable for this function implementation.
Ah, you want to confirm whether
usb_endpoint_dir_in(ep) && usb_endpoint_xfer_bulk(ep)
is the same as
usb_endpoint_xfer_bulk(ep) && usb_endpoint_dir_in(ep)
?
In this case, that is OK. *Mathematically speaking* we have this equivalence:
a & b <=> b & a
In C it is roughly the same except if the expression has some
undefined behaviour. The typical example is:
foo && foo->bar
Here, the short cut evaluation of the && operator will prevent the
undefined behaviour to occur if foo is NULL. And so, obviously,
refactoring as:
foo->bar && foo
would be a bug. In our case, there is no undefined behaviour on the
right hand operand (I mean, if ep is NULL, the undefined behaviour
will already occur on the left hand operand). So we are totally safe
to reorder the operand here.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-26 2:47 ` Vincent Mailhol
@ 2025-06-26 7:22 ` Markus Elfring
2025-06-26 9:23 ` Vincent Mailhol
0 siblings, 1 reply; 16+ messages in thread
From: Markus Elfring @ 2025-06-26 7:22 UTC (permalink / raw)
To: Vincent Mailhol, linux-can
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde
>> I am unsure if the check reordering would be desirable for this function implementation.
>
> Ah, you want to confirm whether
>
> usb_endpoint_dir_in(ep) && usb_endpoint_xfer_bulk(ep)
>
> is the same as
>
> usb_endpoint_xfer_bulk(ep) && usb_endpoint_dir_in(ep)
>
> ?
Exactly, yes.
Commutativity can probably be applied in this case.
But the different execution order will influence the corresponding run time characteristics.
https://en.wikipedia.org/wiki/Short-circuit_evaluation
https://en.wikipedia.org/wiki/Commutative_property
The data processing order from known API function implementations might get priority
also at discussed source code places in the near future.
Regards,
Markus
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: can: ucan: Use usb_endpoint_type() rather than duplicating its implementation
2025-06-26 7:22 ` Markus Elfring
@ 2025-06-26 9:23 ` Vincent Mailhol
2025-06-26 14:46 ` [PATCH v2] can: ucan: Use two USB endpoint API functions rather than duplicating their implementations Markus Elfring
0 siblings, 1 reply; 16+ messages in thread
From: Vincent Mailhol @ 2025-06-26 9:23 UTC (permalink / raw)
To: Markus Elfring, linux-can
Cc: LKML, kernel-janitors, Chen Ni, Marc Kleine-Budde
On 26/06/2025 at 16:22, Markus Elfring wrote:
>>> I am unsure if the check reordering would be desirable for this function implementation.
>>
>> Ah, you want to confirm whether
>>
>> usb_endpoint_dir_in(ep) && usb_endpoint_xfer_bulk(ep)
>>
>> is the same as
>>
>> usb_endpoint_xfer_bulk(ep) && usb_endpoint_dir_in(ep)
>>
>> ?
>
> Exactly, yes.
>
> Commutativity can probably be applied in this case.
> But the different execution order will influence the corresponding run time characteristics.
> https://en.wikipedia.org/wiki/Short-circuit_evaluation
> https://en.wikipedia.org/wiki/Commutative_property
>
> The data processing order from known API function implementations might get priority
> also at discussed source code places in the near future.
Yes. This is what I tried to explain in my previous message: that the short
circuit evaluation may impact the result when there is an undefined behaviour
but that it is not the case here.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] can: ucan: Use two USB endpoint API functions rather than duplicating their implementations
2025-06-26 9:23 ` Vincent Mailhol
@ 2025-06-26 14:46 ` Markus Elfring
2025-06-26 14:50 ` Vincent Mailhol
2025-06-26 15:06 ` Marc Kleine-Budde
0 siblings, 2 replies; 16+ messages in thread
From: Markus Elfring @ 2025-06-26 14:46 UTC (permalink / raw)
To: linux-can, Marc Kleine-Budde, Vincent Mailhol
Cc: LKML, kernel-janitors, Chen Ni
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 26 Jun 2025 16:34:26 +0200
* Reuse existing functionality from usb_endpoint_is_bulk_in()
and usb_endpoint_is_bulk_out() instead of keeping duplicate source code.
* Omit two comment lines which became redundant with this refactoring.
The source code was transformed by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
V2:
Further change possibilities were taken better into account for
the USB endpoint API with the help of Vincent Mailhol.
drivers/net/can/usb/ucan.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index 07406daf7c88..0935a9b540d6 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -1351,19 +1351,11 @@ static int ucan_probe(struct usb_interface *intf,
out_ep_size = 0;
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)) {
- /* In Endpoint */
+ if (usb_endpoint_is_bulk_in(ep)) {
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)) {
- /* Out Endpoint */
+ } else if (usb_endpoint_is_bulk_out(ep)) {
out_ep_addr = ep->bEndpointAddress;
out_ep_addr &= USB_ENDPOINT_NUMBER_MASK;
out_ep_size = le16_to_cpu(ep->wMaxPacketSize);
--
2.50.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] can: ucan: Use two USB endpoint API functions rather than duplicating their implementations
2025-06-26 14:46 ` [PATCH v2] can: ucan: Use two USB endpoint API functions rather than duplicating their implementations Markus Elfring
@ 2025-06-26 14:50 ` Vincent Mailhol
2025-06-26 15:06 ` Marc Kleine-Budde
1 sibling, 0 replies; 16+ messages in thread
From: Vincent Mailhol @ 2025-06-26 14:50 UTC (permalink / raw)
To: Markus Elfring
Cc: LKML, kernel-janitors, Chen Ni, linux-can, Marc Kleine-Budde
Hi Markus,
Thanks for the v2.
On 26/06/2025 at 23:46, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 26 Jun 2025 16:34:26 +0200
>
> * Reuse existing functionality from usb_endpoint_is_bulk_in()
> and usb_endpoint_is_bulk_out() instead of keeping duplicate source code.
>
> * Omit two comment lines which became redundant with this refactoring.
>
> The source code was transformed by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] can: ucan: Use two USB endpoint API functions rather than duplicating their implementations
2025-06-26 14:46 ` [PATCH v2] can: ucan: Use two USB endpoint API functions rather than duplicating their implementations Markus Elfring
2025-06-26 14:50 ` Vincent Mailhol
@ 2025-06-26 15:06 ` Marc Kleine-Budde
2025-06-26 15:17 ` [v2] " Markus Elfring
1 sibling, 1 reply; 16+ messages in thread
From: Marc Kleine-Budde @ 2025-06-26 15:06 UTC (permalink / raw)
To: Markus Elfring; +Cc: linux-can, Vincent Mailhol, LKML, kernel-janitors, Chen Ni
[-- Attachment #1: Type: text/plain, Size: 888 bytes --]
On 26.06.2025 16:46:32, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 26 Jun 2025 16:34:26 +0200
>
> * Reuse existing functionality from usb_endpoint_is_bulk_in()
> and usb_endpoint_is_bulk_out() instead of keeping duplicate source code.
>
> * Omit two comment lines which became redundant with this refactoring.
>
> The source code was transformed by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
Sorry, this patch is not Signed-off-by its sender.
Please don't use existing threads for vN+1.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v2] can: ucan: Use two USB endpoint API functions rather than duplicating their implementations
2025-06-26 15:06 ` Marc Kleine-Budde
@ 2025-06-26 15:17 ` Markus Elfring
0 siblings, 0 replies; 16+ messages in thread
From: Markus Elfring @ 2025-06-26 15:17 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can
Cc: LKML, kernel-janitors, Chen Ni, Vincent Mailhol
>> From: Markus Elfring <elfring@users.sourceforge.net>
…
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>
> Sorry, this patch is not Signed-off-by its sender.
I find my Developer's Certificate of Origin appropriate here.
Regards,
Markus
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2025-06-26 15:18 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-24 17:12 [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation Markus Elfring
2025-06-24 17:29 ` Vincent Mailhol
2025-06-24 18:28 ` Markus Elfring
2025-06-24 22:59 ` Vincent Mailhol
2025-06-25 5:47 ` [PATCH] " Markus Elfring
2025-06-25 15:51 ` Vincent Mailhol
2025-06-25 16:23 ` Markus Elfring
2025-06-26 2:47 ` Vincent Mailhol
2025-06-26 7:22 ` Markus Elfring
2025-06-26 9:23 ` Vincent Mailhol
2025-06-26 14:46 ` [PATCH v2] can: ucan: Use two USB endpoint API functions rather than duplicating their implementations Markus Elfring
2025-06-26 14:50 ` Vincent Mailhol
2025-06-26 15:06 ` Marc Kleine-Budde
2025-06-26 15:17 ` [v2] " Markus Elfring
2025-06-24 19:21 ` [PATCH] can: ucan: Use usb_endpoint_type() rather than duplicating its implementation Markus Elfring
2025-06-24 23:03 ` Vincent Mailhol
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).