linux-kernel - Re: [PATCH RESEND 1/1] can: esd_usb: Allow REC and TEC to return to zero

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]

Date:   Tue, 29 Nov 2022 17:15:56 +0000
From:   Frank Jungclaus <Frank.Jungclaus@....eu>
To:     "mkl@...gutronix.de" <mkl@...gutronix.de>
CC:     Stefan Mätje <Stefan.Maetje@....eu>,
        "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
        "wg@...ndegger.com" <wg@...ndegger.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mailhol.vincent@...adoo.fr" <mailhol.vincent@...adoo.fr>
Subject: Re: [PATCH RESEND 1/1] can: esd_usb: Allow REC and TEC to return to
 zero

Hello Marc,
thanks for commenting.

On Fri, 2022-11-25 at 16:56 +0100, Marc Kleine-Budde wrote:
> On 24.11.2022 21:38:06, Frank Jungclaus wrote:
> > We don't get any further EVENT from an esd CAN USB device for changes
> > on REC or TEC while those counters converge to 0 (with ecc == 0).
> > So when handling the "Back to Error Active"-event force
> > txerr = rxerr = 0, otherwise the berr-counters might stay on
> > values like 95 forever ...
> > 
> > Also, to make life easier during the ongoing development a
> > netdev_dbg() has been introduced to allow dumping error events send by
> > an esd CAN USB device.
> > 
> > Signed-off-by: Frank Jungclaus <frank.jungclaus@....eu>
> 
> Please add a Fixes tag.
> 
> https://elixir.bootlin.com/linux/v6.0/source/Documentation/process/handling-regressions.rst#L107
> 
>From my point of view this is not a regression, it's a sort of
imperfection existing since the initial add of esd_usb(2).c to the
kernel. So should I add a "Fixes:" referring to the initial commit?
(Currently) I'm slow on the uptake ;)

> > ---
> >  drivers/net/can/usb/esd_usb.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > index 1bcfad11b1e4..da24c649aadd 100644
> > --- a/drivers/net/can/usb/esd_usb.c
> > +++ b/drivers/net/can/usb/esd_usb.c
> > @@ -230,10 +230,14 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> >  
> >  	if (id == ESD_EV_CAN_ERROR_EXT) {
> >  		u8 state = msg->msg.rx.data[0];
> > -		u8 ecc = msg->msg.rx.data[1];
> > +		u8 ecc   = msg->msg.rx.data[1];
> 
> unrelated, please remove.
> 
You're right, sorry, will be removed in v2 ...

> >  		u8 rxerr = msg->msg.rx.data[2];
> >  		u8 txerr = msg->msg.rx.data[3];
> >  
> > +		netdev_dbg(priv->netdev,
> > +			   "CAN_ERR_EV_EXT: dlc=%#02x state=%02x ecc=%02x rec=%02x tec=%02x\n",
> > +			   msg->msg.rx.dlc, state, ecc, rxerr, txerr);
> > +
> >  		skb = alloc_can_err_skb(priv->netdev, &cf);
> >  		if (skb == NULL) {
> >  			stats->rx_dropped++;
> > @@ -260,6 +264,8 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> >  				break;
> >  			default:
> >  				priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +				txerr = 0;
> > +				rxerr = 0;
> >  				break;
> >  			}
> >  		} else {
> 
> regards,
> Marc
> 
Regards, Frank