Re: [PATCH v7 phy-next 15/27] phy: move provider API out of public <linux/phy/phy.h>

[Vladimir Oltean <vladimir.oltean@nxp.com>]

Sashiko says:
> commit 65f3c7aba94024fd51e12abfeeb5cdaae7e123cd
> Author: Vladimir Oltean <vladimir.oltean@nxp.com>
> phy: move provider API out of public <linux/phy/phy.h>
> 
> The major goal is to hide the contents of struct phy from consumer
> drivers by moving the provider API out of the public linux/phy/phy.h
> header and into drivers/phy/phy-provider.h. For temporary compatibility,
> the consumer header currently includes the provider header via a relative
> path.
> 
> > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> > index 21aaf2f76e53e..a1aff00fba7c5 100644
> > --- a/drivers/phy/phy-core.c
> > +++ b/drivers/phy/phy-core.c
> [ ... ]
> > @@ -606,6 +622,24 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
> >  }
> >  EXPORT_SYMBOL_GPL(phy_validate);
> >
> > +enum phy_mode phy_get_mode(struct phy *phy)
> > +{
> > +	return phy->attrs.mode;
> 
> The PHY framework supports optional PHYs (represented as NULL pointers via
> devm_phy_optional_get()). Does this unconditional dereference cause a NULL
> pointer dereference if called with an optional PHY?
> 
> I noticed this was also the case in the previous inline implementation, and
> that a fix is provided later in the series in the commit "phy: make
> phy_get_mode(), phy_(get|set)_bus_width() NULL tolerant".
> 
> > +}
> > +EXPORT_SYMBOL_GPL(phy_get_mode);
> > +
> > +int phy_get_bus_width(struct phy *phy)
> > +{
> > +	return phy->attrs.bus_width;
> 
> Can this also cause a NULL pointer dereference when called with an optional
> PHY?
> 
> > +}
> > +EXPORT_SYMBOL_GPL(phy_get_bus_width);
> > +
> > +void phy_set_bus_width(struct phy *phy, int bus_width)
> > +{
> > +	phy->attrs.bus_width = bus_width;
> 
> Can this also cause a NULL pointer dereference when called with an optional
> PHY?

It was a conscious decision to move code first and then to start fixing
it up later, so that more context is present in 'git blame'. So I will
ignore these comments for issues that are resolved later in the series.

> > +}
> > +EXPORT_SYMBOL_GPL(phy_set_bus_width);
> [ ... ]
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > index ea47975e288ae..f208edd25afee 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -1,246 +1,38 @@
> >  /* SPDX-License-Identifier: GPL-2.0-or-later */
> >  /*
> > - * phy.h -- generic phy header file
> > + * phy.h -- Generic PHY consumer API
> [ ... ]
> > -#include <linux/phy/phy-mipi-dphy.h>
> > +#include <linux/phy/phy-props.h>
> >
> > +#include "../../../drivers/phy/phy-provider.h"
> 
> Does this relative include of a private driver header break out-of-tree
> module builds?
> 
> Out-of-tree modules compile against kernel headers packages that typically do
> not contain the private drivers/ directory tree, which would cause
> compilation to fail with a missing file error.
> 
> I note this is resolved later in the series in the commit "phy: remove
> temporary provider compatibility from consumer header".

Yes, out of tree building will temporarily be broken but resolved by the
end of the series for PHY consumers (but remains broken for PHY providers).
My understanding is that support for out of tree modules is best effort,
and I don't see how breakage could be avoided, given Vinod's request to
have phy-provider.h be a non-public header.