Message-ID: <20241008201107.38520-1-ak.alexander.kozhinov@gmail.com>
Date: Tue,  8 Oct 2024 22:09:13 +0200
From: Alexander Kozhinov <ak.alexander.kozhinov@...il.com>
To: linux-kernel@...r.kernel.org
Cc: Alexander Kozhinov <ak.alexander.kozhinov@...il.com>,
	Marc Kleine-Budde <mkl@...gutronix.de>,
	Vincent Mailhol <mailhol.vincent@...adoo.fr>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Maximilian Schneider <max@...neidersoft.net>
Subject: [PATCH v3 1/5] can: gs_usb.c: add usb endpoint address detection at driver probe step

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@...gutronix.de>
Cc: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: David S. Miller <davem@...emloft.net>
Cc: Eric Dumazet <edumazet@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>
Cc: Paolo Abeni <pabeni@...hat.com>
Cc: Maximilian Schneider <max@...neidersoft.net>
Signed-off-by: Alexander Kozhinov <ak.alexander.kozhinov@...il.com>
---
 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@...il.com>
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@...il.com>
---
 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@...il.com>
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@...il.com>
---
 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@...il.com>
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@...il.com>
---
 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@...il.com>
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@...il.com>
---
 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

Powered by blists - more mailing lists