linux-kernel - 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: <20200420153625.GA917792@lunn.ch>
Date:   Mon, 20 Apr 2020 17:36:25 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Michael Walle <michael@...le.cc>
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

> 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?

In general, drivers should not be using __foo functions. We want
drivers to make use of phy_package_write() which would do the bus
locking. Look at a typical PHY driver. There is no locking what so
ever. Just lots of phy_read() and phy write(). The locking is done by
the core and so should be correct.

> > > > 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.

For a cookie, i don't care how the driver decides on the cookie. The
core never uses it, other than comparing cookies to combine individual
PHYs into a package. It could be a PHY address. It could be the PHY
address where the global registers are. Or it could be anything else.

> > 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.

Yes. That is the semantics which is think is most useful. But then we
don't have a cookie, the value has real significance, and we need to
document what is should mean.

     Andrew