Re: [PATCH net-next v9 05/17] net: macb: enforce reverse christmas tree (RCT) convention

[Théo Lebrun]

Replying to non-net sashiko
https://sashiko.dev/#/patchset/20260812-macb-context-v9-0-7ddbf5f715e0%40bootlin.com

> commit da95f184645150278bd5328618e0592f867e39bb
> Author: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
>
> net: macb: enforce reverse christmas tree (RCT) convention
>
> This patch enforces the reverse christmas tree convention for variable
> declarations in several macb functions. The goal is to minimize unrelated
> diffs in future patches.
>
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 77053cb9d8f7b..b138b94ea0d89 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
> > @@ -2476,10 +2475,10 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
> >  	unsigned int q = skb_get_queue_mapping(skb);
> >  	unsigned int desc_cnt, nr_frags, frag_size, f;
> >  	struct macb_queue *queue = &bp->queues[q];
> > +	netdev_tx_t ret = NETDEV_TX_OK;
> >  	unsigned int hdrlen;
> >  	unsigned long flags;
> >  	bool is_lso;
> > -	netdev_tx_t ret = NETDEV_TX_OK;
>
> [Severity: Low]
> This isn't a bug, but it looks like the first few variable declarations in
> macb_start_xmit() are still not in reverse christmas tree order.
>
> With this patch applied, the top declarations in macb_start_xmit() are:
>
> 	struct macb *bp = netdev_priv(dev);
> 	unsigned int q = skb_get_queue_mapping(skb);
> 	unsigned int desc_cnt, nr_frags, frag_size, f;
> 	struct macb_queue *queue = &bp->queues[q];
> 	netdev_tx_t ret = NETDEV_TX_OK;
>
> Is it intended that these first few variables remain in their original order
> due to initialization dependencies? 
>
> The commit message mentions enforcing RCT here, but to fully sort them, the
> initializations might need to be split from the declarations similar to what
> was done in macb_tx_error_task().

It sounds like Sashiko doesn't visualise the tree trunk.
I'll ignore this review!

Thanks,

-- 
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com