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

From: Greg KH <gregkh@linuxfoundation.org>
To: 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,
	russell.h.weight@intel.com, 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: Thu, 19 Jan 2023 14:28:32 +0100	[thread overview]
Message-ID: <Y8lFgKZGKYrM02Wm@kroah.com> (raw)
In-Reply-To: <20230119013602.607466-11-tianfei.zhang@intel.com>

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?

That feels wrong.

thanks,

greg k-h