On Thu, 2022-12-22 at 11:21 +0900, Vincent MAILHOL wrote: > On Thu. 22 Dec. 2022 at 03:42, Frank Jungclaus <Frank.Jungclaus@xxxxxx> 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@xxxxxx> 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@xxxxxx> > > > > Link: [1] https://lore.kernel.org/all/CAMZ6RqKGBWe15aMkf8-QLf-cOQg99GQBebSm+1wEzTqHgvmNuw@xxxxxxxxxxxxxx/ > > > > Link: [2] https://lore.kernel.org/all/CAMZ6Rq+QBO1yTX_o6GV0yhdBj-RzZSRGWDZBS0fs7zbSTy4hmA@xxxxxxxxxxxxxx/ > > > > --- > > > > 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
