| 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: <6a5dce2f-54cf-4517-b541-6941d22a25a1@quicinc.com>
Date: Mon, 17 Mar 2025 23:11:30 +0800
From: Lei Wei <quic_leiwei@...cinc.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
CC: Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, 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>,
<linux-arm-msm@...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>,
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC
Hi Russell,
Gentle reminder to review my responses and provide comments/suggestions.
Thank you.
On 3/6/2025 5:12 PM, Lei Wei wrote:
>
>
> On 2/28/2025 10:22 PM, Russell King (Oracle) wrote:
>> On Wed, Feb 19, 2025 at 06:46:57PM +0800, Lei Wei wrote:
>>>> 2) there's yet another open coded "_get" function for getting the
>>>> PCS given a DT node which is different from every other "_get"
>>>> function - this one checks the parent DT node has an appropriate
>>>> compatible whereas others don't. The whole poliferation of "_get"
>>>> methods that are specific to each PCS still needs solving, and I
>>>> still have the big question around what happens when the PCS driver
>>>> gets unbound - and whether that causes the kernel to oops. I'm also
>>>> not a fan of "look up the struct device and then get its driver data".
>>>> There is *no* locking over accessing the driver data.
>>>
>>> The PCS device in IPQ9574 chipset is built into the SoC chip and is not
>>> pluggable. Also, the PCS driver module is not unloadable until the MAC
>>> driver that depends on it is unloaded. Therefore, marking the driver
>>> '.suppress_bind_attrs = true' to disable user unbind action may be good
>>> enough to cover all possible scenarios of device going away for
>>> IPQ9574 PCS
>>> driver.
>>
>> What I am concerned about is the proliferation of these various PCS
>> specific "_get" methods. Where the PCS is looked up by firmware
>> reference, we should have a common way to do that, rather than all
>> these PCS specific ways.
>>
>> I did start work on that, but I just haven't had the time to take it
>> forward. This is about as far as I'd got:
>>
>> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
>> index 4f7920618b90..0b670fee0757 100644
>> --- a/drivers/net/pcs/Makefile
>> +++ b/drivers/net/pcs/Makefile
>> @@ -1,6 +1,8 @@
>> # SPDX-License-Identifier: GPL-2.0
>> # Makefile for Linux PCS drivers
>> +obj-$(CONFIG_PHYLINK) += pcs-core.o
>> +
>> pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
>> pcs-xpcs-nxp.o pcs-xpcs-wx.o
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 976e569feb70..1c5492dab00e 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -2483,6 +2483,15 @@ void phylink_pcs_change(struct phylink_pcs
>> *pcs, bool up)
>> }
>> EXPORT_SYMBOL_GPL(phylink_pcs_change);
>> +/**
>> + * phylink_pcs_remove() - notify phylink that a PCS is going away
>> + * @pcs: PCS that is going away
>> + */
>> +void phylink_pcs_remove(struct phylink_pcs *pcs)
>> +{
>> +
>> +}
>> +
>> static irqreturn_t phylink_link_handler(int irq, void *data)
>> {
>> struct phylink *pl = data;
>> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
>> index 071ed4683c8c..1e6b7ce0fa7a 100644
>> --- a/include/linux/phylink.h
>> +++ b/include/linux/phylink.h
>> @@ -1,6 +1,7 @@
>> #ifndef NETDEV_PCS_H
>> #define NETDEV_PCS_H
>> +#include <linux/list.h>
>> #include <linux/phy.h>
>> #include <linux/spinlock.h>
>> #include <linux/workqueue.h>
>> @@ -435,9 +436,11 @@ int mac_enable_tx_lpi(struct phylink_config
>> *config, u32 timer,
>> #endif
>> struct phylink_pcs_ops;
>> +struct pcs_lookup;
>> /**
>> * struct phylink_pcs - PHYLINK PCS instance
>> + * @lookup: private member for PCS core management
>> * @supported_interfaces: describing which PHY_INTERFACE_MODE_xxx
>> * are supported by this PCS.
>> * @ops: a pointer to the &struct phylink_pcs_ops structure
>> @@ -455,6 +458,7 @@ struct phylink_pcs_ops;
>> * the PCS driver.
>> */
>> struct phylink_pcs {
>> + struct pcs_lookup *lookup;
>> DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
>> const struct phylink_pcs_ops *ops;
>> struct phylink *phylink;
>> @@ -692,6 +696,7 @@ int phylink_set_fixed_link(struct phylink *,
>> void phylink_mac_change(struct phylink *, bool up);
>> void phylink_pcs_change(struct phylink_pcs *, bool up);
>> +void phylink_pcs_remove(struct phylink_pcs *);
>> int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs);
>> @@ -790,4 +795,11 @@ void phylink_mii_c45_pcs_get_state(struct
>> mdio_device *pcs,
>> void phylink_decode_usxgmii_word(struct phylink_link_state *state,
>> uint16_t lpa);
>> +
>> +/* PCS lookup */
>> +struct phylink_pcs *pcs_find(void *id);
>> +void pcs_remove(struct phylink_pcs *pcs);
>> +int pcs_add(struct phylink_pcs *pcs, void *id);
>> +int devm_pcs_add(struct device *dev, struct phylink_pcs *pcs, void *id);
>> +
>> #endif
>>
>> The idea is that you add the device using whatever identifier you decide
>> (the pointer value is what's matched). For example, a fwnode. You can
>> then find it using pcs_find().
>>
>> If it returns NULL, then it's not (yet) registered - if you know that it
>> should exist (e.g. because the fwnode is marked as available) then you
>> can return -EPROBE_DEFER or fail.
>>
>> There is a hook present so phylink can do something on PCS removal -
>> that's still to be implemented with this. I envision keeping a list
>> of phylink instances, and walking that list to discover if any phylink
>> instances are currently using the PCS. If they are, then we can take
>> the link down.
>>
>
> Thanks for sharing the details about this, the approach looks correct.
>
> Can you suggest whether we can go ahead with the current version of the
> IPQ PCS driver, and update the driver later to use the common way, once
> the infrastructure method is supported? Else (preferably) if the patch
> for your change can be posted, I can modify the IPQ PCS driver patch to
> use the common method and rebase on top of your patch. Please suggest.
>
>>> I would like to clarify on the hardware supported configurations for the
>>> UNIPHY PCS hardware instances. [Note: There are three instances of
>>> 'UNIPHY
>>> PCS' in IPQ9574. However we take the example here for PCS0]
>>>
>>> UNIPHY PCS0 --> pcs0_mii0..pcs0_mii4 (5 PCS MII channels maximum).
>>> Possible combinations: QSGMII (4x 1 SGMII)
>>> PSGMII (5 x 1 SGMII),
>>> SGMII (1 x 1 SGMII)
>>> USXGMII (1 x 1 USXGMII)
>>>
>>> As we can see above, different PCS channels in a 'UNIPHY' PCS block
>>> working
>>> in different PHY interface modes is not supported by the hardware.
>>> So, it
>>> might not be necessary to detect that conflict. If the interface mode
>>> changes from one to another, the same interface mode is applicable to
>>> all
>>> the PCS channels that are associated with the UNIPHY PCS block.
>>>
>>> Below is an example of a DTS configuration which depicts one board
>>> configuration where one 'UNIPHY' (PCS0) is connected with a QCA8075 Quad
>>> PHY, it has 4 MII channels enabled and connected with 4 PPE MAC
>>> ports, and
>>> all the PCS MII channels are in QSGMII mode. For the 'UNIPHY'
>>> connected with
>>> single SGMII or USXGMII PHY (PCS1), only one MII channel is enabled and
>>> connected with one PPE MAC port.
>>>
>>> PHY:
>>> &mdio {
>>> ethernet-phy-package@0 {
>>> compatible = "qcom,qca8075-package";
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> reg = <0x10>;
>>> qcom,package-mode = "qsgmii";
>>>
>>> phy0: ethernet-phy@10 {
>>> reg = <0x10>;
>>> };
>>>
>>> phy1: ethernet-phy@11 {
>>> reg = <0x11>;
>>> };
>>>
>>> phy2: ethernet-phy@12 {
>>> reg = <0x12>;
>>> };
>>>
>>> phy3: ethernet-phy@13 {
>>> reg = <0x13>;
>>> };
>>> };
>>> phy4: ethernet-phy@8 {
>>> compatible ="ethernet-phy-ieee802.3-c45";
>>> reg = <8>;
>>> };
>>> }
>>>
>>> PCS:
>>> pcs0: ethernet-pcs@...0000 {
>>> ......
>>> pcs0_mii0: pcs-mii@0 {
>>> reg = <0>;
>>> status = "enabled";
>>> };
>>>
>>> ......
>>>
>>> pcs0_mii3: pcs-mii@3 {
>>> reg = <3>;
>>> status = "enabled";
>>> };
>>> };
>>
>> Given that this is a package of several PCS which have a global mode, I
>> think it would be a good idea to have a property like
>> "qcom,package-mode" which defines which of the four modes should be used
>> for all PCS.
>>
>> Then the PCS driver initialises supported_interfaces for each of these
>> PCS to only contain that mode, thereby ensuring that unsupported
>> dissimilar modes can't be selected or the mode unexpectedly changed.
>>
>
> OK, I will add the "qcom,package-mode" property to restrict the
> supported_interfaces for each of the MII PCS instances.
Powered by blists - more mailing lists