netdev - Re: [PATCH net-next 0/2] Convert Ocelot and Felix switches to PHYLINK

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: <20191120232152.p22rfjdngm4wtmak@soft-dev3.microsemi.net>
Date:   Thu, 21 Nov 2019 00:21:53 +0100
From:   Horatiu Vultur <horatiu.vultur@...rochip.com>
To:     Vladimir Oltean <olteanv@...il.com>
CC:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Joergen Andreasen <joergen.andreasen@...rochip.com>,
        "Allan W. Nielsen" <allan.nielsen@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "Y.b. Lu" <yangbo.lu@....com>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 0/2] Convert Ocelot and Felix switches to PHYLINK

> >  };
> >
> >  &port0 {
> > +       phy-mode = "sgmii";
> >         phy-handle = <&phy0>;
> >  };
> >
> >  &port1 {
> > +       phy-mode = "sgmii";
> >         phy-handle = <&phy1>;
> >  };
> >
> >  &port2 {
> > +       phy-mode = "sgmii";
> >         phy-handle = <&phy2>;
> >  };
> >
> >  &port3 {
> > +       phy-mode = "sgmii";
> >         phy-handle = <&phy3>;
> >  };
> > diff --git a/drivers/net/ethernet/mscc/ocelot_board.c b/drivers/net/ethernet/mscc/ocelot_board.c
> > index aecaf4ef6ef4..9dad031900b5 100644
> > --- a/drivers/net/ethernet/mscc/ocelot_board.c
> > +++ b/drivers/net/ethernet/mscc/ocelot_board.c
> > @@ -513,6 +513,10 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
> >                 if (IS_ERR(regs))
> >                         continue;
> >
> > +               of_get_phy_mode(portnp, &phy_mode);
> > +               if (phy_mode == PHY_INTERFACE_MODE_NA)
> > +                       continue;
> > +
> 
> So this effectively reverts your own patch 4214fa1efffd ("net: mscc:
> ocelot: omit error check from of_get_phy_mode")?

Not really, at that point it was OK to have interface
PHY_INTERFACE_MODE_NA. There were few more checks before creating the
network device. Now with your changes you were creating
a network device for each port of the soc even if some ports
were not used on a board. Also with your changes you first create the
port and after that you create the phylink but between these two calls it
was the switch which continue for the interface PHY_INTERFACE_MODE_NA,
which is not correct. So these are the 2 reason why I have added the
property phy-mode to the ports and add back the check to see which ports
are used on each board.

> 
> >                 err = ocelot_probe_port(ocelot, port, regs);
> >                 if (err) {
> >                         of_node_put(portnp);
> > @@ -523,11 +527,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
> >                 priv = container_of(ocelot_port, struct ocelot_port_private,
> >                                     port);
> >
> > -               of_get_phy_mode(portnp, &phy_mode);
> > -
> >                 switch (phy_mode) {
> > -               case PHY_INTERFACE_MODE_NA:
> > -                       continue;
> >                 case PHY_INTERFACE_MODE_SGMII:
> >                         break;
> >                 case PHY_INTERFACE_MODE_QSGMII:
> > @@ -549,20 +549,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
> >                 }
> >
> >                 serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> > -               if (IS_ERR(serdes)) {
> > -                       err = PTR_ERR(serdes);
> > -                       if (err == -EPROBE_DEFER)
> > -                               dev_dbg(ocelot->dev, "deferring probe\n");
> 
> Why did you remove the probe deferral for the serdes phy?
Because not all the ports have the "phys" property.
> 
> > -                       else
> > -                               dev_err(ocelot->dev,
> > -                                       "missing SerDes phys for port%d\n",
> > -                                       port);
> > -
> > -                       of_node_put(portnp);
> > -                       goto out_put_ports;
> > -               }
> > -
> > -               if (serdes) {
> > +               if (!IS_ERR(serdes)) {
> >                         err = phy_set_mode_ext(serdes, PHY_MODE_ETHERNET,
> >                                                phy_mode);
> >                         if (err) {
> > --
> > 2.17.1
> >
> >
> > >
> > >    Andrew
> >
> > --
> > /Horatiu
> 
> Thanks,
> -Vladimir

-- 
/Horatiu