RE: [PATCH V4 02/12] misc: xilinx-sdfec: add core driver

[Dragan Cvetic]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx]
> Sent: Thursday 6 June 2019 14:25
> To: Dragan Cvetic <draganc@xxxxxxxxxx>
> Cc: arnd@xxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Derek Kiernan <dkiernan@xxxxxxxxxx>
> Subject: Re: [PATCH V4 02/12] misc: xilinx-sdfec: add core driver
> 
> On Sat, May 25, 2019 at 12:37:15PM +0100, Dragan Cvetic wrote:
> > Implements an platform driver that matches with xlnx,
> > sd-fec-1.1 device tree node and registers as a character
> > device, including:
> > - SD-FEC driver binds to sdfec DT node.
> > - creates and initialise an initial driver dev structure.
> > - add the driver in Linux build and Kconfig.
> >
> > Tested-by: Dragan Cvetic <dragan.cvetic@xxxxxxxxxx>
> > Signed-off-by: Derek Kiernan <derek.kiernan@xxxxxxxxxx>
> > Signed-off-by: Dragan Cvetic <dragan.cvetic@xxxxxxxxxx>
> > ---
> >  drivers/misc/Kconfig             |  12 ++++
> >  drivers/misc/Makefile            |   1 +
> >  drivers/misc/xilinx_sdfec.c      | 136 +++++++++++++++++++++++++++++++++++++++
> >  include/uapi/misc/xilinx_sdfec.h |  44 +++++++++++++
> >  4 files changed, 193 insertions(+)
> >  create mode 100644 drivers/misc/xilinx_sdfec.c
> >  create mode 100644 include/uapi/misc/xilinx_sdfec.h
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index 6a0365b..15d93a7 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -480,6 +480,18 @@ config PCI_ENDPOINT_TEST
> >             Enable this configuration option to enable the host side test driver
> >             for PCI Endpoint.
> >
> > +config XILINX_SDFEC
> > +	tristate "Xilinx SDFEC 16"
> > +	help
> 
> No dependancies at all?  Nice!  Let's see what 0-day has to say about it :)

Yes, no dependencies.

> 
> > +	  This option enables support for the Xilinx SDFEC (Soft Decision
> > +	  Forward Error Correction) driver. This enables a char driver
> > +	  for the SDFEC.
> > +
> > +	  You may select this driver if your design instantiates the
> > +	  SDFEC(16nm) hardened block. To compile this as a module choose M.
> > +
> > +	  If unsure, say N.
> > +
> >  config MISC_RTSX
> >  	tristate
> >  	default MISC_RTSX_PCI || MISC_RTSX_USB
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > index b9affcd..29fd1d7 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -49,6 +49,7 @@ obj-$(CONFIG_VMWARE_VMCI)	+= vmw_vmci/
> >  obj-$(CONFIG_LATTICE_ECP3_CONFIG)	+= lattice-ecp3-config.o
> >  obj-$(CONFIG_SRAM)		+= sram.o
> >  obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.o
> > +obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
> >  obj-y				+= mic/
> >  obj-$(CONFIG_GENWQE)		+= genwqe/
> >  obj-$(CONFIG_ECHO)		+= echo/
> > diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
> > new file mode 100644
> > index 0000000..c437f78
> > --- /dev/null
> > +++ b/drivers/misc/xilinx_sdfec.c
> > @@ -0,0 +1,136 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Xilinx SDFEC
> > + *
> > + * Copyright (C) 2019 Xilinx, Inc.
> > + *
> > + * Description:
> > + * This driver is developed for SDFEC16 (Soft Decision FEC 16nm)
> > + * IP. It exposes a char device interface in sysfs and supports file
> > + * operations like  open(), close() and ioctl().
> 
> There are no "char device interfaces in sysfs".  What are you trying to
> say here?
> 

Accepted. 
Will rename to "char device which supports.."


> > + */
> > +
> > +#include <linux/miscdevice.h>
> > +#include <linux/io.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/poll.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk.h>
> > +
> > +#include <uapi/misc/xilinx_sdfec.h>
> > +
> > +static atomic_t xsdfec_ndevs = ATOMIC_INIT(0);
> 
> why an atomic variable?  What are you using this for?  Why not an idr?

This variable is used to define a device ID number, "0,1,2,3..."
After all the changes it's meaningless.
Accepted.
Will be removed.

> > +
> > +/**
> > + * struct xsdfec_dev - Driver data for SDFEC
> > + * @regs: device physical base address
> > + * @dev: pointer to device struct
> > + * @config: Configuration of the SDFEC device
> > + * @open_count: Count of char device being opened
> 
> Ugh ugh ugh.  Don't try to count the number of times open is called.
> It's pointless and almost always wrong.  And it doesn't stop anyone from
> really accessing the device "twice".  If they do stupid things like
> that, they deserve the errors that it will cause...
> 

Accepted. My mistake.
Will remove this comment.

> > + * @miscdev: Misc device handle
> > + * @irq_lock: Driver spinlock
> 
> locks what?  The irq?
> 

It locks the statistical and state data related to errors in FEC.
Accepted
Will rename to "error_data_lock"

> > + *
> > + * This structure contains necessary state for SDFEC driver to operate
> > + */
> > +struct xsdfec_dev {
> > +	void __iomem *regs;
> > +	struct device *dev;
> 
> Is this the parent pointer?  Or something else?

Yes it is.

> 
> > +	struct xsdfec_config config;
> > +	atomic_t open_count;
> > +	struct miscdevice miscdev;
> > +	/* Spinlock to protect state_updated and stats_updated */
> > +	spinlock_t irq_lock;
> > +};
> > +
> > +static const struct file_operations xsdfec_fops = {
> > +	.owner = THIS_MODULE,
> > +};
> 
> empty fops?
> 

Yes, in this patch.

> > +
> > +#define NAMEBUF_SIZE ((size_t)32)
> 
> what is this for?

This is size of a char buffer used to store a device name.

> 
> > +static int xsdfec_probe(struct platform_device *pdev)
> > +{
> > +	struct xsdfec_dev *xsdfec;
> > +	struct device *dev;
> > +	struct resource *res;
> > +	int err;
> > +	char buf[NAMEBUF_SIZE];
> > +
> > +	xsdfec = devm_kzalloc(&pdev->dev, sizeof(*xsdfec), GFP_KERNEL);
> > +	if (!xsdfec)
> > +		return -ENOMEM;
> > +
> > +	xsdfec->dev = &pdev->dev;
> > +	xsdfec->config.fec_id = atomic_read(&xsdfec_ndevs);
> > +	spin_lock_init(&xsdfec->irq_lock);
> > +
> > +	dev = xsdfec->dev;
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	xsdfec->regs = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(xsdfec->regs)) {
> > +		dev_err(dev, "Unable to map resource");
> 
> doesn't this call already print an error?

Accepted. Will be removed.

> 
> > +		err = PTR_ERR(xsdfec->regs);
> > +		goto err_xsdfec_dev;
> > +	}
> > +
> > +	/* Save driver private data */
> > +	platform_set_drvdata(pdev, xsdfec);
> > +
> > +	snprintf(buf, NAMEBUF_SIZE, "xsdfec%d", xsdfec->config.fec_id);
> > +	xsdfec->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +	xsdfec->miscdev.name = buf;
> > +	xsdfec->miscdev.fops = &xsdfec_fops;
> > +	xsdfec->miscdev.parent = dev;
> > +	err = misc_register(&xsdfec->miscdev);
> > +	if (err) {
> > +		dev_err(dev, "Unable to register device");
> 
> Print the error number that was returned to you?

Accepted. 
Will print the error number.

> 
> > +		goto err_xsdfec_dev;
> > +	}
> > +
> > +	atomic_set(&xsdfec->open_count, 1);
> > +	dev_info(dev, "XSDFEC%d Probe Successful", xsdfec->config.fec_id);
> 
> No need to be noisy when things work correctly, just keep on going.

Accepted.
Will be removed.

> 
> > +	atomic_inc(&xsdfec_ndevs);
> > +	return 0;
> > +
> > +	/* Failure cleanup */
> > +err_xsdfec_dev:
> > +	return err;
> 
> You cleaned up nothing, not good at all :(

Accepted.
Will remove this and return immediately.

> 
> > +}
> > +
> > +static int xsdfec_remove(struct platform_device *pdev)
> > +{
> > +	struct xsdfec_dev *xsdfec;
> > +
> > +	xsdfec = platform_get_drvdata(pdev);
> > +	if (!xsdfec)
> > +		return -ENODEV;
> 
> How can this be null?

Accepted.
Will remove if statement.

> 
> > +
> > +	misc_deregister(&xsdfec->miscdev);
> > +	atomic_dec(&xsdfec_ndevs);
> > +	return 0;
> 
> You free nothing?
> 
> You are leaking resources like crazy here, this is not ok at all.

The managed resources are used.
Anyway, I'll test for memory leak and search for the answer.

> 
> > +}
> > +
> > +static const struct of_device_id xsdfec_of_match[] = {
> > +	{
> > +		.compatible = "xlnx,sd-fec-1.1",
> > +	},
> > +	{ /* end of table */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, xsdfec_of_match);
> > +
> > +static struct platform_driver xsdfec_driver = {
> > +	.driver = {
> > +		.name = "xilinx-sdfec",
> > +		.of_match_table = xsdfec_of_match,
> > +	},
> > +	.probe = xsdfec_probe,
> > +	.remove =  xsdfec_remove,
> > +};
> > +
> > +module_platform_driver(xsdfec_driver);
> > +
> > +MODULE_AUTHOR("Xilinx, Inc");
> > +MODULE_DESCRIPTION("Xilinx SD-FEC16 Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/uapi/misc/xilinx_sdfec.h b/include/uapi/misc/xilinx_sdfec.h
> > new file mode 100644
> > index 0000000..1b8a63f
> > --- /dev/null
> > +++ b/include/uapi/misc/xilinx_sdfec.h
> > @@ -0,0 +1,44 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> > +/*
> > + * Xilinx SD-FEC
> > + *
> > + * Copyright (C) 2016 - 2017 Xilinx, Inc.
> > + *
> > + * Description:
> > + * This driver is developed for SDFEC16 IP. It provides a char device
> > + * in sysfs and supports file operations like open(), close() and ioctl().
> > + */
> > +#ifndef __XILINX_SDFEC_H__
> > +#define __XILINX_SDFEC_H__
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * enum xsdfec_state - State.
> > + * @XSDFEC_INIT: Driver is initialized.
> > + * @XSDFEC_STARTED: Driver is started.
> > + * @XSDFEC_STOPPED: Driver is stopped.
> > + * @XSDFEC_NEEDS_RESET: Driver needs to be reset.
> > + * @XSDFEC_PL_RECONFIGURE: Programmable Logic needs to be recofigured.
> > + *
> > + * This enum is used to indicate the state of the driver.
> > + */
> > +enum xsdfec_state {
> > +	XSDFEC_INIT = 0,
> > +	XSDFEC_STARTED,
> > +	XSDFEC_STOPPED,
> > +	XSDFEC_NEEDS_RESET,
> > +	XSDFEC_PL_RECONFIGURE,
> > +};
> 
> This is not used in this patch, why have it?

Accepted.
Will be removed.

> 
> > +
> > +/**
> > + * struct xsdfec_config - Configuration of SD-FEC core.
> > + * @fec_id: ID of SD-FEC instance. ID is limited to the number of active
> > + *          SD-FEC's in the FPGA and is related to the driver instance
> > + *          Minor number.
> > + */
> > +struct xsdfec_config {
> > +	__s32 fec_id;
> 
> Why signed?
> 
> And you are NOT tieing this to the minor number at all, don't lie in the
> comment, that is only going to cause you major problems.
> 
> Why does userspace care about this structure?
> 

Accepted earlier.
The struct will be removed


> Do I need to really review the rest of this series?
> 
> thanks,
> 
> greg k-h