netdev - Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

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]

Message-ID: <75428c5faab7fc656051ab227663e6e6@walle.cc>
Date:   Mon, 20 Apr 2020 17:10:19 +0200
From:   Michael Walle <michael@...le.cc>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next 3/3] net: phy: bcm54140: add hwmon support

Hi Andrew,

Am 2020-04-19 23:55, schrieb Andrew Lunn:
>> But what does that have to do with the shared structure? I don't think
>> you have to "bundle" the shared structure with the "access the global
>> registers" method.
> 
> We don't need to. But it would be a good way to clean up code which
> locks the mdio bus, does a register access on some other device, and
> then unlocks the bus.

I'd like do an RFC for that. But how should I proceed with the original
patch series? Should I send an updated version; you didn't reply to the
LED stuff. That is the last remark for now.

> As a general rule of thumb, it is better to have the core do the
> locking, rather than the driver. Driver writers don't always think
> about locking, so it is better to give driver writers safe APIs to
> use.

Ok I see, but what locking do you have in mind? We could have something
like

__phy_package_write(struct phy_device *dev, u32 regnum, u16 val)
{
   return __mdiobus_write(phydev->mdio.bus, phydev->shared->addr,
                          regnum, val);
}

and its phy_package_write() equivalent. But that would just be
convenience functions, nothing where you actually help the user with
locking. Am I missing something?

>>> Get the core to do reference counting on the structure?
>>> Add helpers phy_read_shared(), phy_write_shared(), etc, which does
>>> MDIO accesses on the base device, taking care of the locking.
>>> 
>> The "base" access is another thing, I guess, which has nothing to do
>> with the shared structure.
>> 
> I'm making the assumption that all global addresses are at the base
> address. If we don't want to make that assumption, we need the change
> the API above so you pass a cookie, and all PHYs need to use the same
> cookie to identify the package.

how would a phy driver deduce a common cookie? And how would that be a
difference to using a PHY address.

> Maybe base is the wrong name, since MSCC can have the base as the high
> address of the four, not the low?

I'd say it might be any of the four addresses as long as it is the same
across the PHYs in the same package. And in that case you can also have
the phy_package_read/write() functions.

-michael