linux-kernel - Re: [net-next RFC v3 1/7] net: ravb: Simplify poll & receive functions

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: Mon, 15 Apr 2024 12:31:03 +0100
From: Paul Barker <paul.barker.ct@...renesas.com>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Sergey Shtylyov <s.shtylyov@....ru>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Geert Uytterhoeven <geert+renesas@...der.be>, netdev@...r.kernel.org,
 linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next RFC v3 1/7] net: ravb: Simplify poll & receive
 functions

On 15/04/2024 12:23, Niklas Söderlund wrote:
> Hi Paul,
> 
> I really like this patch!
> 
> One nit below.
> 
> On 2024-04-15 10:47:58 +0100, Paul Barker wrote:
>> We don't need to pass the work budget to ravb_rx() by reference, it's
>> cleaner to pass this by value and return the amount of work done. This
>> allows us to simplify the ravb_poll() function and use the common
>> `work_done` variable name seen in other network drivers for consistency
>> and ease of understanding.
>>
>> This is a pure refactor and should not affect behaviour.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@...renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb.h      |  2 +-
>>  drivers/net/ethernet/renesas/ravb_main.c | 29 ++++++++++--------------
>>  2 files changed, 13 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>> index b48935ec7e28..71de2a7aa27c 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> @@ -1039,7 +1039,7 @@ struct ravb_ptp {
>>  };
>>  
>>  struct ravb_hw_info {
>> -	bool (*receive)(struct net_device *ndev, int *quota, int q);
>> +	int (*receive)(struct net_device *ndev, int budget, int q);
>>  	void (*set_rate)(struct net_device *ndev);
>>  	int (*set_feature)(struct net_device *ndev, netdev_features_t features);
>>  	int (*dmac_init)(struct net_device *ndev);
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index fd2679ce4e3d..33f8043143c1 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -759,7 +759,7 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
>>  }
>>  
>>  /* Packet receive function for Gigabit Ethernet */
>> -static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>> +static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>> @@ -778,7 +778,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>>  	stats = &priv->stats[q];
>>  
>> -	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
>> +	for (i = 0; i < limit && rx_packets < budget; i++, priv->cur_rx[q]++) {
>>  		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>>  		desc = &priv->rx_ring[q].desc[entry];
>>  		if (desc->die_dt == DT_FEMPTY)
>> @@ -881,13 +881,11 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>>  		desc->die_dt = DT_FEMPTY;
>>  	}
>>  
>> -	stats->rx_packets += rx_packets;
>> -	*quota -= rx_packets;
>> -	return *quota == 0;
>> +	return rx_packets;
>>  }
>>  
>>  /* Packet receive function for Ethernet AVB */
>> -static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>> +static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>> @@ -904,7 +902,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>>  	int i;
>>  
>>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>> -	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
>> +	for (i = 0; i < limit && rx_packets < budget; i++, priv->cur_rx[q]++) {
>>  		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>>  		desc = &priv->rx_ring[q].ex_desc[entry];
>>  		if (desc->die_dt == DT_FEMPTY)
>> @@ -992,18 +990,16 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>>  		desc->die_dt = DT_FEMPTY;
>>  	}
>>  
>> -	stats->rx_packets += rx_packets;
> 
> Don't we still need to update the statistics? Same for ravb_rx_gbeth().

Good catch! This was present in the previous version of the series [1],
but I accidentally dropped it while splitting out the bugfix patches.
I'll add it back in for the next version.

[1]: https://lore.kernel.org/all/20240206091909.3191-3-paul.barker.ct@bp.renesas.com/

Thanks,

-- 
Paul Barker
Download attachment "OpenPGP_0x27F4B3459F002257.asc" of type "application/pgp-keys" (3521 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)