| 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: <60425d94-a8df-4183-918a-52fca291dfd8@163.com>
Date: Wed, 4 Jun 2025 00:00:42 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: lpieralisi@...nel.org, bhelgaas@...gle.com,
manivannan.sadhasivam@...aro.org, kw@...ux.com, cassel@...nel.org,
robh@...nel.org, jingoohan1@...il.com, linux-pci@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v12 6/6] PCI: cadence: Use cdns_pcie_find_*capability to
avoid hardcode.
On 2025/6/3 17:49, Ilpo Järvinen wrote:
> On Thu, 15 May 2025, Hans Zhang wrote:
>
>> The PCIe capability/extended capability offsets are not guaranteed to be
>> the same across all SoCs integrating the Cadence PCIe IP. Hence, use the
>> cdns_pcie_find_{ext}_capability() APIs for finding them.
>
> A minor point perhaps, but IMO, controller drivers should use the core's
> capability search regardless of the offset being same or not. :-)
Dear Ilpo,
I agree. In the future code iterations, we are committed to finding
better methods.
>
>> This avoids hardcoding the offsets in the driver.
>>
>> Signed-off-by: Hans Zhang <18255117159@....com>
>> ---
>> Changes since v8 ~ v11:
>> - None
>>
>> Changes since v7:
>> - Resolve compilation errors.
>>
>> Changes since v6:
>> https://lore.kernel.org/linux-pci/20250323164852.430546-4-18255117159@163.com/
>>
>> - The patch commit message were modified.
>>
>> Changes since v5:
>> https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com
>>
>> - Kconfig add "select PCI_HOST_HELPERS"
>> ---
>> .../pci/controller/cadence/pcie-cadence-ep.c | 40 +++++++++++--------
>> drivers/pci/controller/cadence/pcie-cadence.h | 5 ---
>> 2 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> index 599ec4b1223e..5c4b2151d181 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
>> @@ -19,12 +19,13 @@
>>
>> static u8 cdns_pcie_get_fn_from_vfn(struct cdns_pcie *pcie, u8 fn, u8 vfn)
>> {
>> - u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
>> u32 first_vf_offset, stride;
>> + u16 cap;
>>
>> if (vfn == 0)
>> return fn;
>>
>> + cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
>> first_vf_offset = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_SRIOV_VF_OFFSET);
>> stride = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_SRIOV_VF_STRIDE);
>> fn = fn + first_vf_offset + ((vfn - 1) * stride);
>> @@ -36,10 +37,11 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
>> struct pci_epf_header *hdr)
>> {
>> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> - u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
>> struct cdns_pcie *pcie = &ep->pcie;
>> u32 reg;
>> + u16 cap;
>>
>> + cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
>> if (vfn > 1) {
>> dev_err(&epc->dev, "Only Virtual Function #1 has deviceID\n");
>> return -EINVAL;
>> @@ -224,9 +226,10 @@ static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 mmc)
>> {
>> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> struct cdns_pcie *pcie = &ep->pcie;
>> - u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
>> u16 flags;
>> + u8 cap;
>>
>> + cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
>> fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>>
>> /*
>> @@ -246,9 +249,10 @@ static int cdns_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn)
>> {
>> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> struct cdns_pcie *pcie = &ep->pcie;
>> - u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
>> u16 flags, mme;
>> + u8 cap;
>>
>> + cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
>> fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>>
>> /* Validate that the MSI feature is actually enabled. */
>> @@ -269,9 +273,10 @@ static int cdns_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
>> {
>> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> struct cdns_pcie *pcie = &ep->pcie;
>> - u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
>> u32 val, reg;
>> + u8 cap;
>>
>> + cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
>> func_no = cdns_pcie_get_fn_from_vfn(pcie, func_no, vfunc_no);
>>
>> reg = cap + PCI_MSIX_FLAGS;
>> @@ -290,9 +295,10 @@ static int cdns_pcie_ep_set_msix(struct pci_epc *epc, u8 fn, u8 vfn,
>> {
>> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> struct cdns_pcie *pcie = &ep->pcie;
>> - u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
>> u32 val, reg;
>> + u8 cap;
>>
>> + cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
>> fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>>
>> reg = cap + PCI_MSIX_FLAGS;
>> @@ -378,11 +384,11 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
>> u8 interrupt_num)
>> {
>> struct cdns_pcie *pcie = &ep->pcie;
>> - u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
>> u16 flags, mme, data, data_mask;
>> - u8 msi_count;
>> u64 pci_addr, pci_addr_mask = 0xff;
>> + u8 msi_count, cap;
>>
>> + cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
>> fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>>
>> /* Check whether the MSI feature has been enabled by the PCI host. */
>> @@ -430,14 +436,14 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
>> u32 *msi_addr_offset)
>> {
>> struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>> - u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
>> struct cdns_pcie *pcie = &ep->pcie;
>> u64 pci_addr, pci_addr_mask = 0xff;
>> u16 flags, mme, data, data_mask;
>> - u8 msi_count;
>> + u8 msi_count, cap;
>> int ret;
>> int i;
>>
>> + cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
>> fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>>
>> /* Check whether the MSI feature has been enabled by the PCI host. */
>> @@ -480,16 +486,16 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
>> static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
>> u16 interrupt_num)
>> {
>> - u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
>> u32 tbl_offset, msg_data, reg;
>> struct cdns_pcie *pcie = &ep->pcie;
>> struct pci_epf_msix_tbl *msix_tbl;
>> struct cdns_pcie_epf *epf;
>> u64 pci_addr_mask = 0xff;
>> u64 msg_addr;
>> + u8 bir, cap;
>> u16 flags;
>> - u8 bir;
>>
>> + cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
>> epf = &ep->epf[fn];
>> if (vfn > 0)
>> epf = &epf->epf[vfn - 1];
>> @@ -563,7 +569,9 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>> int max_epfs = sizeof(epc->function_num_map) * 8;
>> int ret, epf, last_fn;
>> u32 reg, value;
>> + u8 cap;
>>
>> + cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP);
>> /*
>> * BIT(0) is hardwired to 1, hence function 0 is always enabled
>> * and can't be disabled anyway.
>> @@ -587,12 +595,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>> continue;
>>
>> value = cdns_pcie_ep_fn_readl(pcie, epf,
>> - CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
>> - PCI_EXP_DEVCAP);
>> + cap + PCI_EXP_DEVCAP);
>> value &= ~PCI_EXP_DEVCAP_FLR;
>> - cdns_pcie_ep_fn_writel(pcie, epf,
>> - CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
>> - PCI_EXP_DEVCAP, value);
>> + cdns_pcie_ep_fn_writel(pcie, epf, cap + PCI_EXP_DEVCAP,
>> + value);
>> }
>> }
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
>> index 0a4a8bfd3174..e7c108f6e0b2 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence.h
>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
>> @@ -125,11 +125,6 @@
>> */
>> #define CDNS_PCIE_EP_FUNC_BASE(fn) (((fn) << 12) & GENMASK(19, 12))
>>
>> -#define CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET 0x90
>> -#define CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET 0xb0
>> -#define CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET 0xc0
>> -#define CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET 0x200
>> -
>> /*
>> * Endpoint PF Registers
>> */
>>
>
> Nice to see these go away.
>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Thank you for your review.
Best regards,
Hans
>
Powered by blists - more mailing lists