Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758265AbcK3Tt0 (ORCPT ); Wed, 30 Nov 2016 14:49:26 -0500 Received: from mout.gmx.net ([212.227.17.21]:59287 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754676AbcK3Tsy (ORCPT ); Wed, 30 Nov 2016 14:48:54 -0500 Subject: Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver To: Florian Fainelli , davem@davemloft.net, charrer@alacritech.com, liodot@gmail.com, gregkh@linuxfoundation.org, andrew@lunn.ch References: <1480162850-8014-1-git-send-email-LinoSanfilippo@gmx.de> <1480162850-8014-2-git-send-email-LinoSanfilippo@gmx.de> <6b7de79d-b149-9888-2d0f-2acee5c2905e@gmx.de> <34c8e4b3-ccbc-082e-1dd0-3893b56e475a@gmail.com> Cc: devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, netdev@vger.kernel.org From: Lino Sanfilippo Message-ID: <02068116-067a-3df9-b0cd-3ad60bcad9ff@gmx.de> Date: Wed, 30 Nov 2016 20:48:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <34c8e4b3-ccbc-082e-1dd0-3893b56e475a@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Provags-ID: V03:K0:dh8eqq584XzdxuTxlF7qJOBJPuGHkyBXeUkuTwYJlBfmY5dx9Lu JpL7DvnoaP5FhgmAn2dNsG2eArDdKRQ+lzSHtMF2Nf7MVR9/tnrByLPUGgNgLqmaw2taU+7 9fv5wsYDEIHVe4YFk7+66GdrZ0+9qjEmeUFHoMklKXItC1DiOp3b9P5DzeYMNwFBuojJr/k w7Y+bvgBV6339pKRcZpjQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:Yu6YwOMP5uY=:FiW8E8uS7S/ouR1rckY1bq MGeHi706Z1irQxFTnTZS2YCVrjAYgaySfftWscDtUKQpMuRtFIYZvdKYtuhIf+UllMHXl1hJl /dagvfTf1W0r4GL0vXF/UK6E8pjPsFW+cS0hZ/JpMn+hCy+IlQ3NKpyk4ZYVAvpHyqRVbxkvq MTGjfZ/8zaQpxJPoZrBiYKF6DTkImN+vb0VoSuk8or6Ok/2vDAYxksfGGTDBjut8czDkT6dps b4hucooCvn++6Lh5LTjAdc5tuV4uF79IPabxHkK7/lZjh5zoVpZpVTjplG8hu2mUzX1KPV6jm E+iD7cSHLCkyyPhX5Ykmxw1OQPcvUsOw9YyUokGhgMMwmXnZZiMy3fKJT8F8aG5h8k1lQ6u01 /OvS/lnMDTwUAQzoZ+/8feHZ2K651+E5/BBhpECl/0LvNEnIvrVMn40dT3p1nToWcwGnug/sP BBIhkoR6Kto/YqZ+fOdvpXWbLna5EtxK6A9s+9FkpFBhzPXUAJAeVv3FkoHr4z5noux5MMcPE BZJPtCiTigFdT6//hm48rAgwf3nxuCWKPJoySvb50lz5pqfiPJeEg1y2ZiOLDo9eNQTmjB+ZK BPbtJAK7c25viBz74Qnn3aTEnKa6VA2CvelI3DnWriKBXTzSip2FDm1GqNCWv55dawqSe3jse WKEMh29IE/53Anl9pGJxOtQeCWOajDy6EZUx5FuTBdygDU8ulDS63se96N6Pcb6pTzmSj1/B3 s8bQgNEsW2KQQjo6WYP+LyzR4utuSta+3LyR80qXfpELRQVwLhzh9h8UeltIxvx8CfWVxmBRD ViGiJ/L Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4823 Lines: 131 On 29.11.2016 18:14, Florian Fainelli wrote: > 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. > Agreed, I will add such a comment. >> >>> [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. > Ok, will fix this. Good catch BTW! >> >>> [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). Um. In the rx path the SKB will be consumed (by napi_gro_receive()). AFAIK we _have_ to unmap it before this call. Doing only a dma_sync_single_for_cpu() for the packet contents does IMHO only make sense if the corresponding SKB is reused somehow. But this is not the case. The rx buffers are refilled with newly allocated SKBs each time, and thus we need to create a new dma mapping for each of them. Or do I still misunderstand when to call the dma sync functions? BTW: I just realized that if the descriptor has not been used by the HW yet, see: + 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; + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ dma_sync_single_for_device missing there has to be a dma_sync_single_for_device to undo the sync for cpu (since the HW will write to this descr when the next rx packet arrives), right? But this is racy: What if the HW writes to that descr after we synced it for cpu but before we synced it for the HW again? Any ideas? Regards, Lino