Re: [PATCHv2 4/4] pci: make reset_subordinate hotplug safe

[Keith Busch <kbusch@kernel.org>]

On Fri, Jan 30, 2026 at 10:42:22PM -0800, dan.j.williams@intel.com wrote:
> Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Use the slot reset method when resetting the bridge if the bus contains
> 
> What about some renames to make this easier to read, because it is
> ironic that this says "Use the slot reset method" when the functional
> change is to now start using the __pci_reset_slot() method. If I am
> reading this "slot reset vs reset slot" conversion correctly.

Yep, I thnk you've got the sense of the naming issues here.
 
> Something like:
> 
> s/__pci_reset_slot/pci_try_masked_slot_reset/
> 
> Since it is a conditional reset, and it is masking slot events.
> 
> ...but pulling that string also means:
> 
> s/__pci_reset_bus/pci_try_reset_bus/

Yeah, there are a lot of refactoring opportunities here. My experience
with the pci subsystem, though, is that it is a bit conservative (and
perhaps rightfully so) on accepting refactor churn. 

> > -	mutex_unlock(&pci_slot_mutex);
> > -	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
> > +	return __pci_reset_bridge(bridge, false);
> 
> Similar to how PCI_RESET_{DO_RESET,PROBE} makes the ambiguous
> "true/false" readable, how about adding something like:
> 
> #define PCI_SLOT_RESET_MASKED true
> #define PCI_SLOT_RESET_UNMASKED false

Sure, can do. Not sure I like the existing PROBE vs DO_RESET though. I
think the probe should just open code the checks in its own function
(which already exist) so that the DO_RESET doesn't duplicate literally
every check the preceding PROBE does. But that's a refactoring thing.

> > +	mutex_unlock(&pci_slot_mutex);
> > +
> > +	if (save)
> > +		return __pci_reset_bus(bus);
> > +	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
> 
> Logic looks equivalent in the PCI_SLOT_RESET_UNMASKED case, I think it
> wants a comment that it turns into a "try_reset" when
> PCI_SLOT_RESET_MASKED...
> 
> > +}
> > +
> > +int pci_reset_bridge(struct pci_dev *bridge)
> > +{
> > +	return __pci_reset_bridge(bridge, true);
> > +}
> 
> ...and maybe that comment is helped by naming this helper
> pci_try_reset_bridge()?

Sounds good.
 
> I would want a second opinion on the renames before saying they are
> absolutely required.

Thanks for the reviews. I'll think on this a it and mix in your
suggestions for the next version.

> ...note I did not look at anything beyond the claims in the changelog,
> like potential unwanted knock-on effect from the new masking.

I'm more concerned about the previous patch in this series. I have no
experience with slots that can reset a subset of attached bus devices,
but apparently they exist. Not that it should cause a problem beyond
extra locking and config space access that would't be necessary, which
should just cost a few unnecessary cpu cycles.