Hi,
On 20/03/2025 08:23, Vasanthakumar Thiagarajan wrote:
On 3/19/2025 7:03 PM, Dmitry Baryshkov wrote:
On Wed, Mar 19, 2025 at 06:24:57PM +0530, Vasanthakumar Thiagarajan wrote:
On 3/19/2025 5:21 PM, Dmitry Baryshkov wrote:
On Wed, Mar 19, 2025 at 05:02:39PM +0530, Vasanthakumar Thiagarajan wrote:
On 3/19/2025 3:57 PM, Krzysztof Kozlowski wrote:
On 19/03/2025 10:06, Vasanthakumar Thiagarajan wrote:
---
base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
change-id: 20250303-topic-ath12k-fix-crash-49e9055c61a1
Best regards,
NAK since this will break QCN
There is a series under internal review to address MLO issues for WCN chipsets
???
The original commit is wrong, this fixes the conversion, nothing else.
Nope. Driver changes to enable MLO with WCN chipset are not there yet.
Setting the mlo capability flag without having required driver changes
for WCN chipset will likely result in firmware crash. So the recommendation
is to enable MLO (in WCN) only when all the necessary driver changes
(in development, public posting in near future) are in place.
Really, these are your answers? There is regression and first reply is
upstream should wait for whatever you do internally. Second answer is
the same - public posting in near future?
May be I was not clear in my response. I was not telling MLO bug fixes were
in the development. Actually the MLO feature itself is not enabled
yet with WCN chip sets. Any code changes enabling it without full feature
support would result in firmware crashes with the existing firmware binaries
available in upstream.
Is there an undocumented change of the behaviour in the commit
46d16f7e1d14 ("wifi: ath12k: rename mlo_capable_flags to
single_chip_mlo_supp")?
diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c
- if (resp.single_chip_mlo_support_valid) {
- if (resp.single_chip_mlo_support)
- ab->mlo_capable_flags |= ATH12K_INTRA_DEVICE_MLO_SUPPORT;
- else
- ab->mlo_capable_flags &= ~ATH12K_INTRA_DEVICE_MLO_SUPPORT;
- }
The above logic seems to keep the initialized intra MLO support even when
single_chip_mlo_support_valid is not set. The above code removal is correct as
MLO support can not be enabled in host when firmware does not advertise it.
Ack
diff --git a/drivers/net/wireless/ath/ath12k/core.c b/drivers/net/wireless/ath/ath12k/core.c
You skipped an important chunk:
- ab->mlo_capable_flags = ATH12K_INTRA_DEVICE_MLO_SUPPORT;
+ ab->single_chip_mlo_supp = false;
Is this an _undocumented_ change? I think it is. If the developer has
described that the commit disables MLO, there would be no such
questions.
diff --git a/drivers/net/wireless/ath/ath12k/qmi.c b/drivers/net/wireless/ath/ath12k/qmi.c
+ if (resp.single_chip_mlo_support_valid &&
+ resp.single_chip_mlo_support)
+ ab->single_chip_mlo_supp = true;
The above code does it in right way. Overriding firmware MLO capability as done
in the submitted patch under review is obviously wrong. The firmware used to report
the issue seems to have an odd behavior: 1. it does not seem to advertise MLO
capability in single_chip_mlo_support bit and 2. expects configurations to enable
MLO from host. None of the WCN firmware available in upstream either advertises
MLO capability or expects configurations to enable MLO from host.
Additionally, from the commit message:
For the WCN7850 family of chipsets, since the event is not supported,
assumption is made that single chip MLO is supported.
However the code doesn't contain that change. Instead single chip MLO is
unconditionally disabled.
I guess, Neil's change should be reworked to be limited to WCN7850 only,
however it must be done as it is what was expected from the commit
message.
There has been lots of rework gone in to ath12k driver towards enabling
MLO support for QCN chip set. As of now, MLO boot and runtime configurations
are restricted to QCN chipset. WCN will not work with those MLO changes.
Re-enabling single_chip_mlo_supp equivalent (single_chip_mlo_supp got cleaned up in ath/linux-next) will create issues when MLO gets enabled for WCN. Driver
changes/hacks to support this non-upstream firmware specific behavior will
become a major challenge over a period as new features are getting added in driver/firmware.
Making sure a driver doesn't regress with a fw is a major challenge for everyone,
this is why we CI test the kernel on different platform and make sure
it still works over time.
Please share your roadmap with the community and add some documentation
explaining which firmwares & features are currently supported and which
features are in development. It's something usually done in a TODO and
ROADMAP file in the driver directory.
And honestly, please report a warning in the kernel when using a non-supported
firmware version, because Qualcomm ships hundreds of different firmwares
with cryptic namings working with different driver versions and the situation
is honestly impossible to handle for developers & customers when setting up
systems based on mainline Linux.
Finally, it seems you only test the WCN on an x86 platform, and since you ship
the WCN along (at least) the SM8550, SM8650 and SM8750 SoCs (which is like millions
of phones) and since those SoCs are pretty well supported in mainline Linux,
these targets should be validated aswell since they have different power-up
requirements than the standalone PCIe card.
Last note, there's no "upstream firmware", the linux-firmware repository is a handy
collection of firmwares shared by vendors in a central place with clear reditribution
licencing, but it cannot be considered as "upstream" like the Linux code is.
So expliciting which firmware version is supported in the documentation and the
code is the preferred way.
Thanks,
Neil
Vasanth
