From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: ACJfBotajWN5pnQ8XtvgkzKTfWhAJI1FmVTDQPRluCpQHE8FOOaINKBVUtexiKFhcjU57ZOH8uG1 ARC-Seal: i=1; a=rsa-sha256; t=1516216482; cv=none; d=google.com; s=arc-20160816; b=eYaZxLU473uYbaMH+sLlth/kpaB561TfH0doD9uKodVLCpeoSzzmzzy3IUF2K6GEag 9GGmEM3V4xPECjE/OtSWBLOsQiqxVnkXr66t839JvtdqIePpad1b505gWJupnGPykmFf 7+0q/o8UDI01tlaHwRVmGjjXQ7qWMmYEvbz1Byqp4AuDXfLJ7hqRzYGhSkbX6wXBHwgE iBJcozPn9pg8isrcXrViIuMCY7YAbJekdJ4nYAz4mCviCnIIr79l+dCFQf21NFqEzoGJ iULryi35XRGis+D0xqUttHfPq8Jgy0Q7iOv7/jgSO1Y3zUqCTWavgz0WWEeArFoyOCGr Q2Vg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:dkim-signature :arc-authentication-results; bh=kdKsnvOgsUQfS4iX2mzbXVFGJGbigQOq/fiBZYDtoMs=; b=HDQmJdWX8kg0nkG4Wlnofe7IfCFIBFaFAEvoJVvBTv2frVGopbIyyomGbJDh4DPkfN B0UVQ+cly0s//pQfO58x2UD0oEI6gU+FFaYNxCoz5anyn7aVFEc1Tlf3h8ZvfE6Idbui k3UFYsCS8S5fCL9CrAYmSizdOfe29fXknwGdnNddc2dppXbWC3Kb4nFvxSBfMbdD4Ruy KaZhajo8xyKlNwAhqL8r4uUD4SA/wTgKq5e1ZlwzHTFGSW5FUSu0AhaOLfcjkHRHMDQv p2f8M+WYyry4iJulFp7KoBgkJW7R2rsbZQSZ7NVHnBPoV8+e3h/pbJdMsdTDwXsPUevq QnpQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@eso.teric.us header.s=mail header.b=JvbA48nq; spf=pass (google.com: domain of juliac@eso.teric.us designates 2600:3c00::f03c:91ff:fedf:a84c as permitted sender) smtp.mailfrom=juliac@eso.teric.us Authentication-Results: mx.google.com; dkim=pass header.i=@eso.teric.us header.s=mail header.b=JvbA48nq; spf=pass (google.com: domain of juliac@eso.teric.us designates 2600:3c00::f03c:91ff:fedf:a84c as permitted sender) smtp.mailfrom=juliac@eso.teric.us Date: Wed, 17 Jan 2018 13:14:41 -0600 From: Julia Cartwright To: Oleksandr Shamray Cc: gregkh@linuxfoundation.org, arnd@arndb.de, 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, vadimp@mellanox.com, 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 Subject: Re: [patch v17 1/4] drivers: jtag: Add JTAG core driver Message-ID: <20180117191441.GE2818@kryptos.localdomain> References: <1516087139-7510-1-git-send-email-oleksandrs@mellanox.com> <1516087139-7510-2-git-send-email-oleksandrs@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1516087139-7510-2-git-send-email-oleksandrs@mellanox.com> User-Agent: Mutt/1.8.3 (2017-05-23) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1587756731526966143?= X-GMAIL-MSGID: =?utf-8?q?1589868214374722936?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: Hello Oleksandr- On Tue, Jan 16, 2018 at 09:18:56AM +0200, Oleksandr Shamray wrote: [..] > v16->v17 > Comments pointed by Julia Cartwright More review feedback below: [..] > +++ b/drivers/jtag/jtag.c [..] > +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. [..] > --- /dev/null > +++ b/include/uapi/linux/jtag.h [..] > +/** > + * 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