| 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
| ||
|
Message-ID: <CAOX2RU6NGVi1j-Es=t5LkqbOb9LCfWC9Rkqa4x00dPWqXUDfKw@mail.gmail.com>
Date: Mon, 7 Nov 2022 18:15:36 +0100
From: Robert Marko <robimarko@...il.com>
To: Jeffrey Hugo <quic_jhugo@...cinc.com>
Cc: mani@...nel.org, 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 Mon, 7 Nov 2022 at 16:10, Jeffrey Hugo <quic_jhugo@...cinc.com> wrote:
>
> On 11/5/2022 1:49 PM, 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
> >
> > 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.
> >
> > 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));
> > +}
>
> What kind of synchronization is there for this?
None from what I could tell.
>
> Does SBL spin until this is set?
No, the default value is 0x0 and it will boot with that.
>
> What would prevent SBL from booting, sending the notification to the
> host, and then quickly entering runtime mode before the host had a
> chance to get here?
As far as I know nothing really, but this is a question for QCA.
I have tried to make a generic solution from various bits and pieces they
are using downstream but which are really not suitable for upstream.
I agree it's not ideal, but the worst that could happen is that card
won't work which is current
behavior anyway.
Not being able to use AHB + PCI or multiple PCI cards is really
annoying as I am not able
to utilize most of the 5GHz spectrum on my router due to this.
Regards,
Robert
>
>
> > +
> > 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);
> > + break;
> > default:
> > break;
> > }
> > }
> >
> > -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 int ath11k_mhi_read_addr_from_dt(struct mhi_controller *mhi_ctrl)
> > {
> > struct device_node *np;
> > diff --git a/drivers/net/wireless/ath/ath11k/mhi.h b/drivers/net/wireless/ath/ath11k/mhi.h
> > index 8d9f852da695..0db308bc3047 100644
> > --- a/drivers/net/wireless/ath/ath11k/mhi.h
> > +++ b/drivers/net/wireless/ath/ath11k/mhi.h
> > @@ -16,6 +16,9 @@
> > #define MHICTRL 0x38
> > #define MHICTRL_RESET_MASK 0x2
> >
> > +#define BHI_ERRDBG2 0x38
> > +#define QRTR_INSTANCE_MASK GENMASK(7, 0)
> > +
> > int ath11k_mhi_start(struct ath11k_pci *ar_pci);
> > void ath11k_mhi_stop(struct ath11k_pci *ar_pci);
> > int ath11k_mhi_register(struct ath11k_pci *ar_pci);
> > diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> > index 99cf3357c66e..cd26c1567415 100644
> > --- a/drivers/net/wireless/ath/ath11k/pci.c
> > +++ b/drivers/net/wireless/ath/ath11k/pci.c
> > @@ -370,13 +370,16 @@ static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
> > static void ath11k_pci_init_qmi_ce_config(struct ath11k_base *ab)
> > {
> > struct ath11k_qmi_ce_cfg *cfg = &ab->qmi.ce_cfg;
> > + struct ath11k_pci *ab_pci = ath11k_pci_priv(ab);
> > + struct pci_bus *bus = ab_pci->pdev->bus;
> >
> > cfg->tgt_ce = ab->hw_params.target_ce_config;
> > cfg->tgt_ce_len = ab->hw_params.target_ce_count;
> >
> > cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map;
> > cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len;
> > - ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id;
> > + ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id +
> > + (((pci_domain_nr(bus) & 0xF) << 4) | (bus->number & 0xF));
> >
> > ath11k_ce_get_shadow_config(ab, &cfg->shadow_reg_v2,
> > &cfg->shadow_reg_v2_len);
>
Powered by blists - more mailing lists