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

[Sinan Kaya <okaya@kernel.org>]

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
>