| 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: <bab2a500-f507-4ff6-bf69-753718a5f9a5@quicinc.com>
Date: Thu, 6 Mar 2025 17:12:52 +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
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