| 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: <CAKfTPtDfnpzq2CB-isVzvh1ZCWo7kit9KRJvVGoU1C3zZTgdXw@mail.gmail.com>
Date: Thu, 29 Jan 2026 14:01:13 +0100
From: Vincent Guittot <vincent.guittot@...aro.org>
To: Simon Horman <horms@...nel.org>
Cc: vkoul@...nel.org, neil.armstrong@...aro.org, krzk+dt@...nel.org,
conor+dt@...nel.org, ciprianmarian.costea@....nxp.com, s32@....com,
p.zabel@...gutronix.de, linux@...linux.org.uk, ghennadi.procopciuc@....com,
bogdan-gabriel.roman@....com, Ionut.Vicovan@....com,
alexandru-catalin.ionita@....com, linux-phy@...ts.infradead.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
Frank.li@....com
Subject: Re: [PATCH 2/4] phy: s32g: Add serdes subsystem phy
On Thu, 29 Jan 2026 at 10:54, Simon Horman <horms@...nel.org> wrote:
>
> On Mon, Jan 26, 2026 at 10:21:57AM +0100, Vincent Guittot wrote:
>
> ...
>
> > diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> > new file mode 100644
> > index 000000000000..8336c868c8dc
> > --- /dev/null
> > +++ b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
> > @@ -0,0 +1,569 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * SerDes driver for S32G SoCs
> > + *
> > + * Copyright 2021-2026 NXP
> > + */
> > +
> > +#include <dt-bindings/phy/phy.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
>
> Hi Vincent, all,
>
> I think that you also need:
>
> #include <linux/iopoll.h>
>
> So that read_poll_timeout() is declared.
> Else this patch causes a transient build failure
> (for x86_64 allmodconfig)
ok, i will add it
>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_address.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/processor.h>
> > +#include <linux/reset.h>
> > +#include <linux/units.h>
>
> ...
>
> > +static int s32g_serdes_phy_set_mode_ext(struct phy *p,
> > + enum phy_mode mode, int submode)
> > +{
> > + struct s32g_serdes *serdes = phy_get_drvdata(p);
> > +
> > + if (mode == PHY_MODE_PCIE)
> > + return -EINVAL;
>
> This is part of an AI Generated review.
> I have looked over it and I think it warrants investigation.
> For information on how to reproduce locally, as I did, please see [1].
>
> [1] https://netdev-ai.bots.linux.dev/ai-local.html
>
> This returns error if mode IS PHY_MODE_PCIE, but this is a PCIe PHY!
> This looks like a typo. Should be !=.
yes don't know what happened here but it should be !=
>
> > +
> > + if (!is_pcie_phy_mode_valid(submode))
> > + return -EINVAL;
> > +
> > + /*
> > + * Do not configure SRIS or CRSS PHY MODE in conjunction
> > + * with any SGMII mode on the same SerDes subsystem
> > + */
> > + if ((submode == CRSS || submode == SRIS) &&
> > + serdes->ctrl.ss_mode != 0)
> > + return -EINVAL;
> > +
> > + /*
> > + * Internal reference clock cannot be used with either Common clock
> > + * or Spread spectrum, leaving only SRNSS
> > + */
> > + if (submode != SRNS && !serdes->ctrl.ext_clk)
> > + return -EINVAL;
> > +
> > + serdes->pcie.phy_mode = submode;
>
> The AI review also suggested that it may be unsafe
> to set the submode after s32g_serdes_phy_power_on()
> has been called. And that there is nothing preventing that.
>
> TBH, I am unsure if either of those statements are true.
> But it seems worth validating with you.
yes, the usual pattern is :
- phy_set_mode_ext()
- then phy_power_on()
but I can add an additional check
>
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int s32g_serdes_get_ctrl_resources(struct platform_device *pdev, struct s32g_serdes *serdes)
> > +{
> > + struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> > + struct device *dev = &pdev->dev;
> > + int ret, idx;
> > +
> > + ret = of_property_read_u32(dev->of_node, "nxp,sys-mode",
> > + &ctrl->ss_mode);
> > + if (ret) {
> > + dev_err(dev, "Failed to get SerDes subsystem mode\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (ctrl->ss_mode > S32G_SERDES_MODE_MAX) {
> > + dev_err(dev, "Invalid SerDes subsystem mode %u\n",
> > + ctrl->ss_mode);
> > + return -EINVAL;
> > + }
> > +
> > + ctrl->ss_base = devm_platform_ioremap_resource_byname(pdev, "ss_pcie");
> > + if (IS_ERR(ctrl->ss_base)) {
> > + dev_err(dev, "Failed to map 'ss_pcie'\n");
> > + return PTR_ERR(ctrl->ss_base);
> > + }
> > +
> > + ctrl->rst = devm_reset_control_get(dev, "serdes");
> > + if (IS_ERR(ctrl->rst))
> > + return dev_err_probe(dev, PTR_ERR(ctrl->rst),
> > + "Failed to get 'serdes' reset control\n");
> > +
> > + ctrl->nclks = devm_clk_bulk_get_all(dev, &ctrl->clks);
> > + if (ctrl->nclks < 1)
> > + return dev_err_probe(dev, ctrl->nclks,
> > + "Failed to get SerDes clocks\n");
>
> If devm_clk_bulk_get_all returns 0 then this value will
> be passed to dev_err_probe(). And 0 will, in turn be returned by
> dev_err_probe() and this function. However, that will be treated
> as success by the caller, even though this is an error condition.
>
> Perhaps something like this is more appropriate if ctrl->nclks
> must be greater than 0. (Completely untested!)
>
> if (ctrl->nclks < 1) {
> ret = ctrl->nclks ? : -EINVAL;
> return dev_err_probe(dev, ret,
> "Failed to get SerDes clocks\n");
> }
>
> Flagged by Smatch.
okay
>
> ...
>
> > +static int s32g_serdes_parse_lanes(struct device *dev, struct s32g_serdes *serdes)
> > +{
> > + int ret;
> > +
> > + for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > + ret = s32g2_serdes_create_phy(serdes, of_port);
> > + if (ret)
> > + break;
> > + }
> > +
> > + return ret;
>
> Perhaps it cannot occur.
> But if the loop above iterates zero times,
> then ret will be used uninitialised here.
should not but will fix it
>
> Also flagged by Smatch.
>
> > +}
> > +
> > +static int s32g_serdes_probe(struct platform_device *pdev)
> > +{
> > + struct s32g_serdes *serdes;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + serdes = devm_kzalloc(dev, sizeof(*serdes), GFP_KERNEL);
> > + if (!serdes)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, serdes);
> > + serdes->dev = dev;
> > +
> > + ret = s32g_serdes_get_ctrl_resources(pdev, serdes);
> > + if (ret)
> > + return ret;
> > +
> > + ret = s32g_serdes_get_pcie_resources(pdev, serdes);
> > + if (ret)
> > + return ret;
> > +
> > + ret = s32g_serdes_parse_lanes(dev, serdes);
> > + if (ret)
> > + return ret;
>
> The I review also says:
>
> The probe function calls s32g_serdes_init() which enables clocks,
> configures hardware, and deasserts reset. However,
> s32g_serdes_parse_lanes() creates PHY providers via
> devm_of_phy_provider_register().
>
> Problem: PHY consumers can start calling PHY ops (like power_on) as soon
> as the provider is registered, but the hardware isn't initialized until
> s32g_serdes_init() runs afterward. This creates a race window.
>
> Recommendation: Move s32g_serdes_init() before s32g_serdes_parse_lanes().
I will look at this more deeply but part of s32g_serdes_init() needs
lanes to be parsed for configuring clock
>
> > +
> > + ret = s32g_serdes_init(serdes);
> > +
> > + return ret;
>
> nit: This could be more succinctly written as:
fair enough
>
> return s32g_serdes_init(serdes);
>
> > +}
>
> ...
Powered by blists - more mailing lists