Re: [PATCH v4 net-next 03/14] net: enetc: add pre-boot initialization for i.MX94 switch

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: enetc: add pre-boot initialization for i.MX94 switch

This commit adds pre-boot initialization logic for the i.MX94 switch
in the NETC block control driver. It configures external ports, MII
protocols, and MDIO PHY addresses, and handles the MAC selection
constraint between switch port 2 and ENETC 0.

> diff --git a/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c b/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
> index 92a0f824dae7..c21230f7a7fb 100644
> --- a/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
> +++ b/drivers/net/ethernet/freescale/enetc/netc_blk_ctrl.c
[ ... ]
> +static int imx94_switch_link_config(struct netc_blk_ctrl *priv,
> +				    struct device_node *np,
> +				    bool *swp2_en)
> +{
> +	struct device_node *ports;
> +	int port_id, err = 0;

Since of_property_read_u32() expects a u32 pointer, does passing a
signed int pointer here trigger an incompatible pointer types warning
during compilation?

> +
> +	ports = netc_get_switch_ports(np);
> +	if (!ports)
> +		return -ENODEV;
> +
> +	for_each_available_child_of_node_scoped(ports, child) {
> +		if (of_property_read_u32(child, "reg", &port_id) < 0) {

If the device tree provides a reg value with the MSB set, port_id
would be evaluated as a negative number.

[ ... ]
>  static int imx94_netcmix_init(struct platform_device *pdev)
>  {
>  	struct netc_blk_ctrl *priv = platform_get_drvdata(pdev);
>  	struct device_node *np = pdev->dev.of_node;
> +	bool enetc0_en = false, swp2_en = false;
>  	u32 val;
>  	int err;
>  
>  	for_each_child_of_node_scoped(np, child) {
>  		for_each_child_of_node_scoped(child, gchild) {
> -			if (!of_device_is_compatible(gchild, "pci1131,e101"))
> -				continue;
> -
> -			err = imx94_enetc_link_config(priv, gchild);
> -			if (err)
> -				return err;
> +			if (of_device_is_compatible(gchild, "pci1131,e101")) {
> +				err = imx94_enetc_link_config(priv, gchild,
> +							      &enetc0_en);
> +				if (err)
> +					return err;
> +			} else if (of_device_is_compatible(gchild,
> +							   "pci1131,eef2")) {

Is it possible for the switch node to be disabled in the device tree?

If so, should we check of_device_is_available() for gchild before proceeding,
similar to how it is checked for enetc0_en?

Without checking availability, if the disabled switch lacks a ports subnode,
imx94_switch_link_config() might return -ENODEV and abort the entire probe.

Alternatively, if the disabled switch node has a ports subnode with an
available port 2, it could spuriously set swp2_en = true and cause a false
conflict with ENETC 0.

> +				err = imx94_switch_link_config(priv, gchild,
> +							       &swp2_en);
> +				if (err)
> +					return err;
> +			}
>  		}
>  	}
[ ... ]
> @@ -610,6 +682,77 @@ static int imx94_enetc_mdio_phyaddr_config(struct netc_blk_ctrl *priv,
>  	return 0;
>  }
>  
> +static int imx94_ierb_enetc_init(struct netc_blk_ctrl *priv,
[ ... ]
> +static int imx94_switch_mdio_phyaddr_config(struct netc_blk_ctrl *priv,
> +					    struct device_node *np,
> +					    int port_id, u32 phy_mask)
> +{
> +	int addr;
> +
> +	/* The switch has 3 external ports at most */
> +	if (port_id > 2)
> +		return 0;

If port_id is negative due to the signed type, it will bypass this upper
bound check.

> +
> +	addr = netc_get_phy_addr(np);
> +	if (addr < 0) {
> +		if (addr == -ENODEV)
> +			return 0;
> +
> +		return addr;
> +	}
> +
> +	if (phy_mask & BIT(addr)) {
> +		dev_err(&priv->pdev->dev,
> +			"Found same PHY address in EMDIO and switch node\n");
> +		return -EINVAL;
> +	}
> +
> +	netc_reg_write(priv->ierb, IERB_LBCR(port_id),
> +		       LBCR_MDIO_PHYAD_PRTAD(addr));

Could this lead to an out-of-bounds register write prior to the array base
since port_id is negative?

> +
> +	return 0;
> +}
> +
> +static int imx94_ierb_switch_init(struct netc_blk_ctrl *priv,
> +				  struct device_node *np,
> +				  u32 phy_mask)
> +{
> +	struct device_node *ports;
> +	int port_id, err = 0;

A similar signed int declaration is used here for port_id, which would run
into the same pointer type warning and negative value issues.

> +
> +	ports = netc_get_switch_ports(np);
> +	if (!ports)
> +		return -ENODEV;
[ ... ]
-- 
pw-bot: cr