Date:   Thu, 16 Aug 2018 15:04:51 -0500
From:   Alan Tull <atull@...nel.org>
To:     Moritz Fischer <mdf@...nel.org>
Cc:     Jonathan Corbet <corbet@....net>,
        Randy Dunlap <rdunlap@...radead.org>,
        Dinh Nguyen <dinguyen@...nel.org>,
        Appana Durga Kedareswara Rao <appanad@...inx.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-fpga@...r.kernel.org,
        Linux Doc Mailing List <linux-doc@...r.kernel.org>,
        Alan Tull <atull@...nsource.altera.com>,
        Matthew Gerlach <matthew.gerlach@...ux.intel.com>
Subject: Re: [PATCH 2/2] fpga: add FPGA manager debugfs

On Thu, Aug 16, 2018 at 1:59 PM, Moritz Fischer <mdf@...nel.org> wrote:
> Hi Alan,

Hi Moritz,

> comments inline. While I see how this is useful, I have the
> suspicion that from the moment this gets merged vendor kernels
> will just default to use this ...

Yeah, I have that suspicion as well.  That's probably why I sat on
this and didn't upstream it for 2 years.  But on the other hand, I
keep hearing of lots of cases of people implementing this
independently anyway.  At least if it is debugfs, it makes it clear
that it's not intended for production use.

>
> On Wed, Aug 15, 2018 at 05:09:58PM -0500, Alan Tull wrote:
>> From: Alan Tull <atull@...nsource.altera.com>
>>
>> Implement DebugFS for the FPGA Manager Framework for
>> debugging and developmental use.
>>
>> Enabled by CONFIG_FPGA_MGR_DEBUG_FS
>>
>> Each FPGA gets its own directory such as
>>  <debugfs>/fpga_manager/fpga0 and three files:
>>
>>  * [RW] flags          = flags as defined in fpga-mgr.h
>>  * [RW] firmware_name  = write/read back name of FPGA image
>>                          firmware file to program
>>  * [WO] image          = write-only file for directly writing
>>                          fpga image w/o firmware layer
>>  * [RW] config_complete_timeout_us = maximum for the FPGA to
>>                          go to the operating state after
>>                        programming
>>
>> The debugfs is implemented in a separate fpga_mgr_debugfs.c
>> file, but the FPGA manager core is still built as one
>> module.  Note the name change from fpga-mgr.ko to fpga_mgr.ko.
>>
>> Signed-off-by: Alan Tull <atull@...nel.org>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
>> ---
>>  drivers/fpga/Kconfig            |   7 ++
>>  drivers/fpga/Makefile           |   4 +-
>>  drivers/fpga/fpga-mgr-debugfs.c | 221 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/fpga/fpga-mgr-debugfs.h |  22 ++++
>>  drivers/fpga/fpga-mgr.c         |   8 ++
>>  include/linux/fpga/fpga-mgr.h   |   3 +
>>  6 files changed, 264 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.c
>>  create mode 100644 drivers/fpga/fpga-mgr-debugfs.h
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 1ebcef4..600924d 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -9,6 +9,13 @@ menuconfig FPGA
>>         kernel.  The FPGA framework adds a FPGA manager class and FPGA
>>         manager drivers.
>>
>> +config FPGA_MGR_DEBUG_FS
>> +       bool "FPGA Manager DebugFS"
>> +       depends on FPGA && DEBUG_FS
>> +       help
>> +         Say Y here if you want to expose a DebugFS interface for the
>> +      FPGA Manager Framework.
>> +
>>  if FPGA
>>
>>  config FPGA_MGR_SOCFPGA
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 7a2d73b..62910cc 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -4,7 +4,9 @@
>>  #
>>
>>  # Core FPGA Manager Framework
>> -obj-$(CONFIG_FPGA)                   += fpga-mgr.o
>> +obj-$(CONFIG_FPGA)                   += fpga_mgr.o
>> +fpga_mgr-y                           := fpga-mgr.o
>> +fpga_mgr-$(CONFIG_FPGA_MGR_DEBUG_FS) += fpga-mgr-debugfs.o
>>
>>  # FPGA Manager Drivers
>>  obj-$(CONFIG_FPGA_MGR_ALTERA_CVP)    += altera-cvp.o
>> diff --git a/drivers/fpga/fpga-mgr-debugfs.c b/drivers/fpga/fpga-mgr-debugfs.c
>> new file mode 100644
>> index 0000000..f2fdf58
>> --- /dev/null
>> +++ b/drivers/fpga/fpga-mgr-debugfs.c
>> @@ -0,0 +1,221 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * FPGA Manager DebugFS
>> + *
>> + *  Copyright (C) 2016-2018 Intel Corporation.  All rights reserved.
>> + */
>> +#include <linux/debugfs.h>
>> +#include <linux/fpga/fpga-mgr.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +
>> +static struct dentry *fpga_mgr_debugfs_root;
>> +
>> +struct fpga_mgr_debugfs {
>> +     struct dentry *debugfs_dir;
>> +     struct fpga_image_info *info;
>> +};
>> +
>> +static ssize_t fpga_mgr_firmware_write_file(struct file *file,
>> +                                         const char __user *user_buf,
>> +                                         size_t count, loff_t *ppos)
>> +{
>> +     struct fpga_manager *mgr = file->private_data;
>> +     struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
>> +     struct device *dev = &mgr->dev;
>> +     char *buf;
>> +     int ret;
>> +
>> +     ret = fpga_mgr_lock(mgr);
>> +     if (ret) {
>> +             dev_err(dev, "FPGA manager is busy\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     buf = devm_kzalloc(dev, count, GFP_KERNEL);
>> +     if (!buf) {
>> +             fpga_mgr_unlock(mgr);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     if (copy_from_user(buf, user_buf, count)) {
>> +             fpga_mgr_unlock(mgr);
>> +             devm_kfree(dev, buf);
>> +             return -EFAULT;
>> +     }
>> +
>> +     buf[count] = 0;
>> +     if (buf[count - 1] == '\n')
>> +             buf[count - 1] = 0;
>> +
>> +     /* Release previous firmware name (if any). Save current one. */
>> +     if (debugfs->info->firmware_name)
>> +             devm_kfree(dev, debugfs->info->firmware_name);
>> +     debugfs->info->firmware_name = buf;
>> +
>> +     ret = fpga_mgr_load(mgr, debugfs->info);
>> +     if (ret)
>> +             dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
>> +
>> +     fpga_mgr_unlock(mgr);
>> +
>> +     return count;
>> +}
>> +
>> +static ssize_t fpga_mgr_firmware_read_file(struct file *file,
>> +                                        char __user *user_buf,
>> +                                        size_t count, loff_t *ppos)
>> +{
>> +     struct fpga_manager *mgr = file->private_data;
>> +     struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
>> +     char *buf;
>> +     int ret;
>> +
>> +     if (!debugfs->info->firmware_name)
>> +             return 0;
>> +
>> +     buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> +     if (!buf)
>> +             return -ENOMEM;
>> +
>> +     ret = snprintf(buf, PAGE_SIZE, "%s\n", debugfs->info->firmware_name);
> snip:
> ---->8->8->8-----
>> +     if (ret < 0) {
>> +             kfree(buf);
>> +             return ret;
>> +     }
> ---->8->8->8-----
> just replace with:
>         if (ret < 0)
>                 goto out;
>> +
>> +     ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>
> out:
>> +     kfree(buf);
>> +     return ret;
>> +}

Yes, that's better. I'll include that in v2.

>> +
>> +static const struct file_operations fpga_mgr_firmware_fops = {
>> +     .open = simple_open,
>> +     .read = fpga_mgr_firmware_read_file,
>> +     .write = fpga_mgr_firmware_write_file,
>> +     .llseek = default_llseek,
>> +};
>> +
>> +static ssize_t fpga_mgr_image_write_file(struct file *file,
>> +                                      const char __user *user_buf,
>> +                                      size_t count, loff_t *ppos)
>> +{
>> +     struct fpga_manager *mgr = file->private_data;
>> +     struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
>> +     struct device *dev = &mgr->dev;
>> +     char *buf;
>> +     int ret;
>> +
>> +     dev_info(dev, "writing %zu bytes to %s\n", count, mgr->name);
>> +
>> +     ret = fpga_mgr_lock(mgr);
>> +     if (ret) {
>> +             dev_err(dev, "FPGA manager is busy\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     buf = kzalloc(count, GFP_KERNEL);
>> +     if (!buf) {
>> +             fpga_mgr_unlock(mgr);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     if (copy_from_user(buf, user_buf, count)) {
>> +             fpga_mgr_unlock(mgr);
>> +             kfree(buf);
>> +             return -EFAULT;
>> +     }
>> +
>> +     /* If firmware interface was previously used, forget it. */
>> +     if (debugfs->info->firmware_name)
>> +             devm_kfree(dev, debugfs->info->firmware_name);
>> +     debugfs->info->firmware_name = NULL;
>> +
>> +     debugfs->info->buf = buf;
>> +     debugfs->info->count = count;
>> +
>> +     ret = fpga_mgr_load(mgr, debugfs->info);
>> +     if (ret)
>> +             dev_err(dev, "fpga_mgr_load returned with value %d\n", ret);
>> +
>> +     fpga_mgr_unlock(mgr);
>> +
>> +     debugfs->info->buf = NULL;
>> +     debugfs->info->count = 0;
>
> Is that ordering between unlock() and setting those correct?

This order should be alright, but I'll switch it anyway to be more
obviously correct.

>> +
>> +     kfree(buf);
>> +
>> +     return count;
>> +}
>> +
>> +static const struct file_operations fpga_mgr_image_fops = {
>> +     .open = simple_open,
>> +     .write = fpga_mgr_image_write_file,
>> +     .llseek = default_llseek,
>> +};
>> +
>> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr)
>> +{
>> +     struct device *dev = &mgr->dev;
>> +     struct fpga_mgr_debugfs *debugfs;
>> +     struct fpga_image_info *info;
>> +
>> +     if (!fpga_mgr_debugfs_root)
>> +             return;
>> +
>> +     debugfs = kzalloc(sizeof(*debugfs), GFP_KERNEL);
>> +     if (!debugfs)
>> +             return;
>> +
>> +     info = fpga_image_info_alloc(dev);
>> +     if (!info) {
>> +             kfree(debugfs);
>> +             return;
>> +     }
>> +     debugfs->info = info;
>> +
>> +     debugfs->debugfs_dir = debugfs_create_dir(dev_name(dev),
>> +                                               fpga_mgr_debugfs_root);
>> +
>> +     debugfs_create_file("firmware_name", 0600, debugfs->debugfs_dir, mgr,
>> +                         &fpga_mgr_firmware_fops);
>> +
>> +     debugfs_create_file("image", 0200, debugfs->debugfs_dir, mgr,
>> +                         &fpga_mgr_image_fops);
>> +
>> +     debugfs_create_u32("flags", 0600, debugfs->debugfs_dir, &info->flags);
>> +
>> +     debugfs_create_u32("config_complete_timeout_us", 0600,
>> +                        debugfs->debugfs_dir,
>> +                        &info->config_complete_timeout_us);
>> +
>> +     mgr->debugfs = debugfs;
>> +}
>> +
>> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr)
>> +{
>> +     struct fpga_mgr_debugfs *debugfs = mgr->debugfs;
>> +
>> +     if (!fpga_mgr_debugfs_root)
>> +             return;
>> +
>> +     debugfs_remove_recursive(debugfs->debugfs_dir);
>> +
>> +     /* this function also frees debugfs->info->firmware_name */
>> +     fpga_image_info_free(debugfs->info);
>> +
>> +     kfree(debugfs);
>> +}
>> +
>> +void fpga_mgr_debugfs_init(void)
>> +{
>> +     fpga_mgr_debugfs_root = debugfs_create_dir("fpga_manager", NULL);
>> +     if (!fpga_mgr_debugfs_root)
>> +             pr_warn("fpga_mgr: Failed to create debugfs root\n");
>> +}
>> +
>> +void fpga_mgr_debugfs_uninit(void)
>> +{
>> +     debugfs_remove_recursive(fpga_mgr_debugfs_root);
>> +}
>> diff --git a/drivers/fpga/fpga-mgr-debugfs.h b/drivers/fpga/fpga-mgr-debugfs.h
>> new file mode 100644
>> index 0000000..17cd3eb
>> --- /dev/null
>> +++ b/drivers/fpga/fpga-mgr-debugfs.h
>> @@ -0,0 +1,22 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#ifndef _LINUX_FPGA_MGR_DEBUGFS_H
>> +#define _LINUX_FPGA_MGR_DEBUGFS_H
>> +
>> +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
>> +
>> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr);
>> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr);
>> +void fpga_mgr_debugfs_init(void);
>> +void fpga_mgr_debugfs_uninit(void);
>> +
>> +#else
>> +
>> +void fpga_mgr_debugfs_add(struct fpga_manager *mgr) {}
>> +void fpga_mgr_debugfs_remove(struct fpga_manager *mgr) {}
>> +void fpga_mgr_debugfs_init(void) {}
>> +void fpga_mgr_debugfs_uninit(void) {}
>> +
>> +#endif /* CONFIG_FPGA_MGR_DEBUG_FS */
>> +
>> +#endif /*_LINUX_FPGA_MGR_DEBUGFS_H */
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index c8684bc..66eb6f6 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/scatterlist.h>
>>  #include <linux/highmem.h>
>> +#include "fpga-mgr-debugfs.h"
>>
>>  static DEFINE_IDA(fpga_mgr_ida);
>>  static struct class *fpga_mgr_class;
>> @@ -698,6 +699,8 @@ int fpga_mgr_register(struct fpga_manager *mgr)
>>       if (ret)
>>               goto error_device;
>>
>> +     fpga_mgr_debugfs_add(mgr);
>> +
>>       dev_info(&mgr->dev, "%s registered\n", mgr->name);
>>
>>       return 0;
>> @@ -722,6 +725,8 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
>>  {
>>       dev_info(&mgr->dev, "%s %s\n", __func__, mgr->name);
>>
>> +     fpga_mgr_debugfs_remove(mgr);
>> +
>>       /*
>>        * If the low level driver provides a method for putting fpga into
>>        * a desired state upon unregister, do it.
>> @@ -748,11 +753,14 @@ static int __init fpga_mgr_class_init(void)
>>       fpga_mgr_class->dev_groups = fpga_mgr_groups;
>>       fpga_mgr_class->dev_release = fpga_mgr_dev_release;
>>
>> +     fpga_mgr_debugfs_init();
>> +
>>       return 0;
>>  }
>>
>>  static void __exit fpga_mgr_class_exit(void)
>>  {
>> +     fpga_mgr_debugfs_uninit();
>>       class_destroy(fpga_mgr_class);
>>       ida_destroy(&fpga_mgr_ida);
>>  }
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 777c502..e8f2159 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -170,6 +170,9 @@ struct fpga_manager {
>>       struct fpga_compat_id *compat_id;
>>       const struct fpga_manager_ops *mops;
>>       void *priv;
>> +#if IS_ENABLED(CONFIG_FPGA_MGR_DEBUG_FS)
>> +     void *debugfs;
>> +#endif
>>  };
>>
>>  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
>> --
>> 2.7.4
>>
>
> Thanks,
>
> Moritz

Thanks for the review!

Alan

Powered by blists - more mailing lists