Re: [PATCH 1/5] gnss: sirf: write data to gnss only when the gnss device is open

[Andreas Kemnade <andreas@kemnade.info>]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

On Wed, 5 Dec 2018 15:47:39 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Sun, Nov 18, 2018 at 10:57:57PM +0100, Andreas Kemnade wrote:
> > The api forbids writing data there otherwise. Prepare for the
> > serdev_open()/close() being a part of runtime pm.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  drivers/gnss/sirf.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)  
>  
> > @@ -73,6 +76,7 @@ static void sirf_close(struct gnss_device *gdev)
> >  	serdev_device_close(serdev);
> >  
> >  	pm_runtime_put(&serdev->dev);
> > +	data->opened = false;
> >  }
> >  
> >  static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
> > @@ -105,7 +109,17 @@ static int sirf_receive_buf(struct serdev_device *serdev,
> >  	struct sirf_data *data = serdev_device_get_drvdata(serdev);
> >  	struct gnss_device *gdev = data->gdev;
> >  
> > -	return gnss_insert_raw(gdev, buf, count);
> > +	/*
> > +	 * we might come here everytime when runtime is resumed
> > +	 * and data is received. Two cases are possible
> > +	 * 1. device is opened during initialisation
> > +	 * 2. kernel is compiled without runtime pm
> > +	 *    and device is opened all the time
> > +	 */
> > +	if (data->opened)
> > +		return gnss_insert_raw(gdev, buf, count);  
> 
> This can race with sirf_close() when you move serdev handling out of
> sirf_open()/close(). Not sure how best to handle that yet.
> 
Ok, first lets check whether we have a common idea of the problem before
coming to a solution. So race condition here can happen if the serdev
is still opened after the pm_runtime_put in sirf_close() which might
happen if runtime is not suspended after that. I missed that case.
Then if (data->opened) 
is checked, data->opened set to false
then gnss_insert_raw() is executed.
There is data inserted into the fifo which will be read by the next one
opening the gnss device. And there might be trouble at deregistering
time.

And now if we simply add locks:
For sirf_receive_buf(), it somehow feels dangerous if we call
gnss_insert_raw() with a lock held but I have not analyzed it
thoroughly. For sirf_close(), we could simply
put a lock around data->opened = false;
Am I missing something?

I will check if anything changes when I move the serdev_open()/close()
calls a bit. In the end me end to have the serdev open whenever the
userspace wants data or we want to change the state of the gnss chip
without wakeup.
Maybe add a special driver state for that situation where the power
state of the gps chip is changed without having it opened. If we check
that at the beginning of that action, we might have luck.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]