On Mon, Oct 02, 2017 at 05:14:09PM -0700, Govindarajulu Varadarajan wrote:
>
> I would avoid increasing the size of pci_dev. The list_head would be empty after
> we call aer_pci_walk_bus(). We already have pci_bus *subordinate to link all the
> 'device's. Making list_head available to others would require a lock. I would
> avoid that.
It does not require a new lock if you clearly document the concurrency
semantics. And I'd much rather add a hlist_node (which should be enough
if we want to be small) to pci_dev than requiring these additional
memory allocations and boiler plate code in an error recovery situation.
>
>>> +static void aer_pci_walk_bus(struct pci_bus *top,
>>> + int (*cb)(struct pci_dev *, void *),
>>> + struct aer_broadcast_data *result)
>>> +{
>>> + HLIST_HEAD(dev_hlist);
>>> + struct hlist_node *tmp;
>>> + struct aer_device_list *entry;
>>
>> Do we want to offer this as generic PCIe functionality? If not we can
>> probably hardcode the callback and callback data to simplify this a lot.
>>
>
> I could not find any other code which aquires device_lock in pci_walk_bus cb
> function. Can you tell me how we can hardcore callback and callback data here?
The word is hardcode. If you follow the above suggestion we won't
really need the aer_pci_walk_bus helper but could just open code most
of the code. Or maybe just keep passing the cb for simplicity - it
was just extending on the above idea.
