| 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
| ||
|
Message-ID: <786db8fae65a2ed415b5dd0c3001b4dfc8c7112b.camel@esd.eu>
Date: Mon, 23 Jan 2023 15:47:22 +0000
From: Frank Jungclaus <Frank.Jungclaus@....eu>
To: "mailhol.vincent@...adoo.fr" <mailhol.vincent@...adoo.fr>
CC: Stefan Mätje <Stefan.Maetje@....eu>,
"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
"mkl@...gutronix.de" <mkl@...gutronix.de>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"wg@...ndegger.com" <wg@...ndegger.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] can: esd_usb: Improved behavior on esd CAN_ERROR_EXT
event (2)
On Thu, 2022-12-22 at 11:21 +0900, Vincent MAILHOL wrote:
> On Thu. 22 Dec. 2022 at 03:42, Frank Jungclaus <Frank.Jungclaus@....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@....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@....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
Powered by blists - more mailing lists