On 10-09-20, 11:45, Kishon Vijay Abraham I wrote: > Hi Milind, > > On 08/09/20 7:45 pm, Milind Parab wrote: > > 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 > > yeah, let's deal with dynamic attributes later when the use cases arise > unless Vinod disagrees.
I am fine with it, we can modify later as the need arises -- ~Vinod
