netdev - Re: [PATCH v3 net-next 6/7] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 16 Oct 2021 10:39:34 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Jakub Kicinski <kuba@...nel.org>,
        Alvin Šipraga <alvin@...s.dk>
CC:     Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Michael Rasmussen <MIR@...g-olufsen.dk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 net-next 6/7] net: dsa: realtek-smi: add rtl8365mb
 subdriver for RTL8365MB-VC

On 10/16/21 2:40 AM, Jakub Kicinski wrote:
> On Fri, 15 Oct 2021 19:10:27 +0200 Alvin Šipraga wrote:
>> v2 -> v3:
>>    - collect Florian's Reviewed-by
>>    - move IRQ setup earlier in probe per Florian's suggestion
>>    - follow Jakub's suggestion and use the standard ethtool stats API
> 
> Thanks a lot for doing this. The code LGTM, the only thing that stands
> out is the use of spin_locks(). I couldn't quickly parse out what bus
> this device hangs off, if it's MMIO and registers can be read without
> sleeping you could potentially get rid of the delayed work to read
> stats, but I think you need to switch to
> regmap_read_poll_timeout_atomic() because regmap_read_poll_timeout()
> itself can sleep.

It's an "SMI" bus bit-banged with GPIO - details are in 
realtek-smi-core.c - a Realtek peculiarity. Initially I thought this was 
implementation was sleeping, but actually it's using ndelay() which is 
OK in atomic context if I'm not mistaken? So I guess you're right, I can 
get rid of the delayed work. :)

> 
> If the register access sleeps (I2C, SPI, MDIO etc) you need to switch
> from a spin lock to a mutex.
> 
> Either way CONFIG_DEBUG_ATOMIC_SLEEP is your friend.

Thanks, I'll test with this and follow up. I see a kernel test robot 
warning so I will be sending v4 anyway.