> -----Original Message----- > From: Florian Fainelli [mailto:f.fainelli@xxxxxxxxx] > Sent: 26 декабря 2017 г. 1:09 > To: Oleksandr Shamray <oleksandrs@xxxxxxxxxxxx>; > gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx > Cc: 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 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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. ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
