Authenticated-By: 
From:   Tony Chuang <yhchuang@realtek.com>
To:     Tony Chuang <yhchuang@realtek.com>,
        Brian Norris <briannorris@chromium.org>
CC:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "johannes@sipsolutions.net" <johannes@sipsolutions.net>,
        "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
        Pkshih <pkshih@realtek.com>, Andy Huang <tehuang@realtek.com>,
        "sgruszka@redhat.com" <sgruszka@redhat.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: RE: [PATCH v4 03/13] rtw88: hci files
Thread-Topic: [PATCH v4 03/13] rtw88: hci files
Thread-Index: AQHUuFCwL7VQDfSYhE2MAb8drhRitKXJdAKAgBDPKrCAAzN8gA==
Date:   Wed, 13 Feb 2019 11:08:13 +0000
Message-ID: <F7CD281DE3E379468C6D07993EA72F84D17A5B7D@RTITMBSVM04.realtek.com.tw>
References: <1548820940-15237-1-git-send-email-yhchuang@realtek.com>
 <1548820940-15237-4-git-send-email-yhchuang@realtek.com>
 <20190131223609.GA191502@google.com>
 <F7CD281DE3E379468C6D07993EA72F84D17A3A5E@RTITMBSVM04.realtek.com.tw>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D17A3A5E@RTITMBSVM04.realtek.com.tw>
Accept-Language: zh-TW, en-US
Content-Language: zh-TW
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Hi

> On Behalf Of Tony Chuang
> Subject: RE: [PATCH v4 03/13] rtw88: hci files
> 
> > From: Brian Norris [mailto:briannorris@chromium.org]
> >
> > Hi,
> >
> > A few scattered comments:
> >
> > > +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;
> > > +	}
> >
> >
> > Umm, what are you trying to do here? This is a non-sensical loop. I
> > *imagine* you're trying to do some kind of timeout loop here, but since
> > there's nothing telling the compiler that this is anything but normal
> > memory, this loop gets flattened by the compiler into a single check of
> > ->total_pkt_size (I checked; my compiler gets rid of the loop).
> >
> > So, at a minimum, you should just remove the loop. But I'm not sure if
> > this "check" function has any value at all...
> >
> > > +
> > > +	if (i >= 20)
> > > +		rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
> >
> > ...BTW, I'm seeing this get triggered quite a bit.
> 
> It's quite strange though... triggered on my laptop as well.
> I am looking for the reason that cause it. I found that 8822BE does not
> trigger this. And it's like even if I added a volatile before it to hint the
> compiler not to optimize, it still happens. Now trying to figure out why
> the bus continues to timeout.
> 
> After this problem is resolved I'll either remove the loop or add proper
> hint for the compiler to check the value. But it seems to take many days
> to debug, so I think for this patch set I can just remain it here.
> 
> >
> > Do you have some kind of memory mapping/ordering issue or something? I
> > wouldn't think you should expect to just drop packets on the floor so
> > often like this.
> >
> 
> Looks like the cause is that the DMA might not have done when the interrupt
> arrived. Need to do more test to figure it out.
> 


I tested and checked again. I found that I didn't enable DMA sync for
the TRX path. After some work it can be resolved if I turn DMA sync on.
So I will include this in the PATCH v5.


> 
> 
> Yan-Hsuan
> 

Yan-Hsuan