| 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: <7ff1cb04-fc3b-853e-ea8f-0c0b62bbd8c9@seco.com>
Date: Tue, 19 Jul 2022 15:41:03 -0400
From: Sean Anderson <sean.anderson@...o.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
Jakub Kicinski <kuba@...nel.org>,
Madalin Bucur <madalin.bucur@....com>,
"David S . Miller" <davem@...emloft.net>,
Paolo Abeni <pabeni@...hat.com>,
Ioana Ciornei <ioana.ciornei@....com>,
linux-kernel@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
Andrew Lunn <andrew@...n.ch>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Claudiu Manoil <claudiu.manoil@....com>,
Florian Fainelli <f.fainelli@...il.com>,
UNGLinuxDriver@...rochip.com,
Vivien Didelot <vivien.didelot@...il.com>,
Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [RFC PATCH net-next 5/9] net: pcs: lynx: Use pcs_get_by_provider
to get PCS
On 7/19/22 1:26 PM, Vladimir Oltean wrote:
> On Mon, Jul 11, 2022 at 12:05:15PM -0400, Sean Anderson wrote:
>> There is a common flow in several drivers where a lynx PCS is created
>> without a corresponding firmware node. Consolidate these into one helper
>> function. Because we control when the mdiodev is registered, we can add
>> a custom match function which will automatically bind our driver
>> (instead of using device_driver_attach).
>>
>> Signed-off-by: Sean Anderson <sean.anderson@...o.com>
>> ---
>>
>> drivers/net/dsa/ocelot/felix_vsc9959.c | 25 ++++---------------
>> drivers/net/dsa/ocelot/seville_vsc9953.c | 25 ++++---------------
>> .../net/ethernet/freescale/enetc/enetc_pf.c | 21 +++-------------
>> drivers/net/pcs/pcs-lynx.c | 24 ++++++++++++++++++
>> include/linux/pcs-lynx.h | 1 +
>> 5 files changed, 39 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
>> index 57634e2296c0..0a756c25d5e8 100644
>> --- a/drivers/net/dsa/ocelot/felix_vsc9959.c
>> +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
>> @@ -11,6 +11,7 @@
>> #include <net/tc_act/tc_gate.h>
>> #include <soc/mscc/ocelot.h>
>> #include <linux/dsa/ocelot.h>
>> +#include <linux/pcs.h>
>> #include <linux/pcs-lynx.h>
>> #include <net/pkt_sched.h>
>> #include <linux/iopoll.h>
>> @@ -1089,16 +1090,9 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
>> if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
>> continue;
>>
>> - mdio_device = mdio_device_create(felix->imdio, port);
>> - if (IS_ERR(mdio_device))
>> + phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, port);
>> + if (IS_ERR(phylink_pcs))
>> continue;
>> -
>> - phylink_pcs = lynx_pcs_create(mdio_device);
>> - if (IS_ERR(phylink_pcs)) {
>> - mdio_device_free(mdio_device);
>> - continue;
>> - }
>> -
>> felix->pcs[port] = phylink_pcs;
>>
>> dev_info(dev, "Found PCS at internal MDIO address %d\n", port);
>> @@ -1112,17 +1106,8 @@ static void vsc9959_mdio_bus_free(struct ocelot *ocelot)
>> struct felix *felix = ocelot_to_felix(ocelot);
>> int port;
>>
>> - for (port = 0; port < ocelot->num_phys_ports; port++) {
>> - struct phylink_pcs *phylink_pcs = felix->pcs[port];
>> - struct mdio_device *mdio_device;
>> -
>> - if (!phylink_pcs)
>> - continue;
>> -
>> - mdio_device = lynx_get_mdio_device(phylink_pcs);
>> - mdio_device_free(mdio_device);
>> - lynx_pcs_destroy(phylink_pcs);
>> - }
>> + for (port = 0; port < ocelot->num_phys_ports; port++)
>> + pcs_put(felix->pcs[port]);
>> mdiobus_unregister(felix->imdio);
>> mdiobus_free(felix->imdio);
>> }
>> diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c
>> index 8c52de5d0b02..9006dec85ef0 100644
>> --- a/drivers/net/dsa/ocelot/seville_vsc9953.c
>> +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c
>> @@ -9,6 +9,7 @@
>> #include <linux/mdio/mdio-mscc-miim.h>
>> #include <linux/of_mdio.h>
>> #include <linux/of_platform.h>
>> +#include <linux/pcs.h>
>> #include <linux/pcs-lynx.h>
>> #include <linux/dsa/ocelot.h>
>> #include <linux/iopoll.h>
>> @@ -1044,16 +1045,9 @@ static int vsc9953_mdio_bus_alloc(struct ocelot *ocelot)
>> if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_INTERNAL)
>> continue;
>>
>> - mdio_device = mdio_device_create(felix->imdio, addr);
>> - if (IS_ERR(mdio_device))
>> + phylink_pcs = lynx_pcs_create_on_bus(felix->imdio, addr);
>> + if (IS_ERR(phylink_pcs))
>> continue;
>> -
>> - phylink_pcs = lynx_pcs_create(mdio_device);
>> - if (IS_ERR(phylink_pcs)) {
>> - mdio_device_free(mdio_device);
>> - continue;
>> - }
>> -
>> felix->pcs[port] = phylink_pcs;
>>
>> dev_info(dev, "Found PCS at internal MDIO address %d\n", addr);
>> @@ -1067,17 +1061,8 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
>> struct felix *felix = ocelot_to_felix(ocelot);
>> int port;
>>
>> - for (port = 0; port < ocelot->num_phys_ports; port++) {
>> - struct phylink_pcs *phylink_pcs = felix->pcs[port];
>> - struct mdio_device *mdio_device;
>> -
>> - if (!phylink_pcs)
>> - continue;
>> -
>> - mdio_device = lynx_get_mdio_device(phylink_pcs);
>> - mdio_device_free(mdio_device);
>> - lynx_pcs_destroy(phylink_pcs);
>> - }
>> + for (port = 0; port < ocelot->num_phys_ports; port++)
>> + pcs_put(felix->pcs[port]);
>>
>> /* mdiobus_unregister and mdiobus_free handled by devres */
>> }
>> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
>> index 8c923a93da88..8da7c8644e44 100644
>> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
>> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
>> @@ -8,6 +8,7 @@
>> #include <linux/of_platform.h>
>> #include <linux/of_mdio.h>
>> #include <linux/of_net.h>
>> +#include <linux/pcs.h>
>> #include <linux/pcs-lynx.h>
>> #include "enetc_ierb.h"
>> #include "enetc_pf.h"
>> @@ -827,7 +828,6 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>> struct device *dev = &pf->si->pdev->dev;
>> struct enetc_mdio_priv *mdio_priv;
>> struct phylink_pcs *phylink_pcs;
>> - struct mdio_device *mdio_device;
>> struct mii_bus *bus;
>> int err;
>>
>> @@ -851,16 +851,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>> goto free_mdio_bus;
>> }
>>
>> - mdio_device = mdio_device_create(bus, 0);
>> - if (IS_ERR(mdio_device)) {
>> - err = PTR_ERR(mdio_device);
>> - dev_err(dev, "cannot create mdio device (%d)\n", err);
>> - goto unregister_mdiobus;
>> - }
>> -
>> - phylink_pcs = lynx_pcs_create(mdio_device);
>> + phylink_pcs = lynx_pcs_create_on_bus(bus, 0);
>> if (IS_ERR(phylink_pcs)) {
>> - mdio_device_free(mdio_device);
>> err = PTR_ERR(phylink_pcs);
>> dev_err(dev, "cannot create lynx pcs (%d)\n", err);
>> goto unregister_mdiobus;
>> @@ -880,13 +872,8 @@ static int enetc_imdio_create(struct enetc_pf *pf)
>>
>> static void enetc_imdio_remove(struct enetc_pf *pf)
>> {
>> - struct mdio_device *mdio_device;
>> -
>> - if (pf->pcs) {
>> - mdio_device = lynx_get_mdio_device(pf->pcs);
>> - mdio_device_free(mdio_device);
>> - lynx_pcs_destroy(pf->pcs);
>> - }
>> + if (pf->pcs)
>> + pcs_put(pf->pcs);
>> if (pf->imdio) {
>> mdiobus_unregister(pf->imdio);
>> mdiobus_free(pf->imdio);
>> diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
>> index 8272072698e4..adb9fd5ce72e 100644
>> --- a/drivers/net/pcs/pcs-lynx.c
>> +++ b/drivers/net/pcs/pcs-lynx.c
>> @@ -403,6 +403,30 @@ struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
>> }
>> EXPORT_SYMBOL(lynx_pcs_create);
>>
>> +struct phylink_pcs *lynx_pcs_create_on_bus(struct mii_bus *bus, int addr)
>> +{
>> + struct mdio_device *mdio;
>> + struct phylink_pcs *pcs;
>> + int err;
>> +
>> + mdio = mdio_device_create(bus, addr);
>> + if (IS_ERR(mdio))
>> + return ERR_CAST(mdio);
>> +
>> + mdio->bus_match = mdio_device_bus_match;
>> + strncpy(mdio->modalias, "lynx-pcs", sizeof(mdio->modalias));
>> + err = mdio_device_register(mdio);
>
> Yeah, so the reason why mdio_device_register() fails with -EBUSY for the
> PCS devices created by felix_vsc9959.c is this:
>
> int mdiobus_register_device(struct mdio_device *mdiodev)
> {
> int err;
>
> if (mdiodev->bus->mdio_map[mdiodev->addr])
> return -EBUSY;
>
> In other words, we already have an existing mdiodev on the bus at
> address mdiodev->addr. Funnily enough, that device is actually us.
> It was created at MDIO bus creation time, a dummy phydev that no one
> connects to, found by mdiobus_scan(). I knew this was taking place,
> but forgot/didn't realize the connection with this patch set, and that
> dummy phy_device was completely harmless until now.
>
> I can suppress its creation like this:
>
> From b1d1cd1625a27a62fd02598c7015b8ff0afdd28a Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@....com>
> Date: Tue, 19 Jul 2022 20:15:52 +0300
> Subject: [PATCH] net: dsa: ocelot: suppress PHY device scanning on the
> internal MDIO bus
>
> This bus contains Lynx PCS devices, and if the lynx-pcs driver ever
> decided to call mdio_device_register(), it would fail due to
> mdiobus_scan() having created a dummy phydev for the same address
> (the PCS responds to standard clause 22 PHY ID registers and can
> therefore by autodetected by phylib which thinks it's a PHY).
>
> On the Seville driver, things are a bit more complicated, since bus
> creation is handled by mscc_miim_setup() and that is shared with the
> dedicated mscc-miim driver. Suppress PHY scanning only for the Seville
> internal MDIO bus rather than for the whole mscc-miim driver, since we
> know that on NXP T1040, this bus only contains Lynx PCS devices.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> drivers/net/dsa/ocelot/felix_vsc9959.c | 4 ++++
> drivers/net/dsa/ocelot/seville_vsc9953.c | 6 +++++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
Thanks! I'll pick this up for v2.
--Sean
Powered by blists - more mailing lists