linux-kernel - Re: [RFC PATCH 01/11] phy: core add phy_set_netif

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] [day] [month] [year] [list]

Date:   Thu, 25 Oct 2018 15:35:46 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Grygorii Strashko <grygorii.strashko@...com>,
        "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
        Tony Lindgren <tony@...mide.com>,
        Rob Herring <robh+dt@...nel.org>
CC:     Sekhar Nori <nsekhar@...com>, <linux-kernel@...r.kernel.org>,
        <linux-omap@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [RFC PATCH 01/11] phy: core add phy_set_netif_mode() api

Hi,

On Wednesday 10 October 2018 04:13 AM, Grygorii Strashko wrote:
> 
> 
> On 10/09/2018 12:22 AM, Kishon Vijay Abraham I wrote:
>> Hi Grygorii,
>>
>> On Tuesday 09 October 2018 05:19 AM, Grygorii Strashko wrote:
>>> Add new API phy_set_netif_mode(struct phy *phy, phy_interface_t mode) and
>>> new PHY operation callback .set_netif_mode() which intended to be implemnte
>>> by PHY drivers which supports Network interrfaces mode selection. Both
>>> accepts phy_interface_t vlaue as input parameter.
>>>
>>> Cc: Kishon Vijay Abraham I <kishon@...com>
>>> Cc: Tony Lindgren <tony@...mide.com>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
>>> ---
>>>   drivers/phy/phy-core.c  | 15 +++++++++++++++
>>>   include/linux/phy/phy.h | 12 ++++++++++++
>>>   2 files changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index 35fd38c..d9aba1a 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -377,6 +377,21 @@ int phy_set_mode(struct phy *phy, enum phy_mode mode)
>>>   }
>>>   EXPORT_SYMBOL_GPL(phy_set_mode);
>>>   
>>> +int phy_set_netif_mode(struct phy *phy, phy_interface_t mode)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (!phy || !phy->ops->set_netif_mode)
>>> +		return 0;
>>> +
>>> +	mutex_lock(&phy->mutex);
>>> +	ret = phy->ops->set_netif_mode(phy, mode);
>>> +	mutex_unlock(&phy->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(phy_set_netif_mode);
>>
>> We should try to add only generic PHY APIs and not subsystem specific APIs. In
>> this case I think phy_set_mode should suffice.
> 
> 
> This is what I've had in mind first, but all my guts argued against it after I've tried:
> 
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index bc73d2b..961b156 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -41,6 +41,14 @@ enum phy_mode {
>         PHY_MODE_10GKR,
>         PHY_MODE_UFS_HS_A,
>         PHY_MODE_UFS_HS_B,
> +       PHY_MODE_MODE_MII,
> +       PHY_MODE_MODE_GMII,
> +       PHY_MODE_MODE_SGMII,
> +       PHY_MODE_MODE_RMII,
> +       PHY_MODE_MODE_RGMII,
> +       PHY_MODE_MODE_RGMII_ID,
> +       PHY_MODE_MODE_RGMII_RXID,
> +       PHY_MODE_MODE_RGMII_TXID,
>  };
> 
> above introduces ugly constants duplication and required every network phy driver
> to maintain conversation table  phy_interface_t -> enum phy_mode.
> More over, if above change happens third time (first time PHY_MODE_SGMII/PHY_MODE_10GKR were added,
> second - PHY_MODE_2500SGMII) it will never ends (there are ~15 more items only in phy_interface_t).
> As result, enum phy_mode might became a un-maintainable monster.
> 
> So, as per above, and considering that Network subsystem is based on standards (phy_interface_t lists standard intf)
> I've tried to add separate PHY API.
> 
> As an idea:
> - seems it could be reasonable to introduce PHY_MODE_NETWORK (or PHY_MODE_ETHERNET) and
> add generic phy_set_submode(struct phy *phy, long submode). 
> 
> So, single functional PHY device can just use phy_set_submode() and
> multi-functional devices (like serdes which can be muxed between PCIe, USB, NET), can use:
> 
> phy_set_mode(PHY_MODE_ETHERNET)
> phy_set_submode(X);

Agreed on the constant duplication comment above. We can modify set_mode to
take submode as an additional parameter and fix all the users of phy_set_mode.
int phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)

Thanks
Kishon