On 10/19/2018 10:09 PM, Bjorn Helgaas wrote:
This series (and v5, it looks like) lost the cover letter, which had a
nice diffstat overview :)
Yeah, I was too lazy. I was rushing to get it out of my laptop before I go
back to my daily work.
On Fri, Oct 19, 2018 at 02:11:21AM +0000, Sinan Kaya wrote:
We need a contract between the reset API users and the PCI core about the
types of reset that a user needs vs. what PCI core can do internally.
If a platform supports hotplug, we need to do hotplug reset as an example.
Somewhere this needs a description of a bug that's being fixed or some
other justification, e.g., code simplification. The above is a little
too abstract for me to grasp it.
This series was in response to a request from Alex to have external users
some level of control on what PCI core can do internally.
On the other hand, I have been posting patches to remove direct access from
external users to PCI core internals and hide all reset semantics like
save/restore from the users. I like giving less power to the users :)
Expose the reset types to the drivers and try different reset types based
on the new reset_type parameter.
Most users are expected to use PCI_RESET_ANY, PCI_RESET_FUNC or
PCI_RESET_LINK parameters.
There are fewer than a dozen callers of all these functions and the
complication of these interfaces doesn't seem commensurate with the
problem. With six different interfaces and five independent bit
flags, the possibilities are way more than necessary.
True, I posted an RFC to reduce 5 function reset API flavors to 1 and
rely on the flags.
In the end, there would be two reset APIs only.
It seems like there are only three main cases (possibly extended by
locked/unlocked versions):
- a generic reset, used by ipc, genwqe, qlcnic, sfc, liquidio,
mwifiex, xen
- something special for VFIO that is affected by the set of devices
owned by the user
- a link reset for places like these where a device needs help to
train the link correctly:
cik_pcie_gen3_enable() # amdgpu, radeon
si_pcie_gen3_enable() # amdgpu, radeon
do_pcie_gen3_transition() # infiniband/hfi1
The interface for the generic case should be simple and made to be the
obvious choice for drivers, i.e., requires only the pci_dev and no
magic flags.
Even these generic reset examples are not really generic today. Some of them
call reset from probe path and use the locked reset API
(pci_reset_function_locked()).
Others call the reset but prefer to not save/restore context.
(__pci_reset_function_locked()). Some drivers even implemented their own
context save/restore code themselves.
There are others that do obtain the lock as well as save/restore context.
(pci_reset_function())
I really don't like having these many reset API flavors and I thought
we could do something about that.
We can also drop the series if we think that current API are good enough
and nuances are well understood.
Alex wanted to have some more control into the reset APIs. Thus, this series
+ the API reduction in RFC from me (again Alex didn't like this much).
The link reset case is different enough that it might
deserve its own special interface.
Link: https://www.spinics.net/lists/linux-pci/msg75828.html
Can you switch to using https://lore.kernel.org/linux-pci/<Message-ID>
URLs so we don't depend on 3rd-party archives like spinics? See
https://www.kernel.org/lore.html . I usually silently convert to the
lore URLs, but I guess doing it silently only makes more work for
myself.
Bjorn
