Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 31 Jul 2020 18:22:01 +0300
From:   Vadym Kochan <vadym.kochan@plvision.eu>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jiri Pirko <jiri@mellanox.com>,
        Ido Schimmel <idosch@mellanox.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Oleksandr Mazur <oleksandr.mazur@plvision.eu>,
        Serhiy Boiko <serhiy.boiko@plvision.eu>,
        Serhiy Pshyk <serhiy.pshyk@plvision.eu>,
        Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>,
        Taras Chornyi <taras.chornyi@plvision.eu>,
        Andrii Savka <andrii.savka@plvision.eu>,
        netdev <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mickey Rachamim <mickeyr@marvell.com>
Subject: Re: [net-next v4 1/6] net: marvell: prestera: Add driver for
 Prestera family ASIC devices
Message-ID: <20200731152201.GB10391@plvision.eu>
References: <20200727122242.32337-1-vadym.kochan@plvision.eu>
 <20200727122242.32337-2-vadym.kochan@plvision.eu>
 <CAHp75Ve-MyFg5QqHjywGk6X+v_F77HkRBuQsJ0Cx3WLJ5ZV43w@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75Ve-MyFg5QqHjywGk6X+v_F77HkRBuQsJ0Cx3WLJ5ZV43w@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from plvision.eu (217.20.186.93) by AM5PR0601CA0062.eurprd06.prod.outlook.com (2603:10a6:206::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3239.17 via Frontend Transport; Fri, 31 Jul 2020 15:22:04 +0000
X-Originating-IP: [217.20.186.93]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 0a779a36-d19f-46f5-225a-08d835657b88
X-MS-TrafficTypeDiagnostic: HE1P190MB0059:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <HE1P190MB00595C7DA21C676BA03A6898954E0@HE1P190MB0059.EURP190.PROD.OUTLOOK.COM>
X-MS-Oob-TLC-OOBClassifiers: OLM:8882;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: rXvIlZRRByHw3qu3kMpihGPKjRHYoO9uBTJPouaXks2wnr9d6etuRzUBue0gR2sa7eT8KbVs6tA42aSt5KYlJDacvSVRN+a/ygmF75WJElHnIPWt1lHUejSfQS+fw8SDwULHP5jJCG3tiAHh50IjDv0i6hzY6fE0CLmaYJsn6dqcWHzxum0OGYTrYv2cM67s25vcxtL+aZGzUXcMKotB1ikOO887oa4264nTtsWVw3HLSLrvcQY2w/YyOuGOI/sgMz4Y22i2rORi66GXLfq3kqrSMDwY3eSgehLt8AuY2uoh3VAiB0uAd8lqP1a0uaS9WweAkWtqRc0Ac5hDnoXAjQ==
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:HE1P190MB0539.EURP190.PROD.OUTLOOK.COM;PTR:;CAT:NONE;SFTY:;SFS:(396003)(346002)(366004)(376002)(39830400003)(136003)(53546011)(33656002)(6916009)(36756003)(8936002)(55016002)(7696005)(52116002)(8676002)(4326008)(83380400001)(44832011)(1076003)(26005)(956004)(8886007)(86362001)(66556008)(66946007)(186003)(66476007)(508600001)(2906002)(54906003)(6666004)(2616005)(16526019)(5660300002)(316002);DIR:OUT;SFP:1102;
X-MS-Exchange-AntiSpam-MessageData: SVnx+zfygwjf0qjCfvRwkj9ZJOfYvAsNic3kzzEkMnCVJgxStm9VItvYN5fD3Rbnux/O7d03UhtaqBQJ494ElB3OO0myn1J7yg5kAEAbFiez4T7x3iGY//Gfs8alZZ1vgUi9u7yHOvEnMh3ahWwZ23Qzr6tDCPI8FEt1rmUIRqiQv8vlgMz1IguM5aZia0bhmji5/9jDVddMa1eedl4vhYRWAnAppokQDanSpsyBtlJODfi+hRAqSHEcv5bYmg7chEJ7IHjxyve4fBRumOiqSmnQ/tvF8PVVw9Pi8+rNr8LPFS+ZdqL6hf7ItGzU6P/EnTu6t2IriVDIMmKtcJquP4HzVnsigyzpmtRWRDf4vSBsZQ9ipRO+Gud/yD1J8Kl8ov+vIAf8Xm4Nx13tNI2wIRPKbnPGK+SlA8aIsNoAXyvN2PYQfOe+d3vR6ut9mebGlfCaJKLhQ/CyPxLNEiOGyOT3g+bnoDwrFZ2Tj8RxRGvqwVce2a+3gbfL7UfC5PPMd+NzhWvVWZkRzXRymZY/GngDUMJWmHkAAe4CStEsU0V92FBQ15EXn65qA2FRy1wFjfQVncij1snoNK4EN9nJ0vlo72fqb0on2SdskwQf/bPODd5cLCEX5mW6ZQSDVkoz/rTfJ7kmKWzdE6PYE69ABg==
X-OriginatorOrg: plvision.eu
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a779a36-d19f-46f5-225a-08d835657b88
X-MS-Exchange-CrossTenant-AuthSource: HE1P190MB0539.EURP190.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jul 2020 15:22:06.8585
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 03707b74-30f3-46b6-a0e0-ff0a7438c9c4
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: OtdOyuJiME7alZiRV/cj+/QyHe0dxSiDQ7IftaR32hitQfFyq+B+2AfcEHmhkte4yg0cv0rYQv6r9f2xrtSOoSKpYdjonbUABKvbn0EgPsw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1P190MB0059
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Andy,

On Mon, Jul 27, 2020 at 03:59:13PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 27, 2020 at 3:23 PM Vadym Kochan <vadym.kochan@plvision.eu> wrote:
> >
> > Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
> > ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
> > wireless SMB deployment.
> >
> > The current implementation supports only boards designed for the Marvell
> > Switchdev solution and requires special firmware.
> >
> > The core Prestera switching logic is implemented in prestera_main.c,
> > there is an intermediate hw layer between core logic and firmware. It is
> > implemented in prestera_hw.c, the purpose of it is to encapsulate hw
> > related logic, in future there is a plan to support more devices with
> > different HW related configurations.
> >
> > This patch contains only basic switch initialization and RX/TX support
> > over SDMA mechanism.
> >
> > Currently supported devices have DMA access range <= 32bit and require
> > ZONE_DMA to be enabled, for such cases SDMA driver checks if the skb
> > allocated in proper range supported by the Prestera device.
> >
> > Also meanwhile there is no TX interrupt support in current firmware
> > version so recycling work is scheduled on each xmit.
> >
> > Port's mac address is generated from the switch base mac which may be
> > provided via device-tree (static one or as nvme cell), or randomly
> > generated.
> 
> ...
> 
> > Signed-off-by: Andrii Savka <andrii.savka@plvision.eu>
> > Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
> > Signed-off-by: Serhiy Boiko <serhiy.boiko@plvision.eu>
> > Signed-off-by: Serhiy Pshyk <serhiy.pshyk@plvision.eu>
> > Signed-off-by: Taras Chornyi <taras.chornyi@plvision.eu>
> > Signed-off-by: Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>
> > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> 
> This needs more work. You have to really understand the role of each
> person in the above list.
> I highly recommend (re-)read sections 11-13 of Submitting Patches.
> 
At least looks like I need to add these persons as Co-author's.

> ...
> 
> > +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> 
> The idea of SPDX is to have it as a separate (standalone) comment.
OK, thanks.

> 
> ...
> 
> > +enum prestera_event_type {
> > +       PRESTERA_EVENT_TYPE_UNSPEC,
> > +
> > +       PRESTERA_EVENT_TYPE_PORT,
> > +       PRESTERA_EVENT_TYPE_RXTX,
> > +
> > +       PRESTERA_EVENT_TYPE_MAX,
> 
> Commas in the terminators are not good.
> 
OK

> > +};
> 
> ...
> 
> > +#include "prestera_dsa.h"
> 
> The idea that you include more generic headers earlier than more custom ones.
> 
Thanks

> > +#include <linux/string.h>
> > +#include <linux/bitops.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/errno.h>
> 
> Perhaps ordered?
> 
alphabetical ?

> ...
> 
> > +/* TrgDev[4:0] = {Word0[28:24]} */
> 
> > + * SrcPort/TrgPort[7:0] = {Word2[20], Word1[11:10], Word0[23:19]}
> 
> > +/* bits 13:15 -- UP */
> 
> > +/* bits 0:11 -- VID */
> 
> These are examples of useless comments.
> 
OK, removed.

> ...
> 
> > +       dsa->vlan.is_tagged = (bool)FIELD_GET(PRESTERA_W0_IS_TAGGED, words[0]);
> > +       dsa->vlan.cfi_bit = (u8)FIELD_GET(PRESTERA_W1_CFI_BIT, words[1]);
> > +       dsa->vlan.vpt = (u8)FIELD_GET(PRESTERA_W0_VPT, words[0]);
> > +       dsa->vlan.vid = (u16)FIELD_GET(PRESTERA_W0_VID, words[0]);
> 
> Do you need those castings?
> 
Looks like not, because the struct fields are same type as cast'ed ones.

> ...
> 
> > +       struct prestera_msg_event_port *hw_evt;
> > +
> > +       hw_evt = (struct prestera_msg_event_port *)msg;
> 
> Can be one line I suppose.
> 
Yes, but I am trying to avoid line-breaking because of 80 chars
limitation.

> ...
> 
> > +       if (evt->id == PRESTERA_PORT_EVENT_STATE_CHANGED)
> > +               evt->port_evt.data.oper_state = hw_evt->param.oper_state;
> > +       else
> > +               return -EINVAL;
> > +
> > +       return 0;
> 
> Perhaps traditional pattern, i.e.
> 
>   if (...)
>     return -EINVAL;
>   ...
>   return 0;
> 
I am not sure if it is applicable here, because the error state here
is if 'evt->id' did not matched after all checks. Actually this is
simply a 'switch', but I use 'if' to have shorter code.

> ...
> 
> > +       err = fw_event_parsers[msg->type].func(buf, &evt);
> > +       if (!err)
> > +               eh.func(sw, &evt, eh.arg);
> 
> Ditto.
Makes sense.

> 
> > +       return err;
> 
> ...
> 
> > +       memcpy(&req.param.mac, mac, sizeof(req.param.mac));
> 
> Consider to use ether_addr_*() APIs instead of open-coded mem*() ones.
> 
> ...
> 
> > +#define PRESTERA_MTU_DEFAULT 1536
> 
> Don't we have global default for this?
> 
> ...
> 
> > +#define PRESTERA_STATS_DELAY_MS        msecs_to_jiffies(1000)
> 
> It's not _MS.
> 
> ...
> 
> > +       if (!is_up)
> > +               netif_stop_queue(dev);
> > +
> > +       err = prestera_hw_port_state_set(port, is_up);
> > +
> > +       if (is_up && !err)
> > +               netif_start_queue(dev);
> 
> Much better if will look lke
> 
>   if (is_up) {
>   ...
>   err  = ...(..., true);
>   if (err)
>     return err;
>   ...
>   } else {
>     return prestera_*(..., false);
>   }
>   return 0;
> 
> > +       return err;
> 
> ...
> 
> > +       /* Only 0xFF mac addrs are supported */
> > +       if (port->fp_id >= 0xFF)
> > +               goto err_port_init;
> 
> You meant 255, right?
> Otherwise you have to mentioned is it byte limitation or what?
> 
> ...
Yes, 255 is a limitation because of max byte value.

> 
> > +static int prestera_switch_set_base_mac_addr(struct prestera_switch *sw)
> > +{
> > +       struct device_node *base_mac_np;
> > +       struct device_node *np;
> 
> > +       np = of_find_compatible_node(NULL, NULL, "marvell,prestera");
> > +       if (np) {
> > +               base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
> > +               if (base_mac_np) {
> > +                       const char *base_mac;
> > +
> > +                       base_mac = of_get_mac_address(base_mac_np);
> > +                       of_node_put(base_mac_np);
> > +                       if (!IS_ERR(base_mac))
> > +                               ether_addr_copy(sw->base_mac, base_mac);
> > +               }
> > +       }
> > +
> > +       if (!is_valid_ether_addr(sw->base_mac)) {
> > +               eth_random_addr(sw->base_mac);
> > +               dev_info(sw->dev->dev, "using random base mac address\n");
> > +       }
> 
> Isn't it device_get_mac_address() reimplementation?
> 
device_get_mac_address() just tries to get mac via fwnode.

> > +
> > +       return prestera_hw_switch_mac_set(sw, sw->base_mac);
> > +}
> 
> ...
> 
> > +       err = prestera_switch_init(sw);
> > +       if (err) {
> > +               kfree(sw);
> > +               return err;
> > +       }
> > +
> > +       return 0;
> 
> if (err)
>  kfree(...);
> return err;
> 
> Also, check reference counting.
> 
> ...
> 
> > +#define PRESTERA_SDMA_RX_DESC_PKT_LEN(desc) \
> 
> > +       ((le32_to_cpu((desc)->word2) >> 16) & 0x3FFF)
> 
> Why not GENMASK() ?
Yes, GENMASK is right way to go, thanks.

> 
> ...
> 
> > +       if (dma + sizeof(struct prestera_sdma_desc) > sdma->dma_mask) {
> > +               dev_err(dma_dev, "failed to alloc desc\n");
> > +               dma_pool_free(sdma->desc_pool, desc, dma);
> 
> Better first undo something *then* print a message.
> 
> > +               return -ENOMEM;
> > +       }
> 
> ...
> 
> > +static void prestera_sdma_rx_desc_set_len(struct prestera_sdma_desc *desc,
> > +                                         size_t val)
> > +{
> > +       u32 word = le32_to_cpu(desc->word2);
> > +
> > +       word = (word & ~GENMASK(15, 0)) | val;
> 
> Shouldn't you do traditional pattern?
> 
> word = (word & ~mask) | (val & mask);
Looks like this is safer form.

> 
> > +       desc->word2 = cpu_to_le32(word);
> > +}
> 
> ...
> 
> > +       dma = dma_map_single(dev, skb->data, skb->len, DMA_FROM_DEVICE);
> 
> > +
> 
> Redundant blank line.
> 
> > +       if (dma_mapping_error(dev, dma))
> > +               goto err_dma_map;
> 
> ...
> 
> > +               pr_warn_ratelimited("received pkt for non-existent port(%u, %u)\n",
> > +                                   dev_id, hw_port);
> 
> netdev_warn_ratelimited() ? Or something closer to that?
> 
> ...
> 
> > +       qmask = GENMASK(qnum - 1, 0);
> 
> BIT(qnum) - 1 will produce much better code I suppose.
> 
> ...
> 
> > +       if (pkts_done < budget && napi_complete_done(napi, pkts_done))
> > +               prestera_write(sdma->sw, PRESTERA_SDMA_RX_INTR_MASK_REG,
> > +                              0xff << 2);
> 
> GENMASK() ?
> 
> ...
> 
> > +       word = (word & ~GENMASK(30, 16)) | ((len + ETH_FCS_LEN) << 16);
> 
> Consider traditional pattern.
> 
> ...
> 
> > +       word |= PRESTERA_SDMA_TX_DESC_DMA_OWN << 31;
> 
> I hope that was defined with U. Otherwise it's UB.
> 
> ...
> 
> > +       new_skb = alloc_skb(len, GFP_ATOMIC | GFP_DMA);
> 
> Atomic? Why?
> 
TX path might be called from net_tx_action which is softirq.

> ...
> 
> > +static int prestera_sdma_tx_wait(struct prestera_sdma *sdma,
> > +                                struct prestera_tx_ring *tx_ring)
> > +{
> 
> > +       int tx_retry_num = 10 * tx_ring->max_burst;
> 
> Magic!
You mean the code is magic ? Yes, I am trying to relax the
calling of SDMA engine.

> 
> > +       while (--tx_retry_num) {
> > +               if (prestera_sdma_is_ready(sdma))
> > +                       return 0;
> > +
> > +               udelay(1);
> > +       }
> 
> unsigned int counter = ...;
> 
> do { } while (--counter);
> 
> looks better.
> 
> Also, why udelay()? Is it atomic context?
TX path might be called from net_tx_action which is softirq.

> 
> > +       return -EBUSY;
> > +}
> 
> ...
> 
> > +       if (!tx_ring->burst--) {
> 
> Don't do like this. It makes code harder to understand.
> 
>   if (tx_ring->...) {
>     ...->burst--;
>   } else {
>     ...
>   }
I will try.

> 
> > +               tx_ring->burst = tx_ring->max_burst;
> > +
> > +               err = prestera_sdma_tx_wait(sdma, tx_ring);
> > +               if (err)
> > +                       goto drop_skb_unmap;
> > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko