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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR07MB6154CC4A67BC3568A7339CC9C52F0@DM6PR07MB6154.namprd07.prod.outlook.com>
Date:   Wed, 2 Sep 2020 07:09:21 +0000
From:   Swapnil Kashinath Jakhade <sjakhade@...ence.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
CC:     "vkoul@...nel.org" <vkoul@...nel.org>,
        "kishon@...com" <kishon@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "maxime@...no.tech" <maxime@...no.tech>,
        Milind Parab <mparab@...ence.com>,
        Yuti Suresh Amonkar <yamonkar@...ence.com>,
        "nsekhar@...com" <nsekhar@...com>,
        "tomi.valkeinen@...com" <tomi.valkeinen@...com>,
        "jsarha@...com" <jsarha@...com>,
        "praneeth@...com" <praneeth@...com>
Subject: RE: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set
 PHY attributes

Hi Laurent,

Thank you for your review comments.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> Sent: Wednesday, September 2, 2020 6:00 AM
> To: Swapnil Kashinath Jakhade <sjakhade@...ence.com>
> Cc: vkoul@...nel.org; kishon@...com; linux-kernel@...r.kernel.org;
> maxime@...no.tech; Milind Parab <mparab@...ence.com>; Yuti Suresh
> Amonkar <yamonkar@...ence.com>; nsekhar@...com;
> tomi.valkeinen@...com; jsarha@...com; praneeth@...com
> Subject: Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set
> PHY attributes
> 
> EXTERNAL MAIL
> 
> 
> Hi Swapnil,
> 
> Thank you for the patch.
> 
> 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@...ence.com>
> > Acked-by: Kishon Vijay Abraham I <kishon@...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://lkml.org/lkml/2020/5/18/472

Thanks & regards,
Swapnil

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