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

[Kishon Vijay Abraham I]

Hi Swapnil,

On 9/3/2020 4:29 PM, Swapnil Kashinath Jakhade wrote:
> 
> 
>> -----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.

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 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?

Thanks
Kishon

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