Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Swapnil,

On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade wrote:
> On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
> > On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
> > > Use generic PHY framework function phy_set_attrs() to set number of
> > > lanes and maximum link rate supported by PHY.
> > >
> > > Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> > > Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> > > ---
> > >  drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> > > b/drivers/phy/cadence/phy-cadence-torrent.c
> > > index 7116127358ee..eca71467c4a8 100644
> > > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > > @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
> > >  	struct cdns_torrent_phy *cdns_phy;
> > >  	struct device *dev = &pdev->dev;
> > >  	struct phy_provider *phy_provider;
> > > +	struct phy_attrs torrent_attr;
> > >  	const struct of_device_id *match;
> > >  	struct cdns_torrent_data *data;
> > >  	struct device_node *child;
> > > @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
> > >  				 cdns_phy->phys[node].num_lanes,
> > >  				 cdns_phy->max_bit_rate / 1000,
> > >  				 cdns_phy->max_bit_rate % 1000);
> > > +
> > > +			torrent_attr.bus_width = cdns_phy- >phys[node].num_lanes;
> > > +			torrent_attr.max_link_rate = cdns_phy->max_bit_rate;
> > > +			torrent_attr.mode = PHY_MODE_DP;
> > > +
> > > +			phy_set_attrs(gphy, &torrent_attr);
> > 
> > Why is this better than accessing the attributes manually as follows ?
> > 
> > 			gphy->attrs.bus_width = cdns_phy->phys[node].num_lanes;
> > 			gphy->attrs.max_link_rate = cdns_phy->max_bit_rate;
> > 			gphy->attrs.mode = PHY_MODE_DP;
> > 
> > This is called in cdns_torrent_phy_probe(), before the PHY provider is
> > registered, so nothing can access the PHY yet. What race condition are you
> > trying to protect against with usage of phy_set_attrs() ?
> 
> I agree that for Cadence DP bridge driver and Torrent PHY driver use case, it
> would not matter even if we set the attributes in Torrent PHY driver in a way
> you suggested above.
> But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs in future could
> maybe used by other drivers replacing existing individual functions for attributes
> bus_width and mode which are phy_set_bus_width/phy_get_bus_width and
> phy_set_mode/phy_get_mode. So this usage in Torrent PHY driver is an example
> implementation of the API.
> 
> [1] https://lkml.org/lkml/2020/5/18/472

This doesn't seem a very good API to me :-S It will require callers to
always call phy_get_attrs() first, modify the attributes they want to
set, and then call phy_set_attrs(). Not only will be copy the whole
phy_attrs structure needlessly, it will also not be an atomic operation
as someone else could modify attributes between the get and set calls.
The lack of atomicity may not be an issue in practice if there's a
single user of the PHY at all times, but in that case no mutex is
needed.

I think this series tries to fix a problem that doesn't exist.

> > >  		} else {
> > >  			dev_err(dev, "Driver supports only PHY_TYPE_DP\n");
> > >  			ret = -ENOTSUPP;

-- 
Regards,

Laurent Pinchart