[patch v22 1/4] drivers: jtag: Add JTAG core driver
Oleksandr Shamray
oleksandrs at mellanox.com
Tue May 29 01:59:46 AEST 2018
Hi Greg.
Thanks for your review.
Please see my comments inline.
Best Regards,
Oleksandr Shamray
> -----Original Message-----
> From: Greg KH [mailto:gregkh at linuxfoundation.org]
> Sent: 28 мая 2018 г. 15:35
> To: Oleksandr Shamray <oleksandrs at mellanox.com>
> Cc: arnd at arndb.de; linux-kernel at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; devicetree at vger.kernel.org;
> openbmc at lists.ozlabs.org; joel at jms.id.au; jiri at resnulli.us;
> tklauser at distanz.ch; linux-serial at vger.kernel.org; Vadim Pasternak
> <vadimp at mellanox.com>; system-sw-low-level <system-sw-low-
> level at mellanox.com>; robh+dt at kernel.org; openocd-devel-
> owner at lists.sourceforge.net; linux-api at vger.kernel.org;
> davem at davemloft.net; mchehab at kernel.org
> Subject: Re: [patch v22 1/4] drivers: jtag: Add JTAG core driver
>
> On Mon, May 28, 2018 at 01:34:09PM +0300, Oleksandr Shamray wrote:
> > Initial patch for JTAG driver
> > JTAG class driver provide infrastructure to support hardware/software
> > JTAG platform drivers. It provide user layer API interface for
> > flashing and debugging external devices which equipped with JTAG
> > interface using standard transactions.
> >
> > Driver exposes set of IOCTL to user space for:
> > - XFER:
> > - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> > - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> > - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> > number of clocks).
> > - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
> >
> > Driver core provides set of internal APIs for allocation and
> > registration:
> > - jtag_register;
> > - jtag_unregister;
> > - jtag_alloc;
> > - jtag_free;
> >
> > Platform driver on registration with jtag-core creates the next entry
> > in dev folder:
> > /dev/jtagX
> >
> > Signed-off-by: Oleksandr Shamray <oleksandrs at mellanox.com>
> > Signed-off-by: Jiri Pirko <jiri at mellanox.com>
> > Acked-by: Philippe Ombredanne <pombredanne at nexb.com>
> > ---
> > v21->v22
>
> 22 versions, way to stick with this...
>
> Anyway, time to review it again. I feel it's really close, but I don't want to
> apply it yet as the api feels odd in places. If others have asked you to make
> these changes to look like this, I'm sorry, then please push back on me:
>
> > +/*
> > + * JTAG core driver supports up to 256 devices
> > + * JTAG device name will be in range jtag0-jtag255
> > + * Set maximum len of jtag id to 3
> > + */
> > +
> > +#define MAX_JTAG_DEVS 255
>
> Why have a max at all? You should be able to just dynamically allocate them.
> Anyway, if you do want a max, you need to be able to properly keep the max
> number, and right now you have a race when registering (you check the
> number, and then much later do you increment it, a pointless use of an
> atomic value if I've ever seen one...)
>
You are right. It's not good idea to have restriction of max dev number.
I will remove all regarding It.
> > +#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 3)
> > +
> > +struct jtag {
> > + struct miscdevice miscdev;
>
> If you are a miscdev, why even have a max number? Just let the max
> number of miscdev devices rule things.
>
> > + const struct jtag_ops *ops;
> > + int id;
> > + bool opened;
>
> You shouldn't care about this, but ok...
Jtag HW not support to using with multiple requests from different users. So we prohibit this.
>
> > + struct mutex open_lock;
> > + unsigned long priv[0];
> > +};
> > +
> > +static DEFINE_IDA(jtag_ida);
> > +
> > +static atomic_t jtag_refcount = ATOMIC_INIT(0);
>
> Either use it as an atomic properly (test_and_set), or just leave it as an int, or
> better yet, don't use it at all.
It will be removed.
>
> > +void *jtag_priv(struct jtag *jtag)
> > +{
> > + return jtag->priv;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_priv);
>
> Api naming issue here. Traditionally this is a "get/set_drvdata" type function,
> as in dev_get_drvdata(), or dev_set_drvdata. But, you are right in that the
> network stack has a netdev_priv() call, where you probably copied this idea
> from.
>
> I don't know, I guess it's ok as-is, as it's the way networking does it, it's your
> call though.
>
Yes, I did as in networking.
> > +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 data_size;
> > + u32 value;
> > + int err;
> > +
> > + if (!arg)
> > + return -EINVAL;
> > +
> > + switch (cmd) {
> > + case JTAG_GIOCFREQ:
> > + if (!jtag->ops->freq_get)
> > + return -EOPNOTSUPP;
> > +
> > + err = jtag->ops->freq_get(jtag, &value);
> > + if (err)
> > + break;
> > +
> > + if (put_user(value, (__u32 __user *)arg))
> > + err = -EFAULT;
> > + break;
> > +
> > + case JTAG_SIOCFREQ:
> > + if (!jtag->ops->freq_set)
> > + return -EOPNOTSUPP;
> > +
> > + if (get_user(value, (__u32 __user *)arg))
> > + return -EFAULT;
> > + if (value == 0)
> > + return -EINVAL;
> > +
> > + err = jtag->ops->freq_set(jtag, value);
> > + break;
> > +
> > + case JTAG_IOCRUNTEST:
> > + if (copy_from_user(&idle, (const void __user *)arg,
> > + sizeof(struct jtag_run_test_idle)))
> > + return -EFAULT;
> > +
> > + if (idle.endstate > JTAG_STATE_PAUSEDR)
> > + return -EINVAL;
>
> No validation that idle contains sane values? Don't make every jtag driver
> have to do this type of validation please.
>
I have partially validation of idle structure (if (idle.endstate > JTAG_STATE_PAUSEDR)).
But I will add more validation.
>
> > +
> > + err = jtag->ops->idle(jtag, &idle);
> > + break;
> > +
> > + case JTAG_IOCXFER:
> > + if (copy_from_user(&xfer, (const void __user *)arg,
> > + sizeof(struct jtag_xfer)))
> > + return -EFAULT;
> > +
> > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> > + return -EINVAL;
> > +
> > + if (xfer.type > JTAG_SDR_XFER)
> > + return -EINVAL;
> > +
> > + if (xfer.direction > JTAG_WRITE_XFER)
> > + return -EINVAL;
> > +
> > + if (xfer.endstate > JTAG_STATE_PAUSEDR)
> > + return -EINVAL;
> > +
>
> See, you do good error checking here, why not for the previous ioctl?
>
>
> > + data_size = DIV_ROUND_UP(xfer.length, BITS_PER_BYTE);
> > + xfer_data = memdup_user(u64_to_user_ptr(xfer.tdio),
> data_size);
> > +
>
> No blank line.
>
Will remove
> > + if (IS_ERR(xfer_data))
> > + return -EFAULT;
> > +
> > + err = jtag->ops->xfer(jtag, &xfer, xfer_data);
> > + if (err) {
> > + kfree(xfer_data);
> > + return -EFAULT;
>
> Why not return the error given to you? -EFAULT is only if there is a memory
> error in a copy_to/from_user() call.
>
Will change return code to err
> > + }
> > +
> > + err = copy_to_user(u64_to_user_ptr(xfer.tdio),
> > + (void *)xfer_data, data_size);
> > + kfree(xfer_data);
> > + if (err)
> > + return -EFAULT;
>
> Like here, that's correct.
>
> > +
> > + if (copy_to_user((void __user *)arg, (void *)&xfer,
> > + sizeof(struct jtag_xfer)))
> > + return -EFAULT;
> > + break;
> > +
> > + case JTAG_GIOCSTATUS:
> > + err = jtag->ops->status_get(jtag, &value);
> > + if (err)
> > + break;
> > +
> > + err = put_user(value, (__u32 __user *)arg);
> > + break;
> > + case JTAG_SIOCMODE:
> > + if (!jtag->ops->mode_set)
> > + return -EOPNOTSUPP;
> > +
> > + if (get_user(value, (__u32 __user *)arg))
> > + return -EFAULT;
> > + if (value == 0)
> > + return -EINVAL;
> > +
> > + err = jtag->ops->mode_set(jtag, value);
> > + break;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > + return err;
> > +}
> > +
> > +static int jtag_open(struct inode *inode, struct file *file) {
> > + struct jtag *jtag = container_of(file->private_data, struct jtag,
> > +miscdev);
> > +
> > + if (mutex_lock_interruptible(&jtag->open_lock))
> > + return -ERESTARTSYS;
>
> Why restart? Just grab the lock, you don't want to have to do restartable
> logic in userspace, right?
Will change like:
ret = mutex_lock_interruptible(&jtag->open_lock);
if (ret)
return ret;
>
> > +
> > + if (jtag->opened) {
> > + mutex_unlock(&jtag->open_lock);
> > + return -EBUSY;
>
> Why do you care about only 1 userspace open call? What does that buy you?
> Userspace can always pass around that file descriptor and use it in multiple
> places, so you are not really "protecting" yourself from anything.
>
Accessing to JTAG HW from multiple processes will make confusion in the work of the JTAG protocol.
And I want to prohibit this.
> And better yet, if you get rid of this, your open/release functions get very
> tiny and simple.
>
> > + }
> > + jtag->opened = true;
> > + mutex_unlock(&jtag->open_lock);
> > +
> > + nonseekable_open(inode, file);
>
> No error checking of the return value? Not good :(
Will add error handling
>
> > + file->private_data = jtag;
> > + return 0;
> > +}
> > +
> > +static int jtag_release(struct inode *inode, struct file *file) {
> > + struct jtag *jtag = file->private_data;
> > +
> > + mutex_lock(&jtag->open_lock);
> > + jtag->opened = false;
> > + mutex_unlock(&jtag->open_lock);
> > + 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,
> > +};
> > +
> > +struct jtag *jtag_alloc(struct device *host, size_t priv_size,
> > + const struct jtag_ops *ops)
> > +{
> > + struct jtag *jtag;
> > +
> > + if (!host)
> > + return NULL;
> > +
> > + if (!ops)
> > + return NULL;
> > +
> > + if (!ops->idle || !ops->status_get || !ops->xfer)
> > + return NULL;
> > +
> > + jtag = kzalloc(sizeof(*jtag) + priv_size, GFP_KERNEL);
> > + if (!jtag)
> > + return NULL;
> > +
> > + jtag->ops = ops;
> > + jtag->miscdev.parent = host;
> > +
> > + return jtag;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_alloc);
> > +
> > +void jtag_free(struct jtag *jtag)
> > +{
> > + kfree(jtag);
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_free);
> > +
> > +static int jtag_register(struct jtag *jtag) {
> > + struct device *dev = jtag->miscdev.parent;
> > + char *name;
> > + int err;
> > + int id;
> > +
> > + if (!dev)
> > + return -ENODEV;
> > +
> > + if (atomic_read(&jtag_refcount) >= MAX_JTAG_DEVS)
> > + return -ENOSPC;
>
> Here, you are reading the value, and then setting it a long ways away.
> What happens if two people call this at the same time. Not good.
> Either do it correctly, or better yet, don't do it at all.
>
Removed.
> > +
> > + id = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);
> > + if (id < 0)
> > + return id;
> > +
> > + jtag->id = id;
> > + jtag->opened = false;
> > +
> > + name = kzalloc(MAX_JTAG_NAME_LEN, GFP_KERNEL);
> > + if (!name) {
> > + err = -ENOMEM;
> > + goto err_jtag_alloc;
> > + }
> > +
> > + err = snprintf(name, MAX_JTAG_NAME_LEN, "jtag%d", id);
> > + if (err < 0)
> > + goto err_jtag_name;
> > +
> > + mutex_init(&jtag->open_lock);
> > + jtag->miscdev.fops = &jtag_fops;
> > + jtag->miscdev.minor = MISC_DYNAMIC_MINOR;
> > + jtag->miscdev.name = name;
> > +
> > + err = misc_register(&jtag->miscdev);
> > + if (err) {
> > + dev_err(jtag->miscdev.parent, "Unable to register
> device\n");
> > + goto err_jtag_name;
> > + }
> > + pr_info("%s %d\n", __func__, __LINE__);
> > + atomic_inc(&jtag_refcount);
> > + return 0;
> > +
> > +err_jtag_name:
> > + kfree(name);
> > +err_jtag_alloc:
> > + ida_simple_remove(&jtag_ida, id);
> > + return err;
> > +}
> > +
> > +static void jtag_unregister(struct jtag *jtag) {
> > + misc_deregister(&jtag->miscdev);
> > + kfree(jtag->miscdev.name);
> > + ida_simple_remove(&jtag_ida, jtag->id);
> > + atomic_dec(&jtag_refcount);
> > +}
> > +
> > +static void devm_jtag_unregister(struct device *dev, void *res) {
> > + jtag_unregister(*(struct jtag **)res); }
> > +
> > +int devm_jtag_register(struct device *dev, struct jtag *jtag) {
> > + struct jtag **ptr;
> > + int ret;
> > +
> > + ptr = devres_alloc(devm_jtag_unregister, sizeof(*ptr),
> GFP_KERNEL);
> > + if (!ptr)
> > + return -ENOMEM;
> > +
> > + ret = jtag_register(jtag);
> > + if (!ret) {
> > + *ptr = jtag;
> > + devres_add(dev, ptr);
> > + } else {
> > + devres_free(ptr);
> > + }
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_jtag_register);
> > +
> > +static void __exit jtag_exit(void)
> > +{
> > + ida_destroy(&jtag_ida);
> > +}
> > +
> > +module_exit(jtag_exit);
> > +
> > +MODULE_AUTHOR("Oleksandr Shamray <oleksandrs at 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..56d784d
> > --- /dev/null
> > +++ b/include/linux/jtag.h
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// include/linux/jtag.h - JTAG class driver // // Copyright (c) 2018
> > +Mellanox Technologies. All rights reserved.
> > +// Copyright (c) 2018 Oleksandr Shamray <oleksandrs at mellanox.com>
> > +
> > +#ifndef __JTAG_H
> > +#define __JTAG_H
> > +
> > +#include <uapi/linux/jtag.h>
> > +
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
>
> Why do you need such a horrid cast? What is this for? And why use uintptr_t
> at all? It's not a "normal" kernel type.
>
Not needed - will be removed.
> > +
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
> > +
> > +struct jtag;
> > +/**
> > + * struct jtag_ops - callbacks for JTAG control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by dev driver
> > + * @freq_set: set frequency function. Filled by dev driver
> > + * @status_get: set status function. Mandatory func. Filled by dev
> > +driver
> > + * @idle: set JTAG to idle state function. Mandatory func. Filled by
> > +dev driver
> > + * @xfer: send JTAG xfer function. Mandatory func. Filled by dev
> > +driver
> > + * @mode_set: set specific work mode for JTAG. Filled by dev 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, u8 *xfer_data);
> > + int (*mode_set)(struct jtag *jtag, u32 mode_mask); };
> > +
> > +void *jtag_priv(struct jtag *jtag);
> > +int devm_jtag_register(struct device *dev, struct jtag *jtag); void
> > +devm_jtag_unregister(struct device *dev, void *res); struct jtag
> > +*jtag_alloc(struct device *host, 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..8bbf1a7
> > --- /dev/null
> > +++ b/include/uapi/linux/jtag.h
> > @@ -0,0 +1,102 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// include/uapi/linux/jtag.h - JTAG class driver uapi // // Copyright
> > +(c) 2018 Mellanox Technologies. All rights reserved.
> > +// Copyright (c) 2018 Oleksandr Shamray <oleksandrs at mellanox.com>
> > +
> > +#ifndef __UAPI_LINUX_JTAG_H
> > +#define __UAPI_LINUX_JTAG_H
> > +
> > +/*
> > + * JTAG_XFER_HW_MODE: JTAG hardware mode. Used to set HW drived
> or
> > +bitbang
> > + * mode. This is bitmask param of ioctl JTAG_SIOCMODE command */
> > +#define JTAG_XFER_HW_MODE 1
> > +
> > +/**
> > + * 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
> > + *
> > + * @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 provide interface to JTAG device for JTAG IDLE execution.
> > + */
> > +struct jtag_run_test_idle {
> > + __u8 reset;
> > + __u8 endstate;
> > + __u8 tck;
> > +};
> > +
> > +/**
> > + * struct jtag_xfer - jtag xfer:
> > + *
> > + * @type: transfer type
> > + * @direction: xfer direction
> > + * @length: xfer bits len
> > + * @tdio : xfer data array
> > + * @endir: xfer end state
> > + *
> > + * Structure provide interface to JTAG device for JTAG SDR/SIR xfer
> execution.
> > + */
> > +struct jtag_xfer {
> > + __u8 type;
> > + __u8 direction;
> > + __u8 endstate;
> > + __u8 padding;
> > + __u32 length;
> > + __u64 tdio;
> > +};
> > +
> > +/* ioctl interface */
> > +#define __JTAG_IOCTL_MAGIC 0xb2
> > +
> > +#define JTAG_IOCRUNTEST _IOW(__JTAG_IOCTL_MAGIC, 0,\
> > + struct jtag_run_test_idle)
>
> No need for 2 lines here, fits just fine on one.
Ok
>
> > +#define JTAG_SIOCFREQ _IOW(__JTAG_IOCTL_MAGIC, 1, unsigned
> int)
> > +#define JTAG_GIOCFREQ _IOR(__JTAG_IOCTL_MAGIC, 2, unsigned int)
> > +#define JTAG_IOCXFER _IOWR(__JTAG_IOCTL_MAGIC, 3, struct
> jtag_xfer)
> > +#define JTAG_GIOCSTATUS _IOWR(__JTAG_IOCTL_MAGIC, 4, enum
> jtag_endstate)
> > +#define JTAG_SIOCMODE _IOW(__JTAG_IOCTL_MAGIC, 5, unsigned
> int)
> > +
> > +#define JTAG_FIRST_MINOR 0
>
> Why is this in a uapi file?
Not needed - will be removed.
>
> > +#define JTAG_MAX_DEVICES 32
>
> This is never used, why is it in a uapi file?
>
Not needed - will be removed.
> So, almost there, with the above mentioned things, I think you can make the
> code even simpler, which is always better, right?
>
> thanks,
>
> greg k-h
Thanks.
BR,
Oleksandr Shamray
More information about the openbmc
mailing list