On Wed, 5 Dec 2018 15:47:39 +0100 Johan Hovold <johan@xxxxxxxxxx> 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@xxxxxxxxxxxx> > > --- > > 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:
pgpyBkrMMGNsL.pgp
Description: OpenPGP digital signature
