Received: by 10.192.165.148 with SMTP id m20csp440540imm; Wed, 25 Apr 2018 01:58:44 -0700 (PDT) X-Google-Smtp-Source: AIpwx4+DpAfnAVfWrwiFtZL9j2bKH5HlNN5tRf+T+ZZl732V+CYspgw+Y0hpBP3428LaWGhO9/ns X-Received: by 2002:a17:902:684c:: with SMTP id f12-v6mr28627331pln.139.1524646724368; Wed, 25 Apr 2018 01:58:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524646724; cv=none; d=google.com; s=arc-20160816; b=yGGpLxJzdXcI4L4rgnpDlaIC4K4CU6rlpzpnLxjNBofUPDNs11m1+sWrs/lC8JSbos yO1rJQV2KQJk1/NLdBHlVU1ciECLyYxBiunvucb1RMM4/EtZbq5sv0ytLArwiGNvzMVb zSaYXRwyDBLzx6Q8GJzhUMxR8Mw31uaPi9VmC3rUQTyMxgICwDo21RlN0T8vFEPyr1dX U/50SI8QTHB1y+IyP0K411KB//eibdUkZSgvJalMJCwpx8NHn+lUGR/S2czFJMhecGnS qUj90C+TOsxmZbYlo3+jM2wXDqW6QncTPh31twmOygBgWHvLciY3f1Aq9sff6Lw8mfD+ YAeA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=OCuMvvOnGtaBCp3CVPTpr+WPtOReo+Egx55agCfnMgQ=; b=AwWAI7PtbUGLKWmaLBft0MvphC4QAnwoh/wWgM/Z+EeSvzlTl0N4BqVc3fiqNIEZU+ bH3ExZO24Ow0Xftkr4SYvgoV57hL2uShuTu7UeCT9vvvRzMdPj0bWw0iEOQGlDGFneGo 8i+A1OPnFLx+dvZl9lrdvqihtgYnzClLktyp9SPQolKcsmbewUEHShC6h/L0c2WPjrA8 QfjgoPGh0u1bd//aXcd0CRGEZR/n1Z8cJJlxcKNJNaouRYntDqzlmOCrWzRoyoot4eBd P6EbmrP8On9CMluJuSmfJPa0ne2RxXzV22nwdMIAR9GCbk2a8JGcRpiyETIA1kegNs0O yPVQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w33-v6si15421334plb.431.2018.04.25.01.58.29; Wed, 25 Apr 2018 01:58:44 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751554AbeDYI5C (ORCPT + 99 others); Wed, 25 Apr 2018 04:57:02 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:40908 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750986AbeDYI46 (ORCPT ); Wed, 25 Apr 2018 04:56:58 -0400 Received: from localhost (unknown [37.168.186.153]) by mail.linuxfoundation.org (Postfix) with ESMTPSA id 760C42C; Wed, 25 Apr 2018 08:56:57 +0000 (UTC) Date: Wed, 25 Apr 2018 10:56:49 +0200 From: Greg Kroah-Hartman To: Johan Hovold Cc: Rob Herring , Mark Rutland , Andreas Kemnade , Arnd Bergmann , "H . Nikolaus Schaller" , Pavel Machek , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [PATCH 1/7] gnss: add GNSS receiver subsystem Message-ID: <20180425085649.GB13295@kroah.com> References: <20180424163458.11947-1-johan@kernel.org> <20180424163458.11947-2-johan@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180424163458.11947-2-johan@kernel.org> User-Agent: Mutt/1.9.5 (2018-04-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote: > +#define GNSS_MINORS 16 Why only 16? Just have to start somewhere? > +static DEFINE_IDA(gnss_minors); > +static dev_t gnss_first; > + > +/* FIFO size must be a power of two */ > +#define GNSS_READ_FIFO_SIZE 4096 > +#define GNSS_WRITE_BUF_SIZE 1024 > + > +#define to_gnss_device(d) container_of((d), struct gnss_device, dev) > + > +static int gnss_open(struct inode *inode, struct file *file) > +{ > + struct gnss_device *gdev; > + int ret = 0; > + > + gdev = container_of(inode->i_cdev, struct gnss_device, cdev); > + > + get_device(&gdev->dev); > + > + nonseekable_open(inode, file); > + file->private_data = gdev; > + > + down_write(&gdev->rwsem); Just curious, why a rwsem? They can be slower than a normal semaphore, is this really a contentious lock? > + if (gdev->disconnected) { > + ret = -ENODEV; > + goto unlock; > + } > + > + if (gdev->count++ == 0) { > + ret = gdev->ops->open(gdev); > + if (ret) > + gdev->count--; Why do we care about the opens here? Do you only want the "child driver" to have open called once, like a tty driver? Does it matter? Again, just curious. > + } > +unlock: > + up_write(&gdev->rwsem); > + > + if (ret) > + goto err_put_device; > + > + return 0; > + > +err_put_device: > + put_device(&gdev->dev); > + > + return ret; Those last 9 lines can be rewritten as just: if (!ret) put_device(&gdev->dev); return ret; > +} > + > +static int gnss_release(struct inode *inode, struct file *file) > +{ > + struct gnss_device *gdev = file->private_data; > + > + down_write(&gdev->rwsem); > + if (gdev->disconnected) > + goto unlock; > + > + if (--gdev->count == 0) { > + gdev->ops->close(gdev); > + kfifo_reset(&gdev->read_fifo); > + } > +unlock: > + up_write(&gdev->rwsem); > + > + put_device(&gdev->dev); > + > + return 0; > +} > + > +static ssize_t gnss_read(struct file *file, char __user *buf, > + size_t count, loff_t *pos) > +{ > + struct gnss_device *gdev = file->private_data; > + unsigned int copied; > + int ret; > + > + mutex_lock(&gdev->read_mutex); > + while (kfifo_is_empty(&gdev->read_fifo)) { > + mutex_unlock(&gdev->read_mutex); > + > + if (gdev->disconnected) > + return 0; > + > + if (file->f_flags & O_NONBLOCK) > + return -EAGAIN; > + > + ret = wait_event_interruptible(gdev->read_queue, > + gdev->disconnected || > + !kfifo_is_empty(&gdev->read_fifo)); > + if (ret) > + return -ERESTARTSYS; > + > + mutex_lock(&gdev->read_mutex); > + } > + > + ret = kfifo_to_user(&gdev->read_fifo, buf, count, &copied); > + if (ret) > + goto out_unlock; > + > + ret = copied; > + > + dev_dbg(&gdev->dev, "%s - count = %zu\n", __func__, copied); > + > +out_unlock: > + mutex_unlock(&gdev->read_mutex); > + > + return ret; > +} > + > +static ssize_t gnss_write(struct file *file, const char __user *buf, > + size_t count, loff_t *pos) > +{ > + struct gnss_device *gdev = file->private_data; > + size_t written = 0; > + int ret; > + > + dev_dbg(&gdev->dev, "%s - count = %zu\n", __func__, count); Nit, some of these initial dev_dbg() calls might be able to be removed, as ftrace should handle the tracing code now, right? > + > + if (gdev->disconnected) { > + dev_dbg(&gdev->dev, "%s - disconnected\n", __func__); > + return -EIO; > + } > + > + if (!count) > + return 0; > + > + if (!gdev->ops->write_raw) > + return -EIO; > + > + /* Ignoring O_NONBLOCK, write_raw() is synchronous. */ > + > + ret = mutex_lock_interruptible(&gdev->write_mutex); > + if (ret) > + return -ERESTARTSYS; > + > + for (;;) { > + size_t n = count - written; > + > + if (n > GNSS_WRITE_BUF_SIZE) > + n = GNSS_WRITE_BUF_SIZE; > + > + if (copy_from_user(gdev->write_buf, buf, n)) { > + ret = -EFAULT; > + goto out_unlock; > + } > + > + /* > + * Assumes write_raw can always accept GNSS_WRITE_BUF_SIZE > + * bytes. > + * > + * FIXME: revisit > + */ > + down_read(&gdev->rwsem); > + if (!gdev->disconnected) > + ret = gdev->ops->write_raw(gdev, gdev->write_buf, n); > + else > + ret = -EIO; > + up_read(&gdev->rwsem); > + > + if (ret < 0) > + break; > + > + written += ret; > + buf += ret; > + > + if (written == count) > + break; > + } > + > + if (written) > + ret = written; > +out_unlock: > + mutex_unlock(&gdev->write_mutex); > + > + return ret; > +} > + > +static __poll_t gnss_poll(struct file *file, poll_table *wait) > +{ > + struct gnss_device *gdev = file->private_data; > + __poll_t mask = 0; > + > + poll_wait(file, &gdev->read_queue, wait); > + > + if (!kfifo_is_empty(&gdev->read_fifo)) > + mask |= EPOLLIN | EPOLLRDNORM; > + if (gdev->disconnected) > + mask |= EPOLLHUP; > + > + dev_dbg(&gdev->dev, "%s - mask = 0x%04x\n", __func__, mask); > + > + return mask; > +} > + > +static const struct file_operations gnss_fops = { > + .owner = THIS_MODULE, > + .open = gnss_open, > + .release = gnss_release, > + .read = gnss_read, > + .write = gnss_write, > + .poll = gnss_poll, > + .llseek = no_llseek, > +}; > + > +static struct class *gnss_class; > + > +static void gnss_device_release(struct device *dev) > +{ > + struct gnss_device *gdev = to_gnss_device(dev); > + > + dev_dbg(dev, "%s\n", __func__); > + > + kfree(gdev->write_buf); > + kfifo_free(&gdev->read_fifo); > + ida_simple_remove(&gnss_minors, gdev->id); > + kfree(gdev); > +} > + > +struct gnss_device *gnss_allocate_device(struct device *parent) > +{ > + struct gnss_device *gdev; > + struct device *dev; > + int id; > + int ret; > + > + gdev = kzalloc(sizeof(*gdev), GFP_KERNEL); > + if (!gdev) > + return NULL; > + > + id = ida_simple_get(&gnss_minors, 0, GNSS_MINORS, GFP_KERNEL); > + if (id < 0) { > + kfree(gdev); > + return ERR_PTR(id); > + } > + > + gdev->id = id; > + > + dev = &gdev->dev; > + device_initialize(dev); > + dev->devt = gnss_first + id; > + dev->class = gnss_class; > + dev->parent = parent; > + dev->release = gnss_device_release; > + dev_set_drvdata(dev, gdev); > + dev_set_name(dev, "gnss%d", id); > + > + init_rwsem(&gdev->rwsem); > + mutex_init(&gdev->read_mutex); > + mutex_init(&gdev->write_mutex); > + init_waitqueue_head(&gdev->read_queue); > + > + ret = kfifo_alloc(&gdev->read_fifo, GNSS_READ_FIFO_SIZE, GFP_KERNEL); > + if (ret) > + goto err_put_device; > + > + gdev->write_buf = kzalloc(GNSS_WRITE_BUF_SIZE, GFP_KERNEL); > + if (!gdev->write_buf) > + goto err_put_device; > + > + cdev_init(&gdev->cdev, &gnss_fops); > + gdev->cdev.owner = THIS_MODULE; This protects this module from being unloaded, but how do you pass on the module reference counts to the "child" gnss driver? Am I just missing that logic here somewhere? Anyway, minor things, this looks really clean, nice job. greg k-h