Re: [PATCH v1 00/12] add FPGA hotplug manager driver

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 00/12] add FPGA hotplug manager driver
Date: Fri, 20 Jan 2023 08:28:51 -0800	[thread overview]
Message-ID: <a896be81-e482-9d52-ece5-a2ef28822072@intel.com> (raw)
In-Reply-To: <Y8lGxqjuLS8NfJtg@kroah.com>


On 1/19/23 05:33, Greg KH wrote:
> On Wed, Jan 18, 2023 at 08:35:50PM -0500, Tianfei Zhang wrote:
>> This patchset introduces the FPGA hotplug manager (fpgahp) driver which 
>> has been verified on the Intel N3000 card.
>>
>> When a PCIe-based FPGA card is reprogrammed, it temporarily disappears
>> from the PCIe bus. This needs to be managed to avoid PCIe errors and to
>> reprobe the device after reprogramming.
>>
>> To change the FPGA image, the kernel burns a new image into the flash on
>> the card, and then triggers the card BMC to load the new image into FPGA.
>> A new FPGA hotplug manager driver is introduced that leverages the PCIe
>> hotplug framework to trigger and manage the update of the FPGA image,
>> including the disappearance and reappearance of the card on the PCIe bus.
>> The fpgahp driver uses APIs from the pciehp driver. Two new operation
>> callbacks are defined in hotplug_slot_ops:
>>
>>   - available_images: Optional: available FPGA images
>>   - image_load: Optional: trigger the FPGA to load a new image
>>
>>
>> The process of reprogramming an FPGA card begins by removing all devices
>> associated with the card that are not required for the reprogramming of
>> the card. This includes PCIe devices (PFs and VFs) associated with the
>> card as well as any other types of devices (platform, etc.) defined within
>> the FPGA. The remaining devices are referred to here as "reserved" devices.
>> After triggering the update of the FPGA card, the reserved devices are also
>> removed.
>>
>> The complete process for reprogramming the FPGA are:
>>     1. remove all PFs and VFs except for PF0 (reserved).
>>     2. remove all non-reserved devices of PF0.
>>     3. trigger FPGA card to do the image update.
>>     4. disable the link of the hotplug bridge.
>>     5. remove all reserved devices under hotplug bridge.
>>     6. wait for image reload done via BMC, e.g. 10s.
>>     7. re-enable the link of hotplug bridge
>>     8. enumerate PCI devices below the hotplug bridge
>>
>> usage example:
>> [root@localhost]# cd /sys/bus/pci/slot/X-X/
>>
>> Get the available images.
>> [root@localhost 2-1]# cat available_images
>> bmc_factory bmc_user retimer_fw
>>
>> Load the request images for FPGA Card, for example load the BMC user image:
>> [root@localhost 2-1]# echo bmc_user > image_load
> Why is all of this tied into the pci hotplug code? Shouldn't it be
> specific to this one driver instead?  pci hotplug is for removing/adding
> PCI devices to the system, not messing with FPGA images.
>
> This feels like an abuse of the pci hotplug bus to me as this is NOT
> really a PCI hotplug bus at all, right?
While it is true that triggering an FPGA image-load does not involve
hotplug specific registers to be managed, the RTL that comprises
the PCIe interface will disappear and then reappear after the FPGA
is reprogrammed. When it reappears, it_could/_/have a different PCI
ID. The process of managing this event has a lot of similarity to a
PCIe hotplug event; there is a lot of existing PCIe hotplug related
code that could be leveraged.

As alternatives to the idea of creating a hotplug driver, we have
considered creating a new PCIe service driver specifically to
handle FPGA reprogramming, or modifying the existing hotplug
driver(s) to add FPGA support. We have also considered a separate
fpga-reload driver that would not be bound to a PCIe interface,
but would still leverage the PCIe code to manage the event. Do
any of these options sound preferable to creating an FPGA hotplug
driver?
>
> Or is it?  If so, then the slots should show up under the PCI device
> itself, not in /sys/bus/pci/slot/.  That location is there for old old
> stuff, we probably should move it one of these days as there's lots of
> special-cases in the driver core just because of that :(
>
> thanks,
>
> greg k-h