[{"id":1791392,"web_url":"http://patchwork.ozlabs.org/comment/1791392/","msgid":"<20171020115512.GA2073@kroah.com>","list_archive_url":null,"date":"2017-10-20T11:55:12","subject":"Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver","submitter":{"id":11800,"url":"http://patchwork.ozlabs.org/api/people/11800/","name":"Greg Kroah-Hartman","email":"gregkh@linuxfoundation.org"},"content":"On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:\n> +struct jtag {\n> +\tstruct device *dev;\n> +\tstruct cdev cdev;\n\nWhy are you using a cdev here and not just a normal misc device? I\nforgot if this is what you were doing before, sorry...\n\n> +\tint id;\n> +\tatomic_t open;\n\nWhy do you need this?\n\n> +\tconst struct jtag_ops *ops;\n> +\tunsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);\n\nHuh? Why is this needed to be dma aligned? Why not just use the\nprivate pointer in struct device?\n\n> +};\n> +\n> +static dev_t jtag_devt;\n> +static DEFINE_IDA(jtag_ida);\n> +\n> +void *jtag_priv(struct jtag *jtag)\n> +{\n> +\treturn jtag->priv;\n> +}\n> +EXPORT_SYMBOL_GPL(jtag_priv);\n> +\n> +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size)\n> +{\n> +\tunsigned long size;\n> +\tvoid *kdata;\n> +\n> +\tsize = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\n> +\tkdata = memdup_user(u64_to_user_ptr(udata), size);\n\nYou only use this once, why not just open-code it?\n\n> +\n> +\treturn kdata;\n> +}\n> +\n> +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,\n> +\t\t\t\t unsigned long bit_size)\n> +{\n> +\tunsigned long size;\n> +\n> +\tsize = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\n> +\n> +\treturn copy_to_user(u64_to_user_ptr(udata), (void *)(kdata), size);\n\nSame here, making this a separate function seems odd.\n\n> +}\n> +\n> +static struct class jtag_class = {\n> +\t.name = \"jtag\",\n> +\t.owner = THIS_MODULE,\n> +};\n> +\n> +static int jtag_run_test_idle_op(struct jtag *jtag,\n> +\t\t\t\t struct jtag_run_test_idle *idle)\n> +{\n> +\tif (jtag->ops->idle)\n> +\t\treturn jtag->ops->idle(jtag, idle);\n> +\telse\n> +\t\treturn -EOPNOTSUPP;\n\nWhy is this a function? Why not just do this work in the ioctl?\n\n> +}\n> +\n> +static int jtag_xfer_op(struct jtag *jtag, struct jtag_xfer *xfer, u8 *data)\n> +{\n> +\tif (jtag->ops->xfer)\n> +\t\treturn jtag->ops->xfer(jtag, xfer, data);\n> +\telse\n> +\t\treturn -EOPNOTSUPP;\n\nSame here, doesn't need to be a function.\n\n> +}\n> +\n> +static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned long arg)\n> +{\n> +\tstruct jtag *jtag = file->private_data;\n> +\tstruct jtag_run_test_idle idle;\n> +\tstruct jtag_xfer xfer;\n> +\tu8 *xfer_data;\n> +\tu32 value;\n> +\tint err;\n> +\n> +\tswitch (cmd) {\n> +\tcase JTAG_GIOCFREQ:\n> +\t\tif (jtag->ops->freq_get)\n> +\t\t\terr = jtag->ops->freq_get(jtag, &value);\n> +\t\telse\n> +\t\t\terr = -EOPNOTSUPP;\n> +\t\tif (err)\n> +\t\t\tbreak;\n> +\n> +\t\terr = put_user(value, (__u32 *)arg);\n> +\t\tbreak;\n\nAre you sure the return value of put_user is correct here? Shouldn't\nyou return -EFAULT if err is not 0?\n\n> +\n> +\tcase JTAG_SIOCFREQ:\n> +\t\terr = get_user(value, (__u32 *)arg);\n> +\n\nwhy a blank line?\n\n> +\t\tif (value == 0)\n> +\t\t\terr = -EINVAL;\n\nCheck err first.\n\n> +\t\tif (err)\n> +\t\t\tbreak;\n\nAnd set it properly, again err should be -EFAULT, right?\n\n> +\n> +\t\tif (jtag->ops->freq_set)\n> +\t\t\terr = jtag->ops->freq_set(jtag, value);\n> +\t\telse\n> +\t\t\terr = -EOPNOTSUPP;\n> +\t\tbreak;\n> +\n> +\tcase JTAG_IOCRUNTEST:\n> +\t\tif (copy_from_user(&idle, (void *)arg,\n> +\t\t\t\t sizeof(struct jtag_run_test_idle)))\n> +\t\t\treturn -ENOMEM;\n> +\t\terr = jtag_run_test_idle_op(jtag, &idle);\n\nWho validates the structure fields? Is that up to the individual jtag\ndriver? Why not do it in the core correctly so that it only has to be\ndone in one place and you do not have to audit every individual driver?\n\n> +\t\tbreak;\n> +\n> +\tcase JTAG_IOCXFER:\n> +\t\tif (copy_from_user(&xfer, (void *)arg,\n> +\t\t\t\t sizeof(struct jtag_xfer)))\n> +\t\t\treturn -EFAULT;\n> +\n> +\t\tif (xfer.length >= JTAG_MAX_XFER_DATA_LEN)\n> +\t\t\treturn -EFAULT;\n> +\n> +\t\txfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);\n> +\t\tif (!xfer_data)\n> +\t\t\treturn -ENOMEM;\n\nAre you sure that's the correct error value?\n\n> +\n> +\t\terr = jtag_xfer_op(jtag, &xfer, xfer_data);\n> +\t\tif (jtag_copy_to_user(xfer.tdio, xfer_data, xfer.length)) {\n> +\t\t\tkfree(xfer_data);\n> +\t\t\treturn -EFAULT;\n> +\t\t}\n> +\n> +\t\tkfree(xfer_data);\n> +\t\tif (copy_to_user((void *)arg, &xfer, sizeof(struct jtag_xfer)))\n> +\t\t\treturn -EFAULT;\n> +\t\tbreak;\n> +\n> +\tcase JTAG_GIOCSTATUS:\n> +\t\tif (jtag->ops->status_get)\n> +\t\t\terr = jtag->ops->status_get(jtag, &value);\n> +\t\telse\n> +\t\t\terr = -EOPNOTSUPP;\n> +\t\tif (err)\n> +\t\t\tbreak;\n> +\n> +\t\terr = put_user(value, (__u32 *)arg);\n\nSame put_user err issue here.\n\n> +\t\tbreak;\n> +\n> +\tdefault:\n> +\t\treturn -EINVAL;\n> +\t}\n> +\treturn err;\n> +}\n> +\n> +#ifdef CONFIG_COMPAT\n> +static long jtag_ioctl_compat(struct file *file, unsigned int cmd,\n> +\t\t\t unsigned long arg)\n> +{\n> +\treturn jtag_ioctl(file, cmd, (unsigned long)compat_ptr(arg));\n> +}\n> +#endif\n\nWhy do you need a compat callback for a new ioctl? Create your\nstructures properly and this should not be needed, right?\n\n> +\n> +static int jtag_open(struct inode *inode, struct file *file)\n> +{\n> +\tstruct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);\n> +\n> +\tif (atomic_read(&jtag->open)) {\n> +\t\tdev_info(NULL, \"jtag already opened\\n\");\n> +\t\treturn -EBUSY;\n\nWhy do you care if multiple opens can happen?\n\n> +\t}\n> +\n> +\tatomic_inc(&jtag->open);\n\nOh look, you just raced with two different people opening at the same\ntime :(\n\nAn atomic value is never a replacement for a lock, sorry.\n\n> +\tfile->private_data = jtag;\n> +\treturn 0;\n> +}\n> +\n> +static int jtag_release(struct inode *inode, struct file *file)\n> +{\n> +\tstruct jtag *jtag = file->private_data;\n> +\n> +\tatomic_dec(&jtag->open);\n\nAgain, not needed.\n\n> +\treturn 0;\n> +}\n> +\n> +static const struct file_operations jtag_fops = {\n> +\t.owner\t\t= THIS_MODULE,\n> +\t.open\t\t= jtag_open,\n> +\t.release\t= jtag_release,\n> +\t.llseek\t\t= noop_llseek,\n> +\t.unlocked_ioctl = jtag_ioctl,\n> +#ifdef CONFIG_COMPAT\n> +\t.compat_ioctl\t= jtag_ioctl_compat,\n> +#endif\n> +};\n> +\n> +struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops)\n> +{\n> +\tstruct jtag *jtag;\n> +\n> +\tjtag = kzalloc(sizeof(*jtag) + round_up(priv_size, ARCH_DMA_MINALIGN),\n> +\t\t GFP_KERNEL);\n> +\tif (!jtag)\n> +\t\treturn NULL;\n> +\n> +\tjtag->ops = ops;\n> +\treturn jtag;\n> +}\n> +EXPORT_SYMBOL_GPL(jtag_alloc);\n\nregister should allocate and register the device, why pass a structure\nback that the caller then has to do something else with to use it?\n\n> +\n> +void jtag_free(struct jtag *jtag)\n> +{\n> +\tkfree(jtag);\n> +}\n> +EXPORT_SYMBOL_GPL(jtag_free);\n> +\n> +int jtag_register(struct jtag *jtag)\n> +{\n> +\tint id;\n> +\tint err;\n> +\n> +\tid = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);\n> +\tif (id < 0)\n> +\t\treturn id;\n> +\n> +\tjtag->id = id;\n> +\tcdev_init(&jtag->cdev, &jtag_fops);\n> +\tjtag->cdev.owner = THIS_MODULE;\n> +\terr = cdev_add(&jtag->cdev, MKDEV(MAJOR(jtag_devt), jtag->id), 1);\n> +\tif (err)\n> +\t\tgoto err_cdev;\n\nmisc device would be so much simpler and easier here :(\n\n\n> +\n> +\t/* Register this jtag device with the driver core */\n> +\tjtag->dev = device_create(&jtag_class, NULL, MKDEV(MAJOR(jtag_devt),\n> +\t\t\t\t\t\t\t jtag->id),\n> +\t\t\t\t NULL, \"jtag%d\", jtag->id);\n> +\tif (!jtag->dev)\n> +\t\tgoto err_device_create;\n> +\n> +\tatomic_set(&jtag->open, 0);\n> +\tdev_set_drvdata(jtag->dev, jtag);\n> +\treturn err;\n> +\n> +err_device_create:\n> +\tcdev_del(&jtag->cdev);\n> +err_cdev:\n> +\tida_simple_remove(&jtag_ida, id);\n> +\treturn err;\n> +}\n> +EXPORT_SYMBOL_GPL(jtag_register);\n> +\n> +void jtag_unregister(struct jtag *jtag)\n> +{\n> +\tstruct device *dev = jtag->dev;\n> +\n> +\tcdev_del(&jtag->cdev);\n> +\tdevice_unregister(dev);\n> +\tida_simple_remove(&jtag_ida, jtag->id);\n> +}\n> +EXPORT_SYMBOL_GPL(jtag_unregister);\n> +\n> +static int __init jtag_init(void)\n> +{\n> +\tint err;\n> +\n> +\terr = alloc_chrdev_region(&jtag_devt, JTAG_FIRST_MINOR,\n> +\t\t\t\t JTAG_MAX_DEVICES, \"jtag\");\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\terr = class_register(&jtag_class);\n> +\tif (err)\n> +\t\tunregister_chrdev_region(jtag_devt, JTAG_MAX_DEVICES);\n> +\n> +\treturn err;\n> +}\n> +\n> +static void __exit jtag_exit(void)\n> +{\n> +\tclass_unregister(&jtag_class);\n> +\tunregister_chrdev_region(jtag_devt, JTAG_MAX_DEVICES);\n\nYour idr leaked memory :(\n\n\n\n> +}\n> +\n> +module_init(jtag_init);\n> +module_exit(jtag_exit);\n> +\n> +MODULE_AUTHOR(\"Oleksandr Shamray \");\n> +MODULE_DESCRIPTION(\"Generic jtag support\");\n> +MODULE_LICENSE(\"GPL v2\");\n> diff --git a/include/linux/jtag.h b/include/linux/jtag.h\n> new file mode 100644\n> index 0000000..c016fed\n> --- /dev/null\n> +++ b/include/linux/jtag.h\n> @@ -0,0 +1,48 @@\n> +/*\n> + * drivers/jtag/jtag.c\n> + *\n> + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.\n> + * Copyright (c) 2017 Oleksandr Shamray \n> + *\n> + * Released under the GPLv2 only.\n> + * SPDX-License-Identifier: GPL-2.0\n> + */\n> +\n> +#ifndef __JTAG_H\n> +#define __JTAG_H\n> +\n> +#include \n> +\n> +#ifndef ARCH_DMA_MINALIGN\n> +#define ARCH_DMA_MINALIGN 1\n> +#endif\n> +\n> +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)\n> +\n> +#define JTAG_MAX_XFER_DATA_LEN 65535\n> +\n> +struct jtag;\n> +/**\n> + * struct jtag_ops - callbacks for jtag control functions:\n> + *\n> + * @freq_get: get frequency function. Filled by device driver\n> + * @freq_set: set frequency function. Filled by device driver\n> + * @status_get: set status function. Filled by device driver\n> + * @idle: set JTAG to idle state function. Filled by device driver\n> + * @xfer: send JTAG xfer function. Filled by device driver\n> + */\n> +struct jtag_ops {\n> +\tint (*freq_get)(struct jtag *jtag, u32 *freq);\n> +\tint (*freq_set)(struct jtag *jtag, u32 freq);\n> +\tint (*status_get)(struct jtag *jtag, u32 *state);\n> +\tint (*idle)(struct jtag *jtag, struct jtag_run_test_idle *idle);\n> +\tint (*xfer)(struct jtag *jtag, struct jtag_xfer *xfer);\n> +};\n> +\n> +void *jtag_priv(struct jtag *jtag);\n> +int jtag_register(struct jtag *jtag);\n> +void jtag_unregister(struct jtag *jtag);\n> +struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops);\n> +void jtag_free(struct jtag *jtag);\n> +\n> +#endif /* __JTAG_H */\n> diff --git a/include/uapi/linux/jtag.h b/include/uapi/linux/jtag.h\n> new file mode 100644\n> index 0000000..180bf22\n> --- /dev/null\n> +++ b/include/uapi/linux/jtag.h\n> @@ -0,0 +1,115 @@\n> +/*\n> + * JTAG class driver\n> + *\n> + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.\n> + * Copyright (c) 2017 Oleksandr Shamray \n> + *\n> + * Released under the GPLv2/BSD.\n> + * SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause\n> + */\n> +\n> +#ifndef __UAPI_LINUX_JTAG_H\n> +#define __UAPI_LINUX_JTAG_H\n> +\n> +/**\n> + * enum jtag_xfer_mode:\n> + *\n> + * @JTAG_XFER_HW_MODE: hardware mode transfer\n> + * @JTAG_XFER_SW_MODE: software mode transfer\n> + */\n> +enum jtag_xfer_mode {\n> +\tJTAG_XFER_HW_MODE,\n> +\tJTAG_XFER_SW_MODE,\n> +};\n> +\n> +/**\n> + * enum jtag_endstate:\n> + *\n> + * @JTAG_STATE_IDLE: JTAG state machine IDLE state\n> + * @JTAG_STATE_PAUSEIR: JTAG state machine PAUSE_IR state\n> + * @JTAG_STATE_PAUSEDR: JTAG state machine PAUSE_DR state\n> + */\n> +enum jtag_endstate {\n> +\tJTAG_STATE_IDLE,\n> +\tJTAG_STATE_PAUSEIR,\n> +\tJTAG_STATE_PAUSEDR,\n> +};\n> +\n> +/**\n> + * enum jtag_xfer_type:\n> + *\n> + * @JTAG_SIR_XFER: SIR transfer\n> + * @JTAG_SDR_XFER: SDR transfer\n> + */\n> +enum jtag_xfer_type {\n> +\tJTAG_SIR_XFER,\n> +\tJTAG_SDR_XFER,\n> +};\n> +\n> +/**\n> + * enum jtag_xfer_direction:\n> + *\n> + * @JTAG_READ_XFER: read transfer\n> + * @JTAG_WRITE_XFER: write transfer\n> + */\n> +enum jtag_xfer_direction {\n> +\tJTAG_READ_XFER,\n> +\tJTAG_WRITE_XFER,\n> +};\n> +\n> +/**\n> + * struct jtag_run_test_idle - forces JTAG state machine to\n> + * RUN_TEST/IDLE state\n> + *\n> + * @mode: access mode\n> + * @reset: 0 - run IDLE/PAUSE from current state\n> + * 1 - go through TEST_LOGIC/RESET state before IDLE/PAUSE\n> + * @end: completion flag\n> + * @tck: clock counter\n> + *\n> + * Structure represents interface to JTAG device for jtag idle\n> + * execution.\n> + */\n> +struct jtag_run_test_idle {\n> +\t__u8\tmode;\n> +\t__u8\treset;\n> +\t__u8\tendstate;\n> +\t__u8\ttck;\n> +};\n> +\n> +/**\n> + * struct jtag_xfer - jtag xfer:\n> + *\n> + * @mode: access mode\n> + * @type: transfer type\n> + * @direction: xfer direction\n> + * @length: xfer bits len\n> + * @tdio : xfer data array\n> + * @endir: xfer end state\n> + *\n> + * Structure represents interface to Aspeed JTAG device for jtag sdr xfer\n> + * execution.\n> + */\n> +struct jtag_xfer {\n> +\t__u8\tmode;\n> +\t__u8\ttype;\n> +\t__u8\tdirection;\n> +\t__u8\tendstate;\n> +\t__u32\tlength;\n> +\t__u64\ttdio;\n\nI don't see anything odd here that requires a compat ioctl callback, do\nyou?\n\nthanks,\n\ngreg k-h","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yJPPj4BkXz9t69\n\tfor ;\n\tFri, 20 Oct 2017 22:55:37 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yJPPj3BrCzDqKX\n\tfor ;\n\tFri, 20 Oct 2017 22:55:37 +1100 (AEDT)","from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yJPP73dh2zDq5W\n\tfor ; Fri, 20 Oct 2017 22:55:07 +1100 (AEDT)","from localhost (LFbn-1-12253-150.w90-92.abo.wanadoo.fr\n\t[90.92.67.150])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPSA id C24B6BD9;\n\tFri, 20 Oct 2017 11:55:04 +0000 (UTC)"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=linuxfoundation.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=gregkh@linuxfoundation.org; receiver=)","Date":"Fri, 20 Oct 2017 13:55:12 +0200","From":"Greg KH ","To":"Oleksandr Shamray ","Subject":"Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver","Message-ID":"<20171020115512.GA2073@kroah.com>","References":"<1505985932-27568-1-git-send-email-oleksandrs@mellanox.com>\n\t<1505985932-27568-2-git-send-email-oleksandrs@mellanox.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1505985932-27568-2-git-send-email-oleksandrs@mellanox.com>","User-Agent":"Mutt/1.9.1 (2017-09-22)","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"system-sw-low-level@mellanox.com, devicetree@vger.kernel.org,\n\tjiri@resnulli.us, arnd@arndb.de, vadimp@mellanox.com,\n\tlinux-api@vger.kernel.org, openbmc@lists.ozlabs.org,\n\tlinux-kernel@vger.kernel.org, openocd-devel-owner@lists.sourceforge.net, \n\tmec@shout.net, Jiri Pirko , robh+dt@kernel.org,\n\tlinux-serial@vger.kernel.org, tklauser@distanz.ch, mchehab@kernel.org,\n\tdavem@davemloft.net, linux-arm-kernel@lists.infradead.org","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1791516,"web_url":"http://patchwork.ozlabs.org/comment/1791516/","msgid":"","list_archive_url":null,"date":"2017-10-20T14:34:00","subject":"RE: [patch v9 1/4] drivers: jtag: Add JTAG core driver","submitter":{"id":72099,"url":"http://patchwork.ozlabs.org/api/people/72099/","name":"Oleksandr Shamray","email":"oleksandrs@mellanox.com"},"content":"Hi Greg.\r\n\r\n> -----Original Message-----\r\n> From: Greg KH [mailto:gregkh@linuxfoundation.org]\r\n> Sent: Friday, October 20, 2017 2:55 PM\r\n> To: Oleksandr Shamray \r\n> Cc: arnd@arndb.de; linux-kernel@vger.kernel.org; linux-arm-\r\n> kernel@lists.infradead.org; devicetree@vger.kernel.org;\r\n> openbmc@lists.ozlabs.org; joel@jms.id.au; jiri@resnulli.us;\r\n> tklauser@distanz.ch; linux-serial@vger.kernel.org; mec@shout.net; Vadim\r\n> Pasternak ; system-sw-low-level level@mellanox.com>; robh+dt@kernel.org; openocd-devel-\r\n> owner@lists.sourceforge.net; linux-api@vger.kernel.org;\r\n> davem@davemloft.net; mchehab@kernel.org; Jiri Pirko \r\n> Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver\r\n> \r\n> On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:\r\n> > +struct jtag {\r\n> > +\tstruct device *dev;\r\n> > +\tstruct cdev cdev;\r\n> \r\n> Why are you using a cdev here and not just a normal misc device? \r\n\r\nWhat the benefits to use misc instead of cdev?\r\n\r\n> I forgot if this is what you were doing before, sorry...\r\n> \r\n> > +\tint id;\r\n> > +\tatomic_t open;\r\n> \r\n> Why do you need this?\r\n\r\nThis counter used to avoid open at the same time by 2 or more users.\r\n> \r\n> > +\tconst struct jtag_ops *ops;\r\n> > +\tunsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);\r\n> \r\n> Huh? Why is this needed to be dma aligned? Why not just use the private\r\n> pointer in struct device?\r\n> \r\n\r\nIt is critical?\r\n\r\n> > +};\r\n> > +\r\n> > +static dev_t jtag_devt;\r\n> > +static DEFINE_IDA(jtag_ida);\r\n> > +\r\n> > +void *jtag_priv(struct jtag *jtag)\r\n> > +{\r\n> > +\treturn jtag->priv;\r\n> > +}\r\n> > +EXPORT_SYMBOL_GPL(jtag_priv);\r\n> > +\r\n> > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {\r\n> > +\tunsigned long size;\r\n> > +\tvoid *kdata;\r\n> > +\r\n> > +\tsize = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\r\n> > +\tkdata = memdup_user(u64_to_user_ptr(udata), size);\r\n> \r\n> You only use this once, why not just open-code it?\r\n\r\nI think it makes code more understandable.\r\n\r\n> \r\n> > +\r\n> > +\treturn kdata;\r\n> > +}\r\n> > +\r\n> > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,\r\n> > +\t\t\t\t unsigned long bit_size)\r\n> > +{\r\n> > +\tunsigned long size;\r\n> > +\r\n> > +\tsize = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\r\n> > +\r\n> > +\treturn copy_to_user(u64_to_user_ptr(udata), (void *)(kdata), size);\r\n> \r\n> Same here, making this a separate function seems odd.\r\n\r\nSame, I think it makes code more understandable.\r\n\r\n> \r\n> > +}\r\n> > +\r\n> > +static struct class jtag_class = {\r\n> > +\t.name = \"jtag\",\r\n> > +\t.owner = THIS_MODULE,\r\n> > +};\r\n> > +\r\n> > +static int jtag_run_test_idle_op(struct jtag *jtag,\r\n> > +\t\t\t\t struct jtag_run_test_idle *idle) {\r\n> > +\tif (jtag->ops->idle)\r\n> > +\t\treturn jtag->ops->idle(jtag, idle);\r\n> > +\telse\r\n> > +\t\treturn -EOPNOTSUPP;\r\n> \r\n> Why is this a function? Why not just do this work in the ioctl?\r\n\r\nAgree. I will move it just to ioctl function body.\r\n\r\n> \r\n> > +}\r\n> > +\r\n> > +static int jtag_xfer_op(struct jtag *jtag, struct jtag_xfer *xfer, u8\r\n> > +*data) {\r\n> > +\tif (jtag->ops->xfer)\r\n> > +\t\treturn jtag->ops->xfer(jtag, xfer, data);\r\n> > +\telse\r\n> > +\t\treturn -EOPNOTSUPP;\r\n> \r\n> Same here, doesn't need to be a function.\r\n\r\nAgree.\r\n\r\n> \r\n> > +}\r\n> > +\r\n> > +static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned\r\n> > +long arg) {\r\n> > +\tstruct jtag *jtag = file->private_data;\r\n> > +\tstruct jtag_run_test_idle idle;\r\n> > +\tstruct jtag_xfer xfer;\r\n> > +\tu8 *xfer_data;\r\n> > +\tu32 value;\r\n> > +\tint err;\r\n> > +\r\n> > +\tswitch (cmd) {\r\n> > +\tcase JTAG_GIOCFREQ:\r\n> > +\t\tif (jtag->ops->freq_get)\r\n> > +\t\t\terr = jtag->ops->freq_get(jtag, &value);\r\n> > +\t\telse\r\n> > +\t\t\terr = -EOPNOTSUPP;\r\n> > +\t\tif (err)\r\n> > +\t\t\tbreak;\r\n> > +\r\n> > +\t\terr = put_user(value, (__u32 *)arg);\r\n> > +\t\tbreak;\r\n> \r\n> Are you sure the return value of put_user is correct here? Shouldn't you return\r\n> -EFAULT if err is not 0?\r\n\r\nYes, thanks for point of this.\r\n\r\n> \r\n> > +\r\n> > +\tcase JTAG_SIOCFREQ:\r\n> > +\t\terr = get_user(value, (__u32 *)arg);\r\n> > +\r\n> \r\n> why a blank line?\r\n\r\nWill remove\r\n\r\n> \r\n> > +\t\tif (value == 0)\r\n> > +\t\t\terr = -EINVAL;\r\n> \r\n> Check err first.\r\n\r\nOk.\r\n\r\n> \r\n> > +\t\tif (err)\r\n> > +\t\t\tbreak;\r\n> \r\n> And set it properly, again err should be -EFAULT, right?\r\n\r\nRight 😊0\r\n\r\n> \r\n> > +\r\n> > +\t\tif (jtag->ops->freq_set)\r\n> > +\t\t\terr = jtag->ops->freq_set(jtag, value);\r\n> > +\t\telse\r\n> > +\t\t\terr = -EOPNOTSUPP;\r\n> > +\t\tbreak;\r\n> > +\r\n> > +\tcase JTAG_IOCRUNTEST:\r\n> > +\t\tif (copy_from_user(&idle, (void *)arg,\r\n> > +\t\t\t\t sizeof(struct jtag_run_test_idle)))\r\n> > +\t\t\treturn -ENOMEM;\r\n> > +\t\terr = jtag_run_test_idle_op(jtag, &idle);\r\n> \r\n> Who validates the structure fields? Is that up to the individual jtag driver? Why\r\n> not do it in the core correctly so that it only has to be done in one place and you\r\n> do not have to audit every individual driver?\r\n\r\nInput parameters validated by jtag platform driver. I think it not critical.\r\n\r\n> \r\n> > +\t\tbreak;\r\n> > +\r\n> > +\tcase JTAG_IOCXFER:\r\n> > +\t\tif (copy_from_user(&xfer, (void *)arg,\r\n> > +\t\t\t\t sizeof(struct jtag_xfer)))\r\n> > +\t\t\treturn -EFAULT;\r\n> > +\r\n> > +\t\tif (xfer.length >= JTAG_MAX_XFER_DATA_LEN)\r\n> > +\t\t\treturn -EFAULT;\r\n> > +\r\n> > +\t\txfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);\r\n> > +\t\tif (!xfer_data)\r\n> > +\t\t\treturn -ENOMEM;\r\n> \r\n> Are you sure that's the correct error value?\r\n\r\nI think yes, but what you suggest?\r\n\r\n> \r\n> > +\r\n> > +\t\terr = jtag_xfer_op(jtag, &xfer, xfer_data);\r\n> > +\t\tif (jtag_copy_to_user(xfer.tdio, xfer_data, xfer.length)) {\r\n> > +\t\t\tkfree(xfer_data);\r\n> > +\t\t\treturn -EFAULT;\r\n> > +\t\t}\r\n> > +\r\n> > +\t\tkfree(xfer_data);\r\n> > +\t\tif (copy_to_user((void *)arg, &xfer, sizeof(struct jtag_xfer)))\r\n> > +\t\t\treturn -EFAULT;\r\n> > +\t\tbreak;\r\n> > +\r\n> > +\tcase JTAG_GIOCSTATUS:\r\n> > +\t\tif (jtag->ops->status_get)\r\n> > +\t\t\terr = jtag->ops->status_get(jtag, &value);\r\n> > +\t\telse\r\n> > +\t\t\terr = -EOPNOTSUPP;\r\n> > +\t\tif (err)\r\n> > +\t\t\tbreak;\r\n> > +\r\n> > +\t\terr = put_user(value, (__u32 *)arg);\r\n> \r\n> Same put_user err issue here.\r\n\r\nYes.\r\n\r\n> \r\n> > +\t\tbreak;\r\n> > +\r\n> > +\tdefault:\r\n> > +\t\treturn -EINVAL;\r\n> > +\t}\r\n> > +\treturn err;\r\n> > +}\r\n> > +\r\n> > +#ifdef CONFIG_COMPAT\r\n> > +static long jtag_ioctl_compat(struct file *file, unsigned int cmd,\r\n> > +\t\t\t unsigned long arg)\r\n> > +{\r\n> > +\treturn jtag_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); }\r\n> > +#endif\r\n> \r\n> Why do you need a compat callback for a new ioctl? Create your structures\r\n> properly and this should not be needed, right?\r\n\r\n\r\nI does this because Arnd (arndbergmann@gmail.com) writed in review (On Wed, Aug 2, 2017 at 3:18 PM)\r\n\r\n[..]\r\n> + .unlocked_ioctl = jtag_ioctl,\r\n> + .open = jtag_open,\r\n> + .release = jtag_release,\r\n> +};\r\n\r\nadd a compat_ioctl pointer here, after ensuring that all ioctl commands are compatible between 32-bit and 64-bit user space.\r\n[..]\r\n\r\n\r\n> \r\n> > +\r\n> > +static int jtag_open(struct inode *inode, struct file *file) {\r\n> > +\tstruct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);\r\n> > +\r\n> > +\tif (atomic_read(&jtag->open)) {\r\n> > +\t\tdev_info(NULL, \"jtag already opened\\n\");\r\n> > +\t\treturn -EBUSY;\r\n> \r\n> Why do you care if multiple opens can happen?\r\n\r\nJtag HW not support to using with multiple requests from different users. So we prohibit this.\r\n\r\n> \r\n> > +\t}\r\n> > +\r\n> > +\tatomic_inc(&jtag->open);\r\n> \r\n> Oh look, you just raced with two different people opening at the same time :(\r\n> \r\n> An atomic value is never a replacement for a lock, sorry.\r\n> \r\n> > +\tfile->private_data = jtag;\r\n> > +\treturn 0;\r\n> > +}\r\n> > +\r\n> > +static int jtag_release(struct inode *inode, struct file *file) {\r\n> > +\tstruct jtag *jtag = file->private_data;\r\n> > +\r\n> > +\tatomic_dec(&jtag->open);\r\n> \r\n> Again, not needed.\r\n> \r\n> > +\treturn 0;\r\n> > +}\r\n> > +\r\n> > +static const struct file_operations jtag_fops = {\r\n> > +\t.owner\t\t= THIS_MODULE,\r\n> > +\t.open\t\t= jtag_open,\r\n> > +\t.release\t= jtag_release,\r\n> > +\t.llseek\t\t= noop_llseek,\r\n> > +\t.unlocked_ioctl = jtag_ioctl,\r\n> > +#ifdef CONFIG_COMPAT\r\n> > +\t.compat_ioctl\t= jtag_ioctl_compat,\r\n> > +#endif\r\n> > +};\r\n> > +\r\n> > +struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops)\r\n> > +{\r\n> > +\tstruct jtag *jtag;\r\n> > +\r\n> > +\tjtag = kzalloc(sizeof(*jtag) + round_up(priv_size,\r\n> ARCH_DMA_MINALIGN),\r\n> > +\t\t GFP_KERNEL);\r\n> > +\tif (!jtag)\r\n> > +\t\treturn NULL;\r\n> > +\r\n> > +\tjtag->ops = ops;\r\n> > +\treturn jtag;\r\n> > +}\r\n> > +EXPORT_SYMBOL_GPL(jtag_alloc);\r\n> \r\n> register should allocate and register the device, why pass a structure back that\r\n> the caller then has to do something else with to use it?\r\n> \r\n\r\nBefore registration we need to initialize JTAG device HW registers and fill private data with HW specific parameters.\r\nAnd is I see in other Linux drivers it is a common way to separate allocation device and register it\r\n\r\n> > +\r\n> > +void jtag_free(struct jtag *jtag)\r\n> > +{\r\n> > +\tkfree(jtag);\r\n> > +}\r\n> > +EXPORT_SYMBOL_GPL(jtag_free);\r\n> > +\r\n> > +int jtag_register(struct jtag *jtag)\r\n> > +{\r\n> > +\tint id;\r\n> > +\tint err;\r\n> > +\r\n> > +\tid = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);\r\n> > +\tif (id < 0)\r\n> > +\t\treturn id;\r\n> > +\r\n> > +\tjtag->id = id;\r\n> > +\tcdev_init(&jtag->cdev, &jtag_fops);\r\n> > +\tjtag->cdev.owner = THIS_MODULE;\r\n> > +\terr = cdev_add(&jtag->cdev, MKDEV(MAJOR(jtag_devt), jtag->id), 1);\r\n> > +\tif (err)\r\n> > +\t\tgoto err_cdev;\r\n> \r\n> misc device would be so much simpler and easier here :(\r\n> \r\n> \r\n> > +\r\n> > +\t/* Register this jtag device with the driver core */\r\n> > +\tjtag->dev = device_create(&jtag_class, NULL,\r\n> MKDEV(MAJOR(jtag_devt),\r\n> > +\t\t\t\t\t\t\t jtag->id),\r\n> > +\t\t\t\t NULL, \"jtag%d\", jtag->id);\r\n> > +\tif (!jtag->dev)\r\n> > +\t\tgoto err_device_create;\r\n> > +\r\n> > +\tatomic_set(&jtag->open, 0);\r\n> > +\tdev_set_drvdata(jtag->dev, jtag);\r\n> > +\treturn err;\r\n> > +\r\n> > +err_device_create:\r\n> > +\tcdev_del(&jtag->cdev);\r\n> > +err_cdev:\r\n> > +\tida_simple_remove(&jtag_ida, id);\r\n> > +\treturn err;\r\n> > +}\r\n> > +EXPORT_SYMBOL_GPL(jtag_register);\r\n> > +\r\n> > +void jtag_unregister(struct jtag *jtag) {\r\n> > +\tstruct device *dev = jtag->dev;\r\n> > +\r\n> > +\tcdev_del(&jtag->cdev);\r\n> > +\tdevice_unregister(dev);\r\n> > +\tida_simple_remove(&jtag_ida, jtag->id); }\r\n> > +EXPORT_SYMBOL_GPL(jtag_unregister);\r\n> > +\r\n> > +static int __init jtag_init(void)\r\n> > +{\r\n> > +\tint err;\r\n> > +\r\n> > +\terr = alloc_chrdev_region(&jtag_devt, JTAG_FIRST_MINOR,\r\n> > +\t\t\t\t JTAG_MAX_DEVICES, \"jtag\");\r\n> > +\tif (err)\r\n> > +\t\treturn err;\r\n> > +\r\n> > +\terr = class_register(&jtag_class);\r\n> > +\tif (err)\r\n> > +\t\tunregister_chrdev_region(jtag_devt, JTAG_MAX_DEVICES);\r\n> > +\r\n> > +\treturn err;\r\n> > +}\r\n> > +\r\n> > +static void __exit jtag_exit(void)\r\n> > +{\r\n> > +\tclass_unregister(&jtag_class);\r\n> > +\tunregister_chrdev_region(jtag_devt, JTAG_MAX_DEVICES);\r\n> \r\n> Your idr leaked memory :(\r\n> \r\n\r\nYes I will add ida_destroy() here.\r\n> \r\n> \r\n> > +}\r\n> > +\r\n> > +module_init(jtag_init);\r\n> > +module_exit(jtag_exit);\r\n> > +\r\n> > +MODULE_AUTHOR(\"Oleksandr Shamray \");\r\n> > +MODULE_DESCRIPTION(\"Generic jtag support\"); MODULE_LICENSE(\"GPL\r\n> v2\");\r\n> > diff --git a/include/linux/jtag.h b/include/linux/jtag.h new file mode\r\n> > 100644 index 0000000..c016fed\r\n> > --- /dev/null\r\n> > +++ b/include/linux/jtag.h\r\n> > @@ -0,0 +1,48 @@\r\n> > +/*\r\n> > + * drivers/jtag/jtag.c\r\n> > + *\r\n> > + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.\r\n> > + * Copyright (c) 2017 Oleksandr Shamray \r\n> > + *\r\n> > + * Released under the GPLv2 only.\r\n> > + * SPDX-License-Identifier: GPL-2.0\r\n> > + */\r\n> > +\r\n> > +#ifndef __JTAG_H\r\n> > +#define __JTAG_H\r\n> > +\r\n> > +#include \r\n> > +\r\n> > +#ifndef ARCH_DMA_MINALIGN\r\n> > +#define ARCH_DMA_MINALIGN 1\r\n> > +#endif\r\n> > +\r\n> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)\r\n> > +\r\n> > +#define JTAG_MAX_XFER_DATA_LEN 65535\r\n> > +\r\n> > +struct jtag;\r\n> > +/**\r\n> > + * struct jtag_ops - callbacks for jtag control functions:\r\n> > + *\r\n> > + * @freq_get: get frequency function. Filled by device driver\r\n> > + * @freq_set: set frequency function. Filled by device driver\r\n> > + * @status_get: set status function. Filled by device driver\r\n> > + * @idle: set JTAG to idle state function. Filled by device driver\r\n> > + * @xfer: send JTAG xfer function. Filled by device driver */ struct\r\n> > +jtag_ops {\r\n> > +\tint (*freq_get)(struct jtag *jtag, u32 *freq);\r\n> > +\tint (*freq_set)(struct jtag *jtag, u32 freq);\r\n> > +\tint (*status_get)(struct jtag *jtag, u32 *state);\r\n> > +\tint (*idle)(struct jtag *jtag, struct jtag_run_test_idle *idle);\r\n> > +\tint (*xfer)(struct jtag *jtag, struct jtag_xfer *xfer); };\r\n> > +\r\n> > +void *jtag_priv(struct jtag *jtag);\r\n> > +int jtag_register(struct jtag *jtag); void jtag_unregister(struct\r\n> > +jtag *jtag); struct jtag *jtag_alloc(size_t priv_size, const struct\r\n> > +jtag_ops *ops); void jtag_free(struct jtag *jtag);\r\n> > +\r\n> > +#endif /* __JTAG_H */\r\n> > diff --git a/include/uapi/linux/jtag.h b/include/uapi/linux/jtag.h new\r\n> > file mode 100644 index 0000000..180bf22\r\n> > --- /dev/null\r\n> > +++ b/include/uapi/linux/jtag.h\r\n> > @@ -0,0 +1,115 @@\r\n> > +/*\r\n> > + * JTAG class driver\r\n> > + *\r\n> > + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.\r\n> > + * Copyright (c) 2017 Oleksandr Shamray \r\n> > + *\r\n> > + * Released under the GPLv2/BSD.\r\n> > + * SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */\r\n> > +\r\n> > +#ifndef __UAPI_LINUX_JTAG_H\r\n> > +#define __UAPI_LINUX_JTAG_H\r\n> > +\r\n> > +/**\r\n> > + * enum jtag_xfer_mode:\r\n> > + *\r\n> > + * @JTAG_XFER_HW_MODE: hardware mode transfer\r\n> > + * @JTAG_XFER_SW_MODE: software mode transfer */ enum\r\n> jtag_xfer_mode\r\n> > +{\r\n> > +\tJTAG_XFER_HW_MODE,\r\n> > +\tJTAG_XFER_SW_MODE,\r\n> > +};\r\n> > +\r\n> > +/**\r\n> > + * enum jtag_endstate:\r\n> > + *\r\n> > + * @JTAG_STATE_IDLE: JTAG state machine IDLE state\r\n> > + * @JTAG_STATE_PAUSEIR: JTAG state machine PAUSE_IR state\r\n> > + * @JTAG_STATE_PAUSEDR: JTAG state machine PAUSE_DR state */ enum\r\n> > +jtag_endstate {\r\n> > +\tJTAG_STATE_IDLE,\r\n> > +\tJTAG_STATE_PAUSEIR,\r\n> > +\tJTAG_STATE_PAUSEDR,\r\n> > +};\r\n> > +\r\n> > +/**\r\n> > + * enum jtag_xfer_type:\r\n> > + *\r\n> > + * @JTAG_SIR_XFER: SIR transfer\r\n> > + * @JTAG_SDR_XFER: SDR transfer\r\n> > + */\r\n> > +enum jtag_xfer_type {\r\n> > +\tJTAG_SIR_XFER,\r\n> > +\tJTAG_SDR_XFER,\r\n> > +};\r\n> > +\r\n> > +/**\r\n> > + * enum jtag_xfer_direction:\r\n> > + *\r\n> > + * @JTAG_READ_XFER: read transfer\r\n> > + * @JTAG_WRITE_XFER: write transfer\r\n> > + */\r\n> > +enum jtag_xfer_direction {\r\n> > +\tJTAG_READ_XFER,\r\n> > +\tJTAG_WRITE_XFER,\r\n> > +};\r\n> > +\r\n> > +/**\r\n> > + * struct jtag_run_test_idle - forces JTAG state machine to\r\n> > + * RUN_TEST/IDLE state\r\n> > + *\r\n> > + * @mode: access mode\r\n> > + * @reset: 0 - run IDLE/PAUSE from current state\r\n> > + * 1 - go through TEST_LOGIC/RESET state before IDLE/PAUSE\r\n> > + * @end: completion flag\r\n> > + * @tck: clock counter\r\n> > + *\r\n> > + * Structure represents interface to JTAG device for jtag idle\r\n> > + * execution.\r\n> > + */\r\n> > +struct jtag_run_test_idle {\r\n> > +\t__u8\tmode;\r\n> > +\t__u8\treset;\r\n> > +\t__u8\tendstate;\r\n> > +\t__u8\ttck;\r\n> > +};\r\n> > +\r\n> > +/**\r\n> > + * struct jtag_xfer - jtag xfer:\r\n> > + *\r\n> > + * @mode: access mode\r\n> > + * @type: transfer type\r\n> > + * @direction: xfer direction\r\n> > + * @length: xfer bits len\r\n> > + * @tdio : xfer data array\r\n> > + * @endir: xfer end state\r\n> > + *\r\n> > + * Structure represents interface to Aspeed JTAG device for jtag sdr\r\n> > +xfer\r\n> > + * execution.\r\n> > + */\r\n> > +struct jtag_xfer {\r\n> > +\t__u8\tmode;\r\n> > +\t__u8\ttype;\r\n> > +\t__u8\tdirection;\r\n> > +\t__u8\tendstate;\r\n> > +\t__u32\tlength;\r\n> > +\t__u64\ttdio;\r\n> \r\n> I don't see anything odd here that requires a compat ioctl callback, do you?\r\n> \r\n> thanks,\r\n> \r\n> greg k-h\r\n\r\nThanks for your review.\r\n\r\nOleksandr S.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yJSxW25VDz9t38\n\tfor ;\n\tSat, 21 Oct 2017 01:34:55 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yJSxW0nYLzDqfh\n\tfor ;\n\tSat, 21 Oct 2017 01:34:55 +1100 (AEDT)","from EUR02-AM5-obe.outbound.protection.outlook.com\n\t(mail-eopbgr00064.outbound.protection.outlook.com [40.107.0.64])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yJSwc3LWXzDqMY\n\tfor ; Sat, 21 Oct 2017 01:34:07 +1100 (AEDT)","from AM4PR0501MB2194.eurprd05.prod.outlook.com (10.165.82.13) by\n\tAM4PR05MB3332.eurprd05.prod.outlook.com (10.171.187.17) with\n\tMicrosoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id\n\t15.20.77.7; Fri, 20 Oct 2017 14:34:00 +0000","from AM4PR0501MB2194.eurprd05.prod.outlook.com\n\t([fe80::c1dc:8bea:a5cb:bcb6]) by\n\tAM4PR0501MB2194.eurprd05.prod.outlook.com\n\t([fe80::c1dc:8bea:a5cb:bcb6%18]) with mapi id 15.20.0077.022;\n\tFri, 20 Oct 2017 14:34:00 +0000"],"Authentication-Results":["ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"p+fEfaeu\"; dkim-atps=neutral","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"p+fEfaeu\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=mellanox.com\n\t(client-ip=40.107.0.64;\n\thelo=eur02-am5-obe.outbound.protection.outlook.com; \n\tenvelope-from=oleksandrs@mellanox.com; receiver=)","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"p+fEfaeu\"; dkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=oleksandrs@mellanox.com; "],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com;\n\ts=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version;\n\tbh=HR1pTXq4Dal8z/HiduDmP0ZZBg6Jb8dVJHKiLGvdksc=;\n\tb=p+fEfaeupFSWn66f+7n3EaYvf+yRaTfBV9JgbYM26RFnGRl5PvVNEv430YsUSUsYvsuDM/zK1FZtqOR4FqMgsg9hDyu9M3y9RR3QKQIbLvG2XlkxxqLodfXSwHldbIna5/3sE48btHgMm7dJi6Da4+S/C2m/wtWyQ2stZ/g/Qho=","From":"Oleksandr Shamray ","To":"Greg KH ","Subject":"RE: [patch v9 1/4] drivers: jtag: Add JTAG core driver","Thread-Topic":"[patch v9 1/4] drivers: jtag: Add JTAG core driver","Thread-Index":"AQHTMru5yoecjoOKkEeYBkPR/vITOKLszxoAgAAGBCA=","Date":"Fri, 20 Oct 2017 14:34:00 +0000","Message-ID":"","References":"<1505985932-27568-1-git-send-email-oleksandrs@mellanox.com>\n\t<1505985932-27568-2-git-send-email-oleksandrs@mellanox.com>\n\t<20171020115512.GA2073@kroah.com>","In-Reply-To":"<20171020115512.GA2073@kroah.com>","Accept-Language":"en-US, uk-UA","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","authentication-results":["ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"p+fEfaeu\"; dkim-atps=neutral","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"p+fEfaeu\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=mellanox.com\n\t(client-ip=40.107.0.64;\n\thelo=eur02-am5-obe.outbound.protection.outlook.com; \n\tenvelope-from=oleksandrs@mellanox.com; receiver=)","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"p+fEfaeu\"; dkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=oleksandrs@mellanox.com; "],"x-originating-ip":"[80.90.224.13]","x-ms-publictraffictype":"Email","x-microsoft-exchange-diagnostics":"1; AM4PR05MB3332;\n\t6:xThJeGAGOkps+pu5h8Fs3xCeKMi1+LbezWtcZUeiHmsN9dLtAaTcNCbudO5H9+GifhY6cYjXeHHkk6b30R6ZtQ2h2Dcq8L98DSFuuwwjheVxMJVBW8kpveP+2z6buONMqtOKTfLcA/xtt2zLS2cspRPkirilNlhxsdAvlr70McM+cxCmwJ8A1LZsGM2YIgqfxDFSkdxXUojLGvQZx26mbw9109XW2P8BVfQVBKyHL23oa1tG+77GOb/IPLD8hIJQ+i1JZEpbhUBygCo62P84XyBf8IxwmYlxL04WBEccg63dgKGGFjqPgYtX7Oyovs784MFPiUx1LLiguoR1lbJ0TQ==;\n\t5:0ogXv3skV4Sx24eDB7nGW7xLIZataVqJMHAjpiMvAinyc/zeIAMffrzaXTv7hfllIWtMrQ9CODD9PMKhiLdWI3SMX4xFvbEu4pXw4rWjzAs9kG45XwljPMnw85tmQTO/Pky9Ro2YYdrUUxD3erD1qQ==;\n\t24:i2g7bdbKirfEHjedrJXNTLz0B7BELLnM8DElonEaP7TTf65FjTR6sm+krEXvMo8X71wdO1GpVxSP92yrlCGdsePqy30fYT+TwRJTjv7HTYM=;\n\t7:yBLUR0jgGYi8y9TGdT/orlonRpeM6jTxDv6yTB3SnYAX16Gb3IzNjUNzlcPSyG1cosjflHYzjpUB/RqNZr95kNwIrdIMha46cYtRXll6fdVPOyMKoJtO36MIBxsdRQZ8Ap2lRBlhY2WiRUXP2uobyfTFKSb/B7eFOwgT4lDuUgyrlehUFfEqfyGSvAqbHwbHXoQfaQ1ZduQj5WfIpJhEfj2oD5wjQ6LvA+GIq6pGJSo=","x-ms-exchange-antispam-srfa-diagnostics":"SSOS;SSOR;","x-ms-office365-filtering-correlation-id":"f33c2bf1-4afa-4bac-ca70-08d517c79b0a","x-ms-office365-filtering-ht":"Tenant","x-microsoft-antispam":"UriScan:; BCL:0; PCL:0;\n\tRULEID:(22001)(4534020)(4602075)(4627075)(201703031133081)(201702281549075)(48565401081)(2017052603199);\n\tSRVR:AM4PR05MB3332; ","x-ms-traffictypediagnostic":"AM4PR05MB3332:","x-exchange-antispam-report-test":"UriScan:(143289334528602)(9452136761055)(65623756079841)(258649278758335)(42262312472803);","x-microsoft-antispam-prvs":"","x-exchange-antispam-report-cfa-test":"BCL:0; PCL:0;\n\tRULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(5005006)(8121501046)(3002001)(10201501046)(100000703101)(100105400095)(3231020)(93006095)(93001095)(6055026)(6041248)(20161123560025)(20161123558100)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123564025)(20161123562025)(20161123555025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);\n\tSRVR:AM4PR05MB3332; BCL:0; PCL:0;\n\tRULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);\n\tSRVR:AM4PR05MB3332; ","x-forefront-prvs":"0466CA5A45","x-forefront-antispam-report":"SFV:NSPM;\n\tSFS:(10009020)(6009001)(346002)(376002)(39860400002)(24454002)(199003)(189002)(13464003)(5250100002)(86362001)(3846002)(107886003)(55016002)(33656002)(106356001)(99286003)(3660700001)(74316002)(68736007)(6436002)(6246003)(189998001)(105586002)(4326008)(9686003)(6506006)(53936002)(305945005)(3280700002)(97736004)(8936002)(66066001)(53946003)(54906003)(8676002)(2900100001)(478600001)(102836003)(76176999)(316002)(101416001)(7736002)(229853002)(54356999)(2906002)(6116002)(2950100002)(6916009)(50986999)(81166006)(14454004)(7416002)(25786009)(53546010)(7696004)(5660300001)(81156014);\n\tDIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR05MB3332;\n\tH:AM4PR0501MB2194.eurprd05.prod.outlook.com; FPR:; SPF:None;\n\tPTR:InfoNoRecords; A:1; MX:1; LANG:en; ","received-spf":"None (protection.outlook.com: mellanox.com does not designate\n\tpermitted sender hosts)","spamdiagnosticoutput":"1:99","spamdiagnosticmetadata":"NSPM","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","MIME-Version":"1.0","X-OriginatorOrg":"Mellanox.com","X-MS-Exchange-CrossTenant-originalarrivaltime":"20 Oct 2017 14:34:00.8180\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"a652971c-7d2e-4d9b-a6a4-d149256f461b","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"AM4PR05MB3332","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"system-sw-low-level ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"jiri@resnulli.us\" , \"arnd@arndb.de\" ,\n\tVadim Pasternak ,\n\t\"linux-api@vger.kernel.org\" ,\n\t\"openbmc@lists.ozlabs.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\t\"openocd-devel-owner@lists.sourceforge.net\"\n\t,\n\t\"mec@shout.net\" , Jiri Pirko ,\n\t\"robh+dt@kernel.org\" , \n\t\"linux-serial@vger.kernel.org\" ,\n\t\"tklauser@distanz.ch\" ,\n\t\"mchehab@kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"linux-arm-kernel@lists.infradead.org\"\n\t","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1791526,"web_url":"http://patchwork.ozlabs.org/comment/1791526/","msgid":"<20171020145417.GD8965@kroah.com>","list_archive_url":null,"date":"2017-10-20T14:54:17","subject":"Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver","submitter":{"id":11800,"url":"http://patchwork.ozlabs.org/api/people/11800/","name":"Greg Kroah-Hartman","email":"gregkh@linuxfoundation.org"},"content":"On Fri, Oct 20, 2017 at 02:34:00PM +0000, Oleksandr Shamray wrote:\n> Hi Greg.\n> \n> > -----Original Message-----\n> > From: Greg KH [mailto:gregkh@linuxfoundation.org]\n> > Sent: Friday, October 20, 2017 2:55 PM\n> > To: Oleksandr Shamray \n> > Cc: arnd@arndb.de; linux-kernel@vger.kernel.org; linux-arm-\n> > kernel@lists.infradead.org; devicetree@vger.kernel.org;\n> > openbmc@lists.ozlabs.org; joel@jms.id.au; jiri@resnulli.us;\n> > tklauser@distanz.ch; linux-serial@vger.kernel.org; mec@shout.net; Vadim\n> > Pasternak ; system-sw-low-level > level@mellanox.com>; robh+dt@kernel.org; openocd-devel-\n> > owner@lists.sourceforge.net; linux-api@vger.kernel.org;\n> > davem@davemloft.net; mchehab@kernel.org; Jiri Pirko \n> > Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver\n> > \n> > On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:\n> > > +struct jtag {\n> > > +\tstruct device *dev;\n> > > +\tstruct cdev cdev;\n> > \n> > Why are you using a cdev here and not just a normal misc device? \n> \n> What the benefits to use misc instead of cdev?\n\nLess code, simpler logic, easier to review and understand, etc.\n\nLet me ask you, why use a cdev instead of a misc?\n\n> > I forgot if this is what you were doing before, sorry...\n> > \n> > > +\tint id;\n> > > +\tatomic_t open;\n> > \n> > Why do you need this?\n> \n> This counter used to avoid open at the same time by 2 or more users.\n\nBut it isn't working :)\n\nAnd why do you care?\n\n> > > +\tconst struct jtag_ops *ops;\n> > > +\tunsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);\n> > \n> > Huh? Why is this needed to be dma aligned? Why not just use the private\n> > pointer in struct device?\n> > \n> \n> It is critical?\n\nYou are saying it is, so you have to justify it. There is a pointer for\nyou to use, don't make new ones for no reason, right?\n\n> > > +};\n> > > +\n> > > +static dev_t jtag_devt;\n> > > +static DEFINE_IDA(jtag_ida);\n> > > +\n> > > +void *jtag_priv(struct jtag *jtag)\n> > > +{\n> > > +\treturn jtag->priv;\n> > > +}\n> > > +EXPORT_SYMBOL_GPL(jtag_priv);\n> > > +\n> > > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {\n> > > +\tunsigned long size;\n> > > +\tvoid *kdata;\n> > > +\n> > > +\tsize = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\n> > > +\tkdata = memdup_user(u64_to_user_ptr(udata), size);\n> > \n> > You only use this once, why not just open-code it?\n> \n> I think it makes code more understandable.\n\nAs a reviewer, I don't :)\n\n> > > +\n> > > +\treturn kdata;\n> > > +}\n> > > +\n> > > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,\n> > > +\t\t\t\t unsigned long bit_size)\n> > > +{\n> > > +\tunsigned long size;\n> > > +\n> > > +\tsize = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\n> > > +\n> > > +\treturn copy_to_user(u64_to_user_ptr(udata), (void *)(kdata), size);\n> > \n> > Same here, making this a separate function seems odd.\n> \n> Same, I think it makes code more understandable.\n\nBut it doesn't.\n\n> > > +\n> > > +\t\tif (jtag->ops->freq_set)\n> > > +\t\t\terr = jtag->ops->freq_set(jtag, value);\n> > > +\t\telse\n> > > +\t\t\terr = -EOPNOTSUPP;\n> > > +\t\tbreak;\n> > > +\n> > > +\tcase JTAG_IOCRUNTEST:\n> > > +\t\tif (copy_from_user(&idle, (void *)arg,\n> > > +\t\t\t\t sizeof(struct jtag_run_test_idle)))\n> > > +\t\t\treturn -ENOMEM;\n> > > +\t\terr = jtag_run_test_idle_op(jtag, &idle);\n> > \n> > Who validates the structure fields? Is that up to the individual jtag driver? Why\n> > not do it in the core correctly so that it only has to be done in one place and you\n> > do not have to audit every individual driver?\n> \n> Input parameters validated by jtag platform driver. I think it not critical.\n\nNot true at all. It is very critical. Remmeber, \"All Input Is Evil!\"\n\nYou have to validate this. I as a reviewer have to find where you are\nvalidating this data to ensure bad things do not happen. I can't review\nthat here, now I have to go and review all of the individual drivers,\nwhich is a major pain, don't you agree?\n\n> > > +\t\tbreak;\n> > > +\n> > > +\tcase JTAG_IOCXFER:\n> > > +\t\tif (copy_from_user(&xfer, (void *)arg,\n> > > +\t\t\t\t sizeof(struct jtag_xfer)))\n> > > +\t\t\treturn -EFAULT;\n> > > +\n> > > +\t\tif (xfer.length >= JTAG_MAX_XFER_DATA_LEN)\n> > > +\t\t\treturn -EFAULT;\n> > > +\n> > > +\t\txfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);\n> > > +\t\tif (!xfer_data)\n> > > +\t\t\treturn -ENOMEM;\n> > \n> > Are you sure that's the correct error value?\n> \n> I think yes, but what you suggest?\n\nA fault happened, so -EFAULT, right?\n\n> [..]\n> > + .unlocked_ioctl = jtag_ioctl,\n> > + .open = jtag_open,\n> > + .release = jtag_release,\n> > +};\n> \n> add a compat_ioctl pointer here, after ensuring that all ioctl\n> commands are compatible between 32-bit and 64-bit user space.\n> [..]\n\nAnd if you do not, what happens? You shouldn't need it as there is no\nfixups necessary, or am I mistaken about that?\n\n> > > +static int jtag_open(struct inode *inode, struct file *file) {\n> > > +\tstruct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);\n> > > +\n> > > +\tif (atomic_read(&jtag->open)) {\n> > > +\t\tdev_info(NULL, \"jtag already opened\\n\");\n> > > +\t\treturn -EBUSY;\n> > \n> > Why do you care if multiple opens can happen?\n> \n> Jtag HW not support to using with multiple requests from different users. So we prohibit this.\n\nWhy does the kernel care?\n\nAnd again, your implementation is broken, it's not actually doing this\nprotection. I recommend just not doing it at all, but if you really are\ninsisting on it, you have to get it correct :)\n\nthanks,\n\ngreg k-h","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yJTN35gdSz9t5q\n\tfor ;\n\tSat, 21 Oct 2017 01:54:27 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yJTN34tPGzDqYc\n\tfor ;\n\tSat, 21 Oct 2017 01:54:27 +1100 (AEDT)","from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yJTMm1qkvzDq5W\n\tfor ; Sat, 21 Oct 2017 01:54:11 +1100 (AEDT)","from localhost (LFbn-1-12253-150.w90-92.abo.wanadoo.fr\n\t[90.92.67.150])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPSA id C4D9C88A;\n\tFri, 20 Oct 2017 14:54:08 +0000 (UTC)"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=linuxfoundation.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=gregkh@linuxfoundation.org; receiver=)","Date":"Fri, 20 Oct 2017 16:54:17 +0200","From":"Greg KH ","To":"Oleksandr Shamray ","Subject":"Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver","Message-ID":"<20171020145417.GD8965@kroah.com>","References":"<1505985932-27568-1-git-send-email-oleksandrs@mellanox.com>\n\t<1505985932-27568-2-git-send-email-oleksandrs@mellanox.com>\n\t<20171020115512.GA2073@kroah.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.9.1 (2017-09-22)","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"system-sw-low-level ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"jiri@resnulli.us\" , \"arnd@arndb.de\" ,\n\tVadim Pasternak ,\n\t\"linux-api@vger.kernel.org\" ,\n\t\"openbmc@lists.ozlabs.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\t\"openocd-devel-owner@lists.sourceforge.net\"\n\t,\n\t\"mec@shout.net\" , Jiri Pirko ,\n\t\"robh+dt@kernel.org\" , \n\t\"linux-serial@vger.kernel.org\" ,\n\t\"tklauser@distanz.ch\" ,\n\t\"mchehab@kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"linux-arm-kernel@lists.infradead.org\"\n\t","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1793828,"web_url":"http://patchwork.ozlabs.org/comment/1793828/","msgid":"","list_archive_url":null,"date":"2017-10-25T15:58:42","subject":"RE: [patch v9 1/4] drivers: jtag: Add JTAG core driver","submitter":{"id":72099,"url":"http://patchwork.ozlabs.org/api/people/72099/","name":"Oleksandr Shamray","email":"oleksandrs@mellanox.com"},"content":"> -----Original Message-----\n> From: Greg KH [mailto:gregkh@linuxfoundation.org]\n> Sent: Friday, October 20, 2017 5:54 PM\n> To: Oleksandr Shamray \n> Cc: arnd@arndb.de; linux-kernel@vger.kernel.org; linux-arm-\n> kernel@lists.infradead.org; devicetree@vger.kernel.org;\n> openbmc@lists.ozlabs.org; joel@jms.id.au; jiri@resnulli.us;\n> tklauser@distanz.ch; linux-serial@vger.kernel.org; mec@shout.net; Vadim\n> Pasternak ; system-sw-low-level level@mellanox.com>; robh+dt@kernel.org; openocd-devel-\n> owner@lists.sourceforge.net; linux-api@vger.kernel.org;\n> davem@davemloft.net; mchehab@kernel.org; Jiri Pirko \n> Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver\n> \n> On Fri, Oct 20, 2017 at 02:34:00PM +0000, Oleksandr Shamray wrote:\n> > Hi Greg.\n> >\n> > > -----Original Message-----\n> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]\n> > > Sent: Friday, October 20, 2017 2:55 PM\n> > > To: Oleksandr Shamray \n> > > Cc: arnd@arndb.de; linux-kernel@vger.kernel.org; linux-arm-\n> > > kernel@lists.infradead.org; devicetree@vger.kernel.org;\n> > > openbmc@lists.ozlabs.org; joel@jms.id.au; jiri@resnulli.us;\n> > > tklauser@distanz.ch; linux-serial@vger.kernel.org; mec@shout.net;\n> > > Vadim Pasternak ; system-sw-low-level\n> > > ; robh+dt@kernel.org;\n> > > openocd-devel- owner@lists.sourceforge.net;\n> > > linux-api@vger.kernel.org; davem@davemloft.net; mchehab@kernel.org;\n> > > Jiri Pirko \n> > > Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver\n> > >\n> > > On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:\n> > > > +struct jtag {\n> > > > +\tstruct device *dev;\n> > > > +\tstruct cdev cdev;\n> > >\n> > > Why are you using a cdev here and not just a normal misc device?\n> >\n> > What the benefits to use misc instead of cdev?\n> \n> Less code, simpler logic, easier to review and understand, etc.\n> \n> Let me ask you, why use a cdev instead of a misc?\n\nAs I know misc device more applicable if we want to create one device f.e. /dev/jtag. \nBut in current case we can have more than one jtag device /dev/jtag0 ... /dev/jtagN. \nSo I decided to use cdev.\n\n> \n> > > I forgot if this is what you were doing before, sorry...\n> > >\n> > > > +\tint id;\n> > > > +\tatomic_t open;\n> > >\n> > > Why do you need this?\n> >\n> > This counter used to avoid open at the same time by 2 or more users.\n> \n> But it isn't working :)\n> \n> And why do you care?\n> \n> > > > +\tconst struct jtag_ops *ops;\n> > > > +\tunsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);\n> > >\n> > > Huh? Why is this needed to be dma aligned? Why not just use the\n> > > private pointer in struct device?\n> > >\n> >\n> > It is critical?\n> \n> You are saying it is, so you have to justify it. There is a pointer for you to use,\n> don't make new ones for no reason, right?\n> \n\nYou are right. Will remove.\n\n> > > > +};\n> > > > +\n> > > > +static dev_t jtag_devt;\n> > > > +static DEFINE_IDA(jtag_ida);\n> > > > +\n> > > > +void *jtag_priv(struct jtag *jtag) {\n> > > > +\treturn jtag->priv;\n> > > > +}\n> > > > +EXPORT_SYMBOL_GPL(jtag_priv);\n> > > > +\n> > > > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {\n> > > > +\tunsigned long size;\n> > > > +\tvoid *kdata;\n> > > > +\n> > > > +\tsize = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\n> > > > +\tkdata = memdup_user(u64_to_user_ptr(udata), size);\n> > >\n> > > You only use this once, why not just open-code it?\n> >\n> > I think it makes code more understandable.\n> \n> As a reviewer, I don't :)\n\nOk, I will fix :)\n\n> \n> > > > +\n> > > > +\treturn kdata;\n> > > > +}\n> > > > +\n> > > > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,\n> > > > +\t\t\t\t unsigned long bit_size) {\n> > > > +\tunsigned long size;\n> > > > +\n> > > > +\tsize = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\n> > > > +\n> > > > +\treturn copy_to_user(u64_to_user_ptr(udata), (void *)(kdata),\n> > > > +size);\n> > >\n> > > Same here, making this a separate function seems odd.\n> >\n> > Same, I think it makes code more understandable.\n> \n> But it doesn't.\n> \n\nOk, I will fix :)\n\n> > > > +\n> > > > +\t\tif (jtag->ops->freq_set)\n> > > > +\t\t\terr = jtag->ops->freq_set(jtag, value);\n> > > > +\t\telse\n> > > > +\t\t\terr = -EOPNOTSUPP;\n> > > > +\t\tbreak;\n> > > > +\n> > > > +\tcase JTAG_IOCRUNTEST:\n> > > > +\t\tif (copy_from_user(&idle, (void *)arg,\n> > > > +\t\t\t\t sizeof(struct jtag_run_test_idle)))\n> > > > +\t\t\treturn -ENOMEM;\n> > > > +\t\terr = jtag_run_test_idle_op(jtag, &idle);\n> > >\n> > > Who validates the structure fields? Is that up to the individual\n> > > jtag driver? Why not do it in the core correctly so that it only\n> > > has to be done in one place and you do not have to audit every individual\n> driver?\n> >\n> > Input parameters validated by jtag platform driver. I think it not critical.\n> \n> Not true at all. It is very critical. Remmeber, \"All Input Is Evil!\"\n> \n> You have to validate this. I as a reviewer have to find where you are validating\n> this data to ensure bad things do not happen. I can't review that here, now I\n> have to go and review all of the individual drivers, which is a major pain, don't\n> you agree?\n\nAgree. I will add input parameter checking here before call device driver.\n\n> \n> > > > +\t\tbreak;\n> > > > +\n> > > > +\tcase JTAG_IOCXFER:\n> > > > +\t\tif (copy_from_user(&xfer, (void *)arg,\n> > > > +\t\t\t\t sizeof(struct jtag_xfer)))\n> > > > +\t\t\treturn -EFAULT;\n> > > > +\n> > > > +\t\tif (xfer.length >= JTAG_MAX_XFER_DATA_LEN)\n> > > > +\t\t\treturn -EFAULT;\n> > > > +\n> > > > +\t\txfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);\n> > > > +\t\tif (!xfer_data)\n> > > > +\t\t\treturn -ENOMEM;\n> > >\n> > > Are you sure that's the correct error value?\n> >\n> > I think yes, but what you suggest?\n> \n> A fault happened, so -EFAULT, right?\n> \n\nRight.\n\n\n> > [..]\n> > > + .unlocked_ioctl = jtag_ioctl,\n> > > + .open = jtag_open,\n> > > + .release = jtag_release,\n> > > +};\n> >\n> > add a compat_ioctl pointer here, after ensuring that all ioctl\n> > commands are compatible between 32-bit and 64-bit user space.\n> > [..]\n> \n> And if you do not, what happens? You shouldn't need it as there is no fixups\n> necessary, or am I mistaken about that?\n\nYes, you are right. In code compat_ioctl called same function as in unlocked_ioctl. \nSo I can remove compat and system will always call unlocked_ioctl.\n\n> \n> > > > +static int jtag_open(struct inode *inode, struct file *file) {\n> > > > +\tstruct jtag *jtag = container_of(inode->i_cdev, struct jtag,\n> > > > +cdev);\n> > > > +\n> > > > +\tif (atomic_read(&jtag->open)) {\n> > > > +\t\tdev_info(NULL, \"jtag already opened\\n\");\n> > > > +\t\treturn -EBUSY;\n> > >\n> > > Why do you care if multiple opens can happen?\n> >\n> > Jtag HW not support to using with multiple requests from different users. So\n> we prohibit this.\n> \n> Why does the kernel care?\n> \n> And again, your implementation is broken, it's not actually doing this\n> protection. I recommend just not doing it at all, but if you really are insisting\n> on it, you have to get it correct :)\n\nI will follow your recommendations and remove it. \n\n> \n> thanks,\n> \n> greg k-h\n\nThanks. \nOleksandr S","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yMZZq1LgWz9t2h\n\tfor ;\n\tThu, 26 Oct 2017 02:59:31 +1100 (AEDT)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3yMZZp4RmLzDqmT\n\tfor ;\n\tThu, 26 Oct 2017 02:59:30 +1100 (AEDT)","from EUR01-DB5-obe.outbound.protection.outlook.com\n\t(mail-db5eur01on0051.outbound.protection.outlook.com [104.47.2.51])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3yMZZ86HzlzDqln\n\tfor ; Thu, 26 Oct 2017 02:58:54 +1100 (AEDT)","from AM4PR0501MB2194.eurprd05.prod.outlook.com (10.165.82.13) by\n\tHE1PR05MB1756.eurprd05.prod.outlook.com (10.169.120.26) with\n\tMicrosoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id\n\t15.20.156.4; Wed, 25 Oct 2017 15:58:42 +0000","from AM4PR0501MB2194.eurprd05.prod.outlook.com\n\t([fe80::b02a:703f:24d5:1193]) by\n\tAM4PR0501MB2194.eurprd05.prod.outlook.com\n\t([fe80::b02a:703f:24d5:1193%18]) with mapi id 15.20.0156.007;\n\tWed, 25 Oct 2017 15:58:42 +0000"],"Authentication-Results":["ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"FqOiz4gd\"; dkim-atps=neutral","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"FqOiz4gd\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=mellanox.com\n\t(client-ip=104.47.2.51;\n\thelo=eur01-db5-obe.outbound.protection.outlook.com; \n\tenvelope-from=oleksandrs@mellanox.com; receiver=)","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"FqOiz4gd\"; dkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=oleksandrs@mellanox.com; "],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com;\n\ts=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version;\n\tbh=eNHgtBicbgOFkkBPNPWxWm+dfkXDdjiQxBKBrkoKKrY=;\n\tb=FqOiz4gd+GaM+X4AaoQ3041mwtEYGEaDEjkQvepZBrd8Nk2a+gr8LLD3UT8RYapFVKR1tjwTODKuDfkUcQuBk6YxyP6N6BhzlZuKVUAcHLzUOrx0S5XSX/8fg5t/m4F4Z9YF93pMTBQkzZesq16sSxMr1cGYX3xKbAxnb7wpx+U=","From":"Oleksandr Shamray ","To":"Greg KH ","Subject":"RE: [patch v9 1/4] drivers: jtag: Add JTAG core driver","Thread-Topic":"[patch v9 1/4] drivers: jtag: Add JTAG core driver","Thread-Index":"AQHTMru5yoecjoOKkEeYBkPR/vITOKLszxoAgAAGBCCAACwFgIAH2fow","Date":"Wed, 25 Oct 2017 15:58:42 +0000","Message-ID":"","References":"<1505985932-27568-1-git-send-email-oleksandrs@mellanox.com>\n\t<1505985932-27568-2-git-send-email-oleksandrs@mellanox.com>\n\t<20171020115512.GA2073@kroah.com>\n\t\n\t<20171020145417.GD8965@kroah.com>","In-Reply-To":"<20171020145417.GD8965@kroah.com>","Accept-Language":"en-US, uk-UA","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","authentication-results":["ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"FqOiz4gd\"; dkim-atps=neutral","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"FqOiz4gd\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=mellanox.com\n\t(client-ip=104.47.2.51;\n\thelo=eur01-db5-obe.outbound.protection.outlook.com; \n\tenvelope-from=oleksandrs@mellanox.com; receiver=)","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=Mellanox.com header.i=@Mellanox.com\n\theader.b=\"FqOiz4gd\"; dkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=oleksandrs@mellanox.com; "],"x-originating-ip":"[80.90.224.13]","x-ms-publictraffictype":"Email","x-microsoft-exchange-diagnostics":"1; HE1PR05MB1756;\n\t6:6pH5gR+V5H6qUhYXMqJCd23d09OATvqlQjV2KB8WC8s4D54I0g3GmQAfO6fhP00lIdKZ2XfdmnKaGI8saaLJqdH7hzUJaNd61OCvjRusXofBIp81EnbOgZuM1A0derVxE8A6CQ1eq5YeRJW6K+ABrzEAGC1bZhmikU0cT8VClyEVFSQsGehKYjh4Q64cz7QabHavq+eWpcrMsRJfnoWlZGrLxLlVxvFKIGdtOr209Pf1Ho4IAw1oqa4/1swXb5S6PeJ8G/cA4D5bSgoWyI4No+Xy9fK7WjDgXrc578uyCb8cQ1jAVI8GBMf7Vp9JcgfIO6cZTmjpqp/pVQL8jt0+Kg==;\n\t5:I9Va1Ux3fo8b8NGHFmQhXUphjWkXwlvQ5dhT0VzBRsYbyXrx0WTSbIwZU4RI4T5NGbRlDCyttpewxsTKM/SUIaEmvzUN/Ckdj/pP7OZjdy+Pdp5qzoaGRtzUuI7f1N2aeDYH3uwLEkD0waxUjV0y6w==;\n\t24:ku3mxYI4JWZF0nR7+5Fin93gXi9RCgLx+AMTGIJVo+X2eFzzMV0wUYTTne/NeMP+OhsLZfiOYWKgqswXO7AJZr1jB2Yt56syBrnRVpgAcZs=;\n\t7:vrafflJEzH2R7GkQQdsi0pdy6xuWqomTSEDU5TV8JNDaAkjjftptTqSVh8gslTlbG/p5BMp9WI+Sx83rFVI7TZemnri6PYcUdkTZS93XTiDzZwA5xawFFS8+Khj+5P4oqD7oQQhvBdzHzewpEgPoi6NdnRRcOFa0MDFI5J059hkaR5jocXjn0SKUkLgHmPzaQcYsAFS7dgEBGUPGT8JiWmoqpHmRerkH0YBZXYF+UqU=","x-ms-exchange-antispam-srfa-diagnostics":"SSOS;SSOR;","x-ms-office365-filtering-correlation-id":"a93cf6bd-d920-4b6d-a2cb-08d51bc143e1","x-ms-office365-filtering-ht":"Tenant","x-microsoft-antispam":"UriScan:; BCL:0; PCL:0;\n\tRULEID:(22001)(48565401081)(4534020)(4602075)(4627075)(201703031133081)(201702281549075)(2017052603199);\n\tSRVR:HE1PR05MB1756; ","x-ms-traffictypediagnostic":"HE1PR05MB1756:","x-exchange-antispam-report-test":"UriScan:(143289334528602)(9452136761055)(65623756079841)(258649278758335)(42262312472803);","x-microsoft-antispam-prvs":"","x-exchange-antispam-report-cfa-test":"BCL:0; PCL:0;\n\tRULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(93006095)(93001095)(100000703101)(100105400095)(3002001)(10201501046)(3231020)(6055026)(6041248)(20161123558100)(20161123555025)(20161123564025)(20161123562025)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);\n\tSRVR:HE1PR05MB1756; BCL:0; PCL:0;\n\tRULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);\n\tSRVR:HE1PR05MB1756; ","x-forefront-prvs":"0471B73328","x-forefront-antispam-report":"SFV:NSPM;\n\tSFS:(10009020)(6009001)(39860400002)(376002)(346002)(199003)(189002)(24454002)(13464003)(105586002)(6436002)(74316002)(68736007)(2950100002)(6506006)(66066001)(54356999)(6916009)(14454004)(76176999)(25786009)(305945005)(229853002)(97736004)(3280700002)(50986999)(101416001)(2906002)(3660700001)(33656002)(86362001)(106356001)(7696004)(53546010)(5660300001)(7416002)(3846002)(102836003)(93886005)(2900100001)(7736002)(6116002)(55016002)(8676002)(9686003)(54906003)(107886003)(478600001)(53936002)(99286003)(4326008)(8936002)(81156014)(81166006)(5250100002)(6246003)(189998001)(316002);\n\tDIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR05MB1756;\n\tH:AM4PR0501MB2194.eurprd05.prod.outlook.com; FPR:; SPF:None;\n\tPTR:InfoNoRecords; A:1; MX:1; LANG:en; ","received-spf":"None (protection.outlook.com: mellanox.com does not designate\n\tpermitted sender hosts)","spamdiagnosticoutput":"1:99","spamdiagnosticmetadata":"NSPM","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"quoted-printable","MIME-Version":"1.0","X-OriginatorOrg":"Mellanox.com","X-MS-Exchange-CrossTenant-Network-Message-Id":"a93cf6bd-d920-4b6d-a2cb-08d51bc143e1","X-MS-Exchange-CrossTenant-originalarrivaltime":"25 Oct 2017 15:58:42.2729\n\t(UTC)","X-MS-Exchange-CrossTenant-fromentityheader":"Hosted","X-MS-Exchange-CrossTenant-id":"a652971c-7d2e-4d9b-a6a4-d149256f461b","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"HE1PR05MB1756","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.24","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"system-sw-low-level ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"jiri@resnulli.us\" , \"arnd@arndb.de\" ,\n\tVadim Pasternak ,\n\t\"linux-api@vger.kernel.org\" ,\n\t\"openbmc@lists.ozlabs.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\t\"openocd-devel-owner@lists.sourceforge.net\"\n\t,\n\t\"mec@shout.net\" , Jiri Pirko ,\n\t\"robh+dt@kernel.org\" , \n\t\"linux-serial@vger.kernel.org\" ,\n\t\"tklauser@distanz.ch\" ,\n\t\"mchehab@kernel.org\" ,\n\t\"davem@davemloft.net\" ,\n\t\"linux-arm-kernel@lists.infradead.org\"\n\t","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}}]