RE: [PATCH] wifi: ath11k: Add a warning for wcn6855 spurious wakeup events

[Limonciello, Mario]

[Public]

> > On 2/27/23 07:14, Kalle Valo wrote:
> >
> >> Mario Limonciello <mario.limonciello@xxxxxxx> writes:
> >>
> >>> On 2/27/23 06:36, Kalle Valo wrote:
> >>>
> >>>> Mario Limonciello <mario.limonciello@xxxxxxx> writes:
> >>>>
> >>>>> +static void ath11k_check_s2idle_bug(struct ath11k_base *ab)
> >>>>> +{
> >>>>> +	struct pci_dev *rdev;
> >>>>> +
> >>>>> +	if (pm_suspend_target_state != PM_SUSPEND_TO_IDLE)
> >>>>> +		return;
> >>>>> +
> >>>>> +	if (ab->id.device != WCN6855_DEVICE_ID)
> >>>>> +		return;
> >>>>> +
> >>>>> +	if (ab->qmi.target.fw_version >= WCN6855_S2IDLE_VER)
> >>>>> +		return;
> >>>>> +
> >>>>> +	rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0,
> 0));
> >>>>> +	if (rdev->vendor == PCI_VENDOR_ID_AMD)
> >>>>> + ath11k_warn(ab, "fw_version 0x%x may cause spurious wakeups.
> >>>>> Upgrade to 0x%x or later.",
> >>>>> +			    ab->qmi.target.fw_version,
> WCN6855_S2IDLE_VER);
> >>>>
> >>>> I understand the reasons for this warning but I don't really trust the
> >>>> check 'ab->qmi.target.fw_version >= WCN6855_S2IDLE_VER'. I don't
> know
> >>>> how the firmware team populates the fw_version so I'm worried that if
> we
> >>>> ever switch to a different firmware branch (or similar) this warning
> >>>> might all of sudden start triggering for the users.
> >>>>
> >>>
> >>> In that case, maybe would it be better to just have a list of the
> >>> public firmware with issue and ensure it doesn't match one of those?
> >>
> >> You mean ath11k checking for known broken versions and reporting that?
> >> We have so many different firmwares to support in ath11k, I'm not really
> >> keen on adding tests for a specific version.
> >
> > I checked and only found a total of 7 firmware versions published for
> > WCN6855 at your ath11k-firmware repo.  I'm not sure how many went to
> > linux-firmware.  But it seems like a relatively small list to have.
> 
> ath11k supports also other hardware families than just WCN6855, so there
> are a lot of different firmware versions and branches.

Right, but this change was explicitly checking the device ID matched WCN6855.

So it could be a single check for that device and any of the 5 bad firmware binaries.

> 
> >> We have a list of known important bugs in the wiki:
> >>
> >>
> https://wireless.wiki.kernel.org/en/users/drivers/ath11k#known_bugslimita
> tions
> >>
> >> What about adding the issue there, would that get more exposure to the
> >> bug and hopefully the users would upgrade the firmware?
> >>
> >
> > The problem is when this happens users have no way to know it's even
> > caused by wireless.  So why would they go looking at the wireless
> > wiki?
> >
> > The GPIO used for WLAN is different from design to design so we can't
> > put it in the GPIO driver.  There are plenty of designs that have
> > valid reasons to wakeup from other GPIOs as well so it can't just be
> > the GPIO driver IRQ.
> 
> I understand your problem but my problem is that I have three Qualcomm
> drivers to support and that's a major challenge itself. So I try to keep
> the drivers as simple as possible and avoid any hacks.

OK.