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

+static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
+{
+	const u8 get_id[8] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };

Please add some #defines for these.

I'm also not sure this is the natural format for these messages. It
appears they are built from u16, not u8.

+	int ret = -1;
+
+	if (chip->rmu.got_id)
+		return 0;

get_id is supposed to be the first command you send it. So i would
actually this as part of initialising RMU. If it fails, you can then
fallback to MDIO for everything. That would also mean you don't need
mv88e6xxx_rmu_stats_get() or any other user of RMU to have a call to
mv88e6xxx_rmu_get_id().

+
+	ret = mv88e6xxx_rmu_send_wait(chip, port, MV88E6XXX_RMU_REQ_GET_ID, get_id, 8);
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error for command GET_ID %pe\n", ERR_PTR(ret));
+		return ret;
+	}

This is not really the API i was expecting. A classic RPC would have
both the in and the out parameters here. So i would of expected this
call more like:

u16 req = [ MV88E6XXX_RMU_REQ_FORMAT_0, MV88E6XXX_RMU_PAD, MV88E6XXX_RMU_REQ_GET_ID];
u16 res[3];

	ret = mv88e6xxx_rmu_send_wait(chip, port, req, sizeof(req),
	                              &res, sizeof(res));
	if (ret) {
		dev_dbg(chip->dev, "RMU: error for command GET_ID %pe\n", ERR_PTR(ret));
		return ret;
	}

	if (res[0] != MV88E6XXX_RMU_RESP_FORMAT_1 ||
	    res[2] != MV88E6XXX_RMU_REQ_GET_ID) {
	    dev_dbg(chip->dev, "RMU: Bad responce ...");
	    return -EIO;
	}

	id = res[1];

mv88e6xxx_rmu_send_wait() should do the copy into the response buffer,
or return an negative value if the response is too big for the buffer.

+static int mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
+{
+	u8 dump_mib[8] = { 0x00, 0x01, 0x00, 0x00, 0x10, 0x20, 0x00, 0x00 };
+	int ret;
+
+	ret = mv88e6xxx_rmu_get_id(chip, port);
+	if (ret)
+		return ret;
+
+	/* Send a GET_MIB command */
+	dump_mib[7] = port;
+	ret = mv88e6xxx_rmu_send_wait(chip, port, MV88E6XXX_RMU_REQ_DUMP_MIB, dump_mib, 8);
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error for command DUMP_MIB %pe port %d\n",
+			ERR_PTR(ret), port);
+		return ret;
+	}

Here there is an odd disconnect. I don't see how the response from the
switch gets into *data. A classic RPC structure would help with that.

+
+	/* Update MIB for port */
+	if (chip->info->ops->stats_get_stats)
+		return chip->info->ops->stats_get_stats(chip, port, data);
+
+	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;
+	u16 format;
+	u16 code;
+	u8 seqno;
+
+	if (mv88e6xxx_validate_mac(ds, skb))
+		return;
+
+	/* 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_dbg_ratelimited("RMU: Didn't find switch with index %d", source_device);
+		return;
+	}
+
+	chip = ds->priv;
+	seqno = dsa_header[3];
+	if (seqno != chip->rmu.inband_seqno) {
+		net_dbg_ratelimited("RMU: wrong seqno received. Was %d, expected %d",
+				    seqno, chip->rmu.inband_seqno);
+		return;
+	}
+
+	/* Pull DSA L2 data */
+	skb_pull(skb, MV88E6XXX_DSA_HLEN);
+
+	format = get_unaligned_be16(&skb->data[0]);
+	if (format != MV88E6XXX_RMU_RESP_FORMAT_1 &&
+	    format != MV88E6XXX_RMU_RESP_FORMAT_2) {
+		net_dbg_ratelimited("RMU: received unknown format 0x%04x", format);
+		return;
+	}
+
+	code = get_unaligned_be16(&skb->data[4]);
+	if (code == MV88E6XXX_RMU_RESP_ERROR) {
+		net_dbg_ratelimited("RMU: error response code 0x%04x", code);
+		return;

I don't think you can just discard on errors like this. Ideally you want
mv88e6xxx_rmu_send_wait() to return -EIO to indicate there has been
some sort of error.

Also, this is pretty bad, so i would use net_err_ratelimited().

      Andrew