[PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[Min-Hua Chen]

Use cpu_to_le32 to convert the constants to __le32 type
before comparing them with p->des0 and p->des1 (they are __le32 type)
and to fix following sparse warnings:

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:110:23: sparse: warning: restricted __le32 degrades to integer
drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:110:50: sparse: warning: restricted __le32 degrades to integer

Reviewed-by: Simon Horman <simon.horman@corigine.com>
Signed-off-by: Min-Hua Chen <minhuadotchen@gmail.com>
---

Change since v1:
use cpu_to_le32 to the constants

Change since v2:
remove unnecessary parentheses

---
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
index 13c347ee8be9..ffe4a41ffcde 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c
@@ -107,7 +107,8 @@ static int dwxgmac2_rx_check_timestamp(void *desc)
 	ts_valid = !(rdes3 & XGMAC_RDES3_TSD) && (rdes3 & XGMAC_RDES3_TSA);
 
 	if (likely(desc_valid && ts_valid)) {
-		if ((p->des0 == 0xffffffff) && (p->des1 == 0xffffffff))
+		if (p->des0 == cpu_to_le32(0xffffffff) &&
+		    p->des1 == cpu_to_le32(0xffffffff))
 			return -EINVAL;
 		return 0;
 	}
-- 
2.34.1

Re: [PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[Simon Horman]

On Fri, May 19, 2023 at 07:50:28PM +0800, Min-Hua Chen wrote:
> [You don't often get email from minhuadotchen@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Use cpu_to_le32 to convert the constants to __le32 type
> before comparing them with p->des0 and p->des1 (they are __le32 type)
> and to fix following sparse warnings:
> 
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:110:23: sparse: warning: restricted __le32 degrades to integer
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:110:50: sparse: warning: restricted __le32 degrades to integer
> 
> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> Signed-off-by: Min-Hua Chen <minhuadotchen@gmail.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[Jakub Kicinski]

On Fri, 19 May 2023 19:50:28 +0800 Min-Hua Chen wrote:
> -		if ((p->des0 == 0xffffffff) && (p->des1 == 0xffffffff))
> +		if (p->des0 == cpu_to_le32(0xffffffff) &&
> +		    p->des1 == cpu_to_le32(0xffffffff))

Can you try to fix the sparse tool instead? I believe it already
ignores such errors for the constant of 0, maybe it can be taught 
to ignore all "isomorphic" values?

By "isomorphic" I mean that 0xffffffff == cpu_to_le32(0xffffffff)
so there's no point complaining.
-- 
pw-bot: reject

Re: [PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[Min-Hua Chen]

Hi Jakub,

>On Fri, 19 May 2023 19:50:28 +0800 Min-Hua Chen wrote:
>> -		if ((p->des0 == 0xffffffff) && (p->des1 == 0xffffffff))
>> +		if (p->des0 == cpu_to_le32(0xffffffff) &&
>> +		    p->des1 == cpu_to_le32(0xffffffff))
>
>Can you try to fix the sparse tool instead? I believe it already
>ignores such errors for the constant of 0, maybe it can be taught 
>to ignore all "isomorphic" values?
>

I downloaded the source code of sparse and I'm afraid that I cannot make
0xFFFFFFFF ignored easily. I've tried ~0 instead of 0xFFFFFF,
but it did not work with current sparse.

0 is a special case mentioned in [1].

"""
One small note: the constant integer “0” is special. 
You can use a constant zero as a bitwise integer type without
sparse ever complaining. This is because “bitwise” (as the name
implies) was designed for making sure that bitwise types don’t
get mixed up (little-endian vs big-endian vs cpu-endian vs whatever),
and there the constant “0” really _is_ special.
"""

For 0xFFFFFFFF, it may look like a false alarm, but we can silence the
sparse warning by taking a fix like mine and people can keep working on
other sparse warnings easier.
(There are around 7000 sparse warning in ARCH=arm64 defconfig build and
sometimes it is hard to remember all the false alarm cases)

Could you consider taking this patch, please?

>
>By "isomorphic" I mean that 0xffffffff == cpu_to_le32(0xffffffff)
>so there's no point complaining.

thanks,
Min-Hua

[1] https://www.kernel.org/doc/html/v4.12/dev-tools/sparse.html

Re: [PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[Jakub Kicinski]

On Sat, 20 May 2023 09:55:27 +0800 Min-Hua Chen wrote:
> >Can you try to fix the sparse tool instead? I believe it already
> >ignores such errors for the constant of 0, maybe it can be taught 
> >to ignore all "isomorphic" values?
> >  
> 
> I downloaded the source code of sparse and I'm afraid that I cannot make
> 0xFFFFFFFF ignored easily. I've tried ~0 instead of 0xFFFFFF,
> but it did not work with current sparse.
> 
> 0 is a special case mentioned in [1].
> 
> """
> One small note: the constant integer “0” is special. 
> You can use a constant zero as a bitwise integer type without
> sparse ever complaining. This is because “bitwise” (as the name
> implies) was designed for making sure that bitwise types don’t
> get mixed up (little-endian vs big-endian vs cpu-endian vs whatever),
> and there the constant “0” really _is_ special.
> """
> 
> For 0xFFFFFFFF, it may look like a false alarm, but we can silence the
> sparse warning by taking a fix like mine and people can keep working on
> other sparse warnings easier.

We can make working with sparse easier by making sure it doesn't
generate false positive warnings :\

> (There are around 7000 sparse warning in ARCH=arm64 defconfig build and
> sometimes it is hard to remember all the false alarm cases)
> 
> Could you consider taking this patch, please?

No. We don't take patches to address false positive static 
checker warnings.

Re: [PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[Min-Hua Chen]

Hi Jakub,

>We can make working with sparse easier by making sure it doesn't
>generate false positive warnings :\

It will be good if sparse can handle this case correctly.

>
>> (There are around 7000 sparse warning in ARCH=arm64 defconfig build and
>> sometimes it is hard to remember all the false alarm cases)
>>
>> Could you consider taking this patch, please?
>
>No. We don't take patches to address false positive static
>checker warnings.

No problem.

thanks,
Min-Hua

Re: [PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[Edward Cree]

On 20/05/2023 02:55, Min-Hua Chen wrote:
>> On Fri, 19 May 2023 19:50:28 +0800 Min-Hua Chen wrote:
>>> -		if ((p->des0 == 0xffffffff) && (p->des1 == 0xffffffff))
>>> +		if (p->des0 == cpu_to_le32(0xffffffff) &&
>>> +		    p->des1 == cpu_to_le32(0xffffffff))
>>
>> Can you try to fix the sparse tool instead? I believe it already
>> ignores such errors for the constant of 0, maybe it can be taught 
>> to ignore all "isomorphic" values?
>>
> 
> I downloaded the source code of sparse and I'm afraid that I cannot make
> 0xFFFFFFFF ignored easily. I've tried ~0 instead of 0xFFFFFF,
> but it did not work with current sparse.
> 
> 0 is a special case mentioned in [1].

I believe you can do something like
    if ((p->des0 == ~(__le32)0) && (p->des1 == ~(__le32)0))
 and sparse will accept that, because the cast is allowed under the
 special case.
HTH,
-ed

Re: [PATCH v3] net: stmmac: compare p->des0 and p->des1 with __le32 type values

[Min-Hua Chen]

hi Edward,

>>> On Fri, 19 May 2023 19:50:28 +0800 Min-Hua Chen wrote:
>>>> -		if ((p->des0 == 0xffffffff) && (p->des1 == 0xffffffff))
>>>> +		if (p->des0 == cpu_to_le32(0xffffffff) &&
>>>> +		    p->des1 == cpu_to_le32(0xffffffff))
>>>
>>> Can you try to fix the sparse tool instead? I believe it already
>>> ignores such errors for the constant of 0, maybe it can be taught 
>>> to ignore all "isomorphic" values?
>>>
>> 
>> I downloaded the source code of sparse and I'm afraid that I cannot make
>> 0xFFFFFFFF ignored easily. I've tried ~0 instead of 0xFFFFFF,
>> but it did not work with current sparse.
>> 
>> 0 is a special case mentioned in [1].
>
>I believe you can do something like
>    if ((p->des0 == ~(__le32)0) && (p->des1 == ~(__le32)0))
> and sparse will accept that, because the cast is allowed under the
> special case.
>HTH,
>-ed

I tested ~(__le32)0 and it worked: sparse accpets this.
Thanks for sharing this.

cheers,
Min-Hua