Re: [PATCH net-next v8 3/3] net: dsa: yt921x: Add support for Motorcomm YT921x

[Andrew Lunn <andrew@lunn.ch>]

> +static void yt921x_reg_mdio_verify(u32 reg, u16 val, bool lo)
> +{
> +	const char *desc;
> +
> +	switch (val) {
> +	case 0xfade:
> +		desc = "which is likely from a non-existent register";
> +		break;
> +	case 0xdead:
> +		desc = "which is likely a data race condition";
> +		break;

Where did these two values come from? Are they documented in the datasheet?

> +	default:
> +		return;
> +	}
> +
> +	/* Skip registers which are likely to have any valid values */
> +	switch (reg) {
> +	case YT921X_MAC_ADDR_HI2:
> +	case YT921X_MAC_ADDR_LO4:
> +	case YT921X_FDB_OUT0:
> +	case YT921X_FDB_OUT1:
> +		return;
> +	}
> +
> +	pr_warn("%s: Read 0x%x at 0x%x %s32, %s; "
> +		"consider reporting a bug if this happens again\n",
> +		__func__, val, reg, lo ? "lo" : "hi", desc);

You probably have a warning from checkpatch about pr_warn. Ideally you
want to give an indication which device has triggered this, making use
of a struct device. You might want to include that in context.

> +static int
> +yt921x_intif_read(struct yt921x_priv *priv, int port, int reg, u16 *valp)
> +{
> +	if ((u16)val != val)
> +		dev_err(dev,
> +			"%s: port %d, reg 0x%x: Expected u16, got 0x%08x\n",
> +			__func__, port, reg, val);
> +	*valp = (u16)val;
> +	return 0;

You don't treat this as an error, you don't return -EIO or -EPROTO etc.
So maybe this should be dev_info() or dev_dbg().

> +static int
> +yt921x_mbus_int_write(struct mii_bus *mbus, int port, int reg, u16 data)
> +{
> +	struct yt921x_priv *priv = mbus->priv;
> +	int res;
> +
> +	if (port >= YT921X_PORT_NUM)
> +		return 0;

-ENODEV.

> +yt921x_mbus_int_init(struct yt921x_priv *priv, struct device_node *mnp)
> +{
> +	struct device *dev = to_device(priv);
> +	struct mii_bus *mbus;
> +	int res;
> +
> +	if (!mnp)
> +		res = devm_mdiobus_register(dev, mbus);
> +	else
> +		res = devm_of_mdiobus_register(dev, mbus, mnp);

You can call devm_of_mdiobus_register() with a NULL pointer for the
OF, and it will do the correct thing.

> +static int yt921x_extif_wait(struct yt921x_priv *priv)
> +{
> +	u32 val;
> +	int res;
> +
> +	res = yt921x_reg_read(priv, YT921X_EXT_MBUS_OP, &val);
> +	if (res)
> +		return res;
> +	if ((val & YT921X_MBUS_OP_START) != 0) {
> +		res = read_poll_timeout(yt921x_reg_read, res,
> +					(val & YT921X_MBUS_OP_START) == 0,
> +					YT921X_POLL_SLEEP_US,
> +					YT921X_POLL_TIMEOUT_US,
> +					true, priv, YT921X_EXT_MBUS_OP, &val);
> +		if (res)
> +			return res;
> +	}
> +
> +	return 0;

In mv88e6xxx, we have the generic mv88e6xxx_wait_mask() and on top of
that mv88e6xxx_wait_bit(). That allows us to have register specific
wait functions as one liners. Please consider something similar.

> +static int yt921x_mib_read(struct yt921x_priv *priv, int port, void *data)
> +{

As far as i can see, data is always a pointer to struct
yt921x_mib_raw. I would be better to not have the void in the middle.
It also makes it clearer what assumption you are making about the
layout of that structure.

> +	unsigned char *buf = data;
> +	int res = 0;
> +
> +	for (size_t i = 0; i < sizeof(struct yt921x_mib_raw);
> +	     i += sizeof(u32)) {
> +		res = yt921x_reg_read(priv, YT921X_MIBn_DATA0(port) + i,
> +				      (u32 *)&buf[i]);
> +		if (res)
> +			break;
> +	}
> +	return res;
> +}
> +
> +static void yt921x_poll_mib(struct work_struct *work)
> +{
> +	struct yt921x_port *pp = container_of_const(work, struct yt921x_port,
> +						    mib_read.work);
> +	struct yt921x_priv *priv = (void *)(pp - pp->index) -
> +				   offsetof(struct yt921x_priv, ports);

Can you make container_of() work for this?

> +	unsigned long delay = YT921X_STATS_INTERVAL_JIFFIES;
> +	struct device *dev = to_device(priv);
> +	struct yt921x_mib *mib = &pp->mib;
> +	struct yt921x_mib_raw raw;
> +	int port = pp->index;
> +	int res;
> +
> +	yt921x_reg_lock(priv);
> +	res = yt921x_mib_read(priv, port, &raw);
> +	yt921x_reg_unlock(priv);
> +
> +	if (res) {
> +		dev_err(dev, "Failed to %s port %d: %i\n", "read stats for",
> +			port, res);
> +		delay *= 4;
> +		goto end;
> +	}
> +
> +	spin_lock(&pp->stats_lock);
> +
> +	/* Handle overflow of 32bit MIBs */
> +	for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> +		const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> +		u32 *rawp = (u32 *)((u8 *)&raw + desc->offset);
> +		u64 *valp = &((u64 *)mib)[i];
> +		u64 newval;
> +
> +		if (desc->size > 1) {
> +			newval = ((u64)rawp[0] << 32) | rawp[1];
> +		} else {
> +			newval = (*valp & ~(u64)U32_MAX) | *rawp;
> +			if (*rawp < (u32)*valp)
> +				newval += (u64)1 << 32;
> +		}

There are way too many casts here. Think about your types, and how you
can remove some of these casts. In general, casts are bad, and should
be avoided where possible.

> +static void
> +yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct yt921x_port *pp = &priv->ports[port];
> +	struct yt921x_mib *mib = &pp->mib;
> +	size_t j;
> +
> +	spin_lock(&pp->stats_lock);
> +
> +	j = 0;
> +	for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> +		const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> +
> +		if (!desc->unstructured)
> +			continue;
> +
> +		data[j] = ((u64 *)mib)[i];
> +		j++;
> +	}
>

ethtool APIs are called in a context where you can block. So it would
be good to updated the statistics first before copying them. You just
need to think about your locking in case the worker is running.

> +static int yt921x_dsa_get_sset_count(struct dsa_switch *ds, int port, int sset)
> +{
> +	int cnt;
> +
> +	if (sset != ETH_SS_STATS)
> +		return 0;
> +
> +	cnt = 0;

Please do the zeroing above when you declare the local variable.

> +	for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> +		const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> +
> +		if (desc->unstructured)
> +			cnt++;
> +	}
> +
> +	return cnt;
> +}

> +static int
> +yt921x_set_eee(struct yt921x_priv *priv, int port, struct ethtool_keee *e)
> +{

> +	/* Enable / disable port EEE */
> +	res = yt921x_reg_toggle_bits(priv, YT921X_EEE_CTRL,
> +				     YT921X_EEE_CTRL_ENn(port), enable);
> +	if (res)
> +		return res;
> +	res = yt921x_reg_toggle_bits(priv, YT921X_EEEn_VAL(port),
> +				     YT921X_EEE_VAL_DATA, enable);

How do these two different registers differ? Why are there two of
them? Maybe add a comment to explain this.

> +static bool yt921x_dsa_support_eee(struct dsa_switch *ds, int port)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +
> +	return (priv->pon_strap_cap & YT921X_PON_STRAP_EEE) != 0;

What does the strapping actually tell you?

> +static int
> +yt921x_dsa_port_mirror_add(struct dsa_switch *ds, int port,
> +			   struct dsa_mall_mirror_tc_entry *mirror,
> +			   bool ingress, struct netlink_ext_ack *extack)
> +{
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	u32 ctrl;
> +	u32 val;
> +	int res;
> +
> +	yt921x_reg_lock(priv);
> +	do {
> +		u32 srcs;
> +		u32 dst;
> +
> +		if (ingress)
> +			srcs = YT921X_MIRROR_IGR_PORTn(port);
> +		else
> +			srcs = YT921X_MIRROR_EGR_PORTn(port);
> +		dst = YT921X_MIRROR_PORT(mirror->to_local_port);
> +
> +		res = yt921x_reg_read(priv, YT921X_MIRROR, &val);
> +		if (res)
> +			break;
> +
> +		/* other mirror tasks & different dst port -> conflict */
> +		if ((val & ~srcs & (YT921X_MIRROR_EGR_PORTS_M |
> +				    YT921X_MIRROR_IGR_PORTS_M)) != 0 &&
> +		    (val & YT921X_MIRROR_PORT_M) != dst) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Sniffer port is already configured,"
> +					   " delete existing rules & retry");
> +			res = -EBUSY;
> +			break;
> +		}
> +
> +		ctrl = val & ~YT921X_MIRROR_PORT_M;
> +		ctrl |= srcs;
> +		ctrl |= dst;
> +
> +		if (ctrl != val)
> +			res = yt921x_reg_write(priv, YT921X_MIRROR, ctrl);
> +	} while (0);

What does a while (0) loop bring you here?


    Andrew

---
pw-bot: cr