From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46BA5C25B0E for ; Fri, 19 Aug 2022 05:29:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346321AbiHSF3F (ORCPT ); Fri, 19 Aug 2022 01:29:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46948 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244451AbiHSF3D (ORCPT ); Fri, 19 Aug 2022 01:29:03 -0400 Received: from mail-lf1-x131.google.com (mail-lf1-x131.google.com [IPv6:2a00:1450:4864:20::131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 941FC7C745 for ; Thu, 18 Aug 2022 22:29:01 -0700 (PDT) Received: by mail-lf1-x131.google.com with SMTP id b38so2160101lfv.4 for ; Thu, 18 Aug 2022 22:29:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc; bh=fm8jdU/sifb3BsydpGCJuPsk7trxpYRjyRX2OUvuSqU=; b=Yy24xCBN0njXVuhQnJXOyvmyb8caOFLY4/a3XT9FuFsRwir3mnWh4N+n7XwuZEl7Hs H5gTLzBNK8jFB8HoCSbc3IQfmGmLIiYo5gaYk2zUSrOXNO2Cfoo3pO1H+X9WCnM5ZGa4 4EfjSQzEnxiYJpHGVDQw01NgVfcyWxJ1EZwXalG8HkvrAqDX+ediTcHJ1aDlb91zthil Dz/3YmKf6eOfcyqrOFQTWKnNxyy5Qddzshe+EBLZWWOWjkxSMAILQ5xrF5ch5hJQeuZm LUzGoMsZnnR0lG9/mU54vHVnXCzLwGR2/LGqOOksf8B1r3yMXB/Kz5uq5Ej3dFZaUWi7 PDxg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=fm8jdU/sifb3BsydpGCJuPsk7trxpYRjyRX2OUvuSqU=; b=PIrRmtZd3paaIOxDaxMN3wgqOSNOXFzvcOe8+QNPPsAOfTE4uLHXowDHozwKI54z2r v63mxZ5EOGf+tWWio53+19BPjBOSyiKPeefvflad47zJtWQYDn3w7t9h60xSryVWwt4b BIwsIABzfmAM31idIseUt4slCe+f1K86kr1Ug1wWV/D2iASsW/DCijL5qMCJb6XOGwBz 0ENViuUAGh+XXFyQSS8Yxk4RYVAi+CEkVEEKyZTmRavrWoLlOKB+JHavUF5MBlrJgjVs oywIbaJJsaYJd0+EkBSC9qleFGHFfcqSnuOgq/A1gOAXmxNv1Nd23VpRwNuwhdrIdTRO yaTQ== X-Gm-Message-State: ACgBeo3RAkzcxDnPli2UoxIioMUvJb+ksJ4i8bQNEbS7YQC2LNkiNNwP O4iWuLQH8wSwulnZ1alK4tk= X-Google-Smtp-Source: AA6agR5jHqgBv5Cc1yTEdFnC7isES3RKhdwDZmUfrJhhh5DRyHxPSSVJbNgIq9Ke9ZyIu8+WqIz6aw== X-Received: by 2002:ac2:5a44:0:b0:48b:e17:3dd3 with SMTP id r4-20020ac25a44000000b0048b0e173dd3mr1774093lfn.337.1660886939879; Thu, 18 Aug 2022 22:28:59 -0700 (PDT) Received: from [10.0.1.14] (h-98-128-229-160.NA.cust.bahnhof.se. [98.128.229.160]) by smtp.gmail.com with ESMTPSA id p31-20020a056512139f00b0048ad0ad627asm494037lfa.128.2022.08.18.22.28.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 18 Aug 2022 22:28:59 -0700 (PDT) Message-ID: <15eb1ab3-18ae-cd65-5798-bf5af3b046c7@gmail.com> Date: Fri, 19 Aug 2022 07:28:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [RFC net-next PATCH 2/3] mv88e6xxx: Implement remote management support (RMU) Content-Language: en-US To: Andrew Lunn Cc: netdev@vger.kernel.org, Vivien Didelot , Florian Fainelli , Vladimir Oltean , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni References: <20220818102924.287719-1-mattias.forsblad@gmail.com> <20220818102924.287719-3-mattias.forsblad@gmail.com> From: Mattias Forsblad In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 >> + * >> + */ >> + >> +#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