| 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: <20251216091831.GG9275@google.com> Date: Tue, 16 Dec 2025 09:18:31 +0000 From: Lee Jones <lee@...nel.org> To: Vladimir Oltean <vladimir.oltean@....com> Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org Subject: Re: [PATCH net-next 07/15] mfd: core: add ability for cells to probe on a custom parent OF node > > Side note: The implementation is also janky. > > Yes, this is why it's up for review, so I can learn why it's janky and > fix it. I'd be happy to discuss this in great detail if we finally conclude that this device is suitable for MFD. Spoiler alert: Unless you add/convert more child devices that are outside of net/ and drivers/net AND move the core MFD usage to drivers/mfd/, then we can't conclude that. > > There does appear to be at least some level of misunderstanding between > > us. I'm not for one moment suggesting that a switch can't be an MFD. If > > it contains probe-able components that need to be split-up across > > multiple different subsystems, then by all means, move the core driver > > into drivers/mfd/ and register child devices 'till your heart's content. > > Are you still speaking generically here, or have you actually looked at > any "nxp,sja1105q" or "nxp,sja1110a" device trees to see what it would > mean for these compatible strings to be probed by a driver in drivers/mfd? It's not my role to go digging into existing implementations and previous submissions to prove whether a particular submission is suitable for inclusion into MFD. Please put in front of me, in a concise way (please), why you think this is fit for inclusion. I've explained what is usually required, but I'll (over-)simplify again for clarity: - The mfd_* API call-sites must only exist in drivers/mfd/ - Consumers usually spit out non-system specific logic into a 'core' - MFDs need to have more than one child - This is where the 'Multi' comes in - Children should straddle different sub-systems - drivers/net is not enough [0] - If all of your sub-devices are in 'net' use the platform_* API - <other stipulations less relevant to this stipulation> ... There will always be exceptions, but previous mistakes are not good justifications for future ones. [0] .../bindings/net/dsa/nxp,sja1105.yaml | 28 + .../bindings/net/pcs/snps,dw-xpcs.yaml | 8 + MAINTAINERS | 2 + drivers/mfd/mfd-core.c | 11 +- drivers/net/dsa/sja1105/Kconfig | 2 + drivers/net/dsa/sja1105/Makefile | 2 +- drivers/net/dsa/sja1105/sja1105.h | 42 +- drivers/net/dsa/sja1105/sja1105_main.c | 169 +++--- drivers/net/dsa/sja1105/sja1105_mdio.c | 507 ------------------ drivers/net/dsa/sja1105/sja1105_mfd.c | 293 ++++++++++ drivers/net/dsa/sja1105/sja1105_mfd.h | 11 + drivers/net/dsa/sja1105/sja1105_spi.c | 113 +++- drivers/net/mdio/Kconfig | 21 +- drivers/net/mdio/Makefile | 2 + drivers/net/mdio/mdio-regmap-simple.c | 77 +++ drivers/net/mdio/mdio-regmap.c | 7 +- drivers/net/mdio/mdio-sja1110-cbt1.c | 173 ++++++ drivers/net/pcs/pcs-xpcs-plat.c | 146 +++-- drivers/net/pcs/pcs-xpcs.c | 12 + drivers/net/phy/phylink.c | 75 ++- include/linux/mdio/mdio-regmap.h | 2 + include/linux/mfd/core.h | 7 + include/linux/pcs/pcs-xpcs.h | 1 + include/linux/phylink.h | 5 + 24 files changed, 1033 insertions(+), 683 deletions(-) delete mode 100644 drivers/net/dsa/sja1105/sja1105_mdio.c create mode 100644 drivers/net/dsa/sja1105/sja1105_mfd.c create mode 100644 drivers/net/dsa/sja1105/sja1105_mfd.h create mode 100644 drivers/net/mdio/mdio-regmap-simple.c create mode 100644 drivers/net/mdio/mdio-sja1110-cbt1.c > What OF node would remain for the DSA switch (child) device driver? The same? > Or are you suggesting that the entire drivers/net/dsa/sja1105/ would > move to drivers/mfd/? Or? See bullet 1.1 above. [...] > > I don't recall those discussions from 3 years ago, but the Ocelot > > platform, whatever it may be, seems to have quite a lot more > > cross-subsystem device support requirements going on than I see here: > > > > drivers/i2c/busses/i2c-designware-platdrv.c > > drivers/irqchip/irq-mscc-ocelot.c > > drivers/mfd/ocelot-* > > drivers/net/dsa/ocelot/* > > drivers/net/ethernet/mscc/ocelot* > > drivers/net/mdio/mdio-mscc-miim.c > > drivers/phy/mscc/phy-ocelot-serdes.c > > drivers/pinctrl/pinctrl-microchip-sgpio.c > > drivers/pinctrl/pinctrl-ocelot.c > > drivers/power/reset/ocelot-reset.c > > drivers/spi/spi-dw-mmio.c > > net/dsa/tag_ocelot_8021q.c > > This is a natural effect of Ocelot being "whatever it may be". It is a > family of networking SoCs, of which VSC7514 has a MIPS CPU and Linux > port, where the above drivers are used. The VSC7512 is then a simplified > variant with the MIPS CPU removed, and the internal components controlled > externally over SPI. Hence MFD to reuse the same drivers as Linux on > MIPS (using MMIO) did. This is all that matters, not the quantity. >From what I can see, Ocelot ticks all of the boxes for MFD API usage, whereas this submission does not. The fact that the overarching device provides a similar function is neither here nor there. These are the results from my searches of your device: git grep -i SJA1110 | grep -v 'net\|arch\|include' <no results> [...] > > My point is, you don't seem to have have any of that here. > > What do you want to see exactly which is not here? > > I have converted three classes of sub-devices on the NXP SJA1110 to MFD > children in this patch set. Two MDIO buses and an Ethernet PCS for SGMII. > > In the SJA1110 memory map, the important resources look something like this: > > Name Description Start End > SWITCH Ethernet Switch Subsystem 0x000000 0x3ffffc > 100BASE-T1 Internal MDIO bus for 100BASE-T1 PHY (port 5 - 10) 0x704000 0x704ffc > SGMII1 SGMII Port 1 0x705000 0x705ffc > SGMII2 SGMII Port 2 0x706000 0x706ffc > SGMII3 SGMII Port 3 0x707000 0x707ffc > SGMII4 SGMII Port 4 0x708000 0x708ffc > 100BASE-TX Internal MDIO bus for 100BASE-TX PHY 0x709000 0x709ffc All in drivers/net. > ACU Auxiliary Control Unit 0x711000 0x711ffc > GPIO General Purpose Input/Output 0x712000 0x712ffc Where are these drivers? > I need to remind you that my purpose here is not to add drivers in > breadth for all SJA1110 sub-devices now. You'll see from my discussions with Colin, sub-drivers (if they are to be used for MFD justification (point 3 above), then they must be added as part of the first submission. Perhaps this isn't an MFD, "yet"? [...] > The SGMII blocks are highly reusable IPs licensed from Synopsys, and > Linux already has DT bindings and a corresponding platform driver for > the case where their registers are viewed using MMIO. This is a good reason for dividing them up into subordinate platform devices. However, it is not a good use-case of MFD. In it's current guise, your best bet is to use the platform_* API directly. This is a well trodden path and it not challenging: % git grep platform_device_add -- arch drivers sound | wc -l 398 [...] > In my opinion I do not need to add handling for any other sub-device, > for the support to be more "cross-system" like for Ocelot. What is here > is enough for you to decide if this is adequate for MFD or not. Currently ... it's not. [...] Hopefully that helps to clarify my expectations a little. TL;DR, this looks like a good candidate for direct platform_* usage. -- Lee Jones [李琼斯]
Powered by blists - more mailing lists