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

[Doug Brown <doug@schmorgal.com>]

Hi Pavel,

(Dropping Vikram from the email chain; I received "recipient not found"
errors from AMD's mail servers in response to all of my patches)

On 8/20/2024 11:57 PM, Pavel Pisa wrote:
> Hello Doug Brown,
> 
> On Friday 16 of August 2024 18:35:02 Doug Brown wrote:
>> -        if (frame->flags == QEMU_CAN_FRMF_TYPE_FD) {
>> +        if (frame->flags & QEMU_CAN_FRMF_TYPE_FD) {
> 
> 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.

Thank you for reviewing!

> 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.

Nice find. It looks like it would be pretty straightforward to add
support for those flags. As far as I can tell they map directly to
register bits.

> 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're absolutely right. It looks like frame->flags isn't being
initialized at all when it's a non-FD frame. I can also take care of
this. I'll wait and see how the review goes for the other patches, and I
can add another patch to fix these flag issues in the next version of
the series.

> 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

Nice! It seems like using these functions could simplify this code quite
a bit, by eliminating the need for canfd_dlc_array. I can add this as
another patch for the next version.

> 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.
Thanks! I also see that Francisco reviewed a couple of the patches --
thanks Francisco! I will wait and see how review goes on the others.

For what it's worth, I was stress testing this in QEMU today and found
another issue with the FIFO read_index and store_index calculations and
the logic that wraps them around to 0. I will submit the fix for this
problem as another patch in the next version of the series (or a new
series if that's more convenient).

Doug