netdev - Re: [PATCH net-next 3/5] net: phy: allow Clause 45 access via mii ioctl

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: <CA+h21hpXv7678MuKVfAGiwuQwzZHX_1hjXHpwZUFz8wP5aRabg@mail.gmail.com>
Date:   Tue, 28 May 2019 15:54:31 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Russell King <rmk+kernel@...linux.org.uk>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 3/5] net: phy: allow Clause 45 access via mii ioctl

On Tue, 28 May 2019 at 12:58, Russell King <rmk+kernel@...linux.org.uk> wrote:
>
> Allow userspace to generate Clause 45 MII access cycles via phylib.
> This is useful for tools such as mii-diag to be able to inspect Clause
> 45 PHYs.
>
> Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
> Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
> ---
>  drivers/net/phy/phy.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 3745220c5c98..6d279c2ac1f8 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -386,6 +386,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>         struct mii_ioctl_data *mii_data = if_mii(ifr);
>         u16 val = mii_data->val_in;
>         bool change_autoneg = false;
> +       int prtad, devad;
>
>         switch (cmd) {
>         case SIOCGMIIPHY:
> @@ -393,14 +394,29 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>                 /* fall through */
>
>         case SIOCGMIIREG:
> -               mii_data->val_out = mdiobus_read(phydev->mdio.bus,
> -                                                mii_data->phy_id,
> -                                                mii_data->reg_num);
> +               if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> +                       prtad = mdio_phy_id_prtad(mii_data->phy_id);
> +                       devad = mdio_phy_id_devad(mii_data->phy_id);
> +                       devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
> +               } else {
> +                       prtad = mii_data->phy_id;
> +                       devad = mii_data->reg_num;
> +               }
> +               mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad,
> +                                                devad);
>                 return 0;
>
>         case SIOCSMIIREG:
> -               if (mii_data->phy_id == phydev->mdio.addr) {
> -                       switch (mii_data->reg_num) {
> +               if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> +                       prtad = mdio_phy_id_prtad(mii_data->phy_id);
> +                       devad = mdio_phy_id_devad(mii_data->phy_id);
> +                       devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
> +               } else {
> +                       prtad = mii_data->phy_id;
> +                       devad = mii_data->reg_num;
> +               }
> +               if (prtad == phydev->mdio.addr) {
> +                       switch (devad) {
>                         case MII_BMCR:
>                                 if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
>                                         if (phydev->autoneg == AUTONEG_ENABLE)
> @@ -433,11 +449,10 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>                         }
>                 }
>
> -               mdiobus_write(phydev->mdio.bus, mii_data->phy_id,
> -                             mii_data->reg_num, val);
> +               mdiobus_write(phydev->mdio.bus, prtad, devad, val);
>
> -               if (mii_data->phy_id == phydev->mdio.addr &&
> -                   mii_data->reg_num == MII_BMCR &&
> +               if (prtad == phydev->mdio.addr &&
> +                   devad == MII_BMCR &&
>                     val & BMCR_RESET)
>                         return phy_init_hw(phydev);
>
> --
> 2.7.4
>

Hi Russell,

I find the SIOCGMIIREG/SIOCGMIIPHY ioctls useful for C45 just as much
as they are for C22, but I think the way they work is a big hack and
for that reason they're less than useful when you need them most.
These ioctls work by hijacking the MDIO bus driver of a PHY that is
attached to a net_device. Hence they can be used to access at most a
PHY that lies on the same MDIO bus as one you already have a
phy-handle to.
If you have a PHY issue that makes of_phy_connect fail and the
net_device to fail to probe, basically you're SOL because you lose
that one handle that userspace had to the MDIO bus.
Similarly if you're doing a bring-up and all PHY interfaces are fixed-link.
Maybe it would be better to rethink this and expose some sysfs nodes
for raw MDIO access in the bus drivers.

-Vladimir