Date:   Mon, 25 Mar 2019 14:33:48 +0000
From:   Brian Norris <briannorris@chromium.org>
To:     Stanislaw Gruszka <sgruszka@redhat.com>
Cc:     Tony Chuang <yhchuang@realtek.com>,
        "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
        Pkshih <pkshih@realtek.com>, Andy Huang <tehuang@realtek.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        Grant Grundler <grundler@chromium.org>
Subject: Re: [RFC v2 03/12] rtw88: hci files
Message-ID: <20190325143258.wqpy4vt2o3jmb4lq@penguin>
References: <1538553748-26364-1-git-send-email-yhchuang@realtek.com>
 <1538553748-26364-4-git-send-email-yhchuang@realtek.com>
 <20181004130212.GC16819@redhat.com>
 <F7CD281DE3E379468C6D07993EA72F84D172A54A@RTITMBSVM01.realtek.com.tw>
 <20181008093435.GE1961@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181008093435.GE1961@redhat.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

+ Grant

I'm sorry (a little) to drudge up old revisions. But I happened to be
looking back at prior reviews, since I'm reviewing the latest, and I
realized I had this same (then-unresolved) confusion on later versions.
Maybe I should have read more before reviewing... Oh well. Comments
below:

On Mon, Oct 08, 2018 at 11:34:36AM +0200, Stanislaw Gruszka wrote:
> On Fri, Oct 05, 2018 at 12:07:06PM +0000, Tony Chuang wrote:
> > > > +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> > > > +			      struct rtw_pci_rx_ring *rx_ring,
> > > > +			      u32 idx)
> > > > +{
> > > > +	struct rtw_chip_info *chip = rtwdev->chip;
> > > > +	struct rtw_pci_rx_buffer_desc *buf_desc;
> > > > +	u32 desc_sz = chip->rx_buf_desc_sz;
> > > > +	u16 total_pkt_size;
> > > > +	int i;
> > > > +
> > > > +	buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> > > > +						     idx * desc_sz);
> > > > +	for (i = 0; i < 20; i++) {
> > > > +		total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> > > > +		if (total_pkt_size)
> > > > +			return;
> > > > +	}
> > > > +
> > > > +	if (i >= 20)
> > > > +		rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
> > > This is not right, most likely you need to use

Indeed, this was not right. Tony ended up removing it entirely later on,
based on my independent (sorry, didn't read this!) suggestion.

> > > dma_sync_single_for_cpu() .
> > 
> > Not really understand how dma_sync_single_for_cpu() works.
> > Can you show me if possible?
> 
> dma_sync_single_for_cpu() and dma_sync_single_for_device() 
> transfer dma buffer ownership to respectivly CPU and device.
> It is well documented in:
> Documentation/DMA-API-HOWTO.txt:  
> Documentation/DMA-API.txt

Yes, if we expect to need a sort of polling loop like this here, then it
would be important to flush any caches (and, as Grant noted later, it
would probably help to ensure some kind of consistent time deadline --
udelay(), or maybe just check against ktime_get()).

I'll also note here as I did on the later series: these rings are
allocated with consistent memory, so as long as the device is only
signalling this interrupt after it has completed writing to memory, then
we should be reading the latest copy and there should be no need for a
polling loop.

And Tony apparently agreed with me; the loop is gone in the latest, and
instead, we simply read once before emitting a warning.

Regards,
Brian