Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 27 Jul 2020 15:59:13 +0300 | Subject | Re: [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
| |