| 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
| ||
|
Message-ID: <38d7191f-e4bf-4457-9898-bb2b186ec3c7@quicinc.com>
Date: Tue, 10 Dec 2024 21:28:00 +0800
From: Lei Wei <quic_leiwei@...cinc.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet
<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni
<pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski
<krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Andrew Lunn
<andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>, <netdev@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<quic_kkumarcs@...cinc.com>, <quic_suruchia@...cinc.com>,
<quic_pavir@...cinc.com>, <quic_linchen@...cinc.com>,
<quic_luoj@...cinc.com>, <srinivas.kandagatla@...aro.org>,
<bartosz.golaszewski@...aro.org>, <vsmuthu@....qualcomm.com>,
<john@...ozen.org>, <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH net-next v2 3/5] net: pcs: qcom-ipq9574: Add PCS
instantiation and phylink operations
On 12/7/2024 12:28 AM, Russell King (Oracle) wrote:
> On Sat, Dec 07, 2024 at 12:20:25AM +0800, Lei Wei wrote:
>> On 12/4/2024 11:28 PM, Russell King (Oracle) wrote:
>>> On Wed, Dec 04, 2024 at 10:43:55PM +0800, Lei Wei wrote:
>>>> +static int ipq_pcs_enable(struct phylink_pcs *pcs)
>>>> +{
>>>> + struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> + struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>>>> + int index = qpcs_mii->index;
>>>> + int ret;
>>>> +
>>>> + ret = clk_prepare_enable(qpcs_mii->rx_clk);
>>>> + if (ret) {
>>>> + dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = clk_prepare_enable(qpcs_mii->tx_clk);
>>>> + if (ret) {
>>>> + dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
>>>> + clk_disable_unprepare(qpcs_mii->rx_clk);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void ipq_pcs_disable(struct phylink_pcs *pcs)
>>>> +{
>>>> + struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> +
>>>> + if (__clk_is_enabled(qpcs_mii->rx_clk))
>>>> + clk_disable_unprepare(qpcs_mii->rx_clk);
>>>> +
>>>> + if (__clk_is_enabled(qpcs_mii->tx_clk))
>>>> + clk_disable_unprepare(qpcs_mii->tx_clk);
>>>
>>> Why do you need the __clk_is_enabled() calls here? Phylink should be
>>> calling pcs_enable() once when the PCS when starting to use the PCS,
>>> and then pcs_disable() when it stops using it - it won't call
>>> pcs_disable() without a preceeding call to pcs_enable().
>>>
>>> Are you seeing something different?
>>
>> Yes, understand that phylink won't call pcs_disable() without a preceeding
>> call to pcs_enable(). However, the "clk_prepare_enable" may fail in the
>> pcs_enable() method, so I added the __clk_is_enabled() check in
>> pcs_disable() method. This is because the phylink_major_config() function
>> today does not interpret the return value of phylink_pcs_enable().
>
> Right, because failure is essentially fatal in that path - we have no
> context to return an error. I suppose we could stop processing at
> that point, but then it brings up the question of how to unwind anything
> we've already done, which is basically impossible at that point.
>
Sure, understand. I will remove the checks.
>>>> +static void ipq_pcs_get_state(struct phylink_pcs *pcs,
>>>> + struct phylink_link_state *state)
>>>> +{
>>>> + struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> + struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>>>> + int index = qpcs_mii->index;
>>>> +
>>>> + switch (state->interface) {
>>>> + case PHY_INTERFACE_MODE_SGMII:
>>>> + case PHY_INTERFACE_MODE_QSGMII:
>>>> + ipq_pcs_get_state_sgmii(qpcs, index, state);
>>>> + break;
>>>> + default:
>>>> + break;
>>> ...
>>>> +static int ipq_pcs_config(struct phylink_pcs *pcs,
>>>> + unsigned int neg_mode,
>>>> + phy_interface_t interface,
>>>> + const unsigned long *advertising,
>>>> + bool permit)
>>>> +{
>>>> + struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> + struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>>>> + int index = qpcs_mii->index;
>>>> +
>>>> + switch (interface) {
>>>> + case PHY_INTERFACE_MODE_SGMII:
>>>> + case PHY_INTERFACE_MODE_QSGMII:
>>>> + return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
>>>> + default:
>>>> + dev_err(qpcs->dev,
>>>> + "Unsupported interface %s\n", phy_modes(interface));
>>>> + return -EOPNOTSUPP;
>>>> + };
>>>> +}
>>>> +
>>>> +static void ipq_pcs_link_up(struct phylink_pcs *pcs,
>>>> + unsigned int neg_mode,
>>>> + phy_interface_t interface,
>>>> + int speed, int duplex)
>>>> +{
>>>> + struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
>>>> + struct ipq_pcs *qpcs = qpcs_mii->qpcs;
>>>> + int index = qpcs_mii->index;
>>>> + int ret;
>>>> +
>>>> + switch (interface) {
>>>> + case PHY_INTERFACE_MODE_SGMII:
>>>> + case PHY_INTERFACE_MODE_QSGMII:
>>>> + ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
>>>> + neg_mode, speed);
>>>> + break;
>>>> + default:
>>>> + dev_err(qpcs->dev,
>>>> + "Unsupported interface %s\n", phy_modes(interface));
>>>> + return;
>>>> + }
>>>
>>> So you only support SGMII and QSGMII. Rather than checking this in every
>>> method implementation, instead provide a .pcs_validate method that
>>> returns an error for unsupported interfaces please.
>>>
>>
>> Yes, I can add the pcs_validate() method to validate the link
>> configurations. This will catch invalid interface mode during the PCS
>> initialization time, earlier than the pcs_config and pcs_link_up contexts.
>>
>> But after of the PCS init, if at a later point the PHY interface mode
>> changes, it seems phylink today is not calling the pcs_validate() op to
>> validate the changed new interface mode at the time of "phylink_resolve".
>
> ... because by that time it's way too late. Phylink will have already
> looked at what the PHY can do when the PHY is attached, and eliminated
> any link modes that would cause an invalid configuration (provided
> phylink knows what the PHY is capable of.)
>
> However, that assumes phylink knows what the details are of the PCS,
> which is dependent on the .pcs_validate method being implemented.
>
Yes, agree that pcs_validate() is necessary to be implemented and
Phylink will validate the PHY when the PHY is attached. I will implement
this method in the next update. Thanks for pointing to the details here.
We will also remove the debug error print for the 'default' case.
However, I would like to retain the switch statement since we have
different routines for SGMII/USXGMII modes, and we plan to add more
interfaces modes later when we enhance the driver for other IPQ SoC.
Powered by blists - more mailing lists