Re: [PATCH 2/5] hw/net/can/xlnx-versal-canfd: Fix CAN FD flag check

[Pavel Pisa]

Hello Doug Brown,

On Friday 16 of August 2024 18:35:02 Doug Brown wrote:
> When checking the QEMU_CAN_FRMF_TYPE_FD flag, we need to ignore other
> potentially set flags. Before this change, received CAN FD frames from
> SocketCAN weren't being recognized as CAN FD.
>
> Signed-off-by: Doug Brown <doug@schmorgal.com>
> ---
>  hw/net/can/xlnx-versal-canfd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/can/xlnx-versal-canfd.c
> b/hw/net/can/xlnx-versal-canfd.c index ad0c4da3c8..8968672b84 100644
> --- a/hw/net/can/xlnx-versal-canfd.c
> +++ b/hw/net/can/xlnx-versal-canfd.c
> @@ -1003,7 +1003,7 @@ static void store_rx_sequential(XlnxVersalCANFDState
> *s,
>
>          dlc = frame->can_dlc;
>
> -        if (frame->flags == QEMU_CAN_FRMF_TYPE_FD) {
> +        if (frame->flags & QEMU_CAN_FRMF_TYPE_FD) {
>              is_canfd_frame = true;
>
>              /* Store dlc value in Xilinx specific format. */

Reviewed-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>

That is a great catch, I have overlooked this in previous
review of the Xilinx code.

When I look into hw/net/can/xlnx-versal-canfd.c functions
regs2frame and store_rx_sequential then I see missing
handling of flags QEMU_CAN_FRMF_ESI and QEMU_CAN_FRMF_BRS.

In the function regs2frame is missing even initialization
of frame->flags = 0 at the start, which I expect should be there.
The
  frame->flags = QEMU_CAN_FRMF_TYPE_FD;
should be then
  frame->flags |= QEMU_CAN_FRMF_TYPE_FD;

You can see how it was intended to parse and fill flags in our
CTU CAN FD interface code which matches our design of common
QEMU CAN infrastructure and its extension for CAN FD.

See the functions
  ctucan_buff2frame()
  ctucan_frame2buff()
in
  hw/net/can/ctucan_core.c

QEMU_CAN_EFF_FLAG and QEMU_CAN_RTR_FLAG seems to be corrected
in followup patch

[PATCH 3/5] hw/net/can/xlnx-versal-canfd: Translate CAN ID registers

As for the DLC conversion, there are functions

  frame->can_dlc = can_dlc2len(xxxx)
  XXX = can_len2dlc(frame->can_dlc);

provided by net/can/can_core.c

I am not sure how much competent I am for the rest of the patches,
because I do not know XilinX IP core so well. Review by Vikram Garhwal
or somebody else from AMD/XilinX is more valueable there.
But I can add my ACK there based on rough overview.

Best wishes,

                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    social:     https://social.kernel.org/ppisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home