Re: [PATCH v3 7/7] PCI: Change the type of probe argument in reset functions

[Amey Narkhede <ameynarkhede03@gmail.com>]

On 21/05/26 01:52PM, Alex Williamson wrote:
> On Wed, 26 May 2021 15:44:03 +0530
> Amey Narkhede <ameynarkhede03@gmail.com> wrote:
>
> > Introduce a new enum pci_reset_mode_t to make the context
> > of probe argument in reset functions clear and the code
> > easier to read.
> > Change the type of probe argument in functions which implement
> > reset methods from int to pci_reset_mode_t to make the intent clear.
> > Add a new line in return statement of pci_reset_bus_function.
> >
> > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > Suggested-by: Krzysztof Wilczyński <kw@linux.com>
> > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > ---
[...]
> >   */
> > -int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe)
> > +int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, enum pci_reset_mode probe)
>
> This should use your typedef, pci_reset_mode_t.  Is "probe" still the
> best name for this arg?  The enum name suggests a "mode", the MAX entry
> suggests an "action", "probe" is but one mode/action.
>
My bad I missed this. Which sounds more intuitive to you
"mode" or "action"? I update this in v4 to use consistent terminology
once we everybody agrees on name.
[...]
> > @@ -3910,11 +3922,16 @@ static int nvme_disable_and_flr(struct pci_dev *dev, int probe)
> >   * device too soon after FLR.  A 250ms delay after FLR has heuristically
> >   * proven to produce reliably working results for device assignment cases.
> >   */
> > -static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
> > +static int delay_250ms_after_flr(struct pci_dev *dev, pci_reset_mode_t probe)
> >  {
> > -	int ret = pcie_reset_flr(dev, probe);
> > +	int ret;
> > +
> > +	if (probe >= PCI_RESET_ACTION_MAX)
> > +		return -EINVAL;
>
> pcie_reset_flr() handles this case, we could simply test (ret || probe
> == PCI_RESET_PROBE) below.  In fact, that's probably what the code flow
> should have been regardless of this series.
>
[...]
> > -int pci_dev_specific_reset(struct pci_dev *dev, int probe)
> > +int pci_dev_specific_reset(struct pci_dev *dev, pci_reset_mode_t probe)
> >  {
> >  	const struct pci_dev_reset_methods *i;
> >
> > +	if (probe >= PCI_RESET_ACTION_MAX)
> > +		return -EINVAL;
>
> If we test this here, none of the device specific resets modified above
> need a duplicate check.  Thanks,
>
> Alex
>
I went ahead with excessive error checking in this patch.
Will update in v4.

Thanks,
Amey