netdev - Re: [PATCH net-next 3/4] net: renesas: rswitch: Remove gptp flag from rswitch_gwca

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 08 Feb 2023 08:06:38 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com
Cc:     netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] net: renesas: rswitch: Remove gptp flag
 from rswitch_gwca_queue

On Wed, 2023-02-08 at 16:34 +0900, Yoshihiro Shimoda wrote:
> The gptp flag is completely related to the !dir_tx in struct
> rswitch_gwca_queue. In the future, a new queue handling for
> timestamp will be implemented and this gptp flag is confusable.
> So, remove the gptp flag.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>

Based on these changes I am assuming that gptp == !dir_tx? Am I
understanding it correctly? It would be useful if you called that out
in the patch description.

> ---
>  drivers/net/ethernet/renesas/rswitch.c | 26 +++++++++++---------------
>  drivers/net/ethernet/renesas/rswitch.h |  1 -
>  2 files changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index b256dadada1d..e408d10184e8 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -280,11 +280,14 @@ static void rswitch_gwca_queue_free(struct net_device *ndev,
>  {
>  	int i;
>  
> -	if (gq->gptp) {
> +	if (!gq->dir_tx) {
>  		dma_free_coherent(ndev->dev.parent,
>  				  sizeof(struct rswitch_ext_ts_desc) *
>  				  (gq->ring_size + 1), gq->rx_ring, gq->ring_dma);
>  		gq->rx_ring = NULL;
> +
> +		for (i = 0; i < gq->ring_size; i++)
> +			dev_kfree_skb(gq->skbs[i]);
>  	} else {
>  		dma_free_coherent(ndev->dev.parent,
>  				  sizeof(struct rswitch_ext_desc) *
> @@ -292,11 +295,6 @@ static void rswitch_gwca_queue_free(struct net_device *ndev,
>  		gq->tx_ring = NULL;
>  	}
>  
> -	if (!gq->dir_tx) {
> -		for (i = 0; i < gq->ring_size; i++)
> -			dev_kfree_skb(gq->skbs[i]);
> -	}
> -
>  	kfree(gq->skbs);
>  	gq->skbs = NULL;
>  }

One piece I don't understand is why freeing of the skbs stored in the
array here was removed. Is this cleaned up somewhere else before we
call this function?