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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YyYLpyOrmRM/Qj/n@lunn.ch>
Date:   Sat, 17 Sep 2022 20:02:15 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
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>, linux@...linux.org.uk,
        ansuelsmth@...il.com
Subject: Re: [PATCH net-next v13 4/6] net: dsa: mv88e6xxxx: Add RMU
 functionality.

On Fri, Sep 16, 2022 at 02:18:15PM +0200, Mattias Forsblad wrote:
> The Marvell SOHO switches supports a secondary control
> channel for accessing data in the switch. Special crafted
> ethernet frames can access functions in the switch.
> These frames is handled by the Remote Management Unit (RMU)
> in the switch. Accessing data structures is specially
> efficient and lessens the access contention on the MDIO
> bus.

Thanks for continually reworking this. It is nearly there now.

I said earlier, you want lots of small patches which are easy to
review. I would separate the MIB stuff into a separate patch.  This
patch can provide the basic infrastructure to send a request to the
RMU, and do basic validation of the reply. Then have patches which
make use of that. One such patch is getting the ID. Another patch is
getting statistics.

> +int mv88e6xxx_state_get_stats_rmu(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
> +{
> +	const u64 *stats = chip->ports[port].rmu_raw_stats;

...

> +static void mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
> +{
> +	u16 req[4] = { MV88E6XXX_RMU_REQ_FORMAT_SOHO,
> +		       MV88E6XXX_RMU_REQ_PAD,
> +		       MV88E6XXX_RMU_REQ_CODE_DUMP_MIB,
> +		       MV88E6XXX_RMU_REQ_DATA};
> +	struct mv88e6xxx_dump_mib_resp resp;
> +	struct mv88e6xxx_port *p;
> +	u8 resp_port;
> +	int resp_len;
> +	int num_mibs;
> +	int ret;
> +	int i;

...

> +	/* Copy MIB array for further processing according to chip type */
> +	num_mibs = (resp_len - offsetof(struct mv88e6xxx_dump_mib_resp, mib)) / sizeof(__be32);
> +	for (i = 0; i < num_mibs; i++)
> +		p->rmu_raw_stats[i] = be32_to_cpu(resp.mib[i]);
> +
> +	/* Update MIB for port */
> +	mv88e6xxx_state_get_stats_rmu(chip, port, data);

Why copy the stats into p->rmu_raw_stats[i] when you can just pass
resp to mv88e6xxx_state_get_stats_rmu().

This is left over from the way you used the old stats function to
decode the values. But now you have a dedicated function, you can
avoid this copy.

      Andrew