On 11/28/2016 01:41 PM, Lino Sanfilippo wrote:
> The problem is that the HW does not provide a tx completion index. Instead we have to
> iterate the status descriptors until we get an invalid idx which indicates that there
> are no further tx descriptors done for now. I am afraid that if we do not limit the
> number of descriptors processed in the tx completion handler, a continuous transmission
> of frames could keep the loop in xmit_complete() run endlessly. I dont know if this
> can actually happen but I wanted to make sure that this is avoided.
OK, it might be a good idea to put that comment somewhere around the tx
completion handler to understand why it is bounded with a specific value.
>
>> [snip]
>>
>>> + while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
>>> + skb = alloc_skb(maplen + ALIGN_MASK, gfp);
>>> + if (!skb)
>>> + break;
>>> +
>>> + paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
>>> + DMA_FROM_DEVICE);
>>> + if (dma_mapping_error(&sdev->pdev->dev, paddr)) {
>>> + netdev_err(dev, "mapping rx packet failed\n");
>>> + /* drop skb */
>>> + dev_kfree_skb_any(skb);
>>> + break;
>>> + }
>>> + /* ensure head buffer descriptors are 256 byte aligned */
>>> + offset = 0;
>>> + misalign = paddr & ALIGN_MASK;
>>> + if (misalign) {
>>> + offset = SLIC_RX_BUFF_ALIGN - misalign;
>>> + skb_reserve(skb, offset);
>>> + }
>>> + /* the HW expects dma chunks for descriptor + frame data */
>>> + desc = (struct slic_rx_desc *)skb->data;
>>> + memset(desc, 0, sizeof(*desc));
>>
>> Do you really need to zero-out the prepending RX descriptor? Are not you
>> missing a write barrier here?
>
> Indeed, it should be sufficient to make sure that the bit SLIC_IRHDDR_SVALID is not set.
> I will adjust it.
> Concerning the write barrier: You mean a wmb() before slic_write() to ensure that the zeroing
> of the status desc is done before the descriptor is passed to the HW, right?
Correct, that's what I meant here.
>
>
>> [snip]
>>
>>> +
>>> + dma_sync_single_for_cpu(&sdev->pdev->dev,
>>> + dma_unmap_addr(buff, map_addr),
>>> + buff->addr_offset + sizeof(*desc),
>>> + DMA_FROM_DEVICE);
>>> +
>>> + status = le32_to_cpu(desc->status);
>>> + if (!(status & SLIC_IRHDDR_SVALID))
>>> + break;
>>> +
>>> + buff->skb = NULL;
>>> +
>>> + dma_unmap_single(&sdev->pdev->dev,
>>> + dma_unmap_addr(buff, map_addr),
>>> + dma_unmap_len(buff, map_len),
>>> + DMA_FROM_DEVICE);
>>
>> This is potentially inefficient, you already did a cache invalidation
>> for the RX descriptor here, you could be more efficient with just
>> invalidating the packet length, minus the descriptor length.
>>
>
> I am not sure I understand: We have to unmap the complete dma area, no matter if we synced
> part of it before, dont we? AFAIK a dma sync is different from unmapping dma, or do I miss
> something?
Sorry, I was not very clear, what I meant is that you can allocate and
do the initial dma_map_single() of your RX skbs during ndo_open(), and
then, in your RX path, you can only do dma_sync_single_for_cpu() twice
(once for the RX descriptor status, second time for the actual packet
contents), and when you return the SKB to the HW, do a
dma_sync_single_for_device(). The advantage of doing that, is that if
your cache operations are slow, you only do them on exactly packet
length, and not the actual RX buffer size (e.g: 2KB).
--
Florian
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
