[PATCH net-next 0/3] net: ethernet: ti: Address some warnings

[PATCH net-next 0/3] net: ethernet: ti: Address some warnings

[Simon Horman]

Hi,

This patchset addresses some warnings flagged by Sparse, and clang-18 in
TI Ethernet drivers.

Although these changes do not alter the functionality of the code, by
addressing them real problems introduced in future which are flagged by
tooling will stand out more readily.

Compile tested only.

---
Simon Horman (3):
      net: ethernet: ti: am65-cpsw: Address __percpu Sparse warnings
      net: ethernet: ti: am65-cpsw: Use __be64 type for id_temp
      net: ethernet: ti: cpsw_ale: Remove unused accessor functions

 drivers/net/ethernet/ti/am65-cpsw-nuss.c |  8 +++-----
 drivers/net/ethernet/ti/cpsw_ale.c       | 30 +++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 14 deletions(-)

base-commit: a9b1fab3b69f163bbe7a012d0c3f6b5204500c05

[PATCH net-next 1/3] net: ethernet: ti: am65-cpsw: Address __percpu Sparse warnings

[Simon Horman]

Sparse flags a mismatch between percpu stats data, and
it's use with actions which handle non-percpu data.

.../am65-cpsw-nuss.c:2658:55: warning: incorrect type in initializer (different address spaces)
.../am65-cpsw-nuss.c:2658:55:    expected struct am65_cpsw_ndev_stats [noderef] __percpu *stats
.../am65-cpsw-nuss.c:2658:55:    got void *data
.../am65-cpsw-nuss.c:2781:15: warning: incorrect type in argument 3 (different address spaces)
.../am65-cpsw-nuss.c:2781:15:    expected void *data
.../am65-cpsw-nuss.c:2781:15:    got struct am65_cpsw_ndev_stats [noderef] __percpu *stats

Address this using casts.

An alternate, approach would be to create a variant of
devm_add_action_or_reset() which expects __percpu data.  This would
avoid discarding the __percpu annotation, and any value it may have
between the casts added by this patch.  However, doing so appears to
require a significant amount of plumbing.  And, as far as I can see, the
code updated by this patch would be the only user of it.  So this patch
takes a simpler approach.

No functional change intended.
Compile tested only.

Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index b7e5d0fb5d19..a4b0e4bb7529 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2671,9 +2671,7 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
 
 static void am65_cpsw_pcpu_stats_free(void *data)
 {
-	struct am65_cpsw_ndev_stats __percpu *stats = data;
-
-	free_percpu(stats);
+	free_percpu((void __percpu *)data);
 }
 
 static void am65_cpsw_nuss_phylink_cleanup(struct am65_cpsw_common *common)
@@ -2794,7 +2792,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 		return -ENOMEM;
 
 	ret = devm_add_action_or_reset(dev, am65_cpsw_pcpu_stats_free,
-				       ndev_priv->stats);
+				       (__force void *)ndev_priv->stats);
 	if (ret)
 		dev_err(dev, "failed to add percpu stat free action %d\n", ret);
 

-- 
2.45.2

[PATCH net-next 2/3] net: ethernet: ti: am65-cpsw: Use __be64 type for id_temp

[Simon Horman]

The id_temp local variable in am65_cpsw_nuss_probe() is
used to hold a 64-bit big-endian value as it is assigned using
cpu_to_be64().

It is read using memcpy(), where it is written as an identifier into a
byte-array.  So this can also be treated as big endian.

As it's type is currently host byte order (u64), sparse flags
an endian mismatch when compiling for little-endian systems:

.../am65-cpsw-nuss.c:3454:17: warning: incorrect type in assignment (different base types)
.../am65-cpsw-nuss.c:3454:17:    expected unsigned long long [usertype] id_temp
.../am65-cpsw-nuss.c:3454:17:    got restricted __be64 [usertype]

Address this by using __be64 as the type of id_temp.

No functional change intended.
Compile tested only.

Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index a4b0e4bb7529..9e6353e0361e 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -3444,7 +3444,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct clk *clk;
 	int ale_entries;
-	u64 id_temp;
+	__be64 id_temp;
 	int ret, i;
 
 	common = devm_kzalloc(dev, sizeof(struct am65_cpsw_common), GFP_KERNEL);

-- 
2.45.2

[PATCH net-next 3/3] net: ethernet: ti: cpsw_ale: Remove unused accessor functions

[Simon Horman]

W=1 builds flag that some accessor functions for ALE fields are unused.

Address this by splitting up the macros used to define these
accessors to allow only those that are used to be declared.

The warnings are verbose, but for example, the mcast_state case is
flagged by clang-18 as:

.../cpsw_ale.c:220:1: warning: unused function 'cpsw_ale_get_mcast_state' [-Wunused-function]
  220 | DEFINE_ALE_FIELD(mcast_state,           62,     2)
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.../cpsw_ale.c:145:19: note: expanded from macro 'DEFINE_ALE_FIELD'
  145 | static inline int cpsw_ale_get_##name(u32 *ale_entry)                   \
      |                   ^~~~~~~~~~~~~~~~~~~
<scratch space>:196:1: note: expanded from here
  196 | cpsw_ale_get_mcast_state
      | ^~~~~~~~~~~~~~~~~~~~~~~~

Compile tested only.
No functional change intended.

Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/ti/cpsw_ale.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
index 64bf22cd860c..d37b4ddd6787 100644
--- a/drivers/net/ethernet/ti/cpsw_ale.c
+++ b/drivers/net/ethernet/ti/cpsw_ale.c
@@ -141,27 +141,39 @@ static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
 	ale_entry[idx] |=  (value << start);
 }
 
-#define DEFINE_ALE_FIELD(name, start, bits)				\
+#define DEFINE_ALE_FIELD_GET(name, start, bits)				\
 static inline int cpsw_ale_get_##name(u32 *ale_entry)			\
 {									\
 	return cpsw_ale_get_field(ale_entry, start, bits);		\
-}									\
+}
+
+#define DEFINE_ALE_FIELD_SET(name, start, bits)				\
 static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value)	\
 {									\
 	cpsw_ale_set_field(ale_entry, start, bits, value);		\
 }
 
-#define DEFINE_ALE_FIELD1(name, start)					\
+#define DEFINE_ALE_FIELD(name, start, bits)				\
+DEFINE_ALE_FIELD_GET(name, start, bits)					\
+DEFINE_ALE_FIELD_SET(name, start, bits)
+
+#define DEFINE_ALE_FIELD1_GET(name, start)				\
 static inline int cpsw_ale_get_##name(u32 *ale_entry, u32 bits)		\
 {									\
 	return cpsw_ale_get_field(ale_entry, start, bits);		\
-}									\
+}
+
+#define DEFINE_ALE_FIELD1_SET(name, start)				\
 static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value,	\
 		u32 bits)						\
 {									\
 	cpsw_ale_set_field(ale_entry, start, bits, value);		\
 }
 
+#define DEFINE_ALE_FIELD1(name, start)					\
+DEFINE_ALE_FIELD1_GET(name, start)					\
+DEFINE_ALE_FIELD1_SET(name, start)
+
 enum {
 	ALE_ENT_VID_MEMBER_LIST = 0,
 	ALE_ENT_VID_UNREG_MCAST_MSK,
@@ -217,14 +229,14 @@ static const struct ale_entry_fld vlan_entry_k3_cpswxg[] = {
 
 DEFINE_ALE_FIELD(entry_type,		60,	2)
 DEFINE_ALE_FIELD(vlan_id,		48,	12)
-DEFINE_ALE_FIELD(mcast_state,		62,	2)
+DEFINE_ALE_FIELD_SET(mcast_state,	62,	2)
 DEFINE_ALE_FIELD1(port_mask,		66)
 DEFINE_ALE_FIELD(super,			65,	1)
 DEFINE_ALE_FIELD(ucast_type,		62,     2)
-DEFINE_ALE_FIELD1(port_num,		66)
-DEFINE_ALE_FIELD(blocked,		65,     1)
-DEFINE_ALE_FIELD(secure,		64,     1)
-DEFINE_ALE_FIELD(mcast,			40,	1)
+DEFINE_ALE_FIELD1_SET(port_num,		66)
+DEFINE_ALE_FIELD_SET(blocked,		65,     1)
+DEFINE_ALE_FIELD_SET(secure,		64,     1)
+DEFINE_ALE_FIELD_GET(mcast,		40,	1)
 
 #define NU_VLAN_UNREG_MCAST_IDX	1
 

-- 
2.45.2

Re: [PATCH net-next 2/3] net: ethernet: ti: am65-cpsw: Use __be64 type for id_temp

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On Tue, Sep 10, 2024 at 12:48 PM Simon Horman <horms@kernel.org> wrote:
>
> The id_temp local variable in am65_cpsw_nuss_probe() is
> used to hold a 64-bit big-endian value as it is assigned using
> cpu_to_be64().
>
> It is read using memcpy(), where it is written as an identifier into a
> byte-array.  So this can also be treated as big endian.
>
> As it's type is currently host byte order (u64), sparse flags
> an endian mismatch when compiling for little-endian systems:
>
> .../am65-cpsw-nuss.c:3454:17: warning: incorrect type in assignment (different base types)
> .../am65-cpsw-nuss.c:3454:17:    expected unsigned long long [usertype] id_temp
> .../am65-cpsw-nuss.c:3454:17:    got restricted __be64 [usertype]
>
> Address this by using __be64 as the type of id_temp.
>
> No functional change intended.
> Compile tested only.
>
> Signed-off-by: Simon Horman <horms@kernel.org>

Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index a4b0e4bb7529..9e6353e0361e 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -3444,7 +3444,7 @@ static int am65_cpsw_nuss_probe(struct platform_device *pdev)
>         struct resource *res;
>         struct clk *clk;
>         int ale_entries;
> -       u64 id_temp;
> +       __be64 id_temp;
>         int ret, i;
>
>         common = devm_kzalloc(dev, sizeof(struct am65_cpsw_common), GFP_KERNEL);
>
> --
> 2.45.2
>
>


-- 
Regards,
Kalesh A P

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH net-next 1/3] net: ethernet: ti: am65-cpsw: Address __percpu Sparse warnings

[Jakub Kicinski]

On Tue, 10 Sep 2024 08:17:56 +0100 Simon Horman wrote:
> An alternate, approach would be to create a variant of
> devm_add_action_or_reset() which expects __percpu data.  This would
> avoid discarding the __percpu annotation, and any value it may have
> between the casts added by this patch.  However, doing so appears to
> require a significant amount of plumbing.  And, as far as I can see, the
> code updated by this patch would be the only user of it.  So this patch
> takes a simpler approach.

Sorry if this was already discussed, but struct am65_cpsw_ndev_stats
appears to be identical to struct pcpu_sw_netstats but for ordering.
Can we let the core allocate the stats by setting
netdev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS?

Re: [PATCH net-next 3/3] net: ethernet: ti: cpsw_ale: Remove unused accessor functions

[Roger Quadros]

Hi Simon,

On 10/09/2024 10:17, Simon Horman wrote:
> W=1 builds flag that some accessor functions for ALE fields are unused.
> 
> Address this by splitting up the macros used to define these
> accessors to allow only those that are used to be declared.
> 
> The warnings are verbose, but for example, the mcast_state case is
> flagged by clang-18 as:
> 
> .../cpsw_ale.c:220:1: warning: unused function 'cpsw_ale_get_mcast_state' [-Wunused-function]
>   220 | DEFINE_ALE_FIELD(mcast_state,           62,     2)
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> .../cpsw_ale.c:145:19: note: expanded from macro 'DEFINE_ALE_FIELD'
>   145 | static inline int cpsw_ale_get_##name(u32 *ale_entry)                   \
>       |                   ^~~~~~~~~~~~~~~~~~~
> <scratch space>:196:1: note: expanded from here
>   196 | cpsw_ale_get_mcast_state
>       | ^~~~~~~~~~~~~~~~~~~~~~~~
> 
> Compile tested only.
> No functional change intended.
> 
> Signed-off-by: Simon Horman <horms@kernel.org>
> ---
>  drivers/net/ethernet/ti/cpsw_ale.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
> index 64bf22cd860c..d37b4ddd6787 100644
> --- a/drivers/net/ethernet/ti/cpsw_ale.c
> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
> @@ -141,27 +141,39 @@ static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
>  	ale_entry[idx] |=  (value << start);
>  }
>  
> -#define DEFINE_ALE_FIELD(name, start, bits)				\
> +#define DEFINE_ALE_FIELD_GET(name, start, bits)				\
>  static inline int cpsw_ale_get_##name(u32 *ale_entry)			\
>  {									\
>  	return cpsw_ale_get_field(ale_entry, start, bits);		\
> -}									\
> +}
> +
> +#define DEFINE_ALE_FIELD_SET(name, start, bits)				\
>  static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value)	\
>  {									\
>  	cpsw_ale_set_field(ale_entry, start, bits, value);		\
>  }
>  
> -#define DEFINE_ALE_FIELD1(name, start)					\
> +#define DEFINE_ALE_FIELD(name, start, bits)				\
> +DEFINE_ALE_FIELD_GET(name, start, bits)					\
> +DEFINE_ALE_FIELD_SET(name, start, bits)
> +
> +#define DEFINE_ALE_FIELD1_GET(name, start)				\
>  static inline int cpsw_ale_get_##name(u32 *ale_entry, u32 bits)		\
>  {									\
>  	return cpsw_ale_get_field(ale_entry, start, bits);		\
> -}									\
> +}
> +
> +#define DEFINE_ALE_FIELD1_SET(name, start)				\
>  static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value,	\
>  		u32 bits)						\
>  {									\
>  	cpsw_ale_set_field(ale_entry, start, bits, value);		\
>  }
>  
> +#define DEFINE_ALE_FIELD1(name, start)					\
> +DEFINE_ALE_FIELD1_GET(name, start)					\
> +DEFINE_ALE_FIELD1_SET(name, start)
> +
>  enum {
>  	ALE_ENT_VID_MEMBER_LIST = 0,
>  	ALE_ENT_VID_UNREG_MCAST_MSK,
> @@ -217,14 +229,14 @@ static const struct ale_entry_fld vlan_entry_k3_cpswxg[] = {
>  
>  DEFINE_ALE_FIELD(entry_type,		60,	2)
>  DEFINE_ALE_FIELD(vlan_id,		48,	12)
> -DEFINE_ALE_FIELD(mcast_state,		62,	2)
> +DEFINE_ALE_FIELD_SET(mcast_state,	62,	2)

I don't understand why we need separate macros for GET and SET.
The original intent was to use one macro for both.

Otherwise we will have to add DEFINE_ALE_FIELD/1_SET to all the fields.

>  DEFINE_ALE_FIELD1(port_mask,		66)
>  DEFINE_ALE_FIELD(super,			65,	1)
>  DEFINE_ALE_FIELD(ucast_type,		62,     2)
> -DEFINE_ALE_FIELD1(port_num,		66)
> -DEFINE_ALE_FIELD(blocked,		65,     1)
> -DEFINE_ALE_FIELD(secure,		64,     1)
> -DEFINE_ALE_FIELD(mcast,			40,	1)
> +DEFINE_ALE_FIELD1_SET(port_num,		66)
> +DEFINE_ALE_FIELD_SET(blocked,		65,     1)
> +DEFINE_ALE_FIELD_SET(secure,		64,     1)
> +DEFINE_ALE_FIELD_GET(mcast,		40,	1)
>  
>  #define NU_VLAN_UNREG_MCAST_IDX	1
>  
> 

-- 
cheers,
-roger

Re: [PATCH net-next 2/3] net: ethernet: ti: am65-cpsw: Use __be64 type for id_temp

[Roger Quadros]

On 10/09/2024 10:17, Simon Horman wrote:
> The id_temp local variable in am65_cpsw_nuss_probe() is
> used to hold a 64-bit big-endian value as it is assigned using
> cpu_to_be64().
> 
> It is read using memcpy(), where it is written as an identifier into a
> byte-array.  So this can also be treated as big endian.
> 
> As it's type is currently host byte order (u64), sparse flags
> an endian mismatch when compiling for little-endian systems:
> 
> .../am65-cpsw-nuss.c:3454:17: warning: incorrect type in assignment (different base types)
> .../am65-cpsw-nuss.c:3454:17:    expected unsigned long long [usertype] id_temp
> .../am65-cpsw-nuss.c:3454:17:    got restricted __be64 [usertype]
> 
> Address this by using __be64 as the type of id_temp.
> 
> No functional change intended.
> Compile tested only.
> 
> Signed-off-by: Simon Horman <horms@kernel.org>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH net-next 3/3] net: ethernet: ti: cpsw_ale: Remove unused accessor functions

[Simon Horman]

On Thu, Sep 12, 2024 at 10:07:27AM +0300, Roger Quadros wrote:
> Hi Simon,
> 
> On 10/09/2024 10:17, Simon Horman wrote:
> > W=1 builds flag that some accessor functions for ALE fields are unused.
> > 
> > Address this by splitting up the macros used to define these
> > accessors to allow only those that are used to be declared.
> > 
> > The warnings are verbose, but for example, the mcast_state case is
> > flagged by clang-18 as:
> > 
> > .../cpsw_ale.c:220:1: warning: unused function 'cpsw_ale_get_mcast_state' [-Wunused-function]
> >   220 | DEFINE_ALE_FIELD(mcast_state,           62,     2)
> >       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > .../cpsw_ale.c:145:19: note: expanded from macro 'DEFINE_ALE_FIELD'
> >   145 | static inline int cpsw_ale_get_##name(u32 *ale_entry)                   \
> >       |                   ^~~~~~~~~~~~~~~~~~~
> > <scratch space>:196:1: note: expanded from here
> >   196 | cpsw_ale_get_mcast_state
> >       | ^~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Compile tested only.
> > No functional change intended.
> > 
> > Signed-off-by: Simon Horman <horms@kernel.org>
> > ---
> >  drivers/net/ethernet/ti/cpsw_ale.c | 30 +++++++++++++++++++++---------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
> > index 64bf22cd860c..d37b4ddd6787 100644
> > --- a/drivers/net/ethernet/ti/cpsw_ale.c
> > +++ b/drivers/net/ethernet/ti/cpsw_ale.c
> > @@ -141,27 +141,39 @@ static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
> >  	ale_entry[idx] |=  (value << start);
> >  }
> >  
> > -#define DEFINE_ALE_FIELD(name, start, bits)				\
> > +#define DEFINE_ALE_FIELD_GET(name, start, bits)				\
> >  static inline int cpsw_ale_get_##name(u32 *ale_entry)			\
> >  {									\
> >  	return cpsw_ale_get_field(ale_entry, start, bits);		\
> > -}									\
> > +}
> > +
> > +#define DEFINE_ALE_FIELD_SET(name, start, bits)				\
> >  static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value)	\
> >  {									\
> >  	cpsw_ale_set_field(ale_entry, start, bits, value);		\
> >  }
> >  
> > -#define DEFINE_ALE_FIELD1(name, start)					\
> > +#define DEFINE_ALE_FIELD(name, start, bits)				\
> > +DEFINE_ALE_FIELD_GET(name, start, bits)					\
> > +DEFINE_ALE_FIELD_SET(name, start, bits)
> > +
> > +#define DEFINE_ALE_FIELD1_GET(name, start)				\
> >  static inline int cpsw_ale_get_##name(u32 *ale_entry, u32 bits)		\
> >  {									\
> >  	return cpsw_ale_get_field(ale_entry, start, bits);		\
> > -}									\
> > +}
> > +
> > +#define DEFINE_ALE_FIELD1_SET(name, start)				\
> >  static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value,	\
> >  		u32 bits)						\
> >  {									\
> >  	cpsw_ale_set_field(ale_entry, start, bits, value);		\
> >  }
> >  
> > +#define DEFINE_ALE_FIELD1(name, start)					\
> > +DEFINE_ALE_FIELD1_GET(name, start)					\
> > +DEFINE_ALE_FIELD1_SET(name, start)
> > +
> >  enum {
> >  	ALE_ENT_VID_MEMBER_LIST = 0,
> >  	ALE_ENT_VID_UNREG_MCAST_MSK,
> > @@ -217,14 +229,14 @@ static const struct ale_entry_fld vlan_entry_k3_cpswxg[] = {
> >  
> >  DEFINE_ALE_FIELD(entry_type,		60,	2)
> >  DEFINE_ALE_FIELD(vlan_id,		48,	12)
> > -DEFINE_ALE_FIELD(mcast_state,		62,	2)
> > +DEFINE_ALE_FIELD_SET(mcast_state,	62,	2)
> 
> I don't understand why we need separate macros for GET and SET.
> The original intent was to use one macro for both.
> 
> Otherwise we will have to add DEFINE_ALE_FIELD/1_SET to all the fields.

Hi Roger,

Sorry for not being clearer.

My intent was to avoid declaring functions that are never used.
Perhaps it is best explained by some examples.

In the case of mcast_state, the compiler flags that the get accessor is
never used. The intent is of this patch addresses that by declaring the set
accessor for mcast_state. Likewise for other similar cases.

OTOH, in the case of, f.e. vlan_id, the set and get accessor functions are
both used, and DEFINE_ALE_FIELD continues to be used to define them both.
DEFINE_ALE_FIELD is implemented as the combination of _SET and _GET.

> 
> >  DEFINE_ALE_FIELD1(port_mask,		66)
> >  DEFINE_ALE_FIELD(super,			65,	1)
> >  DEFINE_ALE_FIELD(ucast_type,		62,     2)
> > -DEFINE_ALE_FIELD1(port_num,		66)
> > -DEFINE_ALE_FIELD(blocked,		65,     1)
> > -DEFINE_ALE_FIELD(secure,		64,     1)
> > -DEFINE_ALE_FIELD(mcast,			40,	1)
> > +DEFINE_ALE_FIELD1_SET(port_num,		66)
> > +DEFINE_ALE_FIELD_SET(blocked,		65,     1)
> > +DEFINE_ALE_FIELD_SET(secure,		64,     1)
> > +DEFINE_ALE_FIELD_GET(mcast,		40,	1)
> >  
> >  #define NU_VLAN_UNREG_MCAST_IDX	1
> >  
> > 
> 
> -- 
> cheers,
> -roger
> 

Re: [PATCH net-next 1/3] net: ethernet: ti: am65-cpsw: Address __percpu Sparse warnings

[Simon Horman]

On Wed, Sep 11, 2024 at 05:06:43PM -0700, Jakub Kicinski wrote:
> On Tue, 10 Sep 2024 08:17:56 +0100 Simon Horman wrote:
> > An alternate, approach would be to create a variant of
> > devm_add_action_or_reset() which expects __percpu data.  This would
> > avoid discarding the __percpu annotation, and any value it may have
> > between the casts added by this patch.  However, doing so appears to
> > require a significant amount of plumbing.  And, as far as I can see, the
> > code updated by this patch would be the only user of it.  So this patch
> > takes a simpler approach.
> 
> Sorry if this was already discussed, but struct am65_cpsw_ndev_stats
> appears to be identical to struct pcpu_sw_netstats but for ordering.
> Can we let the core allocate the stats by setting
> netdev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS?

Hi Jakub,

Thanks for pointing that out, and sorry for not thinking of it myself.

Looking over the code, and taking a first pass at implementing this,
I believe the answer is yes :)

I also think that, as a second step, by using dev_core_stats,
the custom ndo_get_stats64() implementation can be removed.
LMKWYT.

Re: [PATCH net-next 3/3] net: ethernet: ti: cpsw_ale: Remove unused accessor functions

[Roger Quadros]

On 12/09/2024 11:59, Simon Horman wrote:
> On Thu, Sep 12, 2024 at 10:07:27AM +0300, Roger Quadros wrote:
>> Hi Simon,
>>
>> On 10/09/2024 10:17, Simon Horman wrote:
>>> W=1 builds flag that some accessor functions for ALE fields are unused.
>>>
>>> Address this by splitting up the macros used to define these
>>> accessors to allow only those that are used to be declared.
>>>
>>> The warnings are verbose, but for example, the mcast_state case is
>>> flagged by clang-18 as:
>>>
>>> .../cpsw_ale.c:220:1: warning: unused function 'cpsw_ale_get_mcast_state' [-Wunused-function]
>>>   220 | DEFINE_ALE_FIELD(mcast_state,           62,     2)
>>>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> .../cpsw_ale.c:145:19: note: expanded from macro 'DEFINE_ALE_FIELD'
>>>   145 | static inline int cpsw_ale_get_##name(u32 *ale_entry)                   \
>>>       |                   ^~~~~~~~~~~~~~~~~~~
>>> <scratch space>:196:1: note: expanded from here
>>>   196 | cpsw_ale_get_mcast_state
>>>       | ^~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Compile tested only.
>>> No functional change intended.
>>>
>>> Signed-off-by: Simon Horman <horms@kernel.org>
>>> ---
>>>  drivers/net/ethernet/ti/cpsw_ale.c | 30 +++++++++++++++++++++---------
>>>  1 file changed, 21 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/cpsw_ale.c b/drivers/net/ethernet/ti/cpsw_ale.c
>>> index 64bf22cd860c..d37b4ddd6787 100644
>>> --- a/drivers/net/ethernet/ti/cpsw_ale.c
>>> +++ b/drivers/net/ethernet/ti/cpsw_ale.c
>>> @@ -141,27 +141,39 @@ static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
>>>  	ale_entry[idx] |=  (value << start);
>>>  }
>>>  
>>> -#define DEFINE_ALE_FIELD(name, start, bits)				\
>>> +#define DEFINE_ALE_FIELD_GET(name, start, bits)				\
>>>  static inline int cpsw_ale_get_##name(u32 *ale_entry)			\
>>>  {									\
>>>  	return cpsw_ale_get_field(ale_entry, start, bits);		\
>>> -}									\
>>> +}
>>> +
>>> +#define DEFINE_ALE_FIELD_SET(name, start, bits)				\
>>>  static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value)	\
>>>  {									\
>>>  	cpsw_ale_set_field(ale_entry, start, bits, value);		\
>>>  }
>>>  
>>> -#define DEFINE_ALE_FIELD1(name, start)					\
>>> +#define DEFINE_ALE_FIELD(name, start, bits)				\
>>> +DEFINE_ALE_FIELD_GET(name, start, bits)					\
>>> +DEFINE_ALE_FIELD_SET(name, start, bits)
>>> +
>>> +#define DEFINE_ALE_FIELD1_GET(name, start)				\
>>>  static inline int cpsw_ale_get_##name(u32 *ale_entry, u32 bits)		\
>>>  {									\
>>>  	return cpsw_ale_get_field(ale_entry, start, bits);		\
>>> -}									\
>>> +}
>>> +
>>> +#define DEFINE_ALE_FIELD1_SET(name, start)				\
>>>  static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value,	\
>>>  		u32 bits)						\
>>>  {									\
>>>  	cpsw_ale_set_field(ale_entry, start, bits, value);		\
>>>  }
>>>  
>>> +#define DEFINE_ALE_FIELD1(name, start)					\
>>> +DEFINE_ALE_FIELD1_GET(name, start)					\
>>> +DEFINE_ALE_FIELD1_SET(name, start)
>>> +
>>>  enum {
>>>  	ALE_ENT_VID_MEMBER_LIST = 0,
>>>  	ALE_ENT_VID_UNREG_MCAST_MSK,
>>> @@ -217,14 +229,14 @@ static const struct ale_entry_fld vlan_entry_k3_cpswxg[] = {
>>>  
>>>  DEFINE_ALE_FIELD(entry_type,		60,	2)
>>>  DEFINE_ALE_FIELD(vlan_id,		48,	12)
>>> -DEFINE_ALE_FIELD(mcast_state,		62,	2)
>>> +DEFINE_ALE_FIELD_SET(mcast_state,	62,	2)
>>
>> I don't understand why we need separate macros for GET and SET.
>> The original intent was to use one macro for both.
>>
>> Otherwise we will have to add DEFINE_ALE_FIELD/1_SET to all the fields.
> 
> Hi Roger,
> 
> Sorry for not being clearer.
> 
> My intent was to avoid declaring functions that are never used.
> Perhaps it is best explained by some examples.
> 
> In the case of mcast_state, the compiler flags that the get accessor is
> never used. The intent is of this patch addresses that by declaring the set
> accessor for mcast_state. Likewise for other similar cases.
> 
> OTOH, in the case of, f.e. vlan_id, the set and get accessor functions are
> both used, and DEFINE_ALE_FIELD continues to be used to define them both.
> DEFINE_ALE_FIELD is implemented as the combination of _SET and _GET.
> 

Thanks for the explanation Simon. I understand now.

Would using __maybe_unused__ be preferable to get rid of the warnings?
That way we don't need to care if both set/get helpers are used or not
and don't have to touch the below code ever again except to add new fields.

>>
>>>  DEFINE_ALE_FIELD1(port_mask,		66)
>>>  DEFINE_ALE_FIELD(super,			65,	1)
>>>  DEFINE_ALE_FIELD(ucast_type,		62,     2)
>>> -DEFINE_ALE_FIELD1(port_num,		66)
>>> -DEFINE_ALE_FIELD(blocked,		65,     1)
>>> -DEFINE_ALE_FIELD(secure,		64,     1)
>>> -DEFINE_ALE_FIELD(mcast,			40,	1)
>>> +DEFINE_ALE_FIELD1_SET(port_num,		66)
>>> +DEFINE_ALE_FIELD_SET(blocked,		65,     1)
>>> +DEFINE_ALE_FIELD_SET(secure,		64,     1)
>>> +DEFINE_ALE_FIELD_GET(mcast,		40,	1)
>>>  
>>>  #define NU_VLAN_UNREG_MCAST_IDX	1
>>>  
>>>
>>

-- 
cheers,
-roger

Re: [PATCH net-next 3/3] net: ethernet: ti: cpsw_ale: Remove unused accessor functions

[Simon Horman]

On Thu, Sep 12, 2024 at 01:54:45PM +0300, Roger Quadros wrote:
> 
> 
> On 12/09/2024 11:59, Simon Horman wrote:
> > On Thu, Sep 12, 2024 at 10:07:27AM +0300, Roger Quadros wrote:
> >> Hi Simon,
> >>
> >> On 10/09/2024 10:17, Simon Horman wrote:

...

> >>>  	ALE_ENT_VID_MEMBER_LIST = 0,
> >>>  	ALE_ENT_VID_UNREG_MCAST_MSK,
> >>> @@ -217,14 +229,14 @@ static const struct ale_entry_fld vlan_entry_k3_cpswxg[] = {
> >>>  
> >>>  DEFINE_ALE_FIELD(entry_type,		60,	2)
> >>>  DEFINE_ALE_FIELD(vlan_id,		48,	12)
> >>> -DEFINE_ALE_FIELD(mcast_state,		62,	2)
> >>> +DEFINE_ALE_FIELD_SET(mcast_state,	62,	2)
> >>
> >> I don't understand why we need separate macros for GET and SET.
> >> The original intent was to use one macro for both.
> >>
> >> Otherwise we will have to add DEFINE_ALE_FIELD/1_SET to all the fields.
> > 
> > Hi Roger,
> > 
> > Sorry for not being clearer.
> > 
> > My intent was to avoid declaring functions that are never used.
> > Perhaps it is best explained by some examples.
> > 
> > In the case of mcast_state, the compiler flags that the get accessor is
> > never used. The intent is of this patch addresses that by declaring the set
> > accessor for mcast_state. Likewise for other similar cases.
> > 
> > OTOH, in the case of, f.e. vlan_id, the set and get accessor functions are
> > both used, and DEFINE_ALE_FIELD continues to be used to define them both.
> > DEFINE_ALE_FIELD is implemented as the combination of _SET and _GET.
> > 
> 
> Thanks for the explanation Simon. I understand now.
> 
> Would using __maybe_unused__ be preferable to get rid of the warnings?
> That way we don't need to care if both set/get helpers are used or not
> and don't have to touch the below code ever again except to add new fields.

Thanks Roger,

IMHO, it is nicer to not declare them at all.  But I do get your point and
I'm happy to try that approach if you prefer it.

...

Re: [PATCH net-next 3/3] net: ethernet: ti: cpsw_ale: Remove unused accessor functions

[Roger Quadros]

On 12/09/2024 14:27, Simon Horman wrote:
> On Thu, Sep 12, 2024 at 01:54:45PM +0300, Roger Quadros wrote:
>>
>>
>> On 12/09/2024 11:59, Simon Horman wrote:
>>> On Thu, Sep 12, 2024 at 10:07:27AM +0300, Roger Quadros wrote:
>>>> Hi Simon,
>>>>
>>>> On 10/09/2024 10:17, Simon Horman wrote:
> 
> ...
> 
>>>>>  	ALE_ENT_VID_MEMBER_LIST = 0,
>>>>>  	ALE_ENT_VID_UNREG_MCAST_MSK,
>>>>> @@ -217,14 +229,14 @@ static const struct ale_entry_fld vlan_entry_k3_cpswxg[] = {
>>>>>  
>>>>>  DEFINE_ALE_FIELD(entry_type,		60,	2)
>>>>>  DEFINE_ALE_FIELD(vlan_id,		48,	12)
>>>>> -DEFINE_ALE_FIELD(mcast_state,		62,	2)
>>>>> +DEFINE_ALE_FIELD_SET(mcast_state,	62,	2)
>>>>
>>>> I don't understand why we need separate macros for GET and SET.
>>>> The original intent was to use one macro for both.
>>>>
>>>> Otherwise we will have to add DEFINE_ALE_FIELD/1_SET to all the fields.
>>>
>>> Hi Roger,
>>>
>>> Sorry for not being clearer.
>>>
>>> My intent was to avoid declaring functions that are never used.
>>> Perhaps it is best explained by some examples.
>>>
>>> In the case of mcast_state, the compiler flags that the get accessor is
>>> never used. The intent is of this patch addresses that by declaring the set
>>> accessor for mcast_state. Likewise for other similar cases.
>>>
>>> OTOH, in the case of, f.e. vlan_id, the set and get accessor functions are
>>> both used, and DEFINE_ALE_FIELD continues to be used to define them both.
>>> DEFINE_ALE_FIELD is implemented as the combination of _SET and _GET.
>>>
>>
>> Thanks for the explanation Simon. I understand now.
>>
>> Would using __maybe_unused__ be preferable to get rid of the warnings?
>> That way we don't need to care if both set/get helpers are used or not
>> and don't have to touch the below code ever again except to add new fields.
> 
> Thanks Roger,
> 
> IMHO, it is nicer to not declare them at all.  But I do get your point and
> I'm happy to try that approach if you prefer it.
> 
> ...

Simon,

I don't have any preference. I'll leave it to you to decide on your next spin. Thanks.

-- 
cheers,
-roger

Re: [PATCH net-next 3/3] net: ethernet: ti: cpsw_ale: Remove unused accessor functions

[Simon Horman]

On Thu, Sep 12, 2024 at 04:29:29PM +0300, Roger Quadros wrote:
> 
> 
> On 12/09/2024 14:27, Simon Horman wrote:
> > On Thu, Sep 12, 2024 at 01:54:45PM +0300, Roger Quadros wrote:
> >>
> >>
> >> On 12/09/2024 11:59, Simon Horman wrote:
> >>> On Thu, Sep 12, 2024 at 10:07:27AM +0300, Roger Quadros wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On 10/09/2024 10:17, Simon Horman wrote:
> > 
> > ...
> > 
> >>>>>  	ALE_ENT_VID_MEMBER_LIST = 0,
> >>>>>  	ALE_ENT_VID_UNREG_MCAST_MSK,
> >>>>> @@ -217,14 +229,14 @@ static const struct ale_entry_fld vlan_entry_k3_cpswxg[] = {
> >>>>>  
> >>>>>  DEFINE_ALE_FIELD(entry_type,		60,	2)
> >>>>>  DEFINE_ALE_FIELD(vlan_id,		48,	12)
> >>>>> -DEFINE_ALE_FIELD(mcast_state,		62,	2)
> >>>>> +DEFINE_ALE_FIELD_SET(mcast_state,	62,	2)
> >>>>
> >>>> I don't understand why we need separate macros for GET and SET.
> >>>> The original intent was to use one macro for both.
> >>>>
> >>>> Otherwise we will have to add DEFINE_ALE_FIELD/1_SET to all the fields.
> >>>
> >>> Hi Roger,
> >>>
> >>> Sorry for not being clearer.
> >>>
> >>> My intent was to avoid declaring functions that are never used.
> >>> Perhaps it is best explained by some examples.
> >>>
> >>> In the case of mcast_state, the compiler flags that the get accessor is
> >>> never used. The intent is of this patch addresses that by declaring the set
> >>> accessor for mcast_state. Likewise for other similar cases.
> >>>
> >>> OTOH, in the case of, f.e. vlan_id, the set and get accessor functions are
> >>> both used, and DEFINE_ALE_FIELD continues to be used to define them both.
> >>> DEFINE_ALE_FIELD is implemented as the combination of _SET and _GET.
> >>>
> >>
> >> Thanks for the explanation Simon. I understand now.
> >>
> >> Would using __maybe_unused__ be preferable to get rid of the warnings?
> >> That way we don't need to care if both set/get helpers are used or not
> >> and don't have to touch the below code ever again except to add new fields.
> > 
> > Thanks Roger,
> > 
> > IMHO, it is nicer to not declare them at all.  But I do get your point and
> > I'm happy to try that approach if you prefer it.
> > 
> > ...
> 
> Simon,
> 
> I don't have any preference. I'll leave it to you to decide on your next spin. Thanks.

Thanks, let me think about it.

I'll probably hold off on v2 until the next the development cycle as
the patch queue seems busy enough this week.

Re: [PATCH net-next 1/3] net: ethernet: ti: am65-cpsw: Address __percpu Sparse warnings

[Jakub Kicinski]

On Thu, 12 Sep 2024 10:58:13 +0100 Simon Horman wrote:
> Thanks for pointing that out, and sorry for not thinking of it myself.
> 
> Looking over the code, and taking a first pass at implementing this,
> I believe the answer is yes :)
> 
> I also think that, as a second step, by using dev_core_stats,
> the custom ndo_get_stats64() implementation can be removed.
> LMKWYT.

Second step or one conversion patch, no preference. But AFAICT you're
right, the ndo can be completely removed thanks to the conversion.

Re: [PATCH net-next 1/3] net: ethernet: ti: am65-cpsw: Address __percpu Sparse warnings

[Simon Horman]

On Thu, Sep 12, 2024 at 08:45:15AM -0700, Jakub Kicinski wrote:
> On Thu, 12 Sep 2024 10:58:13 +0100 Simon Horman wrote:
> > Thanks for pointing that out, and sorry for not thinking of it myself.
> > 
> > Looking over the code, and taking a first pass at implementing this,
> > I believe the answer is yes :)
> > 
> > I also think that, as a second step, by using dev_core_stats,
> > the custom ndo_get_stats64() implementation can be removed.
> > LMKWYT.
> 
> Second step or one conversion patch, no preference. But AFAICT you're
> right, the ndo can be completely removed thanks to the conversion.

Thanks, I'll see about making it so.