On Sat. 14 Sep. 2024 at 03:57, 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. > > 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> > --- > drivers/net/can/usb/gs_usb.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c > index bc86e9b329fd..f3eb447267f9 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; > + > + u8 ep_in; > + u8 ep_out; 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). > }; > > /* '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), > + usb_rcvbulkpipe(parent->udev, parent->ep_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), > + usb_sndbulkpipe(dev->udev, dev->parent->ep_out), > hf, dev->hf_size_tx, > gs_usb_xmit_callback, txc); > > @@ -926,7 +926,7 @@ static int gs_can_open(struct net_device *netdev) > usb_fill_bulk_urb(urb, > dev->udev, > usb_rcvbulkpipe(dev->udev, > - GS_USB_ENDPOINT_IN), > + dev->parent->ep_in), > buf, > dev->parent->hf_size_rx, > gs_usb_receive_bulk_callback, parent); > @@ -1421,6 +1421,18 @@ static int gs_usb_probe(struct usb_interface *intf, > struct gs_device_config dconf; > unsigned int icount, i; > int rc; > + struct usb_host_interface *host_iface; > + struct usb_endpoint_descriptor *ep_in, *ep_out; Move this declaration up (c.f. the Reverse christmas tree declarations). > + > + host_iface = intf->cur_altsetting; > + > + /* Find common bulk endpoints reverse */ 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. > + rc = usb_find_common_endpoints_reverse(host_iface, &ep_in, &ep_out, NULL, > + NULL); Nipick: no need to declare a new variable for host_iface which is used only once. Just do: rc = usb_find_common_endpoints_reverse(intf->cur_altsetting, &ep_in, &ep_out, NULL, NULL); and remove this host_iface declaration. > + if (rc) { > + dev_err(&intf->dev, "Required endpoints not found\n"); > + return rc; > + } > > /* send host config */ > rc = usb_control_msg_send(udev, 0, > @@ -1466,6 +1478,10 @@ static int gs_usb_probe(struct usb_interface *intf, > usb_set_intfdata(intf, parent); > parent->udev = udev; Maybe have the usb_find_common_endpoints() next to this? There is no need to do the endpoint search earlier, so better to group this which are related together. > + /* store the detected endpoints */ > + parent->ep_in = ep_in->bEndpointAddress; > + parent->ep_out = ep_out->bEndpointAddress; > + > for (i = 0; i < icount; i++) { > unsigned int hf_size_rx = 0; > > -- > 2.43.0 > >
