pull-request: can 2018-03-14

pull-request: can 2018-03-14

[Marc Kleine-Budde]

Hello David,

this is a pull request of two patches for net/master.

Both patches are by Andri Yngvason and fix problems in the cc770 driver,
that show up quite fast on RT systems, but also on non RT setups.

regards,
Marc

---

The following changes since commit f89782c2d131e6eae0d1ea2569ba76bc4c5875fe:

  qed: Use after free in qed_rdma_free() (2018-03-13 10:54:17 -0400)

are available in the Git repository at:

  ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-4.16-20180314

for you to fetch changes up to 746201235b3f876792099079f4c6fea941d76183:

  can: cc770: Fix queue stall & dropped RTR reply (2018-03-14 13:01:22 +0100)

----------------------------------------------------------------
linux-can-fixes-for-4.16-20180314

----------------------------------------------------------------
Andri Yngvason (2):
      can: cc770: Fix stalls on rt-linux, remove redundant IRQ ack
      can: cc770: Fix queue stall & dropped RTR reply

 drivers/net/can/cc770/cc770.c | 103 ++++++++++++++++++++++++++----------------
 drivers/net/can/cc770/cc770.h |   2 +
 2 files changed, 65 insertions(+), 40 deletions(-)

[PATCH 1/2] can: cc770: Fix stalls on rt-linux, remove redundant IRQ ack

[Marc Kleine-Budde]

From: Andri Yngvason <andri.yngvason@marel.com>

This has been reported to cause stalls on rt-linux.

Suggested-by: Richard Weinberger <richard@nod.at>
Tested-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/cc770/cc770.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
index 1e37313054f3..9fed163262e0 100644
--- a/drivers/net/can/cc770/cc770.c
+++ b/drivers/net/can/cc770/cc770.c
@@ -447,15 +447,6 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	stats->tx_bytes += dlc;
 
-
-	/*
-	 * HM: We had some cases of repeated IRQs so make sure the
-	 * INT is acknowledged I know it's already further up, but
-	 * doing again fixed the issue
-	 */
-	cc770_write_reg(priv, msgobj[mo].ctrl0,
-			MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
-
 	return NETDEV_TX_OK;
 }
 
@@ -684,12 +675,6 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o)
 	/* Nothing more to send, switch off interrupts */
 	cc770_write_reg(priv, msgobj[mo].ctrl0,
 			MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
-	/*
-	 * We had some cases of repeated IRQ so make sure the
-	 * INT is acknowledged
-	 */
-	cc770_write_reg(priv, msgobj[mo].ctrl0,
-			MSGVAL_UNC | TXIE_UNC | RXIE_UNC | INTPND_RES);
 
 	stats->tx_packets++;
 	can_get_echo_skb(dev, 0);
-- 
2.16.1

[PATCH 2/2] can: cc770: Fix queue stall & dropped RTR reply

[Marc Kleine-Budde]

From: Andri Yngvason <andri.yngvason@marel.com>

While waiting for the TX object to send an RTR, an external message with a
matching id can overwrite the TX data. In this case we must call the rx
routine and then try transmitting the message that was overwritten again.

The queue was being stalled because the RX event did not generate an
interrupt to wake up the queue again and the TX event did not happen
because the TXRQST flag is reset by the chip when new data is received.

According to the CC770 datasheet the id of a message object should not be
changed while the MSGVAL bit is set. This has been fixed by resetting the
MSGVAL bit before modifying the object in the transmit function and setting
it after. It is not enough to set & reset CPUUPD.

It is important to keep the MSGVAL bit reset while the message object is
being modified. Otherwise, during RTR transmission, a frame with matching
id could trigger an rx-interrupt, which would cause a race condition
between the interrupt routine and the transmit function.

Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
Tested-by: Richard Weinberger <richard@nod.at>
Cc: linux-stable <stable@vger.kernel.org>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/cc770/cc770.c | 94 ++++++++++++++++++++++++++++++-------------
 drivers/net/can/cc770/cc770.h |  2 +
 2 files changed, 68 insertions(+), 28 deletions(-)

diff --git a/drivers/net/can/cc770/cc770.c b/drivers/net/can/cc770/cc770.c
index 9fed163262e0..2743d82d4424 100644
--- a/drivers/net/can/cc770/cc770.c
+++ b/drivers/net/can/cc770/cc770.c
@@ -390,37 +390,23 @@ static int cc770_get_berr_counter(const struct net_device *dev,
 	return 0;
 }
 
-static netdev_tx_t cc770_start_xmit(struct sk_buff *skb, struct net_device *dev)
+static void cc770_tx(struct net_device *dev, int mo)
 {
 	struct cc770_priv *priv = netdev_priv(dev);
-	struct net_device_stats *stats = &dev->stats;
-	struct can_frame *cf = (struct can_frame *)skb->data;
-	unsigned int mo = obj2msgobj(CC770_OBJ_TX);
+	struct can_frame *cf = (struct can_frame *)priv->tx_skb->data;
 	u8 dlc, rtr;
 	u32 id;
 	int i;
 
-	if (can_dropped_invalid_skb(dev, skb))
-		return NETDEV_TX_OK;
-
-	if ((cc770_read_reg(priv,
-			    msgobj[mo].ctrl1) & TXRQST_UNC) == TXRQST_SET) {
-		netdev_err(dev, "TX register is still occupied!\n");
-		return NETDEV_TX_BUSY;
-	}
-
-	netif_stop_queue(dev);
-
 	dlc = cf->can_dlc;
 	id = cf->can_id;
-	if (cf->can_id & CAN_RTR_FLAG)
-		rtr = 0;
-	else
-		rtr = MSGCFG_DIR;
+	rtr = cf->can_id & CAN_RTR_FLAG ? 0 : MSGCFG_DIR;
+
+	cc770_write_reg(priv, msgobj[mo].ctrl0,
+			MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
 	cc770_write_reg(priv, msgobj[mo].ctrl1,
 			RMTPND_RES | TXRQST_RES | CPUUPD_SET | NEWDAT_RES);
-	cc770_write_reg(priv, msgobj[mo].ctrl0,
-			MSGVAL_SET | TXIE_SET | RXIE_RES | INTPND_RES);
+
 	if (id & CAN_EFF_FLAG) {
 		id &= CAN_EFF_MASK;
 		cc770_write_reg(priv, msgobj[mo].config,
@@ -439,13 +425,30 @@ static netdev_tx_t cc770_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	for (i = 0; i < dlc; i++)
 		cc770_write_reg(priv, msgobj[mo].data[i], cf->data[i]);
 
-	/* Store echo skb before starting the transfer */
-	can_put_echo_skb(skb, dev, 0);
-
 	cc770_write_reg(priv, msgobj[mo].ctrl1,
-			RMTPND_RES | TXRQST_SET | CPUUPD_RES | NEWDAT_UNC);
+			RMTPND_UNC | TXRQST_SET | CPUUPD_RES | NEWDAT_UNC);
+	cc770_write_reg(priv, msgobj[mo].ctrl0,
+			MSGVAL_SET | TXIE_SET | RXIE_SET | INTPND_UNC);
+}
+
+static netdev_tx_t cc770_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct cc770_priv *priv = netdev_priv(dev);
+	unsigned int mo = obj2msgobj(CC770_OBJ_TX);
+
+	if (can_dropped_invalid_skb(dev, skb))
+		return NETDEV_TX_OK;
 
-	stats->tx_bytes += dlc;
+	netif_stop_queue(dev);
+
+	if ((cc770_read_reg(priv,
+			    msgobj[mo].ctrl1) & TXRQST_UNC) == TXRQST_SET) {
+		netdev_err(dev, "TX register is still occupied!\n");
+		return NETDEV_TX_BUSY;
+	}
+
+	priv->tx_skb = skb;
+	cc770_tx(dev, mo);
 
 	return NETDEV_TX_OK;
 }
@@ -671,13 +674,47 @@ static void cc770_tx_interrupt(struct net_device *dev, unsigned int o)
 	struct cc770_priv *priv = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
 	unsigned int mo = obj2msgobj(o);
+	struct can_frame *cf;
+	u8 ctrl1;
+
+	ctrl1 = cc770_read_reg(priv, msgobj[mo].ctrl1);
 
-	/* Nothing more to send, switch off interrupts */
 	cc770_write_reg(priv, msgobj[mo].ctrl0,
 			MSGVAL_RES | TXIE_RES | RXIE_RES | INTPND_RES);
+	cc770_write_reg(priv, msgobj[mo].ctrl1,
+			RMTPND_RES | TXRQST_RES | MSGLST_RES | NEWDAT_RES);
 
-	stats->tx_packets++;
+	if (unlikely(!priv->tx_skb)) {
+		netdev_err(dev, "missing tx skb in tx interrupt\n");
+		return;
+	}
+
+	if (unlikely(ctrl1 & MSGLST_SET)) {
+		stats->rx_over_errors++;
+		stats->rx_errors++;
+	}
+
+	/* When the CC770 is sending an RTR message and it receives a regular
+	 * message that matches the id of the RTR message, it will overwrite the
+	 * outgoing message in the TX register. When this happens we must
+	 * process the received message and try to transmit the outgoing skb
+	 * again.
+	 */
+	if (unlikely(ctrl1 & NEWDAT_SET)) {
+		cc770_rx(dev, mo, ctrl1);
+		cc770_tx(dev, mo);
+		return;
+	}
+
+	can_put_echo_skb(priv->tx_skb, dev, 0);
 	can_get_echo_skb(dev, 0);
+
+	cf = (struct can_frame *)priv->tx_skb->data;
+	stats->tx_bytes += cf->can_dlc;
+	stats->tx_packets++;
+
+	priv->tx_skb = NULL;
+
 	netif_wake_queue(dev);
 }
 
@@ -789,6 +826,7 @@ struct net_device *alloc_cc770dev(int sizeof_priv)
 	priv->can.do_set_bittiming = cc770_set_bittiming;
 	priv->can.do_set_mode = cc770_set_mode;
 	priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
+	priv->tx_skb = NULL;
 
 	memcpy(priv->obj_flags, cc770_obj_flags, sizeof(cc770_obj_flags));
 
diff --git a/drivers/net/can/cc770/cc770.h b/drivers/net/can/cc770/cc770.h
index a1739db98d91..95752e1d1283 100644
--- a/drivers/net/can/cc770/cc770.h
+++ b/drivers/net/can/cc770/cc770.h
@@ -193,6 +193,8 @@ struct cc770_priv {
 	u8 cpu_interface;	/* CPU interface register */
 	u8 clkout;		/* Clock out register */
 	u8 bus_config;		/* Bus conffiguration register */
+
+	struct sk_buff *tx_skb;
 };
 
 struct net_device *alloc_cc770dev(int sizeof_priv);
-- 
2.16.1

Re: pull-request: can 2018-03-14

[Andri Yngvason]

On Wed, Mar 14, 2018 at 01:05:21PM +0100, Marc Kleine-Budde wrote:
> Hello David,
> 
> this is a pull request of two patches for net/master.
> 
> Both patches are by Andri Yngvason and fix problems in the cc770 driver,
> that show up quite fast on RT systems, but also on non RT setups.
> 
> regards,
> Marc
> 
> ---
> 
> The following changes since commit f89782c2d131e6eae0d1ea2569ba76bc4c5875fe:
> 
>   qed: Use after free in qed_rdma_free() (2018-03-13 10:54:17 -0400)
> 
> are available in the Git repository at:
> 
>   ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-4.16-20180314
> 
> for you to fetch changes up to 746201235b3f876792099079f4c6fea941d76183:
> 
>   can: cc770: Fix queue stall & dropped RTR reply (2018-03-14 13:01:22 +0100)
> 
> ----------------------------------------------------------------
> linux-can-fixes-for-4.16-20180314
> 
> ----------------------------------------------------------------
> Andri Yngvason (2):
>       can: cc770: Fix stalls on rt-linux, remove redundant IRQ ack
>       can: cc770: Fix queue stall & dropped RTR reply
> 
>  drivers/net/can/cc770/cc770.c | 103 ++++++++++++++++++++++++++----------------
>  drivers/net/can/cc770/cc770.h |   2 +
>  2 files changed, 65 insertions(+), 40 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wait, do we not want Wolfgang's signed-off-by or reviewed-by?

Thanks,
Andri

Re: pull-request: can 2018-03-14

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2336 bytes --]

On 03/14/2018 01:24 PM, Andri Yngvason wrote:
> On Wed, Mar 14, 2018 at 01:05:21PM +0100, Marc Kleine-Budde wrote:
>> Hello David,
>>
>> this is a pull request of two patches for net/master.
>>
>> Both patches are by Andri Yngvason and fix problems in the cc770 driver,
>> that show up quite fast on RT systems, but also on non RT setups.
>>
>> regards,
>> Marc
>>
>> ---
>>
>> The following changes since commit f89782c2d131e6eae0d1ea2569ba76bc4c5875fe:
>>
>>   qed: Use after free in qed_rdma_free() (2018-03-13 10:54:17 -0400)
>>
>> are available in the Git repository at:
>>
>>   ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-4.16-20180314
>>
>> for you to fetch changes up to 746201235b3f876792099079f4c6fea941d76183:
>>
>>   can: cc770: Fix queue stall & dropped RTR reply (2018-03-14 13:01:22 +0100)
>>
>> ----------------------------------------------------------------
>> linux-can-fixes-for-4.16-20180314
>>
>> ----------------------------------------------------------------
>> Andri Yngvason (2):
>>       can: cc770: Fix stalls on rt-linux, remove redundant IRQ ack
>>       can: cc770: Fix queue stall & dropped RTR reply
>>
>>  drivers/net/can/cc770/cc770.c | 103 ++++++++++++++++++++++++++----------------
>>  drivers/net/can/cc770/cc770.h |   2 +
>>  2 files changed, 65 insertions(+), 40 deletions(-)
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Wait, do we not want Wolfgang's signed-off-by or reviewed-by?

In the review Wolfgang was involved in "[PATCH v2 2/3] can: cc770: Stop
queue on NETDEV_TX_BUSY" and we all agreed to drop the patch.

The other patches were successfully tested by Richard, AFAIK there are
no open points anymore and there was no discussion on these patches for
more than 10 days. So assumed the patches are "ready" for upstreaming now.

Marc

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


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

Re: pull-request: can 2018-03-14

[Andri Yngvason]

On Wed, Mar 14, 2018 at 01:52:46PM +0100, Marc Kleine-Budde wrote:
> On 03/14/2018 01:24 PM, Andri Yngvason wrote:
> > On Wed, Mar 14, 2018 at 01:05:21PM +0100, Marc Kleine-Budde wrote:
> >> Hello David,
> >>
> >> this is a pull request of two patches for net/master.
> >>
> >> Both patches are by Andri Yngvason and fix problems in the cc770 driver,
> >> that show up quite fast on RT systems, but also on non RT setups.
> >>
> >> regards,
> >> Marc
> >>
> >> ---
> >>
> >> The following changes since commit f89782c2d131e6eae0d1ea2569ba76bc4c5875fe:
> >>
> >>   qed: Use after free in qed_rdma_free() (2018-03-13 10:54:17 -0400)
> >>
> >> are available in the Git repository at:
> >>
> >>   ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can.git tags/linux-can-fixes-for-4.16-20180314
> >>
> >> for you to fetch changes up to 746201235b3f876792099079f4c6fea941d76183:
> >>
> >>   can: cc770: Fix queue stall & dropped RTR reply (2018-03-14 13:01:22 +0100)
> >>
> >> ----------------------------------------------------------------
> >> linux-can-fixes-for-4.16-20180314
> >>
> >> ----------------------------------------------------------------
> >> Andri Yngvason (2):
> >>       can: cc770: Fix stalls on rt-linux, remove redundant IRQ ack
> >>       can: cc770: Fix queue stall & dropped RTR reply
> >>
> >>  drivers/net/can/cc770/cc770.c | 103 ++++++++++++++++++++++++++----------------
> >>  drivers/net/can/cc770/cc770.h |   2 +
> >>  2 files changed, 65 insertions(+), 40 deletions(-)
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > Wait, do we not want Wolfgang's signed-off-by or reviewed-by?
> 
> In the review Wolfgang was involved in "[PATCH v2 2/3] can: cc770: Stop
> queue on NETDEV_TX_BUSY" and we all agreed to drop the patch.
> 
> The other patches were successfully tested by Richard, AFAIK there are
> no open points anymore and there was no discussion on these patches for
> more than 10 days. So assumed the patches are "ready" for upstreaming now.
> 
All right!

Thanks,
Andri

Re: pull-request: can 2018-03-14

[David Miller]

From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Wed, 14 Mar 2018 13:05:21 +0100

> this is a pull request of two patches for net/master.
> 
> Both patches are by Andri Yngvason and fix problems in the cc770 driver,
> that show up quite fast on RT systems, but also on non RT setups.

Series applied, thanks Marc.