RE: [patch v15 1/4] drivers: jtag: Add JTAG core driver

[Oleksandr Shamray <oleksandrs@mellanox.com>]

> -----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.