linux-kernel - Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if configured with rgmii-id

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: <2bd19e9e-775d-41b0-99d4-accb9ae8262d@kernel.org>
Date: Wed, 22 Jan 2025 10:48:57 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Yijie Yang <quic_yijiyang@...cinc.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "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>, Vinod Koul <vkoul@...nel.org>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>,
 Richard Cochran <richardcochran@...il.com>
Cc: netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
 linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/4] net: stmmac: dwmac-qcom-ethqos: Mask PHY mode if
 configured with rgmii-id

On 22/01/2025 09:56, Yijie Yang wrote:
> 
> 
> On 2025-01-21 20:47, Krzysztof Kozlowski wrote:
>> On 21/01/2025 08:54, Yijie Yang wrote:
>>> The Qualcomm board always chooses the MAC to provide the delay instead of
>>> the PHY, which is completely opposite to the suggestion of the Linux
>>> kernel.
>>
>>
>> How does the Linux kernel suggest it?
>>
>>> The usage of phy-mode in legacy DTS was also incorrect. Change the
>>> phy_mode passed from the DTS to the driver from PHY_INTERFACE_MODE_RGMII_ID
>>> to PHY_INTERFACE_MODE_RGMII to ensure correct operation and adherence to
>>> the definition.
>>> To address the ABI compatibility issue between the kernel and DTS caused by
>>> this change, handle the compatible string 'qcom,qcs404-evb-4000' in the
>>> code, as it is the only legacy board that mistakenly uses the 'rgmii'
>>> phy-mode.
>>>
>>> Signed-off-by: Yijie Yang <quic_yijiyang@...cinc.com>
>>> ---
>>>   .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 18 +++++++++++++-----
>>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>> index 2a5b38723635b5ef9233ca4709e99dd5ddf06b77..e228a62723e221d58d8c4f104109e0dcf682d06d 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>>> @@ -401,14 +401,11 @@ static int ethqos_dll_configure(struct qcom_ethqos *ethqos)
>>>   static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>>>   {
>>>   	struct device *dev = &ethqos->pdev->dev;
>>> -	int phase_shift;
>>> +	int phase_shift = 0;
>>>   	int loopback;
>>>   
>>>   	/* Determine if the PHY adds a 2 ns TX delay or the MAC handles it */
>>> -	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>>> -	    ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
>>> -		phase_shift = 0;
>>> -	else
>>> +	if (ethqos->phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
>>>   		phase_shift = RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN;
>>>   
>>>   	/* Disable loopback mode */
>>> @@ -810,6 +807,17 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>>   	ret = of_get_phy_mode(np, &ethqos->phy_mode);
>>>   	if (ret)
>>>   		return dev_err_probe(dev, ret, "Failed to get phy mode\n");
>>> +
>>> +	root = of_find_node_by_path("/");
>>> +	if (root && of_device_is_compatible(root, "qcom,qcs404-evb-4000"))
>>
>>
>> First, just check if machine is compatible, don't open code it.
>>
>> Second, drivers should really, really not rely on the machine. I don't
>> think how this resolves ABI break for other users at all.
> 
> As detailed in the commit description, some boards mistakenly use the 
> 'rgmii' phy-mode, and the MAC driver has also incorrectly parsed and 

That's a kind of an ABI now, assuming it worked fine.

> added the delay for it. This code aims to prevent breaking these boards 
> while correcting the erroneous parsing. This issue is similar to the one 
> discussed in another thread:
> https://lore.kernel.org/all/20241225-support_10m100m-v1-2-4b52ef48b488@quicinc.com/
> 
>>
>> You need to check what the ABI is here and do not break it.
> 
> If board compatible string matching is not recommended, can I address 

And what if affected board does not match the compatible, because you
did not include it here? I said this code does not prevent breaking ABI
and you did not address it. To my comment you need to consider ABI, you
repeat the same, so kind of something irrelevant. I have feelings you
want to push your code, just like in the past several of your
colleagues. Learning from past mistakes I will not engage in such
discussions, so please really rethink the concept and comments you
received here.

That's a NAK.

Best regards,
Krzysztof