Re: [PATCH v5 08/11] PCI: Unify pci_reset_function_locked() and __pci_reset_function_locked()

[Sinan Kaya <okaya@kernel.org>]

On 10/11/2018 12:02 PM, Alex Williamson wrote:
>> The difference between pci_reset_function_locked() and
>> __pci_reset_function_locked() is the saving and restoring of the registers.
>> Unify these API by adding saverestore argument that caller passes.
>>
>> Signed-off-by: Sinan Kaya<okaya@kernel.org>
>> ---
>>   drivers/infiniband/hw/hfi1/pcie.c               |  2 +-
>>   drivers/net/ethernet/cavium/liquidio/lio_main.c |  2 +-
>>   drivers/pci/pci.c                               | 10 +++++++---
>>   drivers/pci/pci.h                               |  1 +
>>   drivers/xen/xen-pciback/pci_stub.c              |  6 +++---
>>   include/linux/pci.h                             |  4 ++--
>>   6 files changed, 15 insertions(+), 10 deletions(-)
> TBH, I'm not a fan of this patch or the remainder in this series.
> Having a function prefixed with underscored or specifically indicate
> locked tells callers that these are special cases and should be
> understood before using.  Adding bool parameters to the common
> functions ensures that every caller now needs to understand those
> special cases and potentially get them wrong.  Also, a string of random
> bool options to a function means that any time we want to use it we
> need to go reexamine the function definition.  It's not intuitive to
> use or review.  Thanks,

Thanks for the feedback. I wanted to get it out to gather feedback.

I think it is quite straightforward for people familiar with the existing
API to know what they are dealing with. I didn't know the difference
between

__pci_reset_function_locked() and pci_reset_function_locked()

for a long time. This was an attempt to minimize reset function flavors
and ask explicitly what user wants rather than having them use the wrong
function and suffer the consequence with random failures.