Date:   Tue, 20 Sep 2022 01:39:33 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux@...linux.org.uk,
        ansuelsmth@...il.com
Subject: Re: [PATCH net-next v14 4/7] net: dsa: mv88e6xxxx: Add RMU
 functionality.

On Mon, Sep 19, 2022 at 01:08:44PM +0200, Mattias Forsblad wrote:
>  struct mv88e6xxx_bus_ops {
> diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
> new file mode 100644
> index 000000000000..c5b3c156de40
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/rmu.c
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Marvell 88E6xxx Switch Remote Management Unit Support
> + *
> + * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@...il.com>
> + *
> + */
> +
> +#include <asm/unaligned.h>

Nothing in this file uses asm/unaligned.h. On the other hand, it uses
struct dsa_switch, struct sk_buff, struct net_device, etc etc, which are
nowhere to be found in included headers. Don't rely on transitive header
inclusion. What rmu.h and global1.h include are just for the data
structures referenced within those headers to make sense when included
from any C file.

> +#include "rmu.h"
> +#include "global1.h"
> +
> +static const u8 mv88e6xxx_rmu_dest_addr[ETH_ALEN] = { 0x01, 0x50, 0x43, 0x00, 0x00, 0x00 };
> +
> +static void mv88e6xxx_rmu_create_l2(struct dsa_switch *ds, struct sk_buff *skb)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct ethhdr *eth;
> +	u8 *edsa_header;
> +	u8 *dsa_header;
> +	u8 extra = 0;
> +
> +	if (chip->tag_protocol == DSA_TAG_PROTO_EDSA)
> +		extra = 4;
> +
> +	/* Create RMU L2 header */
> +	dsa_header = skb_push(skb, 6);
> +	dsa_header[0] = FIELD_PREP(MV88E6XXX_CPU_CODE_MASK, MV88E6XXX_RMU);
> +	dsa_header[0] |= FIELD_PREP(MV88E6XXX_TRG_DEV_MASK, ds->index);
> +	dsa_header[1] = FIELD_PREP(MV88E6XXX_RMU_CODE_MASK, 1);
> +	dsa_header[1] |= FIELD_PREP(MV88E6XXX_RMU_L2_BYTE1_RESV, MV88E6XXX_RMU_L2_BYTE1_RESV_VAL);
> +	dsa_header[2] = FIELD_PREP(MV88E6XXX_RMU_PRIO_MASK, MV88E6XXX_RMU_PRIO);
> +	dsa_header[2] |= MV88E6XXX_RMU_L2_BYTE2_RESV;
> +	dsa_header[3] = ++chip->rmu.seqno;
> +	dsa_header[4] = 0;
> +	dsa_header[5] = 0;
> +
> +	/* Insert RMU MAC destination address /w extra if needed */
> +	eth = skb_push(skb, ETH_ALEN * 2 + extra);
> +	memcpy(eth->h_dest, mv88e6xxx_rmu_dest_addr, ETH_ALEN);
> +	ether_addr_copy(eth->h_source, chip->rmu.master_netdev->dev_addr);

Come on.... really? Do I need to spell out "ether_addr_copy()" twice
during review, for consecutive lines?

> +
> +	if (extra) {
> +		edsa_header = (u8 *)&eth->h_proto;
> +		edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
> +		edsa_header[1] = ETH_P_EDSA & 0xff;
> +		edsa_header[2] = 0x00;
> +		edsa_header[3] = 0x00;
> +	}
> +}
> +
> +static int mv88e6xxx_rmu_send_wait(struct mv88e6xxx_chip *chip,
> +				   const void *req, int req_len,
> +				   void *resp, unsigned int *resp_len)
> +{
> +	struct sk_buff *skb;
> +	unsigned char *data;
> +	int ret = 0;
> +
> +	skb = netdev_alloc_skb(chip->rmu.master_netdev, 64);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	/* Take height for an eventual EDSA header */
> +	skb_reserve(skb, 2 * ETH_HLEN + 4);
> +	skb_reset_network_header(skb);
> +
> +	/* Insert RMU request message */
> +	data = skb_put(skb, req_len);
> +	memcpy(data, req, req_len);
> +
> +	mv88e6xxx_rmu_create_l2(chip->ds, skb);
> +
> +	mutex_lock(&chip->rmu.mutex);
> +
> +	ret = dsa_switch_inband_tx(chip->ds, skb, NULL, MV88E6XXX_RMU_WAIT_TIME_MS);
> +	if (!ret) {
> +		dev_err(chip->dev, "RMU: error waiting for request (%pe)\n",
> +			ERR_PTR(ret));
> +		goto out;
> +	}
> +
> +	if (chip->rmu.resp->len > *resp_len) {
> +		ret = -EMSGSIZE;
> +	} else {
> +		*resp_len = chip->rmu.resp->len;
> +		memcpy(resp, chip->rmu.resp->data, chip->rmu.resp->len);
> +	}
> +
> +	kfree_skb(chip->rmu.resp);
> +	chip->rmu.resp = NULL;
> +
> +out:
> +	mutex_unlock(&chip->rmu.mutex);
> +
> +	return ret > 0 ? 0 : ret;
> +}

This function is mixing return values from dsa_switch_inband_tx()
(really wait_for_completion_timeout(), where 0 is timeout) with negative
return values. What should the caller check for success? Yes.

> +
> +static int mv88e6xxx_rmu_validate_response(struct mv88e6xxx_rmu_header *resp, int code)
> +{
> +	if (be16_to_cpu(resp->format) != MV88E6XXX_RMU_RESP_FORMAT_1 &&
> +	    be16_to_cpu(resp->format) != MV88E6XXX_RMU_RESP_FORMAT_2 &&
> +	    be16_to_cpu(resp->code) != code) {
> +		net_dbg_ratelimited("RMU: received unknown format 0x%04x code 0x%04x",
> +				    resp->format, resp->code);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +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 mv88e6xxx_rmu_header resp;
> +	int resp_len;
> +	int ret = -1;

Don't initialize variables you'll overwrite immediately afterwards.

There is at least one other place in this patch which does that and
which should therefore also be changed. I won't say where that is,
I want to force you to read your changes.

> +
> +	resp_len = sizeof(resp);
> +	ret = mv88e6xxx_rmu_send_wait(chip, req, sizeof(req),
> +				      &resp, &resp_len);
> +	if (ret) {
> +		dev_dbg(chip->dev, "RMU: error for command GET_ID %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	/* Got response */
> +	ret = mv88e6xxx_rmu_validate_response(&resp, MV88E6XXX_RMU_RESP_CODE_GOT_ID);
> +	if (ret)
> +		return ret;
> +
> +	chip->rmu.prodnr = be16_to_cpu(resp.prodnr);
> +
> +	return ret;

return 0.

> +}
> +
> +static void mv88e6xxx_disable_rmu(struct mv88e6xxx_chip *chip)
> +{
> +	chip->smi_ops = chip->rmu.smi_ops;
> +	chip->ds->ops = chip->rmu.ds_rmu_ops;

Who modifies chip->ds->ops? This smells trouble. Big, big trouble.

> +	chip->rmu.master_netdev = NULL;
> +
> +	if (chip->info->ops->rmu_disable)
> +		chip->info->ops->rmu_disable(chip);
> +}
> +
> +static int mv88e6xxx_enable_check_rmu(const struct net_device *master,
> +				      struct mv88e6xxx_chip *chip, int port)
> +{
> +	int ret;
> +
> +	chip->rmu.master_netdev = (struct net_device *)master;
> +
> +	/* Check if chip is alive */
> +	ret = mv88e6xxx_rmu_get_id(chip, port);
> +	if (!ret)
> +		return ret;

It's really nice that in the mv88e6xxx_enable_check_rmu() ->
mv88e6xxx_rmu_get_id() -> mv88e6xxx_rmu_send_wait() call path, first you
check for ret != 0 as meaning error, then for ret == 0 as meaning error.
So you catch a bit of everything.

> +
> +	chip->ds->ops = chip->rmu.ds_rmu_ops;
> +	chip->smi_ops = chip->rmu.smi_rmu_ops;
> +
> +	return 0;
> +}
> +
> +void mv88e6xxx_master_state_change(struct dsa_switch *ds, const struct net_device *master,
> +				   bool operational)
> +{
> +	struct dsa_port *cpu_dp = master->dsa_ptr;
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int port;
> +	int ret;
> +
> +	port = dsa_towards_port(ds, cpu_dp->ds->index, cpu_dp->index);
> +
> +	mv88e6xxx_reg_lock(chip);
> +
> +	if (operational && chip->info->ops->rmu_enable) {

This all needs to be rewritten. Like here, if the master is operational
but the chip->info->ops->rmu_enable method is not populated, you call
mv88e6xxx_disable_rmu(). Why?

> +		ret = chip->info->ops->rmu_enable(chip, port);
> +
> +		if (ret == -EOPNOTSUPP)
> +			goto out;
> +
> +		if (!ret) {
> +			dev_dbg(chip->dev, "RMU: Enabled on port %d", port);
> +
> +			ret = mv88e6xxx_enable_check_rmu(master, chip, port);
> +			if (!ret)
> +				goto out;

Or here, if this fails, you goto out, which does nothing special
compared to simply not checking the return code. But you don't disable
the RMU back.

> +
> +		} else {
> +			dev_err(chip->dev, "RMU: Unable to enable on port %d %pe",
> +				port, ERR_PTR(ret));
> +			goto out;
> +		}
> +	} else {
> +		mv88e6xxx_disable_rmu(chip);
> +	}
> +
> +out:
> +	mv88e6xxx_reg_unlock(chip);
> +}

Powered by blists - more mailing lists