Re: [PATCH v4] net: add Faraday FTMAC100 10/100 Ethernet driver

[Po-Yu Chuang]

Dear MichaÅ,

2011/1/21 MichaÅ MirosÅaw <mirqus@xxxxxxxxx>:
> 2011/1/21 Po-Yu Chuang <ratbert.chuang@xxxxxxxxx>:
>> From: Po-Yu Chuang <ratbert@xxxxxxxxxxxxxxxx>
>>
>> FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and
>> MII. ÂThis driver has been working on some ARM/NDS32 SoC's including
>> Faraday A320 and Andes AG101.
>>
>> Signed-off-by: Po-Yu Chuang <ratbert@xxxxxxxxxxxxxxxx>
> [...]
>> +static void ftmac100_txdes_reset(struct ftmac100_txdes *txdes)
>> +{
>> + Â Â Â /* clear all except end of ring bit */
>> + Â Â Â txdes->txdes0 = 0;
>> + Â Â Â txdes->txdes1 &= FTMAC100_TXDES1_EDOTR;
>> + Â Â Â txdes->txdes2 = 0;
>> + Â Â Â txdes->txdes3 = 0;
>> +}
>
> This also probably needs cpu_to_le32().

Ah, I missed that. Fixed.

> [...]
>> +static void ftmac100_free_buffers(struct ftmac100 *priv)
>> +{
>> + Â Â Â int i;
>> +
>> + Â Â Â for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) {
>> + Â Â Â Â Â Â Â struct ftmac100_rxdes *rxdes = &priv->descs->rxdes[i];
>> + Â Â Â Â Â Â Â dma_addr_t d = ftmac100_rxdes_get_dma_addr(rxdes);
>> + Â Â Â Â Â Â Â void *page = ftmac100_rxdes_get_va(rxdes);
>> +
>> + Â Â Â Â Â Â Â if (d)
>> + Â Â Â Â Â Â Â Â Â Â Â dma_unmap_single(priv->dev, d, PAGE_SIZE,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂDMA_FROM_DEVICE);
>> +
>> + Â Â Â Â Â Â Â if (page != NULL)
>> + Â Â Â Â Â Â Â Â Â Â Â free_page((unsigned long)page);
>> + Â Â Â }
>> +
> [...]
>
>> +static int ftmac100_alloc_buffers(struct ftmac100 *priv)
>> +{
>> + Â Â Â int i;
>> +
>> + Â Â Â priv->descs = dma_alloc_coherent(priv->dev,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âsizeof(struct ftmac100_descs),
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&priv->descs_dma_addr,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂGFP_KERNEL | GFP_DMA);
>> + Â Â Â if (priv->descs == NULL)
>> + Â Â Â Â Â Â Â return -ENOMEM;
>> +
>> + Â Â Â memset(priv->descs, 0, sizeof(struct ftmac100_descs));
>> +
>> + Â Â Â /* initialize RX ring */
>> +
>> + Â Â Â ftmac100_rxdes_set_end_of_ring(&priv->descs->rxdes[RX_QUEUE_ENTRIES - 1]);
>> +
>> + Â Â Â for (i = 0; i < RX_QUEUE_ENTRIES; i += 2) {
>> + Â Â Â Â Â Â Â struct ftmac100_rxdes *rxdes = &priv->descs->rxdes[i];
>> + Â Â Â Â Â Â Â void *page;
>> + Â Â Â Â Â Â Â dma_addr_t d;
>> +
>> + Â Â Â Â Â Â Â page = (void *)__get_free_page(GFP_KERNEL | GFP_DMA);
>> + Â Â Â Â Â Â Â if (page == NULL)
>> + Â Â Â Â Â Â Â Â Â Â Â goto err;
>> +
>> + Â Â Â Â Â Â Â d = dma_map_single(priv->dev, page, PAGE_SIZE, DMA_FROM_DEVICE);
>> + Â Â Â Â Â Â Â if (unlikely(dma_mapping_error(priv->dev, d))) {
>> + Â Â Â Â Â Â Â Â Â Â Â free_page((unsigned long)page);
>> + Â Â Â Â Â Â Â Â Â Â Â goto err;
>> + Â Â Â Â Â Â Â }
>> +
>> + Â Â Â Â Â Â Â /*
>> + Â Â Â Â Â Â Â Â* The hardware enforces a sub-2K maximum packet size, so we
>> + Â Â Â Â Â Â Â Â* put two buffers on every hardware page.
>> + Â Â Â Â Â Â Â Â*/
>> + Â Â Â Â Â Â Â ftmac100_rxdes_set_va(rxdes, page);
>> + Â Â Â Â Â Â Â ftmac100_rxdes_set_va(rxdes + 1, page + PAGE_SIZE / 2);
>> +
>> + Â Â Â Â Â Â Â ftmac100_rxdes_set_dma_addr(rxdes, d);
>> + Â Â Â Â Â Â Â ftmac100_rxdes_set_dma_addr(rxdes + 1, d + PAGE_SIZE / 2);
>> +
>> + Â Â Â Â Â Â Â ftmac100_rxdes_set_buffer_size(rxdes, RX_BUF_SIZE);
>> + Â Â Â Â Â Â Â ftmac100_rxdes_set_buffer_size(rxdes + 1, RX_BUF_SIZE);
>> +
>> + Â Â Â Â Â Â Â ftmac100_rxdes_set_dma_own(rxdes);
>> + Â Â Â Â Â Â Â ftmac100_rxdes_set_dma_own(rxdes + 1);
>> + Â Â Â }
> [...]
>
> Did you test this? This looks like it will result in double free after
> packet RX, as you are giving the same page (referenced once) to two
> distinct RX descriptors, that may be assigned different packets.

Yes, this is tested.

> Since your not implementing any RX offloads, you might just allocate
> fresh skb's with alloc_skb() and store skb pointer in rxdes3. Since

rxdes3 does not store virtual address of an skb.
It stores the address of the buffer allocated while open() and freed
only when stop().
The data in that buffer will be memcpy()ed to an skb allocated in
ftmac100_rx_packet().
No double free happens.

> hardware doesn't touch it, you can skip cpu_to_le32()/le32_to_cpu()
> there (leave a comment, though).

Agree. Thanks.

> Unless this needs to work for ISA devices, you should drop GFP_DMA
> allocation flag.

Ben mentioned about this in the previous mail. I thought that it is OK
to keep GFP_DMA
on ARM platform, but since you point out this flag is for ISA, I will drop it.

best regards,
Po-Yu Chuang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/