[{"id":1765605,"web_url":"http://patchwork.ozlabs.org/comment/1765605/","msgid":"","list_archive_url":null,"date":"2017-09-08T19:51:43","subject":"Re: [patch v8 1/4] drivers: jtag: Add JTAG core driver","submitter":{"id":30,"url":"http://patchwork.ozlabs.org/api/people/30/","name":"Arnd Bergmann","email":"arnd@arndb.de"},"content":"On Fri, Sep 8, 2017 at 6:13 PM, Oleksandr Shamray\n wrote:\n> Initial patch for JTAG driver\n> JTAG class driver provide infrastructure to support hardware/software\n> JTAG platform drivers. It provide user layer API interface for flashing\n> and debugging external devices which equipped with JTAG interface\n> using standard transactions.\n>\n> Driver exposes set of IOCTL to user space for:\n> - XFER:\n> - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);\n> - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);\n> - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified\n> number of clocks).\n> - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.\n>\n> Driver core provides set of internal APIs for allocation and\n> registration:\n> - jtag_register;\n> - jtag_unregister;\n> - jtag_alloc;\n> - jtag_free;\n>\n> Platform driver on registration with jtag-core creates the next\n> entry in dev folder:\n> /dev/jtagX\n>\n> Signed-off-by: Oleksandr Shamray \n> Signed-off-by: Jiri Pirko \n> Acked-by: Arnd Bergmann \n\nMy Ack was actually just for the device driver part, I had not\nlooked at the core again, sorry for not being clearer here.\n\nThe other two patches are fine, but looking at this one again,\nI find a couple of things to improve:\n\n> +\n> +static __u64 jtag_copy_from_user(__u64 udata, unsigned long bit_size)\n> +{\n> + unsigned long size;\n> + void *kdata;\n> +\n> + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\n> + kdata = memdup_user(u64_to_user_ptr(udata), size);\n> +\n> + return (__u64)(uintptr_t)kdata;\n> +}\n> +\n> +static unsigned long jtag_copy_to_user(__u64 udata, __u64 kdata,\n> + unsigned long bit_size)\n> +{\n> + unsigned long size;\n> +\n> + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);\n> +\n> + return copy_to_user(u64_to_user_ptr(udata), jtag_u64_to_ptr(kdata),\n> + size);\n> +}\n\nOnly a style comment, but the casting to and from u64 multiple\ntimes here seems overly complicated. Why not use a regular kernel\npointer here, and then pass that as a third argument to\nag->ops->xfer() ?\n\n> +\n> + case JTAG_SIOCFREQ:\n> + err = __get_user(value, uarg);\n\nThis needs to use get_user(), not __get_user(). I think you changed\nall the other instances after an earlier comment, but missed this one.\n\n> +static int jtag_open(struct inode *inode, struct file *file)\n> +{\n> + struct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);\n> +\n> + spin_lock(&jtag->lock);\n> +\n> + if (jtag->open) {\n> + dev_info(NULL, \"jtag already opened\\n\");\n> + spin_unlock(&jtag->lock);\n> + return -EBUSY;\n> + }\n> +\n> + jtag->open++;\n> + file->private_data = jtag;\n> + spin_unlock(&jtag->lock);\n> + return 0;\n> +}\n\nThe spinlock seems to not protect anything but the use count,\nso this could be an atomic_t.\n\n\n> + mutex_lock(&jtag_mutex);\n> + list_add_tail(&jtag->list, &jtag_list);\n> + mutex_unlock(&jtag_mutex);\n\nSimilarly, you only use the mutex to protect the list_add/list_del,\nbut nothing ever walks the list, so I think you can simply remove\nboth the list and the mutex.\n\n> +static int __init jtag_init(void)\n> +{\n> + int err;\n> +\n> + err = alloc_chrdev_region(&jtag_devt, 0, 1, \"jtag\");\n> + if (err)\n> + return err;\n> + return class_register(&jtag_class);\n> +}\n> +\n> +static void __exit jtag_exit(void)\n> +{\n> + class_unregister(&jtag_class);\n> +}\n\nThis looks a bit asymmetric:\n\n- you allocate a chardev region but don't destroy it in jtag_exit,\n so presumably this leaks a major number allocation each\n time you load/unload the module\n\n- you allocate a single minor number at module load time,\n but the individual devices each get another number, and\n the original one does not appear to be used, unless I\n misunderstand the API here.\n\n Arnd\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"kgmkek3J\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xpnyY2BXZz9sCZ\n\tfor ;\n\tSat, 9 Sep 2017 05:51:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932381AbdIHTvr (ORCPT );\n\tFri, 8 Sep 2017 15:51:47 -0400","from mail-oi0-f66.google.com ([209.85.218.66]:36606 \"EHLO\n\tmail-oi0-f66.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S932322AbdIHTvp (ORCPT\n\t); Fri, 8 Sep 2017 15:51:45 -0400","by mail-oi0-f66.google.com with SMTP id g15so2450766oib.3;\n\tFri, 08 Sep 2017 12:51:44 -0700 (PDT)","by 10.157.91.35 with HTTP; Fri, 8 Sep 2017 12:51:43 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=aIEpBOtyr6OCXRZrriRGIuz8QHAd6AVJ/B90bgB1Q9o=;\n\tb=kgmkek3JbGyh8gnGYl59tHQEBg03LT5P1kARf2HjRequB0ftICBVccBL0HMvN6vrRo\n\tgG14R9mEojMTj+Dlj9YdfaHTYO3YsaHkYESmJphSVjTkcNRMoFsXxuXL7GdgQj+rGJDF\n\tEpr7KuH+Xplwq5RoxnO9T97Q1xZgHh27KdRtKNlaZ+oEiUfLPF7VzNToaVlbtBXCxEoQ\n\tVBkam4ak47P+ZOPo9pQjGfyb6GRndLfLqGlY7LfGIKAjkcI41L9qtZOJmoGgS/6fhj6L\n\tMhfvRfdDLdLuQMNxMJHWRMuk/slZPy+Wb6NsqUvVpZcUzziZhX8SLJ3Q4RQkQikwMWla\n\tKCSQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=aIEpBOtyr6OCXRZrriRGIuz8QHAd6AVJ/B90bgB1Q9o=;\n\tb=n8UuIFSxsc26QLg6qPwZHjrIEnP3Z5PmUkouO6+igCodyxlhd204mLzD9wPpzjB4OD\n\tK0fnhmD+b2JFMS2UdhwP6FAHvOOJEqAjrf4zoIyTeljDFwA2G7pYG9/BXQxQfRWTBuMo\n\t6RcEJncdJ63wFoZGcBZx4z8awiD5L9eZtzHaa79LVeRX645JxNrndMfSh6Cj2dCDwDL2\n\tRLFgROCrlGanV2Zru4mOEokRBd1uWPqbO5wfOSuZ10IIV+r6WRUJs8/5xVUsy9xSSwrJ\n\t5r85YDMVnzGF76suG3iQZiXn8Tr6FFYFkJBq5wD1mGFZotbwM8LfrJC5UTJe/h95nzP6\n\tA/WA==","X-Gm-Message-State":"AHPjjUgfZ9vSN5vpXi12jesQhjRDxYi/StzElZZY2VJj/qCGssNHqtx9\n\tZttDjnxez1E5hv+JHQFzxqdwFx+cQ313oK8=","X-Google-Smtp-Source":"AOwi7QDHzfl+WAwsR+NQb+YYZFnpPx68CMtet4Wm9WtDJymIE+XoR8hnsSsa/YT4qSHeXKw8N2wWVRSdJyo9hlEA2QY=","X-Received":"by 10.202.193.65 with SMTP id r62mr4281617oif.313.1504900304426; \n\tFri, 08 Sep 2017 12:51:44 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1504887229-2748-2-git-send-email-oleksandrs@mellanox.com>","References":"<1504887229-2748-1-git-send-email-oleksandrs@mellanox.com>\n\t<1504887229-2748-2-git-send-email-oleksandrs@mellanox.com>","From":"Arnd Bergmann ","Date":"Fri, 8 Sep 2017 21:51:43 +0200","X-Google-Sender-Auth":"aD6RdWzkkUJawZa0gV6giYOisOU","Message-ID":"","Subject":"Re: [patch v8 1/4] drivers: jtag: Add JTAG core driver","To":"Oleksandr Shamray ","Cc":"gregkh , Linux Kernel Mailing List\n\t, Linux ARM\n\t, devicetree@vger.kernel.org,\n\tOpenBMC Maillist , \n\tJoel Stanley , =?utf-8?b?SmnFmcOtIFDDrXJrbw==?=\n\t, Tobias Klauser ,\n\tlinux-serial@vger.kernel.org, mec@shout.net, vadimp@maellanox.com, \n\tsystem-sw-low-level@mellanox.com, Rob Herring , \n\topenocd-devel-owner@lists.sourceforge.net, Linux API\n\t, David Miller , \n\tMauro Carvalho Chehab\n\t, Jiri Pirko ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]