On Mar. 3 Feb. 2026 at 13:05, Michael S. Tsirkin <[email protected]> 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, which, IMHO, is totally *fine* and way better than
deporting down the vq initialization.
> On the flip size, this guarantees we will not forget to initialize.
Yes!
Yours sincerely,
Vincent Mailhol
