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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250110124541.GE7706@kernel.org>
Date: Fri, 10 Jan 2025 12:45:41 +0000
From: Simon Horman <horms@...nel.org>
To: Christophe Leroy <christophe.leroy@...roup.eu>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH net-next] freescale: ucc_geth: Remove set but unused
 variables

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 <horms@...nel.org>
> 
> 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