Re: [PATCH net] wifi: ath12k: properly set single_chip_mlo_supp to true in ath12k_core_alloc()

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry