netdev - Re: [RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of an MDIO bus and PHYs.

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: <20160312000528.GD23969@lunn.ch>
Date:	Sat, 12 Mar 2016 01:05:28 +0100
From:	Andrew Lunn <andrew@...n.ch>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 2/2] phy: fixed-phy: Allow DT description of
 an MDIO bus and PHYs.

> > +Child nodes represent PHYs on this mdio bus. Standard properties for
> > +fixed links, 'speed', 'full-duplex', 'pause', 'asym-pause',
> > +'link-gpios', as defined above are used. Additionally a 'reg' property
> > +is required for the address of the PHY on the bus. This should be of
> > +value 0 to 31.
> 
> This is a virtual bus, the only limitation is because we re-use to the
> maximum permission extent the real MDIO bus code, and this is putting a
> SW constraint on something that does not have one here.

True. I was too lazy to audit the phy and mdio code to see if it
actually allows phy_addr >= 32. And it is easy to instantiate another
bus if you need it.

> > +
> > +Example
> > +-------
> > +
> > +mdio {
> > +	compatible = "linux,mdio-fixed-phy";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	phy0: phy@0 {
> > +		reg = <0>;
> > +		speed = <1000>;
> > +		pause;
> > +		link-gpios = <&gpio0 12 GPIO_ACTIVE_HIGH>;
> > +	};
> > +
> > +	phy1: phy@8 {
> > +		reg = <8>;
> > +		speed = <100>;
> > +		full-duplex;
> > +	};
> 
> The 'fixed-link' property, although it suffers from one basic issue
> which is that it is placed on the consumer of it (e.g: an Ethernet MAC
> Device Tree node) while being a two-headed snake (you actually describe
> a data-pipe with a fixed-link) is not quite perfect, but this does not
> seem to look any better.

It is actually just the same. Not worse, not better.

I see the real benefit in the drivers. They get fixed-phy for
free. Cleanup on release just works, which most drivers don't even do
now, so leak a fixed phy every time they are unloaded. DSA has some
messy code, which has had a number of bugs, handling fixed phys. By
adding this extra code, which i hope is reasonably simple to
understand and review, we make the MAC drivers a lot simpler and less
error prone.

> One thing that Thomas solved nicely was avoid the need for allocating an
> address on the virtual MDIO bus, but this is coming back here, which
> does not seem needed except for correctness wrt. how real MDIO buses.

Actually Thomas's code is broken. Fixed phy addresses are allocated
incrementally. When a fixed phy is released, its address is not
re-used. So as soon as you have gone through 32 alloc/free cycles, all
further allocs fail. This is however fixable.

	Andrew