Hi Julia

-----Original Message-----
From: Julia Cartwright [mailto:juliac at eso.teric.us]
Sent: 15 ?????? 2018 ?. 22:52
To: Oleksandr Shamray <redacted>
Cc: gregkh at linuxfoundation.org; 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 [off-list ref]; 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; 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