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

[Swapnil Kashinath Jakhade]

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, September 2, 2020 5:47 PM
> To: Swapnil Kashinath Jakhade <sjakhade@xxxxxxxxxxx>
> Cc: vkoul@xxxxxxxxxx; kishon@xxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> maxime@xxxxxxxxxx; Milind Parab <mparab@xxxxxxxxxxx>; Yuti Suresh
> Amonkar <yamonkar@xxxxxxxxxxx>; nsekhar@xxxxxx;
> tomi.valkeinen@xxxxxx; jsarha@xxxxxx; praneeth@xxxxxx
> Subject: Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set
> PHY attributes
> 
> EXTERNAL MAIL
> 
> 
> 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@xxxxxxxxxxx>
> > > > Acked-by: Kishon Vijay Abraham I <kishon@xxxxxx>
> > > > ---
> > > >  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://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;!!EH
> > scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-
> SYIEEibYdpBMJTll
> > Yrd-00$
> 
> 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.

Thanks Laurent for your comments.

Hi Kishon,

Could you please suggest what would be the better approach regarding this PHY
attributes series. Should we add individual get/set functions for new attribute
max_link_rate just like mode and bus_width, or should we use phy_get_attrs()
and phy_set_attrs() functions removing mutex.  Your suggestions would really help.

Thanks & regards,
Swapnil

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