From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_PASS,T_DKIMWL_WL_HIGH,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4854AC4321D for ; Thu, 16 Aug 2018 20:05:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D83E321473 for ; Thu, 16 Aug 2018 20:05:35 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="bW0EOcnZ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D83E321473 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725955AbeHPXGC (ORCPT ); Thu, 16 Aug 2018 19:06:02 -0400 Received: from mail.kernel.org ([198.145.29.99]:44710 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725783AbeHPXGB (ORCPT ); Thu, 16 Aug 2018 19:06:01 -0400 Received: from mail-yb0-f171.google.com (mail-yb0-f171.google.com [209.85.213.171]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 910822147A; Thu, 16 Aug 2018 20:05:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1534449932; bh=wuroVYR0JEL3FEwqmKMg7RUchM6aOYjUuGXdCtpFf20=; h=In-Reply-To:References:From:Date:Subject:To:Cc:From; b=bW0EOcnZOnSGV+rA/wRMgffKKWAv07GUTDSV/mc245YvWayXQ8W3kARBYpZ1Oervn 89ADOlOWxGdJYOurxQwkH3gKxsxcl4UV+lUoJhgs8o1Ba2bHt+wQ7DzWR3zszkoycG 7HLIaVZ6+Y1U9TjmtkU+Y9fkQjIpYEmZ8JGPa1Jw= Received: by mail-yb0-f171.google.com with SMTP id r3-v6so1103477ybo.4; Thu, 16 Aug 2018 13:05:32 -0700 (PDT) X-Gm-Message-State: AOUpUlGKLgU3Wav3n+hdpptmw+VbCNmaRAqkH5ROR0jXprcjt4uxR9Jl 27o+xzTZ/PHVCGVhhiqItId4Jacp8D2pOx6uor4= X-Google-Smtp-Source: AA+uWPyO7YrzhCzyjDCoUjS8yke/XVC0Dt3z84Bw8F7SpBmthtR+5tcxuGclPVocypffQX/jbm7Rt3xoFgd2Ry5V/Qw= X-Received: by 2002:a25:f212:: with SMTP id i18-v6mr17441738ybe.267.1534449931724; Thu, 16 Aug 2018 13:05:31 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a25:81c8:0:0:0:0:0 with HTTP; Thu, 16 Aug 2018 13:04:51 -0700 (PDT) In-Reply-To: <20180816185952.GA3932@archbook> References: <20180815220958.3606-1-atull@kernel.org> <20180815220958.3606-2-atull@kernel.org> <20180816185952.GA3932@archbook> From: Alan Tull Date: Thu, 16 Aug 2018 15:04:51 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/2] fpga: add FPGA manager debugfs To: Moritz Fischer Cc: Jonathan Corbet , Randy Dunlap , Dinh Nguyen , Appana Durga Kedareswara Rao , linux-kernel , linux-fpga@vger.kernel.org, Linux Doc Mailing List , Alan Tull , Matthew Gerlach Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 16, 2018 at 1:59 PM, Moritz Fischer 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 >> >> 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 >> /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 >> Signed-off-by: Matthew Gerlach >> --- >> 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 >> +#include >> +#include >> +#include >> + >> +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 >> #include >> #include >> +#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