Re: [PATCH] phy: core: Make 'phy_optional_get' return NULL when not implemented

[Sascha Hauer <sha@pengutronix.de>]

On Mon, Nov 08, 2021 at 02:01:58PM -0800, Trent Piepho wrote:
> On Mon, Nov 8, 2021 at 1:10 AM Sascha Hauer <sha@pengutronix.de> wrote:
> > On Thu, Nov 04, 2021 at 02:40:08PM -0700, Trent Piepho wrote:
> > >
> > > I think the specific situation you are concerned about is where:
> > > A) the dts does define a phy for usb
> > > B) This phy does not work in Barebox, e.g. no driver
> > > C) Despite B, USB will still operate with the desired level
> > > functionality without a working phy driver.
> > >
> > > With the patch and CONFIG_GENERIC_PHY disabled, we get a non-fatal
> > > return at step A and everything is good.  But once enabled, we now get
> > > a fatal error at step B and this is not good.
> > >
> > > Could this be fixed by making the error at B non-fatal?  This is more
> > > how Linux works here: errors that are non-fatal in Linux's
> > > phy_optional_get() path are fatal for Barebox.
> >
> > In Linux phy_optional_get() returns NULL (which is a valid phy) when
> > a) there is no "phys" property
> > b) The phy os compatible to "usb-nop-xceiv"
> > c) The phy has a status = "disabled" property
> 
> Add to this:
> d) Missing phy-names property.
> e) Named phy is not in phy-names.
> f) Malformed phys property.
> g) PHY's index from phy-names does not exist in phys property.
> h) PHY exists in phys property, but has empty phandle
> i) No '#phy-cells' property for phy.
> j) No of_node for device and phy is not in the global phy list. No
> driver, not defined, error adding, anything could cause this to
> happen.
> 
> And then we go to device specific cases:
> An Allwinner sun4i usb phy provider is given a phy index that is
> greater than the number of defined phys.
> A phy from a sun4i phy provider is marked as missing in the device
> configuration data, i.e. phy 1 or 2 on an Allwinner H6.
> A Broadcom stingray phy can not ioremap its registers with ENODEV.
> And the list goes on, but I think my point is made.

You are right, there are many more cases that lead to a -ENODEV which
causes phy_optional_get() to return a NULL phy. I don't think this is
intended and it doesn't make much sense to me.

> 
> > When there is a phy specified in the device tree then it's mandatory
> > and failures in getting it must lead to an error. the "optional" in
> 
> "Must lead to an error"  Can this be said with certainty?  I can find
> no real case where a warning results in a different end than an error.
> Put another way, suppose it does not lead to an error?  What specific
> bad thing happens?
> 
> > phy_optional_get() is only meant for the "there is no phy specified in
> > the device tree" case, it's only a shortcut for
> >
> >         if (is_there_a_phy_in_device_tree())
> >                 phy = phy_get();
> >         else
> >                 phy = NULL;
> 
> Perhaps that is what it was supposed to mean, I agree these seem like
> sensible semantics, but see the various errors I tracked down above,
> it's far far more complex than that.  IMHO, this complexity is
> evidence of a bad design.  No one can keep straight what errors are
> fatal and which are not.  It is better to just make all errors
> non-fatal and be done with it.  Then at least one knows what to
> expect.
> 
> > Once you interpret "optional" as something different you end up
> > returning NULL for cases where you really need a phy. Only you as a
> > human know that, in your special case, on your special board, there is a
> > phy specified, but it can do without initialisation. That's why I
> > suggested you should add a status = "disabled" property to the phy to
> > explicitly state that it can work without the phy. By making it explicit
> > you also document that you thought about the case and not just exploited
> > a heuristic in barebox.
> 
> I think returning NULL on every error, with a warning message, will
> allow this.  The warning lets the user know there may be an issue with
> the usb phy.  If it works, they can decide the warning is spurious and
> silence it, via existing dts editing methods (/delete-property/,
> status = disabled, etc.).  If it doesn't work, they know they need to
> enable a driver, write a driver, or perhaps disable USB entirely as
> unsupported.

Given that phy_optional_get() returns rather random results I tend to
agree now. Let's insert a warning, return successfully and see what
happens.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox