Re: [PATCH net-next v7 4/6] net: dsa: mv88e6xxxx: Add RMU functionality.

On 2022-09-09 03:36, Andrew Lunn wrote:

quoted

+	kfree_skb(chip->rmu.resp);
+	chip->rmu.resp = NULL;
+
+	mutex_unlock(&chip->rmu.mutex);
+
+	return ret > 0 ? 0 : ret;
+}
+
+static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
+{
+	const u16 req[4] = { MV88E6XXX_RMU_REQ_FORMAT_GET_ID,
+			     MV88E6XXX_RMU_REQ_PAD,
+			     MV88E6XXX_RMU_REQ_CODE_GET_ID,
+			     MV88E6XXX_RMU_REQ_DATA};
+	struct rmu_header resp;
+	int ret = -1;
+	u16 format;
+	u16 code;
+
+	ret = mv88e6xxx_rmu_send_wait(chip, port, (const char *)req, sizeof(req),
+				      (char *)&resp, sizeof(resp));
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error for command GET_ID %pe\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Got response */
+	format = get_unaligned_be16(&resp.format);
+	code = get_unaligned_be16(&resp.code);

You don't need get_unaligned_be16() etc here, because resp is a stack
variable, it is guaranteed to be aligned. You only need to use these
helpers when you have no idea about alignment, like data in an skb.

So this I don't understand. We map a packed struct from incoming skb data
(which could be unaligned). Couldn't the struct members then be unaligned
also?
 

quoted

+
+	if (format != MV88E6XXX_RMU_RESP_FORMAT_1 &&
+	    format != MV88E6XXX_RMU_RESP_FORMAT_2 &&
+	    code != MV88E6XXX_RMU_RESP_CODE_GOT_ID) {
+		net_dbg_ratelimited("RMU: received unknown format 0x%04x code 0x%04x",
+				    format, code);
+		return -EIO;
+	}
+
+	chip->rmu.prodnr = get_unaligned_be16(&resp.prodnr);
+
+	return 0;
+}
+
+static void mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)

I would split this into a separate patch. Add the core RMU handling in
one patch, and then add users of it one patch at a time. There is too
much going on in this patch, and it is not obviously correct.

Yes, this should be in the patch 5

quoted

+{
+	u16 req[4] = { MV88E6XXX_RMU_REQ_FORMAT_SOHO,
+		       MV88E6XXX_RMU_REQ_PAD,
+		       MV88E6XXX_RMU_REQ_CODE_DUMP_MIB,
+		       MV88E6XXX_RMU_REQ_DATA};
+	struct dump_mib_resp resp;
+	struct mv88e6xxx_port *p;
+	u8 resp_port;
+	u16 format;
+	u16 code;
+	int ret;
+	int i;
+
+	/* Populate port number in request */
+	req[3] = FIELD_PREP(MV88E6XXX_RMU_REQ_DUMP_MIB_PORT_MASK, port);
+
+	ret = mv88e6xxx_rmu_send_wait(chip, port, (const char *)req, sizeof(req),
+				      (char *)&resp, sizeof(resp));
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error for command DUMP_MIB %pe port %d\n",
+			ERR_PTR(ret), port);
+		return;
+	}

I'm surprised this is a void function, since mv88e6xxx_rmu_send_wait()
etc can return an error.

This is because the caller prototype is declared thus:
 	void	(*get_ethtool_stats)(struct dsa_switch *ds,
				     int port, uint64_t *data);

quoted

+	for (i = 0; i < MV88E6XXX_RMU_MAX_RMON; i++)
+		p->rmu_raw_stats[i] = get_unaligned_be32(&resp.mib[i]);
+
+	/* Update MIB for port */
+	if (chip->info->ops->stats_get_stats)
+		chip->info->ops->stats_get_stats(chip, port, data);
+}
+
+void mv88e6xxx_master_change(struct dsa_switch *ds, const struct net_device *master,
+			     bool operational)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct dsa_port *cpu_dp;
+	int port;
+	int ret;
+
+	cpu_dp = master->dsa_ptr;
+	port = dsa_towards_port(ds, cpu_dp->ds->index, cpu_dp->index);
+
+	mv88e6xxx_reg_lock(chip);
+
+	if (operational && chip->info->ops->rmu_enable) {
+		if (!chip->info->ops->rmu_enable(chip, port)) {

You should probably differentiate on the error here. -EOPNOTSUPP you
want to handle different to other errors, which are fatal.

Will fix.

quoted

+			dev_dbg(chip->dev, "RMU: Enabled on port %d", port);
+			chip->rmu.master_netdev = (struct net_device *)master;
+
+			/* Check if chip is alive */
+			ret = mv88e6xxx_rmu_get_id(chip, port);
+			if (!ret)
+				goto out;
+
+			chip->smi_ops = chip->rmu.rmu_ops;
+
+		} else {
+			dev_err(chip->dev, "RMU: Unable to enable on port %d", port);

Don't you need a goto out; here?

Will fix.

quoted

+		}
+	}
+
+	chip->smi_ops = chip->rmu.smi_ops;
+	chip->rmu.master_netdev = NULL;
+	if (chip->info->ops->rmu_disable)
+		chip->info->ops->rmu_disable(chip);

I would probably pull this function apart, break it up into helpers,
to make the flow simpler.

Ok, will fix.

quoted

+
+out:
+	mv88e6xxx_reg_unlock(chip);
+}
+
+static int mv88e6xxx_validate_mac(struct dsa_switch *ds, struct sk_buff *skb)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	unsigned char *ethhdr;
+
+	/* Check matching MAC */
+	ethhdr = skb_mac_header(skb);
+	if (!ether_addr_equal(chip->rmu.master_netdev->dev_addr, ethhdr)) {
+		dev_dbg_ratelimited(ds->dev, "RMU: mismatching MAC address for request. Rx %pM expecting %pM\n",
+				    ethhdr, chip->rmu.master_netdev->dev_addr);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+void mv88e6xxx_decode_frame2reg_handler(struct net_device *dev, struct sk_buff *skb)
+{
+	struct dsa_port *dp = dev->dsa_ptr;
+	struct dsa_switch *ds = dp->ds;
+	struct mv88e6xxx_chip *chip;
+	int source_device;
+	u8 *dsa_header;
+	u8 seqno;
+
+	/* Decode Frame2Reg DSA portion */
+	dsa_header = skb->data - 2;
+
+	source_device = FIELD_GET(MV88E6XXX_SOURCE_DEV, dsa_header[0]);
+	ds = dsa_switch_find(ds->dst->index, source_device);
+	if (!ds) {
+		net_err_ratelimited("RMU: Didn't find switch with index %d", source_device);
+		return;
+	}
+
+	if (mv88e6xxx_validate_mac(ds, skb))
+		return;
+
+	chip = ds->priv;
+	seqno = dsa_header[3];
+	if (seqno != chip->rmu.seqno) {
+		net_err_ratelimited("RMU: wrong seqno received. Was %d, expected %d",
+				    seqno, chip->rmu.seqno);
+		return;
+	}
+
+	/* Pull DSA L2 data */
+	skb_pull(skb, MV88E6XXX_DSA_HLEN);
+
+	/* Make an copy for further processing in initiator */
+	chip->rmu.resp = skb_copy(skb, GFP_ATOMIC);
+	if (!chip->rmu.resp)
+		return;

I think this should be in the other order. First see if there is
anybody interested in the skb, then make a copy of it.

That's not what it's doing. It's a check that the skb_copy
finished correctly, not that anyone is interested in a reply.
Or did I misinterpret your comment?

quoted

+
+	dsa_switch_inband_complete(ds, NULL);
+}
+
+int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
+{
+	mutex_init(&chip->rmu.mutex);
+
+	/* Remember original ops for restore */
+	chip->rmu.smi_ops = chip->smi_ops;
+
+	/* Change rmu ops with our own pointer */
+	chip->rmu.rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;

Why do you need a cast? In general, casts are wrong, it suggests your
types are wrong.

quoted

+	chip->rmu.rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;
+
+	if (chip->info->ops->rmu_disable)
+		return chip->info->ops->rmu_disable(chip);

Why is a setup function calling disable?

So Vladimir Oltean commented before:
"I think it's very important for the RMU to still start as disabled.
You enable it dynamically when the master goes up."

    Andrew