netdev - RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

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: <BL2PR03MB545A213D584384018CCB30CE67E0@BL2PR03MB545.namprd03.prod.outlook.com>
Date:	Wed, 12 Aug 2015 15:27:47 +0000
From:	Madalin-Cristian Bucur <madalin.bucur@...escale.com>
To:	Stas Sergeev <stsp@...t.ru>,
	Florian Fainelli <f.fainelli@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>
CC:	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Liberman Igal <Igal.Liberman@...escale.com>,
	Stas Sergeev <stsp@...rs.sourceforge.net>,
	"joakim.tjernlund@...nsmode.se" <joakim.tjernlund@...nsmode.se>,
	Shaohui Xie <Shaohui.Xie@...escale.com>
Subject: RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@...t.ru]

<snip>
> But have you looked into the patch I pointed previously?
> https://lkml.org/lkml/2015/7/20/711
> You code will likely clash with it because my patch extends
> of_phy_register_fixed_link().
> 

I admin I failed to grasp the details of your change - the lack of ample context
Lines makes it a bit difficult. I'm sure your change could be merged then the
of parsing could be separated from the actual fixed_phy_register() call if anyone
cares about that.

> > Then I could call only of_* functions but the end result would be the same
> > and  of_phy_register_fixed_phy() would not justify its existence that much...
> You didn't say you wanted to obsolete the of_phy_register_fixed_phy().
> Since it is there (and even changed by me in a way your
> patch will likely clash), IMHO it would be better if it is used,
> rather than copy/pasted into the driver.

Please note I was referring to a fictional new function that would embed the call to 
fixed_phy_register(). I was not talking about some existing API, just about a new 
of_call  named of_phy_register_fixed_phy()  that would in the end be called by
of_phy_register_fixed_link() and by some drivers that want to get in the middle
of things and get a hold on status.

Maybe the fact we're reviewing two patches in one thread is what makes the
discussion less than optimal.

> > The getter for status you suggest would be fine, but not sure how one
> > would retrieve
> > it from the mac_node unless we change of_phy_register_fixed_link() to
> > return the
> > pointer to phy (and all the drivers that use it...).
> If you look for instance to mvneta.c, you'll find the following:
> ---
> err = of_phy_register_fixed_link(dn);
> /* In the case of a fixed PHY, the DT node associated
>   * to the PHY is the Ethernet MAC DT node.
>   */
>   phy_node = of_node_get(dn);
> ...
> phy = of_phy_find_device(dn);
> ---
> 
> So the answer is: just use the same mac_node for both.

I understand, I'll use this approach although is suboptimal imho to
scan the device tree again to get a phy pointer that you need just
to get some of info that was parsed in a call you just made.

> >> Also I meant the description should have been in the patch,
> >> not in the e-mail. :) You only wrote _what_ the patch does
> >> (which is of course obvious from the code itself), but not
> >> _why_ and _what was fixed_ (what didn't work).
> >>
> > If you refer to the first, separation patch, I thought the description was
> enough:
> >
> >      of: separate fixed link parsing from registration
> >
> >      Some drivers may need
> "may need"? I don't understand.
> If it is a fix, then they _do need_, and in this case it should
> be specified what was broken and what is fixed.
> If it is just a clean-up, then "may need" may suffice, but it
> was not mentioned it is a clean-up. So I still don't know what
> this patch is all about.
> "Some drivers" - which ones? The ones that are limited to
> the purely fixed links, and never support AN or MDIO?
> Or some other drivers too?
> So really, the description sounds very cryptic to me.

Mine, when there is a fixed link node, maybe others. When there isn't any
fixed link node, the internal PHY config defaults to 1G full duplex AN enabled
and adjust link takes care of things.

> 
> >   to parse the fixed link values before registering
> >      the fixed link phy. Separate the parsing from the actual registration
> >      and provide an export for the added parsing function.
> >
> >      Signed-off-by: Madalin Bucur <madalin.bucur@...escale.com>
> >
> > For this one it was a bit brief, I admit - the longer version would be that
> before it
> > we were not using from fixed link anything else but the fact the link was
> fixed
> > (ignored actual speed, duplex values there)
> And what didn't work as the result?
> 
> >   and this patch tries to fix that.
> What started to work after that patch that didn't without it?

10M half duplex for instance

I'd close this thread for now and use in my driver of_phy_find_device(mac_node).

Thank you,
Madalin