Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes
From: Kishon Vijay Abraham I
Date: Thu Sep 10 2020 - 02:15:47 EST
Hi Milind,
On 08/09/20 7:45 pm, Milind Parab wrote:
Hi Kishon,
-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Sent: Thursday, September 3, 2020 9:00 PM
To: Kishon Vijay Abraham I <kishon@xxxxxx>
Cc: Swapnil Kashinath Jakhade <sjakhade@xxxxxxxxxxx>; vkoul@xxxxxxxxxx;
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 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-
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