Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling

[Felix Fietkau <nbd@openwrt.org>]

On 2015-04-14 07:55, Rafał Miłecki wrote:
> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote:
>> @@ -382,6 +382,19 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac,
>>         dma_desc->ctl1 = cpu_to_le32(ctl1);
>>  }
>>
>> +static void bgmac_dma_rx_poison_buf(struct device *dma_dev,
>> +                                   struct bgmac_slot_info *slot)
>> +{
>> +       struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET;
>> +
>> +       dma_sync_single_for_cpu(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE,
>> +                               DMA_FROM_DEVICE);
>> +       rx->len = cpu_to_le16(0xdead);
>> +       rx->flags = cpu_to_le16(0xdead);
>> +       dma_sync_single_for_device(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE,
>> +                                  DMA_FROM_DEVICE);
>> +}
> 
> flags should be 0xbeef
Right.

>> @@ -404,51 +417,32 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>>                 void *buf = slot->buf;
>>                 u16 len, flags;
>>
>> -               /* Unmap buffer to make it accessible to the CPU */
>> -               dma_sync_single_for_cpu(dma_dev, slot->dma_addr,
>> -                                       BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
>> +               do {
>> +                       /* Prepare new skb as replacement */
>> +                       if (bgmac_dma_rx_skb_for_slot(bgmac, slot)) {
>> +                               bgmac_dma_rx_poison_buf(dma_dev, slot);
>> +                               break;
>> +                       }
> 
> So you just replaced slot->dma_addr with a mapping of new skb data.
> 
> 
>>
>> -               /* Get info from the header */
>> -               len = le16_to_cpu(rx->len);
>> -               flags = le16_to_cpu(rx->flags);
>> +                       /* Unmap buffer to make it accessible to the CPU */
>> +                       dma_unmap_single(dma_dev, slot->dma_addr,
>> +                                        BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE);
> 
> Now you unmap the new sbk data instead of the old one you want to access.
> That's why the old code included old_dma_addr.
Stupid mistake, I should be more careful with late night coding ;)

>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac,
>>
>>         for (i = 0; i < ring->num_slots; i++) {
>>                 slot = &ring->slots[i];
>> -               if (!slot->buf)
>> +               if (!slot->dma_addr)
>>                         continue;
> 
> I think it breaks error path of bgmac_dma_alloc. It may happen that
> during DMA alloc we alloc skb and then we fail to map it. In such case
> buf should be freed. Please trace how bgmac_dma_free is used in
> bgmac_dma_alloc.
I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer
allocation and dma mapping. If dma mapping fails, the buffer is freed
before any part of the slot is overwritten.

>> -               if (slot->dma_addr)
>> -                       dma_unmap_single(dma_dev, slot->dma_addr,
>> -                                        BGMAC_RX_BUF_SIZE,
>> -                                        DMA_FROM_DEVICE);
>> +               dma_unmap_single(dma_dev, slot->dma_addr,
>> +                                BGMAC_RX_BUF_SIZE,
>> +                                DMA_FROM_DEVICE);
>>                 put_page(virt_to_head_page(slot->buf));
>> +               slot->dma_addr = 0;
>>         }
>>  }

- Felix