Hi Julia > -----Original Message----- > From: Julia Cartwright [mailto:juliac@xxxxxxxxxxxx] > Sent: 15 января 2018 г. 22:52 > To: Oleksandr Shamray <oleksandrs@xxxxxxxxxxxx> > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; openbmc@xxxxxxxxxxxxxxxx; joel@xxxxxxxxx; > jiri@xxxxxxxxxxx; tklauser@xxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx; Vadim > Pasternak <vadimp@xxxxxxxxxxxx>; system-sw-low-level <system-sw-low- > level@xxxxxxxxxxxx>; robh+dt@xxxxxxxxxx; openocd-devel- > owner@xxxxxxxxxxxxxxxxxxxxx; linux-api@xxxxxxxxxxxxxxx; > davem@xxxxxxxxxxxxx; mchehab@xxxxxxxxxx; Jiri Pirko > <jiri@xxxxxxxxxxxx> > 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 -- 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
