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