| 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
| ||
|
Message-ID: <15eb1ab3-18ae-cd65-5798-bf5af3b046c7@gmail.com>
Date: Fri, 19 Aug 2022 07:28:58 +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: [RFC net-next PATCH 2/3] mv88e6xxx: Implement remote management
support (RMU)
On 2022-08-18 15:21, Andrew Lunn wrote:
>> static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>> {
>> + int ret = 0;
>> +
>> if (chip->info->ops->rmu_disable)
>> - return chip->info->ops->rmu_disable(chip);
>> + ret = chip->info->ops->rmu_disable(chip);
>>
>> - return 0;
>> + if (chip->info->ops->rmu_enable) {
>> + ret += chip->info->ops->rmu_enable(chip);
>> + ret += mv88e6xxx_rmu_init(chip);
>
> EINVAL + EOPNOTSUPP = hard to find error code/bug.
>
> I've not looked at rmu_enable() yet, but there are restrictions about
> what ports can be used with RMU. So you have to assume some boards use
> the wrong port for the CPU or upstream DSA port, and so will need to
> return an error code. But such an error code should not be fatal, MDIO
> can still be used.
>
Agree, I'll fix to handle that case.
>> + }
>> +
>> + return ret;
>> }
>>
>> +int mv88e6085_g1_rmu_enable(struct mv88e6xxx_chip *chip)
>> +{
>> + int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
>> + int upstream_port = -1;
>> +
>> + upstream_port = dsa_switch_upstream_port(chip->ds);
>> + dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);
>
> dev_dbg()
I'll tone it down.
>
>> + if (upstream_port < 0)
>> + return -1;
>
> EOPNOTSUPP.
>
Ofc. Thanks.
>> +
>> + switch (upstream_port) {
>> + case 9:
>> + val = MV88E6085_G1_CTL2_RM_ENABLE;
>> + break;
>> + case 10:
>> + val = MV88E6085_G1_CTL2_RM_ENABLE | MV88E6085_G1_CTL2_P10RM;
>> + break;
>
> Does 6085 really have 10 ports? It does!
>
:)
>> + default:
>> + break;
>
> return -EOPNOTSUPP.
>
Will fix.
>> + }
>> +
>> + return mv88e6xxx_g1_ctl2_mask(chip, MV88E6085_G1_CTL2_P10RM |
>> + MV88E6085_G1_CTL2_RM_ENABLE, val);
>> +}
>> +
>> int mv88e6352_g1_rmu_disable(struct mv88e6xxx_chip *chip)
>> {
>> return mv88e6xxx_g1_ctl2_mask(chip, MV88E6352_G1_CTL2_RMU_MODE_MASK,
>> MV88E6352_G1_CTL2_RMU_MODE_DISABLED);
>> }
>>
>> +int mv88e6352_g1_rmu_enable(struct mv88e6xxx_chip *chip)
>> +{
>> + int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
>> + int upstream_port;
>> +
>> + upstream_port = dsa_switch_upstream_port(chip->ds);
>> + dev_err(chip->dev, "RMU: Enabling on port %d", upstream_port);
>> + if (upstream_port < 0)
>> + return -1;
>> +
>> + switch (upstream_port) {
>> + case 4:
>> + val = MV88E6352_G1_CTL2_RMU_MODE_PORT_4;
>> + break;
>> + case 5:
>> + val = MV88E6352_G1_CTL2_RMU_MODE_PORT_5;
>> + break;
>> + case 6:
>> + val = MV88E6352_G1_CTL2_RMU_MODE_PORT_6;
>> + break;
>> + default:
>> + break;
>
> Same comments as above.
>
>
>> diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
>> new file mode 100644
>> index 000000000000..ac68eef12521
>> --- /dev/null
>> +++ b/drivers/net/dsa/mv88e6xxx/rmu.c
>> @@ -0,0 +1,256 @@
>> +// 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 "rmu.h"
>> +#include "global1.h"
>> +
>> +#define MAX_RMON 64
>> +#define RMON_REPLY 2
>> +
>> +#define RMU_REQ_GET_ID 1
>> +#define RMU_REQ_DUMP_MIB 2
>> +
>> +#define RMU_RESP_FORMAT_1 0x0001
>> +#define RMU_RESP_FORMAT_2 0x0002
>> +
>> +#define RMU_RESP_CODE_GOT_ID 0x0000
>> +#define RMU_RESP_CODE_DUMP_MIB 0x1020
>
> These should all go into rmu.h. Please also follow the naming
> convention, add the MV88E6XXX_ prefix.
>
>
Will add prefix.
>> +
>> +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;
>> + __be16 *prodnum;
>> + __be16 *format;
>> + __be16 *code;
>> + __be32 *mib_data;
>> + u8 pkt_dev;
>> + u8 pkt_prt;
>> + int i;
>
> Reverse christmass tree.
>
Will fix. Doesn't checkpatch find these?
>> +
>> + if (!skb || !chip)
>> + return 0;
>
> We generally don't do this sort of defensive programming. Can these be
> NULL?
>
Will investigate.
>> +
>> + /* Extract response data */
>> + format = (__be16 *)&skb->data[0];
>
> You have no idea of the alignment of data, so you should not cast it
> to a pointer type and dereference it. Take a look at the unaligned
> helpers.
>
Can you point me to an example please?
>> + if (*format != htons(RMU_RESP_FORMAT_1) && (*format != htons(RMU_RESP_FORMAT_2))) {
>> + dev_err(chip->dev, "RMU: received unknown format 0x%04x", *format);
>
> rate limit all error messages please.
>
Yes, will fix.
>> + goto out;
>> + }
>> +
>> + code = (__be16 *)&skb->data[4];
>> + if (*code == ntohs(0xffff)) {
>> + netdev_err(skb->dev, "RMU: error response code 0x%04x", *code);
>> + goto out;
>> + }
>> +
>> + pkt_dev = skb->data[6] & 0x1f;
>
> Please replace all these magic numbers with #define in rmu.h.
>
Will fix.
>> + if (pkt_dev >= DSA_MAX_SWITCHES) {
>> + netdev_err(skb->dev, "RMU: response from unknown chip %d\n", *code);
>> + goto out;
>> + }
>
> That is a good first step, but it is not sufficient to prove the
> switch actually exists.
>
Will do additional checks.
>> +
>> + /* Check sequence number */
>> + if (seq_no != chip->rmu.seq_no) {
>> + netdev_err(skb->dev, "RMU: wrong seqno received %d, expected %d",
>> + seq_no, chip->rmu.seq_no);
>> + goto out;
>> + }
>> +
>> + /* Check response code */
>> + switch (chip->rmu.request_cmd) {
>
> Maybe a non-issue, i've not looked hard enough: What is the
> relationship between ds passed to this function and pkt_dev? Does ds
> belong to pkt_dev, or is ds the chip connected to the host? There are
> a few boards with multiple chip in a tree, so we need to get this
> mapping correct. This is something i can test sometime later, i have
> such boards.
>
I've tested this implementation in a system with multiple switchcores
and it works.
>> + case RMU_REQ_GET_ID: {
>> + if (*code == RMU_RESP_CODE_GOT_ID) {
>> + prodnum = (__be16 *)&skb->data[2];
>> + chip->rmu.got_id = *prodnum;
>> + dev_info(chip->dev, "RMU: received id OK with product number: 0x%04x\n",
>> + chip->rmu.got_id);
>> + } else {
>> + dev_err(chip->dev,
>> + "RMU: unknown response for GET_ID format 0x%04x code 0x%04x",
>> + *format, *code);
>> + }
>> + break;
>> + }
>> + case RMU_REQ_DUMP_MIB:
>> + if (*code == RMU_RESP_CODE_DUMP_MIB) {
>> + pkt_prt = (skb->data[7] & 0x78) >> 3;
>> + mib_data = (__be32 *)&skb->data[12];
>> + port = &chip->ports[pkt_prt];
>> + if (!port) {
>> + dev_err(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 < MAX_RMON; i++)
>> + port->rmu_raw_stats[i] = mib_data[i];
>
> This needs some more thought, which i don't have time for right now.
>
> Andrew
Powered by blists - more mailing lists