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

[Greg KH]

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 :)

> +	  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?

> + */
> +
> +#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?

> +
> +/**
> + * 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...

> + * @miscdev: Misc device handle
> + * @irq_lock: Driver spinlock

locks what?  The irq?

> + *
> + * 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?

> +	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?

> +
> +#define NAMEBUF_SIZE ((size_t)32)

what is this for?

> +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?

> +		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?

> +		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.

> +	atomic_inc(&xsdfec_ndevs);
> +	return 0;
> +
> +	/* Failure cleanup */
> +err_xsdfec_dev:
> +	return err;

You cleaned up nothing, not good at all :(

> +}
> +
> +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?

> +
> +	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.

> +}
> +
> +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?

> +
> +/**
> + * 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?

Do I need to really review the rest of this series?

thanks,

greg k-h