On Wed, Mar 11, 2020 at 10:43:56AM +0800, Wu Hao wrote:
> 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@xxxxxxxxx>
> > > > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> > > > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> > > > ---
> > > > 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 for clarify this, I'll remove the flags & capability fields
Yilun
>
> Thanks
> Hao
