Re: [PATCH v7] can: virtio: Add virtio CAN driver

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Feb 03, 2026 at 01:32:47PM +0100, Vincent Mailhol wrote:
> On Mar. 3 Feb. 2026 at 13:05, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Feb 03, 2026 at 12:55:07PM +0100, Harald Mommer wrote:
> > >
> > >
> > > On 1/9/26 18:23, Francesco Valla wrote:
> > > >> +static u8 virtio_can_send_ctrl_msg(struct net_device *ndev, u16 msg_type)
> > > >> +{
> > > >> +  struct scatterlist sg_out, sg_in, *sgs[2] = { &sg_out, &sg_in };
> > > >> +  struct virtio_can_priv *priv = netdev_priv(ndev);
> > > >> +  struct device *dev = &priv->vdev->dev;
> > > >> +  struct virtqueue *vq;
> > > >> +  unsigned int len;
> > > >> +  int err;
> > > >> +
> > > >> +  vq = priv->vqs[VIRTIO_CAN_QUEUE_CONTROL];
> > > > Nit: consider initializing this above, while declaring it.
> > >
> > > All those "Nit" regarding initialization cause problems. There is a reason why it was done the way it is.
> > >
> > > The network people require that the declaration lines are ordered by line length. longest line first. This is called "Reverse Christmas tree". Don't ask me why, this formatting style is what the network people require. Their subsystem, their rules.
> 
> I am fine with the Reverse Christmas Tree in general, except when it
> randomly splits the initialization, as is the case here. As you noted,
> this is a coding rule of the network subsystem, but here we are the
> CAN subsystem. So yes, the CAN is itself a sub-subsystem of network,
> but my point is that we are a different team of maintainers. I would
> like to ask the network maintainers for understanding regarding our
> different preferences on that topic.
> 
> Unless Marc or Oliver have a strong opinion on this, I would prefer
> not to push the Reverse Christmas Tree to its limits and to allow,
> within the CAN subtree, exceptions whenever this would avoid some
> hanging initializations.
> 
> > > To initialize the vq you need now already the priv initialized. If now the vq line becomes longer than the priv line you will violate the special formatting requirements of the network subsystem.
> > >
> > > Solution was: What you see above.
> > >
> > > Regards
> > > Harald
> >
> > So you reorder it then:
> >
> >         struct scatterlist sg_out, sg_in, *sgs[2] = { &sg_out, &sg_in };
> >         struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_CONTROL];
> >         struct virtio_can_priv *priv = netdev_priv(ndev);
> >         struct device *dev = &priv->vdev->dev;
> >         unsigned int len;
> >         int err;
> >
> >
> > and where is the problem?
> 
> The problem is that priv is not yet declared. So this:
> 
>   struct scatterlist sg_out, sg_in, *sgs[2] = { &sg_out, &sg_in };
>   struct virtio_can_priv *priv = netdev_priv(ndev);
>   struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_CONTROL];
>   struct device *dev = &priv->vdev->dev;
>   unsigned int len;
>   int err;
> 
> is forced,

Ah. Good point.

> which, IMHO, is totally *fine* and way better than
> deporting down the vq initialization.

Indeed.


> > On the flip size, this guarantees we will not forget to initialize.
> 
> Yes!
> 
> 
> Yours sincerely,
> Vincent Mailhol