Re: [PATCH] rtw88: 8821c: disable the ASPM of RTL8821CE

[Kai-Heng Feng]

On Mon, Dec 13, 2021 at 5:00 PM Pkshih <pkshih@xxxxxxxxxxx> wrote:
>
>
> > -----Original Message-----
> > From: Jian-Hong Pan <jhp@xxxxxxxxxxxxx>
> > Sent: Monday, December 13, 2021 3:31 PM
> > To: Pkshih <pkshih@xxxxxxxxxxx>
> > Cc: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>; Yan-Hsuan Chuang <tony0620emma@xxxxxxxxx>; Kalle Valo
> > <kvalo@xxxxxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxxx
> > Subject: Re: [PATCH] rtw88: 8821c: disable the ASPM of RTL8821CE
> >
> > Pkshih <pkshih@xxxxxxxxxxx> 於 2021年12月11日 週六 下午2:31寫道：
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jian-Hong Pan <jhp@xxxxxxxxxxxxx>
> > > > Sent: Friday, December 10, 2021 5:34 PM
> > > > To: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > > > Cc: Pkshih <pkshih@xxxxxxxxxxx>; Yan-Hsuan Chuang <tony0620emma@xxxxxxxxx>; Kalle Valo
> > > > <kvalo@xxxxxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> > > > linux-kernel@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxxx
> > > > Subject: Re: [PATCH] rtw88: 8821c: disable the ASPM of RTL8821CE
> > > >
> > > > Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> 於 2021年12月10日 週五 下午5:24寫道：
> > > >
> > > > > Right now it seems like only Intel platforms are affected, so can I
> > > > > propose a patch to disable ASPM when its upstream port is Intel?
> > > >
> > > > I only have laptops with Intel chip now.  So, I am not sure the status
> > > > with AMD platforms.
> > > > If this is true, then "disable ASPM when its upstream port is Intel"
> > > > might be a good idea.
> > > >
> > >
> > > Jian-Hong, could you try Kai-Heng's workaround that only turn off ASPM
> > > during NAPI poll function. If it also works to you, I think it is okay
> > > to apply this workaround to all Intel platform with RTL8821CE chipset.
> > > Because this workaround has little (almost no) impact of power consumption.
> >
> > According to Kai-Heng's hack patch [1] and the comment [2] mentioning
> > checking "ref_cnt" by rtw_pci_link_ps(), I arrange the patch as
> > following.
>
> I meant that move "ref_cnt" into rtw_pci_link_ps() by [2], but you remove
> the "ref_cnt". This leads lower performance, because it must turn off
> ASPM after napi_poll() when we have high traffic.
>
> In fact, Kai-Heng's patch is to leave ASPM before napi_poll(), and
> "restore" ASPM setting. So, we still need "ref_cnt".

I am working on the patch for proper upstream inclusion. Will send out soon.

Kai-Heng

>
>
> > This patch only disables ASPM (if the hardware has the capability)
> > when system gets into rtw_pci_napi_poll() and re-enables ASPM when it
> > leaves rtw_pci_napi_poll().  It is as Ping-Ke mentioned "only turn off
> > ASPM during NAPI poll function".
> > The WiFi & BT work, and system is still alive after I use the internet
> > awhile.  Besides, there is no more "pci bus timeout, check dma status"
> > error.
> >
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=215131#c11
> > [2] https://bugzilla.kernel.org/show_bug.cgi?id=215131#c15
> >
> > Jian-Hong Pan
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > index a7a6ebfaa203..a6fdddecd37d 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -1658,6 +1658,7 @@ static int rtw_pci_napi_poll(struct napi_struct
> > *napi, int budget)
> >                                               priv);
> >         int work_done = 0;
> >
> > +       rtw_pci_link_ps(rtwdev, false);
> >         while (work_done < budget) {
> >                 u32 work_done_once;
> >
> > @@ -1681,6 +1682,7 @@ static int rtw_pci_napi_poll(struct napi_struct
> > *napi, int budget)
> >                 if (rtw_pci_get_hw_rx_ring_nr(rtwdev, rtwpci))
> >                         napi_schedule(napi);
> >         }
> > +       rtw_pci_link_ps(rtwdev, true);
> >
> >         return work_done;
> >  }
> >
>
> How about doing this thing only if 8821CE and Intel platform?
> Could you help to add this?
>
> --
> Ping-ke
>
>