Re: [PATCH v3 1/5] can: gs_usb.c: add usb endpoint address detection at driver probe step

From: Vincent MAILHOL
Date: Wed Oct 09 2024 - 04:11:55 EST

Next message: Jani Nikula: "Re: [PATCH drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc drivers"
Previous message: claudiu beznea: "Re: [PATCH] pinctrl: renesas: rzg2l: Always call rzg2l_gpio_request() for interrupt pins"
In reply to: Alexander Kozhinov: "[PATCH v3 1/5] can: gs_usb.c: add usb endpoint address detection at driver probe step"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Alexander,

On Wed. 9 Oct. 2024 at 05:09, 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.

Thanks for this v3, the code looks good but your patch series has several issue:

- you did not put in CC the most relevant mailing list:
linux-can@xxxxxxxxxxxxxxx (you only added the generic
linux-kernel@xxxxxxxxxxxxxxx address)

- you sent a single message with five patches. Each patch should be
sent in a different message (but in the same thread).
git send-email, if used correctly, should manage this for you.

- when you address comments, you should directly add those to the
initial patch. The idea is that you should split your work in
different logical changes. Here, there is only one logical change:
replace hardcoded endpoint by automatic detection and so, there
should be only one patch.

- Instead, you should add a quick changelog at the end of the
patch. Typically, add a "---" (the cutter) after your
Signed-off-by tag and put the changelog after this
cutter. Everything after the cutter will be discarded when the
maintainers pick-up your patch.

For extra details, refer to this guide:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> 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;
> };
>
> /* '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;
> +
> + host_iface = intf->cur_altsetting;
> +
> + /* Find common bulk endpoints reverse */
> + rc = usb_find_common_endpoints_reverse(host_iface, &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,
> @@ -1466,6 +1478,10 @@ static int gs_usb_probe(struct usb_interface *intf,
> usb_set_intfdata(intf, parent);
> parent->udev = udev;
>
> + /* 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
>
>
> From d0b4d3b1f5fd1b4c33bc5d8f83ed49d04a2286c4 Mon Sep 17 00:00:00 2001
> From: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx>
> Date: Tue, 8 Oct 2024 21:25:51 +0200
> Subject: [PATCH v3 2/5] can: gs_usb.c: 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).
>
> Signed-off-by: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx>
> ---
> drivers/net/can/usb/gs_usb.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index f3eb447267f9..e6511af5f692 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -334,8 +334,8 @@ struct gs_usb {
> unsigned int hf_size_rx;
> u8 active_channels;
>
> - u8 ep_in;
> - u8 ep_out;
> + unsigned int pipe_in;
> + unsigned int pipe_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, parent->ep_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, dev->parent->ep_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,
> - dev->parent->ep_in),
> + dev->parent->pipe_in,
> buf,
> dev->parent->hf_size_rx,
> gs_usb_receive_bulk_callback, parent);
> @@ -1479,8 +1478,8 @@ static int gs_usb_probe(struct usb_interface *intf,
> parent->udev = udev;
>
> /* store the detected endpoints */
> - parent->ep_in = ep_in->bEndpointAddress;
> - parent->ep_out = ep_out->bEndpointAddress;
> + 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;
> --
> 2.43.0
>
>
> From 6515df86641c6eda6dc3e3c4a8f1fc8625835f89 Mon Sep 17 00:00:00 2001
> From: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx>
> Date: Tue, 8 Oct 2024 21:37:13 +0200
> Subject: [PATCH v3 3/5] can: gs_usb.c: use reverse xmas tree declaration
>
> This change implements request: Move this declaration up (c.f. the Reverse christmas tree declarations).
>
> Signed-off-by: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx>
> ---
> drivers/net/can/usb/gs_usb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index e6511af5f692..a88448dbc6af 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -1412,6 +1412,8 @@ 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 usb_host_interface *host_iface;
> struct gs_host_frame *hf;
> struct gs_usb *parent;
> struct gs_host_config hconf = {
> @@ -1420,8 +1422,6 @@ 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;
>
> host_iface = intf->cur_altsetting;
>
> --
> 2.43.0
>
>
> From cddd8290dd877eddcac31366ae69188b79fc35e8 Mon Sep 17 00:00:00 2001
> From: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx>
> Date: Tue, 8 Oct 2024 21:40:59 +0200
> Subject: [PATCH v3 4/5] can: gs_usb.c: use rforward 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.
>
> Signed-off-by: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx>
> ---
> drivers/net/can/usb/gs_usb.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index a88448dbc6af..24f715f9c436 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -1426,8 +1426,7 @@ static int gs_usb_probe(struct usb_interface *intf,
> host_iface = intf->cur_altsetting;
>
> /* Find common bulk endpoints reverse */
> - rc = usb_find_common_endpoints_reverse(host_iface, &ep_in, &ep_out, NULL,
> - NULL);
> + rc = usb_find_common_endpoints(host_iface, &ep_in, &ep_out, NULL, NULL);
> if (rc) {
> dev_err(&intf->dev, "Required endpoints not found\n");
> return rc;
> --
> 2.43.0
>
>
> From b65448554e172c8b419605411248d83846764a1d Mon Sep 17 00:00:00 2001
> From: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx>
> Date: Tue, 8 Oct 2024 21:43:10 +0200
> Subject: [PATCH v3 5/5] can: gs_usb.c: skip new variable declaration
>
> This change implements nipick: no need to declare a new variable for host_iface which is used
> only once.
>
> Signed-off-by: Alexander Kozhinov <ak.alexander.kozhinov@xxxxxxxxx>
> ---
> drivers/net/can/usb/gs_usb.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 24f715f9c436..d93410682d4b 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -1413,7 +1413,6 @@ static int gs_usb_probe(struct usb_interface *intf,
> {
> struct usb_device *udev = interface_to_usbdev(intf);
> struct usb_endpoint_descriptor *ep_in, *ep_out;
> - struct usb_host_interface *host_iface;
> struct gs_host_frame *hf;
> struct gs_usb *parent;
> struct gs_host_config hconf = {
> @@ -1423,10 +1422,9 @@ static int gs_usb_probe(struct usb_interface *intf,
> unsigned int icount, i;
> int rc;
>
> - host_iface = intf->cur_altsetting;
> -
> /* Find common bulk endpoints reverse */
> - rc = usb_find_common_endpoints(host_iface, &ep_in, &ep_out, NULL, NULL);
> + 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;
> --
> 2.43.0
>

Next message: Jani Nikula: "Re: [PATCH drm-dp 1/4] drm/hisilicon/hibmc: add dp aux in hibmc drivers"
Previous message: claudiu beznea: "Re: [PATCH] pinctrl: renesas: rzg2l: Always call rzg2l_gpio_request() for interrupt pins"
In reply to: Alexander Kozhinov: "[PATCH v3 1/5] can: gs_usb.c: add usb endpoint address detection at driver probe step"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]