> -----Original Message-----
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday 6 June 2019 15:12
> To: Dragan Cvetic <drag...@xilinx.com>
> Cc: a...@arndb.de; Michal Simek <mich...@xilinx.com>;
> linux-arm-ker...@lists.infradead.org; robh...@kernel.org;
> mark.rutl...@arm.com; devicet...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Derek Kiernan <dkier...@xilinx.com>
> 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 <sdyav...@xilinx.com>
> > Tested by: Punnaiah Choudary Kalluri <punn...@xilinx.com>
> > Tested-by: Derek Kiernan <derek.kier...@xilinx.com>
> > Tested-by: Dragan Cvetic <dragan.cve...@xilinx.com>
> > Signed-off-by: Derek Kiernan <derek.kier...@xilinx.com>
> > Signed-off-by: Dragan Cvetic <dragan.cve...@xilinx.com>
> > ---
> > 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?
Accepted.
No need to have them atomic. They are lock protected already.
>
> > +
> > + 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);
>
> Wait, you just grabbed a lock, and then read atomic variables, why? Why
> do these need to be atomic variables if you are already locking around
> them? Unless you want to be "extra sure" they are safe? :)
Accepted.
Absolutely no need for atomic variables.
>
> Please fix up.
>
> thanks,
>
> greg k-h
