| 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
| ||
|
Date: Mon, 7 Nov 2022 11:02:32 -0700
From: Jeffrey Hugo <quic_jhugo@...cinc.com>
To: Robert Marko <robimarko@...il.com>,
Manivannan Sadhasivam <mani@...nel.org>
CC: <kvalo@...nel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
<kuba@...nel.org>, <pabeni@...hat.com>,
<gregkh@...uxfoundation.org>, <elder@...aro.org>,
<hemantk@...eaurora.org>, <quic_qianyu@...cinc.com>,
<bbhatt@...eaurora.org>, <mhi@...ts.linux.dev>,
<linux-arm-msm@...r.kernel.org>, <ath11k@...ts.infradead.org>,
<linux-wireless@...r.kernel.org>, <netdev@...r.kernel.org>,
<ansuelsmth@...il.com>
Subject: Re: [PATCH 2/2] wifi: ath11k: use unique QRTR instance ID
On 11/7/2022 10:52 AM, Robert Marko wrote:
> On Mon, 7 Nov 2022 at 18:47, Manivannan Sadhasivam <mani@...nel.org> wrote:
>>
>> On Sat, Nov 05, 2022 at 08:49:43PM +0100, Robert Marko wrote:
>>> Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
>>> will cause a clash in the QRTR instance node ID and prevent the driver
>>> from talking via QMI to the card and thus initializing it with:
>>> [ 9.836329] ath11k c000000.wifi: host capability request failed: 1 90
>>> [ 9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
>>>
>>
>> There is still an outstanding issue where you cannot connect two WLAN modules
>> with same node id.
>
> Yes, but as far as I can understand QRTR that is never gonna be
> possible, node ID-s
> must be different, but I dont have any docs at all.
>
>>
>>> So, in order to allow for this combination of cards, especially AHB + PCI
>>> cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
>>> QRTR instance ID offset by calculating a unique one based on PCI domain
>>> and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
>>> using the SBL state callback that is added as part of the series.
>>> We also have to make sure that new QRTR offset is added on top of the
>>> default QRTR instance ID-s that are currently used in the driver.
>>>
>>
>> Register BHI_ERRDBG2 is listed as Read only from Host as per the BHI spec.
>> So I'm not sure if this solution is going to work on all ath11k supported
>> chipsets.
>>
>> Kalle, can you confirm?
>>
>>> This finally allows using AHB + PCI or multiple PCI cards on the same
>>> system.
>>>
>>> Before:
>>> root@...nWrt:/# qrtr-lookup
>>> Service Version Instance Node Port
>>> 1054 1 0 7 1 <unknown>
>>> 69 1 2 7 3 ATH10k WLAN firmware service
>>>
>>> After:
>>> root@...nWrt:/# qrtr-lookup
>>> Service Version Instance Node Port
>>> 1054 1 0 7 1 <unknown>
>>> 69 1 2 7 3 ATH10k WLAN firmware service
>>> 15 1 0 8 1 Test service
>>> 69 1 8 8 2 ATH10k WLAN firmware service
>>>
>>> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
>>> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
>>>
>>> Signed-off-by: Robert Marko <robimarko@...il.com>
>>> ---
>>> drivers/net/wireless/ath/ath11k/mhi.c | 47 ++++++++++++++++++---------
>>> drivers/net/wireless/ath/ath11k/mhi.h | 3 ++
>>> drivers/net/wireless/ath/ath11k/pci.c | 5 ++-
>>> 3 files changed, 38 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
>>> index 86995e8dc913..23e85ea902f5 100644
>>> --- a/drivers/net/wireless/ath/ath11k/mhi.c
>>> +++ b/drivers/net/wireless/ath/ath11k/mhi.c
>>> @@ -294,6 +294,32 @@ static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
>>> {
>>> }
>>>
>>> +static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
>>> + void __iomem *addr,
>>> + u32 *out)
>>> +{
>>> + *out = readl(addr);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
>>> + void __iomem *addr,
>>> + u32 val)
>>> +{
>>> + writel(val, addr);
>>> +}
>>> +
>>> +static void ath11k_mhi_qrtr_instance_set(struct mhi_controller *mhi_cntrl)
>>> +{
>>> + struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
>>> +
>>> + ath11k_mhi_op_write_reg(mhi_cntrl,
>>> + mhi_cntrl->bhi + BHI_ERRDBG2,
>>> + FIELD_PREP(QRTR_INSTANCE_MASK,
>>> + ab->qmi.service_ins_id - ab->hw_params.qmi_service_ins_id));
>>> +}
>>> +
>>> static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
>>> {
>>> switch (reason) {
>>> @@ -315,6 +341,8 @@ static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
>>> return "MHI_CB_FATAL_ERROR";
>>> case MHI_CB_BW_REQ:
>>> return "MHI_CB_BW_REQ";
>>> + case MHI_CB_EE_SBL_MODE:
>>> + return "MHI_CB_EE_SBL_MODE";
>>> default:
>>> return "UNKNOWN";
>>> }
>>> @@ -336,27 +364,14 @@ static void ath11k_mhi_op_status_cb(struct mhi_controller *mhi_cntrl,
>>> if (!(test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags)))
>>> queue_work(ab->workqueue_aux, &ab->reset_work);
>>> break;
>>> + case MHI_CB_EE_SBL_MODE:
>>> + ath11k_mhi_qrtr_instance_set(mhi_cntrl);
>>
>> I still don't understand how SBL could make use of this information during
>> boot without waiting for an update.
>
> Me neither, but it works reliably as long as it's updated once SBL is live.
> Trying to do it earlier or later does nothing, it will just use the
> default node ID then.
If I recall correctly, PBL will write to the register at the end of the
BHI process, even in the success case. So, you have a race condition
where you need to update the register after BHI is complete, but before
SBL reads it.
If the value is just based on the BDF, I don't see why the device
couldn't compute it without input from the host. This whole mechanism
seems pretty poorly designed, but sadly I don't have any brilliant ideas
on how to fix it for you.
Powered by blists - more mailing lists