netdev - Re: [PATCH 1/3] net: socionext: Stop PHY before resetting netsec

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: <CADQ0-X9pTeKxWwFq-c_2p6_P2kSUaSeFKqMx+rFLHwfrWmu4UA@mail.gmail.com>
Date:   Fri, 19 Oct 2018 15:24:08 +0900
From:   Masahisa Kojima <masahisa.kojima@...aro.org>
To:     f.fainelli@...il.com
Cc:     netdev@...r.kernel.org,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Yoshitoyo Osaki <osaki.yoshitoyo@...ionext.com>
Subject: Re: [PATCH 1/3] net: socionext: Stop PHY before resetting netsec

Hi,

Thank you very much for your comment.
I have not realized that reading MII_PHYSID1/ID2 are ignored
if the PHY is in power down, this patch has a side effect
in ACPI case.

My intention is that netsec_reset_hardware() should be called
in PHY power down state, but current place to add PHY power down
is not proper.
I will consider the right place to do.

On Fri, 19 Oct 2018 at 11:59, Florian Fainelli <f.fainelli@...il.com> wrote:
>
>
>
> On 10/18/2018 6:08 PM, masahisa.kojima@...aro.org wrote:
> > From: Masahisa Kojima <masahisa.kojima@...aro.org>
> >
> > After resetting netsec IP, driver have to wait until
> > netsec mode turns to NRM mode.
> > But sometimes mode transition to NRM will not complete
> > if the PHY is in normal operation state.
> > To avoid this situation, stop PHY before resetting netsec.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@...aro.org>
> > Signed-off-by: Yoshitoyo Osaki <osaki.yoshitoyo@...ionext.com>
> > ---
> >  drivers/net/ethernet/socionext/netsec.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
> > index 4289ccb26e4e..273cc5fc07e0 100644
> > --- a/drivers/net/ethernet/socionext/netsec.c
> > +++ b/drivers/net/ethernet/socionext/netsec.c
> > @@ -1343,11 +1343,11 @@ static int netsec_netdev_stop(struct net_device *ndev)
> >       netsec_uninit_pkt_dring(priv, NETSEC_RING_TX);
> >       netsec_uninit_pkt_dring(priv, NETSEC_RING_RX);
> >
> > -     ret = netsec_reset_hardware(priv, false);
> > -
> >       phy_stop(ndev->phydev);
> >       phy_disconnect(ndev->phydev);
> >
> > +     ret = netsec_reset_hardware(priv, false);
> > +
> >       pm_runtime_put_sync(priv->dev);
> >
> >       return ret;
> > @@ -1415,7 +1415,7 @@ static const struct net_device_ops netsec_netdev_ops = {
> >  };
> >
> >  static int netsec_of_probe(struct platform_device *pdev,
> > -                        struct netsec_priv *priv)
> > +                        struct netsec_priv *priv, u32 *phy_addr)
> >  {
> >       priv->phy_np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> >       if (!priv->phy_np) {
> > @@ -1423,6 +1423,8 @@ static int netsec_of_probe(struct platform_device *pdev,
> >               return -EINVAL;
> >       }
> >
> > +     *phy_addr = of_mdio_parse_addr(&pdev->dev, priv->phy_np);
> > +
> >       priv->clk = devm_clk_get(&pdev->dev, NULL); /* get by 'phy_ref_clk' */
> >       if (IS_ERR(priv->clk)) {
> >               dev_err(&pdev->dev, "phy_ref_clk not found\n");
> > @@ -1473,6 +1475,7 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
> >  {
> >       struct mii_bus *bus;
> >       int ret;
> > +     u16 data;
> >
> >       bus = devm_mdiobus_alloc(priv->dev);
> >       if (!bus)
> > @@ -1486,6 +1489,10 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
> >       bus->parent = priv->dev;
> >       priv->mii_bus = bus;
> >
> > +     /* set phy power down */
> > +     data = netsec_phy_read(bus, phy_addr, 0) | 0x800;
> > +     netsec_phy_write(bus, phy_addr, 0, data);
>
> This is not explained in your commit message, and this is using open
> coded values instead of the symbolic names from include/uapi/linux/mii.h.
>
> Note that depending on the type of PHY you are interfaced with if the
> PHY is in power down, the only register it is allowed to accept writes
> for is MII_BMCR (per 802.3 clause 22 spec), any other read or write to a
> different register can be discarded by its MDIO snooping logic. Since
> probing the MDIO bus involves reading MII_PHYSID1/ID2, what is this
> trying to achieve?
>
> > +
> >       if (dev_of_node(priv->dev)) {
> >               struct device_node *mdio_node, *parent = dev_of_node(priv->dev);
> >
> > @@ -1623,7 +1630,7 @@ static int netsec_probe(struct platform_device *pdev)
> >       }
> >
> >       if (dev_of_node(&pdev->dev))
> > -             ret = netsec_of_probe(pdev, priv);
> > +             ret = netsec_of_probe(pdev, priv, &phy_addr);
> >       else
> >               ret = netsec_acpi_probe(pdev, priv, &phy_addr);
> >       if (ret)
> >
>
> --
> Florian