| 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: <aXsuRTZUUnw0kdzV@horms.kernel.org>
Date: Thu, 29 Jan 2026 09:54:13 +0000
From: Simon Horman <horms@...nel.org>
To: Vincent Guittot <vincent.guittot@...aro.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 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)
> +#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 !=.
> +
> + 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.
> +
> + 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.
...
> +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.
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().
> +
> + ret = s32g_serdes_init(serdes);
> +
> + return ret;
nit: This could be more succinctly written as:
return s32g_serdes_init(serdes);
> +}
...
Powered by blists - more mailing lists