Re: [PATCH] wifi: ath12k: Fix pdev id sent to firmware for single phy devices

[Kalle Valo <kvalo@kernel.org>]

Ramya Gnanasekar <quic_rgnanase@quicinc.com> writes:

> From: Lingbo Kong <quic_lingbok@quicinc.com>
>
> Pdev id from mac phy capabilities will be sent as a part of
> HTT/WMI command to firmware. This causes issue with single pdev
> devices where firmware does not respond to the WMI/HTT request
> sent from host.
>
> For single pdev devices firmware expects pdev id as 1 for 5 GHz/6 GHz
> phy and 2 for 2 GHz band. Add wrapper ath12k_mac_get_target_pdev_id()
> to help fetch right pdev for single pdev devices.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>
> Signed-off-by: Lingbo Kong <quic_lingbok@quicinc.com>
> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@quicinc.com>

[...]

> +static bool ath12k_mac_band_match(enum nl80211_band band1, enum WMI_HOST_WLAN_BAND band2)
> +{
> +	return (((band1 == NL80211_BAND_2GHZ) && (band2 & WMI_HOST_WLAN_2G_CAP)) ||
> +		(((band1 == NL80211_BAND_5GHZ) || (band1 == NL80211_BAND_6GHZ)) &&
> +		   (band2 & WMI_HOST_WLAN_5G_CAP)));
> +}

This is not really pleasent to read. What about something like this:

switch (band1) {
case NL80211_BAND_2GHZ:
        if (band2 & WMI_HOST_WLAN_2G_CAP)
                return true;

        break;
case NL80211_BAND_5GHZ:
case NL80211_BAND_6GHZ:
        if (band2 & WMI_HOST_WLAN_5G_CAP)
                return true;

        break;
}

return false;

Or any other ideas?

> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar)
> +{
> +	struct ath12k_vif *arvif;
> +	struct ath12k_base *ab = ar->ab;
> +
> +	if (!ab->hw_params->single_pdev_only)
> +		return ar->pdev->pdev_id;
> +
> +	arvif = ath12k_mac_get_vif_up(ar);
> +
> +	if (arvif)
> +		return ath12k_mac_get_target_pdev_id_from_vif(arvif);
> +	else
> +		return ar->ab->fw_pdev[0].pdev_id;
> +}

I find this easier to read:

arvif = ath12k_mac_get_vif_up(ar);
if (!arvif)
	return ar->ab->fw_pdev[0].pdev_id;

return ath12k_mac_get_target_pdev_id_from_vif(arvif);

But I still would prefer to have some a code comment explaining the idea
behind here, especially why it's safe to use fw_pdev[0].pdev_id directly
and what scenario that is.

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

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