Hi Kishon,
>-----Original Message-----
>From: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>Sent: Thursday, September 3, 2020 9:00 PM
>To: Kishon Vijay Abraham I <kis...@ti.com>
>Cc: Swapnil Kashinath Jakhade <sjakh...@cadence.com>; vk...@kernel.org;
>linux-kernel@vger.kernel.org; max...@cerno.tech; Milind Parab
><mpa...@cadence.com>; Yuti Suresh Amonkar <yamon...@cadence.com>;
>nsek...@ti.com; tomi.valkei...@ti.com; jsa...@ti.com; prane...@ti.com
>Subject: Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set
>PHY attributes
>
>EXTERNAL MAIL
>
>
>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 <sjakh...@cadence.com>
>> >>>>> Acked-by: Kishon Vijay Abraham I <kis...@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://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;
>> >>> !!EH
>> >>> scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-
>SYIEEibYdpBM
>> >>> JTll
>> >>> 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.
>
Should we proceed accessing attribute manually
>> >>>>> } else {
>> >>>>> dev_err(dev, "Driver supports only
>PHY_TYPE_DP\n");
>> >>>>> ret = -ENOTSUPP;
>
>--
>Regards,
>
>Laurent Pinchart
