* Re: [PATCH net-next v2 04/10] net: phy: add Broadcom BCM7xxx internal PHY driver
From: Francois Romieu @ 2014-02-13 10:34 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, davem, cernekee, devicetree
In-Reply-To: <1392269395-23513-5-git-send-email-f.fainelli@gmail.com>
Florian Fainelli <f.fainelli@gmail.com> :
[...]
> diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
> new file mode 100644
> index 0000000..6aea6e2
> --- /dev/null
> +++ b/drivers/net/phy/bcm7xxx.c
[...]
> +static int bcm7445_config_init(struct phy_device *phydev)
> +{
> + int ret;
It could be declared after 'i' below.
> + const struct bcm7445_regs {
static const
> + int reg;
> + u16 value;
> + } bcm7445_regs_cfg[] = {
> + /* increases ADC latency by 24ns */
> + { 0x17, 0x0038 },
> + { 0x15, 0xAB95 },
> + /* increases internal 1V LDO voltage by 5% */
> + { 0x17, 0x2038 },
> + { 0x15, 0xBB22 },
> + /* reduce RX low pass filter corner frequency */
> + { 0x17, 0x6038 },
> + { 0x15, 0xFFC5 },
> + /* reduce RX high pass filter corner frequency */
> + { 0x17, 0x003a },
> + { 0x15, 0x2002 },
> + };
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(bcm7445_regs_cfg); i++) {
> + ret = phy_write(phydev,
> + bcm7445_regs_cfg[i].reg,
> + bcm7445_regs_cfg[i].value);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void phy_write_exp(struct phy_device *phydev,
> + u16 reg, u16 value)
static void phy_write_exp(struct phy_device *phydev, u16 reg, u16 value)
> +{
> + phy_write(phydev, 0x17, 0xf00 | reg);
> + phy_write(phydev, 0x15, value);
> +}
> +
> +static void phy_write_misc(struct phy_device *phydev,
> + u16 reg, u16 chl, u16 value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ all tabs that don't line up
static void phy_write_misc(struct phy_device *phydev,
u16 reg, u16 chl, u16 value)
static void phy_write_misc(struct phy_device *phydev, u16 reg, u16 chl,
u16 value)
static void phy_write_misc(struct phy_device *phydev, u16 reg, u16 chl, u16 val)
> +{
> + int tmp;
> +
> + phy_write(phydev, 0x18, 0x7);
> +
> + tmp = phy_read(phydev, 0x18);
> + tmp |= 0x800;
> + phy_write(phydev, 0x18, tmp);
> +
> + tmp = (chl * 0x2000) | reg;
> + phy_write(phydev, 0x17, tmp);
> +
> + phy_write(phydev, 0x15, value);
You may use some #define for the 0x15, 0x17 and 0x18 values.
> +}
> +
> +static int bcm7xxx_28nm_afe_config_init(struct phy_device *phydev)
> +{
> + /* write AFE_RXCONFIG_0 */
> + phy_write_misc(phydev, 0x38, 0x0000, 0xeb19);
> +
> + /* write AFE_RXCONFIG_1 */
> + phy_write_misc(phydev, 0x38, 0x0001, 0x9a3f);
> +
> + /* write AFE_RX_LP_COUNTER */
> + phy_write_misc(phydev, 0x38, 0x0003, 0x7fc7);
> +
> + /* write AFE_HPF_TRIM_OTHERS */
> + phy_write_misc(phydev, 0x3A, 0x0000, 0x000b);
> +
> + /* write AFTE_TX_CONFIG */
> + phy_write_misc(phydev, 0x39, 0x0000, 0x0800);
Some #define may be welcome to replace the comments.
[...]
> +static int bcm7xxx_28nm_config_init(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = bcm7445_config_init(phydev);
> + if (ret)
> + return ret;
> +
> + return bcm7xxx_28nm_afe_config_init(phydev);
> +}
> +
> +static int phy_set_clr_bits(struct phy_device *dev, int location,
> + int set_mask, int clr_mask)
> +{
> + int v, ret;
> +
> + v = phy_read(dev, location);
> + if (v < 0)
> + return v;
> +
> + v &= ~clr_mask;
> + v |= set_mask;
> +
> + ret = phy_write(dev, location, v);
> + if (ret < 0)
> + return ret;
> +
> + return v;
> +}
> +
> +static int bcm7xxx_config_init(struct phy_device *phydev)
> +{
> + /* Enable 64 clock MDIO */
> + phy_write(phydev, 0x1d, 0x1000);
> + phy_read(phydev, 0x1d);
> +
> + /* Workaround only required for 100Mbits/sec */
> + if (!(phydev->dev_flags & PHY_BRCM_100MBPS_WAR))
> + return 0;
> +
> + /* set shadow mode 2 */
> + phy_set_clr_bits(phydev, 0x1f, 0x0004, 0x0004);
phy_set_clr_bits returned status code is not checked.
> +
> + /* set iddq_clkbias */
> + phy_write(phydev, 0x14, 0x0F00);
> + udelay(10);
> +
> + /* reset iddq_clkbias */
> + phy_write(phydev, 0x14, 0x0C00);
> +
> + phy_write(phydev, 0x13, 0x7555);
> +
> + /* reset shadow mode 2 */
> + phy_set_clr_bits(phydev, 0x1f, 0x0004, 0);
phy_set_clr_bits returned status code is not checked.
> +
> + return 0;
> +}
> +
> +/* Workaround for putting the PHY in IDDQ mode, required
> + * for all BCM7XXX PHYs
> + */
> +static int bcm7xxx_suspend(struct phy_device *phydev)
Factor out with bcm7445_config_init and some helper ?
> +{
> + int ret;
> + const struct bcm7xxx_regs {
> + int reg;
> + u16 value;
> + } bcm7xxx_suspend_cfg[] = {
> + { 0x1f, 0x008b },
> + { 0x10, 0x01c0 },
> + { 0x14, 0x7000 },
> + { 0x1f, 0x000f },
> + { 0x10, 0x20d0 },
> + { 0x1f, 0x000b },
> + };
> + unsigned int i;
--
Ueimor
^ permalink raw reply
* Re: [PATCH net-next v2 06/10] net: bcmgenet: add main driver file
From: Francois Romieu @ 2014-02-13 10:35 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, davem, cernekee, devicetree
In-Reply-To: <1392269395-23513-7-git-send-email-f.fainelli@gmail.com>
Florian Fainelli <f.fainelli@gmail.com> :
[...]
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> new file mode 100644
> index 0000000..ab71e81
> --- /dev/null
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
[...]
> +static int bcmgenet_set_rx_csum(struct net_device *dev,
> + netdev_features_t wanted)
> +{
> + struct bcmgenet_priv *priv = netdev_priv(dev);
> + u32 rbuf_chk_ctrl;
> + int rx_csum_en;
> +
> + rx_csum_en = !!(wanted & NETIF_F_RXCSUM);
It's a bool.
> +
> + spin_lock_bh(&priv->bh_lock);
> + rbuf_chk_ctrl = bcmgenet_rbuf_readl(priv, RBUF_CHK_CTRL);
> +
> + /* enable rx checksumming */
> + if (!rx_csum_en)
> + rbuf_chk_ctrl &= ~RBUF_RXCHK_EN;
> + else
> + rbuf_chk_ctrl |= RBUF_RXCHK_EN;
> + priv->desc_rxchk_en = rx_csum_en;
> + bcmgenet_rbuf_writel(priv, rbuf_chk_ctrl, RBUF_CHK_CTRL);
> +
> + spin_unlock_bh(&priv->bh_lock);
> +
> + return 0;
> +}
> +static int bcmgenet_set_tx_csum(struct net_device *dev,
Missing empty line.
[...]
> +static void bcmgenet_update_mib_counters(struct bcmgenet_priv *priv)
> +{
> + int i, j = 0;
> +
> + for (i = 0; i < BCMGENET_STATS_LEN; i++) {
> + const struct bcmgenet_stats *s;
> + u32 val = 0;
> + char *p;
> + u8 offset = 0;
Xmas tree please.
[...]
> +static void bcmgenet_get_ethtool_stats(struct net_device *dev,
> + struct ethtool_stats *stats,
> + u64 *data)
> +{
> + struct bcmgenet_priv *priv = netdev_priv(dev);
> + int i;
> +
> + mutex_lock(&priv->mib_mutex);
> + if (netif_running(dev))
> + bcmgenet_update_mib_counters(priv);
> +
> + for (i = 0; i < BCMGENET_STATS_LEN; i++) {
> + const struct bcmgenet_stats *s;
> + char *p;
> +
> + s = &bcmgenet_gstrings_stats[i];
> + if (s->type == BCMGENET_STAT_NETDEV)
> + p = (char *)&dev->stats;
> + else
> + p = (char *)priv;
> + p += s->stat_offset;
> + data[i] = *(u32 *)p;
> + }
> + mutex_unlock(&priv->mib_mutex);
The mutex is not used anywhere else and dev_ethtool runs under RTNL.
[...]
> +/* Assign skb to RX DMA descriptor. */
> +static int bcmgenet_alloc_rx_buffers(struct bcmgenet_priv *priv)
> +{
> + struct enet_cb *cb;
Wrong scope.
> + int ret = 0;
> + int i;
> + u32 reg;
> +
> + netif_dbg(priv, hw, priv->dev, "%s:\n", __func__);
> +
> + /* This function may be called from irq bottom-half. */
> + spin_lock_bh(&priv->bh_lock);
The Rx part of the NAPI handler directly calls bcmgenet_rx_refill through
bcmgenet_desc_rx. bcmgenet_poll does not sync with bh_lock.
Either some factoring was forgotten or some legacy locking / comment was
left in place (there should not be any ->open vs ->poll race)
> +
> + /* loop here for each buffer needing assign */
> + for (i = 0; i < priv->num_rx_bds; i++) {
> + cb = &priv->rx_cbs[priv->rx_bd_assign_index];
> + if (cb->skb)
> + continue;
> +
> + /* set the DMA descriptor length once and for all
> + * it will only change if we support dynamically sizing
> + * priv->rx_buf_len, but we do not
> + */
> + dmadesc_set_length_status(priv, priv->rx_bd_assign_ptr,
> + priv->rx_buf_len << DMA_BUFLENGTH_SHIFT);
> +
> + ret = bcmgenet_rx_refill(priv, cb);
> + if (ret)
> + break;
> +
> + }
> +
> + /* Enable rx DMA incase it was disabled due to running out of rx BD */
Nit: nothing proves even a single descriptor suceeded allocation.
[...]
> +static int reset_umac(struct bcmgenet_priv *priv)
> +{
> + struct device *kdev = &priv->pdev->dev;
> + unsigned int timeout = 0;
> + u32 reg;
> +
> + /* 7358a0/7552a0: bad default in RBUF_FLUSH_CTRL.umac_sw_rst */
> + bcmgenet_rbuf_ctrl_set(priv, 0);
> + udelay(10);
> +
> + /* disable MAC while updating its registers */
> + bcmgenet_umac_writel(priv, 0, UMAC_CMD);
> +
> + /* issue soft reset, wait for it to complete */
> + bcmgenet_umac_writel(priv, CMD_SW_RESET, UMAC_CMD);
> + while (timeout++ < 1000) {
> + reg = bcmgenet_umac_readl(priv, UMAC_CMD);
> + if (!(reg & CMD_SW_RESET))
> + break;
return 0;
> + udelay(1);
> + }
> +
> + if (timeout == 1000) {
> + dev_err(kdev,
> + "timeout waiting for MAC to come out of resetn\n");
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> +/* init_umac: Initializes the uniMac controller */
Useless.
> +static int init_umac(struct bcmgenet_priv *priv)
> +{
[...]
> +static void bcmgenet_init_multiq(struct net_device *dev)
> +{
> + struct bcmgenet_priv *priv = netdev_priv(dev);
> + unsigned int i, dma_enable;
> + u32 reg, dma_ctrl, ring_cfg = 0, dma_priority = 0;
> +
> + if (!netif_is_multiqueue(dev)) {
> + netdev_warn(dev, "called with non multi queue aware HW\n");
> + return;
> + }
> +
> + dma_ctrl = bcmgenet_tdma_readl(priv, DMA_CTRL);
> + dma_enable = dma_ctrl & DMA_EN;
> + dma_ctrl &= ~DMA_EN;
> + bcmgenet_tdma_writel(priv, dma_ctrl, DMA_CTRL);
> +
> + /* Enable strict priority arbiter mode */
> + bcmgenet_tdma_writel(priv, DMA_ARBITER_SP, DMA_ARB_CTRL);
> +
> + for (i = 0; i < priv->hw_params->tx_queues; i++) {
> + /* first 64 tx_cbs are reserved for default tx queue
> + * (ring 16)
> + */
> + bcmgenet_init_tx_ring(priv, i, priv->hw_params->bds_cnt,
> + i * priv->hw_params->bds_cnt,
> + (i + 1) * priv->hw_params->bds_cnt);
> +
> + /* Configure ring as decriptor ring and setup priority */
> + ring_cfg |= (1 << i);
> + dma_priority |= ((GENET_Q0_PRIORITY + i) <<
> + (GENET_MAX_MQ_CNT + 1) * i);
> + dma_ctrl |= (1 << (i + DMA_RING_BUF_EN_SHIFT));
Excess parenthesis.
[...]
> +/* NAPI polling method*/
> +static int bcmgenet_poll(struct napi_struct *napi, int budget)
> +{
> + struct bcmgenet_priv *priv = container_of(napi,
> + struct bcmgenet_priv, napi);
> + unsigned int work_done;
> +
> + work_done = bcmgenet_desc_rx(priv, budget);
> +
> + /* tx reclaim */
> + bcmgenet_tx_reclaim(priv->dev, &priv->tx_rings[DESC_INDEX]);
You may move the quick Tx reclaim before the slower Rx protocol processing.
> + /* Advancing our consumer index*/
> + priv->rx_c_index += work_done;
> + priv->rx_c_index &= DMA_C_INDEX_MASK;
> + bcmgenet_rdma_ring_writel(priv, DESC_INDEX,
> + priv->rx_c_index, RDMA_CONS_INDEX);
> + if (work_done < budget) {
> + napi_complete(napi);
> + bcmgenet_intrl2_0_writel(priv,
> + UMAC_IRQ_RXDMA_BDONE, INTRL2_CPU_MASK_CLEAR);
> + }
> +
> + return work_done;
> +}
> +
> +/* Interrupt bottom half */
> +static void bcmgenet_irq_task(struct work_struct *work)
> +{
> + struct bcmgenet_priv *priv = container_of(
> + work, struct bcmgenet_priv, bcmgenet_irq_work);
> + struct net_device *dev;
struct net_device *dev = priv->dev;
> + u32 reg;
> +
> + dev = priv->dev;
> +
> + netif_dbg(priv, intr, dev, "%s\n", __func__);
> + /* Cable plugged/unplugged event */
> + if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL) {
> + if (priv->irq0_stat & UMAC_IRQ_PHY_DET_R) {
> + priv->irq0_stat &= ~UMAC_IRQ_PHY_DET_R;
> + netif_crit(priv, link, dev,
> + "cable plugged in, powering up\n");
> + bcmgenet_power_up(priv, GENET_POWER_CABLE_SENSE);
> + } else if (priv->irq0_stat & UMAC_IRQ_PHY_DET_F) {
> + priv->irq0_stat &= ~UMAC_IRQ_PHY_DET_F;
> + netif_crit(priv, link, dev,
> + "cable unplugged, powering down\n");
> + bcmgenet_power_down(priv, GENET_POWER_CABLE_SENSE);
> + }
> + }
> + if (priv->irq0_stat & UMAC_IRQ_MPD_R) {
> + priv->irq0_stat &= ~UMAC_IRQ_MPD_R;
> + netif_crit(priv, wol, dev,
> + "magic packet detected, waking up\n");
> + /* disable mpd interrupt */
> + bcmgenet_intrl2_0_writel(priv,
> + UMAC_IRQ_MPD_R, INTRL2_CPU_MASK_SET);
> + /* disable CRC forward.*/
> + reg = bcmgenet_umac_readl(priv, UMAC_CMD);
> + reg &= ~CMD_CRC_FWD;
> + bcmgenet_umac_writel(priv, reg, UMAC_CMD);
> + priv->crc_fwd_en = 0;
> + bcmgenet_power_up(priv, GENET_POWER_WOL_MAGIC);
> +
> + } else if (priv->irq0_stat & (UMAC_IRQ_HFB_SM | UMAC_IRQ_HFB_MM)) {
> + priv->irq0_stat &= ~(UMAC_IRQ_HFB_SM | UMAC_IRQ_HFB_MM);
> + netif_crit(priv, wol, dev,
> + "ACPI pattern matched, waking up\n");
> + /* disable HFB match interrupts */
> + bcmgenet_intrl2_0_writel(priv,
> + UMAC_IRQ_HFB_SM | UMAC_IRQ_HFB_MM, INTRL2_CPU_MASK_SET);
> + bcmgenet_power_up(priv, GENET_POWER_WOL_ACPI);
> + }
It smells of half-backed wol / runtime power support. Imvho it deserves
some comment in the changelog message to hint about its maturity.
> +
> + /* Link UP/DOWN event */
> + if ((priv->hw_params->flags & GENET_HAS_MDIO_INTR) &&
> + (priv->irq0_stat & (UMAC_IRQ_LINK_UP|UMAC_IRQ_LINK_DOWN))) {
> + if (priv->phydev)
> + phy_mac_interrupt(priv->phydev,
> + (priv->irq0_stat & UMAC_IRQ_LINK_UP));
> + priv->irq0_stat &= ~(UMAC_IRQ_LINK_UP|UMAC_IRQ_LINK_DOWN);
> + }
> +}
> +
> +/* bcmgenet_isr1: interrupt handler for ring buffer. */
> +static irqreturn_t bcmgenet_isr1(int irq, void *dev_id)
> +{
> + struct bcmgenet_priv *priv = dev_id;
> + unsigned int index;
Wrong scope.
> +
> + /* Save irq status for bottom-half processing. */
> + priv->irq1_stat =
> + bcmgenet_intrl2_1_readl(priv, INTRL2_CPU_STAT) &
> + ~priv->int1_mask;
> + /* clear inerrupts*/
> + bcmgenet_intrl2_1_writel(priv, priv->irq1_stat, INTRL2_CPU_CLEAR);
> +
> + netif_dbg(priv, intr, priv->dev,
> + "%s: IRQ=0x%x\n", __func__, priv->irq1_stat);
> + /* Check the MBDONE interrupts.
> + * packet is done, reclaim descriptors
> + */
> + if (priv->irq1_stat & 0x0000ffff) {
> + index = 0;
> + for (index = 0; index < 16; index++) {
Proofread patrol alert :o)
(...]
> +static int bcmgenet_dma_teardown(struct bcmgenet_priv *priv)
> +{
> + int timeout = 0;
> + u32 reg;
> +
> + /* Disable TDMA to stop add more frames in TX DMA */
> + reg = bcmgenet_tdma_readl(priv, DMA_CTRL);
> + reg &= ~DMA_EN;
> + bcmgenet_tdma_writel(priv, reg, DMA_CTRL);
> +
> + /* Check TDMA status register to confirm TDMA is disabled */
> + while (!(bcmgenet_tdma_readl(priv, DMA_STATUS) & DMA_DISABLED)) {
> + if (timeout++ == 5000) {
> + netdev_warn(priv->dev,
> + "Timed out while disabling TX DMA\n");
> + return -ETIMEDOUT;
> + }
> + udelay(1);
> + }
On the stylistic side, the driver hesitates between "while", "for" timeout
and conditions loop. I'd go for the boring "int i; for (i = 0; i < max; i++)"
style but it's your call.
On the worrying side, even if Tx DMA does not stop, the driver should
try to disable Rx DMA.
> +
> + /* Wait 10ms for packet drain in both tx and rx dma */
> + usleep_range(10000, 20000);
> +
> + /* Disable RDMA */
> + reg = bcmgenet_rdma_readl(priv, DMA_CTRL);
> + reg &= ~DMA_EN;
> + bcmgenet_rdma_writel(priv, reg, DMA_CTRL);
> +
> + timeout = 0;
> + /* Check RDMA status register to confirm RDMA is disabled */
> + while (!(bcmgenet_rdma_readl(priv, DMA_STATUS) & DMA_DISABLED)) {
> + if (timeout++ == 5000) {
> + netdev_warn(priv->dev,
> + "Timed out while disabling RX DMA\n");
> + return -ETIMEDOUT;
> + }
> + udelay(1);
> + }
> +
> + return 0;
> +}
[...]
> +static void bcmgenet_timeout(struct net_device *dev)
> +{
> + struct bcmgenet_priv *priv = netdev_priv(dev);
> +
> + BUG_ON(dev == NULL);
dev == NULL should be noticed quickly.
> +
> + netif_dbg(priv, tx_err, dev, "bcmgenet_timeout\n");
> +
> + dev->trans_start = jiffies;
> +
> + dev->stats.tx_errors++;
> +
> + netif_tx_wake_all_queues(dev);
dev_watchdog already complains (loudly).
Is it really supposed to recover ?
> +}
> +
> +#define MAX_MC_COUNT 16
> +
> +static inline void bcmgenet_set_mdf_addr(struct bcmgenet_priv *priv,
> + unsigned char *addr,
> + int *i,
> + int *mc)
> +{
> + u32 reg;
> +
> + bcmgenet_umac_writel(priv, addr[0] << 8 | addr[1],
> + UMAC_MDF_ADDR + (*i * 4));
> + bcmgenet_umac_writel(priv,
> + addr[2] << 24 | addr[3] << 16 |
> + addr[4] << 8 | addr[5],
> + UMAC_MDF_ADDR + ((*i + 1) * 4));
I would not expect such an indent to pass beyond davem.
> + reg = bcmgenet_umac_readl(priv, UMAC_MDF_CTRL);
> + reg |= (1 << (MAX_MC_COUNT - *mc));
> + bcmgenet_umac_writel(priv, reg, UMAC_MDF_CTRL);
> + *i += 2;
> + (*mc)++;
> +}
> +
> +static void bcmgenet_set_rx_mode(struct net_device *dev)
> +{
> + struct bcmgenet_priv *priv = netdev_priv(dev);
> + struct netdev_hw_addr *ha;
> + int i, mc;
> + u32 reg;
> +
> + netif_dbg(priv, hw, dev, "%s: %08X\n", __func__, dev->flags);
> +
> + /* Promiscous mode */
> + reg = bcmgenet_umac_readl(priv, UMAC_CMD);
> + if (dev->flags & IFF_PROMISC) {
> + reg |= CMD_PROMISC;
> + bcmgenet_umac_writel(priv, reg, UMAC_CMD);
> + bcmgenet_umac_writel(priv, 0, UMAC_MDF_CTRL);
> + return;
> + } else {
> + reg &= ~CMD_PROMISC;
> + bcmgenet_umac_writel(priv, reg, UMAC_CMD);
> + }
> +
> + /* UniMac doesn't support ALLMULTI */
> + if (dev->flags & IFF_ALLMULTI)
> + return;
The driver did not fulfill the request. It could complain to help user.
[...]
> +static int bcmgenet_set_mac_addr(struct net_device *dev, void *p)
> +{
> + struct sockaddr *addr = p;
> +
> + if (netif_running(dev))
> + return -EBUSY;
Add a comment to specify if it is a hardware shortcoming or something else ?
> +
> + ether_addr_copy(dev->dev_addr, addr->sa_data);
> +
> + return 0;
> +}
[...]
> +static const struct net_device_ops bcmgenet_netdev_ops = {
> + .ndo_open = bcmgenet_open,
> + .ndo_stop = bcmgenet_close,
> + .ndo_start_xmit = bcmgenet_xmit,
> + .ndo_select_queue = bcmgenet_select_queue,
> + .ndo_tx_timeout = bcmgenet_timeout,
> + .ndo_set_rx_mode = bcmgenet_set_rx_mode,
> + .ndo_set_mac_address = bcmgenet_set_mac_addr,
> + .ndo_do_ioctl = bcmgenet_ioctl,
> + .ndo_set_features = bcmgenet_set_features,
> +};
Please use tabs before '=' to line things up.
[...]
> +static int bcmgenet_probe(struct platform_device *pdev)
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct bcmgenet_priv *priv;
> + struct net_device *dev;
> + const void *macaddr;
> + struct resource *r;
> + int err = -EIO;
> +
> + /* Up to GENET_MAX_MQ_CNT + 1 TX queues and a single RX queue */
> + dev = alloc_etherdev_mqs(sizeof(*priv), GENET_MAX_MQ_CNT + 1, 1);
> + if (!dev) {
> + dev_err(&pdev->dev, "can't allocate net device\n");
> + return -ENOMEM;
> + }
> +
> + priv = netdev_priv(dev);
> + priv->irq0 = platform_get_irq(pdev, 0);
> + priv->irq1 = platform_get_irq(pdev, 1);
> + if (!priv->irq0 || !priv->irq1) {
> + dev_err(&pdev->dev, "can't find IRQs\n");
> + err = -EINVAL;
> + goto err;
> + }
> +
> + macaddr = of_get_mac_address(dn);
> + if (!macaddr) {
> + dev_err(&pdev->dev, "can't find MAC address\n");
> + err = -EINVAL;
> + goto err;
> + }
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->base = devm_request_and_ioremap(&pdev->dev, r);
> + if (!priv->base) {
> + dev_err(&pdev->dev, "can't ioremap\n");
> + err = -EINVAL;
> + goto err;
> + }
> +
> + dev->base_addr = (unsigned long)priv->base;
base_addr in net_device is a legacy hack.
> + SET_NETDEV_DEV(dev, &pdev->dev);
> + dev_set_drvdata(&pdev->dev, dev);
> + ether_addr_copy(dev->dev_addr, macaddr);
> + dev->irq = priv->irq0;
And so is irq.
--
Ueimor
^ permalink raw reply
* Re: [net-next 3/3] cfg80211: add MPLS and 802.21 classification
From: Simon Wunderlich @ 2014-02-13 10:46 UTC (permalink / raw)
To: Joe Perches
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
mathias.kretschmer-8LS2qeF34IpklNlQbfROjRvVK+yQ3ZXh
In-Reply-To: <1392205783.1868.5.camel@joe-AO722>
Hey Joe,
thanks for the comments!
> On Wed, 2014-02-12 at 11:53 +0100, Simon Wunderlich wrote:
> > MPLS labels may contain traffic control information, which should be
> > evaluated and used by the wireless subsystem if present.
>
> trivial notes:
> > diff --git a/net/wireless/util.c b/net/wireless/util.c
>
> []
>
> > @@ -11,6 +11,7 @@
> >
> > #include <net/ip.h>
> > #include <net/dsfield.h>
> > #include <linux/if_vlan.h>
> >
> > +#include <uapi/linux/mpls.h>
>
> Please try not to #include uapi files.
>
why not? The packet definition header has been put in UAPI, so I need that
there ... or should we move that to some place else? Other protocol definition
headers like IP, udp, tcp have been defined in uapi too the same way, so I
figured that would be the right position.
> > @@ -710,6 +711,29 @@ unsigned int cfg80211_classify8021d(struct sk_buff
> > *skb,
> >
> > return vlan_priority;
> >
> > }
> >
> > + if (skb_headlen(skb) >= sizeof(struct ethhdr)) {
> > + struct ethhdr *eh = (struct ethhdr *) skb->data;
> > + struct mpls_label_stack mpls_tmp, *mpls;
> > +
> > + switch (eh->h_proto) {
> > + case __constant_htons(ETH_P_MPLS_UC):
>
> > + case __constant_htons(ETH_P_MPLS_MC):
> __constant_ isn't necessary for these labels.
>
OK, I will change that.
> > switch (skb->protocol) {
> >
> > case htons(ETH_P_IP):
> > dscp = ipv4_get_dsfield(ip_hdr(skb)) & 0xfc;
>
> like this, just case htons(ETH_<FOO>) is enough.
OK.
I can rework and resend this particular patch - probably to linux wireless. If
the other patches are ok, would you guys merge the first two patches to net-
next then?
Thanks,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next v2 06/10] net: bcmgenet: add main driver file
From: Joe Perches @ 2014-02-13 10:58 UTC (permalink / raw)
To: Francois Romieu; +Cc: Florian Fainelli, netdev, davem, cernekee, devicetree
In-Reply-To: <20140213103506.GB14941@electric-eye.fr.zoreil.com>
On Thu, 2014-02-13 at 11:35 +0100, Francois Romieu wrote:
> Florian Fainelli <f.fainelli@gmail.com> :
> [...]
> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
[]
> > +static int bcmgenet_set_rx_csum(struct net_device *dev,
> > + netdev_features_t wanted)
> > +{
> > + struct bcmgenet_priv *priv = netdev_priv(dev);
> > + u32 rbuf_chk_ctrl;
> > + int rx_csum_en;
> > +
> > + rx_csum_en = !!(wanted & NETIF_F_RXCSUM);
>
> It's a bool.
It could be a bool.
The struct definition has:
+ unsigned int desc_rxchk_en;
but perhaps a lot of these members could be bool.
It'd be nicer if the variable types were the same.
> > + spin_lock_bh(&priv->bh_lock);
> > + rbuf_chk_ctrl = bcmgenet_rbuf_readl(priv, RBUF_CHK_CTRL);
> > +
> > + /* enable rx checksumming */
> > + if (!rx_csum_en)
> > + rbuf_chk_ctrl &= ~RBUF_RXCHK_EN;
> > + else
> > + rbuf_chk_ctrl |= RBUF_RXCHK_EN;
This is more normally written with a positive test like:
if (rx_csum_en)
rbuf_chk_ctrl |= RBUF_RXCHK_EN;
else
rbuf_chk_ctrl &= RBUF_RXCHK_EN;
> > + priv->desc_rxchk_en = rx_csum_en;
^ permalink raw reply
* Re: [PATCH net-next v2 09/10] Documentation: add Device tree bindings for Broadcom GENET
From: Mark Rutland @ 2014-02-13 11:13 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org,
cernekee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1392269395-23513-10-git-send-email-f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Thu, Feb 13, 2014 at 05:29:54AM +0000, Florian Fainelli wrote:
> This patch adds the Device Tree bindings for the Broadcom GENET Gigabit
> Ethernet controller. A bunch of examples are provided to illustrate the
> versatile aspect of the hardare.
>
> Signed-off-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> Changes since v1:
> - rebased
>
> .../devicetree/bindings/net/broadcom-bcmgenet.txt | 111 +++++++++++++++++++++
> 1 file changed, 111 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt
>
> diff --git a/Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt b/Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt
> new file mode 100644
> index 0000000..93c58e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/broadcom-bcmgenet.txt
> @@ -0,0 +1,111 @@
> +* Broadcom BCM7xxx Ethernet Controller (GENET)
> +
> +Required properties:
> +- compatible: should be "brcm,genet-v1", "brcm,genet-v2", "brcm,genet-v3",
> + "brcm,genet-v4".
Presumably "should contain one of" is a better description than "should
be"?
Are the newer revisions compatible with older revisions?
> +- reg: address and length of the register set for the device.
> +- interrupts: interrupt for the device
> +- mdio bus node: this node should always be present regarless of the PHY
> + configuration of the GENET instance
Nit: a node is not a property, list it after properties.
> +- phy-mode: The interface between the SoC and the PHY (a string that
> + of_get_phy_mode() can understand).
Do we not have a document under bindings listing these? I really don't
like referring to code in bindings docs.
> +
> +MDIO bus node required properties:
> +
> +- compatible: should be "brcm,genet-v<N>-mdio"
Where N is? Could this not be an explicit list as above? It helps when
searching for bindings.
> +- reg: address and length relative to the parent node base register address
The parent node will require #address-cells and #size-cells too then.
> +- address-cells: address cell for MDIO bus addressing, should be 1
> +- size-cells: size of the cells for MDIO bus addressing, should be 0
> +
> +Optional properties:
> +- phy-handle: A phandle to a phy node defining the PHY address (as the reg
> + property, a single integer), used to describe configurations where a PHY
> + (internal or external) is used.
Is there not a phy binding you could refer to instead?
> +
> +- fixed-link: When the GENET interface is connected to a MoCA hardware block
> + or when operating in a RGMII to RGMII type of connection, or when the
> + MDIO bus is voluntarily disabled, this property should be used to describe
> + the "fixed link", the property is described as follows:
> +
> + fixed-link: <a b c d e> where a is emulated phy id - choose any,
> + but unique to the all specified fixed-links, b is duplex - 0 half,
> + 1 full, c is link speed - d#10/d#100/d#1000, d is pause - 0 no
> + pause, 1 pause, e is asym_pause - 0 no asym_pause, 1 asym_pause.
Is this not documented elsewhere such that it can be referred to?
> +
> +Internal Gigabit PHY example:
> +
> +ethernet@f0b60000 {
> + phy-mode = "internal";
> + phy-handle = <&phy1>;
> + mac-address = [ 00 10 18 36 23 1a ];
> + compatible = "brcm,genet-v4";
> + #address-cells = <0x1>;
> + #size-cells = <0x1>;
> + device_type = "ethernet";
What's this needed by? I can't see any other devices with this
device_type, and I was under the impression that we didn't want new
device_type properties cropping up.
> + reg = <0xf0b60000 0xfc4c>;
> + interrupts = <0x0 0x14 0x0 0x0 0x15 0x0>;
How many? The binding implied only one, and I'm not away of any
interrupt controller bindings with #interrupt-cells = <6>.
Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [net-next 3/3] cfg80211: add MPLS and 802.21 classification
From: Joe Perches @ 2014-02-13 11:14 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: netdev, davem, linux-wireless, mathias.kretschmer
In-Reply-To: <201402131146.01749.sw@simonwunderlich.de>
On Thu, 2014-02-13 at 11:46 +0100, Simon Wunderlich wrote:
Hi Simon
> > On Wed, 2014-02-12 at 11:53 +0100, Simon Wunderlich wrote:
> > > MPLS labels may contain traffic control information, which should be
> > > evaluated and used by the wireless subsystem if present.
[]
> > > diff --git a/net/wireless/util.c b/net/wireless/util.c
[]
> > > +#include <uapi/linux/mpls.h>
> >
> > Please try not to #include uapi files.
> why not? The packet definition header has been put in UAPI, so I need that
> there ... or should we move that to some place else? Other protocol definition
> headers like IP, udp, tcp have been defined in uapi to the same way, so I
> figured that would be the right position.
It seems the practice is to have another include/<net|linux>/mpls.h
file that #includes the <uapi/linux/mpls.h>
For instance, look at:
include/linux/udp.h
It does #include <uapi/linux/udp.h>
All the other kernel sources use
#include <linux/udp.h>
^ permalink raw reply
* Re: [PATCH] usbnet: remove generic hard_header_len check
From: Emil Goode @ 2014-02-13 11:20 UTC (permalink / raw)
To: Bjørn Mork
Cc: Steve Glendinning, Oliver Neukum, David S. Miller, Freddy Xin,
Eric Dumazet, Ming Lei, Paul Gortmaker, Jeff Kirsher,
Liu Junliang, Octavian Purdila, linux-usb, netdev, linux-kernel
In-Reply-To: <87fvnn4c2k.fsf@nemi.mork.no>
On Thu, Feb 13, 2014 at 10:05:39AM +0100, Bjørn Mork wrote:
> Emil Goode <emilgoode@gmail.com> writes:
>
> > This patch removes a generic hard_header_len check from the usbnet
> > module that is causing dropped packages under certain circumstances
> > for devices that send rx packets that cross urb boundaries.
> >
> > One example is the AX88772B which occasionally send rx packets that
> > cross urb boundaries where the remaining partial packet is sent with
> > no hardware header. When the buffer with a partial packet is of less
> > number of octets than the value of hard_header_len the buffer is
> > discarded by the usbnet module.
> >
> > With AX88772B this can be reproduced by using ping with a packet
> > size between 1965-1976.
> >
> > The bug has been reported here:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=29082
> >
> > This patch introduces the following changes:
> > - Removes the generic hard_header_len check in the rx_complete
> > function in the usbnet module.
> > - Introduces a ETH_HLEN check for skbs that are not cloned from
> > within a rx_fixup callback.
> > - For safety a hard_header_len check is added to each rx_fixup
> > callback function that could be affected by this change.
> > These extra checks could possibly be removed by someone
> > who has the hardware to test.
> >
> > The changes place full responsibility on the rx_fixup callback
> > functions that clone skbs to only pass valid skbs to the
> > usbnet_skb_return function.
> >
> > Signed-off-by: Emil Goode <emilgoode@gmail.com>
> > Reported-by: Igor Gnatenko <i.gnatenko.brain@gmail.com>
>
>
> Great work! Looks good to me.
Thank you :)
> Just a couple of questions:
>
> > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> > index ff5c871..b2e2aee 100644
> > --- a/drivers/net/usb/qmi_wwan.c
> > +++ b/drivers/net/usb/qmi_wwan.c
> > @@ -80,10 +80,10 @@ static int qmi_wwan_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> > {
> > __be16 proto;
> >
> > - /* usbnet rx_complete guarantees that skb->len is at least
> > - * hard_header_len, so we can inspect the dest address without
> > - * checking skb->len
> > - */
> > + /* This check is no longer done by usbnet */
> > + if (skb->len < dev->net->hard_header_len)
> > + return 0;
> > +
> > switch (skb->data[0] & 0xf0) {
> > case 0x40:
> > proto = htons(ETH_P_IP);
> > diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> > index a48bc0f..524a47a 100644
> > --- a/drivers/net/usb/rndis_host.c
> > +++ b/drivers/net/usb/rndis_host.c
> > @@ -492,6 +492,10 @@ EXPORT_SYMBOL_GPL(rndis_unbind);
> > */
> > int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> > {
> > + /* This check is no longer done by usbnet */
> > + if (skb->len < dev->net->hard_header_len)
> > + return 0;
> > +
>
> Wouldn't it be better to test against ETH_HLEN, since that is a constant
> and "obviously correct" in this case?
Some minidrivers change the default hard_header_len value so using it
guarantees that the patch will not make any change to how the code is
currently working. Using ETH_HLEN could be more informative about what
the minidriver should check before passing skbs to usbnet_skb_return().
Then I think the comment should be changed as well. My intention was to
not make any changes that affect how the code works for devices I cannot
test, but I think either way is fine and if you insist on changing it
let me know.
> > diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> > index 4671da7..dd10d58 100644
> > --- a/drivers/net/usb/usbnet.c
> > +++ b/drivers/net/usb/usbnet.c
> > @@ -542,17 +542,19 @@ static inline void rx_process (struct usbnet *dev, struct sk_buff *skb)
> > }
> > // else network stack removes extra byte if we forced a short packet
> >
> > - if (skb->len) {
> > - /* all data was already cloned from skb inside the driver */
> > - if (dev->driver_info->flags & FLAG_MULTI_PACKET)
> > - dev_kfree_skb_any(skb);
> > - else
> > - usbnet_skb_return(dev, skb);
> > + /* all data was already cloned from skb inside the driver */
> > + if (dev->driver_info->flags & FLAG_MULTI_PACKET)
> > + goto done;
> > +
> > + if (skb->len < ETH_HLEN) {
> > + dev->net->stats.rx_errors++;
> > + dev->net->stats.rx_length_errors++;
> > + netif_dbg(dev, rx_err, dev->net, "rx length %d\n", skb->len);
> > + } else {
> > + usbnet_skb_return(dev, skb);
> > return;
> > }
> >
> > - netif_dbg(dev, rx_err, dev->net, "drop\n");
> > - dev->net->stats.rx_errors++;
> > done:
> > skb_queue_tail(&dev->done, skb);
> > }
>
>
> At first glance I wondered where the dev_kfree_skb_any(skb) went. But
> then I realized that you leave that to the common rx_cleanup path, using
> the dev->done queue. Is that right? If so, then I guess it could use a
> bit of explaining in the commit message?
Yes I should have put a comment in the changelog about this. All skbs that
are passed to rx_process have their state set to rx_cleanup and just because
the skb was cloned doesn't mean that we should free the original in a
different way. As it is I think we are acctually missing a call to
usb_free_urb that is called on the common rx_cleanup path.
I will resend and add a comment about this.
Best regards,
Emil Goode
^ permalink raw reply
* Re: [RFC 2/2] xen-netback: disable multicast and use a random hw MAC address
From: Ian Campbell @ 2014-02-13 11:35 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: netdev@vger.kernel.org, xen-devel, Paul Durrant, Wei Liu, kvm,
linux-kernel@vger.kernel.org
In-Reply-To: <CAB=NE6WFDWW1faoCYJP9zh6rPPvET+dbHxjHnGkYXtRx6z2LOQ@mail.gmail.com>
On Wed, 2014-02-12 at 14:05 -0800, Luis R. Rodriguez wrote:
> > I meant the PV protocol extension which allows guests (netfront) to
> > register to receive multicast frames across the PV ring -- i.e. for
> > multicast to work from the guests PoV.
>
> Not quite sure I understand, ipv6 works on guests so multicast works,
> so its unclear what you mean by multicast frames across the PV ring.
> Is there any code or or documents I can look at ?
xen/include/public/io/netif.h talks about 'feature-multicast-control'
and XEN_NETIF_EXTRA_TYPE_MCAST_{ADD,DEL}.
Looking at it now in the absence of those then flooding is the
default...
> > (maybe that was just an optimisation though and the default is to flood
> > everything, it was a long time ago)
>
> From a networking perspective everything is being flooded as I've seen
> it so far.
... which is why it works ;-)
Ian.
^ permalink raw reply
* Re: [PATCH] usbnet: remove generic hard_header_len check
From: Bjørn Mork @ 2014-02-13 11:36 UTC (permalink / raw)
To: Emil Goode
Cc: Steve Glendinning, Oliver Neukum, David S. Miller, Freddy Xin,
Eric Dumazet, Ming Lei, Paul Gortmaker, Jeff Kirsher,
Liu Junliang, Octavian Purdila, linux-usb, netdev, linux-kernel
In-Reply-To: <20140213112016.GA4245@lianli>
Emil Goode <emilgoode@gmail.com> writes:
> Yes I should have put a comment in the changelog about this. All skbs that
> are passed to rx_process have their state set to rx_cleanup and just because
> the skb was cloned doesn't mean that we should free the original in a
> different way. As it is I think we are acctually missing a call to
> usb_free_urb that is called on the common rx_cleanup path.
Yes, I wondered about that as well, thinking that this part actually
should be a separate patch for net+stable.
But then I wondered how we could possibly have had a bug like that
living here for so long, and the answer is we haven't. You don't want
to free the URB. It is already resubmitted with a different skb as its
buffer. The call to usb_free_urb in the rx_cleanup path is there only
for the cases where the URB is not resubmitted. The rx_complete callback
is controlling this by updating entry->urb as appropriate. So it will
always be NULL if rx_process is called, and the call to usb_free_urb has
no effect.
Rearranging this code to always take the same cleanup path is still a
very nice cleanup IMHO.
Bjørn
^ permalink raw reply
* Re: [PATCH net-next v2 06/10] net: bcmgenet: add main driver file
From: Mark Rutland @ 2014-02-13 11:38 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev@vger.kernel.org, davem@davemloft.net, cernekee@gmail.com,
devicetree@vger.kernel.org
In-Reply-To: <1392269395-23513-7-git-send-email-f.fainelli@gmail.com>
On Thu, Feb 13, 2014 at 05:29:51AM +0000, Florian Fainelli wrote:
> This patch adds the BCMGENET main driver file which supports the
> following:
>
> - GENET hardware from V1 to V4
> - support for reading the UniMAC MIB counters statistics
> - support for the 5 transmit queues
> - support for RX/TX checksum offload and SG
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes since v1:
> - use module_platform_driver boilerplate macro
> - renamed bcmgenet_plat_drv to bcmgenet_driver
> - renamed bcmgenet_drv_{probe,remove} to bcmgenet_{probe,remove}
> - removed priv->phy_type usage and use priv->phy_interface which
> contains the exact same value
> - removed debug module parameters, unused
> - added MODULDE_{AUTHOR,ALIAS,DESCRIPTION} macros
> - remove hardcoded queue index check in bcmgenet_xmit
[...]
> +static int bcmgenet_probe(struct platform_device *pdev)
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct bcmgenet_priv *priv;
> + struct net_device *dev;
> + const void *macaddr;
> + struct resource *r;
> + int err = -EIO;
> +
> + /* Up to GENET_MAX_MQ_CNT + 1 TX queues and a single RX queue */
> + dev = alloc_etherdev_mqs(sizeof(*priv), GENET_MAX_MQ_CNT + 1, 1);
> + if (!dev) {
> + dev_err(&pdev->dev, "can't allocate net device\n");
> + return -ENOMEM;
> + }
> +
> + priv = netdev_priv(dev);
> + priv->irq0 = platform_get_irq(pdev, 0);
> + priv->irq1 = platform_get_irq(pdev, 1);
> + if (!priv->irq0 || !priv->irq1) {
> + dev_err(&pdev->dev, "can't find IRQs\n");
> + err = -EINVAL;
> + goto err;
> + }
The binding did not describe that there were two interrupts. What are
each of them for? Are they named in the documentation?
> +
> + macaddr = of_get_mac_address(dn);
> + if (!macaddr) {
> + dev_err(&pdev->dev, "can't find MAC address\n");
> + err = -EINVAL;
> + goto err;
> + }
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->base = devm_request_and_ioremap(&pdev->dev, r);
> + if (!priv->base) {
> + dev_err(&pdev->dev, "can't ioremap\n");
> + err = -EINVAL;
> + goto err;
> + }
> +
> + dev->base_addr = (unsigned long)priv->base;
Does the net core actually need this?
I can't see anywhere else it's used in this file.
[...]
> + if (of_device_is_compatible(dn, "brcm,genet-v4"))
> + priv->version = GENET_V4;
> + else if (of_device_is_compatible(dn, "brcm,genet-v3"))
> + priv->version = GENET_V3;
> + else if (of_device_is_compatible(dn, "brcm,genet-v2"))
> + priv->version = GENET_V2;
> + else if (of_device_is_compatible(dn, "brcm,genet-v1"))
> + priv->version = GENET_V1;
> + else {
> + dev_err(&pdev->dev, "unknown GENET version\n");
> + err = -EINVAL;
> + goto err;
Surely you can't have probed if none of these are in the compatible
list?
Might it make more sense to place this value in of_device_id::data? You
can get it it with of_match_node, and you only have to place the strings
in one place, so no possible typo issues.
> + }
> +
> + bcmgenet_set_hw_params(priv);
> +
> + spin_lock_init(&priv->lock);
> + spin_lock_init(&priv->bh_lock);
> + mutex_init(&priv->mib_mutex);
> + /* Mii wait queue */
> + init_waitqueue_head(&priv->wq);
> + /* Always use RX_BUF_LENGTH (2KB) buffer for all chips */
> + priv->rx_buf_len = RX_BUF_LENGTH;
> + INIT_WORK(&priv->bcmgenet_irq_work, bcmgenet_irq_task);
> +
> + priv->clk = devm_clk_get(&priv->pdev->dev, "enet");
> + if (IS_ERR(priv->clk))
> + dev_warn(&priv->pdev->dev, "failed to get enet clock\n");
This wasn't in the binding.
> +
> + priv->clk_wol = devm_clk_get(&priv->pdev->dev, "enet-wol");
> + if (IS_ERR(priv->clk_wol))
> + dev_warn(&priv->pdev->dev, "failed to get enet-wol clock\n");
This is also missing from the binding.
> + /* Turn off the clocks */
> + if (!IS_ERR(priv->clk))
> + clk_disable_unprepare(priv->clk);
Either the comment is misleading (s/clocks/clock/), or you're forgetting
about the enet-wol clock here.
Thanks,
Mark.
^ permalink raw reply
* RE: [PATCH] usbnet: remove generic hard_header_len check
From: David Laight @ 2014-02-13 11:49 UTC (permalink / raw)
To: 'Emil Goode', Bjørn Mork
Cc: Steve Glendinning, Oliver Neukum, David S. Miller, Freddy Xin,
Eric Dumazet, Ming Lei, Paul Gortmaker, Jeff Kirsher,
Liu Junliang, Octavian Purdila, linux-usb@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20140213112016.GA4245@lianli>
From: Emil Goode
> > > int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> > > {
> > > + /* This check is no longer done by usbnet */
> > > + if (skb->len < dev->net->hard_header_len)
> > > + return 0;
> > > +
> >
> > Wouldn't it be better to test against ETH_HLEN, since that is a constant
> > and "obviously correct" in this case?
>
> Some minidrivers change the default hard_header_len value so using it
> guarantees that the patch will not make any change to how the code is
> currently working. Using ETH_HLEN could be more informative about what
> the minidriver should check before passing skbs to usbnet_skb_return().
> Then I think the comment should be changed as well. My intention was to
> not make any changes that affect how the code works for devices I cannot
> test, but I think either way is fine and if you insist on changing it
> let me know.
I think that test is to ensure that the data passed to the mini-driver
contains the ethernet frame encapsulation header (this typically
contains the actual frame length and some flags) so that the minidriver
won't read off the end of the usb data.
Any check for stupidly short ethernet frames would be later on.
IIRC the absolute minimum 802.3 ethernet frame is 17 bytes
(after frame padding has been stripped).
David
^ permalink raw reply
* Re: [PATCH net-next v2 07/10] net: bcmgenet: add MDIO routines
From: Mark Rutland @ 2014-02-13 11:50 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev@vger.kernel.org, davem@davemloft.net, cernekee@gmail.com,
devicetree@vger.kernel.org
In-Reply-To: <1392269395-23513-8-git-send-email-f.fainelli@gmail.com>
On Thu, Feb 13, 2014 at 05:29:52AM +0000, Florian Fainelli wrote:
> This patch adds support for configuring the port multiplexer hardware
> which resides in front of the GENET Ethernet MAC controller. This allows
> us to support:
>
> - internal PHYs (using drivers/net/phy/bcm7xxx.c)
> - MoCA PHYs which are an entirely separate hardware block not covered
> here
> - external PHYs and switches
>
> Note that MoCA and switches are currently supported using the emulated
> "fixed PHY" driver.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes since v1:
> - fixed MDIO crash/warning when Device Tree probing fails
> - removed the use of priv->phy_type and use priv->phy_interface
> directly
>
> drivers/net/ethernet/broadcom/genet/bcmmii.c | 481 +++++++++++++++++++++++++++
> 1 file changed, 481 insertions(+)
> create mode 100644 drivers/net/ethernet/broadcom/genet/bcmmii.c
[...]
> +static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
> +{
> + struct device_node *dn = priv->pdev->dev.of_node;
> + struct device *kdev = &priv->pdev->dev;
> + struct device_node *mdio_dn;
> + const __be32 *fixed_link;
This looks a bit odd. Could we not have a common parser for fixed-link
properties?
> + u32 propval;
> + int ret, sz;
> +
> + mdio_dn = of_get_next_child(dn, NULL);
> + if (!mdio_dn) {
> + dev_err(kdev, "unable to find MDIO bus node\n");
> + return -ENODEV;
> + }
Could you please check that this is the node you expect (by looking at
the compatible string list).
> +
> + ret = of_mdiobus_register(priv->mii_bus, mdio_dn);
> + if (ret) {
> + dev_err(kdev, "failed to register MDIO bus\n");
> + return ret;
> + }
> +
> + /* Check if we have an internal or external PHY */
> + priv->phy_dn = of_parse_phandle(dn, "phy-handle", 0);
> + if (priv->phy_dn) {
> + if (!of_property_read_u32(priv->phy_dn, "max-speed", &propval))
> + priv->phy_speed = propval;
Is there no way to find this out without reading values directly off of
the PHY? It seems like something we should have an abstraction for.
> + } else {
> + /* Read the link speed from the fixed-link property */
> + fixed_link = of_get_property(dn, "fixed-link", &sz);
> + if (!fixed_link || sz < sizeof(*fixed_link)) {
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + priv->phy_speed = be32_to_cpu(fixed_link[2]);
Similarly can we not have a common fixed-link parser? Or abstraction
such that you query the phy regardless of what it is and how its binding
represents this?
Thanks,
Mark.
^ permalink raw reply
* [net-next 02/15] i40e: bump driver version
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem; +Cc: Jesse Brandeburg, netdev, gospo, sassmann, Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Update the driver version to 0.3.31-k.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index b901371..f596f74c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -38,7 +38,7 @@ static const char i40e_driver_string[] =
#define DRV_VERSION_MAJOR 0
#define DRV_VERSION_MINOR 3
-#define DRV_VERSION_BUILD 30
+#define DRV_VERSION_BUILD 31
#define DRV_VERSION __stringify(DRV_VERSION_MAJOR) "." \
__stringify(DRV_VERSION_MINOR) "." \
__stringify(DRV_VERSION_BUILD) DRV_KERN
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 00/15] Intel Wired LAN Driver Updates
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem; +Cc: Aaron Brown, netdev, gospo, sassmann
This series contains updates to i40e and i40evf, primarily reset
handling / refactoring along with a fair amount of minor cleanup.
Jesse fixes some spelling, bumps the version and other trivial fixes.
Akeem sets a bit that is needed before shutdown in the case of
tx_timeout recovery failure. Mitch refactors reset handling along
with a whole bunch of clean up.
Jesse Brandeburg (3):
i40e: spelling error
i40e: bump driver version
i40evf: trivial fixes
Akeem G Abodunrin (1):
i40e: Setting i40e_down bit for tx_timeout
Mitch Williams (11):
i40evf: clean up memsets
i40e: remove dead code
i40e: set VF state to active when reset is complete
i40e: reset VFs after PF reset
i40e: enable extant VFs
i40e: don't handle VF reset on unload
i40evf: clean up adapter struct
i40evf: fix bogus comment
i40evf: don't store unnecessary array of strings
i40evf: change type of flags variable
i40evf: refactor reset handling
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 5 +-
drivers/net/ethernet/intel/i40e/i40e_main.c | 28 +++-
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 32 ++---
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h | 1 +
drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 8 +-
drivers/net/ethernet/intel/i40evf/i40e_type.h | 4 +-
drivers/net/ethernet/intel/i40evf/i40evf.h | 42 ++----
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 155 ++++++++++++++++-----
.../net/ethernet/intel/i40evf/i40evf_virtchnl.c | 15 +-
10 files changed, 183 insertions(+), 109 deletions(-)
--
1.8.5.GIT
^ permalink raw reply
* [net-next 03/15] i40evf: trivial fixes
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem; +Cc: Jesse Brandeburg, netdev, gospo, sassmann, Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
This change moves one operator up to the previous line and deletes
the duplicate declaration of ETH_ALEN.
Also update copyrights.
Change-ID: I88de73093b584e0f3b29d481ccd83fc4b1a1afa5
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 6 +++---
drivers/net/ethernet/intel/i40evf/i40e_type.h | 4 +---
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index 3bb7558..827bb5f 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -1,7 +1,7 @@
/*******************************************************************************
*
* Intel Ethernet Controller XL710 Family Linux Virtual Function Driver
- * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2013 - 2014 Intel Corporation.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms and conditions of the GNU General Public License,
@@ -807,8 +807,8 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
rx_desc = I40E_RX_DESC(rx_ring, i);
qword = le64_to_cpu(rx_desc->wb.qword1.status_error_len);
- rx_status = (qword & I40E_RXD_QW1_STATUS_MASK)
- >> I40E_RXD_QW1_STATUS_SHIFT;
+ rx_status = (qword & I40E_RXD_QW1_STATUS_MASK) >>
+ I40E_RXD_QW1_STATUS_SHIFT;
while (rx_status & (1 << I40E_RX_DESC_STATUS_DD_SHIFT)) {
union i40e_rx_desc *next_rxd;
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_type.h b/drivers/net/ethernet/intel/i40evf/i40e_type.h
index 3bffac0..092aace 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_type.h
+++ b/drivers/net/ethernet/intel/i40evf/i40e_type.h
@@ -1,7 +1,7 @@
/*******************************************************************************
*
* Intel Ethernet Controller XL710 Family Linux Virtual Function Driver
- * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2013 - 2014 Intel Corporation.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms and conditions of the GNU General Public License,
@@ -64,8 +64,6 @@
struct i40e_hw;
typedef void (*I40E_ADMINQ_CALLBACK)(struct i40e_hw *, struct i40e_aq_desc *);
-#define ETH_ALEN 6
-
/* Data type manipulation macros. */
#define I40E_DESC_UNUSED(R) \
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 04/15] i40evf: clean up memsets
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Mitch Williams, netdev, gospo, sassmann, Kevin Scott,
Jesse Brandeburg, Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
As politely pointed out by Dave Miller, calls to memset do not need a
void pointer cast. Additionally, it is preferred to use sizeof(*the
actual object) instead of sizeof(type).
Change-ID: Id6a02429b7040111531f3865ea03fbe619167cb3
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Kevin Scott <kevin.c.scott@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
index a50e6b3..ed3902b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
@@ -647,9 +647,8 @@ static u16 i40e_clean_asq(struct i40e_hw *hw)
desc_cb = *desc;
cb_func(hw, &desc_cb);
}
- memset((void *)desc, 0, sizeof(struct i40e_aq_desc));
- memset((void *)details, 0,
- sizeof(struct i40e_asq_cmd_details));
+ memset(desc, 0, sizeof(*desc));
+ memset(details, 0, sizeof(*details));
ntc++;
if (ntc == asq->count)
ntc = 0;
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 06/15] i40e: remove dead code
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Mitch Williams, netdev, gospo, sassmann, Jesse Brandeburg,
Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
This looks like a cut and paste error. The code makes no sense where
it is, and accomplishes nothing. Since we've removed the goto, we can
also get rid of the extraneous brackets.
Change-ID: I9315e3eafeee0a5713c94b0dc57b58b60a849124
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Sibai Li <sibai.li@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index b9d1c1c..299372b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -408,18 +408,10 @@ static int i40e_alloc_vsi_res(struct i40e_vf *vf, enum i40e_vsi_type type)
"Could not allocate VF broadcast filter\n");
}
- if (!f) {
- dev_err(&pf->pdev->dev, "Unable to add ucast filter\n");
- ret = -ENOMEM;
- goto error_alloc_vsi_res;
- }
-
/* program mac filter */
ret = i40e_sync_vsi_filters(vsi);
- if (ret) {
+ if (ret)
dev_err(&pf->pdev->dev, "Unable to program ucast filters\n");
- goto error_alloc_vsi_res;
- }
error_alloc_vsi_res:
return ret;
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 05/15] i40e: Setting i40e_down bit for tx_timeout
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Akeem G Abodunrin, netdev, gospo, sassmann, Jesse Brandeburg,
Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
If tx_timeout recovery failed, then it becomes necessary to set
i40e_down bit before actually shutdown the connection.
Change-ID: Iaac81df0e302116571827aa0cff450697fbb7fa3
Signed-off-by: Akeem G Abodunrin <akeem.g.abodunrin@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index f596f74c..8e44411 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -305,6 +305,7 @@ static void i40e_tx_timeout(struct net_device *netdev)
break;
default:
netdev_err(netdev, "tx_timeout recovery unsuccessful\n");
+ set_bit(__I40E_DOWN, &vsi->state);
i40e_down(vsi);
break;
}
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 01/15] i40e: spelling error
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem; +Cc: Jesse Brandeburg, netdev, gospo, sassmann, Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
Fix a spelling error, s/extention/extension/.
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index d4bb482..19af4ce 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -892,7 +892,7 @@ static inline void i40e_rx_checksum(struct i40e_vsi *vsi,
rx_status & (1 << I40E_RX_DESC_STATUS_L3L4P_SHIFT)))
return;
- /* likely incorrect csum if alternate IP extention headers found */
+ /* likely incorrect csum if alternate IP extension headers found */
if (rx_status & (1 << I40E_RX_DESC_STATUS_IPV6EXADD_SHIFT))
return;
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index ffdb01d..3bb7558 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -722,7 +722,7 @@ static inline void i40e_rx_checksum(struct i40e_vsi *vsi,
rx_status & (1 << I40E_RX_DESC_STATUS_L3L4P_SHIFT)))
return;
- /* likely incorrect csum if alternate IP extention headers found */
+ /* likely incorrect csum if alternate IP extension headers found */
if (rx_status & (1 << I40E_RX_DESC_STATUS_IPV6EXADD_SHIFT))
return;
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 12/15] i40evf: fix bogus comment
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Mitch Williams, netdev, gospo, sassmann, Jesse Brandeburg,
Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
Locate the structure in the correct header file.
Change-ID: Ic7853131728812093a44a75d6b70953311a48dab
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Sibai Li <sibai.li@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40evf/i40evf.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf.h b/drivers/net/ethernet/intel/i40evf/i40evf.h
index 05969b3..37f5877 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf.h
+++ b/drivers/net/ethernet/intel/i40evf/i40evf.h
@@ -236,8 +236,7 @@ struct i40evf_adapter {
struct pci_dev *pdev;
struct net_device_stats net_stats;
- /* structs defined in i40e_vf.h */
- struct i40e_hw hw;
+ struct i40e_hw hw; /* defined in i40e_type.h */
enum i40evf_state_t state;
volatile unsigned long crit_section;
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 08/15] i40e: reset VFs after PF reset
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Mitch Williams, netdev, gospo, sassmann, Jesse Brandeburg,
Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
Reset all of the VFs after a PF reset, so that they are in a known
state, and the VF driver can detect the reset and reinit itself.
Change-ID: I93c5b3a0f8b1371d0da078f92de948b9d3a6413f
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Sibai Li <sibai.li@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 8e44411..21d46f4 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -5332,6 +5332,11 @@ static void i40e_reset_and_rebuild(struct i40e_pf *pf, bool reinit)
/* restart the VSIs that were rebuilt and running before the reset */
i40e_pf_unquiesce_all_vsi(pf);
+ if (pf->num_alloc_vfs) {
+ for (v = 0; v < pf->num_alloc_vfs; v++)
+ i40e_reset_vf(&pf->vf[v], true);
+ }
+
/* tell the firmware that we're starting */
dv.major_version = DRV_VERSION_MAJOR;
dv.minor_version = DRV_VERSION_MINOR;
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 09/15] i40e: enable extant VFs
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Mitch Williams, netdev, gospo, sassmann, Jesse Brandeburg,
Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
If VFs are present when the driver loads, then set up some resources
so they can function.
Change-ID: I485916a811609a9990ce663d06dc645f625b07ff
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Sibai Li <sibai.li@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 10 ++++++++++
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 18 ++++++++++--------
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h | 1 +
3 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 21d46f4..897452d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -8076,6 +8076,16 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
val &= ~I40E_PFGEN_PORTMDIO_NUM_VFLINK_STAT_ENA_MASK;
wr32(hw, I40E_PFGEN_PORTMDIO_NUM, val);
i40e_flush(hw);
+
+ if (pci_num_vf(pdev)) {
+ dev_info(&pdev->dev,
+ "Active VFs found, allocating resources.\n");
+ err = i40e_alloc_vfs(pf, pci_num_vf(pdev));
+ if (err)
+ dev_info(&pdev->dev,
+ "Error %d allocating resources for existing VFs\n",
+ err);
+ }
}
pfs_found++;
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 052be06..9074f63 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -840,7 +840,7 @@ void i40e_free_vfs(struct i40e_pf *pf)
*
* allocate vf resources
**/
-static int i40e_alloc_vfs(struct i40e_pf *pf, u16 num_alloc_vfs)
+int i40e_alloc_vfs(struct i40e_pf *pf, u16 num_alloc_vfs)
{
struct i40e_vf *vfs;
int i, ret = 0;
@@ -848,14 +848,16 @@ static int i40e_alloc_vfs(struct i40e_pf *pf, u16 num_alloc_vfs)
/* Disable interrupt 0 so we don't try to handle the VFLR. */
i40e_irq_dynamic_disable_icr0(pf);
- ret = pci_enable_sriov(pf->pdev, num_alloc_vfs);
- if (ret) {
- dev_err(&pf->pdev->dev,
- "pci_enable_sriov failed with error %d!\n", ret);
- pf->num_alloc_vfs = 0;
- goto err_iov;
+ /* Check to see if we're just allocating resources for extant VFs */
+ if (pci_num_vf(pf->pdev) != num_alloc_vfs) {
+ ret = pci_enable_sriov(pf->pdev, num_alloc_vfs);
+ if (ret) {
+ dev_err(&pf->pdev->dev,
+ "Failed to enable SR-IOV, error %d.\n", ret);
+ pf->num_alloc_vfs = 0;
+ goto err_iov;
+ }
}
-
/* allocate memory */
vfs = kzalloc(num_alloc_vfs * sizeof(struct i40e_vf), GFP_KERNEL);
if (!vfs) {
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
index cc1feee..bedf0ba 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
@@ -102,6 +102,7 @@ struct i40e_vf {
void i40e_free_vfs(struct i40e_pf *pf);
int i40e_pci_sriov_configure(struct pci_dev *dev, int num_vfs);
+int i40e_alloc_vfs(struct i40e_pf *pf, u16 num_alloc_vfs);
int i40e_vc_process_vf_msg(struct i40e_pf *pf, u16 vf_id, u32 v_opcode,
u32 v_retval, u8 *msg, u16 msglen);
int i40e_vc_process_vflr_event(struct i40e_pf *pf);
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 10/15] i40e: don't handle VF reset on unload
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Mitch Williams, netdev, gospo, sassmann, Jesse Brandeburg,
Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
Set the DOWN flag before attempting to disable VFs when unloading the
driver. Also, don't attempt to reset the VFs when the driver is
unloading, because the switch configuration will fail. This fixes a
panic on unload when VFs are enabled.
Change-ID: I25a6567e89c9687145f510ff4f630932412c5c5d
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Sibai Li <sibai.li@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 10 +++++-----
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 3 ++-
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 897452d..628e917 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -8181,16 +8181,16 @@ static void i40e_remove(struct pci_dev *pdev)
i40e_ptp_stop(pf);
- if (pf->flags & I40E_FLAG_SRIOV_ENABLED) {
- i40e_free_vfs(pf);
- pf->flags &= ~I40E_FLAG_SRIOV_ENABLED;
- }
-
/* no more scheduling of any task */
set_bit(__I40E_DOWN, &pf->state);
del_timer_sync(&pf->service_timer);
cancel_work_sync(&pf->service_task);
+ if (pf->flags & I40E_FLAG_SRIOV_ENABLED) {
+ i40e_free_vfs(pf);
+ pf->flags &= ~I40E_FLAG_SRIOV_ENABLED;
+ }
+
i40e_fdir_teardown(pf);
/* If there is a switch structure or any orphans, remove them.
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 9074f63..7d133fa 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -1868,7 +1868,8 @@ int i40e_vc_process_vflr_event(struct i40e_pf *pf)
/* clear the bit in GLGEN_VFLRSTAT */
wr32(hw, I40E_GLGEN_VFLRSTAT(reg_idx), (1 << bit_idx));
- i40e_reset_vf(vf, true);
+ if (!test_bit(__I40E_DOWN, &pf->state))
+ i40e_reset_vf(vf, true);
}
}
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 07/15] i40e: set VF state to active when reset is complete
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Mitch Williams, netdev, gospo, sassmann, Jesse Brandeburg,
Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
Without this, the VF can never communicate with the PF after a VF
reset.
Change-ID: I8d10f1d0d0638d50d39f0aff263422e05d83ad83
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Sibai Li <sibai.li@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index 299372b..052be06 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -674,6 +674,7 @@ complete_reset:
mdelay(10);
i40e_alloc_vf_res(vf);
i40e_enable_vf_mappings(vf);
+ set_bit(I40E_VF_STAT_ACTIVE, &vf->vf_states);
/* tell the VF the reset is done */
wr32(hw, I40E_VFGEN_RSTAT1(vf->vf_id), I40E_VFR_VFACTIVE);
--
1.8.5.GIT
^ permalink raw reply related
* [net-next 13/15] i40evf: don't store unnecessary array of strings
From: Aaron Brown @ 2014-02-13 11:48 UTC (permalink / raw)
To: davem
Cc: Mitch Williams, netdev, gospo, sassmann, Jesse Brandeburg,
Aaron Brown
In-Reply-To: <1392292133-32120-1-git-send-email-aaron.f.brown@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
Since we store the traffic vector names in the queue vector struct, we
don't need to maintain an array of strings for these names in the
adapter structure. Replace this array with a single string and use it
when allocating the misc irq vector.
Also update copyrights.
Change-ID: I664f096c3c008210d6a04a487163e8aa934fee5b
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Sibai Li <sibai.li@intel.com>
Signed-off-by: Aaron Brown <aaron.f.brown@intel.com>
---
drivers/net/ethernet/intel/i40evf/i40evf.h | 2 +-
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 7 ++++---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf.h b/drivers/net/ethernet/intel/i40evf/i40evf.h
index 37f5877..696c9d1 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf.h
+++ b/drivers/net/ethernet/intel/i40evf/i40evf.h
@@ -190,7 +190,7 @@ struct i40evf_adapter {
struct delayed_work init_task;
struct i40e_q_vector *q_vector[MAX_MSIX_Q_VECTORS];
struct list_head vlan_filter_list;
- char name[MAX_MSIX_COUNT][IFNAMSIZ + 9];
+ char misc_vector_name[IFNAMSIZ + 9];
/* TX */
struct i40e_ring *tx_rings[I40E_MAX_VSI_QP];
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index f5caf44..d271d3a 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -1,7 +1,7 @@
/*******************************************************************************
*
* Intel Ethernet Controller XL710 Family Linux Virtual Function Driver
- * Copyright(c) 2013 Intel Corporation.
+ * Copyright(c) 2013 - 2014 Intel Corporation.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms and conditions of the GNU General Public License,
@@ -511,9 +511,10 @@ static int i40evf_request_misc_irq(struct i40evf_adapter *adapter)
struct net_device *netdev = adapter->netdev;
int err;
- sprintf(adapter->name[0], "i40evf:mbx");
+ sprintf(adapter->misc_vector_name, "i40evf:mbx");
err = request_irq(adapter->msix_entries[0].vector,
- &i40evf_msix_aq, 0, adapter->name[0], netdev);
+ &i40evf_msix_aq, 0,
+ adapter->misc_vector_name, netdev);
if (err) {
dev_err(&adapter->pdev->dev,
"request_irq for msix_aq failed: %d\n", err);
--
1.8.5.GIT
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox