| 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: <418498a1-e2c0-4453-a69d-dabe0ee0e5f6@163.com>
Date: Tue, 1 Apr 2025 21:20:34 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Bjorn Helgaas <bhelgaas@...gle.com>, lpieralisi@...nel.org, kw@...ux.com,
robh@...nel.org, jingoohan1@...il.com, thomas.richard@...tlin.com,
linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for
finding the capabilities
On 2025/4/1 00:39, Ilpo Järvinen wrote:
>>>> + read_cfg((priv), (devfn), __pos, 2, (u32 *)&__ent); \
>>>> + \
>>>> + __id = __ent & 0xff; \
>>>> + if (__id == 0xff) \
>>>> + break; \
>>>> + if (__id == (cap)) { \
>>>> + __found_pos = __pos; \
>>>> + break; \
>>>> + } \
>>>> + __pos = (__ent >> 8); \
>>>
>>> I'd add these into uapi/linux/pci_regs.h:
>>
>> This means that you will submit, and I will submit after you?
>> Or should I submit this series of patches together?
>
> I commented these cleanup opportunities so that you could add them to
> your series. If I'd immediately start working on area/lines you're working
> with, it would just trigger conflicts so it's better the original author
> does the improvements within the series he/she is working with. It's a lot
> less work for the maintainer that way :-).
>
Hi Ilpo,
Thanks your for reply. Thank you so much for your comments.
>>> I started to wonder though if the controller drivers could simply create
>>> an "early" struct pci_dev & pci_bus just so they can use the normal
>>> accessors while the real structs are not yet created. It looks not
>>> much is needed from those structs to let the accessors to work.
>>>
>>
>> Here are a few questions:
>> 1. We need to initialize some variables for pci_dev. For example,
>> dev->cfg_size needs to be initialized to 4K for PCIe.
>>
>> u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
>> {
>> ......
>> if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
>> return 0;
>> ......
>>
>
> Sure, it would require some initialization of the struct (but not
> full init like the probe path does that does lots of setup too).
>
>> 2. Create an "early" struct pci_dev & pci_bus for each SOC vendor (Qcom,
>> Rockchip, etc). It leads to a lot of code that feels weird.
>
> The early pci_dev+pci_bus would be created by a helper in PCI core that
> initializes what is necessary for the supported set of early core
> functionality to work. The controller drivers themselves would just call
> that function.
>
Ok, got it.
>> I still prefer the approach we are discussing now.
>
> I'm not saying we should immediately head toward this new idea within this
> series because it's going to be relatively big change. But it's certainly
> something that looks worth exploring so that the current chicken-egg
> problem with controller drivers could be solved.
>
Ok, I hope to have the opportunity to participate in the discussion
together in the future.
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 2e9cf26a9ee9..68c111be521d 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -4,6 +4,65 @@
>>
>> #include <linux/pci.h>
>
> Make sure to add the necessary headers for the function/macros you're
> using so that things won't depend on the #include order in the .c file.
>
Will do.
>>
>> +/* Ilpo: I'd add these into uapi/linux/pci_regs.h: */
>> +#define PCI_CAP_ID_MASK 0x00ff
>> +#define PCI_CAP_LIST_NEXT_MASK 0xff00
>> +
>> +/* Standard capability finder */
>
> Capability
>
> Always use the same capitalization as the specs do.
>
Will change.
> You should probably write a kernel doc for this macro too.
>
Will do.
> I'd put these macro around where pcie_cap_has_*() forward declarations
> are so that the initial define block is not split.
>
Will change.
>> +#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...) \
>> +({ \
>> + u8 __pos = (start); \
>> + int __ttl = PCI_FIND_CAP_TTL; \
>> + u16 __ent; \
>> + u8 __found_pos = 0; \
>> + u8 __id; \
>> + \
>> + read_cfg(args, __pos, 1, (u32 *)&__pos); \
>> + \
>> + while (__ttl--) { \
>> + if (__pos < PCI_STD_HEADER_SIZEOF) \
>> + break; \
>> + __pos = ALIGN_DOWN(__pos, 4); \
>> + read_cfg(args, __pos, 2, (u32 *)&__ent); \
>> + __id = FIELD_GET(PCI_CAP_ID_MASK, __ent); \
>> + if (__id == 0xff) \
>> + break; \
>> + if (__id == (cap)) { \
>> + __found_pos = __pos; \
>> + break; \
>> + } \
>> + __pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent); \
>> + } \
>> + __found_pos; \
>> +})
>> +
>> +/* Extended capability finder */
>> +#define PCI_FIND_NEXT_EXT_CAPABILITY(read_cfg, start, cap, args...) \
>> +({ \
>> + u16 __pos = (start) ?: PCI_CFG_SPACE_SIZE; \
>> + u16 __found_pos = 0; \
>> + int __ttl, __ret; \
>> + u32 __header; \
>> + \
>> + __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8; \
>> + while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) { \
>> + __ret = read_cfg(args, __pos, 4, &__header); \
>> + if (__ret != PCIBIOS_SUCCESSFUL) \
>> + break; \
>> + \
>> + if (__header == 0) \
>> + break; \
>> + \
>> + if (PCI_EXT_CAP_ID(__header) == (cap) && __pos != start) {\
>> + __found_pos = __pos; \
>> + break; \
>> + } \
>> + \
>> + __pos = PCI_EXT_CAP_NEXT(__header); \
>> + } \
>> + __found_pos; \
>> +})
>> +
>> struct pcie_tlp_log;
>>
>> /* Number of possible devfns: 0.0 to 1f.7 inclusive */
>>
>>
>> Looking forward to your latest suggestions.
>
> This generally looked good, I didn't read with a very fine comb but just
> focused on the important bits. I'll take a more detailed look once you
> make the official submission.
Ok, I'm going to prepare the next version of patch. I hope you can
review it again. Thank you very much
Best regards,
Hans
Powered by blists - more mailing lists