Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

Mattias Forsblad
2022-08-31T06:12:16Z

On 2022-08-30 18:35, Vladimir Oltean wrote:
> On Fri, Aug 26, 2022 at 08:38:15AM +0200, Mattias Forsblad wrote:
>> static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>> {
>> + if (chip->info->ops->rmu_enable)
>> + if (!chip->info->ops->rmu_enable(chip))
>> + return mv88e6xxx_rmu_init(chip);
>> +
>> if (chip->info->ops->rmu_disable)
>> return chip->info->ops->rmu_disable(chip);
>
> I think it's very important for the RMU to still start as disabled.
> You enable it dynamically when the master goes up.
>

Please elaborate why this may pose a problem, I might have missed
some information.

>> diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
>> new file mode 100644
>> index 000000000000..b7d850c099c5
>> --- /dev/null
>> +++ b/drivers/net/dsa/mv88e6xxx/rmu.c
>> @@ -0,0 +1,273 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Marvell 88E6xxx Switch Remote Management Unit Support
>> + *
>> + * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
>> + *
>> + */
>> +
>> +#include <asm/unaligned.h>
>> +#include "rmu.h"
>> +#include "global1.h"
>> +
>> +static int mv88e6xxx_validate_port_mac(struct dsa_switch *ds, struct sk_buff *skb)
>> +{
>> + unsigned char *ethhdr;
>> + struct dsa_port *dp;
>> + u8 pkt_port;
>> +
>> + pkt_port = (skb->data[7] >> 3) & 0xf;
>> + dp = dsa_to_port(ds, pkt_port);
>> + if (!dp) {
>> + dev_dbg_ratelimited(ds->dev, "RMU: couldn't find port for %d\n", pkt_port);
>> + return -ENXIO;
>> + }
>> +
>> + /* Check matching MAC */
>> + ethhdr = skb_mac_header(skb);
>> + if (memcmp(dp->slave->dev_addr, ethhdr, ETH_ALEN)) {
>
> ether_addr_equal()
>

Ofc.

> Also, what happens if you don't validate the MAC DA of the response, and
> in general, if you just put your MAC SA as the MAC address of the
> operationally active RMU DSA master? I guess the whole idea is to
> provide a MAC address which the DSA master won't drop with its RX
> filter, and its own MAC address is just fine for that.
>>> + dev_dbg_ratelimited(ds->dev, "RMU: mismatching MAC address for request. Rx %pM expecting %pM\n",
>> + ethhdr, dp->slave->dev_addr);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no)
>> +{
>> + struct mv88e6xxx_chip *chip = ds->priv;
>> + struct mv88e6xxx_port *port;
>> + u16 prodnum;
>> + u16 format;
>> + u8 pkt_dev;
>> + u8 pkt_prt;
>> + u16 code;
>> + int i;
>> +
>> + /* Extract response data */
>> + format = get_unaligned_be16(&skb->data[0]);
>> + if (format != htons(MV88E6XXX_RMU_RESP_FORMAT_1) &&
>> + format != htons(MV88E6XXX_RMU_RESP_FORMAT_2)) {
>> + dev_err_ratelimited(chip->dev, "RMU: received unknown format 0x%04x", format);
>> + goto out;
>> + }
>> +
>> + code = get_unaligned_be16(&skb->data[4]);
>> + if (code == ntohs(MV88E6XXX_RMU_RESP_ERROR)) {
>
> Please build with sparse, "make W=1 C=1". These are all wrong. The
> variables retrieved from packet headers should be __be16, and "htohs"
> (network to host) should be "htons" (host to network).
>

Will check.

>> + dev_err_ratelimited(chip->dev, "RMU: error response code 0x%04x", code);
>> + goto out;
>> + }
>> +
>> + pkt_dev = skb->data[6] & 0x1f;
>> + if (!dsa_switch_find(ds->dst->index, pkt_dev)) {
>
> What is the relation between the pkt_dev from here, the RMU header, and
> the source_device from dsa_inband_rcv_ll()? Are they always the same?
> Can they be different? You throw away the result from dsa_switch_find()
> in any case.
>

Will check.

>> + dev_err_ratelimited(chip->dev, "RMU: response from unknown chip with index %d\n",
>> + pkt_dev);
>> + goto out;
>> + }
>> +
>> + /* Check sequence number */
>> + if (seq_no != chip->rmu.seq_no) {
>> + dev_err_ratelimited(chip->dev, "RMU: wrong seqno received %d, expected %d",
>> + seq_no, chip->rmu.seq_no);
>> + goto out;
>> + }
>> +
>> + /* Check response code */
>> + switch (chip->rmu.request_cmd) {
>> + case MV88E6XXX_RMU_REQ_GET_ID: {
>> + if (code == MV88E6XXX_RMU_RESP_CODE_GOT_ID) {
>
> I'd expect htons to be used even here, with 0, for type consistency.
> The compiler should figure things out and not add extra code.
>

Thanks.

>> + prodnum = get_unaligned_be16(&skb->data[2]);
>> + chip->rmu.got_id = prodnum;
>> + dev_info_ratelimited(chip->dev, "RMU: received id OK with product number: 0x%04x\n",
>> + chip->rmu.got_id);
>> + } else {
>> + dev_dbg_ratelimited(chip->dev,
>> + "RMU: unknown response for GET_ID format 0x%04x code 0x%04x",
>> + format, code);
>> + }
>> + break;
>> + }
>> + case MV88E6XXX_RMU_REQ_DUMP_MIB:
>> + if (code == MV88E6XXX_RMU_RESP_CODE_DUMP_MIB &&
>> + !mv88e6xxx_validate_port_mac(ds, skb)) {
>> + pkt_prt = (skb->data[7] & 0x78) >> 3;
>> + port = &chip->ports[pkt_prt];
>
> It would be good if you could structure the code in such a way that you
> don't parse stuff from the packet twice, once in mv88e6xxx_validate_port_mac()
> and once here.
>

Agreed.

>> + if (!port) {
>> + dev_err_ratelimited(chip->dev, "RMU: illegal port number in response: %d\n",
>> + pkt_prt);
>> + goto out;
>> + }
>> +
>> + /* Copy whole array for further
>> + * processing according to chip type
>> + */
>> + for (i = 0; i < MV88E6XXX_RMU_MAX_RMON; i++)
>> + port->rmu_raw_stats[i] = get_unaligned_be32(&skb->data[12 + i * 4]);
>> + }
>> + break;
>> + default:
>> + dev_err_ratelimited(chip->dev,
>> + "RMU: unknown response format 0x%04x and code 0x%04x from chip %d\n",
>> + format, code, chip->ds->index);
>> + break;
>> + }
>> +
>> +out:
>> + complete(&chip->rmu.completion);
>> +
>> + return 0;
>> +}
>> +
>> +static int mv88e6xxx_rmu_tx(struct mv88e6xxx_chip *chip, int port,
>> + const char *msg, int len)
>> +{
>> + const struct dsa_device_ops *tag_ops;
>> + const struct dsa_port *dp;
>> + unsigned char *data;
>> + struct sk_buff *skb;
>> +
>> + dp = dsa_to_port(chip->ds, port);
>> + if (!dp || !dp->cpu_dp)
>> + return 0;
>> +
>> + tag_ops = dp->cpu_dp->tag_ops;
>> + if (!tag_ops)
>> + return -ENODEV;
>> +
>> + skb = netdev_alloc_skb(chip->rmu.netdev, 64);
>> + if (!skb)
>> + return -ENOMEM;
>> +
>> + skb_reserve(skb, 2 * ETH_HLEN + tag_ops->needed_headroom);
>
> If you decide to rework this using the master netdev, you can use
> dsa_tag_protocol_overhead(master->dsa_ptr->tag_ops). Or even reserve
> enough headroom for the larger header (EDSA) and be done with it.
> But then you need to construct a different header depending on whether
> DSA or EDSA is used.
>

So in the new version a la qca8k we need the 'extra' parameter to
see if we need space for EDSA header, thus we need run through the tagger.
We can discuss that in the next version.

>> + skb_reset_network_header(skb);
>> + skb->pkt_type = PACKET_OUTGOING;
>
> Could you please explain for me what will setting skb->pkt_type to
> PACKET_OUTGOING achieve?
>

I though it was prudent, will remove if it's not needed.

>> + skb->dev = chip->rmu.netdev;
>> +
>> + /* Create RMU L3 message */
>> + data = skb_put(skb, len);
>> + memcpy(data, msg, len);
>> +
>> + return tag_ops->inband_xmit(skb, dp->slave, ++chip->rmu.seq_no);
>> +}
>> +
>> +static int mv88e6xxx_rmu_send_wait(struct mv88e6xxx_chip *chip, int port,
>> + int request, const char *msg, int len)
>> +{
>> + const struct dsa_port *dp;
>> + int ret = 0;
>> +
>> + dp = dsa_to_port(chip->ds, port);
>> + if (!dp)
>> + return 0;
>> +
>> + mutex_lock(&chip->rmu.mutex);
>> +
>> + chip->rmu.request_cmd = request;
>> +
>> + ret = mv88e6xxx_rmu_tx(chip, port, msg, len);
>> + if (ret == -ENODEV) {
>> + /* Device not ready yet? Try again later */
>> + ret = 0;
>
> All the code paths in mv88e6xxx_rmu_tx() that return -ENODEV are
> fabricated reasons to have errors, IMO, and in a correct implementation
> we should never even get there. Please drop the special casing.
>

Re-worked in the next version.

>> + goto out;
>> + }
>> +
>> + if (ret) {
>> + dev_dbg(chip->dev, "RMU: error transmitting request (%d)", ret);
>
> Please have a normal log level for an error, dev_err().
> Also you can print the symbolic name for the error:
>
> dev_err(chip->dev, "RMU: error transmitting request (%pe)",
> ERR_PTR(ret));
>

Thanks.

>> + goto out;
>> + }
>> +
>> + ret = wait_for_completion_timeout(&chip->rmu.completion,
>> + msecs_to_jiffies(MV88E6XXX_WAIT_POLL_TIME_MS));
>> + if (ret == 0) {
>> + dev_dbg(chip->dev,
>> + "RMU: timeout waiting for request %d (%d) on dev:port %d:%d\n",
>> + request, ret, chip->ds->index, port);
>
> Again, please increase the log level of the error condition.
> Also, chip->ds->index is useless information, we have info about the
> switch via dev_dbg/dev_err.
>

Ok, thanks.

>> + ret = -ETIMEDOUT;
>> + }
>> +
>> +out:
>> + mutex_unlock(&chip->rmu.mutex);
>> +
>> + return ret > 0 ? 0 : ret;
>> +}
>> +
>> +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 };
>> + int ret = -1;
>> +
>> + if (chip->rmu.got_id)
>> + return 0;
>> +
>> + chip->rmu.netdev = dev_get_by_name(&init_net, "chan0");
>
> What if I don't have a slave device called "chan0"? I can't use the RMU?
>

Reworked in the next version.

>> + if (!chip->rmu.netdev) {
>> + dev_dbg(chip->dev, "RMU: unable to get interface");
>> + return -ENODEV;
>> + }
>> +
>> + 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 %d index %d\n", ret,
>> + chip->ds->index);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +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;
>> +
>> + if (!chip)
>> + return 0;
>
> How can "chip" be NULL?
>

Yes, Andrew pointed that out. Thanks.

>> +
>> + 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 %d dev %d:%d\n", ret,
>> + chip->ds->index, port);
>> + return ret;
>> + }
>> +
>> + /* Update MIB for port */
>> + if (chip->info->ops->stats_get_stats)
>> + return chip->info->ops->stats_get_stats(chip, port, data);
>> +
>> + return 0;
>> +}
>> +
>> +static struct mv88e6xxx_bus_ops mv88e6xxx_bus_ops = {
>
> static const struct
>
>> + .get_rmon = mv88e6xxx_rmu_stats_get,
>> +};
>> +
>> +int mv88e6xxx_rmu_init(struct mv88e6xxx_chip *chip)
>> +{
>> + dev_info(chip->dev, "RMU: setting up for switch@%d", chip->sw_addr);
>
> Not a whole lotta useful information brought by this. Also,
> chip->sw_addr is already visible via dev_info(chip->dev).
> I would just drop this.
>

Ok.

>> +
>> + init_completion(&chip->rmu.completion);
>> +
>> + mutex_init(&chip->rmu.mutex);
>
> I would just move these to mv88e6xxx_rmu_setup(), and move
> mv88e6xxx_rmu_setup() to rmu.c.
>
>> +
>> + chip->rmu.ops = &mv88e6xxx_bus_ops;
>
> By doing this (combined with the way in which chip->rmu.ops is actually
> tested for), you are saying that RMU is available since driver probe time.
> But it isn't. It's only available when the master goes up and is
> operational. So you're setting yourself up for lots of I/O failures in
> the beginning.
>

Reworked in the next version.

>> +
>> + return 0;
>> +}
>> +
>> +void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
>> + bool operational)
>> +{
>> + struct mv88e6xxx_chip *chip = ds->priv;
>> +
>> + if (operational)
>> + chip->rmu.ops = &mv88e6xxx_bus_ops;
>> + else
>> + chip->rmu.ops = NULL;
>> +}
>
> There is a subtle but very important point to be careful about here,
> which is compatibility with multiple CPU ports. If there is a second DSA
> master whose state flaps from up to down, this should not affect the
> fact that you can still use RMU over the first DSA master. But in your
> case it does, so this is a case of how not to write code that accounts
> for that.
>
> In fact, given this fact, I think your function prototypes for
> chip->info->ops->rmu_enable() are all wrong / not sufficiently
> reflective of what the hardware can do. If the hardware has a bit mask
> of ports on which RMU operations are possible, why hardcode using
> dsa_switch_upstream_port() and not look at which DSA masters/CPU ports
> are actually up? At least for the top-most switch. For downstream
> switches we can use dsa_switch_upstream_port(), I guess (even that can
> be refined, but I'm not aware of setups using multiple DSA links, where
> each DSA link ultimately goes to a different upstream switch).

This is also changed a la qca8k.