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

[Andrew Lunn <andrew@lunn.ch>]

> +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