On Sun, Dec 09, 2018 at 08:51:46PM +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>
> ---
> Changes in v2:
> add locking
>
> drivers/gnss/sirf.c | 28 +++++++++++++++++++++++++++-
> 1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index 2c22836d3ffd..ba663de1db49 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -35,6 +35,12 @@ struct sirf_data {
> struct gpio_desc *wakeup;
> int irq;
> bool active;
> + /*
> + * There might be races between returning data and closing the gnss
> + * device.
> + */
Please drop this comment, which is too verbose. The mutex protects the
opened flag, and that could be indicated using a new line above the
mutex and below the flag, or using a short comment before the mutex.
> + struct mutex gdev_mutex;
Please rename "mutex". We should be able to reuse this for the serdev
open count as well, right?
> + bool opened;
Rename "open" (i.e. same tense as "active").
And just add a newline here too.
> wait_queue_head_t power_wait;
> };
>
> @@ -44,6 +50,7 @@ static int sirf_open(struct gnss_device *gdev)
> struct serdev_device *serdev = data->serdev;
> int ret;
>
> + data->opened = true;
Always hold the mutex when manipulating the open flag so we don't have
to worry about ordering issues.
> ret = serdev_device_open(serdev);
> if (ret)
> return ret;
> @@ -55,6 +62,7 @@ static int sirf_open(struct gnss_device *gdev)
> if (ret < 0) {
> dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
> pm_runtime_put_noidle(&serdev->dev);
> + data->opened = false;
And to avoid problems on error paths.
> goto err_close;
> }
>
> @@ -74,6 +82,9 @@ static void sirf_close(struct gnss_device *gdev)
> serdev_device_close(serdev);
>
> pm_runtime_put(&serdev->dev);
Add a newline here.
> + mutex_lock(&data->gdev_mutex);
> + data->opened = false;
> + mutex_unlock(&data->gdev_mutex);
> }
>
> static int sirf_write_raw(struct gnss_device *gdev, const unsigned char *buf,
> @@ -105,8 +116,22 @@ static int sirf_receive_buf(struct serdev_device *serdev,
> {
> struct sirf_data *data = serdev_device_get_drvdata(serdev);
> struct gnss_device *gdev = data->gdev;
> + int ret = 0;
> +
> + /*
> + * 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
> + */
This comments makes little sense with the current code. Please remove.
> + mutex_lock(&data->gdev_mutex);
> + if (data->opened)
> + ret = gnss_insert_raw(gdev, buf, count);
>
No new line (or add one after mutex_lock() above).
> - return gnss_insert_raw(gdev, buf, count);
> + mutex_unlock(&data->gdev_mutex);
> +
> + return ret;
> }
>
> static const struct serdev_device_ops sirf_serdev_ops = {
> @@ -275,6 +300,7 @@ static int sirf_probe(struct serdev_device *serdev)
> data->serdev = serdev;
> data->gdev = gdev;
>
> + mutex_init(&data->gdev_mutex);
> init_waitqueue_head(&data->power_wait);
>
> serdev_device_set_drvdata(serdev, data);
Johan
