| Message ID | 20220708181235.4104943-5-frank.jungclaus@esd.eu (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | More preparation for supporting esd CAN-USB/3 | expand |
On Tue. 9 Jul. 2022 at 03:15, Frank Jungclaus <frank.jungclaus@esd.eu> wrote: > Started a rework initiated by Vincents remark about "You should not > report the greatest of txerr and rxerr but the one which actually > increased." Now setting CAN_ERR_CRTL_[RT]X_WARNING and > CAN_ERR_CRTL_[RT]X_PASSIVE depending on REC and TEC > > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu> > --- > drivers/net/can/usb/esd_usb.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c > index 0a402a23d7ac..588caba1453b 100644 > --- a/drivers/net/can/usb/esd_usb.c > +++ b/drivers/net/can/usb/esd_usb.c > @@ -304,11 +304,17 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv, > /* Store error in CAN protocol (location) in data[3] */ > 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; > + /* Store error status of CAN-controller in data[1] */ > + if (priv->can.state == CAN_STATE_ERROR_WARNING) { > + if (txerr >= 96) > + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING; As far as I understand, those flags should be set only when the threshold is *reached*: https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can/error.h#L69 I don't think you should set it if the error state does not change. Here, you probably want to compare the new value with the previous one (stored in struct can_berr_counter) to decide whether or not the flags should be set. > + if (rxerr >= 96) > + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING; > + } else if (priv->can.state == CAN_STATE_ERROR_PASSIVE) { > + if (txerr >= 128) > + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE; > + if (rxerr >= 128) > + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE; > } > > cf->data[6] = txerr; > -- > 2.25.1 >
On Tue, 2022-07-12 at 23:39 +0900, Vincent MAILHOL wrote: > On Tue. 9 Jul. 2022 at 03:15, Frank Jungclaus <frank.jungclaus@esd.eu> wrote: > > Started a rework initiated by Vincents remark about "You should not > > report the greatest of txerr and rxerr but the one which actually > > increased." Now setting CAN_ERR_CRTL_[RT]X_WARNING and > > CAN_ERR_CRTL_[RT]X_PASSIVE depending on REC and TEC > > > > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu> > > --- > > drivers/net/can/usb/esd_usb.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c > > index 0a402a23d7ac..588caba1453b 100644 > > --- a/drivers/net/can/usb/esd_usb.c > > +++ b/drivers/net/can/usb/esd_usb.c > > @@ -304,11 +304,17 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv, > > /* Store error in CAN protocol (location) in data[3] */ > > 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; > > + /* Store error status of CAN-controller in data[1] */ > > + if (priv->can.state == CAN_STATE_ERROR_WARNING) { > > + if (txerr >= 96) > > + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING; > > As far as I understand, those flags should be set only when the > threshold is *reached*: > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can/error.h#L69 > > I don't think you should set it if the error state does not change. > > Here, you probably want to compare the new value with the previous > one (stored in struct can_berr_counter) to decide whether or not the > flags should be set. Hi Vincent, I didn't interpret the comments given to data[1] in error.h in that way (obviously). But after checking some other drivers, I see they all seem to handle it the way you proposed it ... So, I'll try to rework and resend patch 4/6, too. Best regards, Frank > > > > + if (rxerr >= 96) > > + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING; > > + } else if (priv->can.state == CAN_STATE_ERROR_PASSIVE) { > > + if (txerr >= 128) > > + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE; > > + if (rxerr >= 128) > > + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE; > > } > > > > cf->data[6] = txerr; > > -- > > 2.25.1 > >
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c index 0a402a23d7ac..588caba1453b 100644 --- a/drivers/net/can/usb/esd_usb.c +++ b/drivers/net/can/usb/esd_usb.c @@ -304,11 +304,17 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv, /* Store error in CAN protocol (location) in data[3] */ 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; + /* Store error status of CAN-controller in data[1] */ + if (priv->can.state == CAN_STATE_ERROR_WARNING) { + if (txerr >= 96) + cf->data[1] |= CAN_ERR_CRTL_TX_WARNING; + if (rxerr >= 96) + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING; + } else if (priv->can.state == CAN_STATE_ERROR_PASSIVE) { + if (txerr >= 128) + cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE; + if (rxerr >= 128) + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE; } cf->data[6] = txerr;
Started a rework initiated by Vincents remark about "You should not report the greatest of txerr and rxerr but the one which actually increased." Now setting CAN_ERR_CRTL_[RT]X_WARNING and CAN_ERR_CRTL_[RT]X_PASSIVE depending on REC and TEC Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu> --- drivers/net/can/usb/esd_usb.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)