* Re: [PATCH] net: tulip: winbond-840: remove never-true 486 CPU check
2026-09-15 2:45 [PATCH] net: tulip: winbond-840: remove never-true 486 CPU check Ethan Nelson-Moore
@ 2026-09-15 3:36 ` Ethan Nelson-Moore
2026-09-16 20:48 ` netdev-bot+sashiko
2026-09-18 15:59 ` Loktionov, Aleksandr
2 siblings, 0 replies; 4+ messages in thread
From: Ethan Nelson-Moore @ 2026-09-15 3:36 UTC (permalink / raw)
To: Arend van Spriel, Aleksandr Loktionov, Jijie Shao, Petr Machata,
Jacob Keller, Uwe Kleine-König (The Capable Hub), netdev,
linux-parisc
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Michael Grzeschik
On Mon, Sep 14, 2026 at 7:46 PM Ethan Nelson-Moore
<enelsonmoore@gmail.com> wrote:
> The winbond-840 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.
Oops, never true*. Should I send a v2, or will whoever merges this fix it?
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] net: tulip: winbond-840: remove never-true 486 CPU check
2026-09-15 2:45 [PATCH] net: tulip: winbond-840: remove never-true 486 CPU check Ethan Nelson-Moore
2026-09-15 3:36 ` Ethan Nelson-Moore
@ 2026-09-16 20:48 ` netdev-bot+sashiko
2026-09-18 15:59 ` Loktionov, Aleksandr
2 siblings, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-16 20:48 UTC (permalink / raw)
To: enelsonmoore
Cc: arend.vanspriel, aleksandr.loktionov, shaojijie, petrm,
jacob.e.keller, u.kleine-koenig, netdev, linux-parisc,
andrew+netdev, davem, edumazet, kuba, pabeni, mgr
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.
- [Low] Stale `#include <asm/processor.h>` with a now-false self-
documenting comment.
- [Low] Comment/code mismatch introduced by the comment rewrite in
init_registers().
--
net: tulip: winbond-840: remove never-true 486 CPU check
This removes the runtime boot_cpu_data.x86 <= 4 test in init_registers()
of the winbond-840 driver and collapses the surrounding preprocessor
chain so that the i386 non-module case now falls into the generic
architecture branch. The comment describing the two cache alignment
cases is reworded at the same time.
> net: tulip: winbond-840: remove never-true 486 CPU check
[Severity: Medium]
The subject and the message body say the condition is always true:
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.
Is the polarity backwards here? boot_cpu_data.x86 holds the CPU family
(3 = 386, 4 = 486, 5 = 586), and after 8b793a92d862 the minimum family
for 32-bit builds is 5:
arch/x86/Kconfig.cpu:
config X86_MINIMUM_CPU_FAMILY
int
default "64" if X86_64
default "6" if X86_32 && (MPENTIUM4 || ...)
default "5"
So a family <= 4 CPU can no longer boot the kernel and the predicate is
never true, which is consistent with the patch keeping i |= 0xE000 (the
else branch) rather than i |= 0x4800.
Should the wording be something like "the condition is never true, so
the else branch is always taken"? As written, a reader or a stable
backporter would expect 0x4800 to be the value preserved for i386
built-in builds.
> diff --git a/drivers/net/ethernet/dec/tulip/winbond-840.c b/drivers/net/ethernet/dec/tulip/winbond-840.c
> index a5581f1d01417..c42eaf0eef642 100644
> --- a/drivers/net/ethernet/dec/tulip/winbond-840.c
> +++ b/drivers/net/ethernet/dec/tulip/winbond-840.c
> @@ -876,24 +876,14 @@ static void init_registers(struct net_device *dev)
> i |= 0x02; /* give Rx priority */
>
> /* Configure the PCI bus bursts and FIFO thresholds.
> - 486: Set 8 longword cache alignment, 8 longword burst.
> - 586: Set 16 longword cache alignment, no burst limit.
> + Set 16 longword cache alignment, no burst limit.
[Severity: Low]
Does this single unconditional line still match the code below it? Two
different values are still programmed, and neither one is "16 longword
cache alignment, no burst limit" (which the table just below decodes as
0x8000):
#if defined(__powerpc__) || defined(__i386__) || ...
i |= 0xE000; /* C000 = 32 longwords | 2000 = 32 longword burst */
#elif defined(CONFIG_SPARC) || defined (CONFIG_PARISC) || defined(CONFIG_ARM)
i |= 0x4800; /* 4000 = 8 longwords | 0800 = 8 longword burst */
#else
i |= 0x4800;
The deleted "486:" line is the one that described 0x4800 accurately, and
that value is still programmed on SPARC, PARISC, ARM and the unknown-arch
fallback. Would it be better to drop the prose entirely, or restate it
for both remaining cases?
The 0xE000 half of the mismatch predates this patch; the new part is that
the wording is now unconditional and also covers the 0x4800 branches.
> Cache alignment bits 15:14 Burst length 13:8
> 0000 <not allowed> 0000 align to cache 0800 8 longwords
> 4000 8 longwords 0100 1 longword 1000 16 longwords
> 8000 16 longwords 0200 2 longwords 2000 32 longwords
> C000 32 longwords 0400 4 longwords */
>
> -#if defined (__i386__) && !defined(MODULE) && !defined(CONFIG_UML)
> - /* When not a module we can work around broken '486 PCI boards. */
> - if (boot_cpu_data.x86 <= 4) {
> - i |= 0x4800;
> - dev_info(&dev->dev,
> - "This is a 386/486 PCI system, setting cache alignment to 8 longwords\n");
> - } else {
> - i |= 0xE000;
> - }
> -#elif defined(__powerpc__) || defined(__i386__) || defined(__alpha__) || defined(__ia64__) || defined(__x86_64__)
> +#if defined(__powerpc__) || defined(__i386__) || defined(__alpha__) || defined(__ia64__) || defined(__x86_64__)
> i |= 0xE000;
[Severity: Low]
With boot_cpu_data gone, is the header include at the top of
drivers/net/ethernet/dec/tulip/winbond-840.c now stale?
#include <asm/processor.h> /* Processor type for cache alignment. */
That was the only user of asm/processor.h in this file, and after the
patch the cache alignment selection in init_registers() is purely
compile-time arch conditionals, so the comment no longer describes
anything the code does. Should the include (or at least the comment) be
dropped in the same change?
> #elif defined(CONFIG_SPARC) || defined (CONFIG_PARISC) || defined(CONFIG_ARM)
> i |= 0x4800;
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915024551.196126-1-enelsonmoore%40gmail.com
^ permalink raw reply [flat|nested] 4+ messages in thread* RE: [PATCH] net: tulip: winbond-840: remove never-true 486 CPU check
2026-09-15 2:45 [PATCH] net: tulip: winbond-840: remove never-true 486 CPU check Ethan Nelson-Moore
2026-09-15 3:36 ` Ethan Nelson-Moore
2026-09-16 20:48 ` netdev-bot+sashiko
@ 2026-09-18 15:59 ` Loktionov, Aleksandr
2 siblings, 0 replies; 4+ messages in thread
From: Loktionov, Aleksandr @ 2026-09-18 15:59 UTC (permalink / raw)
To: Ethan Nelson-Moore, Arend van Spriel, Jijie Shao, Petr Machata,
Keller, Jacob E, Uwe Kleine-König (The Capable Hub),
netdev@vger.kernel.org, linux-parisc@vger.kernel.org
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Michael Grzeschik
> -----Original Message-----
> From: Ethan Nelson-Moore <enelsonmoore@gmail.com>
> Sent: Tuesday, September 15, 2026 4:46 AM
> To: Arend van Spriel <arend.vanspriel@broadcom.com>; Loktionov,
> Aleksandr <aleksandr.loktionov@intel.com>; Jijie Shao
> <shaojijie@huawei.com>; Petr Machata <petrm@nvidia.com>; Keller, Jacob
> E <jacob.e.keller@intel.com>; Uwe Kleine-König (The Capable Hub)
> <u.kleine-koenig@baylibre.com>; netdev@vger.kernel.org; linux-
> parisc@vger.kernel.org
> Cc: Ethan Nelson-Moore <enelsonmoore@gmail.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Michael Grzeschik <mgr@kernel.org>
> Subject: [PATCH] net: tulip: winbond-840: remove never-true 486 CPU
> check
>
> The winbond-840 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.
It looks like condition is always false, isn't it?
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>
> Signed-off-by: Ethan Nelson-Moore <enelsonmoore@gmail.com>
> ---
> drivers/net/ethernet/dec/tulip/winbond-840.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/dec/tulip/winbond-840.c
> b/drivers/net/ethernet/dec/tulip/winbond-840.c
> index a5581f1d0141..c42eaf0eef64 100644
> --- a/drivers/net/ethernet/dec/tulip/winbond-840.c
> +++ b/drivers/net/ethernet/dec/tulip/winbond-840.c
> @@ -876,24 +876,14 @@ static void init_registers(struct net_device
> *dev)
> i |= 0x02; /* give Rx priority */
>
> /* Configure the PCI bus bursts and FIFO thresholds.
> - 486: Set 8 longword cache alignment, 8 longword burst.
> - 586: Set 16 longword cache alignment, no burst limit.
> + Set 16 longword cache alignment, no burst limit.
> Cache alignment bits 15:14 Burst length 13:8
> 0000 <not allowed> 0000 align to cache
> 0800 8 longwords
> 4000 8 longwords 0100 1 longword
> 1000 16 longwords
> 8000 16 longwords 0200 2 longwords 2000 32
> longwords
> C000 32 longwords 0400 4 longwords */
>
> -#if defined (__i386__) && !defined(MODULE) && !defined(CONFIG_UML)
> - /* When not a module we can work around broken '486 PCI boards.
> */
> - if (boot_cpu_data.x86 <= 4) {
> - i |= 0x4800;
> - dev_info(&dev->dev,
> - "This is a 386/486 PCI system, setting cache
> alignment to 8 longwords\n");
> - } else {
> - i |= 0xE000;
> - }
> -#elif defined(__powerpc__) || defined(__i386__) || defined(__alpha__)
> || defined(__ia64__) || defined(__x86_64__)
> +#if defined(__powerpc__) || defined(__i386__) || defined(__alpha__)
> ||
> +defined(__ia64__) || defined(__x86_64__)
> i |= 0xE000;
> #elif defined(CONFIG_SPARC) || defined (CONFIG_PARISC) ||
> defined(CONFIG_ARM)
> i |= 0x4800;
> --
> 2.43.0
^ permalink raw reply [flat|nested] 4+ messages in thread