Re: [PATCH net-next v6 11/12] net: pcs: airoha: add PCS driver for Airoha AN7581 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Passing the address of a local stack variable (`&priv`) to `fwnode_pcs_add_provider()` causes a fatal use-after-free when dereferenced later.
- [High] A division by zero will occur during RX PR calibration if the frequency output perfectly matches the target.
- [High] Integer underflow and misuse of the `abs()` macro on unsigned types breaks frequency calibration.
- [High] The `pcs-airoha` module is silently not generated when configured as a loadable module (`M`).
- [High] Shared device-level hardware resets and PLL configurations disrupt other active ports on multi-port devices.
- [High] Missing bounds check on user-provided device tree arguments leads to out-of-bounds array access.
- [High] Missing `FORCE_SEL` bits in `regmap_update_bits` values causes the hardware to ignore the forced configurations.
- [Medium] `regmap_set_bits` performs a read-modify-write on Write-1-to-Clear (W1C) interrupt status registers, silently dropping unhandled interrupts.
- [Low] Missing `fallthrough;` or `break;` annotation in `switch` statement causes an implicit fallthrough compiler warning.
--

commit a1f0ba4cb5341a223ae17b71c3dfd06228567ad5
Author: Christian Marangi <ansuelsmth@gmail.com>
Date:   Tue Jun 9 15:12:07 2026 +0000

net: pcs: airoha: add PCS driver for Airoha AN7581 SoC

This commit introduces the PCS driver for the Airoha AN7581 SoC, supporting
Ethernet, PON, PCIe, and USB SERDES. It implements phylink PCS operations and
handles the configuration of the PCS and PMA blocks.

> diff --git a/drivers/net/pcs/airoha/Makefile b/drivers/net/pcs/airoha/Makefile
> new file mode 100644
> index 0000000000000..25cb8f090c214
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-y                          := pcs-airoha.o

[Severity: High]
Will this correctly build as a loadable module? It appears that using obj-y
forces static inclusion, which means the module won't be generated when
CONFIG_PCS_AIROHA is set to 'm'. Should this use obj-$(CONFIG_PCS_AIROHA)
instead?

> diff --git a/drivers/net/pcs/airoha/pcs-airoha-common.c b/drivers/net/pcs/airoha/pcs-airoha-common.c
> new file mode 100644
> index 0000000000000..ead0bc8083967
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-airoha-common.c
[ ... ]
> +static int airoha_pcs_setup_scu(struct airoha_pcs_priv *priv,
> +                               int index, phy_interface_t interface)
> +{
[ ... ]
> +       /* TODO better handle reset from MAC */
> +       ret = reset_control_bulk_assert(ARRAY_SIZE(priv->rsts),
> +                                       priv->rsts);

[Severity: High]
Does unconditionally triggering these global reset lines disrupt other active
ports on multi-port devices like PCIe? It seems that initializing a second
port will assert these resets and could drop the link on an already active
first port.

[ ... ]
> +static void airoha_pcs_interrupt_init_usxgmii(struct airoha_pcs_priv *priv,
> +                                             int index)
> +{
[ ... ]
> +       /* Clear any pending interrupt */
> +       regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_INT_STA_2,
> +                       AIROHA_PCS_USXGMII_RPCS_FSM_DEC_ERR_INT |
> +                       AIROHA_PCS_USXGMII_R_TYPE_E_INT |
> +                       AIROHA_PCS_USXGMII_R_TYPE_T_INT |
> +                       AIROHA_PCS_USXGMII_R_TYPE_D_INT);

[Severity: Medium]
Could using regmap_set_bits() on a Write-1-to-Clear (W1C) interrupt status
register cause unhandled interrupts to be silently dropped? This performs a
read-modify-write, which might write back 1s to pending interrupts not
targeted by this bitmask. Should this use a direct regmap_write() instead?

[ ... ]
> +static void airoha_pcs_an_restart(struct phylink_pcs *pcs)
> +{
[ ... ]
> +       case PHY_INTERFACE_MODE_USXGMII:
> +               regmap_set_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_CONTROL_0,
> +                               AIROHA_PCS_USXGMII_AN_RESTART);
> +               udelay(3);
> +               regmap_clear_bits(maps->usxgmii_pcs, AIROHA_PCS_USXGMII_PCS_AN_CONTROL_0,
> +                                 AIROHA_PCS_USXGMII_AN_RESTART);
> +       default:
> +               return;

[Severity: Low]
Does this missing break or fallthrough annotation trigger compiler warnings?
It looks like the PHY_INTERFACE_MODE_USXGMII case falls through to default
without explicitly declaring it.

[ ... ]
> +static struct phylink_pcs *airoha_pcs_get(struct fwnode_reference_args *pcsspec,
> +                                         void *data)
> +{
[ ... ]
> +       if (pcsspec->nargs)
> +               index = pcsspec->args[0];
> +
> +       return &priv->ports[index].pcs;

[Severity: High]
Is there a risk of an out-of-bounds array access here? The index is extracted
directly from the device tree via pcsspec->args[0] without verifying if it is
less than priv->data->num_port.

[ ... ]
> +static int airoha_pcs_probe(struct platform_device *pdev)
> +{
[ ... ]
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
[ ... ]
> +       return fwnode_pcs_add_provider(dev_fwnode(dev), airoha_pcs_get,
> +                                      &priv);

[Severity: Critical]
Could passing the address of the local stack variable &priv cause a
use-after-free? When the MAC driver later calls airoha_pcs_get(), it will
dereference this stack pointer after airoha_pcs_probe() has returned. Should
this pass 'priv' instead of '&priv'?

> diff --git a/drivers/net/pcs/airoha/pcs-an7581.c b/drivers/net/pcs/airoha/pcs-an7581.c
> new file mode 100644
> index 0000000000000..22cf7b4b0108f
> --- /dev/null
> +++ b/drivers/net/pcs/airoha/pcs-an7581.c
[ ... ]
> +static void an7581_pcs_jcpll_bringup(struct airoha_pcs_priv *priv,
> +                                    int index, phy_interface_t interface)
> +{
[ ... ]
> +       /* This comment only apply to Serdes PCIe that expose
> +        * 2 PCS.
> +        *
> +        * The Serdes PCIe expose 2 PCS but always require
> +        * the PMA for the first PCS to be configured
> +        * for correct functionality for JCPLL.
> +        */
> +       pcs_pma = priv->pcs_pma[0];

[Severity: High]
Does forcing pcs_pma to index 0 clobber the active PLL configuration of Port
0 when Port 1 is being initialized? It appears this might disrupt the active
connection on the first port.

[ ... ]
> +static void an7581_pcs_tx_bringup(struct airoha_pcs_priv *priv,
> +                                 int index, phy_interface_t interface)
> +{
[ ... ]
> +       regmap_update_bits(pcs_pma, AIROHA_PCS_PMA_PXP_TX_TERM_SEL,
> +                          AIROHA_PCS_PMA_FORCE_SEL_DA_TX_CKIN_DIVISOR |
> +                          AIROHA_PCS_PMA_FORCE_DA_TX_CKIN_DIVISOR |
> +                          AIROHA_PCS_PMA_FORCE_SEL_DA_TX_TERM_SEL |
> +                          AIROHA_PCS_PMA_FORCE_DA_TX_TERM_SEL,
> +                          AIROHA_PCS_PMA_FORCE_SEL_DA_TX_CKIN_DIVISOR |
> +                          FIELD_PREP(AIROHA_PCS_PMA_FORCE_DA_TX_CKIN_DIVISOR,
> +                                     ckin_divisor) |
> +                          FIELD_PREP(AIROHA_PCS_PMA_FORCE_DA_TX_TERM_SEL, 0x0));

[Severity: High]
Will the hardware ignore this forced configuration? The mask includes
AIROHA_PCS_PMA_FORCE_SEL_DA_TX_TERM_SEL, but the value part omits it, which
might clear the force override bit.

[ ... ]
> +static u32 an7581_pcs_rx_prcal_idac_major(struct airoha_pcs_priv *priv,
> +                                         int index, u32 target_fl_out)
> +{
[ ... ]
> +               fl_out = an7581_pcs_apply_cdr_pr_idac(priv, index, cdr_pr_idac_tmp);
> +
> +               /* Use absolute values to find the closest one to target */
> +               fl_out_diff_new = abs(fl_out - target_fl_out);

[Severity: High]
Since fl_out and target_fl_out are both unsigned integers, doesn't this
subtraction cause an underflow if fl_out is less than target_fl_out? Also,
the abs() macro generally expects signed types. Could this cause the
calibration loop to become inaccurate?

[ ... ]
> +static u32 an7581_pcs_rx_prcal_idac_minor(struct airoha_pcs_priv *priv, int index,
> +                                         u32 target_fl_out, u32 cdr_pr_idac_major)
> +{
[ ... ]
> +               fl_out_diff_new = abs(fl_out - target_fl_out);
[ ... ]
> +               if (fl_out_diff_new > fl_out_diff &&
> +                   (abs(fl_out_diff_new - fl_out_diff) * 100) / fl_out_diff > 10) {

[Severity: High]
Is there a risk of a divide-by-zero condition here? If the initial fl_out
reading perfectly matches target_fl_out, fl_out_diff will evaluate to 0,
which would cause the division to panic the kernel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609151212.29469-1-ansuelsmth@gmail.com?part=11