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

[Laurent Pinchart]

Hi Kishon,

On Thu, Sep 03, 2020 at 05:00:14PM +0530, Kishon Vijay Abraham I wrote:
> On 9/3/2020 4:29 PM, Swapnil Kashinath Jakhade wrote:
> > On Wednesday, September 2, 2020 5:47 PM, Laurent Pinchart wrote:
> >> 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.
> 
> What if the consumer tries to set an attribute at the middle of a
> phy_power_on() operation? That is still a valid operation and phy core layer
> should try to prevent it no?

I see multiple questions here.

First of all, unless I'm mistaken, the attributes set here are static
properties, set by the PHY driver at probe time, and only read by PHY
consumers. There should be no need for any kind of protection or special
API to access them.

Then, there's the question of how to handle dynamic attributes. In
theory a dynamic attribute could be changed at any time, and thus race
wit, for instance phy_power_on(). However, the proposed API won't help
much address this issue. Using a mutex will indeed ensure that the
attribute change will be serialized with other operations, but it won't
give any guarantee to the PHY consumer on whether the attribute will be
set before or after phy_power_on() is processed. The consumer will not
know if the new value of the attribute has been taken into account.

The question is thus whether we want to make the PHY consumer API
thread-safe (note that due to the usage of a mutex, we don't support
calling most of the API functions from an interrupt handler, so it
really requires the consumer to use a work queue, a thread, or possibly
a threaded interrupt). If the answer is yes, the API should define what
use cases are valid, and how the PHY has to behave. This includes
documenting when new attribute values can be set, and when they are
taken into account. If we had to document this as part of this patch
series, we would have to state that the new values are taken into
account at an undefined point of time if the attribute set call is
concurrent with other API calls, which makes the API ill-defined in my
opinion. I expect that we would need to turn attribute setting into a
callback to the PHY driver in that case, or at least make it a more
complex operation handled by the PHY core that would use the existing
PHY ops to reconfigure the PHY.

Is it worth it allowing drivers to call the PHY API from different
threads as opposed to requiring consumers to serialize calls if their
use cases require so ? I would expect most consumers to only try to
reconfigure a PHY when it's stopped, or to manually stop, reconfigure
and restart the PHY.

> >> 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.
> 
> I think Laurent's point is not having an API at all for configuring attributes
> and access them manually?

If the answer to the above question is that a thread-safe API isn't
worth it as we wouldn't have good use cases for it, then I think
accessing the attributes manually is all we need.

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

-- 
Regards,

Laurent Pinchart