Re: [PATCH] net: fealnx: remove never-true 486 CPU check

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 2

- [Medium] Inverted rationale in the commit message of "net: fealnx:
  remove never-true 486 CPU check": the log states that after 486…
- [Low] The block comment above the burst/FIFO register setup in
  drivers/net/ethernet/fealnx.c:netdev_open() was rewritten from the two…
- [Low] The patch removes the last user of asm/processor.h in
  drivers/net/ethernet/fealnx.c (`boot_cpu_data.x86`) but leaves the…

--

net: fealnx: remove never-true 486 CPU check

The patch drops the boot_cpu_data.x86 <= 4 test in netdev_open() and keeps
the value that the else arm used to assign, np->crvalue = 0xe00.  The block
comment above the burst setup is reworded at the same time.

A few questions below.

> diff --git a/drivers/net/ethernet/fealnx.c b/drivers/net/ethernet/fealnx.c
> index bdc38aac58509..708b61f219156 100644
> --- a/drivers/net/ethernet/fealnx.c
> +++ b/drivers/net/ethernet/fealnx.c

>     The fealnx driver contains a check for boot_cpu_data.x86 <= 4.
>     This field encodes the x86 generation (3 = 386, 4 = 486, etc).
>     Therefore, since 486 support was removed from the kernel in commit
>     8b793a92d862 ("x86/cpu: Remove M486/M486SX/ELAN support"), the
>     condition is always true. Remove the check.

[Severity: Medium]
Should this read "always false" rather than "always true"?

boot_cpu_data.x86 is the CPU family (4 == 486, 5 == Pentium), so once 386/486
support is gone the minimum family is 5 and:

	if (boot_cpu_data.x86 <= 4)

can never hold.  The subject line already says "never-true", so the body looks
inverted with respect to the subject.

The code change matches the "always false" reading, since it deletes the
np->crvalue = 0xa00 arm and keeps the else value:

	np->crvalue = 0xe00;	/* rx 128 burst length */

Taken literally, the "always true" wording would call for the opposite edit
(keep 0xa00, rx burst field 0b101 instead of 0b111).  Could the changelog be
reworded so that anyone revisiting or reverting this hunk later does not
program the other burst value?  It may also be worth mentioning that the hunk
was already compiled out for MODULE, non-i386 and UML builds.

> @@ -839,8 +839,7 @@ static int netdev_open(struct net_device *dev)
>  
>  	/* Initialize other registers. */
>  	/* Configure the PCI bus bursts and FIFO thresholds.
> -	   486: Set 8 longword burst.
> -	   586: no burst limit.
> +	   No burst limit.
        ^^^^^^^^^^^^^^^

[Severity: Low]
Does this new line agree with the code below it?  A few lines further down
netdev_open() still programs a finite 8-longword bus burst into BCR:

	np->bcrvalue = 0x10;	/* little-endian, 8 burst length */
	...
	iowrite32(np->bcrvalue, ioaddr + BCR);

Per the encoding table kept in the same comment ("Burst length 5:3", 0 1 0 ->
8), 0x10 sets bits 5:3 = 0b010, i.e. an 8-longword limit.

Since the removal of the CPU check makes this unconditional, would it be
clearer to drop the line along with the conditional, or reword it to describe
what is actually latched (BCR: 8-longword bus burst, TCRRCR rx burst field at
maximum)?

Also, the retained inline comment on np->crvalue = 0xe00 says "rx 128 burst
length" while the table above maps 1 1 1 to 256.  That inaccuracy predates the
patch, but it is now the only case the header comment describes.

The comment rework itself is not mentioned in the changelog, which only
mentions removing the CPU check.

>  	   Burst length 5:3
>  	   0 0 0   1
>  	   0 0 1   4

[ ... ]

> @@ -858,13 +857,7 @@ static int netdev_open(struct net_device *dev)
>  #ifdef __BIG_ENDIAN
>  	np->bcrvalue |= 0x04;	/* big-endian */
>  #endif
> -
> -#if defined(__i386__) && !defined(MODULE) && !defined(CONFIG_UML)
> -	if (boot_cpu_data.x86 <= 4)
> -		np->crvalue = 0xa00;
> -	else
> -#endif
> -		np->crvalue = 0xe00;	/* rx 128 burst length */
> +	np->crvalue = 0xe00;	/* rx 128 burst length */

[Severity: Low]
This removes the last user of asm/processor.h in the file, but the include and
its justification comment near the top of drivers/net/ethernet/fealnx.c are
left behind:

	#include <asm/processor.h>	/* Processor type for cache alignment. */

After this patch, git grep boot_cpu_data on the file returns nothing, and a
grep for other plausible users (cpu_relax, cpu_data, cache_line*) also finds
nothing.  Should the include be dropped together with its only user?

>  
>  
>  // 89/12/29 add,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915024638.198865-1-enelsonmoore%40gmail.com