[PATCH net 0/3] pull-request: can 2022-09-21

[PATCH net 0/3] pull-request: can 2022-09-21

[Marc Kleine-Budde]

Hello Jakub, hello David,

until the mess with yesterdays pull request is sorted out, here's a
pull request with the remaining 3 patches for net/master.

The 1st patch is by me, targets the flexcan driver and fixes a
potential system hang on single core systems under high CAN packet
rate.

The next 2 patches are also by me and target the gs_usb driver. A
potential race condition during the ndo_open callback as well as the
return value if the ethtool identify feature is not supported are
fixed.

regards,
Marc

---

The following changes since commit 6a1dbfefdae4f7809b3e277cc76785dac0ac1cd0:

  net: sh_eth: Fix PHY state warning splat during system resume (2022-09-20 17:05:50 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-6.0-20220921

for you to fetch changes up to 0f2211f1cf58876f00d2fd8839e0fdadf0786894:

  can: gs_usb: gs_usb_set_phys_id(): return with error if identify is not supported (2022-09-21 09:48:52 +0200)

----------------------------------------------------------------
linux-can-fixes-for-6.0-20220921

----------------------------------------------------------------
Marc Kleine-Budde (3):
      can: flexcan: flexcan_mailbox_read() fix return value for drop = true
      can: gs_usb: gs_can_open(): fix race dev->can.state condition
      can: gs_usb: gs_usb_set_phys_id(): return with error if identify is not supported

 drivers/net/can/flexcan/flexcan-core.c | 10 +++++-----
 drivers/net/can/usb/gs_usb.c           | 21 +++++++++++++--------
 2 files changed, 18 insertions(+), 13 deletions(-)

[PATCH net 1/3] can: flexcan: flexcan_mailbox_read() fix return value for drop = true

[Marc Kleine-Budde]

The following happened on an i.MX25 using flexcan with many packets on
the bus:

The rx-offload queue reached a length more than skb_queue_len_max. In
can_rx_offload_offload_one() the drop variable was set to true which
made the call to .mailbox_read() (here: flexcan_mailbox_read()) to
_always_ return ERR_PTR(-ENOBUFS) and drop the rx'ed CAN frame. So
can_rx_offload_offload_one() returned ERR_PTR(-ENOBUFS), too.

can_rx_offload_irq_offload_fifo() looks as follows:

| 	while (1) {
| 		skb = can_rx_offload_offload_one(offload, 0);
| 		if (IS_ERR(skb))
| 			continue;
| 		if (!skb)
| 			break;
| 		...
| 	}

The flexcan driver wrongly always returns ERR_PTR(-ENOBUFS) if drop is
requested, even if there is no CAN frame pending. As the i.MX25 is a
single core CPU, while the rx-offload processing is active, there is
no thread to process packets from the offload queue. So the queue
doesn't get any shorter and this results is a tight loop.

Instead of always returning ERR_PTR(-ENOBUFS) if drop is requested,
return NULL if no CAN frame is pending.

Changes since v1: https://lore.kernel.org/all/20220810144536.389237-1-u.kleine-koenig@pengutronix.de
- don't break in can_rx_offload_irq_offload_fifo() in case of an error,
  return NULL in flexcan_mailbox_read() in case of no pending CAN frame
  instead

Fixes: 4e9c9484b085 ("can: rx-offload: Prepare for CAN FD support")
Link: https://lore.kernel.org/all/20220811094254.1864367-1-mkl@pengutronix.de
Cc: stable@vger.kernel.org # v5.5
Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Tested-by: Thorsten Scherer <t.scherer@eckelmann.de>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/flexcan/flexcan-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
index f857968efed7..ccb438eca517 100644
--- a/drivers/net/can/flexcan/flexcan-core.c
+++ b/drivers/net/can/flexcan/flexcan-core.c
@@ -941,11 +941,6 @@ static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
 	u32 reg_ctrl, reg_id, reg_iflag1;
 	int i;
 
-	if (unlikely(drop)) {
-		skb = ERR_PTR(-ENOBUFS);
-		goto mark_as_read;
-	}
-
 	mb = flexcan_get_mb(priv, n);
 
 	if (priv->devtype_data.quirks & FLEXCAN_QUIRK_USE_RX_MAILBOX) {
@@ -974,6 +969,11 @@ static struct sk_buff *flexcan_mailbox_read(struct can_rx_offload *offload,
 		reg_ctrl = priv->read(&mb->can_ctrl);
 	}
 
+	if (unlikely(drop)) {
+		skb = ERR_PTR(-ENOBUFS);
+		goto mark_as_read;
+	}
+
 	if (reg_ctrl & FLEXCAN_MB_CNT_EDL)
 		skb = alloc_canfd_skb(offload->dev, &cfd);
 	else

base-commit: 6a1dbfefdae4f7809b3e277cc76785dac0ac1cd0
-- 
2.35.1

[PATCH net 2/3] can: gs_usb: gs_can_open(): fix race dev->can.state condition

[Marc Kleine-Budde]

The dev->can.state is set to CAN_STATE_ERROR_ACTIVE, after the device
has been started. On busy networks the CAN controller might receive
CAN frame between and go into an error state before the dev->can.state
is assigned.

Assign dev->can.state before starting the controller to close the race
window.

Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Link: https://lore.kernel.org/all/20220920195216.232481-1-mkl@pengutronix.de
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 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 baf749c8cda3..553e2c7b0b12 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -824,6 +824,7 @@ static int gs_can_open(struct net_device *netdev)
 		flags |= GS_CAN_MODE_TRIPLE_SAMPLE;
 
 	/* finally start device */
+	dev->can.state = CAN_STATE_ERROR_ACTIVE;
 	dm->mode = cpu_to_le32(GS_CAN_MODE_START);
 	dm->flags = cpu_to_le32(flags);
 	rc = usb_control_msg(interface_to_usbdev(dev->iface),
@@ -835,13 +836,12 @@ static int gs_can_open(struct net_device *netdev)
 	if (rc < 0) {
 		netdev_err(netdev, "Couldn't start device (err=%d)\n", rc);
 		kfree(dm);
+		dev->can.state = CAN_STATE_STOPPED;
 		return rc;
 	}
 
 	kfree(dm);
 
-	dev->can.state = CAN_STATE_ERROR_ACTIVE;
-
 	parent->active_channels++;
 	if (!(dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY))
 		netif_start_queue(netdev);
-- 
2.35.1

[PATCH net 3/3] can: gs_usb: gs_usb_set_phys_id(): return with error if identify is not supported

[Marc Kleine-Budde]

Until commit 409c188c57cd ("can: tree-wide: advertise software
timestamping capabilities") the ethtool_ops was only assigned for
devices which support the GS_CAN_FEATURE_IDENTIFY feature. That commit
assigns ethtool_ops unconditionally.

This results on controllers without GS_CAN_FEATURE_IDENTIFY support
for the following ethtool error:

| $ ethtool -p can0 1
| Cannot identify NIC: Broken pipe

Restore the correct error value by checking for
GS_CAN_FEATURE_IDENTIFY in the gs_usb_set_phys_id() function.

| $ ethtool -p can0 1
| Cannot identify NIC: Operation not supported

While there use the variable "netdev" for the "struct net_device"
pointer and "dev" for the "struct gs_can" pointer as in the rest of
the driver.

Fixes: 409c188c57cd ("can: tree-wide: advertise software timestamping capabilities")
Link: http://lore.kernel.org/all/20220818143853.2671854-1-mkl@pengutronix.de
Cc: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 553e2c7b0b12..c1ff3c046d62 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -925,17 +925,21 @@ static int gs_usb_set_identify(struct net_device *netdev, bool do_identify)
 }
 
 /* blink LED's for finding the this interface */
-static int gs_usb_set_phys_id(struct net_device *dev,
+static int gs_usb_set_phys_id(struct net_device *netdev,
 			      enum ethtool_phys_id_state state)
 {
+	const struct gs_can *dev = netdev_priv(netdev);
 	int rc = 0;
 
+	if (!(dev->feature & GS_CAN_FEATURE_IDENTIFY))
+		return -EOPNOTSUPP;
+
 	switch (state) {
 	case ETHTOOL_ID_ACTIVE:
-		rc = gs_usb_set_identify(dev, GS_CAN_IDENTIFY_ON);
+		rc = gs_usb_set_identify(netdev, GS_CAN_IDENTIFY_ON);
 		break;
 	case ETHTOOL_ID_INACTIVE:
-		rc = gs_usb_set_identify(dev, GS_CAN_IDENTIFY_OFF);
+		rc = gs_usb_set_identify(netdev, GS_CAN_IDENTIFY_OFF);
 		break;
 	default:
 		break;
@@ -1072,9 +1076,10 @@ static struct gs_can *gs_make_candev(unsigned int channel,
 		dev->feature |= GS_CAN_FEATURE_REQ_USB_QUIRK_LPC546XX |
 			GS_CAN_FEATURE_QUIRK_BREQ_CANTACT_PRO;
 
-	if (le32_to_cpu(dconf->sw_version) > 1)
-		if (feature & GS_CAN_FEATURE_IDENTIFY)
-			netdev->ethtool_ops = &gs_usb_ethtool_ops;
+	/* GS_CAN_FEATURE_IDENTIFY is only supported for sw_version > 1 */
+	if (!(le32_to_cpu(dconf->sw_version) > 1 &&
+	      feature & GS_CAN_FEATURE_IDENTIFY))
+		dev->feature &= ~GS_CAN_FEATURE_IDENTIFY;
 
 	kfree(bt_const);
 
-- 
2.35.1

Re: [PATCH net 1/3] can: flexcan: flexcan_mailbox_read() fix return value for drop = true

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Marc Kleine-Budde <mkl@pengutronix.de>:

On Wed, 21 Sep 2022 10:36:07 +0200 you wrote:
> The following happened on an i.MX25 using flexcan with many packets on
> the bus:
> 
> The rx-offload queue reached a length more than skb_queue_len_max. In
> can_rx_offload_offload_one() the drop variable was set to true which
> made the call to .mailbox_read() (here: flexcan_mailbox_read()) to
> _always_ return ERR_PTR(-ENOBUFS) and drop the rx'ed CAN frame. So
> can_rx_offload_offload_one() returned ERR_PTR(-ENOBUFS), too.
> 
> [...]

Here is the summary with links:
  - [net,1/3] can: flexcan: flexcan_mailbox_read() fix return value for drop = true
    https://git.kernel.org/netdev/net/c/a09721dd47c8
  - [net,2/3] can: gs_usb: gs_can_open(): fix race dev->can.state condition
    https://git.kernel.org/netdev/net/c/5440428b3da6
  - [net,3/3] can: gs_usb: gs_usb_set_phys_id(): return with error if identify is not supported
    https://git.kernel.org/netdev/net/c/0f2211f1cf58

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 2/3] can: gs_usb: gs_can_open(): fix race dev->can.state condition

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]

Hello,

On 21/09/2022 10:36, Marc Kleine-Budde wrote:
> The dev->can.state is set to CAN_STATE_ERROR_ACTIVE, after the device
> has been started. On busy networks the CAN controller might receive
> CAN frame between and go into an error state before the dev->can.state
> is assigned.
> 
> Assign dev->can.state before starting the controller to close the race
> window.
> 
> Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
> Link: https://lore.kernel.org/all/20220920195216.232481-1-mkl@pengutronix.de
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

FYI, we got a small conflict when merging -net in net-next in the MPTCP
tree due to this patch applied in -net:

  5440428b3da6 ("can: gs_usb: gs_can_open(): fix race dev->can.state
condition")

and this one from net-next:

  45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")

The conflict has been resolved on our side[1] and the resolution we
suggest is attached to this email.

Cheers,
Matt

[1] https://github.com/multipath-tcp/mptcp_net-next/commit/671f1521b564
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[-- Attachment #2: 671f1521b5648e99fb55bbd0f114e4433551d411.patch --]
[-- Type: text/x-patch, Size: 1001 bytes --]

diff --cc drivers/net/can/usb/gs_usb.c
index cc363f1935ce,c1ff3c046d62..5e0d280b0cd3
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@@ -956,11 -823,8 +956,12 @@@ static int gs_can_open(struct net_devic
  	if (ctrlmode & CAN_CTRLMODE_3_SAMPLES)
  		flags |= GS_CAN_MODE_TRIPLE_SAMPLE;
  
 +	/* if hardware supports timestamps, enable it */
 +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
 +		flags |= GS_CAN_MODE_HW_TIMESTAMP;
 +
  	/* finally start device */
+ 	dev->can.state = CAN_STATE_ERROR_ACTIVE;
  	dm->mode = cpu_to_le32(GS_CAN_MODE_START);
  	dm->flags = cpu_to_le32(flags);
  	rc = usb_control_msg(interface_to_usbdev(dev->iface),
@@@ -977,12 -842,6 +979,10 @@@
  
  	kfree(dm);
  
 +	/* start polling timestamp */
 +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
 +		gs_usb_timestamp_init(dev);
 +
- 	dev->can.state = CAN_STATE_ERROR_ACTIVE;
- 
  	parent->active_channels++;
  	if (!(dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY))
  		netif_start_queue(netdev);

Re: [PATCH net 2/3] can: gs_usb: gs_can_open(): fix race dev->can.state condition

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On 22.09.2022 10:04:55, Matthieu Baerts wrote:
> On 21/09/2022 10:36, Marc Kleine-Budde wrote:
> > The dev->can.state is set to CAN_STATE_ERROR_ACTIVE, after the device
> > has been started. On busy networks the CAN controller might receive
> > CAN frame between and go into an error state before the dev->can.state
> > is assigned.
> > 
> > Assign dev->can.state before starting the controller to close the race
> > window.
> > 
> > Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
> > Link: https://lore.kernel.org/all/20220920195216.232481-1-mkl@pengutronix.de
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> 
> FYI, we got a small conflict when merging -net in net-next in the MPTCP
> tree due to this patch applied in -net:
> 
>   5440428b3da6 ("can: gs_usb: gs_can_open(): fix race dev->can.state
> condition")
> 
> and this one from net-next:
> 
>   45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
> 
> The conflict has been resolved on our side[1] and the resolution we
> suggest is attached to this email.

That patch looks good to me.

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net 2/3] can: gs_usb: gs_can_open(): fix race dev->can.state condition

[Jakub Kicinski]

On Thu, 22 Sep 2022 10:23:38 +0200 Marc Kleine-Budde wrote:
> On 22.09.2022 10:04:55, Matthieu Baerts wrote:
> > FYI, we got a small conflict when merging -net in net-next in the MPTCP
> > tree due to this patch applied in -net:
> > 
> >   5440428b3da6 ("can: gs_usb: gs_can_open(): fix race dev->can.state
> > condition")
> > 
> > and this one from net-next:
> > 
> >   45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
> > 
> > The conflict has been resolved on our side[1] and the resolution we
> > suggest is attached to this email.  

Thanks for the resolution! If you happen to remember perhaps throw
"manual merge" into the subject. That's what I search my inbox for
when merging, it will allow us to be even more lazy :)

Re: [PATCH net 2/3] can: gs_usb: gs_can_open(): fix race dev->can.state condition

[Matthieu Baerts]

Hi Jakub,

On 22/09/2022 22:05, Jakub Kicinski wrote:
> On Thu, 22 Sep 2022 10:23:38 +0200 Marc Kleine-Budde wrote:
>> On 22.09.2022 10:04:55, Matthieu Baerts wrote:
>>> FYI, we got a small conflict when merging -net in net-next in the MPTCP
>>> tree due to this patch applied in -net:
>>>
>>>   5440428b3da6 ("can: gs_usb: gs_can_open(): fix race dev->can.state
>>> condition")
>>>
>>> and this one from net-next:
>>>
>>>   45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
>>>
>>> The conflict has been resolved on our side[1] and the resolution we
>>> suggest is attached to this email.  
> 
> Thanks for the resolution! If you happen to remember perhaps throw
> "manual merge" into the subject. That's what I search my inbox for
> when merging, it will allow us to be even more lazy :)

I thought you were going to ask me to impersonate "Stephen Rothwell" :-P

Good to know, I can sure do that! (Or at least try to remember that next
time)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net