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 4/6] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT
 event (3)
Thread-Topic: [PATCH 4/6] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT
 event (3)
Thread-Index: AQHYkvZ8KbGCPl11OkK12FCtHi0K+q161HMAgAAYz4A=
Date:   Tue, 12 Jul 2022 16:08:16 +0000
Message-ID: <ec95c96b761df49ae19ff333aad9857c5ec498e0.camel@esd.eu>
References: <20220708181235.4104943-1-frank.jungclaus@esd.eu>
         <20220708181235.4104943-5-frank.jungclaus@esd.eu>
         <CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRGWDZBS0fs7zbSTy4hmA@mail.gmail.com>
In-Reply-To: <CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRGWDZBS0fs7zbSTy4hmA@mail.gmail.com>
Accept-Language: en-001, de-DE, en-US
Content-Language: en-US
user-agent: Evolution 3.36.5-0ubuntu1 
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-15?Q?UrF+KVzUT1655OsS/vPutfhjmM6+zxYnlZXJjtSHhIy+P/NtX+DHNWrTT?=
 =?iso-8859-15?Q?GeZarq1w3NwKHxR6AcmUgCSVQOkJn+yaqjT1UFF4KdwBKMG6InoltrTcP?=
 =?iso-8859-15?Q?kBvL5OqJOi80GJHl4CYBVfKhh1DwVJt0aB/sbwZTq140QT7sWpDussjvo?=
 =?iso-8859-15?Q?2x6ZyKGOk9sj4k4dQ9S7f65P/ZOe8wN565lk58aWvP6apFRPT9pQboHug?=
 =?iso-8859-15?Q?VOhjE4wzi+Hx7Pyr3omfHxbjOwQBhZJmmIyr3mEl6lpKcvpLrrUfQe3yq?=
 =?iso-8859-15?Q?66nOcxZQWpash05ZvIHqX9LwcD0huilCqBlfa8SC57ermf5JN0KxSrgUg?=
 =?iso-8859-15?Q?oxi+8eZpUPlEG4QQb4mKjHjeDlPaXsRmr3WzOVPBD1Evuth/GZetkbg6e?=
 =?iso-8859-15?Q?4cqmXcZwkFvKtZIaKC/LjK6CdqvAFVqfaay1VIL6OyioUMPZaMsAB/h3F?=
 =?iso-8859-15?Q?g3Pfox/mOW8wmpUM+63QIpCoJKeVP58JeWkpQ/DQirR5QtHmBr45aC4YM?=
 =?iso-8859-15?Q?C/1MNNwXsBIURGmE5agGkaCvUL9joLuKvaAJAB8/85IPHxhJUz1B3mJSN?=
 =?iso-8859-15?Q?T1D3k6vdqI4ab6LbMzV4VkJvr+WOuWg3bh0evTMZgYGcnmHLKLR49AY6S?=
 =?iso-8859-15?Q?saeWUPn0XJ+4HBv4O30s5JbLbnF55z7SZvMU61gznTzG4DBSxCB2zWIvs?=
 =?iso-8859-15?Q?1027CuJmF5GwDabRMjQ4XvZN5cWkbP9D7NUZlGO4ovEM1gR/RXQGtGKwt?=
 =?iso-8859-15?Q?1aca/3ErlhZmMVvbkpM+s1hVeG6qPxvy66ltCe4VsdEKU/gMW+bXZM4Zk?=
 =?iso-8859-15?Q?3zL43AO8vlbBLFgqT894ZcOxx90bNmuWfxHsNwbMt47jw2DHe6aOrt20u?=
 =?iso-8859-15?Q?A771VTy18Vl1+T91ua/61T65UD/W/EuINdpmnX4AJvf2IyZJMkyzp8eAy?=
 =?iso-8859-15?Q?PF1gmyuZrEeU1/y0YiIxZZPUJ8U8IvqlAlEop+czpzlf5jKEguLLoSGwa?=
 =?iso-8859-15?Q?FZlr/Id1GvCn4wapVLDlrvX82XI21zNVKngOLedY+iMCFIWfJ1rEqexJI?=
 =?iso-8859-15?Q?BaGOJbeX/NNXeZKEht9l7wkRVFyje9jylLxlVCih9evdk2nmMpKrJ1XS/?=
 =?iso-8859-15?Q?2vJQHkiwy5sVYATqVvy8IXX+1tAb34M8/XDljSWGugRFX9jRldh+tPWbM?=
 =?iso-8859-15?Q?EIovh8ElnuyGQ6jNEDeXM7md6ONk2ZpEyt1R2aluPhEa62ybbpuODJsU1?=
 =?iso-8859-15?Q?z3t6ETbzuWm70tt9mIws5SIVzezfRfbu3xMr8IuUXlOrnAwwIz7tiwb2R?=
 =?iso-8859-15?Q?BsxHQZP2IEt7O4TV+KAf5O+JKmqt7BQrj/MKazt1C7xwjE89+KGi58EDA?=
 =?iso-8859-15?Q?fwzyh1OQmKDIRMfl99I12rVGf8610mJG8aoM54qQGjgN7G8TodMMwbcdR?=
 =?iso-8859-15?Q?YEpRDzTKYNg1DQuJCwUhjBiVQPY0Mdjhc7saHZH62b+PA9BkERxouuHqt?=
 =?iso-8859-15?Q?snBUAc+A+4qUphUpdDYx5XPUm0XsDO2HMzCJ/7LVZL+AuVoigTL5N3ChF?=
 =?iso-8859-15?Q?M+5WPhPB3ofk0ZtpBviVVdiFe1mbFn9aOD5qIWmiFN68SyVKqIIQWWsHQ?=
 =?iso-8859-15?Q?7uPkY7Sj3A38UrjlC55CvW7yHjpNCegqDazpp/TPQoWl89/uO8LeSqlnt?=
 =?iso-8859-15?Q?ymnDLlTOI+pcHUrRvab/JiAeqQ=3D=3D?=
Content-Type: text/plain; charset="iso-8859-15"
Content-ID: <3A87BDDDCB67374D97058DE907675E99@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: 5585e240-abcb-45dd-eed8-08da6420bb24
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jul 2022 16:08:16.7524
 (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: 6ePjwdwJv6owaOdgY6L4eSY5gesL1W8Hl4tvvB9ERnBVbqLkQ8ZvppfeGdp9+88fyjYoma1sKBCKXmQALIElAQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR03MB7466
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,
        T_SCC_BODY_TEXT_LINE 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-07-12 at 23:39 +0900, Vincent MAILHOL wrote:
> On Tue. 9 Jul. 2022 at 03:15, Frank Jungclaus <frank.jungclaus@esd.eu> wr=
ote:
> > 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
> >=20
> > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > ---
> >  drivers/net/can/usb/esd_usb.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
> >=20
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_us=
b.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_p=
riv *priv,
> >                         /* Store error in CAN protocol (location) in da=
ta[3] */
> >                         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;
> > +                       /* Store error status of CAN-controller in data=
[1] */
> > +                       if (priv->can.state =3D=3D CAN_STATE_ERROR_WARN=
ING) {
> > +                               if (txerr >=3D 96)
> > +                                       cf->data[1] |=3D CAN_ERR_CRTL_T=
X_WARNING;
>=20
> 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/err=
or.h#L69
>=20
> I don't think you should set it if the error state does not change.
>=20
> 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,=20
Frank

>=20
>=20
> > +                               if (rxerr >=3D 96)
> > +                                       cf->data[1] |=3D CAN_ERR_CRTL_R=
X_WARNING;
> > +                       } else if (priv->can.state =3D=3D CAN_STATE_ERR=
OR_PASSIVE) {
> > +                               if (txerr >=3D 128)
> > +                                       cf->data[1] |=3D CAN_ERR_CRTL_T=
X_PASSIVE;
> > +                               if (rxerr >=3D 128)
> > +                                       cf->data[1] |=3D CAN_ERR_CRTL_R=
X_PASSIVE;
> >                         }
> >=20
> >                         cf->data[6] =3D txerr;
> > --
> > 2.25.1
> >=20