lkml.org 
[lkml]   [2020]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [net-next v4 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
    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.

    ...

    > +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0

    The idea of SPDX is to have it as a separate (standalone) comment.

    ...

    > +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.

    > +};

    ...

    > +#include "prestera_dsa.h"

    The idea that you include more generic headers earlier than more custom ones.

    > +#include <linux/string.h>
    > +#include <linux/bitops.h>
    > +#include <linux/bitfield.h>
    > +#include <linux/errno.h>

    Perhaps ordered?

    ...

    > +/* 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.

    ...

    > + 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?

    ...

    > + struct prestera_msg_event_port *hw_evt;
    > +
    > + hw_evt = (struct prestera_msg_event_port *)msg;

    Can be one line I suppose.

    ...

    > + 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;

    ...

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

    Ditto.

    > + 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?

    ...

    > +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?

    > +
    > + 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() ?

    ...

    > + 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);

    > + 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?

    ...

    > +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!

    > + 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?

    > + return -EBUSY;
    > +}

    ...

    > + if (!tx_ring->burst--) {

    Don't do like this. It makes code harder to understand.

    if (tx_ring->...) {
    ...->burst--;
    } else {
    ...
    }

    > + 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

    \
     
     \ /
      Last update: 2020-07-27 15:01    [W:4.700 / U:0.568 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site