On Thu, 2023-02-02 at 16:22 +0100, Marc Kleine-Budde wrote: > On 23.01.2023 15:47:22, Frank Jungclaus wrote: > > 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 ... > > Not taking this series, waiting for the reworked version. > > Marc > Marc, can I just send a reworked patch of [PATCH 2/3], let's say with subject [PATCH v2 2/3] as a reply to this thread or should I better resend the complete patch series as [PATCH v2 0/3] up to [PATCH v2 3/3]? Regards Frank
