Re: [PATCH v1 10/12] PCI: hotplug: implement the hotplug_slot_ops callback for fpgahp

From: Russ Weight <russell.h.weight@intel.com>
To: Greg KH <gregkh@linuxfoundation.org>,
	Tianfei Zhang <tianfei.zhang@intel.com>
Cc: <bhelgaas@google.com>, <linux-pci@vger.kernel.org>,
	<linux-fpga@vger.kernel.org>, <lukas@wunner.de>,
	<kabel@kernel.org>, <mani@kernel.org>, <pali@kernel.org>,
	<mdf@kernel.org>, <hao.wu@intel.com>, <yilun.xu@intel.com>,
	<trix@redhat.com>, <jgg@ziepe.ca>, <ira.weiny@intel.com>,
	<andriy.shevchenko@linux.intel.com>, <dan.j.williams@intel.com>,
	<keescook@chromium.org>, <rafael@kernel.org>, <corbet@lwn.net>,
	<linux-doc@vger.kernel.org>, <ilpo.jarvinen@linux.intel.com>,
	<lee@kernel.org>, <matthew.gerlach@linux.intel.com>
Subject: Re: [PATCH v1 10/12] PCI: hotplug: implement the hotplug_slot_ops callback for fpgahp
Date: Fri, 20 Jan 2023 14:38:43 -0800	[thread overview]
Message-ID: <ea85cb02-a13d-f232-8ebd-c13893fc00c4@intel.com> (raw)
In-Reply-To: <Y8lFgKZGKYrM02Wm@kroah.com>

On 1/19/23 05:28, Greg KH wrote:
> On Wed, Jan 18, 2023 at 08:36:00PM -0500, Tianfei Zhang wrote:
>> Implement the image_load and available_images callback functions
>> for fpgahp driver. This patch leverages some APIs from pciehp
>> driver to implement the device reconfiguration below the PCI hotplug
>> bridge.
>>
>> Here are the steps for a process of image load.
>> 1. remove all PFs and VFs except the PF0.
>> 2. remove all non-reserved devices of PF0.
>> 3. trigger a image load via BMC.
>> 4. disable the link of the hotplug bridge.
>> 5. remove all reserved devices under PF0 and PCI devices
>>    below the hotplug bridge.
>> 6. wait for image load done via BMC, e.g. 10s.
>> 7. re-enable the link of the hotplug bridge.
>> 8. re-enumerate PCI devices below the hotplug bridge.
>>
>> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
>> ---
>>  Documentation/ABI/testing/sysfs-driver-fpgahp |  21 ++
>>  MAINTAINERS                                   |   1 +
>>  drivers/pci/hotplug/fpgahp.c                  | 179 ++++++++++++++++++
>>  3 files changed, 201 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-driver-fpgahp
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-fpgahp b/Documentation/ABI/testing/sysfs-driver-fpgahp
>> new file mode 100644
>> index 000000000000..8d4b1bfc4012
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-fpgahp
>> @@ -0,0 +1,21 @@
>> +What:		/sys/bus/pci/slots/X-X/available_images
>> +Date:		May 2023
>> +KernelVersion:	6.3
>> +Contact:	Tianfei Zhang <tianfei.zhang@intel.com>
>> +Description:	Read-only. This file returns a space separated list of
>> +		key words that may be written into the image_load file
>> +		described below. These keywords decribe an FPGA, BMC,
>> +		or firmware image in FLASH or EEPROM storage that may
>> +		be loaded.
> No, sysfs is "one value per file", why is this a list?
>
> And what exactly defines the values in this list?
>
>> +
>> +What:		/sys/bus/pci/slots/X-X/image_load
>> +Date:		May 2023
>> +KernelVersion:	6.3
>> +Contact:	Tianfei Zhang <tianfei.zhang@intel.com>
>> +Description:	Write-only. A key word may be written to this file to
>> +		trigger a new image loading of an FPGA, BMC, or firmware
>> +		image from FLASH or EEPROM. Refer to the available_images
>> +		file for a list of supported key words for the underlying
>> +		device.
>> +		Writing an unsupported string to this file will result in
>> +		EINVAL being returned.
> Why is this a separate file from the "read the list" file?

The intended usage is like this:

$ cat available_images
bmc_factory bmc_user fpga_factory fpga_user1 fpga_user2
$ echo bmc_user > image_load

This specifies which image stored in flash that you want to have activated
on the device.

An existing example of something like this is in the tracing code:
available_tracers and current_tracer

Would it be preferable to just create a file for each possible image,
and echo 1 to trigger the event? (echo 1 > bmc_user)

Thanks,
- Russ

> That feels wrong.
>
> thanks,
>
> greg k-h