linux-kernel - Re: [RESEND] [PATCH 1/1] can: m_can: Control tx and rx flow to avoid communication stall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230705-return-slogan-36c499673bb6-mkl@pengutronix.de>
Date:   Wed, 5 Jul 2023 14:17:28 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Kumari Pallavi <kumari.pallavi@...el.com>
Cc:     rcsekar@...sung.com, mallikarjunappa.sangannavar@...el.com,
        jarkko.nikula@...el.com, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Srikanth Thokala <srikanth.thokala@...el.com>
Subject: Re: [RESEND] [PATCH 1/1] can: m_can: Control tx and rx flow to avoid
 communication stall

On 23.06.2023 14:29:20, Kumari Pallavi wrote:
> In bi-directional CAN transfer using M_CAN IP, with
> the frame gap being set to '0', it leads to Protocol
> error in Arbitration phase resulting in communication
> stall.

Is there a (public) erratum describing the problem?

> Discussed with Bosch M_CAN IP team and the stall issue
> can only be overcome by controlling the tx and rx 
> packets flow as done by the patch.

Please elaborate the suggested workaround.

> Rx packets would also be serviced when there is a tx 
> interrupt. The solution has been tested extensively for
> more than 10 days, and no issues has been observed.

Can you describe how your patch implements the workaround?

| Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
| instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
| to do frotz", as if you are giving orders to the codebase to change
| its behaviour.

See: https://github.com/torvalds/linux/blob/master/Documentation/process/submitting-patches.rst#describe-your-changes

> Setup that is used to reproduce the issue: 
> 
> +---------------------+		+----------------------+
> |Intel ElkhartLake    |		|Intel ElkhartLake     |		
> |	+--------+    |		|	+--------+     |
> |	|m_can 0 |    |<=======>|	|m_can 0 |     |		    
> |	+--------+    |		|	+--------+     |		 
> +---------------------+		+----------------------+           
> 
> Steps to be run on the two Elkhartlake HW:
> 
> 1. ip link set can0 type can bitrate 1000000
> 2. ip link set can0 txqueuelen 2048
> 3. ip link set can0 up
> 4. cangen -g 0 can0
> 5. candump can0
> 
> cangen -g 0 can0 & candump can0 commands are used for transmit and 
> receive on both the m_can HW simultaneously where -g is the frame gap 
> between two frames.
> 
> Signed-off-by: Kumari Pallavi <kumari.pallavi@...el.com>
> Signed-off-by: Srikanth Thokala <srikanth.thokala@...el.com>
> ---
>  drivers/net/can/m_can/m_can.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a5003435802b..94aa0ba89202 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1118,7 +1118,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
>  			/* New TX FIFO Element arrived */
>  			if (m_can_echo_tx_event(dev) != 0)
>  				goto out_fail;
> -

nitpick: please keep that empty line.

> +			m_can_write(cdev, M_CAN_IE, IR_ALL_INT & ~(IR_TEFN));

- What's the purpose of  "()" around IR_TEFN?
- You enable a lot of interrupts that have not been enabled before. Have
  a look at m_can_chip_config() how the original register value for
  M_CAN_IE is calculated.

>  			if (netif_queue_stopped(dev) &&
>  			    !m_can_tx_fifo_full(cdev))
>  				netif_wake_queue(dev);
> @@ -1787,6 +1787,7 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>  		}
>  	} else {
>  		cdev->tx_skb = skb;
> +		m_can_write(cdev, M_CAN_IE, IR_ALL_INT & (IR_TEFN));

- What's the purpose of  "()" around IR_TEFN?
- "IR_ALL_INT & (IR_TEFN)" is equal to IR_TEFN, isn't it?
- This basically disables all other interrupts, is this what you want to
  do?
- What happens if the bus is busy with high prio CAN frames and you want
  to send low prio ones? You will not get any RX-IRQ, this doesn't look
  correct to me.

>  		return m_can_tx_handler(cdev);
>  	}
>  
> -- 
> 2.17.1
> 
>

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)