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: <jlcyqvcw5hsjtmvf77sm7qni6tzihxudo6baoq7wi7zgqx3lvs@ykbsxgmjkvnf> Date: Tue, 19 Dec 2023 18:48:09 +0300 From: Serge Semin <fancer.lancer@...il.com> To: Vladimir Oltean <olteanv@...il.com>, Andrew Lunn <andrew@...n.ch>, Russell King <linux@...linux.org.uk> Cc: Heiner Kallweit <hkallweit1@...il.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, Jose Abreu <Jose.Abreu@...opsys.com>, Maxime Chevallier <maxime.chevallier@...tlin.com>, Tomer Maimon <tmaimon77@...il.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, openbmc@...ts.ozlabs.org, netdev@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH net-next 06/16] net: pcs: xpcs: Avoid creating dummy XPCS MDIO device Hi Vladimir, Russell, Andrew On Wed, Dec 13, 2023 at 06:27:57PM +0300, Serge Semin wrote: > Hi Vladimir, > > On Tue, Dec 05, 2023 at 01:52:34PM +0200, Vladimir Oltean wrote: > > On Tue, Dec 05, 2023 at 01:35:27PM +0300, Serge Semin wrote: > > > If the DW XPCS MDIO devices are either left unmasked for being auto-probed > > > or explicitly registered in the MDIO subsystem by means of the > > > mdiobus_register_board_info() method > > > > > mdiobus_register_board_info() has exactly one caller, and that is > > dsa_loop. I don't understand the relevance of it w.r.t. Synopsys XPCS. > > I'm reading the patches in order from the beginning. > > Well, one user of the DW XPCS driver is updated in this series in the > framework of the patch: > [PATCH net-next 13/16] net: stmmac: intel: Register generic MDIO device > https://lore.kernel.org/netdev/20231205103559.9605-14-fancer.lancer@gmail.com/ > > I can convert of them (it's sja1105 and wangxun txgbe) and then just > drop the MDIO-device creation part from xpcs_create_mdiodev(). As I > also described in another emails thread below this patch I used to > think that unmasking non-PHY device is also appropriate to get the > MDIO-device instance. I was wrong in that matter obviously. > > Anyway I just realized that my solution of using > mdiobus_register_board_info() is a bit clumsy. Moreover the patch 13 > (see the link above) shouldn't have the mdio_board_info instance > allocation (it can be defined on stack) and most importantly is wrong > in using the device-managed resources for it. The problem is that > mdiobus_register_board_info() registers an MDIO-device once for entire > system lifetime. It isn't that suitable for the hot-swappable devices > and for drivers bind/unbind cases. Since there is no > mdio_board_info-deregistration method, at the simplest case the no > longer used board-info descriptors might be left registered if a > device or driver are unloaded. That's why the device-managed > allocation is harmful in such scenario. At the very least I'll need to > convert the allocations to being non-managed. > > > > > > there is no point in creating the dummy MDIO device instance in order > > > > > Why dummy? There's nothing dummy about the mdio_device. It's how the PCS > > code accesses the hardware. > > I call it 'dummy' because no actual device is registered (though > 'redundant' or similar definition might sound more appropriate). The > entire structure is used as a communication layer between the XPCS > driver and MDIO device, where the device address is the only info > needed. Basically nothing prevents us from converting the current DW > XPCS driver to using the mdiobus_c45_read()/mdiobus_c45_write() > methods. Though in that case I wouldn't be able to easily add the > fwnode-based MDIO-devices support. > > > > > > to get the DW XPCS handler since the MDIO core subsystem will create > > > the device during the MDIO bus registration procedure. > > > > > It won't, though? Unless someone is using mdiobus_register_board_info() > > possibly, but who does that? > > As I said above I wrongly assumed that unmasking non-PHY device was > ok. But mdiobus_register_board_info() could be used for that as I (a > bit clumsy) demonstrated in the patch 13. > > > > > > All what needs to be done is to just reuse the MDIO-device instance > > > available in the mii_bus.mdio_map array (using some getter for it > > > would look better though). It shall prevent the XPCS devices been > > > accessed over several MDIO-device instances. > > > > > > Note since the MDIO-device instance might be retrieved from the MDIO-bus > > > map array its reference counter shall be increased. If the MDIO-device > > > instance is created in the xpcs_create_mdiodev() method its reference > > > counter will be already increased. So there is no point in toggling the > > > reference counter in the xpcs_create() function. Just drop it from there. > > > > > > Signed-off-by: Serge Semin <fancer.lancer@...il.com> > > > --- > > > > > Sorry, because the commit log lost me at the "context presentation" stage, > > I failed to understand the "what"s and the "why"s. > > > > Are you basically trying to add xpcs support on top of an mdio_device > > where the mdio_device_create() call was made externally to the xpcs code, > > through mdiobus_register_board_info() and mdiobus_setup_mdiodev_from_board_info()? > > Basically yes, but there is more of it. The main idea is to convert > the XPCS driver to using the already created non-PHY MDIO-devices > instead of manually creating a 'dummy'/'redundant' one. From my point > of view there are several reasons of doing so: > > 1. mdiobus_register_board_info() provides a way to assign the device > platform data to being registered afterwards device. Thus we can pass > some custom data to the XPCS-device driver (whether it's just an > xpcs_create_*() call or a fully functional MDIO-device driver > registered by the mdio_driver_register() method). For instance it can > be utilized to drop the fake PHYSIDs implementation from > drivers/net/dsa/sja1105/sja1105_mdio.c . > > 2. The MDIO-devices actually registered on the MDIO-bus will be > visible in sysfs with for instance useful IO statistics provided by > the MDIO-bus. Potentially (if it is required) at some point we'll be > able to convert the DW XPCS driver to being true MDIO-device driver > (bindable to the DW XPCS device) with less efforts. > > 3. Having an MDIO-device registered that way would make the DW XPCS > IO-device implementation unified after the fwnode-based XPCS > descriptor creation support is added in one of the subsequent patches. > > So based on the listed above I've got a question. Do you think all of > that is worth to be implemented? Andrew, Russell? > > I am asking because the patchset advance depends on your answers. If > you do I'll need to fix the problem described in my first message, > implement some new mdiobus_register_board_info()-like but > MDIO-bus-specific interface function (so MDIO-device infos would be > attached to the allocated MDIO-bus and then used to register the > respective MDIO-devices on the MDIO-bus registration), then convert > the sja1105 and wangxun txgbe drivers to using it. If you don't I'll > get back the xpcs_create_mdiodev() implementation and just provide a > fwnode-based version of one. Folks, this is the only issue left to settle so I could move on with the series fixing up. So the question is: taking my comment above into account is it worth to convert the xpcs_create_mdiodev() method to re-using the already registered MDIO-device instance instead of always creating a stub-like MDIO-device? -Serge(y) > > Note we already settled that converting DW XPCS driver to being normal > MDIO-device driver is prone to errors at this stage due to a > possibility to have the driver unbindable from user-space. I'll just > move the DT-compatibles check to the xpcs_create_fwnode() method and > drop the rest of the MDIO-device-driver-specific things. > > -Serge(y)
Powered by blists - more mailing lists