[PATCH 1/2] net, can, ifi: fix "write buffer full" error

[PATCH 1/2] net, can, ifi: fix "write buffer full" error

[Heiko Schocher]

the driver reads in the ISR first the IRQpending register,
and clears after that in a write *all* bits in it.

It could happen that the isr register raise bits between
this 2 register accesses, which leads in lost bits ...

In case it clears "TX message sent successfully", the driver
never sends any Tx data, and buffers to userspace run over.

Fixed this:
clear only the bits in the IRQpending register, the
driver had read.

Signed-off-by: Heiko Schocher <hs@denx.de>
---

 drivers/net/can/ifi_canfd/ifi_canfd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 2772d05ff11c..05feb8177936 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -607,7 +607,7 @@ static irqreturn_t ifi_canfd_isr(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	/* Clear all pending interrupts but ErrWarn */
-	writel(clr_irq_mask, priv->base + IFI_CANFD_INTERRUPT);
+	writel(isr & clr_irq_mask, priv->base + IFI_CANFD_INTERRUPT);
 
 	/* RX IRQ or bus warning, start NAPI */
 	if (isr & rx_irq_mask) {
-- 
2.14.3

[PATCH 2/2] net, can, ifi: loopback Tx message in IFI block

[Heiko Schocher]

Current ifi driver reads first Rx messages, than loopback
the Tx message, if the IFI_CANFD_INTERRUPT_TXFIFO_REMOVE
bit is set. This can lead into the case, that Rx messages
overhelm Tx messages!

Fixed this in the following way:

Set in the IFI_CANFD_TXFIFO_DLC register the FN value to
1, so the IFI block loopsback itself the Tx message when
sended correctly on the canfd bus. Only the IFI block can
insert the Tx message in the correct place.

The linux driver now needs only to free the skb, when
the Tx message was sended correctly.

Signed-off-by: Heiko Schocher <hs@denx.de>
---

 drivers/net/can/ifi_canfd/ifi_canfd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 05feb8177936..0d36cb8659ae 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -211,6 +211,7 @@ struct ifi_canfd_priv {
 	struct napi_struct	napi;
 	struct net_device	*ndev;
 	void __iomem		*base;
+	unsigned int		tx_len;
 };
 
 static void ifi_canfd_irq_enable(struct net_device *ndev, bool enable)
@@ -617,8 +618,10 @@ static irqreturn_t ifi_canfd_isr(int irq, void *dev_id)
 
 	/* TX IRQ */
 	if (isr & IFI_CANFD_INTERRUPT_TXFIFO_REMOVE) {
-		stats->tx_bytes += can_get_echo_skb(ndev, 0);
+		can_free_echo_skb(ndev, 0);
+		stats->tx_bytes += priv->tx_len;
 		stats->tx_packets++;
+		priv->tx_len = 0;
 		can_led_event(ndev, CAN_LED_EVENT_TX);
 	}
 
@@ -889,6 +892,7 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
 	}
 
 	txdlc = can_len2dlc(cf->len);
+	priv->tx_len = txdlc;
 	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
 		txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
 		if (cf->flags & CANFD_BRS)
@@ -898,6 +902,12 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
 	if (cf->can_id & CAN_RTR_FLAG)
 		txdlc |= IFI_CANFD_TXFIFO_DLC_RTR;
 
+	/*
+	 * set FNR to 1, so we get our Tx Message looped back
+	 * into RxFIFO
+	 */
+	txdlc += (1 << IFI_CANFD_TXFIFO_DLC_FNR_OFFSET);
+
 	/* message ram configuration */
 	writel(txid, priv->base + IFI_CANFD_TXFIFO_ID);
 	writel(txdlc, priv->base + IFI_CANFD_TXFIFO_DLC);
-- 
2.14.3

Re: [PATCH 1/2] net, can, ifi: fix "write buffer full" error

[Marek Vasut]

On 02/06/2018 01:02 PM, Heiko Schocher wrote:
> the driver reads in the ISR first the IRQpending register,
> and clears after that in a write *all* bits in it.
> 
> It could happen that the isr register raise bits between
> this 2 register accesses, which leads in lost bits ...
> 
> In case it clears "TX message sent successfully", the driver
> never sends any Tx data, and buffers to userspace run over.
> 
> Fixed this:
> clear only the bits in the IRQpending register, the
> driver had read.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>

Reviewed-by: Marek Vasut <marex@denx.de>

> ---
> 
>  drivers/net/can/ifi_canfd/ifi_canfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index 2772d05ff11c..05feb8177936 100644
> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
> @@ -607,7 +607,7 @@ static irqreturn_t ifi_canfd_isr(int irq, void *dev_id)
>  		return IRQ_NONE;
>  
>  	/* Clear all pending interrupts but ErrWarn */
> -	writel(clr_irq_mask, priv->base + IFI_CANFD_INTERRUPT);
> +	writel(isr & clr_irq_mask, priv->base + IFI_CANFD_INTERRUPT);
>  
>  	/* RX IRQ or bus warning, start NAPI */
>  	if (isr & rx_irq_mask) {
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 2/2] net, can, ifi: loopback Tx message in IFI block

[Marek Vasut]

On 02/06/2018 01:02 PM, Heiko Schocher wrote:
> Current ifi driver reads first Rx messages, than loopback
> the Tx message, if the IFI_CANFD_INTERRUPT_TXFIFO_REMOVE
> bit is set. This can lead into the case, that Rx messages
> overhelm Tx messages!
> 
> Fixed this in the following way:
> 
> Set in the IFI_CANFD_TXFIFO_DLC register the FN value to
> 1, so the IFI block loopsback itself the Tx message when
> sended correctly on the canfd bus. Only the IFI block can
> insert the Tx message in the correct place.
> 
> The linux driver now needs only to free the skb, when
> the Tx message was sended correctly.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
> 
>  drivers/net/can/ifi_canfd/ifi_canfd.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index 05feb8177936..0d36cb8659ae 100644
> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
> @@ -211,6 +211,7 @@ struct ifi_canfd_priv {
>  	struct napi_struct	napi;
>  	struct net_device	*ndev;
>  	void __iomem		*base;
> +	unsigned int		tx_len;
>  };
>  
>  static void ifi_canfd_irq_enable(struct net_device *ndev, bool enable)
> @@ -617,8 +618,10 @@ static irqreturn_t ifi_canfd_isr(int irq, void *dev_id)
>  
>  	/* TX IRQ */
>  	if (isr & IFI_CANFD_INTERRUPT_TXFIFO_REMOVE) {
> -		stats->tx_bytes += can_get_echo_skb(ndev, 0);
> +		can_free_echo_skb(ndev, 0);
> +		stats->tx_bytes += priv->tx_len;
>  		stats->tx_packets++;
> +		priv->tx_len = 0;
>  		can_led_event(ndev, CAN_LED_EVENT_TX);
>  	}
>  
> @@ -889,6 +892,7 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
>  	}
>  
>  	txdlc = can_len2dlc(cf->len);
> +	priv->tx_len = txdlc;
>  	if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) && can_is_canfd_skb(skb)) {
>  		txdlc |= IFI_CANFD_TXFIFO_DLC_EDL;
>  		if (cf->flags & CANFD_BRS)
> @@ -898,6 +902,12 @@ static netdev_tx_t ifi_canfd_start_xmit(struct sk_buff *skb,
>  	if (cf->can_id & CAN_RTR_FLAG)
>  		txdlc |= IFI_CANFD_TXFIFO_DLC_RTR;
>  
> +	/*
> +	 * set FNR to 1, so we get our Tx Message looped back
> +	 * into RxFIFO
> +	 */

Nit, you can make this into a single-line comment ;-)

Otherwise,

Reviewed-by: Marek Vasut <marex@denx.de>

> +	txdlc += (1 << IFI_CANFD_TXFIFO_DLC_FNR_OFFSET);
> +
>  	/* message ram configuration */
>  	writel(txid, priv->base + IFI_CANFD_TXFIFO_ID);
>  	writel(txdlc, priv->base + IFI_CANFD_TXFIFO_DLC);
> 


-- 
Best regards,
Marek Vasut