[{"id":1765607,"web_url":"http://patchwork.ozlabs.org/comment/1765607/","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","headers":{"Return-Path":"","X-Original-To":"incoming-imx@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-imx@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.infradead.org\n\t(client-ip=65.50.211.133; helo=bombadil.infradead.org;\n\tenvelope-from=linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=lists.infradead.org\n\theader.i=@lists.infradead.org header.b=\"GCFBEVOo\"; \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 bombadil.infradead.org (bombadil.infradead.org\n\t[65.50.211.133])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xpnz15yGtz9sCZ\n\tfor ;\n\tSat, 9 Sep 2017 05:52:13 +1000 (AEST)","from localhost ([127.0.0.1] helo=bombadil.infradead.org)\n\tby bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dqPJx-0001os-UA; Fri, 08 Sep 2017 19:52:09 +0000","from mail-oi0-x244.google.com ([2607:f8b0:4003:c06::244])\n\tby bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux))\n\tid 1dqPJt-0001kC-LO for linux-arm-kernel@lists.infradead.org;\n\tFri, 08 Sep 2017 19:52:07 +0000","by mail-oi0-x244.google.com with SMTP id l9so1985229oib.4\n\tfor ;\n\tFri, 08 Sep 2017 12:51:45 -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; q=dns/txt; c=relaxed/relaxed;\n\td=lists.infradead.org; s=bombadil.20170209; h=Sender:\n\tContent-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:\n\tList-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From:\n\tReferences:In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description:\n\tResent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:\n\tList-Owner; bh=nYBOdgX+Z3qf45T6YTVJuSNv97TK4/wOyahrPCEyuno=;\n\tb=GCFBEVOoMKlqnT\n\tc2AUfevd+czyATqZbdbL3qhzq/hyhykmIViqUr6O4OCpC+ZqmutuYN6G7MOKbszET6NXAKpCviU3/\n\tWgX+G+Bo5chr4Kc493kWiZrBmqMObBmgJFc/9whAl0eWnD8DoIU6J3u8ErXQNLfJzVHqNASzvAQa5\n\tUt4csxxFJscgRY3hv7yAiMqZML4GXfY9wo0H0nbPfM1dPiplKO4+Tard1UnnpRIq4kTzqx5dlQUt9\n\t0ck3c5SPkWI26OXL+hlcpy856jv/CGyNdWEARd1IbgZUcmJJNDWWUX1U7Mz36HSoenc+6RTJASU/k\n\t/F1DjVXxAPSAeEwIpvFw==;","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=mOTtlBXzR08e2cCK/0fDr2drOSc/Pf/b54stZtqbE2xEIewipmKzKpjSNwKDfWy5Am\n\tXHR+SP7Ig3G5yno1qHEqYjF6NX1Qv85F7KQGF57hJV9n0gA9CIwsnj/QlEiWeG3D6zdd\n\tTEtHE+8yomJmqcVTmH9P+1YHfSk+aY1XUQ8tucIOkFO3X9slT9aQ/mt87Hnd+3Q+UX5H\n\tI0bC4Ew387CYolovzBGxlzDKyu3l8bSVASWINrasUQa0yCprDhzf/PySZWTgDeCrC1F+\n\t0NT20Pe1LkdRmHf3Nnee3b9nGr8GvfoO3/EmDufuW5smIEfnpOHiFvD8OkYkwRNOUeZB\n\tFMkQ==","X-Gm-Message-State":"AHPjjUiDwYQs5TtDFJVTAMrFY7HJZRWQAPc+GPMOEDgEsZDRXdbIaRzH\n\tWRr+3DpH7xM5et1b5VbfX221jsBAIg==","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 ","X-CRM114-Version":"20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 ","X-CRM114-CacheID":"sfid-20170908_125205_762295_A5F12950 ","X-CRM114-Status":"GOOD ( 24.43 )","X-Spam-Score":"-1.7 (-)","X-Spam-Report":"SpamAssassin version 3.4.1 on bombadil.infradead.org summary:\n\tContent analysis details: (-1.7 points)\n\tpts rule name description\n\t---- ----------------------\n\t--------------------------------------------------\n\t-0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at http://www.dnswl.org/,\n\tno\n\ttrust [2607:f8b0:4003:c06:0:0:0:244 listed in] [list.dnswl.org]\n\t-0.0 SPF_PASS SPF: sender matches SPF record\n\t0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\n\tprovider (arndbergmann[at]gmail.com)\n\t0.0 HEADER_FROM_DIFFERENT_DOMAINS From and EnvelopeFrom 2nd level\n\tmail domains are different\n\t-1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1%\n\t[score: 0.0000]\n\t-0.1 DKIM_VALID Message has at least one valid DKIM or DK signature\n\t0.1 DKIM_SIGNED Message has a DKIM or DK signature,\n\tnot necessarily valid\n\t0.2 FREEMAIL_FORGED_FROMDOMAIN 2nd level domains in From and\n\tEnvelopeFrom freemail headers are different","X-BeenThere":"linux-arm-kernel@lists.infradead.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"devicetree@vger.kernel.org, Mauro Carvalho Chehab ,\n\t=?utf-8?b?SmnFmcOtIFDDrXJrbw==?= ,\n\tsystem-sw-low-level@mellanox.com, gregkh , \n\tOpenBMC Maillist , Linux Kernel Mailing List\n\t, \n\topenocd-devel-owner@lists.sourceforge.net, mec@shout.net, Jiri Pirko\n\t, Rob Herring , Joel Stanley\n\t, linux-serial@vger.kernel.org, vadimp@maellanox.com,\n\tTobias Klauser , \n\tLinux API ,\n\tDavid Miller , Linux ARM\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"\"linux-arm-kernel\" ","Errors-To":"linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org","List-Id":"linux-imx-kernel.lists.patchwork.ozlabs.org"}}]