> Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> writes: > > > On 2020-06-17 05:30:22 [+0000], Tony Chuang wrote: > >> 0000], Tony Chuang wrote: > >> > > > On 2020-06-05 15:47:03 [+0800], yhchuang@xxxxxxxxxxx wrote: > >> > > > > From: Yan-Hsuan Chuang <yhchuang@xxxxxxxxxxx> > >> > > > > > >> > > > > Some platforms cannot read the DBI register successfully for the > >> > > > > ASPM settings. After the read failed, the bus could be unstable, > >> > > > > and the device just became unavailable [1]. For those platforms, > >> > > > > the ASPM should be disabled. But as the ASPM can help the driver > >> > > > > to save the power consumption in power save mode, the ASPM is still > >> > > > > needed. So, add a module parameter for them to disable it, then > >> > > > > the device can still work, while others can benefit from the less > >> > > > > power consumption that brings by ASPM enabled. > >> > > > > >> > > > Can you set disable_aspm if rtw_dbi_read8() fails? Or make a test if it > >> > > > is save to use? > >> > > > > >> > > > If someone notices the warning they still have to search for the warning > >> > > > in order to make the link towards loading the module with the > >> > > > disable_aspm=1 paramter. > >> > > > Is it known what causes the failure? > >> > > > > >> > > > >> > > I think as long as the rtw_dbi_read() fails, the consequent register > >> > > operation will also fail, and still get an error read/write the register. > >> > > And this is some sort of PCI issue, and I am not really familiar with it. > >> > > Such as the root cause or how it fails. > >> > > >> > Then it does not sound safe to enable it by default. > >> > >> We have had a discussion about this, but I cannot find the thread now. > >> People suggested that the module parameter should not be used. > >> And they think that if the ASPM can help for power consumption, then > >> it should be default enabled. But I think it should be based on that the > >> other platforms will not just fail to bring up the device. However, the > >> platforms are less than the others, not sure if default enable or disable > >> is better. > > > > What I fail to understand is if this error affects other PCI devices as > > well or just this one. And if it is possible to reset the wifi device > > and everything gets back no normal. Or is it just the register reading, > > that spams the log and would affect the system otherwise if you would > > just avoided after the first fail. > > > >> > > If we can default disable it, then we can help those platforms, but > >> > > then other platform will suffer from higher power consumption. > >> > > >> > So for those platform, where the error occurs, you expect that the user > >> > manages to read the error message (a backtrace from rtw_dbi_read8()) > and > >> > connects this the need to set a certain module option. > >> > >> Yes, we can discuss if it should be default enabled or not. Otherwise the > >> people with those platforms can only do that to prevent this. Really bad. > > > > It would be good to know the root cause of this. So then default enable > > would depend on it. > > You could have a allow/forbid list based on DMI once you identified > > good/bad systems but this includes additional maintenance. > > I think there should be this kind of quirk list in rtw88 which would > disable ASPM automatically on problematic platforms. We should not > require the user to figure out the problem on their own and disable ASPM > manually using the module parameter. OK, I'll add a quirk list for the platforms. > > > I think that at the very least, if the read fails you should give the > > user additional information how to stop this from happening again. And > > either stop issuing the commands again or skip driver loading (depending > > what it means for device stability). > > Yes, if we can guess that this is an ASPM problem giving additional > information is very helpful for the user, please do that as well. > Yes, should add additional information for the users, so they can help to report the platforms. Yen-Hsuan
