| 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: <CAKfTPtB8xDB8qYUZwGnH+h94ww3+gSyOuZ+RL+zOh5tYGuu_HQ@mail.gmail.com>
Date: Thu, 29 Jan 2026 14:24:15 +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 3/4] phy: s32g: Add serdes xpcs subsystem
On Thu, 29 Jan 2026 at 12:59, Simon Horman <horms@...nel.org> wrote:
>
> On Mon, Jan 26, 2026 at 10:21:58AM +0100, Vincent Guittot wrote:
>
> ...
>
> > diff --git a/drivers/phy/freescale/phy-nxp-s32g-serdes.c b/drivers/phy/freescale/phy-nxp-s32g-serdes.c
>
> ...
>
> > +static void s32g_serdes_prepare_pma_mode5(struct s32g_serdes *serdes)
> > +{
> > + u32 val;
> > + /* Configure TX_VBOOST_LVL and TX_TERM_CTRL */
> > + val = readl(serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
> > + val &= ~(EXT_TX_VBOOST_LVL_MASK | EXT_TX_TERM_CTRL_MASK);
> > + val |= FIELD_PREP(EXT_TX_VBOOST_LVL_MASK, 0x3) |
> > + FIELD_PREP(EXT_TX_TERM_CTRL_MASK, 0x4);
> > + writel(val, serdes->ctrl.ss_base + S32G_PCIE_PHY_EXT_MISC_CTRL_2);
>
> 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].
>
> The entire s32g_serdes_prepare_pma_mode5() function is ~70 lines of
> magic numbers with zero explanation. These appear to be
> hardware-specific PLL/PHY tuning parameters for 2.5G mode.
Unfortunately there is no additional information in the reference
manual other than
*step 4:
- Write 3h to EXT_TX_VBOOST_LVL.
- Write 4h to EXT_TX_TERM_CTRL
>
> Please consider using #defines, to give values names.
>
> ...
>
> > +static int s32g_serdes_init_clks(struct s32g_serdes *serdes)
> > +{
> > + struct s32g_serdes_ctrl *ctrl = &serdes->ctrl;
> > + struct s32g_xpcs_ctrl *xpcs = &serdes->xpcs;
> > + int ret, order[2], xpcs_id;
> > + size_t i;
> > +
> > + switch (ctrl->ss_mode) {
> > + case 0:
> > + return 0;
> > + case 1:
> > + order[0] = 0;
> > + order[1] = XPCS_DISABLED;
> > + break;
> > + case 2:
> > + case 5:
> > + order[0] = 1;
> > + order[1] = XPCS_DISABLED;
> > + break;
> > + case 3:
> > + order[0] = 1;
> > + order[1] = 0;
> > + break;
> > + case 4:
> > + order[0] = 0;
> > + order[1] = 1;
> > + break;
> > + default:
> > + return -EINVAL;
>
> AI review also flags that s32g_serdes_get_ctrl_resources() ensures that
> ss_mode is <= 5. So this check is unnecessary.
okay but providing a default seems a good practice
>
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(order); i++) {
> > + xpcs_id = order[i];
> > +
> > + if (xpcs_id == XPCS_DISABLED)
> > + continue;
> > +
> > + ret = s32g_xpcs_init_plls(xpcs->phys[xpcs_id]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + if (ctrl->ss_mode == 5) {
> > + s32g_serdes_prepare_pma_mode5(serdes);
> > +
> > + ret = s32g_xpcs_pre_pcie_2g5(xpcs->phys[1]);
>
> Also from the AI review:
>
> In mode 5, code directly accesses xpcs->phys[1] without checking if
> it was created. If XPCS1 wasn't created for some reason (DT
> misconfiguration, probe failure), this NULL dereferences.
I will add a check
>
> ...
>
> > @@ -460,6 +741,41 @@ static int s32g2_serdes_create_phy(struct s32g_serdes *serdes, struct device_nod
>
> ...
>
> > + xpcs = devm_kmalloc(dev, sizeof(*xpcs), GFP_KERNEL);
> > + if (IS_ERR(xpcs)) {
> > + dev_err(dev, "Failed to allocate xpcs\n");
> > + return -ENOMEM;
> > + }
>
> AI review also flags:
>
> devm_kmalloc() returns NULL on failure, not an error pointer.
> This check will never trigger.
okay
>
> [1] https://netdev-ai.bots.linux.dev/ai-local.html
>
> > +
> > + xpcs_ctrl->phys[port] = xpcs;
> > +
> > + xpcs->an = of_property_read_bool(dev->of_node, "nxp,xpcs_an");
>
> AI review flags that nxp,xpcs_an is not part of the binding.
That's a leftover that should be removed
>
> ...
>
> > +struct phylink_pcs *s32g_serdes_pcs_create(struct device *dev, struct device_node *np)
> > +{
> > + struct platform_device *pdev;
> > + struct device_node *pcs_np;
> > + struct s32g_serdes *serdes;
> > + u32 port;
> > +
> > + if (of_property_read_u32(np, "reg", &port))
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (port > S32G_SERDES_XPCS_MAX)
> > + return ERR_PTR(-EINVAL);
> > +
> > + /* The PCS pdev is attached to the parent node */
> > + pcs_np = of_get_parent(np);
> > + if (!pcs_np)
> > + return ERR_PTR(-ENODEV);
> > +
> > + if (!of_device_is_available(pcs_np)) {
> > + of_node_put(pcs_np);
> > + return ERR_PTR(-ENODEV);
> > + }
> > +
> > + pdev = of_find_device_by_node(pcs_np);
> > + of_node_put(pcs_np);
> > + if (!pdev || !platform_get_drvdata(pdev)) {
> > + if (pdev)
> > + put_device(&pdev->dev);
> > + return ERR_PTR(-EPROBE_DEFER);
> > + }
> > +
> > + serdes = platform_get_drvdata(pdev);
>
> Also from the AI review:
>
> On success, the function gets a reference to pdev but never
> releases it. This leaks a device reference every time a MAC driver
> calls s32g_serdes_pcs_create().
will fix it
>
> > +
> > + return &serdes->xpcs.phys[port]->pcs;
>
> Also from the AI review:
>
> The check port > S32G_SERDES_XPCS_MAX allows port=2,
> but array only has indices 0 and 1.
>
> Also I'm not seeing bounds checking on port in s32g2_serdes_create_phy
> which uses port as an index for the same array both directly and indirectly
> via s32g_serdes_xpcs_init().
will add it
>
> ...
>
> > diff --git a/drivers/phy/freescale/phy-nxp-s32g-xpcs.c b/drivers/phy/freescale/phy-nxp-s32g-xpcs.c
>
> ...
>
> > +static int s32g_xpcs_regmap_reg_read(void *context, unsigned int reg,
> > + unsigned int *result)
> > +{
> > + struct s32g_xpcs *xpcs = context;
> > + u16 ofsleft = (reg >> 8) & 0xffffU;
> > + u16 ofsright = (reg & 0xffU);
> > +
> > + writew(ofsleft, xpcs->base + ADDR1_OFS);
> > + *result = readw(xpcs->base + (ofsright * 4));
> > +
> > + return 0;
> > +}
> > +
> > +static int s32g_xpcs_regmap_reg_write(void *context, unsigned int reg,
> > + unsigned int val)
> > +{
> > + struct s32g_xpcs *xpcs = context;
> > + u16 ofsleft = (reg >> 8) & 0xffffU;
> > + u16 ofsright = (reg & 0xffU);
> > +
> > + writew(ofsleft, xpcs->base + ADDR1_OFS);
> > + writew(val, xpcs->base + (ofsright * 4));
> > +
> > + return 0;
> > +}
> > +
> > +static const struct regmap_config s32g_xpcs_regmap_config = {
> > + .reg_bits = 16,
> > + .val_bits = 16,
> > + .reg_read = s32g_xpcs_regmap_reg_read,
> > + .reg_write = s32g_xpcs_regmap_reg_write,
> > + .wr_table = &s32g_xpcs_wr_table,
> > + .rd_table = &s32g_xpcs_rd_table,
> > + .max_register = 0x1F80E1,
> > +};
>
> AI review also flags that s32g_xpcs_regmap_reg_read and
> s32g_xpcs_regmap_reg_write do not protect against concurrent access.
but regmap framework should
>
> ...
Powered by blists - more mailing lists