From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D8707C43381 for ; Tue, 19 Mar 2019 13:18:12 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A6C3220854 for ; Tue, 19 Mar 2019 13:18:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Wq1VLnnZ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A6C3220854 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arndb.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DHjNac2h/0SesmcVPFyWR/t3KNb2jTOUqQoHIurVziw=; b=Wq1VLnnZaA3zQk EjYNtcEUqBpEc5F6Cx+KNmmsMvXZTudBmowJzGmA0QZIMY484GcdDpWymmKZBCXAM+DKKrpmLRQPb 01Qqp/lTNTok89EjpOFyA4iuo5Mw2yBvcKZRZ2I+tV8qinWEOFhuuQAVBh8rddfiAaSIF9IpBMxxQ Auohax+g7862Ez9Gq/RrTOztGBGphJfaYDAjs86YgoAsuC9J29IpMgOawsXuJumEC+ad+0ykm6RHF ckLHvBkJFO6j1nnjFlJfG+LWZqFhS/Q0JDQCKlkBg4U+fHB6dZRBe8P3b76QflkUXGmskRw94EbPd iIhBBP6GMKJt4t2mKC9g==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6Ed6-00056R-Hd; Tue, 19 Mar 2019 13:18:08 +0000 Received: from mail-qk1-f193.google.com ([209.85.222.193]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6Ed3-00055w-Ad for linux-arm-kernel@lists.infradead.org; Tue, 19 Mar 2019 13:18:06 +0000 Received: by mail-qk1-f193.google.com with SMTP id c189so11784556qke.6 for ; Tue, 19 Mar 2019 06:18:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JValrODhL9L1KoNqoer/wGLOwpigQOqn55f7IV3QNRo=; b=A0Uh3p3AH5/qe2SEcEc2zibuV//9+YtlnHWCRuX31dmYBQPgMxw3wd8rBcsDCgT1HF HjVdSMPGCMQ6yncah89fKztDaNwi0t8tRJVpugc/5koIflh6vJ0meks/fd9vdLjvVCju ownuVRbGwqF/9U/RY8zD0SRiPfSRObhSL2dwN9wGTzzPcatgwzGBo0YcEg5fZJtoI2I1 IEK4mbKZwvADwKNi3iA+vMfioUMNmXSPPUBs9p6WScpSAxi9UfYjbR0hoi6ROR9kwA4h A909VL1X98gDeAHT/UzHnclOBiga4D6oWzMddRNvxKz4tBtUg5ff+iC14KZ7r4OipQGc 56ZQ== X-Gm-Message-State: APjAAAWHvctSbZ1oFl3WNOejyB+y3aOaF+hqGOVDXF4jmNEMZWvJHwWL btY3tAdt3DDkGLw3gJW6ykdQ47qiNMKwDCGzFrI= X-Google-Smtp-Source: APXvYqxPu61nlXjGGarQPN4ErRfaeDVgfbh47aaG4gyl6ikCFueu+FzaUsz0GTw5tLL6yxjsQhVJptJpCpM7BhNgaQw= X-Received: by 2002:a05:620a:153b:: with SMTP id n27mr1694185qkk.343.1553001482905; Tue, 19 Mar 2019 06:18:02 -0700 (PDT) MIME-Version: 1.0 References: <1552997064-432700-1-git-send-email-dragan.cvetic@xilinx.com> <1552997064-432700-5-git-send-email-dragan.cvetic@xilinx.com> In-Reply-To: <1552997064-432700-5-git-send-email-dragan.cvetic@xilinx.com> From: Arnd Bergmann Date: Tue, 19 Mar 2019 14:17:45 +0100 Message-ID: Subject: Re: [PATCH 04/12] misc: xilinx_sdfec: Add open, close and ioctl To: Dragan Cvetic X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190319_061805_368641_58844C86 X-CRM114-Status: GOOD ( 23.10 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: gregkh , Derek Kiernan , Michal Simek , Linux ARM , Linux Kernel Mailing List Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Mar 19, 2019 at 1:05 PM Dragan Cvetic wrote: > > Add char device interface per DT node present and support > file operations: > - open(), which keeps only one open per device at a time, > - close(), which release the open for this device, > - ioctl(), which provides infrastructure for a specific driver > control. > drivers/misc/xilinx_sdfec.c | 79 ++++++++++++++++++++++++++++++++++++++++ > include/uapi/misc/xilinx_sdfec.h | 4 ++ > 2 files changed, 83 insertions(+) > > diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c > index a52a5c6..3407de4 100644 > --- a/drivers/misc/xilinx_sdfec.c > +++ b/drivers/misc/xilinx_sdfec.c > @@ -81,8 +81,87 @@ struct xsdfec_dev { > struct xsdfec_clks clks; > }; > > +static int xsdfec_dev_open(struct inode *iptr, struct file *fptr) > +{ > + struct xsdfec_dev *xsdfec; > + > + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev); > + if (!xsdfec) > + return -EAGAIN; The result of container_of() will not be NULL here. Did you mean to check i_cdev? That probably also won't be NULL, but that check would be more reasonable. > + /* Only one open per device at a time */ > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > + atomic_inc(&xsdfec->open_count); > + return -EBUSY; > + } What is that limitation for? Is it worse to open it twice than to dup() or fork()? Note that the test is not really atomic either: if three processes try to open the file at the same time, it gets decremented from 1 to -2, so only the second one sees 0 and increments it back to -1 afterwards... > +static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd, > + unsigned long data) > +{ > + struct xsdfec_dev *xsdfec = fptr->private_data; > + void __user *arg = NULL; > + int rval = -EINVAL; > + int err = 0; > + > + if (!xsdfec) > + return rval; > + > + if (_IOC_TYPE(cmd) != XSDFEC_MAGIC) { > + dev_err(xsdfec->dev, "Not a xilinx sdfec ioctl"); > + return -ENOTTY; > + } remove the error messages here as well. > + /* Access check of the argument if present */ > + if (_IOC_DIR(cmd) & _IOC_READ) > + err = !access_ok((void *)arg, _IOC_SIZE(cmd)); > + else if (_IOC_DIR(cmd) & _IOC_WRITE) > + err = !access_ok((void *)arg, _IOC_SIZE(cmd)); This seems odd. Why two separate checks, and why the access_ok() check when you do a copy_from_user() that contains the same check later? If you want to get fancy here, you could just copy the data in the main ioctl handler based on _IOC_SIZE, and pass around normal kernel pointers from there. > static const struct file_operations xsdfec_fops = { > .owner = THIS_MODULE, > + .open = xsdfec_dev_open, > + .release = xsdfec_dev_release, > + .unlocked_ioctl = xsdfec_dev_ioctl, > }; This lacks a .compat_ioctl pointer. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel