Re: [patch v8 1/4] drivers: jtag: Add JTAG core driver

On Fri, Sep 8, 2017 at 6:13 PM, Oleksandr Shamray
[off-list ref] 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-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jiri Pirko <jiri-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Acked-by: Arnd Bergmann <redacted>

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