On Fri, Sep 8, 2017 at 6:13 PM, Oleksandr Shamray
<oleksandrs@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Signed-off-by: Jiri Pirko <jiri@xxxxxxxxxxxx>
> Acked-by: Arnd Bergmann <arnd@xxxxxxxx>
My Ack was actually just for the device driver part, I had not
looked at the core again, sorry for not being clearer here.
The other two patches are fine, but looking at this one again,
I find a couple of things to improve:
> +
> +static __u64 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);
> +
> + return (__u64)(uintptr_t)kdata;
> +}
> +
> +static unsigned long jtag_copy_to_user(__u64 udata, __u64 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), jtag_u64_to_ptr(kdata),
> + size);
> +}
Only a style comment, but the casting to and from u64 multiple
times here seems overly complicated. Why not use a regular kernel
pointer here, and then pass that as a third argument to
ag->ops->xfer() ?
> +
> + case JTAG_SIOCFREQ:
> + err = __get_user(value, uarg);
This needs to use get_user(), not __get_user(). I think you changed
all the other instances after an earlier comment, but missed this one.
> +static int jtag_open(struct inode *inode, struct file *file)
> +{
> + struct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);
> +
> + spin_lock(&jtag->lock);
> +
> + if (jtag->open) {
> + dev_info(NULL, "jtag already opened\n");
> + spin_unlock(&jtag->lock);
> + return -EBUSY;
> + }
> +
> + jtag->open++;
> + file->private_data = jtag;
> + spin_unlock(&jtag->lock);
> + return 0;
> +}
The spinlock seems to not protect anything but the use count,
so this could be an atomic_t.
> + mutex_lock(&jtag_mutex);
> + list_add_tail(&jtag->list, &jtag_list);
> + mutex_unlock(&jtag_mutex);
Similarly, you only use the mutex to protect the list_add/list_del,
but nothing ever walks the list, so I think you can simply remove
both the list and the mutex.
> +static int __init jtag_init(void)
> +{
> + int err;
> +
> + err = alloc_chrdev_region(&jtag_devt, 0, 1, "jtag");
> + if (err)
> + return err;
> + return class_register(&jtag_class);
> +}
> +
> +static void __exit jtag_exit(void)
> +{
> + class_unregister(&jtag_class);
> +}
This looks a bit asymmetric:
- you allocate a chardev region but don't destroy it in jtag_exit,
so presumably this leaks a major number allocation each
time you load/unload the module
- you allocate a single minor number at module load time,
but the individual devices each get another number, and
the original one does not appear to be used, unless I
misunderstand the API here.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
