Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 28.04.2021 11:09:14, Marc Kleine-Budde wrote:
> From: "Sottas Guillaume (LMB)" <Guillaume.Sottas@liebherr.com>
> 
> Link: https://github.com/hartkopp/can-isotp/pull/43
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Not-Signed-off-by: Sottas Guillaume (LMB) <Guillaume.Sottas@liebherr.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> 
> Hello,
> 
> for better reviewing I'm posting Sottas's patch here. While porting to
> current net/master I've fixed up the indention.
> 
> regards,
> Marc
> 
>  net/can/isotp.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 9f94ad3caee9..d02e8f1f240f 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -110,6 +110,11 @@ MODULE_ALIAS("can-proto-6");
>  #define ISOTP_FC_WT 1		/* wait */
>  #define ISOTP_FC_OVFLW 2	/* overflow */
>  
> +enum {
> +	ISOTP_ERR_NO_ERROR = 0,
> +	ISOTP_ERR_FC_TIMEOUT,
> +};
> +
>  enum {
>  	ISOTP_IDLE = 0,
>  	ISOTP_WAIT_FIRST_FC,
> @@ -122,6 +127,7 @@ struct tpcon {
>  	int idx;
>  	int len;
>  	u8 state;
> +	u8 error;
>  	u8 bs;
>  	u8 sn;
>  	u8 ll_dl;
> @@ -756,6 +762,10 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)

>               sk->sk_err = ECOMM;
>  		if (!sock_flag(sk, SOCK_DEAD))
>  			sk->sk_error_report(sk);

I think the idea is that sk_error_report takes care of propagation of
the error to the user space. I don't know why it's not working as
expected.

>  
> +		/* set error flag in order to consume it later in the
> +		 * isotp_sendmsg function */
> +		so->tx.error = ISOTP_ERR_FC_TIMEOUT;
> +
>  		/* reset tx state */
>  		so->tx.state = ISOTP_IDLE;
>  		wake_up_interruptible(&so->wait);
> @@ -954,6 +964,13 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	if (wait_tx_done) {
>  		/* wait for complete transmission of current pdu */
>  		wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
> +
> +		/* check if an error has been raised in the timer
> +		 * function handler */
> +		if (so->tx.error == ISOTP_ERR_FC_TIMEOUT) {
> +			so->tx.error = ISOTP_ERR_NO_ERROR;
> +			return -ECOMM;
> +		}
>  	}
>  
>  	return size;
> @@ -1371,6 +1388,9 @@ static int isotp_init(struct sock *sk)
>  	so->tx.state = ISOTP_IDLE;
>  
>  	hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
> +
> +	so->tx.error = ISOTP_ERR_NO_ERROR;
> +	so->rx.error = ISOTP_ERR_NO_ERROR;
>  	so->rxtimer.function = isotp_rx_timer_handler;
>  	hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
>  	so->txtimer.function = isotp_tx_timer_handler;


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 --]