[PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[Frank Jungclaus]

Started a rework initiated by Vincents remarks "You should not report
the greatest of txerr and rxerr but the one which actually increased."
[1] and "As far as I understand, those flags should be set only when
the threshold is *reached*" [2] .

Now setting the flags for CAN_ERR_CRTL_[RT]X_WARNING and
CAN_ERR_CRTL_[RT]X_PASSIVE regarding REC and TEC, when the
appropriate threshold is reached.

Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
Link: [1] https://lore.kernel.org/all/CAMZ6RqKGBWe15aMkf8-QLf-cOQg99GQBebSm+1wEzTqHgvmNuw@mail.gmail.com/
Link: [2] https://lore.kernel.org/all/CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRGWDZBS0fs7zbSTy4hmA@mail.gmail.com/
---
 drivers/net/can/usb/esd_usb.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 5e182fadd875..09745751f168 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -255,10 +255,18 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
 				can_bus_off(priv->netdev);
 				break;
 			case ESD_BUSSTATE_WARN:
+				cf->can_id |= CAN_ERR_CRTL;
+				cf->data[1] = (txerr > rxerr) ?
+						CAN_ERR_CRTL_TX_WARNING :
+						CAN_ERR_CRTL_RX_WARNING;
 				priv->can.state = CAN_STATE_ERROR_WARNING;
 				priv->can.can_stats.error_warning++;
 				break;
 			case ESD_BUSSTATE_ERRPASSIVE:
+				cf->can_id |= CAN_ERR_CRTL;
+				cf->data[1] = (txerr > rxerr) ?
+						CAN_ERR_CRTL_TX_PASSIVE :
+						CAN_ERR_CRTL_RX_PASSIVE;
 				priv->can.state = CAN_STATE_ERROR_PASSIVE;
 				priv->can.can_stats.error_passive++;
 				break;
@@ -296,12 +304,6 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
 			/* Bit stream position in CAN frame as the error was detected */
 			cf->data[3] = ecc & SJA1000_ECC_SEG;
 
-			if (priv->can.state == CAN_STATE_ERROR_WARNING ||
-			    priv->can.state == CAN_STATE_ERROR_PASSIVE) {
-				cf->data[1] = (txerr > rxerr) ?
-					CAN_ERR_CRTL_TX_PASSIVE :
-					CAN_ERR_CRTL_RX_PASSIVE;
-			}
 			cf->data[6] = txerr;
 			cf->data[7] = rxerr;
 		}
-- 
2.25.1

[PATCH 3/3] can: esd_usb: Improved decoding for ESD_EV_CAN_ERROR_EXT messages

[Frank Jungclaus]

As suggested by Marc there now is a union plus a struct ev_can_err_ext
for easier decoding of an ESD_EV_CAN_ERROR_EXT event message (which
simply is a rx_msg with some dedicated data).

Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Link: https://lore.kernel.org/linux-can/20220621071152.ggyhrr5sbzvwpkpx@pengutronix.de/
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 09745751f168..f90bb2c0ba15 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -127,7 +127,15 @@ struct rx_msg {
 	u8 dlc;
 	__le32 ts;
 	__le32 id; /* upper 3 bits contain flags */
-	u8 data[8];
+	union {
+		u8 data[8];
+		struct {
+			u8 status; /* CAN Controller Status */
+			u8 ecc;    /* Error Capture Register */
+			u8 rec;    /* RX Error Counter */
+			u8 tec;    /* TX Error Counter */
+		} ev_can_err_ext;  /* For ESD_EV_CAN_ERROR_EXT */
+	};
 };
 
 struct tx_msg {
@@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
 	u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
 
 	if (id == ESD_EV_CAN_ERROR_EXT) {
-		u8 state = msg->msg.rx.data[0];
-		u8 ecc = msg->msg.rx.data[1];
-		u8 rxerr = msg->msg.rx.data[2];
-		u8 txerr = msg->msg.rx.data[3];
+		u8 state = msg->msg.rx.ev_can_err_ext.status;
+		u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
+		u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
+		u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
 
 		netdev_dbg(priv->netdev,
 			   "CAN_ERR_EV_EXT: dlc=%#02x state=%02x ecc=%02x rec=%02x tec=%02x\n",
-- 
2.25.1

Re: [PATCH 3/3] can: esd_usb: Improved decoding for ESD_EV_CAN_ERROR_EXT messages

[Vincent MAILHOL]

Le mar. 20 déc. 2022 à 06:28, Frank Jungclaus <frank.jungclaus@esd.eu> a écrit :
>
> As suggested by Marc there now is a union plus a struct ev_can_err_ext
> for easier decoding of an ESD_EV_CAN_ERROR_EXT event message (which
> simply is a rx_msg with some dedicated data).
>
> Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Link: https://lore.kernel.org/linux-can/20220621071152.ggyhrr5sbzvwpkpx@pengutronix.de/
> Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> ---
>  drivers/net/can/usb/esd_usb.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 09745751f168..f90bb2c0ba15 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -127,7 +127,15 @@ struct rx_msg {
>         u8 dlc;
>         __le32 ts;
>         __le32 id; /* upper 3 bits contain flags */
> -       u8 data[8];
> +       union {
> +               u8 data[8];
> +               struct {
> +                       u8 status; /* CAN Controller Status */
> +                       u8 ecc;    /* Error Capture Register */
> +                       u8 rec;    /* RX Error Counter */
> +                       u8 tec;    /* TX Error Counter */
> +               } ev_can_err_ext;  /* For ESD_EV_CAN_ERROR_EXT */
> +       };
>  };
>
>  struct tx_msg {
> @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
>         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
>
>         if (id == ESD_EV_CAN_ERROR_EXT) {
> -               u8 state = msg->msg.rx.data[0];
> -               u8 ecc = msg->msg.rx.data[1];
> -               u8 rxerr = msg->msg.rx.data[2];
> -               u8 txerr = msg->msg.rx.data[3];
> +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;

I do not like how you have to write msg->msg.rx.something. I think it
would be better to make the union within struct esd_usb_msg anonymous:

  https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169

That said, this is not a criticism of this patch but more something to
be addressed in a separate clean-up patch.

>                 netdev_dbg(priv->netdev,
>                            "CAN_ERR_EV_EXT: dlc=%#02x state=%02x ecc=%02x rec=%02x tec=%02x\n",
> --
> 2.25.1
>

Re: [PATCH 3/3] can: esd_usb: Improved decoding for ESD_EV_CAN_ERROR_EXT messages

[Vincent MAILHOL]

On Tue. 20 Dec. 2022 at 14:27, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> Le mar. 20 déc. 2022 à 06:28, Frank Jungclaus <frank.jungclaus@esd.eu> a écrit :
> > As suggested by Marc there now is a union plus a struct ev_can_err_ext
> > for easier decoding of an ESD_EV_CAN_ERROR_EXT event message (which
> > simply is a rx_msg with some dedicated data).
> >
> > Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > Link: https://lore.kernel.org/linux-can/20220621071152.ggyhrr5sbzvwpkpx@pengutronix.de/
> > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > ---
> >  drivers/net/can/usb/esd_usb.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > index 09745751f168..f90bb2c0ba15 100644
> > --- a/drivers/net/can/usb/esd_usb.c
> > +++ b/drivers/net/can/usb/esd_usb.c
> > @@ -127,7 +127,15 @@ struct rx_msg {
> >         u8 dlc;
> >         __le32 ts;
> >         __le32 id; /* upper 3 bits contain flags */
> > -       u8 data[8];
> > +       union {
> > +               u8 data[8];
> > +               struct {
> > +                       u8 status; /* CAN Controller Status */
> > +                       u8 ecc;    /* Error Capture Register */
> > +                       u8 rec;    /* RX Error Counter */
> > +                       u8 tec;    /* TX Error Counter */
> > +               } ev_can_err_ext;  /* For ESD_EV_CAN_ERROR_EXT */
> > +       };
> >  };
> >
> >  struct tx_msg {
> > @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> >         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
> >
> >         if (id == ESD_EV_CAN_ERROR_EXT) {
> > -               u8 state = msg->msg.rx.data[0];
> > -               u8 ecc = msg->msg.rx.data[1];
> > -               u8 rxerr = msg->msg.rx.data[2];
> > -               u8 txerr = msg->msg.rx.data[3];
> > +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> > +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> > +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> > +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
>
> I do not like how you have to write msg->msg.rx.something. I think it
> would be better to make the union within struct esd_usb_msg anonymous:
>
>   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169

Or maybe just declare esd_usb_msg as an union instead of a struct:

  union esd_usb_msg {
          struct header_msg hdr;
          struct version_msg version;
          struct version_reply_msg version_reply;
          struct rx_msg rx;
          struct tx_msg tx;
          struct tx_done_msg txdone;
          struct set_baudrate_msg setbaud;
          struct id_filter_msg filter;
  };

Re: [PATCH 3/3] can: esd_usb: Improved decoding for ESD_EV_CAN_ERROR_EXT messages

[Marc Kleine-Budde]

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

On 20.12.2022 17:53:28, Vincent MAILHOL wrote:
> > >  struct tx_msg {
> > > @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > >         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
> > >
> > >         if (id == ESD_EV_CAN_ERROR_EXT) {
> > > -               u8 state = msg->msg.rx.data[0];
> > > -               u8 ecc = msg->msg.rx.data[1];
> > > -               u8 rxerr = msg->msg.rx.data[2];
> > > -               u8 txerr = msg->msg.rx.data[3];
> > > +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> > > +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> > > +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> > > +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
> >
> > I do not like how you have to write msg->msg.rx.something. I think it
> > would be better to make the union within struct esd_usb_msg anonymous:
> >
> >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169
> 
> Or maybe just declare esd_usb_msg as an union instead of a struct:

+1

>   union esd_usb_msg {
>           struct header_msg hdr;
>           struct version_msg version;
>           struct version_reply_msg version_reply;
>           struct rx_msg rx;
>           struct tx_msg tx;
>           struct tx_done_msg txdone;
>           struct set_baudrate_msg setbaud;
>           struct id_filter_msg filter;
>   };

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

Re: [PATCH 3/3] can: esd_usb: Improved decoding for ESD_EV_CAN_ERROR_EXT messages

[Frank Jungclaus]

On Tue, 2022-12-20 at 10:05 +0100, Marc Kleine-Budde wrote:
> On 20.12.2022 17:53:28, Vincent MAILHOL wrote:
> > > >  struct tx_msg {
> > > > @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > > >         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
> > > > 
> > > >         if (id == ESD_EV_CAN_ERROR_EXT) {
> > > > -               u8 state = msg->msg.rx.data[0];
> > > > -               u8 ecc = msg->msg.rx.data[1];
> > > > -               u8 rxerr = msg->msg.rx.data[2];
> > > > -               u8 txerr = msg->msg.rx.data[3];
> > > > +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> > > > +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> > > > +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> > > > +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
> > > 
> > > I do not like how you have to write msg->msg.rx.something. I think it
> > > would be better to make the union within struct esd_usb_msg anonymous:
> > > 
> > >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169
> > 
> > Or maybe just declare esd_usb_msg as an union instead of a struct:
> 
> +1

Accepted ;)
I'll try to address this in a separate code-clean-up patch.

> 
> >   union esd_usb_msg {
> >           struct header_msg hdr;
> >           struct version_msg version;
> >           struct version_reply_msg version_reply;
> >           struct rx_msg rx;
> >           struct tx_msg tx;
> >           struct tx_done_msg txdone;
> >           struct set_baudrate_msg setbaud;
> >           struct id_filter_msg filter;
> >   };
> 
> Marc
> 

Re: [PATCH 3/3] can: esd_usb: Improved decoding for ESD_EV_CAN_ERROR_EXT messages

[Frank Jungclaus]

On Tue, 2022-12-20 at 17:53 +0900, Vincent MAILHOL wrote:
> On Tue. 20 Dec. 2022 at 14:27, Vincent MAILHOL
> <mailhol.vincent@wanadoo.fr> wrote:
> > Le mar. 20 déc. 2022 à 06:28, Frank Jungclaus <frank.jungclaus@esd.eu> a écrit :
> > > As suggested by Marc there now is a union plus a struct ev_can_err_ext
> > > for easier decoding of an ESD_EV_CAN_ERROR_EXT event message (which
> > > simply is a rx_msg with some dedicated data).
> > > 
> > > Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > Link: https://lore.kernel.org/linux-can/20220621071152.ggyhrr5sbzvwpkpx@pengutronix.de/
> > > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > > ---
> > >  drivers/net/can/usb/esd_usb.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > > index 09745751f168..f90bb2c0ba15 100644
> > > --- a/drivers/net/can/usb/esd_usb.c
> > > +++ b/drivers/net/can/usb/esd_usb.c
> > > @@ -127,7 +127,15 @@ struct rx_msg {
> > >         u8 dlc;
> > >         __le32 ts;
> > >         __le32 id; /* upper 3 bits contain flags */
> > > -       u8 data[8];
> > > +       union {
> > > +               u8 data[8];
> > > +               struct {
> > > +                       u8 status; /* CAN Controller Status */
> > > +                       u8 ecc;    /* Error Capture Register */
> > > +                       u8 rec;    /* RX Error Counter */
> > > +                       u8 tec;    /* TX Error Counter */
> > > +               } ev_can_err_ext;  /* For ESD_EV_CAN_ERROR_EXT */
> > > +       };
> > >  };
> > > 
> > >  struct tx_msg {
> > > @@ -229,10 +237,10 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > >         u32 id = le32_to_cpu(msg->msg.rx.id) & ESD_IDMASK;
> > > 
> > >         if (id == ESD_EV_CAN_ERROR_EXT) {
> > > -               u8 state = msg->msg.rx.data[0];
> > > -               u8 ecc = msg->msg.rx.data[1];
> > > -               u8 rxerr = msg->msg.rx.data[2];
> > > -               u8 txerr = msg->msg.rx.data[3];
> > > +               u8 state = msg->msg.rx.ev_can_err_ext.status;
> > > +               u8 ecc = msg->msg.rx.ev_can_err_ext.ecc;
> > > +               u8 rxerr = msg->msg.rx.ev_can_err_ext.rec;
> > > +               u8 txerr = msg->msg.rx.ev_can_err_ext.tec;
> > 
> > I do not like how you have to write msg->msg.rx.something. I think it
> > would be better to make the union within struct esd_usb_msg anonymous:
> > 
> >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/esd_usb.c#L169
> 
> Or maybe just declare esd_usb_msg as an union instead of a struct:
> 
>   union esd_usb_msg {
>           struct header_msg hdr;
>           struct version_msg version;
>           struct version_reply_msg version_reply;
>           struct rx_msg rx;
>           struct tx_msg tx;
>           struct tx_done_msg txdone;
>           struct set_baudrate_msg setbaud;
>           struct id_filter_msg filter;
>   };

Apart from the fact that this change would probably require several
dozen lines of code to be adjusted, I like the idea ;)

Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[Vincent MAILHOL]

On Tue. 20 Dec. 2022 at 06:29, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> Started a rework initiated by Vincents remarks "You should not report
> the greatest of txerr and rxerr but the one which actually increased."
> [1]

I do not see this comment being addressed. You are still assigning the
flags depending on the highest value, not the one which actually
changed.

> and "As far as I understand, those flags should be set only when
> the threshold is *reached*" [2] .
>
> Now setting the flags for CAN_ERR_CRTL_[RT]X_WARNING and
> CAN_ERR_CRTL_[RT]X_PASSIVE regarding REC and TEC, when the
> appropriate threshold is reached.
>
> Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
> Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> Link: [1] https://lore.kernel.org/all/CAMZ6RqKGBWe15aMkf8-QLf-cOQg99GQBebSm+1wEzTqHgvmNuw@mail.gmail.com/
> Link: [2] https://lore.kernel.org/all/CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRGWDZBS0fs7zbSTy4hmA@mail.gmail.com/
> ---
>  drivers/net/can/usb/esd_usb.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 5e182fadd875..09745751f168 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -255,10 +255,18 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
>                                 can_bus_off(priv->netdev);
>                                 break;
>                         case ESD_BUSSTATE_WARN:
> +                               cf->can_id |= CAN_ERR_CRTL;
> +                               cf->data[1] = (txerr > rxerr) ?
> +                                               CAN_ERR_CRTL_TX_WARNING :
> +                                               CAN_ERR_CRTL_RX_WARNING;

Nitpick: when a ternary operator is too long to fit on one line,
prefer an if/else.

>                                 priv->can.state = CAN_STATE_ERROR_WARNING;
>                                 priv->can.can_stats.error_warning++;
>                                 break;
>                         case ESD_BUSSTATE_ERRPASSIVE:
> +                               cf->can_id |= CAN_ERR_CRTL;
> +                               cf->data[1] = (txerr > rxerr) ?
> +                                               CAN_ERR_CRTL_TX_PASSIVE :
> +                                               CAN_ERR_CRTL_RX_PASSIVE;

Same.

>                                 priv->can.state = CAN_STATE_ERROR_PASSIVE;
>                                 priv->can.can_stats.error_passive++;
>                                 break;
> @@ -296,12 +304,6 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
>                         /* Bit stream position in CAN frame as the error was detected */
>                         cf->data[3] = ecc & SJA1000_ECC_SEG;
>
> -                       if (priv->can.state == CAN_STATE_ERROR_WARNING ||
> -                           priv->can.state == CAN_STATE_ERROR_PASSIVE) {
> -                               cf->data[1] = (txerr > rxerr) ?
> -                                       CAN_ERR_CRTL_TX_PASSIVE :
> -                                       CAN_ERR_CRTL_RX_PASSIVE;
> -                       }
>                         cf->data[6] = txerr;
>                         cf->data[7] = rxerr;
>                 }

Yours sincerely,
Vincent Mailhol

Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[Frank Jungclaus]

On Tue, 2022-12-20 at 14:49 +0900, Vincent MAILHOL wrote:
> On Tue. 20 Dec. 2022 at 06:29, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> > Started a rework initiated by Vincents remarks "You should not report
> > the greatest of txerr and rxerr but the one which actually increased."
> > [1]
> 
> I do not see this comment being addressed. You are still assigning the
> flags depending on the highest value, not the one which actually
> changed.


Yes, I'm assigning depending on the highest value, but from my point of
view doing so is analogue to what is done by can_change_state(). And
it should be fine, because e.g. my "case ESD_BUSSTATE_WARN:" is reached
exactly once while the transition from ERROR_ACTIVE to
ERROR_WARN. Than one of rec or tec is responsible for this
transition.
There is no second pass for "case ESD_BUSSTATE_WARN:"
when e.g. rec is already on WARN (or above) and now tec also reaches
WARN.
Man, this is even difficult to explain in German language ;)


> 
> > and "As far as I understand, those flags should be set only when
> > the threshold is *reached*" [2] .
> > 
> > Now setting the flags for CAN_ERR_CRTL_[RT]X_WARNING and
> > CAN_ERR_CRTL_[RT]X_PASSIVE regarding REC and TEC, when the
> > appropriate threshold is reached.
> > 
> > Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
> > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > Link: [1] https://lore.kernel.org/all/CAMZ6RqKGBWe15aMkf8-QLf-cOQg99GQBebSm+1wEzTqHgvmNuw@mail.gmail.com/
> > Link: [2] https://lore.kernel.org/all/CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRGWDZBS0fs7zbSTy4hmA@mail.gmail.com/
> > ---
> >  drivers/net/can/usb/esd_usb.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > index 5e182fadd875..09745751f168 100644
> > --- a/drivers/net/can/usb/esd_usb.c
> > +++ b/drivers/net/can/usb/esd_usb.c
> > @@ -255,10 +255,18 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> >                                 can_bus_off(priv->netdev);
> >                                 break;
> >                         case ESD_BUSSTATE_WARN:
> > +                               cf->can_id |= CAN_ERR_CRTL;
> > +                               cf->data[1] = (txerr > rxerr) ?
> > +                                               CAN_ERR_CRTL_TX_WARNING :
> > +                                               CAN_ERR_CRTL_RX_WARNING;
> 
> Nitpick: when a ternary operator is too long to fit on one line,
> prefer an if/else.

AFAIR line length up to 120 chars is tolerated nowadays. So putting
this on a single line might also be an option!(?)
How will this be handled in the CAN sub tree?


> 
> >                                 priv->can.state = CAN_STATE_ERROR_WARNING;
> >                                 priv->can.can_stats.error_warning++;
> >                                 break;
> >                         case ESD_BUSSTATE_ERRPASSIVE:
> > +                               cf->can_id |= CAN_ERR_CRTL;
> > +                               cf->data[1] = (txerr > rxerr) ?
> > +                                               CAN_ERR_CRTL_TX_PASSIVE :
> > +                                               CAN_ERR_CRTL_RX_PASSIVE;
> 
> Same.
> 
> >                                 priv->can.state = CAN_STATE_ERROR_PASSIVE;
> >                                 priv->can.can_stats.error_passive++;
> >                                 break;
> > @@ -296,12 +304,6 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> >                         /* Bit stream position in CAN frame as the error was detected */
> >                         cf->data[3] = ecc & SJA1000_ECC_SEG;
> > 
> > -                       if (priv->can.state == CAN_STATE_ERROR_WARNING ||
> > -                           priv->can.state == CAN_STATE_ERROR_PASSIVE) {
> > -                               cf->data[1] = (txerr > rxerr) ?
> > -                                       CAN_ERR_CRTL_TX_PASSIVE :
> > -                                       CAN_ERR_CRTL_RX_PASSIVE;
> > -                       }
> >                         cf->data[6] = txerr;
> >                         cf->data[7] = rxerr;
> >                 }
> 
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[Vincent MAILHOL]

On Thu. 22 Dec. 2022 at 03:42, Frank Jungclaus <Frank.Jungclaus@esd.eu> wrote:
> On Tue, 2022-12-20 at 14:49 +0900, Vincent MAILHOL wrote:
> > On Tue. 20 Dec. 2022 at 06:29, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> > > Started a rework initiated by Vincents remarks "You should not report
> > > the greatest of txerr and rxerr but the one which actually increased."
> > > [1]
> >
> > I do not see this comment being addressed. You are still assigning the
> > flags depending on the highest value, not the one which actually
> > changed.
>
>
> Yes, I'm assigning depending on the highest value, but from my point of
> view doing so is analogue to what is done by can_change_state().

On the surface, it may look similar. But if you look into details,
can_change_state() is only called when there is a change on enum
can_state. enum can_state is the global state and does not
differentiate the RX and TX.

I will give an example. Imagine that:

  - txerr is 128 (ERROR_PASSIVE)
  - rxerr is 95 (ERROR_ACTIVE)

Imagine that rxerr then increases to 96. If you call
can_change_state() under this condition, the old state:
can_priv->state is still equal to the new one: max(tx_state, rx_state)
and you would get the oops message:

  https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/dev.c#L100

So can_change_state() is indeed correct because it excludes the case
when the smallest err counter changed.

> And
> it should be fine, because e.g. my "case ESD_BUSSTATE_WARN:" is reached
> exactly once while the transition from ERROR_ACTIVE to
> ERROR_WARN. Than one of rec or tec is responsible for this
> transition.
> There is no second pass for "case ESD_BUSSTATE_WARN:"
> when e.g. rec is already on WARN (or above) and now tec also reaches
> WARN.
> Man, this is even difficult to explain in German language ;)

OK. This is new information. I agree that it should work. But I am
still puzzled because the code doesn't make this limitation apparent.

Also, as long as you have the rxerr and txerr value, you should still
be able to set the correct flag by comparing the err counters instead
of relying on your device events.

I am thinking of something like this:


  enum can_state can_get_state_from_err_cnt(u16 berr_counter)
  {
          if (berr_counter >= CAN_BUS_OFF_THRESHOLD)
                  return CAN_STATE_BUS_OFF;

          if (berr_counter >= CAN_ERROR_PASSIVE_THRESHOLD)
                  return CAN_STATE_ERROR_PASSIVE;

          if (berr_counter >= CAN_ERROR_WARNING_THRESHOLD)
                  return CAN_STATE_ERROR_WARNING;

          return CAN_STATE_ERROR_ACTIVE;
  }
  EXPORT_SYMBOL_GPL(can_get_state_from_err_cnt);

  void can_frame_set_error_status(struct net_device *dev, struct can_frame *cf,
                                  struct can_berr_counter *old_ctr,
                                  struct can_berr_counter *new_ctr)
  {
          enum can_state old_state, new_state;

          /* TX err cnt */
          old_state = can_get_state_from_err_cnt(old_ctr->txerr);
          new_state = can_get_state_from_err_cnt(new_ctr->txerr);
          if (old_state != new_state)
                  cf->data[1] |= can_tx_state_to_frame(dev, new_state);

          /* RX err cnt */
          old_state = can_get_state_from_err_cnt(old_ctr->rxerr);
          new_state = can_get_state_from_err_cnt(new_ctr->rxerr);
          if (old_state != new_state)
                  cf->data[1] |= can_rx_state_to_frame(dev, new_state);
  }
  EXPORT_SYMBOL_GPL(can_set_error_status);


If this looks good to you, I can put this in a patch (N.B. code not
tested! But should be enough for you to get the idea).

> >
> > > and "As far as I understand, those flags should be set only when
> > > the threshold is *reached*" [2] .
> > >
> > > Now setting the flags for CAN_ERR_CRTL_[RT]X_WARNING and
> > > CAN_ERR_CRTL_[RT]X_PASSIVE regarding REC and TEC, when the
> > > appropriate threshold is reached.
> > >
> > > Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
> > > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > > Link: [1] https://lore.kernel.org/all/CAMZ6RqKGBWe15aMkf8-QLf-cOQg99GQBebSm+1wEzTqHgvmNuw@mail.gmail.com/
> > > Link: [2] https://lore.kernel.org/all/CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRGWDZBS0fs7zbSTy4hmA@mail.gmail.com/
> > > ---
> > >  drivers/net/can/usb/esd_usb.c | 14 ++++++++------
> > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > > index 5e182fadd875..09745751f168 100644
> > > --- a/drivers/net/can/usb/esd_usb.c
> > > +++ b/drivers/net/can/usb/esd_usb.c
> > > @@ -255,10 +255,18 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > >                                 can_bus_off(priv->netdev);
> > >                                 break;
> > >                         case ESD_BUSSTATE_WARN:
> > > +                               cf->can_id |= CAN_ERR_CRTL;
> > > +                               cf->data[1] = (txerr > rxerr) ?
> > > +                                               CAN_ERR_CRTL_TX_WARNING :
> > > +                                               CAN_ERR_CRTL_RX_WARNING;
> >
> > Nitpick: when a ternary operator is too long to fit on one line,
> > prefer an if/else.
>
> AFAIR line length up to 120 chars is tolerated nowadays. So putting
> this on a single line might also be an option!(?)
> How will this be handled in the CAN sub tree?

Correct. The 120 is tolerated but the recommendation remains the 80
characters. I am not supportive of squeezing things and making this a
~120 characters line.

Also, this is a nitpick. I will not fight for you to change it. Just
be aware that there are often comments on the mailing list asking not
to use ternary operators (and I will not do an archivist job to find
such messages).

> >
> > >                                 priv->can.state = CAN_STATE_ERROR_WARNING;
> > >                                 priv->can.can_stats.error_warning++;
> > >                                 break;
> > >                         case ESD_BUSSTATE_ERRPASSIVE:
> > > +                               cf->can_id |= CAN_ERR_CRTL;
> > > +                               cf->data[1] = (txerr > rxerr) ?
> > > +                                               CAN_ERR_CRTL_TX_PASSIVE :
> > > +                                               CAN_ERR_CRTL_RX_PASSIVE;
> >
> > Same.
> >
> > >                                 priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > >                                 priv->can.can_stats.error_passive++;
> > >                                 break;
> > > @@ -296,12 +304,6 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > >                         /* Bit stream position in CAN frame as the error was detected */
> > >                         cf->data[3] = ecc & SJA1000_ECC_SEG;
> > >
> > > -                       if (priv->can.state == CAN_STATE_ERROR_WARNING ||
> > > -                           priv->can.state == CAN_STATE_ERROR_PASSIVE) {
> > > -                               cf->data[1] = (txerr > rxerr) ?
> > > -                                       CAN_ERR_CRTL_TX_PASSIVE :
> > > -                                       CAN_ERR_CRTL_RX_PASSIVE;
> > > -                       }
> > >                         cf->data[6] = txerr;
> > >                         cf->data[7] = rxerr;
> > >                 }
> >
> > Yours sincerely,
> > Vincent Mailhol

Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[Frank Jungclaus]

On Thu, 2022-12-22 at 11:21 +0900, Vincent MAILHOL wrote:
> On Thu. 22 Dec. 2022 at 03:42, Frank Jungclaus <Frank.Jungclaus@esd.eu> wrote:
> > On Tue, 2022-12-20 at 14:49 +0900, Vincent MAILHOL wrote:
> > > On Tue. 20 Dec. 2022 at 06:29, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> > > > Started a rework initiated by Vincents remarks "You should not report
> > > > the greatest of txerr and rxerr but the one which actually increased."
> > > > [1]
> > > 
> > > I do not see this comment being addressed. You are still assigning the
> > > flags depending on the highest value, not the one which actually
> > > changed.
> > 
> > 
> > Yes, I'm assigning depending on the highest value, but from my point of
> > view doing so is analogue to what is done by can_change_state().
> 
> On the surface, it may look similar. But if you look into details,
> can_change_state() is only called when there is a change on enum
> can_state. enum can_state is the global state and does not
> differentiate the RX and TX.
> 
> I will give an example. Imagine that:
> 
>   - txerr is 128 (ERROR_PASSIVE)
>   - rxerr is 95 (ERROR_ACTIVE)
> 
> Imagine that rxerr then increases to 96. If you call
> can_change_state() under this condition, the old state:
> can_priv->state is still equal to the new one: max(tx_state, rx_state)
> and you would get the oops message:
> 
>   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/dev.c#L100
> 
> So can_change_state() is indeed correct because it excludes the case
> when the smallest err counter changed.
> 
> > And
> > it should be fine, because e.g. my "case ESD_BUSSTATE_WARN:" is reached
> > exactly once while the transition from ERROR_ACTIVE to
> > ERROR_WARN. Than one of rec or tec is responsible for this
> > transition.
> > There is no second pass for "case ESD_BUSSTATE_WARN:"
> > when e.g. rec is already on WARN (or above) and now tec also reaches
> > WARN.
> > Man, this is even difficult to explain in German language ;)
> 
> OK. This is new information. I agree that it should work. But I am
> still puzzled because the code doesn't make this limitation apparent.
> 
> Also, as long as you have the rxerr and txerr value, you should still
> be able to set the correct flag by comparing the err counters instead
> of relying on your device events.
> 

I agree, this would be an option. But I dislike the fact that then
- beside the USB firmware - there is a second instance which decides on
the bus state. I'll send a reworked patch which makes use of
can_change_state(). Hopefully that will address your concerns ;) 
This also will fix the imperfection, that our current code e.g. does
an error_warning++ when going back in direction of ERROR_ACTIVE ...

> I am thinking of something like this:
> 
> 
>   enum can_state can_get_state_from_err_cnt(u16 berr_counter)
>   {
>           if (berr_counter >= CAN_BUS_OFF_THRESHOLD)
>                   return CAN_STATE_BUS_OFF;
> 
>           if (berr_counter >= CAN_ERROR_PASSIVE_THRESHOLD)
>                   return CAN_STATE_ERROR_PASSIVE;
> 
>           if (berr_counter >= CAN_ERROR_WARNING_THRESHOLD)
>                   return CAN_STATE_ERROR_WARNING;
> 
>           return CAN_STATE_ERROR_ACTIVE;
>   }
>   EXPORT_SYMBOL_GPL(can_get_state_from_err_cnt);
> 
>   void can_frame_set_error_status(struct net_device *dev, struct can_frame *cf,
>                                   struct can_berr_counter *old_ctr,
>                                   struct can_berr_counter *new_ctr)
>   {
>           enum can_state old_state, new_state;
> 
>           /* TX err cnt */
>           old_state = can_get_state_from_err_cnt(old_ctr->txerr);
>           new_state = can_get_state_from_err_cnt(new_ctr->txerr);
>           if (old_state != new_state)
>                   cf->data[1] |= can_tx_state_to_frame(dev, new_state);
> 
>           /* RX err cnt */
>           old_state = can_get_state_from_err_cnt(old_ctr->rxerr);
>           new_state = can_get_state_from_err_cnt(new_ctr->rxerr);
>           if (old_state != new_state)
>                   cf->data[1] |= can_rx_state_to_frame(dev, new_state);
>   }
>   EXPORT_SYMBOL_GPL(can_set_error_status);
> 
> 
> If this looks good to you, I can put this in a patch (N.B. code not
> tested! But should be enough for you to get the idea).
> 
> > > 
> > > > and "As far as I understand, those flags should be set only when
> > > > the threshold is *reached*" [2] .
> > > > 
> > > > Now setting the flags for CAN_ERR_CRTL_[RT]X_WARNING and
> > > > CAN_ERR_CRTL_[RT]X_PASSIVE regarding REC and TEC, when the
> > > > appropriate threshold is reached.
> > > > 
> > > > Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
> > > > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > > > Link: [1] https://lore.kernel.org/all/CAMZ6RqKGBWe15aMkf8-QLf-cOQg99GQBebSm+1wEzTqHgvmNuw@mail.gmail.com/
> > > > Link: [2] https://lore.kernel.org/all/CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRGWDZBS0fs7zbSTy4hmA@mail.gmail.com/
> > > > ---
> > > >  drivers/net/can/usb/esd_usb.c | 14 ++++++++------
> > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > > > index 5e182fadd875..09745751f168 100644
> > > > --- a/drivers/net/can/usb/esd_usb.c
> > > > +++ b/drivers/net/can/usb/esd_usb.c
> > > > @@ -255,10 +255,18 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > > >                                 can_bus_off(priv->netdev);
> > > >                                 break;
> > > >                         case ESD_BUSSTATE_WARN:
> > > > +                               cf->can_id |= CAN_ERR_CRTL;
> > > > +                               cf->data[1] = (txerr > rxerr) ?
> > > > +                                               CAN_ERR_CRTL_TX_WARNING :
> > > > +                                               CAN_ERR_CRTL_RX_WARNING;
> > > 
> > > Nitpick: when a ternary operator is too long to fit on one line,
> > > prefer an if/else.
> > 
> > AFAIR line length up to 120 chars is tolerated nowadays. So putting
> > this on a single line might also be an option!(?)
> > How will this be handled in the CAN sub tree?
> 
> Correct. The 120 is tolerated but the recommendation remains the 80
> characters. I am not supportive of squeezing things and making this a
> ~120 characters line.
> 
> Also, this is a nitpick. I will not fight for you to change it. Just
> be aware that there are often comments on the mailing list asking not
> to use ternary operators (and I will not do an archivist job to find
> such messages).
> 
> > > 
> > > >                                 priv->can.state = CAN_STATE_ERROR_WARNING;
> > > >                                 priv->can.can_stats.error_warning++;
> > > >                                 break;
> > > >                         case ESD_BUSSTATE_ERRPASSIVE:
> > > > +                               cf->can_id |= CAN_ERR_CRTL;
> > > > +                               cf->data[1] = (txerr > rxerr) ?
> > > > +                                               CAN_ERR_CRTL_TX_PASSIVE :
> > > > +                                               CAN_ERR_CRTL_RX_PASSIVE;
> > > 
> > > Same.
> > > 
> > > >                                 priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > > >                                 priv->can.can_stats.error_passive++;
> > > >                                 break;
> > > > @@ -296,12 +304,6 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> > > >                         /* Bit stream position in CAN frame as the error was detected */
> > > >                         cf->data[3] = ecc & SJA1000_ECC_SEG;
> > > > 
> > > > -                       if (priv->can.state == CAN_STATE_ERROR_WARNING ||
> > > > -                           priv->can.state == CAN_STATE_ERROR_PASSIVE) {
> > > > -                               cf->data[1] = (txerr > rxerr) ?
> > > > -                                       CAN_ERR_CRTL_TX_PASSIVE :
> > > > -                                       CAN_ERR_CRTL_RX_PASSIVE;
> > > > -                       }
> > > >                         cf->data[6] = txerr;
> > > >                         cf->data[7] = rxerr;
> > > >                 }
> > > 
> > > Yours sincerely,
> > > Vincent Mailhol

Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[Marc Kleine-Budde]

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

On 23.01.2023 15:47:22, Frank Jungclaus wrote:
> On Thu, 2022-12-22 at 11:21 +0900, Vincent MAILHOL wrote:
> > On Thu. 22 Dec. 2022 at 03:42, Frank Jungclaus <Frank.Jungclaus@esd.eu> wrote:
> > > On Tue, 2022-12-20 at 14:49 +0900, Vincent MAILHOL wrote:
> > > > On Tue. 20 Dec. 2022 at 06:29, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> > > > > Started a rework initiated by Vincents remarks "You should not report
> > > > > the greatest of txerr and rxerr but the one which actually increased."
> > > > > [1]
> > > > 
> > > > I do not see this comment being addressed. You are still assigning the
> > > > flags depending on the highest value, not the one which actually
> > > > changed.
> > > 
> > > 
> > > Yes, I'm assigning depending on the highest value, but from my point of
> > > view doing so is analogue to what is done by can_change_state().
> > 
> > On the surface, it may look similar. But if you look into details,
> > can_change_state() is only called when there is a change on enum
> > can_state. enum can_state is the global state and does not
> > differentiate the RX and TX.
> > 
> > I will give an example. Imagine that:
> > 
> >   - txerr is 128 (ERROR_PASSIVE)
> >   - rxerr is 95 (ERROR_ACTIVE)
> > 
> > Imagine that rxerr then increases to 96. If you call
> > can_change_state() under this condition, the old state:
> > can_priv->state is still equal to the new one: max(tx_state, rx_state)
> > and you would get the oops message:
> > 
> >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/dev.c#L100
> > 
> > So can_change_state() is indeed correct because it excludes the case
> > when the smallest err counter changed.
> > 
> > > And
> > > it should be fine, because e.g. my "case ESD_BUSSTATE_WARN:" is reached
> > > exactly once while the transition from ERROR_ACTIVE to
> > > ERROR_WARN. Than one of rec or tec is responsible for this
> > > transition.
> > > There is no second pass for "case ESD_BUSSTATE_WARN:"
> > > when e.g. rec is already on WARN (or above) and now tec also reaches
> > > WARN.
> > > Man, this is even difficult to explain in German language ;)
> > 
> > OK. This is new information. I agree that it should work. But I am
> > still puzzled because the code doesn't make this limitation apparent.
> > 
> > Also, as long as you have the rxerr and txerr value, you should still
> > be able to set the correct flag by comparing the err counters instead
> > of relying on your device events.
> > 
> 
> I agree, this would be an option. But I dislike the fact that then
> - beside the USB firmware - there is a second instance which decides on
> the bus state. I'll send a reworked patch which makes use of
                      ^^^^^^^^^^^^^^^^^^^^^
> can_change_state(). Hopefully that will address your concerns ;)
> This also will fix the imperfection, that our current code e.g. does
> an error_warning++ when going back in direction of ERROR_ACTIVE ...

Not taking this series, waiting for the reworked version.

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

Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[Frank Jungclaus]

On Thu, 2023-02-02 at 16:22 +0100, Marc Kleine-Budde wrote:
> On 23.01.2023 15:47:22, Frank Jungclaus wrote:
> > On Thu, 2022-12-22 at 11:21 +0900, Vincent MAILHOL wrote:
> > > On Thu. 22 Dec. 2022 at 03:42, Frank Jungclaus <Frank.Jungclaus@esd.eu> wrote:
> > > > On Tue, 2022-12-20 at 14:49 +0900, Vincent MAILHOL wrote:
> > > > > On Tue. 20 Dec. 2022 at 06:29, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> > > > > > Started a rework initiated by Vincents remarks "You should not report
> > > > > > the greatest of txerr and rxerr but the one which actually increased."
> > > > > > [1]
> > > > > 
> > > > > I do not see this comment being addressed. You are still assigning the
> > > > > flags depending on the highest value, not the one which actually
> > > > > changed.
> > > > 
> > > > 
> > > > Yes, I'm assigning depending on the highest value, but from my point of
> > > > view doing so is analogue to what is done by can_change_state().
> > > 
> > > On the surface, it may look similar. But if you look into details,
> > > can_change_state() is only called when there is a change on enum
> > > can_state. enum can_state is the global state and does not
> > > differentiate the RX and TX.
> > > 
> > > I will give an example. Imagine that:
> > > 
> > >   - txerr is 128 (ERROR_PASSIVE)
> > >   - rxerr is 95 (ERROR_ACTIVE)
> > > 
> > > Imagine that rxerr then increases to 96. If you call
> > > can_change_state() under this condition, the old state:
> > > can_priv->state is still equal to the new one: max(tx_state, rx_state)
> > > and you would get the oops message:
> > > 
> > >   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/dev.c#L100
> > > 
> > > So can_change_state() is indeed correct because it excludes the case
> > > when the smallest err counter changed.
> > > 
> > > > And
> > > > it should be fine, because e.g. my "case ESD_BUSSTATE_WARN:" is reached
> > > > exactly once while the transition from ERROR_ACTIVE to
> > > > ERROR_WARN. Than one of rec or tec is responsible for this
> > > > transition.
> > > > There is no second pass for "case ESD_BUSSTATE_WARN:"
> > > > when e.g. rec is already on WARN (or above) and now tec also reaches
> > > > WARN.
> > > > Man, this is even difficult to explain in German language ;)
> > > 
> > > OK. This is new information. I agree that it should work. But I am
> > > still puzzled because the code doesn't make this limitation apparent.
> > > 
> > > Also, as long as you have the rxerr and txerr value, you should still
> > > be able to set the correct flag by comparing the err counters instead
> > > of relying on your device events.
> > > 
> > 
> > I agree, this would be an option. But I dislike the fact that then
> > - beside the USB firmware - there is a second instance which decides on
> > the bus state. I'll send a reworked patch which makes use of
>                       ^^^^^^^^^^^^^^^^^^^^^
> > can_change_state(). Hopefully that will address your concerns ;)
> > This also will fix the imperfection, that our current code e.g. does
> > an error_warning++ when going back in direction of ERROR_ACTIVE ...
> 
> Not taking this series, waiting for the reworked version.
> 
> Marc
> 
Marc, can I just send a reworked patch of [PATCH 2/3], let's say
with subject [PATCH v2 2/3] as a reply to this thread or should I
better resend the complete patch series as [PATCH v2 0/3] up to
[PATCH v2 3/3]?

Regards Frank

Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT event (2)

[Marc Kleine-Budde]

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

On 09.02.2023 19:00:54, Frank Jungclaus wrote:
> > Not taking this series, waiting for the reworked version.
> > 
> > Marc
> > 
> Marc, can I just send a reworked patch of [PATCH 2/3], let's say
> with subject [PATCH v2 2/3] as a reply to this thread or should I
> better resend the complete patch series as [PATCH v2 0/3] up to
> [PATCH v2 3/3]?

Just re-post the whole series. Complete series are easier to handle.

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