From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felix Fietkau Subject: Re: [ath9k-devel] [PATCH v2 07/46] net/wireless: ath9k: fix DMA API usage Date: Tue, 12 Jul 2011 22:21:05 +0800 Message-ID: References: <280ad9176e6532f231e054b38b952b20580874c5.1310339688.git.mirq-linux@rere.qmqm.pl> <4E1BCF36.2010506@openwrt.org> <20110712095541.GA6236@rere.qmqm.pl> <20110712130316.GA8621@rere.qmqm.pl> Mime-Version: 1.0 (iPhone Mail 8H7) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Felix Fietkau , "netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Jouni Malinen , Senthil Balasubramanian , "ath9k-devel-xDcbHBWguxHbcTqmT+pZeQ@public.gmane.org" , Vasanthakumar Thiagarajan , Ralf Baechle , "linux-mips-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org" To: =?utf-8?Q?Micha=C5=82_Miros=C5=82aw?= Return-path: In-Reply-To: <20110712130316.GA8621-CoA6ZxLDdyEEUmgCuDUIdw@public.gmane.org> Sender: linux-wireless-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: netdev.vger.kernel.org On 12.07.2011, at 21:03, Micha=C5=82 Miros=C5=82aw wrote: > On Tue, Jul 12, 2011 at 08:54:32PM +0800, Felix Fietkau wrote: >> On 12.07.2011, at 17:55, Micha=C5=82 Miros=C5=82aw wrote: >>=20 >>> On Tue, Jul 12, 2011 at 12:36:06PM +0800, Felix Fietkau wrote: >>>> On 2011-07-11 8:52 AM, Micha=C5=82 Miros=C5=82aw wrote: >>>>> Also constify buf_addr for ath9k_hw_process_rxdesc_edma() to veri= fy >>>>> assumptions --- dma_sync_single_for_device() call can be removed. >>>>>=20 >>>>> Signed-off-by: Micha=C5=82 Miros=C5=82aw >>>>> --- >>>>> drivers/net/wireless/ath/ath9k/ar9003_mac.c | 4 ++-- >>>>> drivers/net/wireless/ath/ath9k/ar9003_mac.h | 2 +- >>>>> drivers/net/wireless/ath/ath9k/recv.c | 10 +++------- >>>>> 3 files changed, 6 insertions(+), 10 deletions(-) >>>>>=20 >>>>> diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/= wireless/ath/ath9k/recv.c >>>>> index 70dc8ec..c5f46d5 100644 >>>>> --- a/drivers/net/wireless/ath/ath9k/recv.c >>>>> +++ b/drivers/net/wireless/ath/ath9k/recv.c >>>>> @@ -684,15 +684,11 @@ static bool ath_edma_get_buffers(struct ath= _softc *sc, >>>>> BUG_ON(!bf); >>>>>=20 >>>>> dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr, >>>>> - common->rx_bufsize, DMA_FROM_DEVICE); >>>>> + common->rx_bufsize, DMA_BIDIRECTIONAL); >>>>>=20 >>>>> ret =3D ath9k_hw_process_rxdesc_edma(ah, NULL, skb->data); >>>>> - if (ret =3D=3D -EINPROGRESS) { >>>>> - /*let device gain the buffer again*/ >>>>> - dma_sync_single_for_device(sc->dev, bf->bf_buf_addr, >>>>> - common->rx_bufsize, DMA_FROM_DEVICE); >>>>> + if (ret =3D=3D -EINPROGRESS) >>>>> return false; >>>>> - } >>>>>=20 >>>>> __skb_unlink(skb,&rx_edma->rx_fifo); >>>>> if (ret =3D=3D -EINVAL) { >>>> I have strong doubts about this change. On most MIPS devices, >>>> dma_sync_single_for_cpu is a no-op, whereas >>>> dma_sync_single_for_device flushes the cache range. With this >>>> change, the CPU could cache the DMA status part behind skb->data a= nd >>>> that cache entry would not be flushed inbetween calls to this >>>> functions on the same buffer, likely leading to rx stalls. >>>=20 >>> You're suggesting a platform implementation bug then. If the platfo= rm is not >>> cache-coherent, it should invalidate relevant CPU cache lines for s= ync_to_cpu >>> and unmap cases. Do other devices show such symptoms on MIPS system= s? >>>=20 >>> I'm not familiar with the platform internals, so we should ask MIPS= people. >> I only mentioned MIPS to describe the potential side effect of this = change. From my current understanding of the DMA API, it would be wrong= on other platforms as well. I believe the _for_device function needs t= o be used to transfer ownership of the buffer back to the device, befor= e calling _for_cpu again later for another read. >=20 > What you're saying reminds the wording in DMA-API-HOWTO.txt that I fi= nd > wrong (or at least misleading) compared to what DMA-API.txt describes= =2E > DMA sync calls do not transfer the ownership of the buffer - they are > cache synchronization points, ownership passing is handled entirely b= y > the driver. What I meant was that the DMA sync calls reflect the ownership transfer= of the memory regions. In this case ownership is transferred between d= evice and CPU multiple times and the code reflects that. >=20 >> This is definitely required in this case, because when the return co= de is -EINPROGRESS, the driver waits for the hardware to complete this = buffer, and the next call has to fetch the memory again after the devic= e has updated it. >=20 > Correctness of this access should be provided by sync_to_cpu() call. At least in MIPS I'm sure it isn't. If I remember correctly, it also is= n't on ARM, so I'm pretty sure that either your understanding of the AP= I is incorrect, or arch code does not implement it properly. In either = case, this change (and probably also the p54 one) should not be merged. - Felix-- To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html