Date:   Fri, 9 Sep 2022 08:37:17 +0200
From:   Mattias Forsblad <mattias.forsblad@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v7 4/6] net: dsa: mv88e6xxxx: Add RMU
 functionality.

On 2022-09-09 03:36, Andrew Lunn wrote:
>> +	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?
 
>> +
>> +	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

>> +{
>> +	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);


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

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

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

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

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

Powered by blists - more mailing lists