| 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: <20121227220344.GA20409@electric-eye.fr.zoreil.com>
Date: Thu, 27 Dec 2012 23:03:44 +0100
From: Francois Romieu <romieu@...zoreil.com>
To: Rafał Miłecki <zajec5@...il.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] bgmac: driver for GBit MAC core on BCMA bus
Rafał Miłecki <zajec5@...il.com> :
[...]
> I'm not 100% sure if I understand... Do you mean I should declare variables
> inside "while" loop when possible ?
Yes. Please consider the deepest inner block.
While at it:
> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
[...]
> +static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
[...]
> + /* Check for poison and drop or pass the packet */
> + if (len == 0xdead && flags == 0xbeef) {
> + bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n", ring->start);
> + } else {
> + new_skb = netdev_alloc_skb(bgmac->net_dev, len);
> + if (new_skb) {
> + skb_put(new_skb, len);
> + skb_copy_from_linear_data_offset(skb, BGMAC_RX_FRAME_OFFSET,
> + new_skb->data, len);
> + new_skb->protocol =
> + eth_type_trans(new_skb, bgmac->net_dev);
> + netif_receive_skb(new_skb);
> + handled++;
> + } else {
> + bgmac_err(bgmac, "Allocation of skb for copying packet failed!\n");
> + }
You should increase bgmac->net_dev->stats.rx_dropped
[...]
> +static int bgmac_dma_alloc(struct bgmac *bgmac)
> +{
> + struct device *dma_dev = bgmac->core->dma_dev;
> + struct bgmac_dma_ring *ring;
> + u16 ring_base[] = { BGMAC_DMA_BASE0, BGMAC_DMA_BASE1, BGMAC_DMA_BASE2,
> + BGMAC_DMA_BASE3, };
ring_base could be 'static const'.
> + int size; /* ring size: different for Tx and Rx */
> + int err;
> + int i;
> +
> + BUILD_BUG_ON(BGMAC_MAX_TX_RINGS > ARRAY_SIZE(ring_base));
> + BUILD_BUG_ON(BGMAC_MAX_RX_RINGS > ARRAY_SIZE(ring_base));
> +
> + if (!(bcma_aread32(bgmac->core, BCMA_IOST) & BCMA_IOST_DMA64)) {
> + bgmac_err(bgmac, "Core does not report 64-bit DMA\n");
> + return -ENOTSUPP;
> + }
> +
> + for (i = 0; i < BGMAC_MAX_TX_RINGS; i++) {
> + ring = &bgmac->tx_ring[i];
> + ring->tx = true;
> + ring->num_slots = BGMAC_TX_RING_SLOTS;
> + ring->mmio_base = ring_base[i];
> + if (bgmac_dma_unaligned(bgmac, ring))
> + bgmac_warn(bgmac, "TX on ring 0x%X supports unaligned addressing but this feature is not implemented\n", ring->mmio_base);
ring->tx is used once in bgmac_dma_unaligned.
Add bgmac_dma_unaligned an extra parameter for the BGMAC_DMA_[RT]X_RINGLO
offset and ring->[rt]x can go away.
> +
> + /* Alloc ring of descriptors */
> + size = ring->num_slots * sizeof(struct bgmac_dma_desc);
> + ring->cpu_base = dma_zalloc_coherent(dma_dev, size,
> + &(ring->dma_base),
Plese remove the useless parenthesis.
[...]
> +static void bgmac_chip_stats_update(struct bgmac *bgmac)
> +{
> + int i;
> +
> + if (bgmac->core->id.id != BCMA_CORE_4706_MAC_GBIT) {
> + for (i = 0; i < BGMAC_NUM_MIB_TX_REGS; i++)
> + bgmac->mib_tx_regs[i] =
> + bgmac_read(bgmac,
> + BGMAC_TX_GOOD_OCTETS + (i * 4));
> + for (i = 0; i < BGMAC_NUM_MIB_RX_REGS; i++)
> + bgmac->mib_rx_regs[i] =
> + bgmac_read(bgmac,
> + BGMAC_RX_GOOD_OCTETS + (i * 4));
> + }
Neither of mib_[rt]x_regs is ever read.
[...]
> +static void bgmac_chip_reset(struct bgmac *bgmac)
> +{
[...]
> + if (ci->id == BCMA_CHIP_ID_BCM5357 || ci->id == BCMA_CHIP_ID_BCM4749 ||
> + ci->id == BCMA_CHIP_ID_BCM53572) {
> + struct bcma_drv_cc *cc = &bgmac->core->bus->drv_cc;
> + u8 et_swtype = 0;
> + u8 sw_type = BGMAC_CHIPCTL_1_SW_TYPE_EPHY |
> + BGMAC_CHIPCTL_1_IF_TYPE_RMII;
> + char buf[2];
> + if (nvram_getenv("et_swtype", buf, 1) > 0) {
Please keep an empty line between variables declaration and body.
[...]
> +static irqreturn_t bgmac_interrupt(int irq, void *dev_id)
> +{
> + struct bgmac *bgmac = netdev_priv(dev_id);
> +
> + u32 int_status = bgmac_read(bgmac, BGMAC_INT_STATUS);
> + int_status &= bgmac->int_mask;
...
[...]
> +/**************************************************
> + * net_device_ops
> + **************************************************/
> +
> +static int bgmac_open(struct net_device *net_dev)
> +{
> + struct bgmac *bgmac = netdev_priv(net_dev);
> +
> + /* Reset */
> + bgmac_chip_reset(bgmac);
Please remove the useless comment(s).
> + /* Specs say about reclaiming rings here, but we do that in DMA init */
> + bgmac_chip_init(bgmac, true);
> +
> + /* Enable IRQs */
> + if (request_irq(bgmac->core->irq, bgmac_interrupt, IRQF_SHARED,
> + KBUILD_MODNAME, net_dev) < 0)
> + bgmac_err(bgmac, "IRQ request error!\n");
> + napi_enable(&bgmac->napi);
> +
> + return 0;
bgmac_open should not succeed when request_irq fails. Please propagate its
error status code.
[...]
> +static int bgmac_get_settings(struct net_device *net_dev,
> + struct ethtool_cmd *cmd)
> +{
> + struct bgmac *bgmac = netdev_priv(net_dev);
> +
> + cmd->supported = SUPPORTED_10baseT_Half |
> + SUPPORTED_10baseT_Full |
> + SUPPORTED_100baseT_Half |
> + SUPPORTED_100baseT_Full |
> + SUPPORTED_1000baseT_Half |
> + SUPPORTED_1000baseT_Full |
> + SUPPORTED_Autoneg;
> +
> + if (bgmac->autoneg) {
> + WARN_ON(cmd->advertising);
> + if (bgmac->full_duplex) {
> + if (bgmac->speed & BGMAC_SPEED_10)
> + cmd->advertising |= ADVERTISED_10baseT_Full;
> + if (bgmac->speed & BGMAC_SPEED_100)
> + cmd->advertising |= ADVERTISED_100baseT_Full;
> + if (bgmac->speed & BGMAC_SPEED_1000)
> + cmd->advertising |= ADVERTISED_1000baseT_Full;
> + } else {
> + if (bgmac->speed & BGMAC_SPEED_10)
> + cmd->advertising |= ADVERTISED_10baseT_Half;
> + if (bgmac->speed & BGMAC_SPEED_100)
> + cmd->advertising |= ADVERTISED_100baseT_Half;
> + if (bgmac->speed & BGMAC_SPEED_1000)
> + cmd->advertising |= ADVERTISED_1000baseT_Half;
> + }
> + } else {
> + switch (bgmac->speed) {
> + case BGMAC_SPEED_10:
> + ethtool_cmd_speed_set(cmd, SPEED_10);
> + break;
> + case BGMAC_SPEED_100:
> + ethtool_cmd_speed_set(cmd, SPEED_100);
> + break;
> + case BGMAC_SPEED_1000:
> + ethtool_cmd_speed_set(cmd, SPEED_1000);
> + break;
> + }
> + }
It should not take long for users to complain that the driver does not
report the current link speed. A struct mii_if_info will ease your life.
Btw the driver does not manage the link (netif_carrier_{on/off}).
[...]
> diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
[...]
> +struct bgmac_dma_ring {
> + bool tx;
> + u16 num_slots;
> + u16 start;
> + u16 end;
> +
> + u16 mmio_base;
> + void *cpu_base;
struct bgmac_dma_desc *cpu_base; ?
--
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists