Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Frank Jungclaus <Frank.Jungclaus@esd.eu>
To:     "mailhol.vincent@wanadoo.fr" <mailhol.vincent@wanadoo.fr>
CC:     =?iso-8859-15?Q?Stefan_M=E4tje?= <Stefan.Maetje@esd.eu>,
        "linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
        "mkl@pengutronix.de" <mkl@pengutronix.de>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "wg@grandegger.com" <wg@grandegger.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT
 event (2)
Thread-Topic: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT
 event (2)
Thread-Index: AQHZE/C1wQWkeZ5g50Kutho7W0eaTK52RcWAgAJmpAA=
Date:   Wed, 21 Dec 2022 18:29:23 +0000
Message-ID: <a1d253bacdf296947a45fb069a0fd64eabb7e117.camel@esd.eu>
References: <20221219212717.1298282-1-frank.jungclaus@esd.eu>
         <CAMZ6RqKAmrgQUKLehUZx+hiSk3jD+o44uGtzrRFk+RBk8Bt81A@mail.gmail.com>
In-Reply-To: <CAMZ6RqKAmrgQUKLehUZx+hiSk3jD+o44uGtzrRFk+RBk8Bt81A@mail.gmail.com>
Accept-Language: en-001, de-DE, en-US
Content-Language: en-US
user-agent: Evolution 3.44.4-0ubuntu1 
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-15?Q?W5tcNt3qaEkmoK7ou/WAmJgjy33SjG9/2xebiE+WZdSGAAnja/c8zefjL?=
 =?iso-8859-15?Q?yduu6M7BDxbgmbrX+TrZNuPxacGIw8CUgQdanZPMY7o5xBnDUccoJbsPE?=
 =?iso-8859-15?Q?Ifo9ShFFno6gK+dedbkgGYn814znR4+EGNFuTBunmnmKBkVxDw6gN3+B4?=
 =?iso-8859-15?Q?iK9DzLVF2AQD3+t63Kym3PdNkJPyH8dgArkV2gVyKo6cLxNV/+vcuRfMG?=
 =?iso-8859-15?Q?YRMxorFYQoFcSlfY1MUKYLoNXFuvVd9aeEIFo0s1qCZpHp25szw8gnF7m?=
 =?iso-8859-15?Q?bR2OBhzEiICGWf9maeJG0PHJzrfCyXJrXaFkCPtmrR8JZ/qIb3/nPw6vz?=
 =?iso-8859-15?Q?XFDAOhQk96Nt9Wy+3LCPOP2r8OES7eJ/OokXfkKw6hDAjF/P1RbiXQXFL?=
 =?iso-8859-15?Q?Xem8DUI4d0kKb50BorCozeIH5TKq7kfMhasyAX0y1k3sCC6zhWi4mcRMS?=
 =?iso-8859-15?Q?2mcxSCzEqc+xM308fGgFV529fdAdMxnnomOXTOl4ZbJKvc/vDDBj+7FIw?=
 =?iso-8859-15?Q?/EIIcyoumow9zCmlxJJz2t8/T9vaQJxcVdBuXjfjOxY/F6Nob2TOLahLK?=
 =?iso-8859-15?Q?hf7c1VE2iReGUUJP1gDp92GfMhcUFnUrlTty/8GWMn5Xy1akPYUn9qyA9?=
 =?iso-8859-15?Q?ixBPl1xX3TDVKJVTGnhYEz7NNjPia3sv1pbKO5avAfUBFGPmgiB46/fbT?=
 =?iso-8859-15?Q?w6pTM9hlCf9I53yhz5O8HvpVo9Sh22HC0iUfpO4T97I9eZ79UgQHexIY7?=
 =?iso-8859-15?Q?QC9WNmGHLBlOv5MIVK9GhgixcjZjTiRJtPc3bAHRyWb+t2VMPwiKpeUaB?=
 =?iso-8859-15?Q?r4UbryJ0Jj+hlibrjrmx0Vk3FefMC86jLoifKXGXmoamKxggzWCgskWDE?=
 =?iso-8859-15?Q?KogoO8g3bZaXLJvfq9Oc6gz208jTel+ujz2nmgPC0Zi2zAANM1o5O2adM?=
 =?iso-8859-15?Q?oHmGrp7IFOmu5hqt7ysTkLLXJWqVsf4FzBmZkDYzpn27OtJHozNXgdvH2?=
 =?iso-8859-15?Q?J+WSbA2HgppXrhwyXJMwqFPrIiZBe54lDQfQuCPeb7klqYVp01qTsTmHF?=
 =?iso-8859-15?Q?nm9jUMYwqP7wlDoNqHRm25MhE6ETakw6S3zMP/8rTXSxpWPAmqG92sczC?=
 =?iso-8859-15?Q?JxGf+zp1vKeGvYx19zJBV6fKNEeVOLkyM1mMWBGWD3K19J6emF0kJYInl?=
 =?iso-8859-15?Q?g7eiicEAgAcHpUv2IJxbt9p4nS3kaisqyqdL0OWW7EUt3ULmHhOvHV0Pw?=
 =?iso-8859-15?Q?n873y3zmj374Rg6ycad+tHdunNxvbPjMpPOBzjf8yEFPSmfEOuRusr2C6?=
 =?iso-8859-15?Q?2V8baRuBZ3h7B1lH9f2JOfLCppGkE9vBRREagfTitLkT4M9MZ/ldMgyNI?=
 =?iso-8859-15?Q?IND6ysu7mqj4/GbeW4Qy+hXJAaJaEFWI4DczjPP4Knq0W0TQknilzg+Yg?=
 =?iso-8859-15?Q?zwnP/EYHmceOquPoM4SZg5QBqe6938qgwo9djcgxdNvhsyXuUZNkSttnw?=
 =?iso-8859-15?Q?Q54EUGA8xhVBQBhBaOucqneSHnyDHPVVQ+W73rN9XnmDoONQESLh0MmYe?=
 =?iso-8859-15?Q?4CwsdtJPfVkS0+Io9JP+HiDIy9lRQVsdN8+mMHyM+LupMsguDlSLCAQtz?=
 =?iso-8859-15?Q?Gs9+meuxmfPmQrZ11cKKAkV2Z0DNBGrDO9xlJnvkeQrgKWvQxPrxqas5T?=
 =?iso-8859-15?Q?Zs+UC/JyN7jOs3EO9TFoyfnABg=3D=3D?=
Content-Type: text/plain; charset="iso-8859-15"
Content-ID: <82DF22B19F378845BF77170533C919A9@eurprd03.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: esd.eu
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: GVXPR03MB8426.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d50e37ee-5386-47e9-75f3-08dae38148ca
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Dec 2022 18:29:23.7548
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 5a9c3a1d-52db-4235-b74c-9fd851db2e6b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: a4e3GlhtOQDZmy8DF/ktfituX78yjSARkIWwGgIfCiLS656KQFHGeVe7ShjXVl4sjx/kpX1xoWXOJ6T7eHZfUw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS2PR03MB9648
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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> w=
rote:
> > Started a rework initiated by Vincents remarks "You should not report
> > the greatest of txerr and rxerr but the one which actually increased."
> > [1]
>=20
> 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 ;)


>=20
> > and "As far as I understand, those flags should be set only when
> > the threshold is *reached*" [2] .
> >=20
> > 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.
> >=20
> > 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-cOQg99GQB=
ebSm+1wEzTqHgvmNuw@mail.gmail.com/
> > Link: [2] https://lore.kernel.org/all/CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRG=
WDZBS0fs7zbSTy4hmA@mail.gmail.com/
> > ---
> >  drivers/net/can/usb/esd_usb.c | 14 ++++++++------
> >  1 file changed, 8 insertions(+), 6 deletions(-)
> >=20
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_us=
b.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_p=
riv *priv,
> >                                 can_bus_off(priv->netdev);
> >                                 break;
> >                         case ESD_BUSSTATE_WARN:
> > +                               cf->can_id |=3D CAN_ERR_CRTL;
> > +                               cf->data[1] =3D (txerr > rxerr) ?
> > +                                               CAN_ERR_CRTL_TX_WARNING=
 :
> > +                                               CAN_ERR_CRTL_RX_WARNING=
;
>=20
> 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?


>=20
> >                                 priv->can.state =3D CAN_STATE_ERROR_WAR=
NING;
> >                                 priv->can.can_stats.error_warning++;
> >                                 break;
> >                         case ESD_BUSSTATE_ERRPASSIVE:
> > +                               cf->can_id |=3D CAN_ERR_CRTL;
> > +                               cf->data[1] =3D (txerr > rxerr) ?
> > +                                               CAN_ERR_CRTL_TX_PASSIVE=
 :
> > +                                               CAN_ERR_CRTL_RX_PASSIVE=
;
>=20
> Same.
>=20
> >                                 priv->can.state =3D CAN_STATE_ERROR_PAS=
SIVE;
> >                                 priv->can.can_stats.error_passive++;
> >                                 break;
> > @@ -296,12 +304,6 @@ static void esd_usb_rx_event(struct esd_usb_net_pr=
iv *priv,
> >                         /* Bit stream position in CAN frame as the erro=
r was detected */
> >                         cf->data[3] =3D ecc & SJA1000_ECC_SEG;
> >=20
> > -                       if (priv->can.state =3D=3D CAN_STATE_ERROR_WARN=
ING ||
> > -                           priv->can.state =3D=3D CAN_STATE_ERROR_PASS=
IVE) {
> > -                               cf->data[1] =3D (txerr > rxerr) ?
> > -                                       CAN_ERR_CRTL_TX_PASSIVE :
> > -                                       CAN_ERR_CRTL_RX_PASSIVE;
> > -                       }
> >                         cf->data[6] =3D txerr;
> >                         cf->data[7] =3D rxerr;
> >                 }
>=20
> Yours sincerely,
> Vincent Mailhol