Hi Julia

-----Original Message-----
From: Julia Cartwright [mailto:juliac-SPobxgzVcRdvFhSgSjN+lA@public.gmane.org]
Sent: 15 января 2018 г. 22:52
To: Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org; arnd-r2nGTMty4D4@public.gmane.org; linux-
kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org;
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; openbmc-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org;
jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org; tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org; linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Vadim
Pasternak [off-list ref]; system-sw-low-level <system-sw-low-
level-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; openocd-devel-
owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org; mchehab-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Jiri Pirko
[off-list ref]
Subject: Re: [patch v16 1/4] drivers: jtag: Add JTAG core driver

Hello Oleksandr-

I have a few comments below.

On Fri, Jan 12, 2018 at 07:08:26PM +0200, Oleksandr Shamray wrote:

quoted

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.

[..]

quoted

+		break;
+
+	case JTAG_IOCXFER:
+		if (!jtag->ops->xfer)

Are all ops optional?  That seems bizarre.  I would have expected at least one
callback to be required.

You right. But I'm preferred to check pointers before use it.

[..]

quoted

+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;
+
+	if (jtag->opened) {
+		mutex_unlock(&jtag->open_lock);
+		return -EBUSY;
+	}
+
+	nonseekable_open(inode, file);
+	file->private_data = jtag;

These two can be moved out of the lock.

Agree.

quoted

+	jtag->opened = true;
+	mutex_unlock(&jtag->open_lock);
+	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(size_t priv_size, const struct jtag_ops *ops)
+{
+	struct jtag *jtag;
+
+	jtag = kzalloc(sizeof(*jtag), GFP_KERNEL);

Did you mean to allocate: sizeof(*jtag) + priv_size?

Thank, it's a mistake.

quoted

+	if (!jtag)
+		return NULL;
+
+	jtag->ops = ops;
+	return jtag;
+}
+EXPORT_SYMBOL_GPL(jtag_alloc);
+
+void jtag_free(struct jtag *jtag)
+{
+	kfree(jtag);
+}
+EXPORT_SYMBOL_GPL(jtag_free);
+
+int jtag_register(struct jtag *jtag)
+{
+	char *name;
+	int err;
+	int id;
+
+	id = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);
+	if (id < 0)
+		return id;
+
+	jtag->id = id;
+
+	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->dev, "Unable to register device\n");
+	else
+		return 0;
+	jtag->opened = false;

Well, this code flow is just confusing.

I suggest a redo with:

	err = misc_register(&jtag->miscdev);
	if (err) {
		dev_err(jtag->dev, "Unable to register device\n");
		goto err_jtag_name;
	}

If you need to initialize 'opened', do it prior to misc_register.

Thanks

Thanks,
   Julia

Thanks for Review.

Br,
Oleksandr

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html