Messages in this thread | | | From | Oleksandr Shamray <> | Subject | RE: [patch v15 1/4] drivers: jtag: Add JTAG core driver | Date | Fri, 12 Jan 2018 16:42:35 +0000 |
| |
> -----Original Message----- > From: Florian Fainelli [mailto:f.fainelli@gmail.com] > Sent: 26 декабря 2017 г. 1:09 > To: Oleksandr Shamray <oleksandrs@mellanox.com>; > gregkh@linuxfoundation.org; arnd@arndb.de > Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; joel@jms.id.au; > jiri@resnulli.us; tklauser@distanz.ch; linux-serial@vger.kernel.org; Vadim > Pasternak <vadimp@mellanox.com>; system-sw-low-level <system-sw-low- > level@mellanox.com>; robh+dt@kernel.org; openocd-devel- > owner@lists.sourceforge.net; linux-api@vger.kernel.org; > davem@davemloft.net; mchehab@kernel.org; Jiri Pirko > <jiri@mellanox.com> > Subject: Re: [patch v15 1/4] drivers: jtag: Add JTAG core driver > >
[snip]
> > + > > + case JTAG_IOCXFER: > > + if (copy_from_user(&xfer, (void *)arg, > > + sizeof(struct jtag_xfer))) > > + return -EFAULT; > > + > > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN) > > + return -EINVAL; > > + > > + if (xfer.type > JTAG_SDR_XFER) > > + return -EINVAL; > > + > > + if (xfer.direction > JTAG_WRITE_XFER) > > + return -EINVAL; > > + > > + if (xfer.endstate > JTAG_STATE_PAUSEDR) > > + return -EINVAL; > > + > > + data_size = DIV_ROUND_UP(xfer.length, BITS_PER_BYTE); > > + xfer_data = memdup_user(u64_to_user_ptr(xfer.tdio), > data_size); > > + > > + if (!xfer_data) > > + return -EFAULT; > > + > > + if (jtag->ops->xfer) { > > + err = jtag->ops->xfer(jtag, &xfer, xfer_data); > > + } else { > > + kfree(xfer_data); > > + return -EOPNOTSUPP; > > + } > > Why don't you move all of the code here into a function which will make the > error handling consistent? Also, checking whether the jtag adapter
Greg KH <gregkh@linuxfoundation.org> Say to move all of this insight ioctl
> implements ops->xfer should probably be done before you do the > memdup_user().
Yes
> > + if (err) { > > + kfree(xfer_data); > > + return -EFAULT; > > + } > > + > > + if (jtag->ops->mode_set) > > + err = jtag->ops->mode_set(jtag, value); > > + else > > + err = -EOPNOTSUPP; > > + break; > > Same here, this can be checked before get_user(). >
Yes
> > + if (jtag->opened) { > > + mutex_unlock(&jtag->open_lock); > > + return -EINVAL; > > -EBUSY maybe? >
Yes
> > + > > + jtag = kzalloc(sizeof(*jtag) + round_up(priv_size, > ARCH_DMA_MINALIGN), > > + GFP_KERNEL); > > + if (!jtag) > > + return NULL; > > If you set ARCH_DMA_MINALIGN to 1 when not defined, what is this > achieving that kmalloc() is not already doing? >
Removed ARCH_DMA_MINALIGN
> > + > > + 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; > > + } > > Can't you use jtag->miscdev.dev here to simplify the allocation error > handling? >
How, what you mean?
> > +#ifndef ARCH_DMA_MINALIGN > > +#define ARCH_DMA_MINALIGN 1 > > +#endif > > Why? >
Not used now, Deleted
> > +#endif /* __JTAG_H */ > > diff --git a/include/uapi/linux/jtag.h b/include/uapi/linux/jtag.h new > > file mode 100644 index 0000000..cda2520 > > --- /dev/null > > +++ b/include/uapi/linux/jtag.h > > [snip] > > > +struct jtag_xfer { > > + __u8 type; > > + __u8 direction; > > Can these two be an enum referring to what you defined earlier? >
Greg KH <gregkh@linuxfoundation.org> say:
"All structures that cross the user/kernel boundry have to use the __ type variables. No "unsigned char", it has to be "__u8", no "unsigned short", it has to be "__u16", and so on. Also, watch out for your enumerated types, what's the packing end up looking like on these structures? Have you verified it works with a 64bit kernel and 32bit userspace all correctly?"
So I use __u8 type instead of enum to avoid errors while crossing 64bit kernel and 32bit userspace.
> > + __u8 endstate; > > + __u32 length; > > + __u64 tdio; > > +}; > -- > Florian
Thaks.
| |