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

[Arthur Crepin Leblond <arthur@marmottus.net>]

On Thu, Sep 10, 2026 at 01:27:42PM +0200, Paolo Abeni wrote:
>On 9/9/26 2:39 PM, netdev-bot+sashiko@kernel.org 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