LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Wu Hao <hao.wu@intel.com>
To: Xu Yilun <yilun.xu@intel.com>
Cc: mdf@kernel.org, linux-fpga@vger.kernel.org,
	linux-kernel@vger.kernel.org, Luwei Kang <luwei.kang@intel.com>
Subject: Re: [PATCH 4/7] fpga: dfl: afu: add interrupt support for error reporting
Date: Wed, 11 Mar 2020 10:43:56 +0800	[thread overview]
Message-ID: <20200311024356.GA26202@hao-dev> (raw)
In-Reply-To: <20200310164738.GC30868@yilunxu-OptiPlex-7050>

On Wed, Mar 11, 2020 at 12:47:38AM +0800, Xu Yilun wrote:
> On Tue, Mar 10, 2020 at 06:39:21PM +0800, Wu Hao wrote:
> > On Mon, Mar 09, 2020 at 06:29:47PM +0800, Xu Yilun wrote:
> > > Error reporting interrupt is very useful to notify users that some
> > > errors are detected by the hardware. Once users are notified, they
> > > could query hardware logged error states, no need to continuously
> > > poll on these states.
> > > 
> > > This patch follows the common DFL interrupt notification and handling
> > > mechanism, implements two ioctl commands below for user to query
> > > hardware capability, and set/unset interrupt triggers.
> > > 
> > >  Ioctls:
> > >  * DFL_FPGA_PORT_ERR_GET_INFO
> > >    get error reporting feature info, including num_irqs which is used to
> > >    determine whether/how many interrupts it supports.
> > > 
> > >  * DFL_FPGA_PORT_ERR_SET_IRQ
> > >    set/unset given eventfds as error interrupt triggers.
> > > 
> > > Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> > > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > > ---
> > >  drivers/fpga/dfl-afu-error.c  | 69 +++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/fpga/dfl-afu-main.c   |  4 +++
> > >  include/uapi/linux/fpga-dfl.h | 34 +++++++++++++++++++++
> > >  3 files changed, 107 insertions(+)
> > > 
> > > diff --git a/drivers/fpga/dfl-afu-error.c b/drivers/fpga/dfl-afu-error.c
> > > index c1467ae..a2c5454 100644
> > > --- a/drivers/fpga/dfl-afu-error.c
> > > +++ b/drivers/fpga/dfl-afu-error.c
> > > @@ -15,6 +15,7 @@
> > >   */
> > >  
> > >  #include <linux/uaccess.h>
> > > +#include <linux/fpga-dfl.h>
> > >  
> > >  #include "dfl-afu.h"
> > >  
> > > @@ -219,6 +220,73 @@ static void port_err_uinit(struct platform_device *pdev,
> > >  	afu_port_err_mask(&pdev->dev, true);
> > >  }
> > >  
> > > +static long
> > > +port_err_get_info(struct platform_device *pdev,
> > > +		  struct dfl_feature *feature, unsigned long arg)
> > > +{
> > > +	struct dfl_fpga_port_err_info info;
> > > +
> > > +	info.flags = 0;
> > > +	info.capability = 0;
> > 
> > as flags and capability are not used at this moment, so actually it only exposes
> > irq information to user. I understand flags and capability are used for
> > future extension, but it may not work without argsz, as we never know what
> > comes next, e.g. a capability requires > 32bit can't fit into this ioctl.
> > So maybe just a ioctl for IRQ_INFO is enough for now.
> > 
> > How do you think?
> 
> Yes the flags & capability are for future extension.
> 
> The capability field is planned to a bitmask, each bit could indicate the feature
> has some capability or not. So it could describe up to 32 capabilities.
> I think it would be enough for one sub feature.
> 
> With this field, users could get a general description of capabilities with one
> ioctl. If some capability has more detailed info, we may add addtional ioctl to
> fetch it. This is how it works without argsz. Does it make sense?
> 
> And same definition for flag field. The flag fields could contain some
> bool running state represented by each bit.

This should work for some cases, but kernel doc (core-api/ioctl.rst) says it's
better to avoid bitfield completely. I understand it's possible to extend this
ioctl with flags and capability, even we can define if flags = A, then given 
capability = definition B, if flags = C, then capbaility definition is D, to
maximum the usage for extension, but it may make this interface very very
complicated to users. This should be the same reason why you didn't put irq
info into capability directly. Another reason is, in my understanding, it
choices ioctl to expose irq info becasue user must use ioctl to set irq, for
other capabilities which doesn't use device file, then some sysfs may be enough
for their own functions.

Thanks
Hao

  reply	other threads:[~2020-03-11  3:04 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-09 10:29 [PATCH 0/7] Add interrupt support to FPGA DFL drivers Xu Yilun
2020-03-09 10:29 ` [PATCH 1/7] fpga: dfl: parse interrupt info for feature devices on enumeration Xu Yilun
2020-03-10  2:26   ` Wu Hao
2020-03-10  9:25     ` Xu Yilun
2020-03-10 10:21       ` Wu Hao
2020-03-09 10:29 ` [PATCH 2/7] fpga: dfl: pci: add irq info for feature devices enumeration Xu Yilun
2020-03-10  2:46   ` Wu Hao
2020-03-10  9:41     ` Xu Yilun
2020-03-10 10:26       ` Wu Hao
2020-03-09 10:29 ` [PATCH 3/7] fpga: dfl: introduce interrupt trigger setting API Xu Yilun
2020-03-10 10:30   ` Wu Hao
2020-03-11  2:14     ` Xu Yilun
2020-03-09 10:29 ` [PATCH 4/7] fpga: dfl: afu: add interrupt support for error reporting Xu Yilun
2020-03-10 10:39   ` Wu Hao
2020-03-10 16:47     ` Xu Yilun
2020-03-11  2:43       ` Wu Hao [this message]
2020-03-11  6:35         ` Xu Yilun
2020-03-09 10:29 ` [PATCH 5/7] fpga: dfl: fme: add interrupt support for global " Xu Yilun
2020-03-09 10:29 ` [PATCH 6/7] fpga: dfl: afu: add user interrupt support Xu Yilun
2020-03-09 10:29 ` [PATCH 7/7] Documentation: fpga: dfl: add descriptions for interrupt related interfaces Xu Yilun

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200311024356.GA26202@hao-dev \
    --to=hao.wu@intel.com \
    --cc=linux-fpga@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luwei.kang@intel.com \
    --cc=mdf@kernel.org \
    --cc=yilun.xu@intel.com \
    --subject='Re: [PATCH 4/7] fpga: dfl: afu: add interrupt support for error reporting' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).