netdev - Re: [PATCH] bgmac: driver for GBit MAC core on BCMA bus

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Jan 2013 16:57:00 +0100
From:	Rafał Miłecki <zajec5@...il.com>
To:	Francois Romieu <romieu@...zoreil.com>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] bgmac: driver for GBit MAC core on BCMA bus

2012/12/27 Francois Romieu <romieu@...zoreil.com>:
> Rafał Miłecki <zajec5@...il.com> :
> [...]
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> [...]
>> +static void bgmac_dma_tx_reset(struct bgmac *bgmac, struct bgmac_dma_ring *ring)
>> +{
>> +     u32 val;
>> +     int i;
>> +
>> +     if (!ring->mmio_base)
>> +             return;
>
> static int bgmac_probe(struct bcma_device *core)
> [...]
>         bgmac_chip_reset(bgmac);
>
>         err = bgmac_dma_alloc(bgmac);
>
> mmio_base is only set in bgmac_dma_alloc.
>
> -> remove the useless bgmac_chip_reset() in bgmac_probe() and the driver
> won't need to test for !ring->mmio_base.

bgmac_chip_reset also does different things, so I can't just remove
bgmac_chip_reset from bgmac_probe. I could add some parameter to
bgmac_chip_reset to let it know if DMA should be reset as well, but
I'm note sure about the advantage.


>> +     if (skb->len > BGMAC_DESC_CTL1_LEN) {
>> +             bgmac_err(bgmac, "Too long skb (%d)\n", skb->len);
>> +             return NETDEV_TX_BUSY;
>> +     }
>
> The driver will loop if you don't stop queuing. It won't help.

I hoped net layer will drop the packed after few failed xmit. OK, I'll
add netif_stop_queue here.


>> +
>> +     if (ring->start <= ring->end)
>> +             free_slots = ring->start - ring->end + BGMAC_TX_RING_SLOTS;
>> +     else
>> +             free_slots = ring->start - ring->end;
>> +     if (free_slots <= 1) {
>> +             bgmac_err(bgmac, "No free slots on ring 0x%X!\n", ring->mmio_base);
>> +             netif_stop_queue(bgmac->net_dev);
>> +             return NETDEV_TX_BUSY;
>> +     }
>
> The driver should stop queueing after its processing if it detects that
> it can't handle more requests.

I think it's what the code does... netif_stop_queue and return
NETDEV_TX_BUSY. Anything wrong about it?


>> +     slot = &ring->slots[ring->end];
>> +     slot->skb = skb;
>> +     slot->dma_addr = dma_map_single(dma_dev, skb->data, skb->len, DMA_TO_DEVICE);
>> +     if (dma_mapping_error(dma_dev, slot->dma_addr)) {
>> +             bgmac_err(bgmac, "Mapping error of skb on ring 0x%X\n", ring->mmio_base);
>> +             return NETDEV_TX_BUSY;
>> +     }
>
> Why don't you drop the packet and return TX_OK ?

Didn't know about that and there isn't any nice NAPI tutorial for developers :(


>> +     dma_desc->addr_low = cpu_to_le32(lower_32_bits(slot->dma_addr));
>> +     dma_desc->addr_high = cpu_to_le32(upper_32_bits(slot->dma_addr));
>> +     dma_desc->ctl0 = cpu_to_le32(ctl0);
>> +     dma_desc->ctl1 = cpu_to_le32(ctl1);
>> +
>> +     wmb();
>
> Are we sure that the hardware will never read a descriptor where ctl0 is
> set and ctl1 is not (start_xmit, dma starts, competing start_xmit, oops) ?

Do you mean more that one ->ndo_start_xmit(...) can be executed at a
time? So I need mutex for it?


-- 
Rafał
--
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