mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Oleksij Rempel <o.rempel@pengutronix.de>
To: Sascha Hauer <sha@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH v1 3/9] net: add DSA framework to support basic switch functionality
Date: Mon, 28 Mar 2022 14:23:29 +0200	[thread overview]
Message-ID: <20220328122329.GC23999@pengutronix.de> (raw)
In-Reply-To: <20220328103139.GT12181@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


  reply	other threads:[~2022-03-28 12:24 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-21  9:25 [PATCH v1 0/9] add basic DSA support Oleksij Rempel
2022-03-21  9:25 ` [PATCH v1 1/9] net: add RX preprocessor support Oleksij Rempel
2022-03-21  9:25 ` [PATCH v1 2/9] net: add of_find_eth_device_by_node() function Oleksij Rempel
2022-03-21  9:26 ` [PATCH v1 3/9] net: add DSA framework to support basic switch functionality Oleksij Rempel
2022-03-25  9:37   ` Oleksij Rempel
2022-03-28 10:31   ` Sascha Hauer
2022-03-28 12:23     ` Oleksij Rempel [this message]
2022-03-21  9:26 ` [PATCH v1 4/9] driver: add dev_get_priv() helper Oleksij Rempel
2022-03-21  9:26 ` [PATCH v1 5/9] net: port part of if_vlan header from kernel v5.17 Oleksij Rempel
2022-03-21  9:26 ` [PATCH v1 6/9] spi: port spi_sync_transfer() function " Oleksij Rempel
2022-03-21  9:26 ` [PATCH v1 7/9] net: mdio: add MDIO_DEVAD_NONE define Oleksij Rempel
2022-03-21  9:26 ` [PATCH v1 8/9] net: phy: make sure MDIO bus is probed if we search for the PHY Oleksij Rempel
2022-03-21  9:26 ` [PATCH v1 9/9] net: dsa: add support for SJA11xx switches Oleksij Rempel
2022-03-28 10:40   ` Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220328122329.GC23999@pengutronix.de \
    --to=o.rempel@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=sha@pengutronix.de \
    --subject='Re: [PATCH v1 3/9] net: add DSA framework to support basic switch functionality' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox