| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20181117003002.GC752@lunn.ch>
Date: Sat, 17 Nov 2018 01:30:02 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Claudiu Manoil <claudiu.manoil@....com>
Cc: "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, alexandru.marginean@....com,
catalin.horghidan@....com
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
Powered by blists - more mailing lists