On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote: <--- snip ---> > > > > > > The pci_bus_read_config() interface itself may have been a > > > > > > mistake, can't the callers just use the underlying helpers > > > > > > directly? > > > > > > > > > > > > > > > > They can! Since the callers of this API is mostly the macros, we can easily > > > > > implement the logic to call relevant accessors based on the requested size. > > > > > > > > > > Hans, could you please respin the series based the feedback since the series is > > > > > dropped for 6.17. > > > > > > > > > > > > > Dear all, > > > > > > > > I am once again deeply sorry for the problems that occurred in this series. > > > > I only test pulling the ARM platform. > > > > > > > > Thank you very much, Gerd, for reporting the problem. no worries! > > > > Thank you all for your discussions and suggestions for revision. > > > > > > > > Hi Mani, > > > > > > > > Geert provided a solution. My patch based on this is as follows. Please > > > > check if there are any problems. > > > > https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@xxxxxxxxxxxxxx/ > > > > > > > > Also, please ask Gerd to help test whether it works properly. Thank you very > > > > much. > > > > I found Geert's proposal intriguing for a quick resolution of the issue. Yet, I have not tried that proposal, though. Instead I spent some more cycles on Lukas' and Mani's question about the value of the pci_bus_read_config() helper. So I changed PCI_FIND_NEXT_CAP and PCI_FIND_NEXT_EXT_CAP to use size-aware versions of read_cfg accessor functions like this: diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index ac954584d991..9e2f75ede95f 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -109,17 +109,17 @@ int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size, ({ \ int __ttl = PCI_FIND_CAP_TTL; \ u8 __id, __found_pos = 0; \ - u32 __pos = (start); \ - u32 __ent; \ + u8 __pos = (start); \ + u16 __ent; \ \ - read_cfg(args, __pos, 1, &__pos); \ + read_cfg##_byte(args, __pos, &__pos); \ \ while (__ttl--) { \ if (__pos < PCI_STD_HEADER_SIZEOF) \ break; \ \ __pos = ALIGN_DOWN(__pos, 4); \ - read_cfg(args, __pos, 2, &__ent); \ + read_cfg##_word(args, __pos, &__ent); \ \ __id = FIELD_GET(PCI_CAP_ID_MASK, __ent); \ if (__id == 0xff) \ @@ -158,7 +158,7 @@ int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size, \ __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); \ + __ret = read_cfg##_dword(args, __pos, &__header); \ if (__ret != PCIBIOS_SUCCESSFUL) \ break; \ \ This fixes the issue for s390's use-cases. With that pci_bus_read_config() becomes unused - and could be removed in further refinements. However, this probably breaks your dwc and cdns use-cases. I think, with the accessor functions for dwc and cadence changed to follow the {_byte|_word|_dword} naming pattern they could be used straight out of PCI_FIND_NEXT_{EXT_}CAP, too. Then, dw_pcie_read_cfg() and cdns_pcie_read_cfg become obsolete as well. Thoughts?
