Hello Oleksandr-

On Tue, Jan 16, 2018 at 09:18:56AM +0200, Oleksandr Shamray wrote:
[..]

v16->v17
Comments pointed by Julia Cartwright [off-list ref]

More review feedback below:

[..]

[..]

+static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct jtag *jtag = file->private_data;
+	struct jtag_run_test_idle idle;
+	struct jtag_xfer xfer;
+	u8 *xfer_data;
+	u32 data_size;
+	u32 value;
+	int err;
+
+	if (!arg)
+		return -EINVAL;
+
+	switch (cmd) {
+	case JTAG_GIOCFREQ:
+		if (!jtag->ops->freq_get)
+			err = -EOPNOTSUPP;

Did you mean:

			return -EOPNOTSUPP;

?

+
+		err = jtag->ops->freq_get(jtag, &value);

Otherwise you're check was worthless, you'll call NULL here.

Also, w.r.t. the set of ops which are required to be implemented: this
isn't the right place to do the check.

Instead, do it in jtag_alloc():

	struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops)
	{
		struct jtag *jtag;

		if (!ops->freq_get || !ops->xfer || ...) /* fixup condition */
			return NULL;

		jtag = kzalloc(sizeof(*jtag) + priv_size, GFP_KERNEL);
		if (!jtag)
			return NULL;

		jtag->ops = ops;
		return jtag;
	}
	EXPORT_SYMBOL_GPL(jtag_alloc);

[..]

+	case JTAG_IOCXFER:

[..]

+		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)

memdup_user() doesn't return NULL on error.  You need to check for
IS_ERR(xfer_data).

+			return -EFAULT;
+
+		err = jtag->ops->xfer(jtag, &xfer, xfer_data);
+		if (err) {
+			kfree(xfer_data);
+			return -EFAULT;
+		}
+
+		err = copy_to_user(u64_to_user_ptr(xfer.tdio),
+				   (void *)(xfer_data), data_size);
+
+		if (err) {
+			kfree(xfer_data);
+			return -EFAULT;
+		}
+
+		kfree(xfer_data);

Move the kfree() above the if (err).

+		if (copy_to_user((void *)arg, &xfer, sizeof(struct jtag_xfer)))
+			return -EFAULT;
+		break;
+
+	case JTAG_GIOCSTATUS:
+		if (!jtag->ops->status_get)
+			return -EOPNOTSUPP;
+
+		err = jtag->ops->status_get(jtag, &value);
+		if (err)
+			break;
+
+		err = put_user(value, (__u32 *)arg);
+		if (err)
+			err = -EFAULT;

put_user() returns -EFAULT on failure, so this shouldn't be necessary.

[..]

[..]

+/**
+ * struct jtag_xfer - jtag xfer:
+ *
+ * @type: transfer type
+ * @direction: xfer direction
+ * @length: xfer bits len
+ * @tdio : xfer data array
+ * @endir: xfer end state
+ *
+ * Structure represents interface to JTAG device for jtag sdr xfer
+ * execution.
+ */
+struct jtag_xfer {
+	__u8	type;
+	__u8	direction;
+	__u8	endstate;

Just to be as unambiguous as possible, considering this is ABI, I would
suggest explicitly putting a padding byte here.

+	__u32	length;
+	__u64	tdio;
+};

Thanks,
   Julia