Subject: Re: [PATCH net-next] freescale: ucc_geth: Remove set but unused variables

From: Simon Horman <[email-masked]>
Date: Fri, 10 Jan 2025 12:45:41 +0000

On Fri, Jan 10, 2025 at 12:07:25PM +0100, Christophe Leroy wrote:
> 
> 
> Le 10/01/2025 à 11:18, Simon Horman a écrit :
> > Remove set but unused variables. These seem to provide no value.
> > So in the spirit of less being more, remove them.
> 
> Would be good to identify when those variables became unused.
> 
> There is for instance commit 64a99fe596f9 ("ethernet: ucc_geth: remove
> bd_mem_part and all associated code")

Sure, I can work on an updated commit message for v2 along those lines.

> 
> ...
> 
> > 
> > Compile tested only.
> > No runtime effect intended.
> > 
> > Signed-off-by: Simon Horman <[email-masked]>
> 
> As you are playing with that driver, there are also sparse warnings to be
> fixed, getting plenty when building with C=2

Yes, I noticed.
That is on my todo list :)

> 
> > ---
> >   drivers/net/ethernet/freescale/ucc_geth.c | 39 +++++++------------------------
> >   1 file changed, 8 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
> > index 88510f822759..1e3a1cb997c3 100644
> > --- a/drivers/net/ethernet/freescale/ucc_geth.c
> > +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> > @@ -1704,14 +1704,8 @@ static int ugeth_82xx_filtering_clear_addr_in_paddr(struct ucc_geth_private *uge
> >   static void ucc_geth_free_rx(struct ucc_geth_private *ugeth)
> >   {
> > -	struct ucc_geth_info *ug_info;
> > -	struct ucc_fast_info *uf_info;
> > -	u16 i, j;
> >   	u8 __iomem *bd;
> > -
> > -
> > -	ug_info = ugeth->ug_info;
> > -	uf_info = &ug_info->uf_info;
> > +	u16 i, j;
> 
> Why do you need to move this declaration ? Looks like cosmetics. That goes
> beyond the purpose of this patch which is already big enough and should be
> avoided. The same applies several times in this patch.

It seemed convenient to move this around at the same time,
as the lines are adjacent. But will drop them as you wish.

> 
> >   	for (i = 0; i < ucc_geth_rx_queues(ugeth->ug_info); i++) {
> >   		if (ugeth->p_rx_bd_ring[i]) {

...

> > @@ -2904,7 +2883,7 @@ static int ucc_geth_rx(struct ucc_geth_private *ugeth, u8 rxQ, int rx_work_limit
> >   	/* while there are received buffers and BD is full (~R_E) */
> >   	while (!((bd_status & (R_E)) || (--rx_work_limit < 0))) {
> > -		bdBuffer = (u8 *) in_be32(&((struct qe_bd __iomem *)bd)->buf);
> > +		in_be32(&((struct qe_bd __iomem *)bd)->buf);
> 
> This line should go completely.

Thanks,

I was slightly concerned that it may have some side effect - that
I have no way to test. But I will remove it on your advice.

> 
> >   		length = (u16) ((bd_status & BD_LENGTH_MASK) - 4);
> >   		skb = ugeth->rx_skbuff[rxQ][ugeth->skb_currx[rxQ]];

...

-- 
pw-bot: changes-requested