Hi Julia > -----Original Message----- > From: Julia Cartwright [mailto:jul...@eso.teric.us] > Sent: 15 января 2018 г. 22:52 > To: Oleksandr Shamray <oleksan...@mellanox.com> > Cc: gre...@linuxfoundation.org; a...@arndb.de; linux- > ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; > devicet...@vger.kernel.org; open...@lists.ozlabs.org; j...@jms.id.au; > j...@resnulli.us; tklau...@distanz.ch; linux-ser...@vger.kernel.org; Vadim > Pasternak <vad...@mellanox.com>; system-sw-low-level <system-sw-low- > le...@mellanox.com>; robh...@kernel.org; openocd-devel- > ow...@lists.sourceforge.net; linux-...@vger.kernel.org; > da...@davemloft.net; mche...@kernel.org; Jiri Pirko > <j...@mellanox.com> > 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: > > 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. > >
[..] > > + 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. > > [..] > > +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. > > > + 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. > > > + 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