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

[Kalle Valo <kvalo@kernel.org>]

Mario Limonciello <mario.limonciello@amd.com> writes:

> On 2/27/23 07:14, Kalle Valo wrote:
>
>> Mario Limonciello <mario.limonciello@amd.com> writes:
>>
>>> On 2/27/23 06:36, Kalle Valo wrote:
>>>
>>>> Mario Limonciello <mario.limonciello@amd.com> 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.

>> We have a list of known important bugs in the wiki:
>>
>> https://wireless.wiki.kernel.org/en/users/drivers/ath11k#known_bugslimitations
>>
>> 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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches