RE: [PATCH V4 10/12] misc: xilinx_sdfec: Add stats & status ioctls
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday 6 June 2019 15:12
> To: Dragan Cvetic
> Cc: a...@arndb.de; Michal Simek ;
> linux-arm-ker...@lists.infradead.org; robh...@kernel.org;
> mark.rutl...@arm.com; devicet...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Derek Kiernan
> Subject: Re: [PATCH V4 10/12] misc: xilinx_sdfec: Add stats & status ioctls
>
> On Sat, May 25, 2019 at 12:37:23PM +0100, Dragan Cvetic wrote:
> > SD-FEC statistic data are:
> > - count of data interface errors (isr_err_count)
> > - count of Correctable ECC errors (cecc_count)
> > - count of Uncorrectable ECC errors (uecc_count)
> >
> > Add support:
> > 1. clear stats ioctl callback which clears collected
> > statistic data,
> > 2. get stats ioctl callback which reads a collected
> > statistic data,
> > 3. set default configuration ioctl callback,
> > 4. start ioctl callback enables SD-FEC HW,
> > 5. stop ioctl callback disables SD-FEC HW.
> >
> > In a failed state driver enables the following ioctls:
> > - get status
> > - get statistics
> > - clear stats
> > - set default SD-FEC device configuration
> >
> > Tested-by: Santhosh Dyavanapally
> > Tested by: Punnaiah Choudary Kalluri
> > Tested-by: Derek Kiernan
> > Tested-by: Dragan Cvetic
> > Signed-off-by: Derek Kiernan
> > Signed-off-by: Dragan Cvetic
> > ---
> > drivers/misc/xilinx_sdfec.c | 121
> > +++
> > include/uapi/misc/xilinx_sdfec.h | 75
> > 2 files changed, 196 insertions(+)
> >
> > diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
> > index 544e746..6e04492 100644
> > --- a/drivers/misc/xilinx_sdfec.c
> > +++ b/drivers/misc/xilinx_sdfec.c
> > @@ -189,6 +189,7 @@ struct xsdfec_clks {
> > * @dev: pointer to device struct
> > * @state: State of the SDFEC device
> > * @config: Configuration of the SDFEC device
> > + * @intr_enabled: indicates IRQ enabled
> > * @state_updated: indicates State updated by interrupt handler
> > * @stats_updated: indicates Stats updated by interrupt handler
> > * @isr_err_count: Count of ISR errors
> > @@ -207,6 +208,7 @@ struct xsdfec_dev {
> > struct device *dev;
> > enum xsdfec_state state;
> > struct xsdfec_config config;
> > + bool intr_enabled;
> > bool state_updated;
> > bool stats_updated;
> > atomic_t isr_err_count;
> > @@ -290,6 +292,26 @@ static int xsdfec_dev_release(struct inode *iptr,
> > struct file *fptr)
> > return 0;
> > }
> >
> > +static int xsdfec_get_status(struct xsdfec_dev *xsdfec, void __user *arg)
> > +{
> > + struct xsdfec_status status;
> > + int err;
> > +
> > + status.fec_id = xsdfec->config.fec_id;
> > + spin_lock_irqsave(&xsdfec->irq_lock, xsdfec->flags);
> > + status.state = xsdfec->state;
> > + xsdfec->state_updated = false;
> > + spin_unlock_irqrestore(&xsdfec->irq_lock, xsdfec->flags);
> > + status.activity = (xsdfec_regread(xsdfec, XSDFEC_ACTIVE_ADDR) &
> > + XSDFEC_IS_ACTIVITY_SET);
> > +
> > + err = copy_to_user(arg, &status, sizeof(status));
> > + if (err)
> > + err = -EFAULT;
> > +
> > + return err;
> > +}
> > +
> > static int xsdfec_get_config(struct xsdfec_dev *xsdfec, void __user *arg)
> > {
> > int err;
> > @@ -850,6 +872,80 @@ static int xsdfec_cfg_axi_streams(struct xsdfec_dev
> > *xsdfec)
> > return 0;
> > }
> >
> > +static int xsdfec_start(struct xsdfec_dev *xsdfec)
> > +{
> > + u32 regread;
> > +
> > + regread = xsdfec_regread(xsdfec, XSDFEC_FEC_CODE_ADDR);
> > + regread &= 0x1;
> > + if (regread != xsdfec->config.code) {
> > + dev_dbg(xsdfec->dev,
> > + "%s SDFEC HW code does not match driver code, reg %d,
> > code %d",
> > + __func__, regread, xsdfec->config.code);
> > + return -EINVAL;
> > + }
> > +
> > + /* Set AXIS enable */
> > + xsdfec_regwrite(xsdfec, XSDFEC_AXIS_ENABLE_ADDR,
> > + XSDFEC_AXIS_ENABLE_MASK);
> > + /* Done */
> > + xsdfec->state = XSDFEC_STARTED;
> > + return 0;
> > +}
> > +
> > +static int xsdfec_stop(struct xsdfec_dev *xsdfec)
> > +{
> > + u32 regread;
> > +
&g
Re: [PATCH V4 10/12] misc: xilinx_sdfec: Add stats & status ioctls
On Sat, May 25, 2019 at 12:37:23PM +0100, Dragan Cvetic wrote:
> SD-FEC statistic data are:
> - count of data interface errors (isr_err_count)
> - count of Correctable ECC errors (cecc_count)
> - count of Uncorrectable ECC errors (uecc_count)
>
> Add support:
> 1. clear stats ioctl callback which clears collected
> statistic data,
> 2. get stats ioctl callback which reads a collected
> statistic data,
> 3. set default configuration ioctl callback,
> 4. start ioctl callback enables SD-FEC HW,
> 5. stop ioctl callback disables SD-FEC HW.
>
> In a failed state driver enables the following ioctls:
> - get status
> - get statistics
> - clear stats
> - set default SD-FEC device configuration
>
> Tested-by: Santhosh Dyavanapally
> Tested by: Punnaiah Choudary Kalluri
> Tested-by: Derek Kiernan
> Tested-by: Dragan Cvetic
> Signed-off-by: Derek Kiernan
> Signed-off-by: Dragan Cvetic
> ---
> drivers/misc/xilinx_sdfec.c | 121
> +++
> include/uapi/misc/xilinx_sdfec.h | 75
> 2 files changed, 196 insertions(+)
>
> diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
> index 544e746..6e04492 100644
> --- a/drivers/misc/xilinx_sdfec.c
> +++ b/drivers/misc/xilinx_sdfec.c
> @@ -189,6 +189,7 @@ struct xsdfec_clks {
> * @dev: pointer to device struct
> * @state: State of the SDFEC device
> * @config: Configuration of the SDFEC device
> + * @intr_enabled: indicates IRQ enabled
> * @state_updated: indicates State updated by interrupt handler
> * @stats_updated: indicates Stats updated by interrupt handler
> * @isr_err_count: Count of ISR errors
> @@ -207,6 +208,7 @@ struct xsdfec_dev {
> struct device *dev;
> enum xsdfec_state state;
> struct xsdfec_config config;
> + bool intr_enabled;
> bool state_updated;
> bool stats_updated;
> atomic_t isr_err_count;
> @@ -290,6 +292,26 @@ static int xsdfec_dev_release(struct inode *iptr, struct
> file *fptr)
> return 0;
> }
>
> +static int xsdfec_get_status(struct xsdfec_dev *xsdfec, void __user *arg)
> +{
> + struct xsdfec_status status;
> + int err;
> +
> + status.fec_id = xsdfec->config.fec_id;
> + spin_lock_irqsave(&xsdfec->irq_lock, xsdfec->flags);
> + status.state = xsdfec->state;
> + xsdfec->state_updated = false;
> + spin_unlock_irqrestore(&xsdfec->irq_lock, xsdfec->flags);
> + status.activity = (xsdfec_regread(xsdfec, XSDFEC_ACTIVE_ADDR) &
> +XSDFEC_IS_ACTIVITY_SET);
> +
> + err = copy_to_user(arg, &status, sizeof(status));
> + if (err)
> + err = -EFAULT;
> +
> + return err;
> +}
> +
> static int xsdfec_get_config(struct xsdfec_dev *xsdfec, void __user *arg)
> {
> int err;
> @@ -850,6 +872,80 @@ static int xsdfec_cfg_axi_streams(struct xsdfec_dev
> *xsdfec)
> return 0;
> }
>
> +static int xsdfec_start(struct xsdfec_dev *xsdfec)
> +{
> + u32 regread;
> +
> + regread = xsdfec_regread(xsdfec, XSDFEC_FEC_CODE_ADDR);
> + regread &= 0x1;
> + if (regread != xsdfec->config.code) {
> + dev_dbg(xsdfec->dev,
> + "%s SDFEC HW code does not match driver code, reg %d,
> code %d",
> + __func__, regread, xsdfec->config.code);
> + return -EINVAL;
> + }
> +
> + /* Set AXIS enable */
> + xsdfec_regwrite(xsdfec, XSDFEC_AXIS_ENABLE_ADDR,
> + XSDFEC_AXIS_ENABLE_MASK);
> + /* Done */
> + xsdfec->state = XSDFEC_STARTED;
> + return 0;
> +}
> +
> +static int xsdfec_stop(struct xsdfec_dev *xsdfec)
> +{
> + u32 regread;
> +
> + if (xsdfec->state != XSDFEC_STARTED)
> + dev_dbg(xsdfec->dev, "Device not started correctly");
> + /* Disable AXIS_ENABLE Input interfaces only */
> + regread = xsdfec_regread(xsdfec, XSDFEC_AXIS_ENABLE_ADDR);
> + regread &= (~XSDFEC_AXIS_IN_ENABLE_MASK);
> + xsdfec_regwrite(xsdfec, XSDFEC_AXIS_ENABLE_ADDR, regread);
> + /* Stop */
> + xsdfec->state = XSDFEC_STOPPED;
> + return 0;
> +}
> +
> +static int xsdfec_clear_stats(struct xsdfec_dev *xsdfec)
> +{
> + atomic_set(&xsdfec->isr_err_count, 0);
> + atomic_set(&xsdfec->uecc_count, 0);
> + atomic_set(&xsdfec->cecc_count, 0);
Atomics for counters? Are you sure? Don't we have some sort of sane
counter api these days for stuff like this instead of abusing atomic
variables? What does the networking people use? How often/fast do
these change that you need to synchronize things?
> +
> + return 0;
> +}
> +
> +static int xsdfec_get_stats(struct xsdfec_dev *xsdfec, void __user *arg)
> +{
> + int err;
> + struct xsdfec_stats user_stats;
> +
> + spin_lock_irqsave(&xsdfec->irq_lock, xsdfec->flags);
> + user_stats.isr_err_count = atomic_read(&xsdfec->isr_err_count);
> + user_stats.cecc_count = atomic_read(&xsdfec->cecc_count);
> + user_stats.uecc_count = atomic_read(&xsdfec->uec
[PATCH V4 10/12] misc: xilinx_sdfec: Add stats & status ioctls
SD-FEC statistic data are:
- count of data interface errors (isr_err_count)
- count of Correctable ECC errors (cecc_count)
- count of Uncorrectable ECC errors (uecc_count)
Add support:
1. clear stats ioctl callback which clears collected
statistic data,
2. get stats ioctl callback which reads a collected
statistic data,
3. set default configuration ioctl callback,
4. start ioctl callback enables SD-FEC HW,
5. stop ioctl callback disables SD-FEC HW.
In a failed state driver enables the following ioctls:
- get status
- get statistics
- clear stats
- set default SD-FEC device configuration
Tested-by: Santhosh Dyavanapally
Tested by: Punnaiah Choudary Kalluri
Tested-by: Derek Kiernan
Tested-by: Dragan Cvetic
Signed-off-by: Derek Kiernan
Signed-off-by: Dragan Cvetic
---
drivers/misc/xilinx_sdfec.c | 121 +++
include/uapi/misc/xilinx_sdfec.h | 75
2 files changed, 196 insertions(+)
diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
index 544e746..6e04492 100644
--- a/drivers/misc/xilinx_sdfec.c
+++ b/drivers/misc/xilinx_sdfec.c
@@ -189,6 +189,7 @@ struct xsdfec_clks {
* @dev: pointer to device struct
* @state: State of the SDFEC device
* @config: Configuration of the SDFEC device
+ * @intr_enabled: indicates IRQ enabled
* @state_updated: indicates State updated by interrupt handler
* @stats_updated: indicates Stats updated by interrupt handler
* @isr_err_count: Count of ISR errors
@@ -207,6 +208,7 @@ struct xsdfec_dev {
struct device *dev;
enum xsdfec_state state;
struct xsdfec_config config;
+ bool intr_enabled;
bool state_updated;
bool stats_updated;
atomic_t isr_err_count;
@@ -290,6 +292,26 @@ static int xsdfec_dev_release(struct inode *iptr, struct
file *fptr)
return 0;
}
+static int xsdfec_get_status(struct xsdfec_dev *xsdfec, void __user *arg)
+{
+ struct xsdfec_status status;
+ int err;
+
+ status.fec_id = xsdfec->config.fec_id;
+ spin_lock_irqsave(&xsdfec->irq_lock, xsdfec->flags);
+ status.state = xsdfec->state;
+ xsdfec->state_updated = false;
+ spin_unlock_irqrestore(&xsdfec->irq_lock, xsdfec->flags);
+ status.activity = (xsdfec_regread(xsdfec, XSDFEC_ACTIVE_ADDR) &
+ XSDFEC_IS_ACTIVITY_SET);
+
+ err = copy_to_user(arg, &status, sizeof(status));
+ if (err)
+ err = -EFAULT;
+
+ return err;
+}
+
static int xsdfec_get_config(struct xsdfec_dev *xsdfec, void __user *arg)
{
int err;
@@ -850,6 +872,80 @@ static int xsdfec_cfg_axi_streams(struct xsdfec_dev
*xsdfec)
return 0;
}
+static int xsdfec_start(struct xsdfec_dev *xsdfec)
+{
+ u32 regread;
+
+ regread = xsdfec_regread(xsdfec, XSDFEC_FEC_CODE_ADDR);
+ regread &= 0x1;
+ if (regread != xsdfec->config.code) {
+ dev_dbg(xsdfec->dev,
+ "%s SDFEC HW code does not match driver code, reg %d,
code %d",
+ __func__, regread, xsdfec->config.code);
+ return -EINVAL;
+ }
+
+ /* Set AXIS enable */
+ xsdfec_regwrite(xsdfec, XSDFEC_AXIS_ENABLE_ADDR,
+ XSDFEC_AXIS_ENABLE_MASK);
+ /* Done */
+ xsdfec->state = XSDFEC_STARTED;
+ return 0;
+}
+
+static int xsdfec_stop(struct xsdfec_dev *xsdfec)
+{
+ u32 regread;
+
+ if (xsdfec->state != XSDFEC_STARTED)
+ dev_dbg(xsdfec->dev, "Device not started correctly");
+ /* Disable AXIS_ENABLE Input interfaces only */
+ regread = xsdfec_regread(xsdfec, XSDFEC_AXIS_ENABLE_ADDR);
+ regread &= (~XSDFEC_AXIS_IN_ENABLE_MASK);
+ xsdfec_regwrite(xsdfec, XSDFEC_AXIS_ENABLE_ADDR, regread);
+ /* Stop */
+ xsdfec->state = XSDFEC_STOPPED;
+ return 0;
+}
+
+static int xsdfec_clear_stats(struct xsdfec_dev *xsdfec)
+{
+ atomic_set(&xsdfec->isr_err_count, 0);
+ atomic_set(&xsdfec->uecc_count, 0);
+ atomic_set(&xsdfec->cecc_count, 0);
+
+ return 0;
+}
+
+static int xsdfec_get_stats(struct xsdfec_dev *xsdfec, void __user *arg)
+{
+ int err;
+ struct xsdfec_stats user_stats;
+
+ spin_lock_irqsave(&xsdfec->irq_lock, xsdfec->flags);
+ user_stats.isr_err_count = atomic_read(&xsdfec->isr_err_count);
+ user_stats.cecc_count = atomic_read(&xsdfec->cecc_count);
+ user_stats.uecc_count = atomic_read(&xsdfec->uecc_count);
+ xsdfec->stats_updated = false;
+ spin_unlock_irqrestore(&xsdfec->irq_lock, xsdfec->flags);
+
+ err = copy_to_user(arg, &user_stats, sizeof(user_stats));
+ if (err)
+ err = -EFAULT;
+
+ return err;
+}
+
+static int xsdfec_set_default_config(struct xsdfec_dev *xsdfec)
+{
+ /* Ensure registers are aligned with core configuration */
+ xsdfec_regwrite(xsdfec, XSDFEC_FEC_CODE_ADDR, xsdfec->config.code);
+ xsdfec_cfg
