Message-ID: <65049dff-cb22-44fd-8663-e4521d8b23fb@163.com>
Date: Sun, 23 Mar 2025 23:36:33 +0800
From: Hans Zhang <18255117159@....com>
To: Lukas Wunner <lukas@...ner.de>
Cc: lpieralisi@...nel.org, kw@...ux.com, manivannan.sadhasivam@...aro.org,
 robh@...nel.org, bhelgaas@...gle.com, jingoohan1@...il.com,
 thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [v5 1/4] PCI: Introduce generic capability search functions



On 2025/3/23 00:11, Lukas Wunner wrote:
> On Sat, Mar 22, 2025 at 11:47:18PM +0800, Hans Zhang wrote:
>> On 2025/3/22 01:06, Lukas Wunner wrote:
>>> On Sat, Mar 22, 2025 at 12:38:00AM +0800, Hans Zhang wrote:
>>>> Add pci_host_bridge_find_*capability() in drivers/pci/pci.c, accepting
>>>> controller-specific read functions and device data as parameters.
>>>
>>> Please put this in a .c file which is only compiled and linked if
>>> one of the controller drivers using those new helpers is enabled
>>> in .config.
>>>
>>> If you put the helpers in drivers/pci/pci.c, they unnecessarily
>>> enlarge the kernel's .text section even if it's known already
>>> at compile time that they're never going to be used (e.g. on x86).
>>>
>>> You could put them in drivers/pci/controller/pci-host-common.c
>>> and then select PCI_HOST_COMMON for each driver using them.
>>> Or put them in a separate completely new file.
>>
>> I add a drivers/pci/controller/pci-host-helpers.c file, how do you like it?
>> Below, I have rearranged the patch, please kindly review it, thank you very
>> much.
> 
> Looks fine to me, but I'm not one of the maintainers for the controller
> drivers, they may have a different opinion.  I'd recommending submitting
> this and see if any of them doesn't like it.
> 

Hi Lukas,

Thanks your for reply. I will submit it in the next version.

> Just one nit that caught my eye:
> 
>> --- a/drivers/pci/controller/Kconfig
>> +++ b/drivers/pci/controller/Kconfig
>> @@ -132,6 +132,22 @@ config PCI_HOST_GENERIC
>>   	  Say Y here if you want to support a simple generic PCI host
>>   	  controller, such as the one emulated by kvmtool.
>>
>> +config PCI_HOST_HELPERS
>> + 	bool "PCI Host Controller Helper Functions"
>> + 	help
>> +	  This provides common infrastructure for PCI host controller drivers to
>> +	  handle PCI capability scanning and other shared operations. The helper
>> +	  functions eliminate code duplication across controller drivers.
>> +
>> +	  These functions are used by PCI controller drivers that need to scan
>> +	  PCI capabilities using controller-specific access methods (e.g. when
>> +	  the controller is behind a non-standard configuration space).
>> +
>> +	  If you are using any PCI host controller drivers that require these
>> +	  helpers (such as DesignWare, Cadence, etc), this will be
>> +	  automatically selected. Say N unless you are developing a custom PCI
>> +	  host controller driver.
> 
> I like the comprehensive help text, but I'd recommend removing the
> input prompt "PCI Host Controller Helper Functions" after the "bool".
> 
> I think generally only menu entries should be displayed that are relevant
> for end users.  The prompt is only needed for developers and they can
> easily modify Kconfig to select the item when they add their driver.
> 
> If you absolutely positively want to have a prompt, I'd recommend
> hiding the menu entry unless CONFIG_EXPERT is also enabled, i.e.:
> 
> 	bool
> 	prompt "PCI Host Controller Helper Functions" if EXPERT
> 
> Or maybe there's something better than CONFIG_EXPERT for cases like this,
> dunno.
> 

Well, thank you for your advice.

Best regards,
Hans

Powered by blists - more mailing lists