Re: [PATCH net-next v7 3/3] w5100: detect carrier state using link status bit and optional interrupt

[Arthur Crepin Leblond]

On Thu, Sep 10, 2026 at 01:27:42PM +0200, Paolo Abeni wrote:
> On 9/9/26 2:39 PM, netdev-bot+sashiko@xxxxxxxxxx wrote:

Hi Paolo,

> > Thank you for your contribution! Sashiko AI review found 4 potential
> > issue(s) to consider:
> >
> > Critical: 0 · High: 1 · Medium: 2 · Low: 1
> >
> > - [High] On w5500 the carrier state is now sampled exactly once per ifup
> >   (and once per resume) from PHYCFGR, with no polling fallback and no…
>
> I read this one as 'with a bad config/devtree the eth could non functional'
> which does look a big deal to me...

Yeah I would also say so, wtihout an additional interrupt configured
you cannot really get notified of the carrier state changes. Only
.get_link is useful (and only for w5500).

But where Sashiko might be right is that the previous implementation
was not calling netif_carrier_on/off on w5100_open and did not have
.get_link set. So maybe .get_link should only be set on w5500 and
netif_carrier_on/off called only if the link irq is set.

> > - [Medium] `w5500_get_phycfgr_lnk()` returns a tri-state value (<0 on
> >   SPI/register read error, 0 link down, 1 link up), and its consumers…
> > - [Medium] The carrier state is computed and published by two
> >   unsynchronized contexts.
>
> ... but the above 2 looks relevant to me. Note that ordering the irq
> enabling and initial link detection is not enough, you need explicit
> synchronization.

I'll try to use rtnl_lock.

> Finally please note commit c82ff94592fb68f529afe63ca7f5ddb7dae4ba83, you
> should explicitly handle sashiko's comments.
>
> /P

Thank you for pointing this out, I'll address the AI reviews.

Arthur