Le lun. 8 mai 2023 à 04:20, Marek Vasut <marex@xxxxxxx> a écrit :
>
> On 5/7/23 17:58, Vincent Mailhol wrote:
> > Hi Marek,
>
> Hi,
>
> > The patches should have been in reverse order:
> >
> > 1st: can: mcp251xfd: Move generic macros into length.h
> > 2nd: can: mcp251xfd: Increase poll timeout
> >
> > so that you do not have to remove the lines just added in the previous patch.
>
> The current order is actually deliberate to make it easy to backport
> this one patch via stable queue, since it Fixes: a bug. The 2/2 is just
> a generalization.
I see your point. However, your patch uses CANFD_FRAME_LEN_MAX which
was introduced in Linux 5.12 but mcp251xfd was introduced in 5.10. So
you will need a separate patch for the 5.10 LTS branch.
As for the other stable branches (5.15 and greater), patches toward
linux/can/length.h can easily be backported. To conclude, in that
particular instance, I think that the correct approach is:
- One separate mcp251xfd patch for 5.10.
- A pair of patches targeting mcp251xfd and length.c for 5.15+
> > On Tue. 6 May 2023 at 07:36, Marek Vasut <marex@xxxxxxx> wrote:
> >> From: Fedor Ross <fedor.ross@xxxxxxx>
> >>
> >> Make `MCP251XFD_POLL_TIMEOUT_US` timeout calculation dynamic. Use
> >> maximum of 1ms (arbitrarily chosen during driver development) and
> >> bit time of one full CANFD frame including bit stuffing and bus idle
> >> condition sample cycles, at the current bitrate. This seems to be
> >> necessary when configuring low bit rates like 10 Kbit/s for example.
> >> Otherwise during polling for the CAN controller to enter
> >> 'Normal CAN 2.0 mode' the timeout limit is exceeded and the
> >> configuration fails with:
> >>
> >> $ ip link set dev can1 up type can bitrate 10000
> >> [ 731.911072] mcp251xfd spi2.1 can1: Controller failed to enter mode CAN 2.0 Mode (6) and stays in Configuration Mode (4) (con=0x068b0760, osc=0x00000468).
> >> [ 731.927192] mcp251xfd spi2.1 can1: CRC read error at address 0x0e0c (length=4, data=00 00 00 00, CRC=0x0000) retrying.
> >> [ 731.938101] A link change request failed with some changes committed already. Interface can1 may have been left with an inconsistent configuration, please check.
> >> RTNETLINK answers: Connection timed out
> >>
> >> Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")
> >> Signed-off-by: Fedor Ross <fedor.ross@xxxxxxx>
> >> Signed-off-by: Marek Vasut <marex@xxxxxxx>
>
> [...]
>
> >> @@ -251,7 +252,12 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
> >> FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
> >> con) == mode_req,
> >> MCP251XFD_POLL_SLEEP_US,
> >> - MCP251XFD_POLL_TIMEOUT_US);
> >> + max(MCP251XFD_POLL_TIMEOUT_US,
> >> + (unsigned int)(CANFD_FRAME_LEN_MAX *
> >> + BITS_PER_BYTE *
> >> + CAN_BIT_STUFFING_OVERHEAD +
> >
> > The goal is to have the exact number of bits, right?
>
> Not really, the goal is to calculate a suitable delay, for which the
> kernel has to wait for this SPI CAN controller to switch mode . That
> delay is dependent on the bit timing though.
Ack. In that sense, the above formula is indeed suitable. Regardless,
CANFD_FRAME_LEN_MAX * BITS_PER_BYTE
expands to:
(DIV_ROUND_UP(80, 8) + 64) * 8
and that confuses me a lot to divide and then just multiply again by
the same value.
> > It seems odd to me to use a rounded value and then try to recalculate
> > the exact length in bits.
> > I understand that because CANFD_FRAME_OVERHEAD_EFF is a multiple of
> > BITS_PER_BYTE, CANFD_FRAME_LEN_MAX happened to be the exact value.
> > Regardless, that is a fluke.
> >
> > I think that we should have another set of definitions for the frame
> > lengths in bits. I sent a proposal here:
> >
> > https://lore.kernel.org/linux-can/20230507155506.3179711-1-mailhol.vincent@xxxxxxxxxx/
> >
> > If you like it, you can rebase this patch on top of mine and use the
> > newly defined CANFD_FRAME_LEN_MAX_BITS_STUFFING.
>
> I think I don't have enough experience to decide one way or the other,
> so I will wait for the reviewers to suggest the course of action.
ACK. I am fine with delegating the final decision to others.
