Messages in this thread | | | Date | Sat, 17 Nov 2018 01:30:02 +0100 | From | Andrew Lunn <> | Subject | Re: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC ethernet drivers |
| |
> +++ b/drivers/net/ethernet/freescale/enetc/Kconfig > @@ -0,0 +1,19 @@ > +# SPDX-License-Identifier: GPL-2.0 > +config FSL_ENETC > + tristate "ENETC PF driver" > + depends on PCI && PCI_MSI && ARCH_LAYERSCAPE
It would be good to add COMPILE_TEST.
> + help > + This driver supports NXP ENETC gigabit ethernet controller PCIe > + physical function (PF) devices, managing ENETC Ports at a privileged > + level. > + > + If compiled as module (M), the module name is fsl-enetc. > + > +config FSL_ENETC_VF > + tristate "ENETC VF driver" > + depends on PCI && PCI_MSI && ARCH_LAYERSCAPE
and here.
> +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) > +/* Copyright 2017-2018 NXP */ > + > +#include "enetc.h" > +#include <linux/tcp.h> > +#include <linux/udp.h> > +#include <linux/of_mdio.h> > + > +static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb); > +static void enetc_unmap_tx_buff(struct enetc_bdr *tx_ring, > + struct enetc_tx_swbd *tx_swbd); > +static int enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int budget); > + > +static struct sk_buff *enetc_map_rx_buff_to_skb(struct enetc_bdr *rx_ring, > + int i, u16 size); > +static void enetc_add_rx_buff_to_skb(struct enetc_bdr *rx_ring, int i, > + u16 size, struct sk_buff *skb); > +static void enetc_process_skb(struct enetc_bdr *rx_ring, struct sk_buff *skb); > +static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring, > + struct napi_struct *napi, int work_limit); > +
Please try to avoid forward declarations. Move the code around so you don't need them.
> +static bool enetc_tx_csum(struct sk_buff *skb, union enetc_tx_bd *txbd) > +{ > + int l3_start, l3_hsize; > + u16 l3_flags, l4_flags; > + > + if (skb->ip_summed != CHECKSUM_PARTIAL) > + return false; > + > + switch (skb->csum_offset) { > + case offsetof(struct tcphdr, check): > + l4_flags = ENETC_TXBD_L4_TCP; > + break; > + case offsetof(struct udphdr, check): > + l4_flags = ENETC_TXBD_L4_UDP; > + break; > + default: > + skb_checksum_help(skb); > + return false; > + } > + > + l3_start = skb_network_offset(skb); > + l3_hsize = skb_network_header_len(skb); > + > + l3_flags = 0; > + if (skb->protocol == htons(ETH_P_IPV6)) > + l3_flags = ENETC_TXBD_L3_IPV6;
Is there no need to do anything with IPv4?
> + > + /* write BD fields */ > + txbd->l3_csoff = enetc_txbd_l3_csoff(l3_start, l3_hsize, l3_flags); > + txbd->l4_csoff = l4_flags; > + > + return true; > +} > + > +static int enetc_setup_irqs(struct enetc_ndev_priv *priv) > +{ > + struct pci_dev *pdev = priv->si->pdev; > + int i, j, err; > + > + for (i = 0; i < priv->bdr_int_num; i++) { > + int irq = pci_irq_vector(pdev, ENETC_BDR_INT_BASE_IDX + i); > + struct enetc_int_vector *v = priv->int_vector[i]; > + int entry = ENETC_BDR_INT_BASE_IDX + i; > + struct enetc_hw *hw = &priv->si->hw; > + > + sprintf(v->name, "%s-rxtx%d", priv->ndev->name, i);
I would not be too surprised if static analyser people complain that you can in theory overflow name. You might want to use snprintf() to prevent this.
> + err = request_irq(irq, enetc_msix, 0, v->name, v); > + if (err) { > + dev_err(priv->dev, "request_irq() failed!\n"); > + goto irq_err; > + } > + > + v->tbier_base = hw->reg + ENETC_BDR(TX, 0, ENETC_TBIER); > + v->rbier = hw->reg + ENETC_BDR(RX, i, ENETC_RBIER); > + > + enetc_wr(hw, ENETC_SIMSIRRV(i), entry); > + > + for (j = 0; j < v->count_tx_rings; j++) { > + int idx = v->tx_ring[j].index; > + > + enetc_wr(hw, ENETC_SIMSITRV(idx), entry); > + } > + } > + > + return 0; > + > +irq_err: > + while (i--) > + free_irq(pci_irq_vector(pdev, ENETC_BDR_INT_BASE_IDX + i), > + priv->int_vector[i]); > + > + return err; > +}
> +static void adjust_link(struct net_device *ndev) > +{ > + struct phy_device *phydev = ndev->phydev; > + > + phy_print_status(phydev);
You normally need more than that. Maybe later patches add to this?
> +static int enetc_phy_connect(struct net_device *ndev) > +{ > + struct enetc_ndev_priv *priv = netdev_priv(ndev); > + struct phy_device *phydev; > + > + if (!priv->phy_node) > + return 0; /* phy-less mode */
What are your user-cases for phy-less? If you don't have a PHY it is mostly because you are connected to an Ethernet switch. In that case you use fixed-link, which gives you a phy.
> + > + phydev = of_phy_connect(ndev, priv->phy_node, &adjust_link, > + 0, priv->if_mode); > + if (!phydev) { > + dev_err(&ndev->dev, "could not attach to PHY\n"); > + return -ENODEV; > + } > + > + phy_attached_info(phydev); > + > + return 0; > +} > +
> +int enetc_ioctl(struct net_device *dev, struct ifreq *rq, int cmd) > +{ > + return -EINVAL; > +} > +
I think EOPNOTSUPP is more normal. But it should be O.K, to not have an ioctl handler at all.
> +int enetc_pci_probe(struct pci_dev *pdev, const char *name, int sizeof_priv) > +{ > + struct enetc_si *si, *p; > + struct enetc_hw *hw; > + size_t alloc_size; > + int err, len; > + > + err = pci_enable_device_mem(pdev); > + if (err) { > + dev_err(&pdev->dev, "device enable failed\n"); > + return err; > + } > + > + /* set up for high or low dma */ > + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > + if (err) { > + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > + if (err) { > + dev_err(&pdev->dev, > + "DMA configuration failed: 0x%x\n", err); > + goto err_dma; > + } > + }
Humm, i thought drivers were not supposed to do this. The architecture code should be setting masks. But i've not followed all those conversations...
> +static int enetc_get_reglen(struct net_device *ndev) > +{ > + struct enetc_ndev_priv *priv = netdev_priv(ndev); > + struct enetc_hw *hw = &priv->si->hw; > + int len; > + > + len = ARRAY_SIZE(enetc_si_regs); > + len += ARRAY_SIZE(enetc_txbdr_regs) * priv->num_tx_rings; > + len += ARRAY_SIZE(enetc_rxbdr_regs) * priv->num_rx_rings; > + > + if (hw->port) > + len += ARRAY_SIZE(enetc_port_regs); > + > + len *= sizeof(u32) * 2; /* store 2 etries per reg: addr and value */
entries
> + > + return len; > +} > +
Andrew
| |