linux-kernel - Re: [PATCH 5/6] wifi: ath11k: add multipd support for QCN6122

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <DS7PR19MB88834F5A352F93EE909AB42F9DFAA@DS7PR19MB8883.namprd19.prod.outlook.com>
Date: Wed, 29 Oct 2025 21:41:14 +0400
From: George Moussalem <george.moussalem@...look.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 Johannes Berg <johannes@...solutions.net>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jeff Johnson <jjohnson@...nel.org>
Cc: linux-wireless@...r.kernel.org, devicetree@...r.kernel.org,
 ath11k@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] wifi: ath11k: add multipd support for QCN6122



On 10/29/25 18:43, Krzysztof Kozlowski wrote:
> On 29/10/2025 15:26, George Moussalem via B4 Relay wrote:
>> From: George Moussalem <george.moussalem@...look.com>
>>
>> IPQ5018/QCN6122 platforms use multi PD (protection domains) to avoid
>> having one instance of the running Q6 firmware crashing resulting in
>> crashing the others.
>>
>> The IPQ5018 platform can have up to two QCN6122 wifi chips.
>> To differentiate the two, the PD instance number (1 or 2) is added to
>> the QMI service instance ID, which the QCN6122 firmware also expects.
>> IPQ5018 internal wifi is always the first PD while QCN6122 cards must be
>> second or third.
>>
>> Signed-off-by: George Moussalem <george.moussalem@...look.com>
>> ---
>> See below patch for more info:
>> https://lore.kernel.org/all/20231110091939.3025413-1-quic_mmanikan@quicinc.com/
> 
> I don't see any common part with that. Your bindings are completely
> different and while PD was justified there, there is no such
> justification here. Neither in the bindings.

Will adjust the description in the bindings and add more details. Mind
you that I don't have access to the proprietary designs / data sheets,
so it will be based on my understanding:

The firmware running on the Q6 remote processor takes WCSS - wifi
functionality - out of reset and brings them up. Each wifi radio on the
IPQ5018/QCN6122 platform runs on a separate process (referred to as
protection domain) which avoids one that crashed crashing the others.
The firmware and kernel driver communicate over QMI leveraging the user
PD IDs as instance IDs to distinguish between multiple QCN6122 wifi chips.

With that, would the additional property in the bindings be considered /
accepted? Else, any guidance would be appreciated.

> 
>> ---
>>  drivers/net/wireless/ath/ath11k/ahb.c  | 31 +++++++++++++++++++++++++++++++
>>  drivers/net/wireless/ath/ath11k/core.h |  4 ++++
>>  drivers/net/wireless/ath/ath11k/pci.c  |  1 +
>>  3 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
>> index 7b267dd62e964b2c4d6c3bbe016abd1ad0297219..820a383e88caf125892176e421b0121fed7e7055 100644
>> --- a/drivers/net/wireless/ath/ath11k/ahb.c
>> +++ b/drivers/net/wireless/ath/ath11k/ahb.c
>> @@ -429,6 +429,7 @@ static void ath11k_ahb_init_qmi_ce_config(struct ath11k_base *ab)
>>  	cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len;
>>  	cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map;
>>  	ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id;
>> +	ab->qmi.service_ins_id += ab->userpd_id;
>>  }
>>  
>>  static void ath11k_ahb_free_ext_irq(struct ath11k_base *ab)
>> @@ -1101,6 +1102,28 @@ static int ath11k_ahb_fw_resources_init(struct ath11k_base *ab)
>>  	return ret;
>>  }
>>  
>> +static int ath11k_get_userpd_id(struct device *dev, int *userpd)
>> +{
>> +	int ret, userpd_id;
>> +
>> +	ret = of_property_read_u32(dev->of_node, "qcom,userpd", &userpd_id);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (userpd_id) {
>> +	case 2:
>> +		*userpd = ATH11K_QCN6122_USERPD_2;
>> +		break;
>> +	case 3:
>> +		*userpd = ATH11K_QCN6122_USERPD_3;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>>  static int ath11k_ahb_fw_resource_deinit(struct ath11k_base *ab)
>>  {
>>  	struct ath11k_ahb *ab_ahb = ath11k_ahb_priv(ab);
>> @@ -1142,6 +1165,7 @@ static int ath11k_ahb_probe(struct platform_device *pdev)
>>  	const struct ath11k_hif_ops *hif_ops;
>>  	const struct ath11k_pci_ops *pci_ops;
>>  	enum ath11k_hw_rev hw_rev;
>> +	int userpd_id = 0;
>>  	int ret;
>>  
>>  	hw_rev = (uintptr_t)device_get_match_data(&pdev->dev);
>> @@ -1160,6 +1184,12 @@ static int ath11k_ahb_probe(struct platform_device *pdev)
>>  	case ATH11K_HW_QCN6122_HW10:
>>  		hif_ops = &ath11k_ahb_hif_ops_qcn6122;
>>  		pci_ops = &ath11k_ahb_pci_ops_wcn6750;
>> +		ret = ath11k_get_userpd_id(&pdev->dev, &userpd_id);
>> +		if (ret) {
>> +			dev_err(&pdev->dev, "failed to get userpd: %d\n", ret);
>> +			return ret;
>> +		}
>> +		dev_info(&pdev->dev, "multi-pd architecture - userpd: %d\n", userpd_id);
> 
> This does not look like useful printk message. Drivers should be silent
> on success:
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/coding-style.rst#L913
> https://elixir.bootlin.com/linux/v6.15-rc7/source/Documentation/process/debugging/driver_development_debugging_guide.rst#L79
> 
>>  		break;
>>  	default:
> 
> Best regards,
> Krzysztof
>