linux-kernel - Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy eee-disable-advert option documentation

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: <1479744993.17538.85.camel@baylibre.com>
Date:   Mon, 21 Nov 2016 17:16:33 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, devicetree@...r.kernel.org,
        Florian Fainelli <f.fainelli@...il.com>,
        Alexandre TORGUE <alexandre.torgue@...com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Kevin Hilman <khilman@...libre.com>,
        linux-kernel@...r.kernel.org, Andre Roth <neolynx@...il.com>,
        linux-amlogic@...ts.infradead.org, Carlo Caione <carlo@...one.org>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC PATCH net v2 2/3] dt: bindings: add ethernet phy
 eee-disable-advert option documentation

On Mon, 2016-11-21 at 17:01 +0100, Andrew Lunn wrote:
> On Mon, Nov 21, 2016 at 04:35:23PM +0100, Jerome Brunet wrote:
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@...libre.com>
> > ---
> >  Documentation/devicetree/bindings/net/phy.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/phy.txt
> > b/Documentation/devicetree/bindings/net/phy.txt
> > index bc1c3c8bf8fa..7f066b7c1e2c 100644
> > --- a/Documentation/devicetree/bindings/net/phy.txt
> > +++ b/Documentation/devicetree/bindings/net/phy.txt
> > @@ -35,6 +35,11 @@ Optional Properties:
> >  - broken-turn-around: If set, indicates the PHY device does not
> > correctly
> >    release the turn around line low at the end of a MDIO
> > transaction.
> >  
> > +- eee-advert-disable: Bits to clear in the MDIO_AN_EEE_ADV
> > register to
> > +  disable EEE modes. Example
> > +    * 0x4: disable EEE for 1000T,
> > +    * 0x6: disable EEE for 100TX and 1000T
> > +
> 
> Hi Jerome
> 
> I like the direction this patchset is taking. But hex values are
> pretty unfriendly. 

Agreed

> Please add a set of boolean properties, and do the
> mapping to hex in the C code.
> 
> That would also make extending this API easier. e.g. say you have a
> 10Gbps PHY with EEE, and you need to disable it. This hex value
> quickly gets ugly, eee-advert-disable-10000 is nice and simple.

What I did not realize when doing this patch for the realtek driver is
that there is already 6 valid modes defined in the kernel

#define MDIO_EEE_100TX		MDIO_AN_EEE_ADV_100TX	/*
100TX EEE cap */
#define MDIO_EEE_1000T		MDIO_AN_EEE_ADV_1000T	/*
1000T EEE cap */
#define MDIO_EEE_10GT		0x0008	/* 10GT EEE cap */
#define MDIO_EEE_1000KX		0x0010	/* 1000KX EEE cap
*/
#define MDIO_EEE_10GKX4		0x0020	/* 10G KX4 EEE cap
*/
#define MDIO_EEE_10GKR		0x0040	/* 10G KR EEE cap
*/

I took care of only 2 in the case of realtek.c since it only support
MDIO_EEE_100TX and MDIO_EEE_1000T.

Defining a property for each is certainly doable but it does not look
very nice either. If it extends in the future, it will get even more
messier, especially if you want to disable everything.

What do you think about keeping a single mask value but use the define
above in the DT ? It would be more readable than hex and easy to
extend, don't you think ?

These defines are already part of the uapi so I guess we can use those
in the DT bindings ?

> 
> 	Andrew