Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Sun, Apr 29, 2018 at 03:35:11PM +0200, Greg Kroah-Hartman wrote:

> I'll wait for the next resend of this series to review it again and
> consider merging it.

Thanks, I'll send a v2 sometime next week.

Johan

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Sun, Apr 29, 2018 at 03:35:11PM +0200, Greg Kroah-Hartman wrote:

> I'll wait for the next resend of this series to review it again and
> consider merging it.

Thanks, I'll send a v2 sometime next week.

Johan

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Greg Kroah-Hartman]

On Wed, Apr 25, 2018 at 02:23:15PM +0200, Johan Hovold wrote:
> On Wed, Apr 25, 2018 at 12:56:45PM +0200, Johan Hovold wrote:
> > On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:
> 
> > > > +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(>dev);
> > > > +
> > > > +   nonseekable_open(inode, file);
> > > > +   file->private_data = gdev;
> > > > +
> > > > +   down_write(>rwsem);
> > > 
> > > Just curious, why a rwsem?  They can be slower than a normal semaphore,
> > > is this really a contentious lock?
> > 
> > I use the rwsem to deal with hotplugging; the underlying device can go
> > away at any time and the core makes sure that no further calls into the
> > corresponding driver is made once all currently executing callbacks
> > return.
> 
> I just did find one access to the gnss ops which was unsafe however; the
> existence check for a write_raw callback in write() needs to be replaced
> by a device flag.

Ok, in looking at this closer, it makes more sense to me, and my worries
are not true, you are handling this properly, sorry for the noise.

I'll wait for the next resend of this series to review it again and
consider merging it.

thanks,

greg k-h

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Greg Kroah-Hartman]

On Wed, Apr 25, 2018 at 02:23:15PM +0200, Johan Hovold wrote:
> On Wed, Apr 25, 2018 at 12:56:45PM +0200, Johan Hovold wrote:
> > On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:
> 
> > > > +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(>dev);
> > > > +
> > > > +   nonseekable_open(inode, file);
> > > > +   file->private_data = gdev;
> > > > +
> > > > +   down_write(>rwsem);
> > > 
> > > Just curious, why a rwsem?  They can be slower than a normal semaphore,
> > > is this really a contentious lock?
> > 
> > I use the rwsem to deal with hotplugging; the underlying device can go
> > away at any time and the core makes sure that no further calls into the
> > corresponding driver is made once all currently executing callbacks
> > return.
> 
> I just did find one access to the gnss ops which was unsafe however; the
> existence check for a write_raw callback in write() needs to be replaced
> by a device flag.

Ok, in looking at this closer, it makes more sense to me, and my worries
are not true, you are handling this properly, sorry for the noise.

I'll wait for the next resend of this series to review it again and
consider merging it.

thanks,

greg k-h

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Wed, Apr 25, 2018 at 12:56:45PM +0200, Johan Hovold wrote:
> On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:

> > > +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(>dev);
> > > +
> > > + nonseekable_open(inode, file);
> > > + file->private_data = gdev;
> > > +
> > > + down_write(>rwsem);
> > 
> > Just curious, why a rwsem?  They can be slower than a normal semaphore,
> > is this really a contentious lock?
> 
> I use the rwsem to deal with hotplugging; the underlying device can go
> away at any time and the core makes sure that no further calls into the
> corresponding driver is made once all currently executing callbacks
> return.

I just did find one access to the gnss ops which was unsafe however; the
existence check for a write_raw callback in write() needs to be replaced
by a device flag.

Thanks,
Johan

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Wed, Apr 25, 2018 at 12:56:45PM +0200, Johan Hovold wrote:
> On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:

> > > +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(>dev);
> > > +
> > > + nonseekable_open(inode, file);
> > > + file->private_data = gdev;
> > > +
> > > + down_write(>rwsem);
> > 
> > Just curious, why a rwsem?  They can be slower than a normal semaphore,
> > is this really a contentious lock?
> 
> I use the rwsem to deal with hotplugging; the underlying device can go
> away at any time and the core makes sure that no further calls into the
> corresponding driver is made once all currently executing callbacks
> return.

I just did find one access to the gnss ops which was unsafe however; the
existence check for a write_raw callback in write() needs to be replaced
by a device flag.

Thanks,
Johan

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:
> > +#define GNSS_MINORS16
> 
> Why only 16?  Just have to start somewhere?

Yeah, a mostly arbitrarily picked number. I figured only developers
would have an interest in having even that many GNSS receivers in one
system. But it can be raised, now or later.

> > +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(>dev);
> > +
> > +   nonseekable_open(inode, file);
> > +   file->private_data = gdev;
> > +
> > +   down_write(>rwsem);
> 
> Just curious, why a rwsem?  They can be slower than a normal semaphore,
> is this really a contentious lock?

I use the rwsem to deal with hotplugging; the underlying device can go
away at any time and the core makes sure that no further calls into the
corresponding driver is made once all currently executing callbacks
return.

> > +   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?

Exactly, core deals with the open counts so that the individual gnss
drivers need not. Serdev, for example, blows up if you try to open the
same device twice.

> Again, just curious.
> 
> > +   }
> > +unlock:
> > +   up_write(>rwsem);
> > +
> > +   if (ret)
> > +   goto err_put_device;
> > +
> > +   return 0;
> > +
> > +err_put_device:
> > +   put_device(>dev);
> > +
> > +   return ret;
> 
> Those last 9 lines can be rewritten as just:
> 
>   if (!ret)
>   put_device(>dev);
> 
>   return ret;

Yeah, I usually prefer having an explicit return 0 in the success path
and clearly separate the error paths even if it adds few extra lines.
This case could become an exception though.

> > +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(>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?

Right. The were probably mostly useful to me during development, and can
be added back later if it turns out they have any further value.

> > +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,
> > +};

> > +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(_minors, 0, GNSS_MINORS, GFP_KERNEL);
> > +   if (id < 0) {
> > +   kfree(gdev);
> > +   return ERR_PTR(id);
> > +   }
> > +
> > +   gdev->id = id;
> > +
> > +   dev = >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(>rwsem);
> > +   mutex_init(>read_mutex);
> > +   mutex_init(>write_mutex);
> > +   init_waitqueue_head(>read_queue);
> > +
> > +   ret = kfifo_alloc(>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(>cdev, _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?

Due to the hotplug support mentioned above, I do not need to pin the
"child" gnss driver modules. Their devices can go away at any time, be
it due to hotplugging, driver unbind through sysfs, or module unload.

> Anyway, minor things, this looks really clean, nice job.

Thanks again!

Johan

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

On Wed, Apr 25, 2018 at 10:56:49AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 24, 2018 at 06:34:52PM +0200, Johan Hovold wrote:
> > +#define GNSS_MINORS16
> 
> Why only 16?  Just have to start somewhere?

Yeah, a mostly arbitrarily picked number. I figured only developers
would have an interest in having even that many GNSS receivers in one
system. But it can be raised, now or later.

> > +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(>dev);
> > +
> > +   nonseekable_open(inode, file);
> > +   file->private_data = gdev;
> > +
> > +   down_write(>rwsem);
> 
> Just curious, why a rwsem?  They can be slower than a normal semaphore,
> is this really a contentious lock?

I use the rwsem to deal with hotplugging; the underlying device can go
away at any time and the core makes sure that no further calls into the
corresponding driver is made once all currently executing callbacks
return.

> > +   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?

Exactly, core deals with the open counts so that the individual gnss
drivers need not. Serdev, for example, blows up if you try to open the
same device twice.

> Again, just curious.
> 
> > +   }
> > +unlock:
> > +   up_write(>rwsem);
> > +
> > +   if (ret)
> > +   goto err_put_device;
> > +
> > +   return 0;
> > +
> > +err_put_device:
> > +   put_device(>dev);
> > +
> > +   return ret;
> 
> Those last 9 lines can be rewritten as just:
> 
>   if (!ret)
>   put_device(>dev);
> 
>   return ret;

Yeah, I usually prefer having an explicit return 0 in the success path
and clearly separate the error paths even if it adds few extra lines.
This case could become an exception though.

> > +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(>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?

Right. The were probably mostly useful to me during development, and can
be added back later if it turns out they have any further value.

> > +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,
> > +};

> > +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(_minors, 0, GNSS_MINORS, GFP_KERNEL);
> > +   if (id < 0) {
> > +   kfree(gdev);
> > +   return ERR_PTR(id);
> > +   }
> > +
> > +   gdev->id = id;
> > +
> > +   dev = >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(>rwsem);
> > +   mutex_init(>read_mutex);
> > +   mutex_init(>write_mutex);
> > +   init_waitqueue_head(>read_queue);
> > +
> > +   ret = kfifo_alloc(>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(>cdev, _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?

Due to the hotplug support mentioned above, I do not need to pin the
"child" gnss driver modules. Their devices can go away at any time, be
it due to hotplugging, driver unbind through sysfs, or module unload.

> Anyway, minor things, this looks really clean, nice job.

Thanks again!

Johan

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Greg Kroah-Hartman]

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(>dev);
> +
> + nonseekable_open(inode, file);
> + file->private_data = gdev;
> +
> + down_write(>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(>rwsem);
> +
> + if (ret)
> + goto err_put_device;
> +
> + return 0;
> +
> +err_put_device:
> + put_device(>dev);
> +
> + return ret;

Those last 9 lines can be rewritten as just:

if (!ret)
put_device(>dev);

return ret;

> +}
> +
> +static int gnss_release(struct inode *inode, struct file *file)
> +{
> + struct gnss_device *gdev = file->private_data;
> +
> + down_write(>rwsem);
> + if (gdev->disconnected)
> + goto unlock;
> +
> + if (--gdev->count == 0) {
> + gdev->ops->close(gdev);
> + kfifo_reset(>read_fifo);
> + }
> +unlock:
> + up_write(>rwsem);
> +
> + put_device(>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(>read_mutex);
> + while (kfifo_is_empty(>read_fifo)) {
> + mutex_unlock(>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(>read_fifo));
> + if (ret)
> + return -ERESTARTSYS;
> +
> + mutex_lock(>read_mutex);
> + }
> +
> + ret = kfifo_to_user(>read_fifo, buf, count, );
> + if (ret)
> + goto out_unlock;
> +
> + ret = copied;
> +
> + dev_dbg(>dev, "%s - count = %zu\n", __func__, copied);
> +
> +out_unlock:
> + mutex_unlock(>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(>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(>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(>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(>rwsem);
> + if (!gdev->disconnected)
> + ret = gdev->ops->write_raw(gdev, gdev->write_buf, n);
> + else
> + ret = -EIO;
> + up_read(>rwsem);
> +
> + if (ret < 0)
> + break;
> +
> + written += ret;
> + buf += ret;
> +
> + if (written == count)
> + break;
> + }
> +
> + if 

Re: [PATCH 1/7] gnss: add GNSS receiver subsystem

[Greg Kroah-Hartman]

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(>dev);
> +
> + nonseekable_open(inode, file);
> + file->private_data = gdev;
> +
> + down_write(>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(>rwsem);
> +
> + if (ret)
> + goto err_put_device;
> +
> + return 0;
> +
> +err_put_device:
> + put_device(>dev);
> +
> + return ret;

Those last 9 lines can be rewritten as just:

if (!ret)
put_device(>dev);

return ret;

> +}
> +
> +static int gnss_release(struct inode *inode, struct file *file)
> +{
> + struct gnss_device *gdev = file->private_data;
> +
> + down_write(>rwsem);
> + if (gdev->disconnected)
> + goto unlock;
> +
> + if (--gdev->count == 0) {
> + gdev->ops->close(gdev);
> + kfifo_reset(>read_fifo);
> + }
> +unlock:
> + up_write(>rwsem);
> +
> + put_device(>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(>read_mutex);
> + while (kfifo_is_empty(>read_fifo)) {
> + mutex_unlock(>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(>read_fifo));
> + if (ret)
> + return -ERESTARTSYS;
> +
> + mutex_lock(>read_mutex);
> + }
> +
> + ret = kfifo_to_user(>read_fifo, buf, count, );
> + if (ret)
> + goto out_unlock;
> +
> + ret = copied;
> +
> + dev_dbg(>dev, "%s - count = %zu\n", __func__, copied);
> +
> +out_unlock:
> + mutex_unlock(>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(>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(>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(>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(>rwsem);
> + if (!gdev->disconnected)
> + ret = gdev->ops->write_raw(gdev, gdev->write_buf, n);
> + else
> + ret = -EIO;
> + up_read(>rwsem);
> +
> + if (ret < 0)
> + break;
> +
> + written += ret;
> + buf += ret;
> +
> + if (written == count)
> + break;
> + }
> +
> + if 

[PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

Add a new subsystem for GNSS (e.g. GPS) receivers.

While GNSS receivers are typically accessed using a UART interface they
often also support other I/O interfaces such as I2C, SPI and USB, while
yet other devices use iomem or even some form of remote-processor
messaging (rpmsg).

The new GNSS subsystem abstracts the underlying interface and provides a
new "gnss" class type, which exposes a character-device interface (e.g.
/dev/gnss0) to user space. This allows GNSS receivers to have a
representation in the Linux device model, something which is important
not least for power management purposes.

Note that the character-device interface provides raw access to whatever
protocol the receiver is (currently) using, such as NMEA 0183, UBX or
SiRF Binary. These protocols are expected to be continued to be handled
by user space for the time being, even if some hybrid solutions are also
conceivable (e.g. to have kernel drivers issue management commands).

This will still allow for better platform integration by allowing GNSS
devices and their resources (e.g. regulators and enable-gpios) to be
described by firmware and managed by kernel drivers rather than
platform-specific scripts and services.

While the current interface is kept minimal, it could be extended using
IOCTLs, sysfs or uevents as needs and proper abstraction levels are
identified and determined (e.g. for device and feature identification).

Signed-off-by: Johan Hovold 
---
 MAINTAINERS   |   6 +
 drivers/Kconfig   |   2 +
 drivers/Makefile  |   1 +
 drivers/gnss/Kconfig  |  11 ++
 drivers/gnss/Makefile |   7 +
 drivers/gnss/core.c   | 385 ++
 include/linux/gnss.h  |  64 +++
 7 files changed, 476 insertions(+)
 create mode 100644 drivers/gnss/Kconfig
 create mode 100644 drivers/gnss/Makefile
 create mode 100644 drivers/gnss/core.c
 create mode 100644 include/linux/gnss.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 0a1410d5a621..dc3df211c1a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5962,6 +5962,12 @@ F:   Documentation/isdn/README.gigaset
 F: drivers/isdn/gigaset/
 F: include/uapi/linux/gigaset_dev.h
 
+GNSS SUBSYSTEM
+M: Johan Hovold 
+S: Maintained
+F: drivers/gnss/
+F: include/linux/gnss.h
+
 GO7007 MPEG CODEC
 M: Hans Verkuil 
 L: linux-me...@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 95b9ccc08165..ab4d43923c4d 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -9,6 +9,8 @@ source "drivers/bus/Kconfig"
 
 source "drivers/connector/Kconfig"
 
+source "drivers/gnss/Kconfig"
+
 source "drivers/mtd/Kconfig"
 
 source "drivers/of/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd47014657..cc9a7c5f7d2c 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -185,3 +185,4 @@ obj-$(CONFIG_TEE)   += tee/
 obj-$(CONFIG_MULTIPLEXER)  += mux/
 obj-$(CONFIG_UNISYS_VISORBUS)  += visorbus/
 obj-$(CONFIG_SIOX) += siox/
+obj-$(CONFIG_GNSS) += gnss/
diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
new file mode 100644
index ..103fcc70992e
--- /dev/null
+++ b/drivers/gnss/Kconfig
@@ -0,0 +1,11 @@
+#
+# GNSS receiver configuration
+#
+
+menuconfig GNSS
+   tristate "GNSS receiver support"
+   ---help---
+ Say Y here if you have a GNSS receiver (e.g. a GPS receiver).
+
+ To compile this driver as a module, choose M here: the module will
+ be called gnss.
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
new file mode 100644
index ..1f7a7baab1d9
--- /dev/null
+++ b/drivers/gnss/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the GNSS subsystem.
+#
+
+obj-$(CONFIG_GNSS) += gnss.o
+gnss-y := core.o
diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
new file mode 100644
index ..9b88c7e72233
--- /dev/null
+++ b/drivers/gnss/core.c
@@ -0,0 +1,385 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GNSS receiver core
+ *
+ * Copyright (C) 2018 Johan Hovold 
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define GNSS_MINORS16
+
+static DEFINE_IDA(gnss_minors);
+static dev_t gnss_first;
+
+/* FIFO size must be a power of two */
+#define GNSS_READ_FIFO_SIZE4096
+#define GNSS_WRITE_BUF_SIZE1024
+
+#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(>dev);
+
+   nonseekable_open(inode, file);
+   file->private_data = gdev;
+
+   down_write(>rwsem);
+   if (gdev->disconnected) {
+ 

[PATCH 1/7] gnss: add GNSS receiver subsystem

[Johan Hovold]

Add a new subsystem for GNSS (e.g. GPS) receivers.

While GNSS receivers are typically accessed using a UART interface they
often also support other I/O interfaces such as I2C, SPI and USB, while
yet other devices use iomem or even some form of remote-processor
messaging (rpmsg).

The new GNSS subsystem abstracts the underlying interface and provides a
new "gnss" class type, which exposes a character-device interface (e.g.
/dev/gnss0) to user space. This allows GNSS receivers to have a
representation in the Linux device model, something which is important
not least for power management purposes.

Note that the character-device interface provides raw access to whatever
protocol the receiver is (currently) using, such as NMEA 0183, UBX or
SiRF Binary. These protocols are expected to be continued to be handled
by user space for the time being, even if some hybrid solutions are also
conceivable (e.g. to have kernel drivers issue management commands).

This will still allow for better platform integration by allowing GNSS
devices and their resources (e.g. regulators and enable-gpios) to be
described by firmware and managed by kernel drivers rather than
platform-specific scripts and services.

While the current interface is kept minimal, it could be extended using
IOCTLs, sysfs or uevents as needs and proper abstraction levels are
identified and determined (e.g. for device and feature identification).

Signed-off-by: Johan Hovold 
---
 MAINTAINERS   |   6 +
 drivers/Kconfig   |   2 +
 drivers/Makefile  |   1 +
 drivers/gnss/Kconfig  |  11 ++
 drivers/gnss/Makefile |   7 +
 drivers/gnss/core.c   | 385 ++
 include/linux/gnss.h  |  64 +++
 7 files changed, 476 insertions(+)
 create mode 100644 drivers/gnss/Kconfig
 create mode 100644 drivers/gnss/Makefile
 create mode 100644 drivers/gnss/core.c
 create mode 100644 include/linux/gnss.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 0a1410d5a621..dc3df211c1a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5962,6 +5962,12 @@ F:   Documentation/isdn/README.gigaset
 F: drivers/isdn/gigaset/
 F: include/uapi/linux/gigaset_dev.h
 
+GNSS SUBSYSTEM
+M: Johan Hovold 
+S: Maintained
+F: drivers/gnss/
+F: include/linux/gnss.h
+
 GO7007 MPEG CODEC
 M: Hans Verkuil 
 L: linux-me...@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 95b9ccc08165..ab4d43923c4d 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -9,6 +9,8 @@ source "drivers/bus/Kconfig"
 
 source "drivers/connector/Kconfig"
 
+source "drivers/gnss/Kconfig"
+
 source "drivers/mtd/Kconfig"
 
 source "drivers/of/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd47014657..cc9a7c5f7d2c 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -185,3 +185,4 @@ obj-$(CONFIG_TEE)   += tee/
 obj-$(CONFIG_MULTIPLEXER)  += mux/
 obj-$(CONFIG_UNISYS_VISORBUS)  += visorbus/
 obj-$(CONFIG_SIOX) += siox/
+obj-$(CONFIG_GNSS) += gnss/
diff --git a/drivers/gnss/Kconfig b/drivers/gnss/Kconfig
new file mode 100644
index ..103fcc70992e
--- /dev/null
+++ b/drivers/gnss/Kconfig
@@ -0,0 +1,11 @@
+#
+# GNSS receiver configuration
+#
+
+menuconfig GNSS
+   tristate "GNSS receiver support"
+   ---help---
+ Say Y here if you have a GNSS receiver (e.g. a GPS receiver).
+
+ To compile this driver as a module, choose M here: the module will
+ be called gnss.
diff --git a/drivers/gnss/Makefile b/drivers/gnss/Makefile
new file mode 100644
index ..1f7a7baab1d9
--- /dev/null
+++ b/drivers/gnss/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the GNSS subsystem.
+#
+
+obj-$(CONFIG_GNSS) += gnss.o
+gnss-y := core.o
diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
new file mode 100644
index ..9b88c7e72233
--- /dev/null
+++ b/drivers/gnss/core.c
@@ -0,0 +1,385 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GNSS receiver core
+ *
+ * Copyright (C) 2018 Johan Hovold 
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define GNSS_MINORS16
+
+static DEFINE_IDA(gnss_minors);
+static dev_t gnss_first;
+
+/* FIFO size must be a power of two */
+#define GNSS_READ_FIFO_SIZE4096
+#define GNSS_WRITE_BUF_SIZE1024
+
+#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(>dev);
+
+   nonseekable_open(inode, file);
+   file->private_data = gdev;
+
+   down_write(>rwsem);
+   if (gdev->disconnected) {
+   ret = -ENODEV;
+   goto unlock;
+   }
+
+   if