| 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: <20210817094110.vfrrbt264trmn7ri@skbuf>
Date: Tue, 17 Aug 2021 12:41:10 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Colin Foster <colin.foster@...advantage.com>
Cc: andrew@...n.ch, vivien.didelot@...il.com, f.fainelli@...il.com,
davem@...emloft.net, kuba@...nel.org, robh+dt@...nel.org,
claudiu.manoil@....com, alexandre.belloni@...tlin.com,
UNGLinuxDriver@...rochip.com, hkallweit1@...il.com,
linux@...linux.org.uk, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Ioana Ciornei <ioana.ciornei@....com>
Subject: Re: [RFC PATCH v3 net-next 09/10] net: dsa: ocelot: felix: add
support for VSC75XX control over SPI
On Sun, Aug 15, 2021 at 01:41:49PM -0700, Colin Foster wrote:
> I also came across some curious code in Seville where it is callocing a
> struct phy_device * array instead of struct lynx_pcs *. I'm not sure if
> that's technically a bug or if the thought is "a pointer array is a
> pointer array."
git blame will show you that it is a harmless leftover of commit
588d05504d2d ("net: dsa: ocelot: use the Lynx PCS helpers in Felix and
Seville"). Before that patch, the pcs was a struct phy_device.
> @@ -1062,12 +1062,12 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot)
> int port;
>
> for (port = 0; port < ocelot->num_phys_ports; port++) {
> - struct lynx_pcs *pcs = felix->pcs[port];
> + struct phylink_pcs *pcs = felix->pcs[port];
>
> if (!pcs)
> continue;
>
> - mdio_device_free(pcs->mdio);
> + mdio_device_free(lynx_pcs_get_mdio(pcs));
Don't really have a better suggestion than lynx_pcs_get_mdio.
> lynx_pcs_destroy(pcs);
> }
> felix_mdio_bus_free(ocelot);
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> index ccaf7e35abeb..484f0d4efefe 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> @@ -270,10 +270,11 @@ static int dpaa2_pcs_create(struct dpaa2_mac *mac,
>
> static void dpaa2_pcs_destroy(struct dpaa2_mac *mac)
> {
> - struct lynx_pcs *pcs = mac->pcs;
> + struct phylink_pcs *pcs = mac->pcs;
>
> if (pcs) {
> - struct device *dev = &pcs->mdio->dev;
> + struct mdio_device *mdio = lynx_get_mdio_device(pcs);
> + struct device *dev = &mdio->dev;
> lynx_pcs_destroy(pcs);
> put_device(dev);
Ideally dpaa2 would call mdio_device_free too, just like the others.
> mac->pcs = NULL;
> @@ -336,7 +337,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
> mac->phylink = phylink;
>
> if (mac->pcs)
> - phylink_set_pcs(mac->phylink, &mac->pcs->pcs);
> + phylink_set_pcs(mac->phylink, mac->pcs);
>
> err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0);
> if (err) {
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> index 31274325159a..cc2ca51ac984 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c
> @@ -823,7 +823,7 @@ static int enetc_imdio_create(struct enetc_pf *pf)
> {
> struct device *dev = &pf->si->pdev->dev;
> struct enetc_mdio_priv *mdio_priv;
> - struct lynx_pcs *pcs_lynx;
> + struct phylink_pcs *pcs_phylink;
> struct mdio_device *pcs;
Agree with Russell's suggestion to replace "pcs" with "mdiodev" wherever
it refers to a struct mdio_device. Likely as a separate patch.
> struct mii_bus *bus;
> int err;
> @@ -341,13 +355,13 @@ struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio)
> lynx_pcs->pcs.ops = &lynx_pcs_phylink_ops;
> lynx_pcs->pcs.poll = true;
>
> - return lynx_pcs;
> + return lynx_to_phylink_pcs(lynx_pcs);
I would probably write another patch to convert all occurrences of
"struct lynx_pcs" variables to the same naming scheme. Currently we have
"lynx", "pcs", "lynx_pcs" only within the pcs-lynx.c file itself. "lynx"
seems to be the predominant name so all others could be replaced with
that too.
> }
> EXPORT_SYMBOL(lynx_pcs_create);
>
> -void lynx_pcs_destroy(struct lynx_pcs *pcs)
> +void lynx_pcs_destroy(struct phylink_pcs *pcs)
> {
> - kfree(pcs);
> + kfree(phylink_pcs_to_lynx(pcs));
I would perhaps do this in two stages
struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs);
kfree(lynx);
> }
> EXPORT_SYMBOL(lynx_pcs_destroy);
>
> diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h
> index a6440d6ebe95..5712cc2ce775 100644
> --- a/include/linux/pcs-lynx.h
> +++ b/include/linux/pcs-lynx.h
> @@ -9,13 +9,10 @@
> #include <linux/mdio.h>
> #include <linux/phylink.h>
>
> -struct lynx_pcs {
> - struct phylink_pcs pcs;
> - struct mdio_device *mdio;
> -};
Good that this structure is no longer exposed.
> +struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs);
>
> -struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio);
> +struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio);
>
> -void lynx_pcs_destroy(struct lynx_pcs *pcs);
> +void lynx_pcs_destroy(struct phylink_pcs *pcs);
We don't want the few phylink_pcs drivers going in different directions,
so we should modify pcs-xpcs.c too such that it no longer exposes struct
dw_xpcs to the outside world. I think I hid most of that away already,
and grepping for "xpcs->" in drivers/net/dsa and drivers/net/ethernet,
I only see xpcs->mdiodev and xpcs->pcs being accessed, so converting
khat should be a walk in the park.
Anyway, I would focus for now on getting the ocelot hardware to work and
writing the phylink_pcs driver for that. That is one part where I can't
help a lot with.
Powered by blists - more mailing lists