Message-ID: <20210623140711.ur67lbbrsudgxrp4@archlinux>
Date:   Wed, 23 Jun 2021 19:37:11 +0530
From:   Amey Narkhede <ameynarkhede03@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     alex.williamson@...hat.com,
        Raphael Norwitz <raphael.norwitz@...anix.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        kw@...ux.com, Shanker Donthineni <sdonthineni@...dia.com>,
        Sinan Kaya <okaya@...nel.org>, Len Brown <lenb@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v7 4/8] PCI/sysfs: Allow userspace to query and set
 device reset mechanism

On 21/06/23 07:06AM, Bjorn Helgaas wrote:
> On Tue, Jun 22, 2021 at 01:03:07AM +0530, Amey Narkhede wrote:
> > On 21/06/21 02:07PM, Bjorn Helgaas wrote:
> > > On Mon, Jun 21, 2021 at 10:58:54PM +0530, Amey Narkhede wrote:
> > > > On 21/06/21 08:01AM, Bjorn Helgaas wrote:
> > > > > On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:
> > > > > > On 21/06/18 03:00PM, Bjorn Helgaas wrote:
> > > > > > > On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:
> > > > > > > > Add reset_method sysfs attribute to enable user to
> > > > > > > > query and set user preferred device reset methods and
> > > > > > > > their ordering.
> > > > >
> > > > > > > > +	if (sysfs_streq(options, "default")) {
> > > > > > > > +		for (i = 0; i < PCI_RESET_METHODS_NUM; i++)
> > > > > > > > +			reset_methods[i] = reset_methods[i] ? prio-- : 0;
> > > > > > > > +		goto set_reset_methods;
> > > > > > > > +	}
> > > > > > >
> > > > > > > If you use pci_init_reset_methods() here, you can also get this case
> > > > > > > out of the way early.
> > > > > > >
> > > > > > The problem with alternate encoding is we won't be able to know if
> > > > > > one of the reset methods was disabled previously. For example,
> > > > > >
> > > > > > # cat reset_methods
> > > > > > flr,bus 			# dev->reset_methods = [3, 5, 0, ...]
> > > > > > # echo bus > reset_methods 	# dev->reset_methods = [5, 0, 0, ...]
> > > > > > # cat reset_methods
> > > > > > bus
> > > > > >
> > > > > > Now if an user wants to enable flr
> > > > > >
> > > > > > # echo flr > reset_methods 	# dev->reset_methods = [3, 0, 0, ...]
> > > > > > OR
> > > > > > # echo bus,flr > reset_methods 	# dev->reset_methods = [5, 3, 0, ...]
> > > > > >
> > > > > > either they need to write "default" first then flr or we will need to
> > > > > > reprobe reset methods each time when user writes to reset_method attribute.
> > > > >
> > > > > Not sure I completely understand the problem here.  I think relying on
> > > > > previous state that is invisible to the user is a little problematic
> > > > > because it's hard for the user to predict what will happen.
> > > > >
> > > > > If the user enables a method that was previously "disabled" because
> > > > > the probe failed, won't the reset method itself just fail with
> > > > > -ENOTTY?  Is that a problem?
> > > > >
> > > > I think I didn't explain this correctly. With current implementation
> > > > its not necessary to explicitly set *order of availabe* reset methods.
> > > > User can directly write a single supported reset method only and then perform
> > > > the reset. Side effect of that is other methods are disabled if user
> > > > writes single or less than available number of supported reset method.
> > > > Current implementation is able to handle this case but with new encoding
> > > > we'll need to reprobe reset methods everytime because we have no way
> > > > of distingushing supported and currently enabled reset method.
> > >
> > > I'm confused.  I thought the point of the nested loops to find the
> > > highest priority enabled reset method was to allow the user to control
> > > the order.  The sysfs doc says writing "reset_method" sets the "reset
> > > methods and their ordering."
> > >
> > > It seems complicated to track "supported" and "enabled" separately,
> > > and I don't know what the benefit is.  If we write "reset_method" to
> > > enable reset X, can we just probe reset X to see if it's supported?
> >
> > Although final result is same whether user writes a supported reset method or
> > their ordering that is,
> > # echo bus > reset_methods
> > and
> > # echo bus,flr > reset_methods
> >
> > are the same but in the first version, users don't have to explicitly
> > set the ordering if they just want to perform bus reset.
> > Current implementation allows the flexibility for switching between
> > first and second option.
>
> Sorry, I can't quite make sense of the above.
>
> Your doc implies the following are different:
>
>   # echo bus,flr > reset_methods
>   # echo flr,bus > reset_methods
>
> Are they?  If you don't need to provide control over the order of
> trying resets, this can all be simplified quite a bit.
>
> Bjorn
The v1 of the patch series allowed only single reset method to be
written instead of ordering of all supported reset methods.
With your example, current implementation allows both writing single
value and list of supported reset methods.

# echo bus > reset_methods
and
# echo bus,flr > reset_methods

OR

# echo flr > reset_methods
and
echo flr,bus > reset_methods

Its more of a preference than a functional point. Ultimately the
__pci_reset_function_locked() function will only perform 'bus' reset in
this example because we make sure 'reset_methods' file only contains
valid device supported reset methods all the time so
__pci_reset_function_locked() won't go into -ENOTTY case but with new
encoding theres no way to make sure `reset_methods`sysfs attribute will
contain valid supported reset methods all the time because of the reset
methods can be masked implicitly if user uses first option of writing
only single value.

My point is current implementation allows more flexibility for the user.

Thanks,
Amey

Powered by blists - more mailing lists