Re: [PATCH v6 1/7] PCI: Expose reset_type to users of __pci_reset_function_locked()

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Nov 08, 2018 at 01:31:47PM -0700, Alex Williamson wrote:
> On Sat, 20 Oct 2018 10:03:53 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> > I don't think the current API is good enough :)  It's just a small
> > matter of sorting out a better one.
> 
> The thing that I was really hoping to fix was what happened in
> 811c5cb37df4 where we used to have separate bus vs slot reset
> interfaces that vfio-pci made use of, but these were abstracted away
> such that even though vfio-pci is specifically trying to achieve one or
> the other, not just whichever the pci-core chooses to use, we no longer
> have an interface to do that.  It doesn't make sense that we have
> pci_probe_reset_slot() and pci_probe_reset_bus() exported for drivers
> yet only pci_reset_bus() to execute the reset, which actually does a
> slot reset, not a bus reset, if it's available (and hopefully it picks
> the one that aligns with what the caller was intending).
> 
> I agree that we should have the "I don't really know what I'm doing,
> please make the correct choice obvious" interface, but at the same time
> I don't see the value in penalizing drivers that do know what they're
> doing and want to make a specific choice.  In that sense a desire to
> give less power to the user (ie. GPL kernel drivers) seems misplaced, I
> think we need to aim for a balance where the interface is hard to
> misuse, but at the same time retains useful functionality.  It can't
> just be one or the other.
> 
> I was also in a discussion today that seems like it could leverage
> aspects of this series where there's a desire to allow system admin
> level influence over the type of reset used for a device.  We can do
> this in the kernel via device specific resets, but it's not always the
> case that we can develop a universal recipe for blacklisting a reset
> mechanism for a given device.  Consider for instance a device where FLR
> works well only with some firmware revisions, but different system
> vendors might report similar firmware revisions, some afflicted, others
> not.
> 
> The bar is relatively high to not introduce device regressions and
> incorrectly requiring a device to use SBR rather than FLR can have
> functionality repercussions.  I know Bjorn has a desire to have things
> work well by default and command line options and sysfs tweaking of
> devices are obstacles to that, but there are cases where I don't see
> that we have a clear path to impose a device specific quirk without
> fear of regressions, and some mechanism of letting userspace policy
> influence the operation seems a viable path.  IMO it would also be a
> useful debugging tool if I could select a specific method of reset for a
> device from userspace and perhaps the infrastructure for this might
> make it easier to test and implement device specific quirks that simply
> need to pick a reset mechanism different from the one the kernel
> chooses by default.
> 
> The interface in this proposal maybe needs some work, but at some level
> it seems we need to consider which resets the device/bus/slot is capable
> of, resets blacklisted/selected/prioritized by the user/driver, and
> allow the caller to identify the type of reset they want to achieve.
> Can we agree that's a desirable goal?  Thanks,

I can't sign up completely to that whole list because it basically
mandates much of the complexity I want to avoid.

I do certainly agree we need to figure out which resets are supported
by a device.  Along that line, I want to get rid of all the "probe"
arguments.  In almost all cases we can figure out what's supported at
enumeration-time, and I think we should cache that information in a
series of bits in the pci_dev.  We already do much of that in
pci_probe_reset_function(), but we only save overall "can this device
be reset by any method", not the results for each individual method.

Of course, the parent bus reset is slightly tricky because we can't
decide for each device individually; it depends on whether there are
peer devices.  But I think that's still doable.

If we cache each bit, then it's a small step to have quirks or some
sort of sysfs hook to disable specific resets for firmware issues or
sysadmin control.

I don't want to penalize drivers that need to do something specific.
I just don't believe there are very many of those cases.  I think we
should list them explicitly, combine the ones that are needlessly
different, and build something that's sufficient for those cases, but
not overly generalized.

Bjorn