Re: [RFC] PCI / ACPI: Implementing Type 3 _HPX records

[Alex_Gagniuc]

Hi Bjorn

On 1/14/19 2:01 PM, Bjorn Helgaas wrote:
> On Thu, Jan 10, 2019 at 05:11:27PM -0600, Alexandru Gagniuc wrote:
>> _HPX Type 3 is intended to be more generic and allow configuration of
>> settings not possible with Type 2 tables. For example, FW could ensure
>> that the completion timeout value is set accordingly throughout the PCI
>> tree; some switches require very special handling.
>>
>> Type 3 can come in an arbitrary number of ACPI packages. With the older
>> types we only ever had one descriptor. We could get lazy and store it
>> on the stack. The current flow is to parse the HPX table
>> --pci_get_hp_params()-- and then program the PCIe device registers.
>>
>> In the current flow, we'll need to malloc(). Here's what I tried:
>>   1) devm_kmalloc() on the pci_dev
>>    This ended up giving me NULL dereference at boot time.
> 
> Yeah, I don't think devm_*() is going to work because that's part of the
> driver binding model; this is in the PCI core and this use is not related
> to the lifetime of a driver's binding to a device.
> 
>>   2) Add a cleanup function to be called after writing the PCIe registers
>>
>> This RFC implements method 2. The HPX3 stuff is still NDA'd, but the
>> housekeeping parts are in here. Is this a good way to do things? I'm not
>> too sure about the need to call cleanup() on a stack variable. But if
>> not that, then what else?
> 
> pci_get_hp_params() is currently exported, but only used inside
> drivers/pci, so I think it could be unexported.

It can.

> I don't remember if there's a reason why things are split between
> pci_get_hp_params(), which runs _HPX or _HPP, and the program_hpp_type*()
> functions.  If we could get rid of that split, we could get rid of struct
> hotplug_params and do the configuration directly in the pci_get_hp_params()
> path.  I think that would simplify the memory management.

Thanks for the in-depth analysis. I'm working on a proof-of-concept 
patch to do what you suggested. On downside though, is that we may have 
to parse some things twice.

Another thing, I've been thinking is, why not store the hotplug 
parameters from ACPI with the pci_bus struct. That way, you only need to 
parse ACPI once per bus, instead of each time a device is probed. There 
would be some memory overhead, but the hotplug structures are 
(thankfully) still relatively small. I could try a proof of concept for 
this idea as well... let me know.

Alex