Hi Alexander, Thank you for this v4, the patch is overall good (there are two minor comments left to address). With this, I think you are done with your on boarding on understanding the patch submission process. Congrats for your first patch on the mailing list! On Sat. 12 Oct. 2024 at 20:25, Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx> wrote: > > There is an approach made to implement gs_usb firmware/driver based on > Zephyr RTOS. It was found that USB stack of Zephyr RTOS overwrites USB > EP addresses, if they have different last 4 bytes in absence of other > endpoints. > > For example in case of gs_usb candlelight firmware EP-IN is 0x81 and > EP-OUT 0x02. If there are no additional USB endpoints, Zephyr RTOS will > overwrite EP-OUT to 0x01. More information can be found in the > discussion with Zephyr RTOS USB stack maintainer here: > > https://github.com/zephyrproject-rtos/zephyr/issues/67812 > > There are already two different gs_usb FW driver implementations based > on Zephyr RTOS: > > 1. https://github.com/CANnectivity/cannectivity > (by: https://github.com/henrikbrixandersen) > 2. https://github.com/zephyrproject-rtos/zephyr/compare/main...KozhinovAlexander:zephyr:gs_usb > (by: https://github.com/KozhinovAlexander) > > At the moment both Zephyr RTOS implementations use dummy USB endpoint, > to overcome described USB stack behavior from Zephyr itself. Since > Zephyr RTOS is intended to be used on microcontrollers with very > constrained amount of resources (ROM, RAM) and additional endpoint > requires memory, it is more convenient to update the gs_usb driver in > the Linux kernel. > > To fix this problem, update the gs_usb driver from using hard coded > endpoint numbers to evaluate the endpoint descriptors and use the > endpoints provided there. If I understand correctly, this patch addresses a current issue. In that case, it is good to add a Fixes: tag to inform the maintainers of the stable branches how to back-port it. Additional info: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes For this patch, the code you are fixing is present since the introduction of the gs-usb module, so you can add this tag: Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices") This way, the users of older kernel releases will also benefit from your patch! > To: linux-can@xxxxxxxxxxxxxxx > Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > Cc: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Cc: David S. Miller <davem@xxxxxxxxxxxxx> > Cc: Eric Dumazet <edumazet@xxxxxxxxxx> > Cc: Jakub Kicinski <kuba@xxxxxxxxxx> > Cc: Paolo Abeni <pabeni@xxxxxxxxxx> > Cc: Maximilian Schneider <max@xxxxxxxxxxxxxxxxx> > Signed-off-by: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx> Please address the last two comments (c.f. below), but meanwhile, you can directly add my review tag to the v5: Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > --- > Changes in v3: > - store pipe instead of endpoint address. > This change implements nitpick: > Instead of storing the ep_in and ep_out in your priv, you can > instead directly store the result of usb_rcvbulkpipe(parent->udev, > parent->ep_in) and usb_sndbulkpipe(dev->udev, dev->parent->ep_out). > - use reverse xmas tree declaration. > This change implements request: > Move this declaration up (c.f. the Reverse christmas tree declarations). > - use forward usb endpoints search. > This change implements request: > Any specific reason for doing this in reverse? The previous > GS_USB_ENDPOINT_IN and GS_USB_ENDPOINT_OUT macros were respectively 1 > and 2, so at the beginning. And in such a case, the normal search > would find those quicker. > - skip new variable declaration. > This change implements nipick: > no need to declare a new variable for host_iface which is used only once. > > Changes in v4: > - put in CC most relevant mailing ist linux-can@xxxxxxxxxxxxxxx > - use one patch only since only one logical/contextual change were introduced. > - add quick changelog over multiple pathc versions. > --- > drivers/net/can/usb/gs_usb.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c > index bc86e9b329fd..d93410682d4b 100644 > --- a/drivers/net/can/usb/gs_usb.c > +++ b/drivers/net/can/usb/gs_usb.c > @@ -43,9 +43,6 @@ > #define USB_XYLANTA_SAINT3_VENDOR_ID 0x16d0 > #define USB_XYLANTA_SAINT3_PRODUCT_ID 0x0f30 > > -#define GS_USB_ENDPOINT_IN 1 > -#define GS_USB_ENDPOINT_OUT 2 > - > /* Timestamp 32 bit timer runs at 1 MHz (1 µs tick). Worker accounts > * for timer overflow (will be after ~71 minutes) > */ > @@ -336,6 +333,9 @@ struct gs_usb { > > unsigned int hf_size_rx; > u8 active_channels; > + > + unsigned int pipe_in; > + unsigned int pipe_out; > }; Nitpick: by putting some integers after a u8, you are creating a hole for 3 bytes between gs_usb->active_channels and gs_usb->pipe_in, as shown by the pahole tool: $ pahole --class_name=gs_usb drivers/net/can/usb/gs_usb.o struct gs_usb { struct gs_can * canch[3]; /* 0 24 */ struct usb_anchor rx_submitted; /* 24 56 */ /* XXX last struct has 4 bytes of padding, 31 bits of padding */ /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */ struct usb_device * udev; /* 80 8 */ struct cyclecounter cc; /* 88 24 */ struct timecounter tc; /* 112 40 */ /* --- cacheline 2 boundary (128 bytes) was 24 bytes ago --- */ spinlock_t tc_lock; /* 152 4 */ /* XXX 4 bytes hole, try to pack */ struct delayed_work timestamp; /* 160 88 */ /* XXX last struct has 4 bytes of padding */ /* --- cacheline 3 boundary (192 bytes) was 56 bytes ago --- */ unsigned int hf_size_rx; /* 248 4 */ u8 active_channels; /* 252 1 */ /* XXX 3 bytes hole, try to pack */ /* --- cacheline 4 boundary (256 bytes) --- */ unsigned int pipe_in; /* 256 4 */ unsigned int pipe_out; /* 260 4 */ /* size: 264, cachelines: 5, members: 11 */ /* sum members: 257, holes: 2, sum holes: 7 */ /* member types with bit paddings: 1, total: 31 bits */ /* paddings: 2, sum paddings: 8 */ /* last cacheline: 8 bytes */ }; This 3 bytes hole will always exist even if you group the integers together (because the padding also occurs at the end of the structure) *but*, it is easier to maintain if the hole is at the end. So, in summary, do it like this: unsigned int hf_size_rx; + unsigned int pipe_in; + unsigned int pipe_out; u8 active_channels; }; Note that there is also 2 other 4 bytes holes, so a packing optimization could be done here, but this is out of scope of your patch. If you want to address that, it must go in a different patch because, this time, it is another "logical change". > /* 'allocate' a tx context. > @@ -687,7 +687,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb) > > resubmit_urb: > usb_fill_bulk_urb(urb, parent->udev, > - usb_rcvbulkpipe(parent->udev, GS_USB_ENDPOINT_IN), > + parent->pipe_in, > hf, dev->parent->hf_size_rx, > gs_usb_receive_bulk_callback, parent); > > @@ -819,7 +819,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, > } > > usb_fill_bulk_urb(urb, dev->udev, > - usb_sndbulkpipe(dev->udev, GS_USB_ENDPOINT_OUT), > + dev->parent->pipe_out, > hf, dev->hf_size_tx, > gs_usb_xmit_callback, txc); > > @@ -925,8 +925,7 @@ static int gs_can_open(struct net_device *netdev) > /* fill, anchor, and submit rx urb */ > usb_fill_bulk_urb(urb, > dev->udev, > - usb_rcvbulkpipe(dev->udev, > - GS_USB_ENDPOINT_IN), > + dev->parent->pipe_in, > buf, > dev->parent->hf_size_rx, > gs_usb_receive_bulk_callback, parent); > @@ -1413,6 +1412,7 @@ static int gs_usb_probe(struct usb_interface *intf, > const struct usb_device_id *id) > { > struct usb_device *udev = interface_to_usbdev(intf); > + struct usb_endpoint_descriptor *ep_in, *ep_out; > struct gs_host_frame *hf; > struct gs_usb *parent; > struct gs_host_config hconf = { > @@ -1422,6 +1422,14 @@ static int gs_usb_probe(struct usb_interface *intf, > unsigned int icount, i; > int rc; > > + /* Find common bulk endpoints reverse */ ^^^^^^^ You forget to update the comment. Also, about the comments in general, have a look at this: https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting Here, your comment: /* Find common bulk endpoints */ paraphrases the function name: usb_find_common_endpoints(). So it does not add any value. It is better to simply remove that comment and let the code speak for itself. > + rc = usb_find_common_endpoints(intf->cur_altsetting, > + &ep_in, &ep_out, NULL, NULL); > + if (rc) { > + dev_err(&intf->dev, "Required endpoints not found\n"); > + return rc; > + } > + > /* send host config */ > rc = usb_control_msg_send(udev, 0, > GS_USB_BREQ_HOST_FORMAT, > @@ -1466,6 +1474,10 @@ static int gs_usb_probe(struct usb_interface *intf, > usb_set_intfdata(intf, parent); > parent->udev = udev; > > + /* store the detected endpoints */ > + parent->pipe_in = usb_rcvbulkpipe(parent->udev, ep_in->bEndpointAddress); > + parent->pipe_out = usb_sndbulkpipe(parent->udev, ep_out->bEndpointAddress); > + > for (i = 0; i < icount; i++) { > unsigned int hf_size_rx = 0; Yours sincerely, Vincent Mailhol
