Re: [PATCH v1 3/9] net: add DSA framework to support basic switch functionality

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Mon, Mar 28, 2022 at 12:31:39PM +0200, Sascha Hauer wrote:
> On Mon, Mar 21, 2022 at 10:26:00AM +0100, Oleksij Rempel wrote:
 > +static void dsa_port_set_ethaddr(struct eth_device *edev)
> > +{
> > +	struct dsa_port *dp = edev->priv;
> > +	struct dsa_switch *ds = dp->ds;
> > +
> > +	if (is_valid_ether_addr(edev->ethaddr))
> > +		return;
> > +
> > +	if (!is_valid_ether_addr(ds->edev_master->ethaddr))
> > +		return;
> > +
> > +	eth_set_ethaddr(edev, ds->edev_master->ethaddr);
> 
> You are setting a ports MAC address here. Shouldn't this be different
> from the master MAC address?

Potentially it can be different, but this functionality will need more
work without notable advantage. We will need promisc support for master
MAC and filters to pass own MACs only.

I assume, for current step this is not needed.

> > +}
> > +
> > +static int dsa_port_start(struct eth_device *edev)
> > +{
> > +	struct dsa_port *dp = edev->priv;
> > +	struct dsa_switch *ds = dp->ds;
> > +	const struct dsa_ops *ops = ds->ops;
> > +	int ret;
> > +
> > +	if (!dp->edev.phydev)
> > +		return -ENODEV;
> > +
....
> > +
> > +	}
> > +
> > +	if (!ds->cpu_port_active) {
> > +		struct dsa_port *dpc = ds->dp[ds->cpu_port];
> > +		ret = ops->port_enable(dpc, ds->cpu_port,
> > +				       ds->cpu_port_fixed_phy);
> 
> Is ops->port_enable optional or not?

ack

> 
> > +		if (ret)
> > +			return ret;
> > +		eth_open(ds->edev_master);
> > +		ds->cpu_port_active = true;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/* Stop the desired port, the CPU port and the master Eth interface */
> > +static void dsa_port_stop(struct eth_device *edev)
> > +{
> > +	struct dsa_port *dp = edev->priv;
> > +	struct dsa_switch *ds = dp->ds;
> > +	const struct dsa_ops *ops = ds->ops;
> > +
> > +	if (ops->port_disable)
> > +		ops->port_disable(dp, dp->index, dp->edev.phydev);
> 
> This suggests ops->port_disable is optional...
> 
> > +
> > +
> > +	if (ds->cpu_port_active) {
> > +		struct dsa_port *dpc = ds->dp[ds->cpu_port];
> > +		ops->port_disable(dpc, ds->cpu_port, ds->cpu_port_fixed_phy);
> 
> ... but it's called unconditionally here.

ack

> > +		eth_close(ds->edev_master);
> 
> This function stops a single port. Is it correct to call eth_close on
> the master device here? What if other ports are still active?

ack, i'll need to implement refcount support or something like this.

> > +	if (of_property_read_u32(phy_node, "speed", &phydev->speed))
> > +		return -ENODEV;
> > +
> > +	phydev->duplex = of_property_read_bool(phy_node,"full-duplex");
> > +	phydev->pause = of_property_read_bool(phy_node, "pause");
> > +	phydev->asym_pause = of_property_read_bool(phy_node, "asym-pause");
> 
> We already have a function parsing these properties. Could we
> consolidate that?

ok, i'll do it

> > +	for_each_available_child_of_node(ports, port) {
> > +		struct device_node *master;
> > +
> > +		ret = of_property_read_u32(port, "reg", &reg);
> > +		if (ret) {
> > +			dev_err(ds->dev, "No or too many ports are configured\n");
> > +			goto out_put_node;
> > +		}
> 
> You won't hit this case here, it is already caught above.
> 
> > +
> > +		if (reg >= ds->num_ports) {
> > +			dev_err(ds->dev, "port %pOF index %u exceeds num_ports (%u)\n",
> > +				port, reg, ds->num_ports);
> > +			ret = -EINVAL;
> > +			goto out_put_node;
> > +		}
> 
> ditto

good point.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox