On Tue, Feb 03, 2026 at 04:18:09PM +0100, Harald Mommer wrote:
>
>
> On 2/3/26 13:05, Michael S. Tsirkin 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.
> >>
> >> 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]; // priv not
> > initialized, will be done too late in the next line
> > struct virtio_can_priv *priv = netdev_priv(ndev); // you see it?
> > struct device *dev = &priv->vdev->dev;
> > unsigned int len;
> > int err;
> >
> >
> > and where is the problem?
>
> The problem is that you use priv here to initialize vq in the line before
> priv is initialized.
Got it. Ignore the tree thing then. It's a guideline.
> >
> > On the flip size, this guarantees we will not forget to initialize.
>
> Static analysis is your friend.
And then someone monkey patches it to = NULL or something else silly.
I prefer correct by construction.
