[net-next,v3,1/1] net: stmmac: Batch set RX OWN flag and other flags

[net-next,v3,1/1] net: stmmac: Batch set RX OWN flag and other flags

[ende.tan]

From: Tan En De <ende.tan@starfivetech.com>

Minimize access to the RX descriptor by collecting all the flags in a
local variable and then updating the descriptor at once.

Signed-off-by: Tan En De <ende.tan@starfivetech.com>
---
v3:
- Use local variable to batch set the descriptor flags.
- This reduces the number of accesses to the descriptor.
v2: https://patchwork.kernel.org/project/netdevbpf/patch/20240821060307.46350-1-ende.tan@starfivetech.com/
- Avoid introducing a new function just to set the interrupt-on-completion
  bit, as it is wasteful to do so.
- Delegate the responsibility of calling dma_wmb() from main driver code
  to set_rx_owner() callbacks (i.e. let callbacks to manage the low-level
  ordering/barrier rather than cluttering up the main driver code).
v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240814092438.3129-1-ende.tan@starfivetech.com/
---
 drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c   | 6 ++++--
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c | 6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
index 1c5802e0d7f4..dfcbe7036988 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
@@ -186,10 +186,12 @@ static void dwmac4_set_tx_owner(struct dma_desc *p)
 
 static void dwmac4_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
 {
-	p->des3 |= cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR);
+	u32 flags = cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR);
 
 	if (!disable_rx_ic)
-		p->des3 |= cpu_to_le32(RDES3_INT_ON_COMPLETION_EN);
+		flags |= cpu_to_le32(RDES3_INT_ON_COMPLETION_EN);
+
+	p->des3 |= flags;
 }
 
 static int dwmac4_get_tx_ls(struct dma_desc *p)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
index fc82862a612c..0c7ea939f787 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
@@ -56,10 +56,12 @@ static void dwxgmac2_set_tx_owner(struct dma_desc *p)
 
 static void dwxgmac2_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
 {
-	p->des3 |= cpu_to_le32(XGMAC_RDES3_OWN);
+	u32 flags = cpu_to_le32(XGMAC_RDES3_OWN);
 
 	if (!disable_rx_ic)
-		p->des3 |= cpu_to_le32(XGMAC_RDES3_IOC);
+		 flags |= cpu_to_le32(XGMAC_RDES3_IOC);
+
+	p->des3 |= flags;
 }
 
 static int dwxgmac2_get_tx_ls(struct dma_desc *p)
-- 
2.34.1

Re: [net-next,v3,1/1] net: stmmac: Batch set RX OWN flag and other flags

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on horms-ipvs/master v6.11-rc5 next-20240830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/ende-tan-starfivetech-com/net-stmmac-Batch-set-RX-OWN-flag-and-other-flags/20240829-214324
base:   linus/master
patch link:    https://lore.kernel.org/r/20240829134043.323855-1-ende.tan%40starfivetech.com
patch subject: [net-next,v3,1/1] net: stmmac: Batch set RX OWN flag and other flags
config: x86_64-randconfig-r132-20240830 (https://download.01.org/0day-ci/archive/20240831/202408310604.E3C4zDID-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240831/202408310604.E3C4zDID-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408310604.E3C4zDID-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:59:21: sparse: sparse: incorrect type in initializer (different base types) @@     expected unsigned int [usertype] flags @@     got restricted __le32 [usertype] @@
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:59:21: sparse:     expected unsigned int [usertype] flags
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:59:21: sparse:     got restricted __le32 [usertype]
>> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:62:24: sparse: sparse: invalid assignment: |=
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:62:24: sparse:    left side has type unsigned int
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:62:24: sparse:    right side has type restricted __le32
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:64:17: sparse: sparse: invalid assignment: |=
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:64:17: sparse:    left side has type restricted __le32
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:64:17: sparse:    right side has type unsigned int
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:110:23: sparse: sparse: restricted __le32 degrades to integer
   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:110:50: sparse: sparse: restricted __le32 degrades to integer
--
>> drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:189:21: sparse: sparse: incorrect type in initializer (different base types) @@     expected unsigned int [usertype] flags @@     got restricted __le32 [usertype] @@
   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:189:21: sparse:     expected unsigned int [usertype] flags
   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:189:21: sparse:     got restricted __le32 [usertype]
>> drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:192:23: sparse: sparse: invalid assignment: |=
   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:192:23: sparse:    left side has type unsigned int
   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:192:23: sparse:    right side has type restricted __le32
   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:194:17: sparse: sparse: invalid assignment: |=
   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:194:17: sparse:    left side has type restricted __le32
   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:194:17: sparse:    right side has type unsigned int

vim +59 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c

    56	
    57	static void dwxgmac2_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
    58	{
  > 59		u32 flags = cpu_to_le32(XGMAC_RDES3_OWN);
    60	
    61		if (!disable_rx_ic)
  > 62			 flags |= cpu_to_le32(XGMAC_RDES3_IOC);
    63	
    64		p->des3 |= flags;
    65	}
    66	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [net-next,v3,1/1] net: stmmac: Batch set RX OWN flag and other flags

[Florian Fainelli]

On 8/29/24 06:40, ende.tan@starfivetech.com wrote:
> From: Tan En De <ende.tan@starfivetech.com>
> 
> Minimize access to the RX descriptor by collecting all the flags in a
> local variable and then updating the descriptor at once.
> 
> Signed-off-by: Tan En De <ende.tan@starfivetech.com>
> ---
> v3:
> - Use local variable to batch set the descriptor flags.
> - This reduces the number of accesses to the descriptor.
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20240821060307.46350-1-ende.tan@starfivetech.com/
> - Avoid introducing a new function just to set the interrupt-on-completion
>    bit, as it is wasteful to do so.
> - Delegate the responsibility of calling dma_wmb() from main driver code
>    to set_rx_owner() callbacks (i.e. let callbacks to manage the low-level
>    ordering/barrier rather than cluttering up the main driver code).
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240814092438.3129-1-ende.tan@starfivetech.com/
> ---
>   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c   | 6 ++++--
>   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c | 6 ++++--
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> index 1c5802e0d7f4..dfcbe7036988 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> @@ -186,10 +186,12 @@ static void dwmac4_set_tx_owner(struct dma_desc *p)
>   
>   static void dwmac4_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
>   {
> -	p->des3 |= cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR);
> +	u32 flags = cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR);
>   
>   	if (!disable_rx_ic)
> -		p->des3 |= cpu_to_le32(RDES3_INT_ON_COMPLETION_EN);
> +		flags |= cpu_to_le32(RDES3_INT_ON_COMPLETION_EN);
> +
> +	p->des3 |= flags;

You could just batch the endian conversion too:

	u32 flags = DES3_OWN | RDES3_BUFFER1_VALID_ADDR;

	if (!disable_rx_ic)
		flags |= RDES3_INT_ON_COMPLETION_EN;

	p->desc3 |= cpu_to_le32(flags);

>   }
>   
>   static int dwmac4_get_tx_ls(struct dma_desc *p)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> index fc82862a612c..0c7ea939f787 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> @@ -56,10 +56,12 @@ static void dwxgmac2_set_tx_owner(struct dma_desc *p)
>   
>   static void dwxgmac2_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
>   {
> -	p->des3 |= cpu_to_le32(XGMAC_RDES3_OWN);
> +	u32 flags = cpu_to_le32(XGMAC_RDES3_OWN);
>   
>   	if (!disable_rx_ic)
> -		p->des3 |= cpu_to_le32(XGMAC_RDES3_IOC);
> +		 flags |= cpu_to_le32(XGMAC_RDES3_IOC);
> +
> +	p->des3 |= flags;

And likewise here, and that would likely squash the sparse warning, 
since you would not be assigning an 'u32' field to a __le32 field anymore.
-- 
Florian

Re: [net-next,v3,1/1] net: stmmac: Batch set RX OWN flag and other flags

[Florian Fainelli]

On 8/29/24 06:40, ende.tan@starfivetech.com wrote:
> From: Tan En De <ende.tan@starfivetech.com>
> 
> Minimize access to the RX descriptor by collecting all the flags in a
> local variable and then updating the descriptor at once.
> 
> Signed-off-by: Tan En De <ende.tan@starfivetech.com>
> ---
> v3:
> - Use local variable to batch set the descriptor flags.
> - This reduces the number of accesses to the descriptor.
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20240821060307.46350-1-ende.tan@starfivetech.com/
> - Avoid introducing a new function just to set the interrupt-on-completion
>    bit, as it is wasteful to do so.
> - Delegate the responsibility of calling dma_wmb() from main driver code
>    to set_rx_owner() callbacks (i.e. let callbacks to manage the low-level
>    ordering/barrier rather than cluttering up the main driver code).
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20240814092438.3129-1-ende.tan@starfivetech.com/
> ---
>   drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c   | 6 ++++--
>   drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c | 6 ++++--
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> index 1c5802e0d7f4..dfcbe7036988 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> @@ -186,10 +186,12 @@ static void dwmac4_set_tx_owner(struct dma_desc *p)
>   
>   static void dwmac4_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
>   {
> -	p->des3 |= cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR);
> +	u32 flags = cpu_to_le32(RDES3_OWN | RDES3_BUFFER1_VALID_ADDR);
>   
>   	if (!disable_rx_ic)
> -		p->des3 |= cpu_to_le32(RDES3_INT_ON_COMPLETION_EN);
> +		flags |= cpu_to_le32(RDES3_INT_ON_COMPLETION_EN);
> +
> +	p->des3 |= flags;

You could just batch the endian conversion too:

	u32 flags = DES3_OWN | RDES3_BUFFER1_VALID_ADDR;

	if (!disable_rx_ic)
		flags |= RDES3_INT_ON_COMPLETION_EN;

	p->desc3 |= cpu_to_le32(flags);

>   }
>   
>   static int dwmac4_get_tx_ls(struct dma_desc *p)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> index fc82862a612c..0c7ea939f787 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
> @@ -56,10 +56,12 @@ static void dwxgmac2_set_tx_owner(struct dma_desc *p)
>   
>   static void dwxgmac2_set_rx_owner(struct dma_desc *p, int disable_rx_ic)
>   {
> -	p->des3 |= cpu_to_le32(XGMAC_RDES3_OWN);
> +	u32 flags = cpu_to_le32(XGMAC_RDES3_OWN);
>   
>   	if (!disable_rx_ic)
> -		p->des3 |= cpu_to_le32(XGMAC_RDES3_IOC);
> +		 flags |= cpu_to_le32(XGMAC_RDES3_IOC);
> +
> +	p->des3 |= flags;

And likewise here, and that would likely squash the sparse warning, 
since you would not be assigning an 'u32' field to a __le32 field anymore.
-- 
Florian