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. */
--
2.34.1On Fri, Aug 16, 2024 at 09:35:02AM -0700, 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>
Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.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. */
> --
> 2.34.1
>
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
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
Hello Doug and Francisco,
thanks for cooperation
On Thursday 22 of August 2024 02:01:01 Doug Brown wrote:
> (Dropping Vikram from the email chain; I received "recipient not found"
> errors from AMD's mail servers in response to all of my patches)
If the address Vikram Garhwal <vikram.garhwal@amd.com> is not valid
then if he has still interest in CAN in QEMU it would be great
if he updates the address in the QEMU MAINTAINERS file. If he does
not plan to participate then the MAINTAINERS file should be updated
as well.
Vikram Garhwal is listed even as whole CAN subsystem comaintainer
https://gitlab.com/qemu-project/qemu/-/blob/3472f54522a928f0020d6928d54c007f862c5478/MAINTAINERS#L2690
I plan take an eye on the system long term and I have
ideas and plans how to enhance it in more directions when
I find some spare time or project, studnets, thesis,
company interested in adding another controller etc.
But I would be more comfortable if there is somebody else
who is willing to be at least my backup when I am on some
trip without Internet (hiking etc.). I am quite loaded by
teaching etc. and all these my CAN and in the fact all my
open-source and other development projects are mostly out
of any interrest of the university department where I serve.
They would care a little if/when I bring paid contract from
some company, as we have from Volkswagen and its subsidiaries.
But it is long time ago at university and even longer
at my company.
So all depends on my enthusiasm and free time which
should have at least some maintainership backup by somebody
who intend to use the project in frame of company or some
automotive consortium. I know that there are big money flowing
on base of these activities.
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
Hello Pavel and Francisco, On 8/21/2024 6:11 PM, Pavel Pisa wrote: > Vikram Garhwal is listed even as whole CAN subsystem comaintainer > > https://gitlab.com/qemu-project/qemu/-/blob/3472f54522a928f0020d6928d54c007f862c5478/MAINTAINERS#L2690 Unfortunately I am totally new to submitting patches for QEMU and have no connection with AMD so I can't speak about Vikram's situation. I agree though, at the very least something needs to change in the MAINTAINERS file. Maybe Francisco knows more? > But I would be more comfortable if there is somebody else > who is willing to be at least my backup when I am on some > trip without Internet (hiking etc.). I am quite loaded by > teaching etc. and all these my CAN and in the fact all my > open-source and other development projects are mostly out > of any interrest of the university department where I serve. > They would care a little if/when I bring paid contract from > some company, as we have from Volkswagen and its subsidiaries. > But it is long time ago at university and even longer > at my company. I'm very, very new to QEMU development so I think I would be a poor fit as a maintainer, at least right now. I'm also not affiliated with any big money companies. I can completely understand that the subsystem needs an additional maintainer though. That's a lot on one person's shoulders! Thank you for reviewing all of my patches, Francisco. Now, all of these patches are reviewed but there are a few other issues we talked about here (dlc2len/len2dlc and issues with the flags), and I also found a FIFO issue. Would it make the most sense for me to submit a V2 of this series with a few extra patches tacked on the end, or should I wait for this series to be applied first? I can do it either way, I just wasn't sure what would be preferred. Thanks, Doug
On Sat, 24 Aug 2024 at 02:55, Doug Brown <doug@schmorgal.com> wrote: > Now, all of these patches are reviewed but there are a few other issues > we talked about here (dlc2len/len2dlc and issues with the flags), and I > also found a FIFO issue. Would it make the most sense for me to submit a > V2 of this series with a few extra patches tacked on the end, or should > I wait for this series to be applied first? I can do it either way, I > just wasn't sure what would be preferred. We're currently still in codefreeze for the upcoming 9.1 release, so I would recommend sending a v2 with the extra patches. Nothing except critical bugfixes is going to be applied upstream for the next week or two. (Since this is xilinx related I'm happy to pick it up in a target-arm pullreq once we reopen for 9.2, unless anybody wants to grab it some other way.) thanks -- PMM
Hi Peter and Pavel, On 8/25/2024 10:30 AM, Peter Maydell wrote: > We're currently still in codefreeze for the upcoming 9.1 release, > so I would recommend sending a v2 with the extra patches. Nothing > except critical bugfixes is going to be applied upstream for > the next week or two. Thanks, that makes sense. Will do. Doug
Hello Doug
On Saturday 24 of August 2024 03:54:00 Doug Brown wrote:
> Thank you for reviewing all of my patches, Francisco.
>
> Now, all of these patches are reviewed but there are a few other issues
> we talked about here (dlc2len/len2dlc and issues with the flags), and I
> also found a FIFO issue. Would it make the most sense for me to submit a
> V2 of this series with a few extra patches tacked on the end, or should
> I wait for this series to be applied first? I can do it either way, I
> just wasn't sure what would be preferred.
I agree with both ways,
1) merging actual versing and then providing followup
patches
2) updating and extending the series
I have little inclination to the second choice (2), because
you have already patch where you touch dlc2len/len2dlc issue
and making it without middle step would be more straightforward
and readable to me.
Anyway, I am the initiator of QEMU CAN subsystem as GSoC and studnets
mentor and coauthor but I have no commit right to the QEMU mainline.
So actual merge has to be realized by people with commit right.
Paolo Bonzini has provided help with CAN subsystem integration
and the committing.
Best wishes,
Pavel
--
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
© 2016 - 2024 Red Hat, Inc.