| Message ID | 20240816163504.789393-3-doug@schmorgal.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net/can/xlnx-versal-canfd: Miscellaneous fixes | expand |
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
On 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 >
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
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
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
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. */
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(-)