RFC on PCI Device Lock

RFC on PCI Device Lock

[Sinan Kaya]

Hi Bjorn, Alex;

Since my recent series to relocate secondary bus reset code into the PCI core,
we hit a deadlock while trying to obtain a device lock during probe since
device lock is already held.

https://bugzilla.kernel.org/show_bug.cgi?id=200985

I posted a patch into the bugzilla so that we skip locks if we are probing
to follow the same strategy found in other lock routines in pci.c.

https://bugzilla.kernel.org/attachment.cgi?id=278221

I wanted to hear some opinions since this is a regression and need to find
a solution and also waiting for test feedback

Sinan

Re: RFC on PCI Device Lock

[Alex Williamson]

On Fri, 31 Aug 2018 16:03:38 -0700
Sinan Kaya <okaya@kernel.org> wrote:

> Hi Bjorn, Alex;
> 
> Since my recent series to relocate secondary bus reset code into the PCI core,
> we hit a deadlock while trying to obtain a device lock during probe since
> device lock is already held.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=200985
> 
> I posted a patch into the bugzilla so that we skip locks if we are probing
> to follow the same strategy found in other lock routines in pci.c.
> 
> https://bugzilla.kernel.org/attachment.cgi?id=278221
> 
> I wanted to hear some opinions since this is a regression and need to find
> a solution and also waiting for test feedback

TBH, I'm having a hard time seeing the value in the original series.
Commit 811c5cb37df4 is fiddling with two drivers, hfi1 and vfio-pci.
In the case of hfi1 the comment before the call is specifically talking
about performing a secondary bus reset, so all they're looking for is
bouncing the link, it seems they don't even necessarily want a slot
reset, which might entail a full power cycle. In the case of vfio-pci,
we're looking for whatever gets the device closest to a power on state,
so by all means we want the slot reset if we can get it.  So really
there's only one driver here looking for a slot reset and this commit
doesn't do anything to abstract bus vs slot reset because vfio-pci
needs to collect all the devices affected by the reset, which can be
different depending on whether it's a slot or bus (think conventional
PCI).  So all the logic in vfio-pci to figure out which it is still
exists, we just no longer have control over which option PCI-core uses.

Commit 409888e0966e switches hfi1 to use pci_try_reset_bus(), which of
course does the device save and restore, but the hfi1 driver still has
their own logic for all of that and of course it doesn't rely on device
lock and doesn't need to because they're within their own probe path.

I won't deny that we're getting a little sloppy with some of the reset
interfaces and I'd love to solve the device lock issue more generally,
but trying to abstract slot vs bus and hide the lower level interfaces
from drivers doesn't seem like it's really working here.  Thanks,

Alex

Re: RFC on PCI Device Lock

[Sinan Kaya]

On 9/4/2018 3:52 PM, Alex Williamson wrote:
> I won't deny that we're getting a little sloppy with some of the reset
> interfaces and I'd love to solve the device lock issue more generally,
> but trying to abstract slot vs bus and hide the lower level interfaces
> from drivers doesn't seem like it's really working here.  Thanks,

Counter argument is that drivers need to know about bus/slot distinction
and call two different API for hotplug capable systems. I think that's
too much to ask.

The motivation for original patch was that hotplug information was leaking
and we could do a better job about it. But, you are right; I partially
covered VFIO needs as it seemed to be doing more work than a simple
reset due to ownership dependencies.

Re: RFC on PCI Device Lock

[Alex Williamson]

On Tue, 4 Sep 2018 18:12:16 -0700
Sinan Kaya <okaya@kernel.org> wrote:

> On 9/4/2018 3:52 PM, Alex Williamson wrote:
> > I won't deny that we're getting a little sloppy with some of the reset
> > interfaces and I'd love to solve the device lock issue more generally,
> > but trying to abstract slot vs bus and hide the lower level interfaces
> > from drivers doesn't seem like it's really working here.  Thanks,  
> 
> Counter argument is that drivers need to know about bus/slot distinction
> and call two different API for hotplug capable systems. I think that's
> too much to ask.
> 
> The motivation for original patch was that hotplug information was leaking
> and we could do a better job about it. But, you are right; I partially
> covered VFIO needs as it seemed to be doing more work than a simple
> reset due to ownership dependencies.

Maybe let's clarify what is the actual leak of the hotplug API to the
driver.  I think vfio assumes full responsibility here, it needs to
know the scope of the reset in order to determine whether the user has
ownership of all the devices affected by the reset.  In that case it's
even preferable that we have control of specifying which type of reset
is performed or else we open ourselves for the possibility that vfio-pci
and PCI-core could diverge and we don't get the reset we expect.  I
think that's a point against this sort of blind, unified reset
interface.

The hfi1 driver previously didn't care about hotplug slots, but the
case I see where they should have is if the slot enabled surprise
hotplug.  In that case their direct call to
pci_reset_bridge_secondary_bus might have triggered such an event;
surprise!  They also go to the trouble to manually save and restore
config space around the reset and make sure they're the only device
affected by the reset, so this really starts to sound like a place
where we should use a pci_reset_function type interface (though they
have quite a bit of setup working towards the SBR, so it may not be
trivial to convert).  In fact, we've even got and exported
__pci_reset_function_locked() which can already handle the device_lock
being held, but it will prefer function level resets before it gets to
the bus/slot resets that this driver wants.

It seems a common theme here is that the driver wants some degree of
control of the type of reset used, vfio specifically wants to specify a
bus or slot reset while hfi1 just wants to specify that the link is
reset and doesn't care if it's a bus or slot reset that accomplishes
that.  So maybe the right "unified" interface is one that takes a
parameter allowing the caller to specify the scope or type of reset
with aliases so drivers can ignore the hotplug interface if they wish.
An ugly version of that might be based around something like:

#define PCI_RESET_DEV_SPECIFIC	(1 << 0)
#define PCI_RESET_FLR		(1 << 1)
#define PCI_RESET_PM		(1 << 2)
#define PCI_RESET_SLOT		(1 << 3)
#define PCI_RESET_BUS		(1 << 4)

#define PCI_RESET_ANY		(~0)
#define PCI_RESET_FUNC		(PCI_RESET_DEV_SPECIFIC | \
				 PCI_RESET_FLR | PCI_RESET_PM)
#define PCI_RESET_LINK		(PCI_RESET_SLOT | PCI_RESET_BUS)

Thanks,
Alex

Re: RFC on PCI Device Lock

[Dennis Dalessandro]

On 9/4/2018 6:52 PM, Alex Williamson wrote:
> On Fri, 31 Aug 2018 16:03:38 -0700
> Sinan Kaya <okaya@kernel.org> wrote:
> 
>> Hi Bjorn, Alex;
>>
>> Since my recent series to relocate secondary bus reset code into the PCI core,
>> we hit a deadlock while trying to obtain a device lock during probe since
>> device lock is already held.
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=200985
>>
>> I posted a patch into the bugzilla so that we skip locks if we are probing
>> to follow the same strategy found in other lock routines in pci.c.
>>
>> https://bugzilla.kernel.org/attachment.cgi?id=278221
>>
>> I wanted to hear some opinions since this is a regression and need to find
>> a solution and also waiting for test feedback

Responded on the bugzilla, the patch seems to pass basic tests. I will 
queue up some more but so far so good.

> TBH, I'm having a hard time seeing the value in the original series.
> Commit 811c5cb37df4 is fiddling with two drivers, hfi1 and vfio-pci.
> In the case of hfi1 the comment before the call is specifically talking
> about performing a secondary bus reset, so all they're looking for is
> bouncing the link, it seems they don't even necessarily want a slot
> reset, which might entail a full power cycle.

Correct. Keep it simple as possible is usually the best approach.

> I won't deny that we're getting a little sloppy with some of the reset
> interfaces and I'd love to solve the device lock issue more generally,
> but trying to abstract slot vs bus and hide the lower level interfaces
> from drivers doesn't seem like it's really working here. 

The issue I have with all of this is we used to have a function that did 
a very simple thing. Do an SBR, get out. Changing the API, I'm fine 
with, but these changes did more than just change the API. They changed 
the underlying behavior and what is happening now is different.

It sounds like all that is being done is put a band-aid on things and 
there is some redesign work needed. Perhaps the best thing to do is back 
this out for 4.19 and revisit with a more general solution for 4.20. 
That's a call for you folks to make. My concern is getting our HW 
functioning again with 4.19 and Sinan's patch along with my 1/1 seems to 
allow that at least.

-Denny

Re: RFC on PCI Device Lock

[Sinan Kaya]

On 9/4/2018 7:46 PM, Alex Williamson wrote:
> #define PCI_RESET_DEV_SPECIFIC	(1 << 0)
> #define PCI_RESET_FLR		(1 << 1)
> #define PCI_RESET_PM		(1 << 2)
> #define PCI_RESET_SLOT		(1 << 3)
> #define PCI_RESET_BUS		(1 << 4)
> 
> #define PCI_RESET_ANY		(~0)
> #define PCI_RESET_FUNC		(PCI_RESET_DEV_SPECIFIC | \
> 				 PCI_RESET_FLR | PCI_RESET_PM)
> #define PCI_RESET_LINK		(PCI_RESET_SLOT | PCI_RESET_BUS)

I should be able to put something together using __pci_reset_function_locked()
shortly.

This is not that difficult to implement.

Re: RFC on PCI Device Lock

[Sinan Kaya]

On 9/4/2018 7:59 PM, Dennis Dalessandro wrote:
> It sounds like all that is being done is put a band-aid on things and there is 
> some redesign work needed. Perhaps the best thing to do is back this out for 
> 4.19 and revisit with a more general solution for 4.20. That's a call for you 
> folks to make. My concern is getting our HW functioning again with 4.19 and 
> Sinan's patch along with my 1/1 seems to allow that at least.

Alex proposed another solution. That looks better IMO.
I think we can close this issue shortly. We have not exhausted bug fixing window
unless Bjorn thinks otherwise.

I'll upload another patch to bugzilla soon.

Re: RFC on PCI Device Lock

[Sinan Kaya]

On 9/4/2018 8:09 PM, Sinan Kaya wrote:
> On 9/4/2018 7:46 PM, Alex Williamson wrote:
>> #define PCI_RESET_DEV_SPECIFIC    (1 << 0)
>> #define PCI_RESET_FLR        (1 << 1)
>> #define PCI_RESET_PM        (1 << 2)
>> #define PCI_RESET_SLOT        (1 << 3)
>> #define PCI_RESET_BUS        (1 << 4)
>>
>> #define PCI_RESET_ANY        (~0)
>> #define PCI_RESET_FUNC        (PCI_RESET_DEV_SPECIFIC | \
>>                  PCI_RESET_FLR | PCI_RESET_PM)
>> #define PCI_RESET_LINK        (PCI_RESET_SLOT | PCI_RESET_BUS)
> 
> I should be able to put something together using __pci_reset_function_locked()
> shortly.
> 
> This is not that difficult to implement.
> 

I posted these

https://bugzilla.kernel.org/attachment.cgi?id=278297
https://bugzilla.kernel.org/attachment.cgi?id=278299

Let me know if I misunderstood you. I'll again wait for test feedback first.

Re: RFC on PCI Device Lock

[Alex Williamson]

On Tue, 4 Sep 2018 20:41:20 -0700
Sinan Kaya <okaya@kernel.org> wrote:

> On 9/4/2018 8:09 PM, Sinan Kaya wrote:
> > On 9/4/2018 7:46 PM, Alex Williamson wrote:  
> >> #define PCI_RESET_DEV_SPECIFIC    (1 << 0)
> >> #define PCI_RESET_FLR        (1 << 1)
> >> #define PCI_RESET_PM        (1 << 2)
> >> #define PCI_RESET_SLOT        (1 << 3)
> >> #define PCI_RESET_BUS        (1 << 4)
> >>
> >> #define PCI_RESET_ANY        (~0)
> >> #define PCI_RESET_FUNC        (PCI_RESET_DEV_SPECIFIC | \
> >>                  PCI_RESET_FLR | PCI_RESET_PM)
> >> #define PCI_RESET_LINK        (PCI_RESET_SLOT | PCI_RESET_BUS)  
> > 
> > I should be able to put something together using __pci_reset_function_locked()
> > shortly.
> > 
> > This is not that difficult to implement.
> >   
> 
> I posted these
> 
> https://bugzilla.kernel.org/attachment.cgi?id=278297
> https://bugzilla.kernel.org/attachment.cgi?id=278299
> 
> Let me know if I misunderstood you. I'll again wait for test feedback first.

Yep, that's essentially it.  I assume we'd want to trickle this through
other interfaces and the hfi1 folks can look at whether they want to
drop their custom config save/restore in favor of using
pci_reset_function_locked(), which would do that for them.
pci_reset_bus() should also allow the caller to set a mask so vfio
doesn't worry about defaults changing in the core.  Thanks,

Alex

Re: RFC on PCI Device Lock

[Sinan Kaya]

On 9/4/2018 9:13 PM, Alex Williamson wrote:
>> Let me know if I misunderstood you. I'll again wait for test feedback first.
> Yep, that's essentially it.  I assume we'd want to trickle this through
> other interfaces and the hfi1 folks can look at whether they want to
> drop their custom config save/restore in favor of using
> pci_reset_function_locked(), which would do that for them.
> pci_reset_bus() should also allow the caller to set a mask so vfio
> doesn't worry about defaults changing in the core.  Thanks,
The testing was successful.

"--- Comment #15 from Dennis (dennis.dalessandro@intel.com) ---
I have tested the two new patches applied on top of 4.19-rc1 and they seem to
be working as well."

Before I post the code on the list, I want to make sure I capture your
request correctly.

You are asking for more APIs to have the mask feature. I can add
slot and bus reset options to pci_reset_bus().

If there are other functions that need mask feature, we can deal
with them later as I want to focus on the regression short term.

Though, it would be good to enumerate the functions with needed masks
to start the conversation.

Re: RFC on PCI Device Lock

[Alex Williamson]

On Wed, 5 Sep 2018 07:26:52 -0700
Sinan Kaya <okaya@kernel.org> wrote:

> On 9/4/2018 9:13 PM, Alex Williamson wrote:
> >> Let me know if I misunderstood you. I'll again wait for test feedback first.  
> > Yep, that's essentially it.  I assume we'd want to trickle this through
> > other interfaces and the hfi1 folks can look at whether they want to
> > drop their custom config save/restore in favor of using
> > pci_reset_function_locked(), which would do that for them.
> > pci_reset_bus() should also allow the caller to set a mask so vfio
> > doesn't worry about defaults changing in the core.  Thanks,  
> The testing was successful.
> 
> "--- Comment #15 from Dennis (dennis.dalessandro@intel.com) ---
> I have tested the two new patches applied on top of 4.19-rc1 and they seem to
> be working as well."
> 
> Before I post the code on the list, I want to make sure I capture your
> request correctly.
> 
> You are asking for more APIs to have the mask feature. I can add
> slot and bus reset options to pci_reset_bus().
> 
> If there are other functions that need mask feature, we can deal
> with them later as I want to focus on the regression short term.
> 
> Though, it would be good to enumerate the functions with needed masks
> to start the conversation.

Well, if __pci_reset_function_locked() has this option, would only make
sense that these would as well:

pci_probe_reset_function()
pci_reset_function()
pci_reset_function_locked()
pci_try_reset_function()

Then I'd like to see the same sort of caller specification added to
pci_reset_bus() so that vfio can specify whether it's expecting a sbr
or slot reset.  Thanks,

Alex

Re: RFC on PCI Device Lock

[Sinan Kaya]

On 9/5/2018 10:37 AM, Alex Williamson wrote:
> On Wed, 5 Sep 2018 07:26:52 -0700
> Sinan Kaya <okaya@kernel.org> wrote:
> 
>> On 9/4/2018 9:13 PM, Alex Williamson wrote:
>>>> Let me know if I misunderstood you. I'll again wait for test feedback first.
>>> Yep, that's essentially it.  I assume we'd want to trickle this through
>>> other interfaces and the hfi1 folks can look at whether they want to
>>> drop their custom config save/restore in favor of using
>>> pci_reset_function_locked(), which would do that for them.
>>> pci_reset_bus() should also allow the caller to set a mask so vfio
>>> doesn't worry about defaults changing in the core.  Thanks,
>> The testing was successful.
>>
>> "--- Comment #15 from Dennis (dennis.dalessandro@intel.com) ---
>> I have tested the two new patches applied on top of 4.19-rc1 and they seem to
>> be working as well."
>>
>> Before I post the code on the list, I want to make sure I capture your
>> request correctly.
>>
>> You are asking for more APIs to have the mask feature. I can add
>> slot and bus reset options to pci_reset_bus().
>>
>> If there are other functions that need mask feature, we can deal
>> with them later as I want to focus on the regression short term.
>>
>> Though, it would be good to enumerate the functions with needed masks
>> to start the conversation.
> 
> Well, if __pci_reset_function_locked() has this option, would only make
> sense that these would as well:
> 
> pci_probe_reset_function()
> pci_reset_function()
> pci_reset_function_locked()
> pci_try_reset_function()
> 
> Then I'd like to see the same sort of caller specification added to
> pci_reset_bus() so that vfio can specify whether it's expecting a sbr
> or slot reset.  Thanks,

OK. I can work on these. We can review and hopefully reach to a consensus.

I posted the bare minimum needed to bugfix this specific issue for the moment.

> 
> Alex
>