Messages in this thread | | | Subject | Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes | From | Kishon Vijay Abraham I <> | Date | Thu, 10 Sep 2020 11:45:21 +0530 |
| |
Hi Milind,
On 08/09/20 7:45 pm, Milind Parab wrote: > Hi Kishon, > >> -----Original Message----- >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Sent: Thursday, September 3, 2020 9:00 PM >> To: Kishon Vijay Abraham I <kishon@ti.com> >> Cc: Swapnil Kashinath Jakhade <sjakhade@cadence.com>; vkoul@kernel.org; >> linux-kernel@vger.kernel.org; maxime@cerno.tech; Milind Parab >> <mparab@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com>; >> nsekhar@ti.com; tomi.valkeinen@ti.com; jsarha@ti.com; praneeth@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 <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://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
yeah, let's deal with dynamic attributes later when the use cases arise unless Vinod disagrees.
Thanks Kishon
| |