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

[Wei Fang]

> > +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?
> 

Okay, I will change port_id to u32 type.

> > +
> > +	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?

The switch may be owned by a guest OS, in this case, the switch node of the
host OS will be disabled, but we still hope that the host OS could do some
configurations for the switch, as the netc_blk_ctrl is owned by host OS, so
we cannot check of_device_is_available() for the switch node.

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

This is the expected behavior, only one of swp2 and enetc0 can be enabled,
as they share the same parallel interface. If all of them are enabled, the driver
will detect this incorrect DTS configuration and return a failure.

> 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;
> > +			}
> >  		}
> >  	}