Message-ID:
 <DS7PR12MB6070785F5A9381D22D03536DCD49A@DS7PR12MB6070.namprd12.prod.outlook.com>
Date: Wed, 9 Jul 2025 05:34:17 +0000
From: "Manne, Nava kishore" <nava.kishore.manne@....com>
To: Xu Yilun <yilun.xu@...ux.intel.com>
CC: "mdf@...nel.org" <mdf@...nel.org>, "hao.wu@...el.com" <hao.wu@...el.com>,
	"yilun.xu@...el.com" <yilun.xu@...el.com>, "trix@...hat.com"
	<trix@...hat.com>, "robh@...nel.org" <robh@...nel.org>,
	"saravanak@...gle.com" <saravanak@...gle.com>, "linux-fpga@...r.kernel.org"
	<linux-fpga@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "git (AMD-Xilinx)" <git@....com>
Subject: RE: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for
 runtime FPGA configuration

Ping!

> -----Original Message-----
> From: Manne, Nava kishore <nava.kishore.manne@....com>
> Sent: Friday, June 20, 2025 4:45 PM
> To: Xu Yilun <yilun.xu@...ux.intel.com>
> Cc: mdf@...nel.org; hao.wu@...el.com; yilun.xu@...el.com; trix@...hat.com;
> robh@...nel.org; saravanak@...gle.com; linux-fpga@...r.kernel.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org; git (AMD-Xilinx)
> <git@....com>
> Subject: RE: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime
> FPGA configuration
> 
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi Yilun,
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@...ux.intel.com>
> > Sent: Friday, June 13, 2025 3:40 PM
> > To: Manne, Nava kishore <nava.kishore.manne@....com>
> > Cc: mdf@...nel.org; hao.wu@...el.com; yilun.xu@...el.com;
> > trix@...hat.com; robh@...nel.org; saravanak@...gle.com;
> > linux-fpga@...r.kernel.org; linux- kernel@...r.kernel.org;
> > devicetree@...r.kernel.org; git (AMD-Xilinx) <git@....com>
> > Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface
> > for runtime FPGA configuration
> >
> > On Thu, Jun 12, 2025 at 09:05:21AM +0000, Manne, Nava kishore wrote:
> > > [AMD Official Use Only - AMD Internal Distribution Only]
> > >
> > > Hi Yilun,
> > >
> > >         Thanks for quick response.
> > > Please find my response inline.
> > >
> > > > -----Original Message-----
> > > > From: Xu Yilun <yilun.xu@...ux.intel.com>
> > > > Sent: Sunday, June 1, 2025 8:32 PM
> > > > To: Manne, Nava kishore <nava.kishore.manne@....com>
> > > > Cc: mdf@...nel.org; hao.wu@...el.com; yilun.xu@...el.com;
> > > > trix@...hat.com; robh@...nel.org; saravanak@...gle.com;
> > > > linux-fpga@...r.kernel.org; linux- kernel@...r.kernel.org;
> > > > devicetree@...r.kernel.org; git (AMD-Xilinx) <git@....com>
> > > > Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS
> > > > interface for runtime FPGA configuration
> > > >
> > > > On Mon, May 19, 2025 at 09:09:37AM +0530, Nava kishore Manne wrote:
> > > > > Introduces an ConfigFS interface within the fpga-region
> > > > > subsystem, providing a generic and standardized mechanism for
> > > > > configuring
> > > > > (or) reprogramming FPGAs during runtime. The newly added
> > > > > interface supports both OF (Open Firmware) and non-OF devices,
> > > > > leveraging vendor-specific callbacks (e.g., pre_config,
> > > > > post_config, removal, and status) to accommodate a wide range of device
> specific configurations.
> > > > >
> > > > > The ConfigFS interface ensures compatibility with both OF and
> > > > > non-OF devices, allowing for seamless FPGA reprogramming across
> > > > > diverse platforms.
> > > > >
> > > > > Vendor-specific callbacks are integrated into the interface,
> > > > > enabling custom FPGA pre_config, post_config, removal, and
> > > > > status reporting mechanisms, ensuring flexibility for vendor implementations.
> > > > >
> > > > > This solution enhances FPGA runtime management, supporting
> > > > > various device types and vendors, while ensuring compatibility
> > > > > with the current FPGA configuration flow.
> > > > >
> > > > > Signed-off-by: Nava kishore Manne <nava.kishore.manne@....com>
> > > > > ---
> > > > > Changes for v3:
> > > > >  - As discussed with Yilun, the implementation continues to use
> > > > > a callback-based  approach to seamlessly support both OF (Open
> > > > > Firmware) and non-OF devices via  vendor-specific hooks.
> > > > > Additionally, the earlier IOCTL-based interface has been
> > > > > replaced with a more suitable ConfigFS-based mechanism to enable
> > > > > runtime FPGA
> > configuration.
> > > > >
> > > > > Changes for v2:
> > > > >  - As discussed with Yilun, the implementation has been modified
> > > > > to utilize a  callback approach, enabling seamless handling of
> > > > > both OF and non-OF
> > > > devices.
> > > > >
> > > > >  - As suggested by Yilun in the POC code, we have moved away
> > > > > from using  void *args  as a parameter for ICOTL inputs to
> > > > > obtain the required user inputs. Instead, we are  utilizing the
> > > > > fpga_region_config_info structure to gather user inputs.
> > > > > Currently, this structure is implemented to support only OF
> > > > > devices, but we intend to extend
> > > > it by incorporating new members to accommodate non-OF devices in the
> future.
> > > > >
> > > > >  drivers/fpga/fpga-region.c       | 196 +++++++++++++
> > > > >  drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
> > > > >  include/linux/fpga/fpga-region.h |  34 +++
> > > > >  3 files changed, 493 insertions(+), 211 deletions(-)
> > > > >
> > > > > diff --git a/drivers/fpga/fpga-region.c
> > > > > b/drivers/fpga/fpga-region.c index 753cd142503e..d583fc22955b
> > > > > 100644
> > > > > --- a/drivers/fpga/fpga-region.c
> > > > > +++ b/drivers/fpga/fpga-region.c
> > > > > @@ -5,6 +5,7 @@
> > > > >   *  Copyright (C) 2013-2016 Altera Corporation
> > > > >   *  Copyright (C) 2017 Intel Corporation
> > > > >   */
> > > > > +#include <linux/configfs.h>
> > > > >  #include <linux/fpga/fpga-bridge.h>  #include
> > > > > <linux/fpga/fpga-mgr.h>  #include <linux/fpga/fpga-region.h> @@
> > > > > -180,6 +181,158 @@ static struct attribute *fpga_region_attrs[]
> > > > > = { };  ATTRIBUTE_GROUPS(fpga_region);
> > > > >
> > > > > +static struct fpga_region *item_to_fpga_region(struct
> > > > > +config_item
> > > > > +*item) {
> > > > > +   return container_of(to_configfs_subsystem(to_config_group(item)),
> > > > > +                       struct fpga_region, subsys); }
> > > > > +
> > > > > +/**
> > > > > + * fpga_region_image_store - Set firmware image name for FPGA
> > > > > +region
> > > > > + * This function sets the firmware image name for an FPGA
> > > > > +region through
> > > > configfs.
> > > > > + * @item: Configfs item representing the FPGA region
> > > > > + * @buf: Input buffer containing the firmware image name
> > > > > + * @count: Size of the input buffer
> > > > > + *
> > > > > + * Return: Number of bytes written on success, or negative errno on failure.
> > > > > + */
> > > > > +static ssize_t fpga_region_image_store(struct config_item
> > > > > +*item, const char *buf, size_t count) {
> > > > > +   struct fpga_region *region = item_to_fpga_region(item);
> > > > > +   struct device *dev = &region->dev;
> > > > > +   struct fpga_image_info *info;
> > > > > +   char firmware_name[NAME_MAX];
> > > > > +   char *s;
> > > > > +
> > > > > +   if (region->info) {
> > > > > +           dev_err(dev, "Region already has already configured.\n");
> > > > > +           return -EINVAL;
> > > > > +   }
> > > > > +
> > > > > +   info = fpga_image_info_alloc(dev);
> > > > > +   if (!info)
> > > > > +           return -ENOMEM;
> > > > > +
> > > > > +   /* copy to path buffer (and make sure it's always zero terminated */
> > > > > +   count = snprintf(firmware_name, sizeof(firmware_name) - 1, "%s", buf);
> > > > > +   firmware_name[sizeof(firmware_name) - 1] = '\0';
> > > > > +
> > > > > +   /* strip trailing newlines */
> > > > > +   s = firmware_name + strlen(firmware_name);
> > > > > +   while (s > firmware_name && *--s == '\n')
> > > > > +           *s = '\0';
> > > > > +
> > > > > +   region->firmware_name = devm_kstrdup(dev, firmware_name,
> > > > GFP_KERNEL);
> > > > > +   if (!region->firmware_name)
> > > > > +           return -ENOMEM;
> > > > > +
> > > > > +   region->info = info;
> > > > > +
> > > > > +   return count;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * fpga_region_config_store - Trigger FPGA configuration via
> > > > > +configfs
> > > > > + * @item: Configfs item representing the FPGA region
> > > > > + * @buf: Input buffer containing the configuration command
> > > > > +(expects "1" to program, "0" to remove)
> > > > > + * @count: Size of the input buffer
> > > > > + *
> > > > > + * If the input is "1", this function performs:
> > > > > + *   1. region_pre_config() (if defined)
> > > >
> > > > Please define explicit workflow, and explicit expectation for each
> > > > callback, or this framework makes no sense. From your
> > > > of-fpga-region implementation, seems
> > > > pre_config() means "parse image", post_config() means "populate devices".
> > > >
> > > > > + *   2. Bitstream programming via fpga_region_program_fpga() (unless
> > external
> > > > config flag is set)
> > > > > + *   3. region_post_config() (if defined)
> > > > > + *
> > > > > + * If the input is "0", it triggers region_remove() (if defined).
> > > > > + *
> > > > > + * Return: Number of bytes processed on success, or negative
> > > > > + errno on
> > failure.
> > > >
> > > > Please put the uAPI description in Documentation/ABI/testing. Then
> > > > we could know the file path layout from userspace POV.
> > > >
> > > > > + */
> > > > > +static ssize_t fpga_region_config_store(struct config_item *item,
> > > > > +                                   const char *buf, size_t
> > > > > +count) {
> > > > > +   struct fpga_region *region = item_to_fpga_region(item);
> > > > > +   int config_value, ret = 0;
> > > > > +
> > > > > +   /* Parse input: must be "0" or "1" */
> > > > > +   if (kstrtoint(buf, 10, &config_value) || (config_value != 0
> > > > > + && config_value !=
> > > > 1))
> > > > > +           return -EINVAL;
> > > > > +
> > > > > +   /* Ensure fpga_image_info is available */
> > > > > +   if (!region->info)
> > > > > +           return -EINVAL;
> > > > > +
> > > > > +   if (config_value == 1) {
> > > > > +           /* Pre-config */
> > > > > +           if (region->region_ops->region_pre_config) {
> > > > > +                   ret = region->region_ops->region_pre_config(region);
> > > > > +                   if (ret)
> > > > > +                           return ret;
> > > > > +           }
> > > > > +
> > > > > +           /* Program bitstream if not external */
> > > > > +           if (!(region->info->flags & FPGA_MGR_EXTERNAL_CONFIG)) {
> > > > > +                   ret = fpga_region_program_fpga(region);
> > > > > +                   if (ret)
> > > > > +                           return ret;
> > > > > +           }
> > > > > +
> > > > > +           /* Post-config */
> > > > > +           if (region->region_ops->region_post_config) {
> > > > > +                   ret = region->region_ops->region_post_config(region);
> > > > > +                   if (ret)
> > > > > +                           return ret;
> > > > > +           }
> > > > > +
> > > > > +   } else {
> > > > > +           /* Remove configuration */
> > > > > +           if (region->region_ops->region_remove) {
> > > > > +                   ret = region->region_ops->region_remove(region);
> > > > > +                   if (ret)
> > > > > +                           return ret;
> > > > > +           }
> > > > > +   }
> > > > > +
> > > > > +   return count;
> > > > > +}
> > > > > +
> > > > > +/* Define Attributes */
> > > > > +CONFIGFS_ATTR_WO(fpga_region_, image);
> > > > CONFIGFS_ATTR_WO(fpga_region_,
> > > > > +config);
> > > > > +
> > > > > +/* Attribute List */
> > > > > +static struct configfs_attribute *fpga_region_config_attrs[] = {
> > > > > +   &fpga_region_attr_image,
> > > > > +   &fpga_region_attr_config,
> > > > > +   NULL,
> > > > > +};
> > > > > +
> > > > > +/* ConfigFS Item Type */
> > > > > +static const struct config_item_type fpga_region_item_type = {
> > > > > +   .ct_attrs = fpga_region_config_attrs,
> > > > > +   .ct_owner = THIS_MODULE,
> > > > > +};
> > > >
> > > > I think this is still the sysfs methodology. My understanding from
> > > > configfs.rst is, use userspace interfaces to control the lifecycle of a kernel
> object.
> > > >
> > > > Now for existing kernel reprogramming flow, the image object for
> > > > fpga_region is the struct fpga_image_info. We need to associate
> > > > the struct with a config_item: alloc the struct fpga_image_info
> > > > instance by mkdir, expose necessary fields (enable_timeout_us,
> > > > disable_timeout_us, firmware_name, and the most important for
> > > > of-fpga-region - overlay blob ...) for user to fill/query via
> > > > configfs attributes. And
> > finally use a writeable attribute (e.g. load) to trigger fpga_region_program_fpga().
> > > >
> > >
> > > Thanks for the inputs. We've now implemented a ConfigFS-based
> > > reprogramming interface for fpga_region that aligns with the
> > > intended lifecycle-driven model described in configfs.rst.
> > >
> > > New Interface Usage Instructions:
> > > # 1. Mount configfs (if not already mounted) mkdir -p /configfs
> > > mount -t configfs configfs /configfs
> > >
> > > # 2. Create a region directory (e.g., region0) mkdir
> > > /configfs/fpga_regions/region0
> >
> > FPGA region is the device driver generated kernel object, so this per
> > fpga_region group should be created by kernel, not userspace.  User
> > should create fpga_image config_item under region group.
> >
> >   mkdir /configfs/fpga_region/region0/my_image
> >
> > >
> > > # 3. Copy your bitstream or overlay files to firmware path cp
> > > pl.dtbo /lib/firmware/
> > >
> > > # 4. Set the firmware name (e.g., overlay .dtbo) echo pl.dtbo >
> > > /configfs/fpga_regions/region0/image
> > >
> > > # 5. Trigger Programming/reprogramming echo 1 >
> > > /configfs/fpga_regions/region0/config
> >
> > My idea is, when an image item is first created, it is not activated,
> > user needs to read/write its attributes to initialize it, then we
> > could have an attribute (e.g. enable) to activate/reprograme the image.
> >
> > For example:
> >
> >   echo 1 > /configfs/fpga_regions/region0/my_image/enable
> >
> > >
> > > # 6. Unload/Reset the Programmable Logic echo 0 >
> > > /configfs/fpga_regions/region0/config
> >
> > Maybe we could just delete the image item for unloading
> >
> >   rmdir /configfs/fpga_region/region0/my_image
> >
> I’ve implemented the FPGA Region ConfigFS interface with the following hierarchy:
> 
> /configfs
> └── fpga_regions             ← Registered via configfs_register_subsystem()
>       └── region0                ← Added using configfs_add_default_group()
>            └── my_image      ← Created via mkdir from userspace
>                 ├── firmware    ← Write firmware name here
>                 └── config        ← Trigger programming/unloading
> Observation:
> If configfs is not mounted before configfs_add_default_group() is invoked (e.g., when
> regions are registered early via base DTB), the path
> /configfs/fpga_regions/region0 does not appear in userspace, even though it’s
> properly initialized in the kernel.
> 
> This appears to be due to how default groups function.
> they require the configfs filesystem to be mounted prior to the group addition in order
> to be visible. As a result, the mount order becomes a strict dependency, which may
> affect or break early-boot FPGA flows where regions are created before configfs is
> available.
> 
> Proposal:
> Use configfs_register_subsystem(&region->cfg_subsys) for each FPGA region
> instead of relying on configfs_add_default_group().
> 
> This approach places each FPGA region directly under /configfs/region0, avoiding
> the timing issues associated with default groups.
> The interface becomes available as soon as configfs is mounted.
> regardless of when the region was registered (boot time via base DTB or
> dynamically via overlays).
> 
> New user hierarchy:
> /configfs
> └── region0              ← Region appears as its own root node
>       └── my_image   ← Created via mkdir from userspace
>            ├── firmware ← Write firmware name here
>            └── config     ← Trigger programming/unloading
> 
> Would like to know if this approach looks good, or if there are better suggestions to
> handle this scenario?
> 
> Regards,
> Navakishore.

Powered by blists - more mailing lists