[PATCH 2/2] phy: renesas: Add Renesas Multi-Protocol PHY driver for R-Car X5H
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Fri Aug 28 12:35:46 PDT 2026
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Regmap readable and writable ranges specify inverted address bounds and omit valid registers, preventing the driver from initializing hardware registers.
- [High] Runtime PM usage counter leaks when `mp_phy_init()` encounters an error or returns early.
- [High] Missing firmware size validation leads to out-of-bounds memory accesses and memory-mapped I/O corruption.
- [High] Inconsistent use of `phy->id` in PCIe initialization leads to incorrect port configuration and out-of-bounds register accesses.
- [High] `mp_phy_suspend()` clears `chan->initialized` without dropping PM domains, permanently bricking PHY channels after a suspend/resume cycle.
- [High] Unconditional register access in `mp_phy_resume()` for all channels without checking if their respective power domains are turned on.
- [Medium] Memory and resource leaks in `mp_phy_probe()` error paths: firmware buffer and attached power domains are not released on subsequent initialization failures.
- [Medium] Asynchronous `pm_runtime_put()` is immediately followed by `pm_runtime_disable()` during device removal, violating PM subsystem guidelines.
- [Medium] Missing bounds check on device tree index in `mp_phy_xlate()` leads to an out-of-bounds array access.
- [Medium] `reset_control_bulk_deassert()` iterates sequentially, violating the hardware requirement to deassert `mpphy02` last.
- [Medium] PHY provider is exposed to consumers before PHY instances are created and hardware is powered on.
--
commit 54820191afa1ae9e0aa531e1706a7cd7acbab1ca
Author: Thanh Quan <thanh.quan.xn at renesas.com>
Subject: phy: renesas: Add Renesas Multi-Protocol PHY driver for R-Car X5H
This commit introduces a new Renesas Multi-Protocol PHY driver for the R-Car
X5H SoC. It supports multiplexing five input IPs (PCIe4, USB 3.2, Ethernet)
to four separate IO blocks based on the device tree configuration.
> diff --git a/drivers/phy/renesas/phy-rcar-gen5-mpphy.c b/drivers/phy/renesas/phy-rcar-gen5-mpphy.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/renesas/phy-rcar-gen5-mpphy.c
[ ... ]
> +#define MPPHY_PX_RD_RANGE(n) \
> + regmap_reg_range(MPPHY_PXCNTXT1(n), MPPHY_CHAN_BASE(n)), \
> + regmap_reg_range(MPPHY_PXREFCLK(n), MPPHY_PXREFCLK(n)), \
> + regmap_reg_range(MPPHY_PXRXREQ1(n), MPPHY_PXRXREQ1(n)), \
> + regmap_reg_range(MPPHY_PXRXCNT(n), MPPHY_PXRXCNT(n)), \
> + regmap_reg_range(MPPHY_PXSRAMCNT(n), MPPHY_PXTXREQ(n))
[Severity: High]
Does this inverted range cause regmap to reject accesses?
MPPHY_PXCNTXT1(n) is defined as (MPPHY_CHAN_BASE(n) + 0x4).
Since range_min > range_max, regmap_reg_range will set a range where the
minimum is greater than the maximum, preventing the driver from reading
these hardware registers during initialization.
[ ... ]
> +static void mp_phy_update_firmware(struct phy *phy)
> +{
> + struct mp_phy_priv *priv = phy_get_drvdata(phy);
> + int i;
> +
> + for (i = 0; i < priv->fw->size; i += 2) {
> + writew(priv->fw->data[i] | (priv->fw->data[i + 1] << 8),
> + priv->base + MPPHY_FW_BASE + (MPPHY_FW_CH_OFFSET * phy->id) + i);
> + }
> +}
[Severity: High]
Does this code risk an out-of-bounds read or memory corruption if the
firmware size is odd or larger than expected?
Since mp_phy_parse_dt() doesn't validate priv->fw->size, a large firmware
file could write beyond the allocated MPPHY_FW_CH_OFFSET (0x20000) or outside
the I/O region completely. If the size is odd, reading priv->fw->data[i + 1]
will result in an out-of-bounds access.
[ ... ]
> +static int mp_phy_init_pcie4(struct phy *phy)
> +{
> + struct mp_phy_priv *priv = phy_get_drvdata(phy);
> +
> + if (priv->num_lanes[phy->id] == 1 || priv->num_lanes[phy->id] == 2) {
> + if (phy->id == 0) {
> + regmap_set_bits(priv->map, MPPHY_PXCNTXT1(0), 0x2010002);
> + regmap_set_bits(priv->map, MPPHY_PXCNTXT2(0), 0x2020201);
> + regmap_set_bits(priv->map, MPPHY_PXTXREQ(0), 0x80004);
> + regmap_set_bits(priv->map, MPPHY_PXREFCLK(0), 0x30);
> + regmap_set_bits(priv->map, MPPHY_PXREFCLK(0), 0x4);
> + regmap_set_bits(priv->map, MPPHY_PXREFCLK(0), 0x1);
> + regmap_set_bits(priv->map, MPPHY_PXRXCNT(0), 0x202);
> + regmap_clear_bits(priv->map, MPPHY_PXRXCNT(0), 0x202);
> + } else if (phy->id == 1) {
> + regmap_set_bits(priv->map, MPPHY_PXCNTXT1(2), 0x2010002);
[Severity: High]
Is phy->id used consistently here?
The code seems to treat phy->id as a controller index rather than a port index,
ignoring cases where phy->id is 2 or 3. This could lead to incorrect port
configuration if phy->id maps directly to the port being initialized.
[ ... ]
> +static int mp_phy_init(struct phy *phy)
> +{
> + struct mp_phy_priv *priv = phy_get_drvdata(phy);
> + struct mp_phy_chan_priv *chan = &priv->chan[phy->id];
> + int ret;
> +
> + /*
> + * Note: Current source code support for Ethernet, PCIe
> + * initialization is based on the bare metal code shared
> + * by the board team.
> + */
> + ret = pm_runtime_get_sync(priv->pd_list->pd_devs[phy->id]);
> + if (ret < 0) {
> + dev_err(priv->dev,
> + "Failed to power on domain for channel %d: %d\n",
> + phy->id, ret);
> + return ret;
> + }
> +
> + /* Check if initialized with same protocol then skip */
> + if (chan->initialized)
> + return 0;
[Severity: High]
Does this code leak the power management usage counter?
If chan->initialized is true, the function returns directly without dropping
the reference acquired by pm_runtime_get_sync().
> +
> + if (chan->protocol_id == PHY_MODE_PCIE)
> + ret = mp_phy_init_pcie4(phy);
> + else if (chan->protocol_id == PHY_MODE_ETHERNET)
> + ret = mp_phy_init_ethernet(phy);
> + else
> + ret = mp_phy_init_usb(phy);
> + if (ret)
> + return ret;
[Severity: High]
Could this error path also leak the PM usage counter?
If protocol initialization fails, the function returns without decrementing
the usage counter, preventing the power domain from ever suspending.
[ ... ]
> +static int mp_phy_power_on(struct phy *phy)
> +{
> + struct mp_phy_priv *priv = phy_get_drvdata(phy);
> + struct mp_phy_chan_priv *chan = &priv->chan[phy->id];
> + int ret;
> +
> + if (!chan->initialized) {
> + dev_err(priv->dev, "Channel %d not initialized\n", phy->id);
> + return -EINVAL;
> + }
[Severity: High]
Will this unconditionally fail after a suspend/resume cycle?
Since mp_phy_suspend() clears chan->initialized for all channels without
dropping their associated PM domain references, subsequent calls to
phy_power_on() after resume will hit this condition and fail.
[ ... ]
> + } else if (priv->num_lanes[phy->id] == 4) {
> + ret = mp_phy_reg_wait(priv, MPPHY_PXSRAMCNT(2 * phy->id), 0x20, 0x20);
> + if (ret)
> + return ret;
> + ret = mp_phy_reg_wait(priv, MPPHY_PXSRAMCNT((2 * phy->id) + 1), 0x20, 0x20);
[Severity: High]
Can this result in an out-of-bounds regmap access?
If phy->id is 2, (2 * phy->id) + 1 evaluates to 5, which exceeds the
maximum channel index (MPPHY_NUM_CHANNELS is 4).
[ ... ]
> +static struct phy *mp_phy_xlate(struct device *dev,
> + const struct of_phandle_args *args)
> +{
> + struct mp_phy_priv *priv = dev_get_drvdata(dev);
> + int id;
> +
> + if (args->args_count > 1) {
> + dev_err(dev, "Invalid args_count: %d\n", args->args_count);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (args->args_count >= 1)
> + id = args->args[0];
> + else
> + id = 0;
> +
> + return priv->chan[id].phy;
> +}
[Severity: Medium]
Does this code overflow the priv->chan[] array?
If a consumer passes an id >= MPPHY_NUM_CHANNELS via the device tree,
this will result in an out-of-bounds array access since there is no
bounds checking on the raw argument before indexing the array.
[ ... ]
> +static int mp_phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> + ret = mp_phy_parse_dt(pdev, priv);
> + if (ret)
> + return ret;
> +
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(priv->base))
> + return dev_err_probe(dev, PTR_ERR(priv->base), "Failed to map PHY registers\n");
[Severity: Medium]
Does this error path leak the allocated firmware?
mp_phy_parse_dt() can successfully allocate priv->fw via request_firmware().
If devm_platform_ioremap_resource() fails, the function returns directly
without calling release_firmware().
[ ... ]
> + /*
> + * The reset ID order here does matters, reset_control_bulk_assert()
> + * asserts these resets in this order, with mpphy02 reset being
> + * asserted first, reset_control_bulk_deassert() deasserts these
> + * resets in reverse order, with mpphy02 being reset being
> + * deasserted last. This is the behavior the hardware expects.
> + */
> + priv->resets[0].id = "mpphy02";
> + priv->resets[1].id = "mpphy01";
> + priv->resets[2].id = "mpphy11";
> + priv->resets[3].id = "mpphy21";
> + priv->resets[4].id = "mpphy31";
> + ret = devm_reset_control_bulk_get_exclusive(&pdev->dev, ARRAY_SIZE(priv->resets),
> + priv->resets);
[Severity: Medium]
Will reset_control_bulk_deassert() actually deassert these resets in reverse
order?
The reset_control_bulk_deassert() API processes the array sequentially unless
it is unwinding from a failure. This would mean mpphy02 is deasserted first
rather than last, violating the hardware expectation described in the comment.
[ ... ]
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable PHY runtime PM\n");
[Severity: Medium]
Could this error path leak priv->fw and the power domains?
If devm_pm_runtime_enable() fails, the function returns without calling
release_firmware() or dev_pm_domain_detach_list().
> +
> + provider = devm_of_phy_provider_register(dev, mp_phy_xlate);
> + if (IS_ERR(provider))
> + return dev_err_probe(dev, PTR_ERR(provider), "Failed to register PHY provider\n");
> +
> + for (i = 0; i < MPPHY_NUM_CHANNELS; i++) {
> + priv->chan[i].phy = devm_phy_create(dev, NULL, &mp_phy_ops);
> + if (IS_ERR(priv->chan[i].phy)) {
> + return dev_err_probe(dev, PTR_ERR(priv->chan[i].phy),
> + "Failed to create PHY %d\n", i);
> + }
> +
> + priv->chan[i].phy->id = i;
> + phy_set_drvdata(priv->chan[i].phy, priv);
> + }
[Severity: Medium]
Does registering the provider here expose the PHY to consumers prematurely?
The provider is registered before the PHY objects (priv->chan[i].phy) are
created. If a consumer probes immediately, mp_phy_xlate() would return a
NULL pointer.
[ ... ]
> +static void mp_phy_remove(struct platform_device *pdev)
> +{
> + struct mp_phy_priv *priv = dev_get_drvdata(&pdev->dev);
> + struct device *dev = &pdev->dev;
> +
> + pm_runtime_put(dev);
> +
> + dev_pm_domain_detach_list(priv->pd_list);
> +
> + pm_runtime_disable(&pdev->dev);
[Severity: Medium]
Can this result in an aborted power state transition?
According to PM subsystem rules, calling pm_runtime_disable() immediately
after an asynchronous pm_runtime_put() can cancel pending suspend work.
Should pm_runtime_put_sync() be used here instead?
[ ... ]
> +static int mp_phy_suspend(struct device *dev)
> +{
> + struct mp_phy_priv *priv = dev_get_drvdata(dev);
> + int i;
> +
> + for (i = 0; i < MPPHY_NUM_CHANNELS; i++)
> + priv->chan[i].initialized = false;
> +
> + clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
> +
> + dev_info(dev, "Multi-Protocol PHY suspended\n");
> +
> + return 0;
> +}
[Severity: High]
Will clearing chan->initialized here break the PHY on resume?
This flag is cleared without decrementing the associated PM domain usage
counter. On resume, it is never restored to true, causing subsequent calls
to mp_phy_power_on() to fail unconditionally.
[ ... ]
> +static int mp_phy_resume(struct device *dev)
> +{
[ ... ]
> + for (i = 0; i < MPPHY_NUM_CHANNELS; i++) {
> + regmap_set_bits(priv->map, MPPHY_PXTEST(i), MPPHY_PXTEST_BIT);
> + regmap_write(priv->map, MPPHY_PXSRAMCNT(i), priv->sramcnt[i]);
> +
> + if (priv->write_cntxt1 & BIT(i)) {
> + regmap_write(priv->map, MPPHY_CHAN_BASE(i) + 0x10c, 0xff0ff);
[Severity: High]
Are we guaranteed that the power domains for all 4 channels are turned on here?
If a channel is unused and its power domain is off, accessing its registers
blindly in this loop could lead to a bus fault and system crash.
[Severity: High]
Also, will regmap reject this write to 0x10c?
This register offset does not appear to be declared in the
mp_phy_writeable_range table, meaning the regmap core will return an -EIO
error.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260828192223.402422-1-marek.vasut+renesas@mailbox.org?part=2
More information about the linux-phy
mailing list