net: macb: sparse warning fixes

net: macb: sparse warning fixes

[Ben Dooks]

These are 3 hopefully easy patches for fixing sparse errors due to
endian-ness warnings. There are still some left, but there are not
as easy as they mix host and network fields together.

For example, gem_prog_cmp_regs() has two u32 variables that it does
bitfield manipulation on for the tcp ports and these are __be16 into
u32, so not sure how these are meant to be changed. I've also no hardware
to test on, so even if these did get changed then I can't check if it is
working pre/post change.

Also gem_writel and gem_writel_n, it is not clear if both of these are
meant to be host order or not.

Ben Dooks (3):
  net: macb: check constant to define and fix __be32 warnings
  net: macb: add port constant to fix __be16 warnings
  net: macb: fix __be32 warnings in debug code

 drivers/net/ethernet/cadence/macb_main.c | 25 +++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

-- 
2.40.1

[PATCH 1/3] net: macb: check constant to define and fix __be32 warnings

[Ben Dooks]

The checks on ipv4 addresses in the filtering code check against
a constant of 0xFFFFFFFF, so replace it with MACB_IPV4_MASK and
then make sure it is of __be32 type to avoid the following
sparse warnigns:

drivers/net/ethernet/cadence/macb_main.c:3448:39: warning: restricted __be32 degrades to integer
drivers/net/ethernet/cadence/macb_main.c:3453:39: warning: restricted __be32 degrades to integer
drivers/net/ethernet/cadence/macb_main.c:3483:20: warning: restricted __be32 degrades to integer
drivers/net/ethernet/cadence/macb_main.c:3497:20: warning: restricted __be32 degrades to integer

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/net/ethernet/cadence/macb_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index f20ec0d5260b..538d4c7e023b 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3418,6 +3418,8 @@ static int macb_get_ts_info(struct net_device *netdev,
 	return ethtool_op_get_ts_info(netdev, info);
 }
 
+#define MACB_IPV4_MASK htonl(0xFFFFFFFF)
+
 static void gem_enable_flow_filters(struct macb *bp, bool enable)
 {
 	struct net_device *netdev = bp->dev;
@@ -3445,12 +3447,12 @@ static void gem_enable_flow_filters(struct macb *bp, bool enable)
 		/* only enable fields with no masking */
 		tp4sp_m = &(fs->m_u.tcp_ip4_spec);
 
-		if (enable && (tp4sp_m->ip4src == 0xFFFFFFFF))
+		if (enable && (tp4sp_m->ip4src == MACB_IPV4_MASK))
 			t2_scr = GEM_BFINS(CMPAEN, 1, t2_scr);
 		else
 			t2_scr = GEM_BFINS(CMPAEN, 0, t2_scr);
 
-		if (enable && (tp4sp_m->ip4dst == 0xFFFFFFFF))
+		if (enable && (tp4sp_m->ip4dst == MACB_IPV4_MASK))
 			t2_scr = GEM_BFINS(CMPBEN, 1, t2_scr);
 		else
 			t2_scr = GEM_BFINS(CMPBEN, 0, t2_scr);
@@ -3480,7 +3482,7 @@ static void gem_prog_cmp_regs(struct macb *bp, struct ethtool_rx_flow_spec *fs)
 	tp4sp_m = &(fs->m_u.tcp_ip4_spec);
 
 	/* ignore field if any masking set */
-	if (tp4sp_m->ip4src == 0xFFFFFFFF) {
+	if (tp4sp_m->ip4src == MACB_IPV4_MASK) {
 		/* 1st compare reg - IP source address */
 		w0 = 0;
 		w1 = 0;
@@ -3494,7 +3496,7 @@ static void gem_prog_cmp_regs(struct macb *bp, struct ethtool_rx_flow_spec *fs)
 	}
 
 	/* ignore field if any masking set */
-	if (tp4sp_m->ip4dst == 0xFFFFFFFF) {
+	if (tp4sp_m->ip4dst == MACB_IPV4_MASK) {
 		/* 2nd compare reg - IP destination address */
 		w0 = 0;
 		w1 = 0;
-- 
2.40.1

Re: [PATCH 1/3] net: macb: check constant to define and fix __be32 warnings

[Simon Horman]

On Thu, Jun 22, 2023 at 02:05:05PM +0100, Ben Dooks wrote:
> The checks on ipv4 addresses in the filtering code check against
> a constant of 0xFFFFFFFF, so replace it with MACB_IPV4_MASK and
> then make sure it is of __be32 type to avoid the following
> sparse warnigns:
> 
> drivers/net/ethernet/cadence/macb_main.c:3448:39: warning: restricted __be32 degrades to integer
> drivers/net/ethernet/cadence/macb_main.c:3453:39: warning: restricted __be32 degrades to integer
> drivers/net/ethernet/cadence/macb_main.c:3483:20: warning: restricted __be32 degrades to integer
> drivers/net/ethernet/cadence/macb_main.c:3497:20: warning: restricted __be32 degrades to integer
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index f20ec0d5260b..538d4c7e023b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3418,6 +3418,8 @@ static int macb_get_ts_info(struct net_device *netdev,
>  	return ethtool_op_get_ts_info(netdev, info);
>  }
>  
> +#define MACB_IPV4_MASK htonl(0xFFFFFFFF)
> +

Hi Ben,

according to a recent thread, it seems that the preferred approach might be
~(__le32)0.

https://lore.kernel.org/netdev/20230522153615.247577-1-minhuadotchen@gmail.com/

Re: [PATCH 1/3] net: macb: check constant to define and fix __be32 warnings

[Ben Dooks]

On 2023-06-22 16:49, Simon Horman wrote:
> On Thu, Jun 22, 2023 at 02:05:05PM +0100, Ben Dooks wrote:
>> The checks on ipv4 addresses in the filtering code check against
>> a constant of 0xFFFFFFFF, so replace it with MACB_IPV4_MASK and
>> then make sure it is of __be32 type to avoid the following
>> sparse warnigns:
>> 
>> drivers/net/ethernet/cadence/macb_main.c:3448:39: warning: restricted 
>> __be32 degrades to integer
>> drivers/net/ethernet/cadence/macb_main.c:3453:39: warning: restricted 
>> __be32 degrades to integer
>> drivers/net/ethernet/cadence/macb_main.c:3483:20: warning: restricted 
>> __be32 degrades to integer
>> drivers/net/ethernet/cadence/macb_main.c:3497:20: warning: restricted 
>> __be32 degrades to integer
>> 
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index f20ec0d5260b..538d4c7e023b 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -3418,6 +3418,8 @@ static int macb_get_ts_info(struct net_device 
>> *netdev,
>>  	return ethtool_op_get_ts_info(netdev, info);
>>  }
>> 
>> +#define MACB_IPV4_MASK htonl(0xFFFFFFFF)
>> +
> 
> Hi Ben,
> 
> according to a recent thread, it seems that the preferred approach 
> might be
> ~(__le32)0.
> 
> https://lore.kernel.org/netdev/20230522153615.247577-1-minhuadotchen@gmail.com/

Out of interest, should we keep the define then or simply go through 
changing
all the places where change is needed?

-- 
Ben

Re: [PATCH 1/3] net: macb: check constant to define and fix __be32 warnings

[Simon Horman]

On Fri, Jun 23, 2023 at 10:40:52AM +0100, Ben Dooks wrote:
> 
> 
> On 2023-06-22 16:49, Simon Horman wrote:
> > On Thu, Jun 22, 2023 at 02:05:05PM +0100, Ben Dooks wrote:
> > > The checks on ipv4 addresses in the filtering code check against
> > > a constant of 0xFFFFFFFF, so replace it with MACB_IPV4_MASK and
> > > then make sure it is of __be32 type to avoid the following
> > > sparse warnigns:
> > > 
> > > drivers/net/ethernet/cadence/macb_main.c:3448:39: warning:
> > > restricted __be32 degrades to integer
> > > drivers/net/ethernet/cadence/macb_main.c:3453:39: warning:
> > > restricted __be32 degrades to integer
> > > drivers/net/ethernet/cadence/macb_main.c:3483:20: warning:
> > > restricted __be32 degrades to integer
> > > drivers/net/ethernet/cadence/macb_main.c:3497:20: warning:
> > > restricted __be32 degrades to integer
> > > 
> > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > > ---
> > >  drivers/net/ethernet/cadence/macb_main.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/cadence/macb_main.c
> > > b/drivers/net/ethernet/cadence/macb_main.c
> > > index f20ec0d5260b..538d4c7e023b 100644
> > > --- a/drivers/net/ethernet/cadence/macb_main.c
> > > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > > @@ -3418,6 +3418,8 @@ static int macb_get_ts_info(struct net_device
> > > *netdev,
> > >  	return ethtool_op_get_ts_info(netdev, info);
> > >  }
> > > 
> > > +#define MACB_IPV4_MASK htonl(0xFFFFFFFF)
> > > +
> > 
> > Hi Ben,
> > 
> > according to a recent thread, it seems that the preferred approach might
> > be
> > ~(__le32)0.
> > 
> > https://lore.kernel.org/netdev/20230522153615.247577-1-minhuadotchen@gmail.com/
> 
> Out of interest, should we keep the define then or simply go through
> changing
> all the places where change is needed?

My personal opinion is that a #define is a nice way to go here.

[PATCH 2/3] net: macb: add port constant to fix __be16 warnings

[Ben Dooks]

Add a constant MACB_PORT_MASK to use instead of 0xffff when checking against
ipv4 address port numbers. This allows the use of the __be16 constant and
silences the following sparse warnings:

drivers/net/ethernet/cadence/macb_main.c:3458:40: warning: restricted __be16 degrades to integer
drivers/net/ethernet/cadence/macb_main.c:3458:69: warning: restricted __be16 degrades to integer
drivers/net/ethernet/cadence/macb_main.c:3511:21: warning: restricted __be16 degrades to integer
drivers/net/ethernet/cadence/macb_main.c:3511:50: warning: restricted __be16 degrades to integer

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/net/ethernet/cadence/macb_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 538d4c7e023b..56e202b74bd7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3419,6 +3419,7 @@ static int macb_get_ts_info(struct net_device *netdev,
 }
 
 #define MACB_IPV4_MASK htonl(0xFFFFFFFF)
+#define MACB_PORT_MASK htons(0xFFFF)
 
 static void gem_enable_flow_filters(struct macb *bp, bool enable)
 {
@@ -3457,7 +3458,7 @@ static void gem_enable_flow_filters(struct macb *bp, bool enable)
 		else
 			t2_scr = GEM_BFINS(CMPBEN, 0, t2_scr);
 
-		if (enable && ((tp4sp_m->psrc == 0xFFFF) || (tp4sp_m->pdst == 0xFFFF)))
+		if (enable && ((tp4sp_m->psrc == MACB_PORT_MASK) || (tp4sp_m->pdst == MACB_PORT_MASK)))
 			t2_scr = GEM_BFINS(CMPCEN, 1, t2_scr);
 		else
 			t2_scr = GEM_BFINS(CMPCEN, 0, t2_scr);
@@ -3510,7 +3511,7 @@ static void gem_prog_cmp_regs(struct macb *bp, struct ethtool_rx_flow_spec *fs)
 	}
 
 	/* ignore both port fields if masking set in both */
-	if ((tp4sp_m->psrc == 0xFFFF) || (tp4sp_m->pdst == 0xFFFF)) {
+	if ((tp4sp_m->psrc == MACB_PORT_MASK) || (tp4sp_m->pdst == MACB_PORT_MASK)) {
 		/* 3rd compare reg - source port, destination port */
 		w0 = 0;
 		w1 = 0;
@@ -3524,7 +3525,7 @@ static void gem_prog_cmp_regs(struct macb *bp, struct ethtool_rx_flow_spec *fs)
 			/* only one port definition */
 			w1 = GEM_BFINS(T2DISMSK, 0, w1); /* 16-bit compare */
 			w0 = GEM_BFINS(T2MASK, 0xFFFF, w0);
-			if (tp4sp_m->psrc == 0xFFFF) { /* src port */
+			if (tp4sp_m->psrc == MACB_PORT_MASK) { /* src port */
 				w0 = GEM_BFINS(T2CMP, tp4sp_v->psrc, w0);
 				w1 = GEM_BFINS(T2OFST, IPHDR_SRCPORT_OFFSET, w1);
 			} else { /* dst port */
-- 
2.40.1

[PATCH 3/3] net: macb: fix __be32 warnings in debug code

[Ben Dooks]

The netdev_dbg() calls in gem_add_flow_filter() and gem_del_flow_filter()
call ntohl() on the ipv4 addresses, which will put them into the host order
but not the right type (returns a __be32, not an u32 as would be expected).

Chaning the htonl() to nthol() should still do the right conversion, return
the correct u32 type and  should not change any functional to remove the
following sparse warnings:

drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types)
drivers/net/ethernet/cadence/macb_main.c:3568:9:    expected unsigned int [usertype] val
drivers/net/ethernet/cadence/macb_main.c:3568:9:    got restricted __be32 [usertype] ip4src
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types)
drivers/net/ethernet/cadence/macb_main.c:3568:9:    expected unsigned int [usertype] val
drivers/net/ethernet/cadence/macb_main.c:3568:9:    got restricted __be32 [usertype] ip4dst
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
d
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types)
drivers/net/ethernet/cadence/macb_main.c:3622:25:    expected unsigned int [usertype] val
drivers/net/ethernet/cadence/macb_main.c:3622:25:    got restricted __be32 [usertype] ip4src
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types)
drivers/net/ethernet/cadence/macb_main.c:3622:25:    expected unsigned int [usertype] val
drivers/net/ethernet/cadence/macb_main.c:3622:25:    got restricted __be32 [usertype] ip4dst
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/net/ethernet/cadence/macb_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 56e202b74bd7..59a90c2b307f 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3568,8 +3568,8 @@ static int gem_add_flow_filter(struct net_device *netdev,
 	netdev_dbg(netdev,
 			"Adding flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n",
 			fs->flow_type, (int)fs->ring_cookie, fs->location,
-			htonl(fs->h_u.tcp_ip4_spec.ip4src),
-			htonl(fs->h_u.tcp_ip4_spec.ip4dst),
+			ntohl(fs->h_u.tcp_ip4_spec.ip4src),
+			ntohl(fs->h_u.tcp_ip4_spec.ip4dst),
 			be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc),
 			be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
 
@@ -3622,8 +3622,8 @@ static int gem_del_flow_filter(struct net_device *netdev,
 			netdev_dbg(netdev,
 					"Deleting flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n",
 					fs->flow_type, (int)fs->ring_cookie, fs->location,
-					htonl(fs->h_u.tcp_ip4_spec.ip4src),
-					htonl(fs->h_u.tcp_ip4_spec.ip4dst),
+					ntohl(fs->h_u.tcp_ip4_spec.ip4src),
+					ntohl(fs->h_u.tcp_ip4_spec.ip4dst),
 					be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc),
 					be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
 
-- 
2.40.1

Re: [PATCH 3/3] net: macb: fix __be32 warnings in debug code

[Simon Horman]

On Thu, Jun 22, 2023 at 02:05:07PM +0100, Ben Dooks wrote:
> The netdev_dbg() calls in gem_add_flow_filter() and gem_del_flow_filter()
> call ntohl() on the ipv4 addresses, which will put them into the host order
> but not the right type (returns a __be32, not an u32 as would be expected).
> 
> Chaning the htonl() to nthol() should still do the right conversion, return
> the correct u32 type and  should not change any functional to remove the
> following sparse warnings:
> 
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types)
> drivers/net/ethernet/cadence/macb_main.c:3568:9:    expected unsigned int [usertype] val
> drivers/net/ethernet/cadence/macb_main.c:3568:9:    got restricted __be32 [usertype] ip4src
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types)
> drivers/net/ethernet/cadence/macb_main.c:3568:9:    expected unsigned int [usertype] val
> drivers/net/ethernet/cadence/macb_main.c:3568:9:    got restricted __be32 [usertype] ip4dst
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32
> d
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types)
> drivers/net/ethernet/cadence/macb_main.c:3622:25:    expected unsigned int [usertype] val
> drivers/net/ethernet/cadence/macb_main.c:3622:25:    got restricted __be32 [usertype] ip4src
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types)
> drivers/net/ethernet/cadence/macb_main.c:3622:25:    expected unsigned int [usertype] val
> drivers/net/ethernet/cadence/macb_main.c:3622:25:    got restricted __be32 [usertype] ip4dst
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Hi Ben,

this code-change looks good to me, but I have a few minor nits for your
consideration.

1. Please specify the target tree, in this case net-next, for patch sets
   for Networking code.

	Subject: [PATCH net-next ...] ...

2. It might be nicer to write '.../macb_main.c' or similar,
   rather tha nthe full path, in the patch description.

3. checkpatch --codespell says: 'Chaning' -> 'Chaining'

> ---
>  drivers/net/ethernet/cadence/macb_main.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 56e202b74bd7..59a90c2b307f 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3568,8 +3568,8 @@ static int gem_add_flow_filter(struct net_device *netdev,
>  	netdev_dbg(netdev,
>  			"Adding flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n",
>  			fs->flow_type, (int)fs->ring_cookie, fs->location,
> -			htonl(fs->h_u.tcp_ip4_spec.ip4src),
> -			htonl(fs->h_u.tcp_ip4_spec.ip4dst),
> +			ntohl(fs->h_u.tcp_ip4_spec.ip4src),
> +			ntohl(fs->h_u.tcp_ip4_spec.ip4dst),
>  			be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc),
>  			be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
>  
> @@ -3622,8 +3622,8 @@ static int gem_del_flow_filter(struct net_device *netdev,
>  			netdev_dbg(netdev,
>  					"Deleting flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n",
>  					fs->flow_type, (int)fs->ring_cookie, fs->location,
> -					htonl(fs->h_u.tcp_ip4_spec.ip4src),
> -					htonl(fs->h_u.tcp_ip4_spec.ip4dst),
> +					ntohl(fs->h_u.tcp_ip4_spec.ip4src),
> +					ntohl(fs->h_u.tcp_ip4_spec.ip4dst),
>  					be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc),
>  					be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
>  
> -- 
> 2.40.1
> 
> 

Re: [PATCH 3/3] net: macb: fix __be32 warnings in debug code

[Ben Dooks]

On 2023-06-22 16:44, Simon Horman wrote:
> On Thu, Jun 22, 2023 at 02:05:07PM +0100, Ben Dooks wrote:
>> The netdev_dbg() calls in gem_add_flow_filter() and 
>> gem_del_flow_filter()
>> call ntohl() on the ipv4 addresses, which will put them into the host 
>> order
>> but not the right type (returns a __be32, not an u32 as would be 
>> expected).
>> 
>> Chaning the htonl() to nthol() should still do the right conversion, 
>> return
>> the correct u32 type and  should not change any functional to remove 
>> the
>> following sparse warnings:
>> 
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect 
>> type in argument 1 (different base types)
>> drivers/net/ethernet/cadence/macb_main.c:3568:9:    expected unsigned 
>> int [usertype] val
>> drivers/net/ethernet/cadence/macb_main.c:3568:9:    got restricted 
>> __be32 [usertype] ip4src
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect 
>> type in argument 1 (different base types)
>> drivers/net/ethernet/cadence/macb_main.c:3568:9:    expected unsigned 
>> int [usertype] val
>> drivers/net/ethernet/cadence/macb_main.c:3568:9:    got restricted 
>> __be32 [usertype] ip4dst
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from 
>> restricted __be32
>> d
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect 
>> type in argument 1 (different base types)
>> drivers/net/ethernet/cadence/macb_main.c:3622:25:    expected unsigned 
>> int [usertype] val
>> drivers/net/ethernet/cadence/macb_main.c:3622:25:    got restricted 
>> __be32 [usertype] ip4src
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect 
>> type in argument 1 (different base types)
>> drivers/net/ethernet/cadence/macb_main.c:3622:25:    expected unsigned 
>> int [usertype] val
>> drivers/net/ethernet/cadence/macb_main.c:3622:25:    got restricted 
>> __be32 [usertype] ip4dst
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from 
>> restricted __be32
>> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from 
>> restricted __be32
>> 
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> 
> Hi Ben,
> 
> this code-change looks good to me, but I have a few minor nits for your
> consideration.
> 
> 1. Please specify the target tree, in this case net-next, for patch 
> sets
>    for Networking code.
> 
> 	Subject: [PATCH net-next ...] ...

Ah, was using net, but I assume net-next is probably ok

> 2. It might be nicer to write '.../macb_main.c' or similar,
>    rather tha nthe full path, in the patch description.
> 
> 3. checkpatch --codespell says: 'Chaning' -> 'Chaining'

Ok, thank you. I didn't know about that.

Since there's another patch that needs work I'll re-send this early next 
week
with the fixes in.

>> ---
>>  drivers/net/ethernet/cadence/macb_main.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 56e202b74bd7..59a90c2b307f 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -3568,8 +3568,8 @@ static int gem_add_flow_filter(struct net_device 
>> *netdev,
>>  	netdev_dbg(netdev,
>>  			"Adding flow filter 
>> entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n",
>>  			fs->flow_type, (int)fs->ring_cookie, fs->location,
>> -			htonl(fs->h_u.tcp_ip4_spec.ip4src),
>> -			htonl(fs->h_u.tcp_ip4_spec.ip4dst),
>> +			ntohl(fs->h_u.tcp_ip4_spec.ip4src),
>> +			ntohl(fs->h_u.tcp_ip4_spec.ip4dst),
>>  			be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc),
>>  			be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
>> 
>> @@ -3622,8 +3622,8 @@ static int gem_del_flow_filter(struct net_device 
>> *netdev,
>>  			netdev_dbg(netdev,
>>  					"Deleting flow filter 
>> entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n",
>>  					fs->flow_type, (int)fs->ring_cookie, fs->location,
>> -					htonl(fs->h_u.tcp_ip4_spec.ip4src),
>> -					htonl(fs->h_u.tcp_ip4_spec.ip4dst),
>> +					ntohl(fs->h_u.tcp_ip4_spec.ip4src),
>> +					ntohl(fs->h_u.tcp_ip4_spec.ip4dst),
>>  					be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc),
>>  					be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
>> 
>> --
>> 2.40.1
>> 
>> 

Re: [PATCH 3/3] net: macb: fix __be32 warnings in debug code

[Simon Horman]

On Fri, Jun 23, 2023 at 10:43:12AM +0100, Ben Dooks wrote:
> 
> 
> On 2023-06-22 16:44, Simon Horman wrote:
> > On Thu, Jun 22, 2023 at 02:05:07PM +0100, Ben Dooks wrote:

...

> > Hi Ben,
> > 
> > this code-change looks good to me, but I have a few minor nits for your
> > consideration.
> > 
> > 1. Please specify the target tree, in this case net-next, for patch sets
> >    for Networking code.
> > 
> > 	Subject: [PATCH net-next ...] ...
> 
> Ah, was using net, but I assume net-next is probably ok

I think net-next is best for this kind of change.
FWIIW, this series did apply there.

> > 2. It might be nicer to write '.../macb_main.c' or similar,
> >    rather tha nthe full path, in the patch description.
> > 
> > 3. checkpatch --codespell says: 'Chaning' -> 'Chaining'
> 
> Ok, thank you. I didn't know about that.

It is quite handy :)

> Since there's another patch that needs work I'll re-send this early next
> week
> with the fixes in.

I think a repost of the series is a good plan.

Please note that when v6.4 is released, which may well be over the weekend,
then net-next will be closed until after rc1 is released - approximately
two weeks. If it is closed then you'll need to wait until it reopens
before posting v2.

Re: net: macb: sparse warning fixes

[Nicolas Ferre]

Hi Ben,

On 22/06/2023 at 15:05, Ben Dooks wrote:
> These are 3 hopefully easy patches for fixing sparse errors due to
> endian-ness warnings. There are still some left, but there are not
> as easy as they mix host and network fields together.
> 
> For example, gem_prog_cmp_regs() has two u32 variables that it does
> bitfield manipulation on for the tcp ports and these are __be16 into
> u32, so not sure how these are meant to be changed. I've also no hardware
> to test on, so even if these did get changed then I can't check if it is
> working pre/post change.

Do you know if there could be any impact on performance (even if limited)?

Best regards,
   Nicolas

> Also gem_writel and gem_writel_n, it is not clear if both of these are
> meant to be host order or not.
> 
> Ben Dooks (3):
>    net: macb: check constant to define and fix __be32 warnings
>    net: macb: add port constant to fix __be16 warnings
>    net: macb: fix __be32 warnings in debug code
> 
>   drivers/net/ethernet/cadence/macb_main.c | 25 +++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)

-- 
Nicolas Ferre

Re: net: macb: sparse warning fixes

[Andrew Lunn]

On Fri, Jun 23, 2023 at 03:16:23PM +0200, Nicolas Ferre wrote:
> Hi Ben,
> 
> On 22/06/2023 at 15:05, Ben Dooks wrote:
> > These are 3 hopefully easy patches for fixing sparse errors due to
> > endian-ness warnings. There are still some left, but there are not
> > as easy as they mix host and network fields together.
> > 
> > For example, gem_prog_cmp_regs() has two u32 variables that it does
> > bitfield manipulation on for the tcp ports and these are __be16 into
> > u32, so not sure how these are meant to be changed. I've also no hardware
> > to test on, so even if these did get changed then I can't check if it is
> > working pre/post change.
> 
> Do you know if there could be any impact on performance (even if limited)?

Hi Nicolas

This is inside a netdev_dbg(). So 99% of the time it is compiled
out. The other 1% of the time, your 115200 baud serial port is
probably the bottleneck, not an endianness swap.

	 Andrew

Re: net: macb: sparse warning fixes

[Nicolas Ferre]

On 23/06/2023 at 17:38, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jun 23, 2023 at 03:16:23PM +0200, Nicolas Ferre wrote:
>> Hi Ben,
>>
>> On 22/06/2023 at 15:05, Ben Dooks wrote:
>>> These are 3 hopefully easy patches for fixing sparse errors due to
>>> endian-ness warnings. There are still some left, but there are not
>>> as easy as they mix host and network fields together.
>>>
>>> For example, gem_prog_cmp_regs() has two u32 variables that it does
>>> bitfield manipulation on for the tcp ports and these are __be16 into
>>> u32, so not sure how these are meant to be changed. I've also no hardware
>>> to test on, so even if these did get changed then I can't check if it is
>>> working pre/post change.
>>
>> Do you know if there could be any impact on performance (even if limited)?
> 
> Hi Nicolas
> 
> This is inside a netdev_dbg(). So 99% of the time it is compiled
> out. The other 1% of the time, your 115200 baud serial port is
> probably the bottleneck, not an endianness swap.

Yeah, sure thing: I was not talking about the 3/3 patch where, yes, 
indeed it doesn't have an importance.

Others in gem_enable_flow_filters() and gem_prog_cmp_regs() look like 
they are away from hot path anyway, so here again probably no impact.

Regards,
   Nicolas

-- 
Nicolas Ferre

Re: net: macb: sparse warning fixes

[Ben Dooks]

On 23/06/2023 14:16, Nicolas Ferre wrote:
> Hi Ben,
> 
> On 22/06/2023 at 15:05, Ben Dooks wrote:
>> These are 3 hopefully easy patches for fixing sparse errors due to
>> endian-ness warnings. There are still some left, but there are not
>> as easy as they mix host and network fields together.
>>
>> For example, gem_prog_cmp_regs() has two u32 variables that it does
>> bitfield manipulation on for the tcp ports and these are __be16 into
>> u32, so not sure how these are meant to be changed. I've also no hardware
>> to test on, so even if these did get changed then I can't check if it is
>> working pre/post change.
> 
> Do you know if there could be any impact on performance (even if limited)?

There shouldn't be, these are either constants so should be compile time
sorted or they are just using the swap code the wrong way round... same
values, just the wrong endian markers going in/out.

The only device with a macb I've got is an unmatched, so don't even know
if I can test any of this.

The filter code I would like to get some feedback on, as I didn't want
to do any modifications without being able to test.

> Best regards,
>    Nicolas
> 
>> Also gem_writel and gem_writel_n, it is not clear if both of these are
>> meant to be host order or not.
>>
>> Ben Dooks (3):
>>    net: macb: check constant to define and fix __be32 warnings
>>    net: macb: add port constant to fix __be16 warnings
>>    net: macb: fix __be32 warnings in debug code
>>
>>   drivers/net/ethernet/cadence/macb_main.c | 25 +++++++++++++-----------
>>   1 file changed, 14 insertions(+), 11 deletions(-)
> 

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html