netdev - Re: [PATCH net-next v3 11/11] net: mscc: ocelot: make use of SerDes PHYs for handling their configuration

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: <20181001094245.cr4hdcechrqkjymq@qschulz>
Date:   Mon, 1 Oct 2018 11:42:45 +0200
From:   Quentin Schulz <quentin.schulz@...tlin.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     alexandre.belloni@...tlin.com, ralf@...ux-mips.org,
        paul.burton@...s.com, jhogan@...nel.org, robh+dt@...nel.org,
        mark.rutland@....com, davem@...emloft.net, kishon@...com,
        andrew@...n.ch, allan.nielsen@...rochip.com,
        linux-mips@...ux-mips.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        thomas.petazzoni@...tlin.com
Subject: Re: [PATCH net-next v3 11/11] net: mscc: ocelot: make use of SerDes
 PHYs for handling their configuration

Hi Florian,

On Sat, Sep 15, 2018 at 02:25:05PM -0700, Florian Fainelli wrote:
> 
> 
> On 09/14/18 01:16, Quentin Schulz wrote:
> > Previously, the SerDes muxing was hardcoded to a given mode in the MAC
> > controller driver. Now, the SerDes muxing is configured within the
> > Device Tree and is enforced in the MAC controller driver so we can have
> > a lot of different SerDes configurations.
> > 
> > Make use of the SerDes PHYs in the MAC controller to set up the SerDes
> > according to the SerDes<->switch port mapping and the communication mode
> > with the Ethernet PHY.
> 
> This looks good, just a few comments below:
> 
> [snip]
> 
> > +		err = of_get_phy_mode(portnp);
> > +		if (err < 0)
> > +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> > +		else
> > +			ocelot->ports[port]->phy_mode = err;
> > +
> > +		switch (ocelot->ports[port]->phy_mode) {
> > +		case PHY_INTERFACE_MODE_NA:
> > +			continue;
> 
> Would not you want to issue a message indicating that the Device Tree
> must be updated here? AFAICT with your patch series, this should no
> longer be a condition that you will hit unless you kept the old DTB
> around, right?
> 

It'll occur for internal PHYs. On the PCB123[1], there are four of them,
so we need to be able to give no mode in the DT for those. For the
upcoming PCB120, there'll be 4 external PHYs that require a mode in the
DT and 4 internal PHYs that do not require any mode. I could put a debug
message that says this or that PHY is configured as an internal PHY but
I wouldn't put a message that is printed with the default log level.

So I think we should keep it, shouldn't we?

[1] https://elixir.bootlin.com/linux/latest/source/arch/mips/boot/dts/mscc/ocelot_pcb123.dts

> > +		case PHY_INTERFACE_MODE_SGMII:
> > +			phy_mode = PHY_MODE_SGMII;
> > +			break;
> > +		case PHY_INTERFACE_MODE_QSGMII:
> > +			phy_mode = PHY_MODE_QSGMII;
> > +			break;
> > +		default:
> > +			dev_err(ocelot->dev,
> > +				"invalid phy mode for port%d, (Q)SGMII only\n",
> > +				port);
> > +			return -EINVAL;
> > +		}
> > +
> > +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> > +		if (IS_ERR(serdes)) {
> > +			err = PTR_ERR(serdes);
> > +			if (err == -EPROBE_DEFER) {
> 
> This can be simplified into:
> 
> 			if (err == -EPROBE_DEFER)
> 				dev_dbg();
> 			else
> 				dev_err();
> 			goto err_probe_ports;
> 

Indeed, good catch.

Thanks,
Quentin

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)