Hi Greg.
> -----Original Message-----
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Friday, October 20, 2017 2:55 PM
> To: Oleksandr Shamray <oleksan...@mellanox.com>
> Cc: a...@arndb.de; linux-kernel@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devicet...@vger.kernel.org;
> open...@lists.ozlabs.org; j...@jms.id.au; j...@resnulli.us;
> tklau...@distanz.ch; linux-ser...@vger.kernel.org; m...@shout.net; Vadim
> Pasternak <vad...@mellanox.com>; system-sw-low-level <system-sw-low-
> le...@mellanox.com>; robh...@kernel.org; openocd-devel-
> ow...@lists.sourceforge.net; linux-...@vger.kernel.org;
> da...@davemloft.net; mche...@kernel.org; Jiri Pirko <j...@mellanox.com>
> Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
>
> On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:
> > +struct jtag {
> > + struct device *dev;
> > + struct cdev cdev;
>
> Why are you using a cdev here and not just a normal misc device?
What the benefits to use misc instead of cdev?
> I forgot if this is what you were doing before, sorry...
>
> > + int id;
> > + atomic_t open;
>
> Why do you need this?
This counter used to avoid open at the same time by 2 or more users.
>
> > + const struct jtag_ops *ops;
> > + unsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);
>
> Huh? Why is this needed to be dma aligned? Why not just use the private
> pointer in struct device?
>
It is critical?
> > +};
> > +
> > +static dev_t jtag_devt;
> > +static DEFINE_IDA(jtag_ida);
> > +
> > +void *jtag_priv(struct jtag *jtag)
> > +{
> > + return jtag->priv;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_priv);
> > +
> > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {
> > + unsigned long size;
> > + void *kdata;
> > +
> > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > + kdata = memdup_user(u64_to_user_ptr(udata), size);
>
> You only use this once, why not just open-code it?
I think it makes code more understandable.
>
> > +
> > + return kdata;
> > +}
> > +
> > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,
> > + unsigned long bit_size)
> > +{
> > + unsigned long size;
> > +
> > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > +
> > + return copy_to_user(u64_to_user_ptr(udata), (void *)(kdata), size);
>
> Same here, making this a separate function seems odd.
Same, I think it makes code more understandable.
>
> > +}
> > +
> > +static struct class jtag_class = {
> > + .name = "jtag",
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int jtag_run_test_idle_op(struct jtag *jtag,
> > + struct jtag_run_test_idle *idle) {
> > + if (jtag->ops->idle)
> > + return jtag->ops->idle(jtag, idle);
> > + else
> > + return -EOPNOTSUPP;
>
> Why is this a function? Why not just do this work in the ioctl?
Agree. I will move it just to ioctl function body.
>
> > +}
> > +
> > +static int jtag_xfer_op(struct jtag *jtag, struct jtag_xfer *xfer, u8
> > +*data) {
> > + if (jtag->ops->xfer)
> > + return jtag->ops->xfer(jtag, xfer, data);
> > + else
> > + return -EOPNOTSUPP;
>
> Same here, doesn't need to be a function.
Agree.
>
> > +}
> > +
> > +static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned
> > +long arg) {
> > + struct jtag *jtag = file->private_data;
> > + struct jtag_run_test_idle idle;
> > + struct jtag_xfer xfer;
> > + u8 *xfer_data;
> > + u32 value;
> > + int err;
> > +
> > + switch (cmd) {
> > + case JTAG_GIOCFREQ:
> > + if (jtag->ops->freq_get)
> > + err = jtag->ops->freq_get(jtag, &value);
> > + else
> > + err = -EOPNOTSUPP;
> > + if (err)
> > + break;
> > +
> > + err = put_user(value, (__u32 *)arg);
> > + break;
>
> Are you sure the return value of put_user is correct here? Shouldn't you
> return
> -EFAULT if err is not 0?
Yes, thanks for point of this.
>
> > +
> > + case JTAG_SIOCFREQ:
> > + err = get_user(value, (__u32 *)arg);
> > +
>
> why a blank line?
Will remove
>
> > + if (value == 0)
> > + err = -EINVAL;
>
> Check err first.
Ok.
>
> > + if (err)
> > + break;
>
> And set it properly, again err should be -EFAULT, right?
Right 😊0
>
> > +
> > + if (jtag->ops->freq_set)
> > + err = jtag->ops->freq_set(jtag, value);
> > + else
> > + err = -EOPNOTSUPP;
> > + break;
> > +
> > + case JTAG_IOCRUNTEST:
> > + if (copy_from_user(&idle, (void *)arg,
> > + sizeof(struct jtag_run_test_idle)))
> > + return -ENOMEM;
> > + err = jtag_run_test_idle_op(jtag, &idle);
>
> Who validates the structure fields? Is that up to the individual jtag
> driver? Why
> not do it in the core correctly so that it only has to be done in one place
> and you
> do not have to audit every individual driver?
Input parameters validated by jtag platform driver. I think it not critical.
>
> > + break;
> > +
> > + case JTAG_IOCXFER:
> > + if (copy_from_user(&xfer, (void *)arg,
> > + sizeof(struct jtag_xfer)))
> > + return -EFAULT;
> > +
> > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> > + return -EFAULT;
> > +
> > + xfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);
> > + if (!xfer_data)
> > + return -ENOMEM;
>
> Are you sure that's the correct error value?
I think yes, but what you suggest?
>
> > +
> > + err = jtag_xfer_op(jtag, &xfer, xfer_data);
> > + if (jtag_copy_to_user(xfer.tdio, xfer_data, xfer.length)) {
> > + kfree(xfer_data);
> > + return -EFAULT;
> > + }
> > +
> > + kfree(xfer_data);
> > + if (copy_to_user((void *)arg, &xfer, sizeof(struct jtag_xfer)))
> > + return -EFAULT;
> > + break;
> > +
> > + case JTAG_GIOCSTATUS:
> > + if (jtag->ops->status_get)
> > + err = jtag->ops->status_get(jtag, &value);
> > + else
> > + err = -EOPNOTSUPP;
> > + if (err)
> > + break;
> > +
> > + err = put_user(value, (__u32 *)arg);
>
> Same put_user err issue here.
Yes.
>
> > + break;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > + return err;
> > +}
> > +
> > +#ifdef CONFIG_COMPAT
> > +static long jtag_ioctl_compat(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return jtag_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); }
> > +#endif
>
> Why do you need a compat callback for a new ioctl? Create your structures
> properly and this should not be needed, right?
I does this because Arnd (arndbergm...@gmail.com) writed in review (On Wed, Aug
2, 2017 at 3:18 PM)
[..]
> + .unlocked_ioctl = jtag_ioctl,
> + .open = jtag_open,
> + .release = jtag_release,
> +};
add a compat_ioctl pointer here, after ensuring that all ioctl commands are
compatible between 32-bit and 64-bit user space.
[..]
>
> > +
> > +static int jtag_open(struct inode *inode, struct file *file) {
> > + struct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);
> > +
> > + if (atomic_read(&jtag->open)) {
> > + dev_info(NULL, "jtag already opened\n");
> > + return -EBUSY;
>
> Why do you care if multiple opens can happen?
Jtag HW not support to using with multiple requests from different users. So we
prohibit this.
>
> > + }
> > +
> > + atomic_inc(&jtag->open);
>
> Oh look, you just raced with two different people opening at the same time :(
>
> An atomic value is never a replacement for a lock, sorry.
>
> > + file->private_data = jtag;
> > + return 0;
> > +}
> > +
> > +static int jtag_release(struct inode *inode, struct file *file) {
> > + struct jtag *jtag = file->private_data;
> > +
> > + atomic_dec(&jtag->open);
>
> Again, not needed.
>
> > + return 0;
> > +}
> > +
> > +static const struct file_operations jtag_fops = {
> > + .owner = THIS_MODULE,
> > + .open = jtag_open,
> > + .release = jtag_release,
> > + .llseek = noop_llseek,
> > + .unlocked_ioctl = jtag_ioctl,
> > +#ifdef CONFIG_COMPAT
> > + .compat_ioctl = jtag_ioctl_compat,
> > +#endif
> > +};
> > +
> > +struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops)
> > +{
> > + struct jtag *jtag;
> > +
> > + jtag = kzalloc(sizeof(*jtag) + round_up(priv_size,
> ARCH_DMA_MINALIGN),
> > + GFP_KERNEL);
> > + if (!jtag)
> > + return NULL;
> > +
> > + jtag->ops = ops;
> > + return jtag;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_alloc);
>
> register should allocate and register the device, why pass a structure back
> that
> the caller then has to do something else with to use it?
>
Before registration we need to initialize JTAG device HW registers and fill
private data with HW specific parameters.
And is I see in other Linux drivers it is a common way to separate allocation
device and register it
> > +
> > +void jtag_free(struct jtag *jtag)
> > +{
> > + kfree(jtag);
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_free);
> > +
> > +int jtag_register(struct jtag *jtag)
> > +{
> > + int id;
> > + int err;
> > +
> > + id = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);
> > + if (id < 0)
> > + return id;
> > +
> > + jtag->id = id;
> > + cdev_init(&jtag->cdev, &jtag_fops);
> > + jtag->cdev.owner = THIS_MODULE;
> > + err = cdev_add(&jtag->cdev, MKDEV(MAJOR(jtag_devt), jtag->id), 1);
> > + if (err)
> > + goto err_cdev;
>
> misc device would be so much simpler and easier here :(
>
>
> > +
> > + /* Register this jtag device with the driver core */
> > + jtag->dev = device_create(&jtag_class, NULL,
> MKDEV(MAJOR(jtag_devt),
> > + jtag->id),
> > + NULL, "jtag%d", jtag->id);
> > + if (!jtag->dev)
> > + goto err_device_create;
> > +
> > + atomic_set(&jtag->open, 0);
> > + dev_set_drvdata(jtag->dev, jtag);
> > + return err;
> > +
> > +err_device_create:
> > + cdev_del(&jtag->cdev);
> > +err_cdev:
> > + ida_simple_remove(&jtag_ida, id);
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_register);
> > +
> > +void jtag_unregister(struct jtag *jtag) {
> > + struct device *dev = jtag->dev;
> > +
> > + cdev_del(&jtag->cdev);
> > + device_unregister(dev);
> > + ida_simple_remove(&jtag_ida, jtag->id); }
> > +EXPORT_SYMBOL_GPL(jtag_unregister);
> > +
> > +static int __init jtag_init(void)
> > +{
> > + int err;
> > +
> > + err = alloc_chrdev_region(&jtag_devt, JTAG_FIRST_MINOR,
> > + JTAG_MAX_DEVICES, "jtag");
> > + if (err)
> > + return err;
> > +
> > + err = class_register(&jtag_class);
> > + if (err)
> > + unregister_chrdev_region(jtag_devt, JTAG_MAX_DEVICES);
> > +
> > + return err;
> > +}
> > +
> > +static void __exit jtag_exit(void)
> > +{
> > + class_unregister(&jtag_class);
> > + unregister_chrdev_region(jtag_devt, JTAG_MAX_DEVICES);
>
> Your idr leaked memory :(
>
Yes I will add ida_destroy() here.
>
>
> > +}
> > +
> > +module_init(jtag_init);
> > +module_exit(jtag_exit);
> > +
> > +MODULE_AUTHOR("Oleksandr Shamray <oleksan...@mellanox.com>");
> > +MODULE_DESCRIPTION("Generic jtag support"); MODULE_LICENSE("GPL
> v2");
> > diff --git a/include/linux/jtag.h b/include/linux/jtag.h new file mode
> > 100644 index 0000000..c016fed
> > --- /dev/null
> > +++ b/include/linux/jtag.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * drivers/jtag/jtag.c
> > + *
> > + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.
> > + * Copyright (c) 2017 Oleksandr Shamray <oleksan...@mellanox.com>
> > + *
> > + * Released under the GPLv2 only.
> > + * SPDX-License-Identifier: GPL-2.0
> > + */
> > +
> > +#ifndef __JTAG_H
> > +#define __JTAG_H
> > +
> > +#include <uapi/linux/jtag.h>
> > +
> > +#ifndef ARCH_DMA_MINALIGN
> > +#define ARCH_DMA_MINALIGN 1
> > +#endif
> > +
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
> > +
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
> > +
> > +struct jtag;
> > +/**
> > + * struct jtag_ops - callbacks for jtag control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by device driver
> > + * @freq_set: set frequency function. Filled by device driver
> > + * @status_get: set status function. Filled by device driver
> > + * @idle: set JTAG to idle state function. Filled by device driver
> > + * @xfer: send JTAG xfer function. Filled by device driver */ struct
> > +jtag_ops {
> > + int (*freq_get)(struct jtag *jtag, u32 *freq);
> > + int (*freq_set)(struct jtag *jtag, u32 freq);
> > + int (*status_get)(struct jtag *jtag, u32 *state);
> > + int (*idle)(struct jtag *jtag, struct jtag_run_test_idle *idle);
> > + int (*xfer)(struct jtag *jtag, struct jtag_xfer *xfer); };
> > +
> > +void *jtag_priv(struct jtag *jtag);
> > +int jtag_register(struct jtag *jtag); void jtag_unregister(struct
> > +jtag *jtag); struct jtag *jtag_alloc(size_t priv_size, const struct
> > +jtag_ops *ops); void jtag_free(struct jtag *jtag);
> > +
> > +#endif /* __JTAG_H */
> > diff --git a/include/uapi/linux/jtag.h b/include/uapi/linux/jtag.h new
> > file mode 100644 index 0000000..180bf22
> > --- /dev/null
> > +++ b/include/uapi/linux/jtag.h
> > @@ -0,0 +1,115 @@
> > +/*
> > + * JTAG class driver
> > + *
> > + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.
> > + * Copyright (c) 2017 Oleksandr Shamray <oleksan...@mellanox.com>
> > + *
> > + * Released under the GPLv2/BSD.
> > + * SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> > +
> > +#ifndef __UAPI_LINUX_JTAG_H
> > +#define __UAPI_LINUX_JTAG_H
> > +
> > +/**
> > + * enum jtag_xfer_mode:
> > + *
> > + * @JTAG_XFER_HW_MODE: hardware mode transfer
> > + * @JTAG_XFER_SW_MODE: software mode transfer */ enum
> jtag_xfer_mode
> > +{
> > + JTAG_XFER_HW_MODE,
> > + JTAG_XFER_SW_MODE,
> > +};
> > +
> > +/**
> > + * enum jtag_endstate:
> > + *
> > + * @JTAG_STATE_IDLE: JTAG state machine IDLE state
> > + * @JTAG_STATE_PAUSEIR: JTAG state machine PAUSE_IR state
> > + * @JTAG_STATE_PAUSEDR: JTAG state machine PAUSE_DR state */ enum
> > +jtag_endstate {
> > + JTAG_STATE_IDLE,
> > + JTAG_STATE_PAUSEIR,
> > + JTAG_STATE_PAUSEDR,
> > +};
> > +
> > +/**
> > + * enum jtag_xfer_type:
> > + *
> > + * @JTAG_SIR_XFER: SIR transfer
> > + * @JTAG_SDR_XFER: SDR transfer
> > + */
> > +enum jtag_xfer_type {
> > + JTAG_SIR_XFER,
> > + JTAG_SDR_XFER,
> > +};
> > +
> > +/**
> > + * enum jtag_xfer_direction:
> > + *
> > + * @JTAG_READ_XFER: read transfer
> > + * @JTAG_WRITE_XFER: write transfer
> > + */
> > +enum jtag_xfer_direction {
> > + JTAG_READ_XFER,
> > + JTAG_WRITE_XFER,
> > +};
> > +
> > +/**
> > + * struct jtag_run_test_idle - forces JTAG state machine to
> > + * RUN_TEST/IDLE state
> > + *
> > + * @mode: access mode
> > + * @reset: 0 - run IDLE/PAUSE from current state
> > + * 1 - go through TEST_LOGIC/RESET state before IDLE/PAUSE
> > + * @end: completion flag
> > + * @tck: clock counter
> > + *
> > + * Structure represents interface to JTAG device for jtag idle
> > + * execution.
> > + */
> > +struct jtag_run_test_idle {
> > + __u8 mode;
> > + __u8 reset;
> > + __u8 endstate;
> > + __u8 tck;
> > +};
> > +
> > +/**
> > + * struct jtag_xfer - jtag xfer:
> > + *
> > + * @mode: access mode
> > + * @type: transfer type
> > + * @direction: xfer direction
> > + * @length: xfer bits len
> > + * @tdio : xfer data array
> > + * @endir: xfer end state
> > + *
> > + * Structure represents interface to Aspeed JTAG device for jtag sdr
> > +xfer
> > + * execution.
> > + */
> > +struct jtag_xfer {
> > + __u8 mode;
> > + __u8 type;
> > + __u8 direction;
> > + __u8 endstate;
> > + __u32 length;
> > + __u64 tdio;
>
> I don't see anything odd here that requires a compat ioctl callback, do you?
>
> thanks,
>
> greg k-h
Thanks for your review.
Oleksandr S.
