Re: [PATCH v7 4/8] PCI/sysfs: Allow userspace to query and set device reset mechanism

On Mon, Jun 21, 2021 at 10:58:54PM +0530, Amey Narkhede wrote:

On 21/06/21 08:01AM, Bjorn Helgaas wrote:

quoted

On Sat, Jun 19, 2021 at 07:29:20PM +0530, Amey Narkhede wrote:

quoted

On 21/06/18 03:00PM, Bjorn Helgaas wrote:

quoted

On Tue, Jun 08, 2021 at 11:18:53AM +0530, Amey Narkhede wrote:

quoted

Add reset_method sysfs attribute to enable user to
query and set user preferred device reset methods and
their ordering.

quoted

quoted

quoted

+	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?

Bjorn