RE: [PATCH v1 1/6] net/can: Initial host SocketCan support for CAN FD.

[Vikram Garhwal]

Hi Pavel,
Forgot to add this in last reply: Francisco Iglesias(in cc) was also involved a 
lot in net/can QEMU devices and willing to help in the review if needed. 

Regards
Vikram

> -----Original Message-----
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
> Sent: Wednesday, September 2, 2020 10:20 PM
> To: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> Cc: qemu-devel@nongnu.org; Paolo Bonzini <pbonzini@redhat.com>;
> Stefan Hajnoczi <stefanha@gmail.com>; Konrad Frederic
> <frederic.konrad@adacore.com>; Deniz Eren <deniz.eren@icloud.com>;
> Oliver Hartkopp <socketcan@hartkopp.net>; Marek Vasut
> <marex@denx.de>; Jan Kiszka <jan.kiszka@siemens.com>; Oleksij Rempel
> <o.rempel@pengutronix.de>; Markus Armbruster <armbru@redhat.com>;
> Ondrej Ille <ondrej.ille@gmail.com>; Jan Charvat <charvj10@fel.cvut.cz>;
> Jiri Novak <jnovak@fel.cvut.cz>
> Subject: Re: [PATCH v1 1/6] net/can: Initial host SocketCan support for CAN
> FD.
> 
> On Wed, Sep 02, 2020 at 09:51:44AM +0200, Pavel Pisa wrote:
> Hi Pavel,
> > Hello Vikram,
> >
> > thanks much for the patches review.
> >
> > On Tuesday 01 of September 2020 22:01:26 Vikram Garhwal wrote:
> > > Hi Jan,
> > > A couple of comments on this patch.
> > >
> > > On Tue, Jul 14, 2020 at 02:20:14PM +0200, pisa@cmp.felk.cvut.cz
> wrote:
> > > > From: Jan Charvat <charvj10@fel.cvut.cz> @@ -185,13 +204,34 @@
> > > > static void can_host_socketcan_connect(CanHostState
> > > > *ch, Error **errp) addr.can_family = AF_CAN;
> > > >      memset(&ifr.ifr_name, 0, sizeof(ifr.ifr_name));
> > > >      strcpy(ifr.ifr_name, c->ifname);
> > > > +    /* check if the frame fits into the CAN netdevice */
> > > >      if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
> > > >          error_setg_errno(errp, errno,
> > > > -                         "SocketCAN host interface %s not available",
> > > > c->ifname); +                         "SocketCAN host interface %s not
> > > > available", +                         c->ifname);
> > >
> > > May be this formatting change in a different patch? As this is not
> > > related to CANFD.
> > > > @@ -232,7 +272,8 @@ static char
> *can_host_socketcan_get_if(Object
> > > > *obj, Error **errp) return g_strdup(c->ifname);  }
> > > >
> > > > -static void can_host_socketcan_set_if(Object *obj, const char
> > > > *value, Error **errp) +static void
> > > > can_host_socketcan_set_if(Object *obj, const char *value,
> > > > +                                      Error **errp)
> > >
> > > This one also not relevant change for CANFD. Rest of the patch looks
> good.
> >
> >
> > I am responsible for mentioned lines change in net/can/can_socketcan.c.
> > When I have reviewed patches after Jan Charvat theses submittion, I
> > have done another bunch of rounds to check that the patches as well as
> > the whole net/can and hw/net/can are checkpatch clean. I am not sure
> > if the incorrect formatting sneaked in in my 2018 submission or
> > patcheck became more strict last years.
> >
> > I can separate these changes changes into separate patch if required.
> May be we can keep them in same patch. I was just referring to "Don't
> include irrelevant changes" section on this page about patches:
> https://wiki.qemu.org/Contribute/SubmitAPatch.
> >
> > By the way, if you or other of your colleagues is willing to
> > participate in net/can and or  hw/net/can patches reviews, I would be
> > happy if you join my attempt and we can record that we are available
> > to take care abut these in MAINTAINERS file.
> Given that I spent good amount of time working with net/can, I am willing
> to review the patches. Thanks!
> >
> > Best wishes,
> >
> > Pavel
> >
> >