Re: [PATCH v2 4/4] net: mtip: The L2 switch driver for imx287

[Andrew Lunn]

> +static bool bridge_offload;
> +module_param(bridge_offload, bool, 0644); /* Allow setting by root on boot */
> +MODULE_PARM_DESC(bridge_offload, "L2 switch offload mode enable:1, disable:0");

Please drop. module parameters are not liked.

In Linux, ports of a switch always starting in isolated mode, and
userspace needs to add them to the same bridge.

> +
> +static netdev_tx_t mtip_start_xmit(struct sk_buff *skb,
> +				   struct net_device *dev);
> +static void mtip_switch_tx(struct net_device *dev);
> +static int mtip_switch_rx(struct net_device *dev, int budget, int *port);
> +static void mtip_set_multicast_list(struct net_device *dev);
> +static void mtip_switch_restart(struct net_device *dev, int duplex0,
> +				int duplex1);

Forwards references are not like. Put the functions in the correct
order so they are not needed.

> +/* Calculate Galois Field Arithmetic CRC for Polynom x^8+x^2+x+1.
> + * It omits the final shift in of 8 zeroes a "normal" CRC would do
> + * (getting the remainder).
> + *
> + *  Examples (hexadecimal values):<br>
> + *   10-11-12-13-14-15  => CRC=0xc2
> + *   10-11-cc-dd-ee-00  => CRC=0xe6
> + *
> + *   param: pmacaddress
> + *          A 6-byte array with the MAC address.
> + *          The first byte is the first byte transmitted
> + *   return The 8-bit CRC in bits 7:0
> + */
> +static int crc8_calc(unsigned char *pmacaddress)
> +{
> +	/* byte index */
> +	int byt;
> +	/* bit index */
> +	int bit;
> +	int inval;
> +	int crc;

Reverse Christmas tree. Please look through the whole driver and fix
it up.

> +/* updates MAC address lookup table with a static entry
> + * Searches if the MAC address is already there in the block and replaces
> + * the older entry with new one. If MAC address is not there then puts a
> + * new entry in the first empty slot available in the block
> + *
> + * mac_addr Pointer to the array containing MAC address to
> + *          be put as static entry
> + * port     Port bitmask numbers to be added in static entry,
> + *          valid values are 1-7
> + * priority The priority for the static entry in table
> + *
> + * return 0 for a successful update else -1  when no slot available

It would be nice to turn this into proper kerneldoc. It is not too far
away at the moment.

Also, return a proper error code not -1. ENOSPC?

> +static int mtip_update_atable_dynamic1(unsigned long write_lo,
> +				       unsigned long write_hi, int block_index,
> +				       unsigned int port,
> +				       unsigned int curr_time,
> +				       struct switch_enet_private *fep)

It would be good to document the return value, because it is not the
usual 0 success or negative error code.

> +static const struct net_device_ops mtip_netdev_ops;

more forward declarations.

> +struct switch_enet_private *mtip_netdev_get_priv(const struct net_device *ndev)
> +{
> +	if (ndev->netdev_ops == &mtip_netdev_ops)
> +		return netdev_priv(ndev);
> +
> +	return NULL;
> +}

I _think_ the return value is not actually used. So maybe 0 or
-ENODEV?

> +static int esw_mac_addr_static(struct switch_enet_private *fep)
> +{
> +	int i;
> +
> +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> +		if (is_valid_ether_addr(fep->ndev[i]->dev_addr)) {

Is that possible? This is the interfaces own MAC address? If it is not
valid, the probe should of failed.

> +			mtip_update_atable_static((unsigned char *)
> +						  fep->ndev[i]->dev_addr,
> +						  7, 7, fep);
> +		} else {
> +			dev_err(&fep->pdev->dev,
> +				"Can not add mac address %pM to switch!\n",
> +				fep->ndev[i]->dev_addr);
> +			return -EFAULT;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void mtip_print_link_status(struct phy_device *phydev)
> +{
> +	if (phydev->link)
> +		netdev_info(phydev->attached_dev,
> +			    "Link is Up - %s/%s - flow control %s\n",
> +			    phy_speed_to_str(phydev->speed),
> +			    phy_duplex_to_str(phydev->duplex),
> +			    phydev->pause ? "rx/tx" : "off");
> +	else
> +		netdev_info(phydev->attached_dev, "Link is Down\n");
> +}

phy_print_status()

> +static void mtip_adjust_link(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	struct phy_device *phy_dev;
> +	int status_change = 0, idx;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fep->hw_lock, flags);
> +
> +	idx = priv->portnum - 1;
> +	phy_dev = fep->phy_dev[idx];
> +
> +	/* Prevent a state halted on mii error */
> +	if (fep->mii_timeout && phy_dev->state == PHY_HALTED) {
> +		phy_dev->state = PHY_UP;
> +		goto spin_unlock;
> +	}

A MAC driver should not be playing around with the internal state of
phylib.

> +static int mtip_mii_probe(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +	int port_idx = priv->portnum - 1;
> +	struct phy_device *phy_dev = NULL;
> +
> +	if (fep->phy_np[port_idx]) {
> +		phy_dev = of_phy_connect(dev, fep->phy_np[port_idx],
> +					 &mtip_adjust_link, 0,
> +					 fep->phy_interface[port_idx]);
> +		if (!phy_dev) {
> +			netdev_err(dev, "Unable to connect to phy\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	phy_set_max_speed(phy_dev, 100);
> +	fep->phy_dev[port_idx] = phy_dev;
> +	fep->link[port_idx] = 0;
> +	fep->full_duplex[port_idx] = 0;
> +
> +	dev_info(&dev->dev,
> +		 "MTIP PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)\n",
> +		 fep->phy_dev[port_idx]->drv->name,
> +		 phydev_name(fep->phy_dev[port_idx]),
> +		 fep->phy_dev[port_idx]->irq);

phylib already prints something like that.

> +static int mtip_mdiobus_reset(struct mii_bus *bus)
> +{
> +	if (!bus || !bus->reset_gpiod) {
> +		dev_err(&bus->dev, "Reset GPIO pin not provided!\n");
> +		return -EINVAL;
> +	}
> +
> +	gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> +
> +	/* Extra time to allow:
> +	 * 1. GPIO RESET pin go high to prevent situation where its value is
> +	 *    "LOW" as it is NOT configured.
> +	 * 2. The ENET CLK to stabilize before GPIO RESET is asserted
> +	 */
> +	usleep_range(200, 300);
> +
> +	gpiod_set_value_cansleep(bus->reset_gpiod, 0);
> +	usleep_range(bus->reset_delay_us, bus->reset_delay_us + 1000);
> +	gpiod_set_value_cansleep(bus->reset_gpiod, 1);
> +
> +	if (bus->reset_post_delay_us > 0)
> +		usleep_range(bus->reset_post_delay_us,
> +			     bus->reset_post_delay_us + 1000);
> +
> +	return 0;
> +}

What is wrong with the core code __mdiobus_register() which does the
bus reset.

> +static void mtip_get_drvinfo(struct net_device *dev,
> +			     struct ethtool_drvinfo *info)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +	struct switch_enet_private *fep = priv->fep;
> +
> +	strscpy(info->driver, fep->pdev->dev.driver->name,
> +		sizeof(info->driver));
> +	strscpy(info->version, VERSION, sizeof(info->version));

Leave this empty, so you get the git hash of the kernel.

> +static void mtip_ndev_setup(struct net_device *dev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(dev);
> +
> +	ether_setup(dev);

That is pretty unusual

> +	dev->ethtool_ops = &mtip_ethtool_ops;
> +	dev->netdev_ops = &mtip_netdev_ops;
> +
> +	memset(priv, 0, sizeof(struct mtip_ndev_priv));

priv should already be zero....

> +static int mtip_ndev_init(struct switch_enet_private *fep)
> +{
> +	struct mtip_ndev_priv *priv;
> +	int i, ret = 0;
> +
> +	for (i = 0; i < SWITCH_EPORT_NUMBER; i++) {
> +		fep->ndev[i] = alloc_netdev(sizeof(struct mtip_ndev_priv),
> +					    fep->ndev_name[i], NET_NAME_USER,
> +					    mtip_ndev_setup);

This explains the ether_setup(). It would be more normal to pass
ether_setup() here, and set dev->ethtool_ops and dev->netdev_ops here.

> +		if (!fep->ndev[i]) {
> +			ret = -1;

-ENOMEM?

> +			break;
> +		}
> +
> +		priv = netdev_priv(fep->ndev[i]);
> +		priv->fep = fep;
> +		priv->portnum = i + 1;
> +		fep->ndev[i]->irq = fep->irq;
> +
> +		ret = mtip_setup_mac(fep->ndev[i]);
> +		if (ret) {
> +			dev_err(&fep->ndev[i]->dev,
> +				"%s: ndev %s MAC setup err: %d\n",
> +				__func__, fep->ndev[i]->name, ret);
> +			break;
> +		}
> +
> +		ret = register_netdev(fep->ndev[i]);
> +		if (ret) {
> +			dev_err(&fep->ndev[i]->dev,
> +				"%s: ndev %s register err: %d\n", __func__,
> +				fep->ndev[i]->name, ret);
> +			break;
> +		}
> +		dev_info(&fep->ndev[i]->dev, "%s: MTIP eth L2 switch %pM\n",
> +			 fep->ndev[i]->name, fep->ndev[i]->dev_addr);

I would drop this. A driver is normally silent unless things go wrong.

> +	}
> +
> +	if (ret)
> +		mtip_ndev_cleanup(fep);
> +
> +	return 0;

return ret?

> +static int mtip_ndev_port_link(struct net_device *ndev,
> +			       struct net_device *br_ndev)
> +{
> +	struct mtip_ndev_priv *priv = netdev_priv(ndev);
> +	struct switch_enet_private *fep = priv->fep;
> +
> +	dev_dbg(&ndev->dev, "%s: ndev: %s br: %s fep: 0x%x\n",
> +		__func__, ndev->name,  br_ndev->name, (unsigned int)fep);
> +
> +	/* Check if MTIP switch is already enabled */
> +	if (!fep->br_offload) {
> +		if (!priv->master_dev)
> +			priv->master_dev = br_ndev;

It needs to be a little bit more complex than that, because the two
ports could be assigned to two different bridges. You should only
enable hardware bridging if they are a member of the same bridge.

	Andrew