lkml.org 
[lkml]   [2019]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V4 02/12] misc: xilinx-sdfec: add core driver
Date


> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday 6 June 2019 14:25
> To: Dragan Cvetic <draganc@xilinx.com>
> Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org;
> mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com>
> 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@xilinx.com>
> > Signed-off-by: Derek Kiernan <derek.kiernan@xilinx.com>
> > Signed-off-by: Dragan Cvetic <dragan.cvetic@xilinx.com>
> > ---
> > 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

\
 
 \ /
  Last update: 2019-06-06 20:17    [W:0.075 / U:3.652 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site