Re: git: 871b33ad65ba - main - pci: Consistently use pci_vf_* for suballocated VF memory resources

On 6/4/24 8:00 PM, Jessica Clarke wrote:
> On 5 Jun 2024, at 00:52, John Baldwin <jhb@FreeBSD.org> wrote:
>>
>> The branch main has been updated by jhb:
>>
>> URL: https://cgit.FreeBSD.org/src/commit/?id=871b33ad65baf07c92cce120a4fc1978c2ed7b3b
>>
>> commit 871b33ad65baf07c92cce120a4fc1978c2ed7b3b
>> Author:     John Baldwin <jhb@FreeBSD.org>
>> AuthorDate: 2024-06-04 23:51:37 +0000
>> Commit:     John Baldwin <jhb@FreeBSD.org>
>> CommitDate: 2024-06-04 23:51:37 +0000
>>
>>     pci: Consistently use pci_vf_* for suballocated VF memory resources
>>
>>     Some of the bus resource methods were passing these up to the parent
>>     which triggered rman mismatch assertions in INVARIANTS kernels.
>>
>>     Reported by:    kp
>>     Reviewed by:    imp
>>     Tested by:      kp (earlier version)
>>     Differential Revision:  https://reviews.freebsd.org/D45406
>> ---
>> sys/dev/pci/pci.c         | 118 ++++++++++++++++++++++++++++++++++--
>> sys/dev/pci/pci_iov.c     | 151 ++++++++++++++++++++++++++++++++++++++++++++++
>> sys/dev/pci/pci_private.h |  19 ++++++
>> 3 files changed, 284 insertions(+), 4 deletions(-)
>>
>> diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
>> index 2cb8b4ce9fcc..2093d6a8b5ef 100644
>> --- a/sys/dev/pci/pci.c
>> +++ b/sys/dev/pci/pci.c
>> @@ -164,10 +164,18 @@ static device_method_t pci_methods[] = {
>> DEVMETHOD(bus_get_resource, bus_generic_rl_get_resource),
>> DEVMETHOD(bus_delete_resource, pci_delete_resource),
>> DEVMETHOD(bus_alloc_resource, pci_alloc_resource),
>> +#ifdef PCI_IOV
>> + DEVMETHOD(bus_adjust_resource, pci_adjust_resource),
>> +#else
>> DEVMETHOD(bus_adjust_resource, bus_generic_adjust_resource),
>> +#endif
>> DEVMETHOD(bus_release_resource, pci_release_resource),
>> DEVMETHOD(bus_activate_resource, pci_activate_resource),
>> DEVMETHOD(bus_deactivate_resource, pci_deactivate_resource),
>> +#ifdef PCI_IOV
>> + DEVMETHOD(bus_map_resource, pci_map_resource),
>> + DEVMETHOD(bus_unmap_resource, pci_unmap_resource),
>> +#endif
>> DEVMETHOD(bus_child_deleted, pci_child_deleted),
>> DEVMETHOD(bus_child_detached, pci_child_detached),
>> DEVMETHOD(bus_child_pnpinfo, pci_child_pnpinfo_method),
> 
> Would it make sense to instead #ifdef parts of the body of
> pci_adjust_resource rather than switching which function you’re using
> in the first place? I feel that is generally easier to understand, as
> there’s less conditionality, and it’s more easily extensible if any
> other special cases come along.

Hmm, I could do that I guess.  These aren't hot paths so the
extra jump to a tail call in the #ifndef case isn't that bad, and it
probably is a bit more readable.

In related news, you should really land your patches to enable
NEW_PCIB and PCI_RES_BUS by default (and remove the !NEW_PCIB
code). :)

-- 
John Baldwin