[PATCH net-next] mctp i2c: notify user space on TX failure

[PATCH net-next] mctp i2c: notify user space on TX failure

[Jian Zhang]

Currently, there is no error handling mechanism for TX failures, causing
user space to remain unaware of these failures until a timeout occurs.
This leads to unnecessary waiting and delays.

This update sends an immediate error notification to user space upon TX
failure, reducing wait times and improving response handling for failed
tx.

Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com>
---
 drivers/net/mctp/mctp-i2c.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
index 4dc057c121f5..e9a835606dfc 100644
--- a/drivers/net/mctp/mctp-i2c.c
+++ b/drivers/net/mctp/mctp-i2c.c
@@ -485,6 +485,7 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
 	struct mctp_i2c_hdr *hdr;
 	struct i2c_msg msg = {0};
 	u8 *pecp;
+	struct sock *sk;
 	int rc;
 
 	fs = mctp_i2c_get_tx_flow_state(midev, skb);
@@ -551,6 +552,14 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
 		dev_warn_ratelimited(&midev->adapter->dev,
 				     "__i2c_transfer failed %d\n", rc);
 		stats->tx_errors++;
+
+		sk = skb->sk;
+		if (sk) {
+			sk->sk_err = -rc;
+			if (!sock_flag(sk, SOCK_DEAD))
+				sk_error_report(sk);
+		}
+
 	} else {
 		stats->tx_bytes += skb->len;
 		stats->tx_packets++;
-- 
2.30.2

Re: [PATCH net-next] mctp i2c: notify user space on TX failure

[Jakub Kicinski]

On Fri,  8 Nov 2024 17:42:06 +0800 Jian Zhang wrote:
> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
> index 4dc057c121f5..e9a835606dfc 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c
> @@ -485,6 +485,7 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
>  	struct mctp_i2c_hdr *hdr;
>  	struct i2c_msg msg = {0};
>  	u8 *pecp;
> +	struct sock *sk;
>  	int rc;
>  

nit: order the variable declaration lines longest to shortest

> @@ -551,6 +552,14 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
>  		dev_warn_ratelimited(&midev->adapter->dev,
>  				     "__i2c_transfer failed %d\n", rc);
>  		stats->tx_errors++;
> +
> +		sk = skb->sk;
> +		if (sk) {
> +			sk->sk_err = -rc;
> +			if (!sock_flag(sk, SOCK_DEAD))
> +				sk_error_report(sk);
> +		}

notifying socket in the xmit handler of a netdev is a bit strange,
could you do it somewhere higher in the MCTP stack?
-- 
pw-bot: cr

Re: [PATCH net-next] mctp i2c: notify user space on TX failure

[Jeremy Kerr]

Hi Jakub,

> > @@ -551,6 +552,14 @@ static void mctp_i2c_xmit(struct mctp_i2c_dev *midev, struct sk_buff *skb)
> >                 dev_warn_ratelimited(&midev->adapter->dev,
> >                                      "__i2c_transfer failed %d\n", rc);
> >                 stats->tx_errors++;
> > +
> > +               sk = skb->sk;
> > +               if (sk) {
> > +                       sk->sk_err = -rc;
> > +                       if (!sock_flag(sk, SOCK_DEAD))
> > +                               sk_error_report(sk);
> > +               }
> 
> notifying socket in the xmit handler of a netdev is a bit strange,
> could you do it somewhere higher in the MCTP stack?

Sounds like that would be useful in general for MCTP, but we don't have
a facility for that at present.  Any existing implementation you would
suggest modelling this on?

Cheers,


Jeremy

Re: [PATCH net-next] mctp i2c: notify user space on TX failure

[Jakub Kicinski]

On Thu, 14 Nov 2024 11:13:33 +0800 Jeremy Kerr wrote:
> > notifying socket in the xmit handler of a netdev is a bit strange,
> > could you do it somewhere higher in the MCTP stack?  
> 
> Sounds like that would be useful in general for MCTP, but we don't have
> a facility for that at present.  Any existing implementation you would
> suggest modelling this on?

routing isn't really my forte, TBH, what eats the error so that it
doesn't come out of mctp_local_output() ? Do you use qdiscs on top
of the MCTP devices?

Re: [PATCH net-next] mctp i2c: notify user space on TX failure

[Jeremy Kerr]

Hi Jakub,


> routing isn't really my forte, TBH, what eats the error so that it
> doesn't come out of mctp_local_output() ? Do you use qdiscs on top
> of the MCTP devices?

There are no qdiscs involved at this stage, as we need to preserve
packet ordering in most cases. The route output functions will end up
in a dev_queue_xmit, so any tx error would have been decoupled from the
route output at that stage.

I'm happy to do the exploring myself if you don't have strong opinions
on an implementation.

Cheers,


Jeremy

Re: [PATCH net-next] mctp i2c: notify user space on TX failure

[Jakub Kicinski]

On Thu, 14 Nov 2024 14:48:57 +0800 Jeremy Kerr wrote:
> > routing isn't really my forte, TBH, what eats the error so that it
> > doesn't come out of mctp_local_output() ? Do you use qdiscs on top
> > of the MCTP devices?  
> 
> There are no qdiscs involved at this stage, as we need to preserve
> packet ordering in most cases. The route output functions will end up
> in a dev_queue_xmit, so any tx error would have been decoupled from the
> route output at that stage.

Ah, it's the driver eating the errors, it puts the packet on a local
queue and returns OK no matter what. The I2C transfer happens from 
a thread.

I wonder if there is precedent, let's ask CAN experts.

Mark, MCTP would like to report errors from the drivers all the way 
to the socket. Do CAN drivers do something along these lines?

Re: [PATCH net-next] mctp i2c: notify user space on TX failure

[Marc Kleine-Budde]

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

On 14.11.2024 07:02:35, Jakub Kicinski wrote:
> On Thu, 14 Nov 2024 14:48:57 +0800 Jeremy Kerr wrote:
> > > routing isn't really my forte, TBH, what eats the error so that it
> > > doesn't come out of mctp_local_output() ? Do you use qdiscs on top
> > > of the MCTP devices?  
> > 
> > There are no qdiscs involved at this stage, as we need to preserve
> > packet ordering in most cases. The route output functions will end up
> > in a dev_queue_xmit, so any tx error would have been decoupled from the
> > route output at that stage.
> 
> Ah, it's the driver eating the errors, it puts the packet on a local
> queue and returns OK no matter what. The I2C transfer happens from 
> a thread.
> 
> I wonder if there is precedent, let's ask CAN experts.
> 
> Mark, MCTP would like to report errors from the drivers all the way 
> to the socket. Do CAN drivers do something along these lines?

On CAN_RAW we send fixed size messages (struct can_frame) and there is a
bit left to mark a can_frame as an error frame. This basically means we
send the error notification inline.

What about using sock_queue_err_skb()? We do this in CAN_J1939.

regards,
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   |

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