[PATCH v2 0/5] gnss: sirf: add support for w2sg0004 + lna

[PATCH v2 0/5] gnss: sirf: add support for w2sg0004 + lna

[Andreas Kemnade]

Here is another chapter of the story to get gta04 gnss power
management into the mainline kernel.
There is a w2sg0004 without wakeup line in there, so power state
can only be determined indirectly by looking at the serial data lines.
Then there as also an lna which needs to be powered for real gps
reception.

Depends on:
  gnss: sirf: fix activation retry handling

Changes in v2:
  - do not change behavior of devices with wakeup line
    - do not keep serdev open if runtime is active and device is not used
  - style cleanup
  - locking of sirf_close() vs. gnss_insert_raw()
  - name reordering

Andreas Kemnade (5):
  gnss: sirf: write data to gnss only when the gnss device is open
  gnss: sirf: power on logic for devices without wakeup signal
  dt-bindings: gnss: add w2sg0004 compatible string
  gnss: sirf: add a separate supply for a lna
  dt-bindings: gnss: add lna-supply property

 .../devicetree/bindings/gnss/sirfstar.txt          |   2 +
 drivers/gnss/sirf.c                                | 158 ++++++++++++++++++---
 2 files changed, 137 insertions(+), 23 deletions(-)

-- 
2.11.0

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

[Andreas Kemnade]

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>
---
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.
+	 */
+	struct mutex gdev_mutex;
+	bool opened;
 	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;
 	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;
 		goto err_close;
 	}
 
@@ -74,6 +82,9 @@ static void sirf_close(struct gnss_device *gdev)
 	serdev_device_close(serdev);
 
 	pm_runtime_put(&serdev->dev);
+	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
+	 */
+	mutex_lock(&data->gdev_mutex);
+	if (data->opened)
+		ret = gnss_insert_raw(gdev, buf, count);
 
-	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);
-- 
2.11.0

[PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Andreas Kemnade]

Some Wi2Wi devices do not have a wakeup output, so device state can
only be indirectly detected by looking whether there is communitcation
over the serial lines.
Additionally it checks for the initial state of the device during
probing to ensure it is off.
Timeout values need to be increased, because the reaction on serial line
is slower and are in line  with previous patches by
Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v2:
 - style cleanup
 - do not keep serdev open just because runtime is active,
   only when needed (gnss device is opened or state is changed)
 - clearer timeout semantics

 drivers/gnss/sirf.c | 114 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 91 insertions(+), 23 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index ba663de1db49..c64369494afb 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -23,8 +23,13 @@
 
 #define SIRF_BOOT_DELAY			500
 #define SIRF_ON_OFF_PULSE_TIME		100
+/* activate till reaction of wakeup pin */
 #define SIRF_ACTIVATE_TIMEOUT		200
+/* activate till reception of data */
+#define SIRF_ACTIVATE_TILL_OUTPUT_TIMEOUT	1000
 #define SIRF_HIBERNATE_TIMEOUT		200
+/* If no data arrives for this time, we expect the chip to be off. */
+#define SIRF_MIN_SILENCE	1000
 
 struct sirf_data {
 	struct gnss_device *gdev;
@@ -41,9 +46,44 @@ struct sirf_data {
 	 */
 	struct mutex gdev_mutex;
 	bool opened;
+	int serdev_usecount;
+	/*
+	 * serdev will be opened for power state changing and when userspace
+	 * needs it, so we have a usecount and need locking.
+	 */
+	struct mutex serdev_mutex;
 	wait_queue_head_t power_wait;
 };
 
+static int sirf_dev_get(struct sirf_data *data)
+{
+	int ret = 0;
+
+	mutex_lock(&data->serdev_mutex);
+	data->serdev_usecount++;
+	if (data->serdev_usecount == 1) {
+		ret = serdev_device_open(data->serdev);
+		if (ret)
+			data->serdev_usecount--;
+
+		serdev_device_set_baudrate(data->serdev, data->speed);
+		serdev_device_set_flow_control(data->serdev, false);
+	}
+
+	mutex_unlock(&data->serdev_mutex);
+	return ret;
+}
+
+static void sirf_dev_put(struct sirf_data *data)
+{
+	mutex_lock(&data->serdev_mutex);
+	data->serdev_usecount--;
+	if (data->serdev_usecount == 0)
+		serdev_device_close(data->serdev);
+
+	mutex_unlock(&data->serdev_mutex);
+}
+
 static int sirf_open(struct gnss_device *gdev)
 {
 	struct sirf_data *data = gnss_get_drvdata(gdev);
@@ -51,13 +91,10 @@ static int sirf_open(struct gnss_device *gdev)
 	int ret;
 
 	data->opened = true;
-	ret = serdev_device_open(serdev);
+	ret = sirf_dev_get(data);
 	if (ret)
 		return ret;
 
-	serdev_device_set_baudrate(serdev, data->speed);
-	serdev_device_set_flow_control(serdev, false);
-
 	ret = pm_runtime_get_sync(&serdev->dev);
 	if (ret < 0) {
 		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
@@ -69,7 +106,7 @@ static int sirf_open(struct gnss_device *gdev)
 	return 0;
 
 err_close:
-	serdev_device_close(serdev);
+	sirf_dev_put(data);
 
 	return ret;
 }
@@ -78,9 +115,7 @@ static void sirf_close(struct gnss_device *gdev)
 {
 	struct sirf_data *data = gnss_get_drvdata(gdev);
 	struct serdev_device *serdev = data->serdev;
-
-	serdev_device_close(serdev);
-
+	sirf_dev_put(data);
 	pm_runtime_put(&serdev->dev);
 	mutex_lock(&data->gdev_mutex);
 	data->opened = false;
@@ -118,12 +153,16 @@ static int sirf_receive_buf(struct serdev_device *serdev,
 	struct gnss_device *gdev = data->gdev;
 	int ret = 0;
 
+	if (!data->wakeup && !data->active) {
+		data->active = true;
+		wake_up_interruptible(&data->power_wait);
+	}
+
 	/*
-	 * 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
+	 * We might come here everytime when runtime is resumed
+	 * and data is received. Two cases are possible:
+	 * 1. during power state change
+	 * 2. userspace has opened the gnss device
 	 */
 	mutex_lock(&data->gdev_mutex);
 	if (data->opened)
@@ -161,6 +200,24 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
 {
 	int ret;
 
+	if (!data->wakeup && !active && data->active) {
+		/* Wait for the chip to turn off. */
+		msleep(timeout);
+		data->active = false;
+		/* Now check if it is really off. */
+		ret = wait_event_interruptible_timeout(data->power_wait,
+			data->active,
+			msecs_to_jiffies(SIRF_MIN_SILENCE));
+
+		if (ret < 0)
+			return ret;
+
+		/* We are still getting data. -> state change timeout */
+		if (ret > 0)
+			return -ETIMEDOUT;
+		return 0;
+	}
+
 	ret = wait_event_interruptible_timeout(data->power_wait,
 			data->active == active, msecs_to_jiffies(timeout));
 	if (ret < 0)
@@ -189,13 +246,23 @@ static int sirf_set_active(struct sirf_data *data, bool active)
 	int ret;
 
 	if (active)
-		timeout = SIRF_ACTIVATE_TIMEOUT;
+		timeout = data->wakeup ?
+			SIRF_ACTIVATE_TIMEOUT :
+			SIRF_ACTIVATE_TILL_OUTPUT_TIMEOUT;
 	else
 		timeout = SIRF_HIBERNATE_TIMEOUT;
 
 	do {
+		/* Open the serdev of wakeup-less devices to check for input. */
+		if (!data->wakeup) {
+			ret = sirf_dev_get(data);
+			if (ret)
+				return ret;
+		}
 		sirf_pulse_on_off(data);
 		ret = sirf_wait_for_power_state(data, active, timeout);
+		if (!data->wakeup)
+			sirf_dev_put(data);
 		if (ret < 0) {
 			if (ret == -ETIMEDOUT)
 				continue;
@@ -301,6 +368,7 @@ static int sirf_probe(struct serdev_device *serdev)
 	data->gdev = gdev;
 
 	mutex_init(&data->gdev_mutex);
+	mutex_init(&data->serdev_mutex);
 	init_waitqueue_head(&data->power_wait);
 
 	serdev_device_set_drvdata(serdev, data);
@@ -327,15 +395,6 @@ static int sirf_probe(struct serdev_device *serdev)
 		if (IS_ERR(data->wakeup))
 			goto err_put_device;
 
-		/*
-		 * Configurations where WAKEUP has been left not connected,
-		 * are currently not supported.
-		 */
-		if (!data->wakeup) {
-			dev_err(dev, "no wakeup gpio specified\n");
-			ret = -ENODEV;
-			goto err_put_device;
-		}
 	}
 
 	if (data->wakeup) {
@@ -365,6 +424,14 @@ static int sirf_probe(struct serdev_device *serdev)
 	if (IS_ENABLED(CONFIG_PM)) {
 		pm_runtime_set_suspended(dev);	/* clear runtime_error flag */
 		pm_runtime_enable(dev);
+		/*
+		 * Device might be enabled at boot, so lets first enable it,
+		 * then disable it to bring it into a clear state.
+		 */
+		ret = pm_runtime_get_sync(dev);
+		if (ret)
+			goto err_disable_rpm;
+		pm_runtime_put(dev);
 	} else {
 		ret = sirf_runtime_resume(dev);
 		if (ret < 0)
@@ -412,6 +479,7 @@ static void sirf_remove(struct serdev_device *serdev)
 static const struct of_device_id sirf_of_match[] = {
 	{ .compatible = "fastrax,uc430" },
 	{ .compatible = "linx,r4" },
+	{ .compatible = "wi2wi,w2sg0004" },
 	{ .compatible = "wi2wi,w2sg0008i" },
 	{ .compatible = "wi2wi,w2sg0084i" },
 	{},
-- 
2.11.0

[PATCH v2 3/5] dt-bindings: gnss: add w2sg0004 compatible string

[Andreas Kemnade]

Add w2sg0004 compatible string since devices without wakeup
pins are now supported.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v2:
 - reordering of entries
With the original order there was a
Reviewed-by: Rob Herring <robh@kernel.org>

 Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gnss/sirfstar.txt b/Documentation/devicetree/bindings/gnss/sirfstar.txt
index 648d183cdb77..f4252b6b660b 100644
--- a/Documentation/devicetree/bindings/gnss/sirfstar.txt
+++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
@@ -12,6 +12,7 @@ Required properties:
 
 			"fastrax,uc430"
 			"linx,r4"
+			"wi2wi,w2sg0004"
 			"wi2wi,w2sg0008i"
 			"wi2wi,w2sg0084i"
 
-- 
2.11.0

[PATCH v2 4/5] gnss: sirf: add a separate supply for a lna

[Andreas Kemnade]

Devices might have a separate lna between antenna output of the gps
chip and the antenna which might have a separate supply.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v2:
 - handle lna also if there is no on-off gpio
 - rebase on changed 2/5

 drivers/gnss/sirf.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
index c64369494afb..d339e8ef2508 100644
--- a/drivers/gnss/sirf.c
+++ b/drivers/gnss/sirf.c
@@ -36,6 +36,7 @@ struct sirf_data {
 	struct serdev_device *serdev;
 	speed_t	speed;
 	struct regulator *vcc;
+	struct regulator *lna;
 	struct gpio_desc *on_off;
 	struct gpio_desc *wakeup;
 	int irq;
@@ -282,21 +283,32 @@ static int sirf_set_active(struct sirf_data *data, bool active)
 static int sirf_runtime_suspend(struct device *dev)
 {
 	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret = 0;
 
 	if (!data->on_off)
-		return regulator_disable(data->vcc);
+		ret = regulator_disable(data->vcc);
+	else
+		ret = sirf_set_active(data, false);
 
-	return sirf_set_active(data, false);
+	if (ret)
+		return ret;
+
+	return regulator_disable(data->lna);
 }
 
 static int sirf_runtime_resume(struct device *dev)
 {
 	struct sirf_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regulator_enable(data->lna);
+	if (ret)
+		return ret;
 
 	if (!data->on_off)
 		return regulator_enable(data->vcc);
-
-	return sirf_set_active(data, true);
+	else
+		return sirf_set_active(data, true);
 }
 
 static int __maybe_unused sirf_suspend(struct device *dev)
@@ -384,6 +396,12 @@ static int sirf_probe(struct serdev_device *serdev)
 		goto err_put_device;
 	}
 
+	data->lna = devm_regulator_get(dev, "lna");
+	if (IS_ERR(data->lna)) {
+		ret = PTR_ERR(data->lna);
+		goto err_put_device;
+	}
+
 	data->on_off = devm_gpiod_get_optional(dev, "sirf,onoff",
 			GPIOD_OUT_LOW);
 	if (IS_ERR(data->on_off))
-- 
2.11.0

[PATCH v2 5/5] dt-bindings: gnss: add lna-supply property

[Andreas Kemnade]

Add lna-supply property.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Changes in v2:
 - reordering
Original order had a
Reviewed-by: Rob Herring <robh@kernel.org>

 Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gnss/sirfstar.txt b/Documentation/devicetree/bindings/gnss/sirfstar.txt
index f4252b6b660b..8d24f37ea1ba 100644
--- a/Documentation/devicetree/bindings/gnss/sirfstar.txt
+++ b/Documentation/devicetree/bindings/gnss/sirfstar.txt
@@ -26,6 +26,7 @@ Required properties (SPI):
 
 Optional properties:
 
+- lna-supply		: Separate supply for a LNA
 - sirf,onoff-gpios	: GPIO used to power on and off device (pin name: ON_OFF)
 - sirf,wakeup-gpios	: GPIO used to determine device power state
 			  (pin name: RFPWRUP, WAKEUP)
-- 
2.11.0

Re: [Letux-kernel] [PATCH v2 4/5] gnss: sirf: add a separate supply for a lna

[H. Nikolaus Schaller]

Hi,

> Am 09.12.2018 um 20:51 schrieb Andreas Kemnade <andreas@kemnade.info>:
> 
> Devices might have a separate lna between antenna output of the gps

some nitpicking: the LNA (low noise amplifier) is between the antenna
and the GPS receiver. Therefore:

s/between antenna output of the gps/between antenna input of the gps/

Otherwise fine!

BR and thanks,
Nikolaus

> chip and the antenna which might have a separate supply.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v2:
> - handle lna also if there is no on-off gpio
> - rebase on changed 2/5
> 
> drivers/gnss/sirf.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index c64369494afb..d339e8ef2508 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -36,6 +36,7 @@ struct sirf_data {
> 	struct serdev_device *serdev;
> 	speed_t	speed;
> 	struct regulator *vcc;
> +	struct regulator *lna;
> 	struct gpio_desc *on_off;
> 	struct gpio_desc *wakeup;
> 	int irq;
> @@ -282,21 +283,32 @@ static int sirf_set_active(struct sirf_data *data, bool active)
> static int sirf_runtime_suspend(struct device *dev)
> {
> 	struct sirf_data *data = dev_get_drvdata(dev);
> +	int ret = 0;
> 
> 	if (!data->on_off)
> -		return regulator_disable(data->vcc);
> +		ret = regulator_disable(data->vcc);
> +	else
> +		ret = sirf_set_active(data, false);
> 
> -	return sirf_set_active(data, false);
> +	if (ret)
> +		return ret;
> +
> +	return regulator_disable(data->lna);
> }
> 
> static int sirf_runtime_resume(struct device *dev)
> {
> 	struct sirf_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regulator_enable(data->lna);
> +	if (ret)
> +		return ret;
> 
> 	if (!data->on_off)
> 		return regulator_enable(data->vcc);
> -
> -	return sirf_set_active(data, true);
> +	else
> +		return sirf_set_active(data, true);
> }
> 
> static int __maybe_unused sirf_suspend(struct device *dev)
> @@ -384,6 +396,12 @@ static int sirf_probe(struct serdev_device *serdev)
> 		goto err_put_device;
> 	}
> 
> +	data->lna = devm_regulator_get(dev, "lna");
> +	if (IS_ERR(data->lna)) {
> +		ret = PTR_ERR(data->lna);
> +		goto err_put_device;
> +	}
> +
> 	data->on_off = devm_gpiod_get_optional(dev, "sirf,onoff",
> 			GPIOD_OUT_LOW);
> 	if (IS_ERR(data->on_off))
> -- 
> 2.11.0
> 
> _______________________________________________
> http://projects.goldelico.com/p/gta04-kernel/
> Letux-kernel mailing list
> Letux-kernel@openphoenux.org
> http://lists.goldelico.com/mailman/listinfo.cgi/letux-kernel

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

[Johan Hovold]

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@kemnade.info>
> ---
> 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

Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Johan Hovold]

Please change the commit summary to something more descriptive like

	gnss: sirf: add support for configurations without wakeup signal

On Sun, Dec 09, 2018 at 08:51:47PM +0100, Andreas Kemnade wrote:
> Some Wi2Wi devices do not have a wakeup output, so device state can
> only be indirectly detected by looking whether there is communitcation
> over the serial lines.

> Additionally it checks for the initial state of the device during
> probing to ensure it is off.

Is this really needed? If so, it should probably go in a separate patch.

> Timeout values need to be increased, because the reaction on serial line
> is slower and are in line  with previous patches by

I don't think this is an accurate description, but more on that below.

> Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v2:
>  - style cleanup
>  - do not keep serdev open just because runtime is active,
>    only when needed (gnss device is opened or state is changed)
>  - clearer timeout semantics
> 
>  drivers/gnss/sirf.c | 114 +++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 91 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index ba663de1db49..c64369494afb 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -23,8 +23,13 @@
>  
>  #define SIRF_BOOT_DELAY			500
>  #define SIRF_ON_OFF_PULSE_TIME		100
> +/* activate till reaction of wakeup pin */
>  #define SIRF_ACTIVATE_TIMEOUT		200
> +/* activate till reception of data */
> +#define SIRF_ACTIVATE_TILL_OUTPUT_TIMEOUT	1000
>  #define SIRF_HIBERNATE_TIMEOUT		200
> +/* If no data arrives for this time, we expect the chip to be off. */
> +#define SIRF_MIN_SILENCE	1000

You only need to add one new define for the no-wakeup case and it should
reflect the report cycle (e.g. name it SIRF_NOWAKEUP_REPORT_CYCLE).
Specifically, it is the time we must wait in order to infer that a
device has failed to be activated, or succeeded to hibernate.

In pseudo code we have:

  activate:
   - toggle on-off
   - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
     - reception: success
     - timeout: failure

  hibernate:
   - toggle on-off
   - sleep(HIBERNATE_TIMEOUT)
   - wait(data-received, REPORT_CYCLE)
     - reception: failure
     - timeout: success

A problem with your implementation, is that you assume a report cycle
of one second, but this need to be the case. Judging from a quick look
at the sirf protocols (sirf nmea and sirf binary) report cycles can be
configured to be as long as 30s (sirf binary) or even 255s (sirf nmea).
[ To make things worse these numbers can be even higher in some
low-power modes it seems. ]

Adding just a one-second timeout (the minimum supported cycle length)
seems way too low, even if whatever value we choose will be reflected
directly in the time it takes to hibernate (suspend) the device.

And since this is configurable at runtime, the only safe value would be
the maximum one...

Perhaps we can say that no-wakeup support depends on having reasonably
short report cycles, but this needs to be documented.

>  struct sirf_data {
>  	struct gnss_device *gdev;
> @@ -41,9 +46,44 @@ struct sirf_data {
>  	 */
>  	struct mutex gdev_mutex;
>  	bool opened;
> +	int serdev_usecount;

Rename serdev_count.

> +	/*
> +	 * serdev will be opened for power state changing and when userspace
> +	 * needs it, so we have a usecount and need locking.
> +	 */

Too verbose to have here. And see if you can reuse the mutex protecting
open (opened).

> +	struct mutex serdev_mutex;
>  	wait_queue_head_t power_wait;
>  };
>  
> +static int sirf_dev_get(struct sirf_data *data)

Rename to the more descriptive sirf_serdev_open().

> +{
> +	int ret = 0;
> +
> +	mutex_lock(&data->serdev_mutex);
> +	data->serdev_usecount++;

Move increment inside conditional.

> +	if (data->serdev_usecount == 1) {
> +		ret = serdev_device_open(data->serdev);
> +		if (ret)
> +			data->serdev_usecount--;
> +
> +		serdev_device_set_baudrate(data->serdev, data->speed);
> +		serdev_device_set_flow_control(data->serdev, false);

You will crash here if open failed above.

> +	}
> +

No newline (or add one after mutex_lock() above).

> +	mutex_unlock(&data->serdev_mutex);

Newline before return please.

> +	return ret;
> +}
> +
> +static void sirf_dev_put(struct sirf_data *data)
> +{
> +	mutex_lock(&data->serdev_mutex);
> +	data->serdev_usecount--;
> +	if (data->serdev_usecount == 0)
> +		serdev_device_close(data->serdev);
> +
> +	mutex_unlock(&data->serdev_mutex);
> +}

Similar comments as for open above.

> +
>  static int sirf_open(struct gnss_device *gdev)
>  {
>  	struct sirf_data *data = gnss_get_drvdata(gdev);
> @@ -51,13 +91,10 @@ static int sirf_open(struct gnss_device *gdev)
>  	int ret;
>  
>  	data->opened = true;
> -	ret = serdev_device_open(serdev);
> +	ret = sirf_dev_get(data);
>  	if (ret)
>  		return ret;

You're leaving opened set when returning here.

>  
> -	serdev_device_set_baudrate(serdev, data->speed);
> -	serdev_device_set_flow_control(serdev, false);
> -
>  	ret = pm_runtime_get_sync(&serdev->dev);
>  	if (ret < 0) {
>  		dev_err(&gdev->dev, "failed to runtime resume: %d\n", ret);
> @@ -69,7 +106,7 @@ static int sirf_open(struct gnss_device *gdev)
>  	return 0;
>  
>  err_close:
> -	serdev_device_close(serdev);
> +	sirf_dev_put(data);
>  
>  	return ret;
>  }
> @@ -78,9 +115,7 @@ static void sirf_close(struct gnss_device *gdev)
>  {
>  	struct sirf_data *data = gnss_get_drvdata(gdev);
>  	struct serdev_device *serdev = data->serdev;
> -

Keep the newline after the declarations.

> -	serdev_device_close(serdev);
> -
> +	sirf_dev_put(data);
>  	pm_runtime_put(&serdev->dev);
>  	mutex_lock(&data->gdev_mutex);
>  	data->opened = false;
> @@ -118,12 +153,16 @@ static int sirf_receive_buf(struct serdev_device *serdev,
>  	struct gnss_device *gdev = data->gdev;
>  	int ret = 0;
>  
> +	if (!data->wakeup && !data->active) {
> +		data->active = true;
> +		wake_up_interruptible(&data->power_wait);
> +	}
> +
>  	/*
> -	 * 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
> +	 * We might come here everytime when runtime is resumed
> +	 * and data is received. Two cases are possible:
> +	 * 1. during power state change
> +	 * 2. userspace has opened the gnss device
>  	 */

Drop this comment.

>  	mutex_lock(&data->gdev_mutex);
>  	if (data->opened)
> @@ -161,6 +200,24 @@ static int sirf_wait_for_power_state(struct sirf_data *data, bool active,
>  {
>  	int ret;
>  
> +	if (!data->wakeup && !active && data->active) {

Should you really check data->active here?

> +		/* Wait for the chip to turn off. */
> +		msleep(timeout);
> +		data->active = false;
> +		/* Now check if it is really off. */
> +		ret = wait_event_interruptible_timeout(data->power_wait,
> +			data->active,
> +			msecs_to_jiffies(SIRF_MIN_SILENCE));

Continuation lines should be indented at least two tabs further.

> +

No no newline.

> +		if (ret < 0)
> +			return ret;
> +
> +		/* We are still getting data. -> state change timeout */
> +		if (ret > 0)
> +			return -ETIMEDOUT;

Add newline.

> +		return 0;
> +	}
> +
>  	ret = wait_event_interruptible_timeout(data->power_wait,
>  			data->active == active, msecs_to_jiffies(timeout));
>  	if (ret < 0)
> @@ -189,13 +246,23 @@ static int sirf_set_active(struct sirf_data *data, bool active)
>  	int ret;
>  
>  	if (active)
> -		timeout = SIRF_ACTIVATE_TIMEOUT;
> +		timeout = data->wakeup ?
> +			SIRF_ACTIVATE_TIMEOUT :
> +			SIRF_ACTIVATE_TILL_OUTPUT_TIMEOUT;

Add the report cycle length to the timeout in
sirf_wait_for_power_state() also for the activation case.

>  	else
>  		timeout = SIRF_HIBERNATE_TIMEOUT;
>  
>  	do {
> +		/* Open the serdev of wakeup-less devices to check for input. */

I'd drop this comment.

> +		if (!data->wakeup) {
> +			ret = sirf_dev_get(data);
> +			if (ret)
> +				return ret;
> +		}

And move this outside of the retry loop. No need to reopen for every
retry, right?

>  		sirf_pulse_on_off(data);
>  		ret = sirf_wait_for_power_state(data, active, timeout);
> +		if (!data->wakeup)
> +			sirf_dev_put(data);
>  		if (ret < 0) {
>  			if (ret == -ETIMEDOUT)
>  				continue;
> @@ -301,6 +368,7 @@ static int sirf_probe(struct serdev_device *serdev)
>  	data->gdev = gdev;
>  
>  	mutex_init(&data->gdev_mutex);
> +	mutex_init(&data->serdev_mutex);
>  	init_waitqueue_head(&data->power_wait);
>  
>  	serdev_device_set_drvdata(serdev, data);
> @@ -327,15 +395,6 @@ static int sirf_probe(struct serdev_device *serdev)
>  		if (IS_ERR(data->wakeup))
>  			goto err_put_device;
>  
> -		/*
> -		 * Configurations where WAKEUP has been left not connected,
> -		 * are currently not supported.
> -		 */
> -		if (!data->wakeup) {
> -			dev_err(dev, "no wakeup gpio specified\n");
> -			ret = -ENODEV;
> -			goto err_put_device;
> -		}
>  	}
>  
>  	if (data->wakeup) {
> @@ -365,6 +424,14 @@ static int sirf_probe(struct serdev_device *serdev)
>  	if (IS_ENABLED(CONFIG_PM)) {
>  		pm_runtime_set_suspended(dev);	/* clear runtime_error flag */
>  		pm_runtime_enable(dev);
> +		/*
> +		 * Device might be enabled at boot, so lets first enable it,
> +		 * then disable it to bring it into a clear state.
> +		 */
> +		ret = pm_runtime_get_sync(dev);
> +		if (ret)
> +			goto err_disable_rpm;
> +		pm_runtime_put(dev);

As mentioned above, this seems unrelated to the rest of the patch, so
break it out.

I'm not sure why it is even needed though. And you should probably just
call sirf_set_active(false) directly, if anything.


>  	} else {
>  		ret = sirf_runtime_resume(dev);
>  		if (ret < 0)

Johan

Re: [PATCH v2 3/5] dt-bindings: gnss: add w2sg0004 compatible string

[Johan Hovold]

On Sun, Dec 09, 2018 at 08:51:48PM +0100, Andreas Kemnade wrote:
> Add w2sg0004 compatible string since devices without wakeup
> pins are now supported.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v2:
>  - reordering of entries
> With the original order there was a
> Reviewed-by: Rob Herring <robh@kernel.org>

You can keep that and add my

Reviewed-by: Johan Hovold <johan@kernel.org>

too.

Re: [PATCH v2 4/5] gnss: sirf: add a separate supply for a lna

[Johan Hovold]

On Sun, Dec 09, 2018 at 08:51:49PM +0100, Andreas Kemnade wrote:
> Devices might have a separate lna between antenna output of the gps
> chip and the antenna which might have a separate supply.

Fix the s/antenna output/antenna input/ as per Nikolaus comment.

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v2:
>  - handle lna also if there is no on-off gpio
>  - rebase on changed 2/5
> 
>  drivers/gnss/sirf.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)

You also need to update the binding docs. As already discussed I think
adding this to the generic binding is appropriate (even if only sirf
implements it initially).

> diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> index c64369494afb..d339e8ef2508 100644
> --- a/drivers/gnss/sirf.c
> +++ b/drivers/gnss/sirf.c
> @@ -36,6 +36,7 @@ struct sirf_data {
>  	struct serdev_device *serdev;
>  	speed_t	speed;
>  	struct regulator *vcc;
> +	struct regulator *lna;
>  	struct gpio_desc *on_off;
>  	struct gpio_desc *wakeup;
>  	int irq;
> @@ -282,21 +283,32 @@ static int sirf_set_active(struct sirf_data *data, bool active)
>  static int sirf_runtime_suspend(struct device *dev)
>  {
>  	struct sirf_data *data = dev_get_drvdata(dev);
> +	int ret = 0;

No need to initialise.

>  	if (!data->on_off)

Perhaps invert this test now too.

> -		return regulator_disable(data->vcc);
> +		ret = regulator_disable(data->vcc);
> +	else
> +		ret = sirf_set_active(data, false);
>  
> -	return sirf_set_active(data, false);
> +	if (ret)
> +		return ret;
> +
> +	return regulator_disable(data->lna);

You need to undo the above if this call fails.

>  }
>  
>  static int sirf_runtime_resume(struct device *dev)
>  {
>  	struct sirf_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = regulator_enable(data->lna);
> +	if (ret)
> +		return ret;
>  
>  	if (!data->on_off)
>  		return regulator_enable(data->vcc);
> -
> -	return sirf_set_active(data, true);
> +	else
> +		return sirf_set_active(data, true);

You must undo the changes here too on errors since we call this function
directly from probe in one case.

>  }
>  
>  static int __maybe_unused sirf_suspend(struct device *dev)
> @@ -384,6 +396,12 @@ static int sirf_probe(struct serdev_device *serdev)
>  		goto err_put_device;
>  	}
>  
> +	data->lna = devm_regulator_get(dev, "lna");
> +	if (IS_ERR(data->lna)) {
> +		ret = PTR_ERR(data->lna);
> +		goto err_put_device;
> +	}
> +
>  	data->on_off = devm_gpiod_get_optional(dev, "sirf,onoff",
>  			GPIOD_OUT_LOW);
>  	if (IS_ERR(data->on_off))

Johan

Re: [PATCH v2 5/5] dt-bindings: gnss: add lna-supply property

[Johan Hovold]

On Sun, Dec 09, 2018 at 08:51:50PM +0100, Andreas Kemnade wrote:
> Add lna-supply property.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> Changes in v2:
>  - reordering
> Original order had a
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
>  Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
>  1 file changed, 1 insertion(+)

Ah, missed this one at first.

But shouldn't this go into the generic binding since it isn't specific
for sirf-star based receivers?

Johan

Re: [PATCH v2 5/5] dt-bindings: gnss: add lna-supply property

[Andreas Kemnade]

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

On Thu, 10 Jan 2019 13:27:57 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Sun, Dec 09, 2018 at 08:51:50PM +0100, Andreas Kemnade wrote:
> > Add lna-supply property.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > Changes in v2:
> >  - reordering
> > Original order had a
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > 
> >  Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
> >  1 file changed, 1 insertion(+)  
> 
> Ah, missed this one at first.
> 
> But shouldn't this go into the generic binding since it isn't specific
> for sirf-star based receivers?

That idea was formulated with a question mark towards Rob.
And no reaction from Rob about that.

Regards,
Andreas

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

Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Andreas Kemnade]

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

Hi Johan,

On Thu, 10 Jan 2019 13:10:38 +0100
Johan Hovold <johan@kernel.org> wrote:

> Please change the commit summary to something more descriptive like
> 
> 	gnss: sirf: add support for configurations without wakeup signal
> 
> On Sun, Dec 09, 2018 at 08:51:47PM +0100, Andreas Kemnade wrote:
> > Some Wi2Wi devices do not have a wakeup output, so device state can
> > only be indirectly detected by looking whether there is communitcation
> > over the serial lines.  
> 
> > Additionally it checks for the initial state of the device during
> > probing to ensure it is off.  
> 
> Is this really needed? If so, it should probably go in a separate patch.
> 
Well, it is the main motivation for the new try of upstreaming this again.
You know the loooong history...
It has several times messed up my power consumption statistics. As I try
to test patches on top of mainline, this has often led to false alarms
regarding pm bugs in other drivers.

We could also come from another kernel here via kexec having the device in
another state. 

And why we do not want to check for uncommon things here? We e.g. do multiple tries
for changing power state. 

> > Timeout values need to be increased, because the reaction on serial line
> > is slower and are in line  with previous patches by  
> 
> I don't think this is an accurate description, but more on that below.
> 
I do not think so, but I do not have a too strong opinion here.

> > Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > Changes in v2:
> >  - style cleanup
> >  - do not keep serdev open just because runtime is active,
> >    only when needed (gnss device is opened or state is changed)
> >  - clearer timeout semantics
> > 
> >  drivers/gnss/sirf.c | 114 +++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 91 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> > index ba663de1db49..c64369494afb 100644
> > --- a/drivers/gnss/sirf.c
> > +++ b/drivers/gnss/sirf.c
> > @@ -23,8 +23,13 @@
> >  
> >  #define SIRF_BOOT_DELAY			500
> >  #define SIRF_ON_OFF_PULSE_TIME		100
> > +/* activate till reaction of wakeup pin */
> >  #define SIRF_ACTIVATE_TIMEOUT		200
> > +/* activate till reception of data */
> > +#define SIRF_ACTIVATE_TILL_OUTPUT_TIMEOUT	1000
> >  #define SIRF_HIBERNATE_TIMEOUT		200
> > +/* If no data arrives for this time, we expect the chip to be off. */
> > +#define SIRF_MIN_SILENCE	1000  
> 
> You only need to add one new define for the no-wakeup case and it should
> reflect the report cycle (e.g. name it SIRF_NOWAKEUP_REPORT_CYCLE).
> Specifically, it is the time we must wait in order to infer that a
> device has failed to be activated, or succeeded to hibernate.
> 
GPS chips will have usually some boot messages. So it is not the
"send nmea data set every X seconds" for the wakeup case, it is
another situation deserving IMHO another name.

> In pseudo code we have:
> 
>   activate:
>    - toggle on-off
>    - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
>      - reception: success 

Note: we can also get the goodbye/shutdown message from the chip here
so there are chances of a false success, but since we initially power down,
we will rule out wrong state here.

>      - timeout: failure
> 
>   hibernate:
>    - toggle on-off
>    - sleep(HIBERNATE_TIMEOUT)
we could also additionally check here for 
   if (last_bytes_received == GOODBYE_MSG)
or alternatively check for
   if (!BOOTUP_MSG_RECEIVED)
     - return success

>    - wait(data-received, REPORT_CYCLE)
>      - reception: failure
>      - timeout: success
> 
> A problem with your implementation, is that you assume a report cycle
> of one second, but this need to be the case. Judging from a quick look
> at the sirf protocols (sirf nmea and sirf binary) report cycles can be
> configured to be as long as 30s (sirf binary) or even 255s (sirf nmea).
> [ To make things worse these numbers can be even higher in some
> low-power modes it seems. ]
> 
As far as I see we will only might have a problem if 
  - those settings are nonvolatile (or we come in with those
    settings on another way)
  - or a state change lateron fails which we cannot properly detect


> Adding just a one-second timeout (the minimum supported cycle length)
> seems way too low, even if whatever value we choose will be reflected
> directly in the time it takes to hibernate (suspend) the device.
> 
> And since this is configurable at runtime, the only safe value would be
> the maximum one...
> 
> Perhaps we can say that no-wakeup support depends on having reasonably
> short report cycles, but this needs to be documented.
> 
Where should it be documented? Comment in code?
devicetree bindings?

So in general I see these possibilities:
1. just document tho problem with low cycle length requirement
   lateron we might improve the solution

2. do the data parsing like above just for the first or/and last bytes
   if these things really come reliable, we would catch the low-power
   corner cases like only data reports if you move around.
   I have to do some research here.

3. monitor serial output in off state and send a pulse if data arrives
  this would require the serial device to do aggressive power saving
  in that time(you send an example patch for that) or giving a gpio interrupt
  coming from the rx line to the gnss driver.
  Those things look like more restructuring work (or having a separate driver
  which is not that practical for further extensions) and would not
  catch low-power modes

My personal opinion here
is
first 1.
later improve to 2. (and do some more tests in regards of 2.)

Just for the general things first.

Regards,
Andreas

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

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

[Andreas Kemnade]

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

On Thu, 10 Jan 2019 13:02:28 +0100
Johan Hovold <johan@kernel.org> wrote:

> 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@kemnade.info>
> > ---
> > 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?

No. we cannot. The problem here is that we would take the same mutex
in a serdev callback and around a serdev call. Then we have things like
that:

[   36.700408] ======================================================
[   36.706970] WARNING: possible circular locking dependency detected
[   36.713531] 5.0.0-rc1-00008-g29801984a0fa #1 Not tainted
[   36.719177] ------------------------------------------------------
[   36.725708] kworker/u2:1/16 is trying to acquire lock:
[   36.731170] (ptrval) (&data->mutex){+.+.}, at: sirf_receive_buf+0x50/0x80 [g]
[   36.739532] 
[   36.739532] but task is already holding lock:
[   36.745727] (ptrval) (&buf->lock){+.+.}, at: flush_to_ldisc+0x1c/0xcc
[   36.752593] 
[   36.752593] which lock already depends on the new lock.
[   36.752593] 
[   36.761230] 
[   36.761230] the existing dependency chain (in reverse order) is:
[   36.769165] 
[   36.769165] -> #3 (&buf->lock){+.+.}:
[   36.774658]        mutex_lock_nested+0x18/0x20
[   36.779388]        tty_buffer_flush+0x3c/0xb4
[   36.783996]        tty_ldisc_flush+0x1c/0x30
[   36.788543]        tty_port_close_start+0x17c/0x18c
[   36.793731]        tty_port_close+0x10/0x58
[   36.798187]        ttyport_close+0x40/0x58
[   36.802581]        sirf_set_active+0x1c4/0x2dc [gnss_sirf]
[   36.808410]        sirf_runtime_resume+0x54/0x58 [gnss_sirf]
[   36.814422]        __rpm_callback+0x30/0x194
[   36.818969]        rpm_callback+0x70/0x80
[   36.823242]        rpm_resume+0x58c/0x66c
[   36.827514]        __pm_runtime_resume+0x78/0x94
[   36.832458]        sirf_probe+0x170/0x278 [gnss_sirf]
[   36.837799]        serdev_drv_probe+0x28/0x44
[   36.842437]        really_probe+0x1f0/0x2c0
[   36.846893]        driver_probe_device+0x140/0x15c
[   36.851989]        __driver_attach+0x94/0xd0
[   36.856536]        bus_for_each_dev+0x64/0xa0
[   36.861175]        async_run_entry_fn+0x44/0xfc
[   36.865997]        process_one_work+0x384/0x710
[   36.870819]        worker_thread+0x290/0x3e4
[   36.875366]        kthread+0x13c/0x158
[   36.879364]        ret_from_fork+0x14/0x20
[   36.883697]          (null)
[   36.886688] 
[   36.886688] -> #2 (&tty->ldisc_sem){++++}:
[   36.892608]        tty_ldisc_lock+0x50/0x74
[   36.897094]        tty_init_dev+0xc0/0x194
[   36.901458]        tty_open+0x308/0x334
[   36.905517]        chrdev_open+0x150/0x184
[   36.909912]        do_dentry_open+0x214/0x37c
[   36.914520]        path_openat+0xdf8/0xfe8
[   36.918884]        do_filp_open+0x3c/0x9c
[   36.923156]        do_sys_open+0xf4/0x1e4
[   36.927429]        kernel_init_freeable+0x370/0x4ac
[   36.932617]        kernel_init+0x8/0x10c
[   36.936798]        ret_from_fork+0x14/0x20
[   36.941131]          (null)
[   36.944122] 
[   36.944122] -> #1 (&tty->legacy_mutex){+.+.}:
[   36.950347]        mutex_lock_nested+0x18/0x20
[   36.955047]        tty_init_dev+0x38/0x194
[   36.959411]        ttyport_open+0x28/0x140
[   36.963775]        serdev_device_open+0x28/0xa8
[   36.968627]        sirf_serdev_open.part.0+0x10/0x40 [gnss_sirf]
[   36.975006]        sirf_set_active+0x6c/0x2dc [gnss_sirf]
[   36.980743]        sirf_runtime_resume+0x54/0x58 [gnss_sirf]
[   36.986755]        __rpm_callback+0x30/0x194
[   36.991302]        rpm_callback+0x70/0x80
[   36.995574]        rpm_resume+0x58c/0x66c
[   36.999847]        __pm_runtime_resume+0x78/0x94
[   37.004791]        sirf_probe+0x170/0x278 [gnss_sirf]
[   37.010131]        serdev_drv_probe+0x28/0x44
[   37.014770]        really_probe+0x1f0/0x2c0
[   37.019226]        driver_probe_device+0x140/0x15c
[   37.024322]        __driver_attach+0x94/0xd0
[   37.028869]        bus_for_each_dev+0x64/0xa0
[   37.033508]        async_run_entry_fn+0x44/0xfc
[   37.038330]        process_one_work+0x384/0x710
[   37.043151]        worker_thread+0x290/0x3e4
[   37.047698]        kthread+0x13c/0x158
[   37.051666]        ret_from_fork+0x14/0x20
[   37.056030]          (null)
[   37.058990] 
[   37.058990] -> #0 (&data->mutex){+.+.}:
[   37.064666]        __mutex_lock+0x74/0x9c0
[   37.069030]        mutex_lock_nested+0x18/0x20
[   37.073760]        sirf_receive_buf+0x50/0x80 [gnss_sirf]
[   37.079498]        ttyport_receive_buf+0x58/0xd0
[   37.084411]        flush_to_ldisc+0x94/0xcc
[   37.088867]        process_one_work+0x384/0x710
[   37.093688]        worker_thread+0x290/0x3e4
[   37.098236]        kthread+0x13c/0x158
[   37.102203]        ret_from_fork+0x14/0x20
[   37.106567]          (null)
[   37.109527] 
[   37.109527] other info that might help us debug this:
[   37.109527] 
[   37.118011] Chain exists of:
[   37.118011]   &data->mutex --> &tty->ldisc_sem --> &buf->lock
[   37.118011] 
[   37.128723]  Possible unsafe locking scenario:
[   37.128723] 
[   37.134979]        CPU0                    CPU1
[   37.139801]        ----                    ----
[   37.144592]   lock(&buf->lock);
[   37.147949]                                lock(&tty->ldisc_sem);
[   37.154418]                                lock(&buf->lock);
[   37.160400]   lock(&data->mutex);
[   37.163940] 
[   37.163940]  *** DEADLOCK ***
[   37.163940] 
[   37.170227] 3 locks held by kworker/u2:1/16:
[   37.174743]  #0: (ptrval) ((wq_completion)"events_unbound"){+.+.}, at: proce0
[   37.184356]  #1: (ptrval) ((work_completion)(&buf->work)){+.+.}, at: process0
[   37.193786]  #2: (ptrval) (&buf->lock){+.+.}, at: flush_to_ldisc+0x1c/0xcc
[   37.201110] 
[   37.201110] stack backtrace:
[   37.205749] CPU: 0 PID: 16 Comm: kworker/u2:1 Not tainted 5.0.0-rc1-00008-g21
[   37.214508] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[   37.221160] Workqueue: events_unbound flush_to_ldisc
[   37.226501] [<c01109a4>] (unwind_backtrace) from [<c010c63c>] (show_stack+0x)
[   37.234710] [<c010c63c>] (show_stack) from [<c0811670>] (dump_stack+0x90/0xc)
[   37.242401] [<c0811670>] (dump_stack) from [<c01853e0>] (print_circular_bug.)
[   37.252197] [<c01853e0>] (print_circular_bug.constprop.17) from [<c01879b0>])
[   37.262451] [<c01879b0>] (__lock_acquire) from [<c0188e3c>] (lock_acquire+0x)
[   37.270843] [<c0188e3c>] (lock_acquire) from [<c082afd0>] (__mutex_lock+0x74)
[   37.278991] [<c082afd0>] (__mutex_lock) from [<c082b934>] (mutex_lock_nested)
[   37.287506] [<c082b934>] (mutex_lock_nested) from [<bf0330d0>] (sirf_receive)
[   37.297485] [<bf0330d0>] (sirf_receive_buf [gnss_sirf]) from [<c054c63c>] (t)
[   37.307647] [<c054c63c>] (ttyport_receive_buf) from [<c0532260>] (flush_to_l)
[   37.316497] [<c0532260>] (flush_to_ldisc) from [<c014ae08>] (process_one_wor)
[   37.325286] [<c014ae08>] (process_one_work) from [<c014c300>] (worker_thread)
[   37.333984] [<c014c300>] (worker_thread) from [<c01515f0>] (kthread+0x13c/0x)
[   37.341827] [<c01515f0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x2)
[   37.349487] Exception stack(0xeea17fb0 to 0xeea17ff8)
[   37.354858] 7fa0:                                     00000000 00000000 00000
[   37.363525] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000
[   37.372192] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
000000�[   62.887756] random: crng init done
M_FCLK��00
    DLES[  248.800109] INFO: task kworker/u2:1:16 blocked for more than 120 sec.
[  248.807495]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  248.814575] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  248.823333] kworker/u2:1    D    0    16      2 0x00000000
[  248.831909] Workqueue: events_unbound flush_to_ldisc
[  248.837310] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  248.844909] [<c0829ca4>] (schedule) from [<c082a0f0>] (schedule_preempt_disa)
[  248.853912] [<c082a0f0>] (schedule_preempt_disabled) from [<c082b7d4>] (__mu)
[  248.863433] [<c082b7d4>] (__mutex_lock) from [<c082b934>] (mutex_lock_nested)
[  248.872070] [<c082b934>] (mutex_lock_nested) from [<bf0330d0>] (sirf_receive)
[  248.882202] [<bf0330d0>] (sirf_receive_buf [gnss_sirf]) from [<c054c63c>] (t)
[  248.892425] [<c054c63c>] (ttyport_receive_buf) from [<c0532260>] (flush_to_l)
[  248.901428] [<c0532260>] (flush_to_ldisc) from [<c014ae08>] (process_one_wor)
[  248.910308] [<c014ae08>] (process_one_work) from [<c014c300>] (worker_thread)
[  248.919128] [<c014c300>] (worker_thread) from [<c01515f0>] (kthread+0x13c/0x)
[  248.927032] [<c01515f0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x2)
[  248.934844] Exception stack(0xeea17fb0 to 0xeea17ff8)
[  248.940368] 7fa0:                                     00000000 00000000 00000
[  248.949188] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000
[  248.957916] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[  248.965118] INFO: task kworker/u2:2:35 blocked for more than 120 seconds.
[  248.972473]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  248.978820] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  248.987091] kworker/u2:2    D    0    35      2 0x00000000
[  248.993011] Workqueue: events_unbound async_run_entry_fn
[  248.998809] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  249.006256] [<c0829ca4>] (schedule) from [<c082a0f0>] (schedule_preempt_disa)
[  249.015167] [<c082a0f0>] (schedule_preempt_disabled) from [<c082b7d4>] (__mu)
[  249.024658] [<c082b7d4>] (__mutex_lock) from [<c082b934>] (mutex_lock_nested)
[  249.033264] [<c082b934>] (mutex_lock_nested) from [<c0532474>] (tty_buffer_f)
[  249.042236] [<c0532474>] (tty_buffer_flush) from [<c0531478>] (tty_ldisc_flu)
[  249.050994] [<c0531478>] (tty_ldisc_flush) from [<c0533060>] (tty_port_close)
[  249.060333] [<c0533060>] (tty_port_close_start) from [<c0533080>] (tty_port_)
[  249.069366] [<c0533080>] (tty_port_close) from [<c054c8d8>] (ttyport_close+0)
[  249.077758] [<c054c8d8>] (ttyport_close) from [<bf033618>] (sirf_set_active+)
[  249.087554] [<bf033618>] (sirf_set_active [gnss_sirf]) from [<bf033784>] (si)
[  249.098785] [<bf033784>] (sirf_runtime_resume [gnss_sirf]) from [<c056d98c>])
[  249.108947] [<c056d98c>] (__rpm_callback) from [<c056db60>] (rpm_callback+0x)
[  249.117187] [<c056db60>] (rpm_callback) from [<c056d7e8>] (rpm_resume+0x58c/)
[  249.125366] [<c056d7e8>] (rpm_resume) from [<c056d940>] (__pm_runtime_resume)
[  249.133972] [<c056d940>] (__pm_runtime_resume) from [<bf0332cc>] (sirf_probe)
[  249.143829] [<bf0332cc>] (sirf_probe [gnss_sirf]) from [<c054c1b4>] (serdev_)
[  249.153228] [<c054c1b4>] (serdev_drv_probe) from [<c0562a60>] (really_probe+)
[  249.161926] [<c0562a60>] (really_probe) from [<c0562dbc>] (driver_probe_devi)
[  249.170867] [<c0562dbc>] (driver_probe_device) from [<c0562e6c>] (__driver_a)
[  249.179931] [<c0562e6c>] (__driver_attach) from [<c056104c>] (bus_for_each_d)
[  249.188659] [<c056104c>] (bus_for_each_dev) from [<c0154db4>] (async_run_ent)
[  249.197692] [<c0154db4>] (async_run_entry_fn) from [<c014ae08>] (process_one)
[  249.206939] [<c014ae08>] (process_one_work) from [<c014c300>] (worker_thread)
[  249.215728] [<c014c300>] (worker_thread) from [<c01515f0>] (kthread+0x13c/0x)
[  249.223663] [<c01515f0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x2)
[  249.231414] Exception stack(0xeea7ffb0 to 0xeea7fff8)
[  249.236846] ffa0:                                     00000000 00000000 00000
[  249.245635] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000
[  249.254394] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
[  249.261566] INFO: task modprobe:1100 blocked for more than 120 seconds.
[  249.268646]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  249.274963] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  249.283386] modprobe        D    0  1100   1090 0x00000000
[  249.289337] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  249.296874] [<c0829ca4>] (schedule) from [<c0155124>] (async_synchronize_coo)
[  249.306488] [<c0155124>] (async_synchronize_cookie_domain) from [<c01d0148>])
[  249.316619] [<c01d0148>] (do_init_module) from [<c01cedcc>] (load_module+0x1)
[  249.325225] [<c01cedcc>] (load_module) from [<c01cf69c>] (sys_finit_module+0)
[  249.333648] [<c01cf69c>] (sys_finit_module) from [<c0101000>] (ret_fast_sysc)
[  249.342407] Exception stack(0xedc6bfa8 to 0xedc6bff0)
[  249.347839] bfa0:                   00497b00 004aabb0 00000004 00496e80 00000
[  249.356628] bfc0: 00497b00 004aabb0 14840a00 0000017b 00040000 00000000 00000
[  249.365386] bfe0: be9fea48 be9fea38 0048d6c3 b6f3aa42
[  249.370849] INFO: lockdep is turned off.
[  371.680145] INFO: task kworker/u2:1:16 blocked for more than 120 seconds.
[  371.687530]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  371.694854] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  371.703643] kworker/u2:1    D    0    16      2 0x00000000
[  371.712249] Workqueue: events_unbound flush_to_ldisc
[  371.717620] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  371.725250] [<c0829ca4>] (schedule) from [<c082a0f0>] (schedule_preempt_disa)
[  371.734252] [<c082a0f0>] (schedule_preempt_disabled) from [<c082b7d4>] (__mu)
[  371.743743] [<c082b7d4>] (__mutex_lock) from [<c082b934>] (mutex_lock_nested)
[  371.752349] [<c082b934>] (mutex_lock_nested) from [<bf0330d0>] (sirf_receive)
[  371.762420] [<bf0330d0>] (sirf_receive_buf [gnss_sirf]) from [<c054c63c>] (t)
[  371.772674] [<c054c63c>] (ttyport_receive_buf) from [<c0532260>] (flush_to_l)
[  371.781616] [<c0532260>] (flush_to_ldisc) from [<c014ae08>] (process_one_wor)
[  371.790496] [<c014ae08>] (process_one_work) from [<c014c300>] (worker_thread)
[  371.799285] [<c014c300>] (worker_thread) from [<c01515f0>] (kthread+0x13c/0x)
[  371.807159] [<c01515f0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x2)
[  371.814941] Exception stack(0xeea17fb0 to 0xeea17ff8)
[  371.820434] 7fa0:                                     00000000 00000000 00000
[  371.829193] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000
[  371.837921] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[  371.845062] INFO: task kworker/u2:2:35 blocked for more than 120 seconds.
[  371.852416]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  371.858642] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  371.867065] kworker/u2:2    D    0    35      2 0x00000000
[  371.873077] Workqueue: events_unbound async_run_entry_fn
[  371.878814] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  371.886230] [<c0829ca4>] (schedule) from [<c082a0f0>] (schedule_preempt_disa)
[  371.895141] [<c082a0f0>] (schedule_preempt_disabled) from [<c082b7d4>] (__mu)
[  371.904693] [<c082b7d4>] (__mutex_lock) from [<c082b934>] (mutex_lock_nested)
[  371.913299] [<c082b934>] (mutex_lock_nested) from [<c0532474>] (tty_buffer_f)
[  371.922271] [<c0532474>] (tty_buffer_flush) from [<c0531478>] (tty_ldisc_flu)
[  371.931091] [<c0531478>] (tty_ldisc_flush) from [<c0533060>] (tty_port_close)
[  371.940429] [<c0533060>] (tty_port_close_start) from [<c0533080>] (tty_port_)
[  371.949523] [<c0533080>] (tty_port_close) from [<c054c8d8>] (ttyport_close+0)
[  371.957946] [<c054c8d8>] (ttyport_close) from [<bf033618>] (sirf_set_active+)
[  371.967773] [<bf033618>] (sirf_set_active [gnss_sirf]) from [<bf033784>] (si)
[  371.979064] [<bf033784>] (sirf_runtime_resume [gnss_sirf]) from [<c056d98c>])
[  371.989257] [<c056d98c>] (__rpm_callback) from [<c056db60>] (rpm_callback+0x)
[  371.997589] [<c056db60>] (rpm_callback) from [<c056d7e8>] (rpm_resume+0x58c/)
[  372.005737] [<c056d7e8>] (rpm_resume) from [<c056d940>] (__pm_runtime_resume)
[  372.014373] [<c056d940>] (__pm_runtime_resume) from [<bf0332cc>] (sirf_probe)
[  372.024291] [<bf0332cc>] (sirf_probe [gnss_sirf]) from [<c054c1b4>] (serdev_)
[  372.033752] [<c054c1b4>] (serdev_drv_probe) from [<c0562a60>] (really_probe+)
[  372.042480] [<c0562a60>] (really_probe) from [<c0562dbc>] (driver_probe_devi)
[  372.051452] [<c0562dbc>] (driver_probe_device) from [<c0562e6c>] (__driver_a)
[  372.060516] [<c0562e6c>] (__driver_attach) from [<c056104c>] (bus_for_each_d)
[  372.069335] [<c056104c>] (bus_for_each_dev) from [<c0154db4>] (async_run_ent)
[  372.078369] [<c0154db4>] (async_run_entry_fn) from [<c014ae08>] (process_one)
[  372.087615] [<c014ae08>] (process_one_work) from [<c014c300>] (worker_thread)
[  372.096405] [<c014c300>] (worker_thread) from [<c01515f0>] (kthread+0x13c/0x)
[  372.104370] [<c01515f0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x2)
[  372.112152] Exception stack(0xeea7ffb0 to 0xeea7fff8)
[  372.117645] ffa0:                                     00000000 00000000 00000
[  372.126434] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000
[  372.135223] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
[  372.142395] INFO: task modprobe:1100 blocked for more than 120 seconds.
[  372.149566]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  372.155853] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  372.164276] modprobe        D    0  1100   1090 0x00000000
[  372.170257] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  372.177825] [<c0829ca4>] (schedule) from [<c0155124>] (async_synchronize_coo)
[  372.187469] [<c0155124>] (async_synchronize_cookie_domain) from [<c01d0148>])
[  372.197631] [<c01d0148>] (do_init_module) from [<c01cedcc>] (load_module+0x1)
[  372.206237] [<c01cedcc>] (load_module) from [<c01cf69c>] (sys_finit_module+0)
[  372.214660] [<c01cf69c>] (sys_finit_module) from [<c0101000>] (ret_fast_sysc)
[  372.223449] Exception stack(0xedc6bfa8 to 0xedc6bff0)
[  372.229003] bfa0:                   00497b00 004aabb0 00000004 00496e80 00000
[  372.237701] bfc0: 00497b00 004aabb0 14840a00 0000017b 00040000 00000000 00000
[  372.246490] bfe0: be9fea48 be9fea38 0048d6c3 b6f3aa42
[  372.251983] INFO: lockdep is turned off.
[  494.560119] INFO: task kworker/u2:1:16 blocked for more than 120 seconds.
[  494.567535]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  494.574859] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  494.583618] kworker/u2:1    D    0    16      2 0x00000000
[  494.592224] Workqueue: events_unbound flush_to_ldisc
[  494.597625] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  494.605255] [<c0829ca4>] (schedule) from [<c082a0f0>] (schedule_preempt_disa)
[  494.614227] [<c082a0f0>] (schedule_preempt_disabled) from [<c082b7d4>] (__mu)
[  494.623718] [<c082b7d4>] (__mutex_lock) from [<c082b934>] (mutex_lock_nested)
[  494.632324] [<c082b934>] (mutex_lock_nested) from [<bf0330d0>] (sirf_receive)
[  494.642395] [<bf0330d0>] (sirf_receive_buf [gnss_sirf]) from [<c054c63c>] (t)
[  494.652618] [<c054c63c>] (ttyport_receive_buf) from [<c0532260>] (flush_to_l)
[  494.661621] [<c0532260>] (flush_to_ldisc) from [<c014ae08>] (process_one_wor)
[  494.670471] [<c014ae08>] (process_one_work) from [<c014c300>] (worker_thread)
[  494.679260] [<c014c300>] (worker_thread) from [<c01515f0>] (kthread+0x13c/0x)
[  494.687133] [<c01515f0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x2)
[  494.694915] Exception stack(0xeea17fb0 to 0xeea17ff8)
[  494.700439] 7fa0:                                     00000000 00000000 00000
[  494.709228] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000
[  494.717956] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[  494.725097] INFO: task kworker/u2:2:35 blocked for more than 120 seconds.
[  494.732391]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  494.738647] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  494.747039] kworker/u2:2    D    0    35      2 0x00000000
[  494.753021] Workqueue: events_unbound async_run_entry_fn
[  494.758819] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  494.766265] [<c0829ca4>] (schedule) from [<c082a0f0>] (schedule_preempt_disa)
[  494.775177] [<c082a0f0>] (schedule_preempt_disabled) from [<c082b7d4>] (__mu)
[  494.784698] [<c082b7d4>] (__mutex_lock) from [<c082b934>] (mutex_lock_nested)
[  494.793304] [<c082b934>] (mutex_lock_nested) from [<c0532474>] (tty_buffer_f)
[  494.802276] [<c0532474>] (tty_buffer_flush) from [<c0531478>] (tty_ldisc_flu)
[  494.811065] [<c0531478>] (tty_ldisc_flush) from [<c0533060>] (tty_port_close)
[  494.820373] [<c0533060>] (tty_port_close_start) from [<c0533080>] (tty_port_)
[  494.829437] [<c0533080>] (tty_port_close) from [<c054c8d8>] (ttyport_close+0)
[  494.837799] [<c054c8d8>] (ttyport_close) from [<bf033618>] (sirf_set_active+)
[  494.847595] [<bf033618>] (sirf_set_active [gnss_sirf]) from [<bf033784>] (si)
[  494.858825] [<bf033784>] (sirf_runtime_resume [gnss_sirf]) from [<c056d98c>])
[  494.869018] [<c056d98c>] (__rpm_callback) from [<c056db60>] (rpm_callback+0x)
[  494.877288] [<c056db60>] (rpm_callback) from [<c056d7e8>] (rpm_resume+0x58c/)
[  494.885437] [<c056d7e8>] (rpm_resume) from [<c056d940>] (__pm_runtime_resume)
[  494.894073] [<c056d940>] (__pm_runtime_resume) from [<bf0332cc>] (sirf_probe)
[  494.903961] [<bf0332cc>] (sirf_probe [gnss_sirf]) from [<c054c1b4>] (serdev_)
[  494.913391] [<c054c1b4>] (serdev_drv_probe) from [<c0562a60>] (really_probe+)
[  494.922119] [<c0562a60>] (really_probe) from [<c0562dbc>] (driver_probe_devi)
[  494.931121] [<c0562dbc>] (driver_probe_device) from [<c0562e6c>] (__driver_a)
[  494.940185] [<c0562e6c>] (__driver_attach) from [<c056104c>] (bus_for_each_d)
[  494.948974] [<c056104c>] (bus_for_each_dev) from [<c0154db4>] (async_run_ent)
[  494.958007] [<c0154db4>] (async_run_entry_fn) from [<c014ae08>] (process_one)
[  494.967254] [<c014ae08>] (process_one_work) from [<c014c300>] (worker_thread)
[  494.976104] [<c014c300>] (worker_thread) from [<c01515f0>] (kthread+0x13c/0x)
[  494.984069] [<c01515f0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x2)
[  494.991851] Exception stack(0xeea7ffb0 to 0xeea7fff8)
[  494.997314] ffa0:                                     00000000 00000000 00000
[  495.006134] ffc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000
[  495.014892] ffe0: 00000000 00000000 00000000 00000000 00000013 00000000
[  495.022064] INFO: task modprobe:1100 blocked for more than 120 seconds.
[  495.029174]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  495.035430] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  495.043853] modprobe        D    0  1100   1090 0x00000000
[  495.049835] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  495.057342] [<c0829ca4>] (schedule) from [<c0155124>] (async_synchronize_coo)
[  495.066955] [<c0155124>] (async_synchronize_cookie_domain) from [<c01d0148>])
[  495.077117] [<c01d0148>] (do_init_module) from [<c01cedcc>] (load_module+0x1)
[  495.085693] [<c01cedcc>] (load_module) from [<c01cf69c>] (sys_finit_module+0)
[  495.094116] [<c01cf69c>] (sys_finit_module) from [<c0101000>] (ret_fast_sysc)
[  495.102874] Exception stack(0xedc6bfa8 to 0xedc6bff0)
[  495.108306] bfa0:                   00497b00 004aabb0 00000004 00496e80 00000
[  495.117095] bfc0: 00497b00 004aabb0 14840a00 0000017b 00040000 00000000 00000
[  495.125885] bfe0: be9fea48 be9fea38 0048d6c3 b6f3aa42
[  495.131378] INFO: lockdep is turned off.
[  617.440093] INFO: task kworker/u2:1:16 blocked for more than 120 seconds.
[  617.447479]       Not tainted 5.0.0-rc1-00008-g29801984a0fa #1
[  617.454589] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  617.463348] kworker/u2:1    D    0    16      2 0x00000000
[  617.471923] Workqueue: events_unbound flush_to_ldisc
[  617.477325] [<c0829b1c>] (__schedule) from [<c0829ca4>] (schedule+0x98/0xb0)
[  617.484954] [<c0829ca4>] (schedule) from [<c082a0f0>] (schedule_preempt_disa)
[  617.493927] [<c082a0f0>] (schedule_preempt_disabled) from [<c082b7d4>] (__mu)
[  617.503448] [<c082b7d4>] (__mutex_lock) from [<c082b934>] (mutex_lock_nested)
[  617.512054] [<c082b934>] (mutex_lock_nested) from [<bf0330d0>] (sirf_receive)
[  617.522155] [<bf0330d0>] (sirf_receive_buf [gnss_sirf]) from [<c054c63c>] (t)
[  617.532409] [<c054c63c>] (ttyport_receive_buf) from [<c0532260>] (flush_to_l)
[  617.541381] [<c0532260>] (flush_to_ldisc) from [<c014ae08>] (process_one_wor)
[  617.550262] [<c014ae08>] (process_one_work) from [<c014c300>] (worker_thread)
[  617.559020] [<c014c300>] (worker_thread) from [<c01515f0>] (kthread+0x13c/0x)
[  617.566925] [<c01515f0>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x2)
[  617.574707] Exception stack(0xeea17fb0 to 0xeea17ff8)
[  617.580230] 7fa0:                                     00000000 00000000 00000
[  617.588989] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000
[  617.597686] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[  617.604858] INFO: lockdep is turned off.

Regards,
Andreas

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

Re: [PATCH v2 5/5] dt-bindings: gnss: add lna-supply property

[Johan Hovold]

On Thu, Jan 10, 2019 at 06:07:32PM +0100, Andreas Kemnade wrote:
> On Thu, 10 Jan 2019 13:27:57 +0100
> Johan Hovold <johan@kernel.org> wrote:
> 
> > On Sun, Dec 09, 2018 at 08:51:50PM +0100, Andreas Kemnade wrote:
> > > Add lna-supply property.
> > > 
> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > ---
> > > Changes in v2:
> > >  - reordering
> > > Original order had a
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > 
> > >  Documentation/devicetree/bindings/gnss/sirfstar.txt | 1 +
> > >  1 file changed, 1 insertion(+)  
> > 
> > Ah, missed this one at first.
> > 
> > But shouldn't this go into the generic binding since it isn't specific
> > for sirf-star based receivers?
> 
> That idea was formulated with a question mark towards Rob.
> And no reaction from Rob about that.

Right, maybe that suggestion wasn't very clear.

But since this property is not specific to sirf-star based devices, it
needs to go in the generic binding.

Thanks,
Johan

Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Johan Hovold]

On Thu, Jan 10, 2019 at 11:02:23PM +0100, Andreas Kemnade wrote:
> Hi Johan,
> 
> On Thu, 10 Jan 2019 13:10:38 +0100
> Johan Hovold <johan@kernel.org> wrote:

> > On Sun, Dec 09, 2018 at 08:51:47PM +0100, Andreas Kemnade wrote:

> > > Additionally it checks for the initial state of the device during
> > > probing to ensure it is off.  
> > 
> > Is this really needed? If so, it should probably go in a separate patch.
> > 
> Well, it is the main motivation for the new try of upstreaming this again.
> You know the loooong history...
> It has several times messed up my power consumption statistics. As I try
> to test patches on top of mainline, this has often led to false alarms
> regarding pm bugs in other drivers.
> 
> We could also come from another kernel here via kexec having the
> device in another state. 
> 
> And why we do not want to check for uncommon things here? We e.g. do
> multiple tries for changing power state. 

You still need to argue why it is needed (e.g. the kexec case) and that
needs to go in the commit message of a separate patch adding something
like that as it is orthogonal to supporting configurations without
wakeup.

This may also be better handled by a shutdown() callback which is where
such kexec concerns are supposed to be handled, and that would also take
care of the reboot case. This way, not everyone has to pay a penalty on
every boot for the arguable rare use case of kexec.

> > > Timeout values need to be increased, because the reaction on serial line
> > > is slower and are in line  with previous patches by  
> > 
> > I don't think this is an accurate description, but more on that below.
> > 
> I do not think so, but I do not have a too strong opinion here.
> 
> > > Neil Brown <neilb@suse.de> and  H. Nikolaus Schaller <hns@goldelico.com>.
> > > 
> > > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > > ---
> > > Changes in v2:
> > >  - style cleanup
> > >  - do not keep serdev open just because runtime is active,
> > >    only when needed (gnss device is opened or state is changed)
> > >  - clearer timeout semantics
> > > 
> > >  drivers/gnss/sirf.c | 114 +++++++++++++++++++++++++++++++++++++++++-----------
> > >  1 file changed, 91 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gnss/sirf.c b/drivers/gnss/sirf.c
> > > index ba663de1db49..c64369494afb 100644
> > > --- a/drivers/gnss/sirf.c
> > > +++ b/drivers/gnss/sirf.c
> > > @@ -23,8 +23,13 @@
> > >  
> > >  #define SIRF_BOOT_DELAY			500
> > >  #define SIRF_ON_OFF_PULSE_TIME		100
> > > +/* activate till reaction of wakeup pin */
> > >  #define SIRF_ACTIVATE_TIMEOUT		200
> > > +/* activate till reception of data */
> > > +#define SIRF_ACTIVATE_TILL_OUTPUT_TIMEOUT	1000
> > >  #define SIRF_HIBERNATE_TIMEOUT		200
> > > +/* If no data arrives for this time, we expect the chip to be off. */
> > > +#define SIRF_MIN_SILENCE	1000  
> > 
> > You only need to add one new define for the no-wakeup case and it should
> > reflect the report cycle (e.g. name it SIRF_NOWAKEUP_REPORT_CYCLE).
> > Specifically, it is the time we must wait in order to infer that a
> > device has failed to be activated, or succeeded to hibernate.
> > 
> GPS chips will have usually some boot messages. So it is not the
> "send nmea data set every X seconds" for the wakeup case, it is
> another situation deserving IMHO another name.

Ok, but unless all supported (sirf-star-based) chips have boot messages,
we'd still need to wait that long for wakeup.

Are these messages you refer to output also on wake from hibernate, and
not just on boot?

> > In pseudo code we have:
> > 
> >   activate:
> >    - toggle on-off
> >    - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
> >      - reception: success 
> 
> Note: we can also get the goodbye/shutdown message from the chip here
> so there are chances of a false success, but since we initially power down,
> we will rule out wrong state here.

Good point. Unless we know the current state, we'd need to sleep for
HIBERNATE_TIMEOUT before waiting for data reception.

> >      - timeout: failure
> > 
> >   hibernate:
> >    - toggle on-off
> >    - sleep(HIBERNATE_TIMEOUT)
> we could also additionally check here for 
>    if (last_bytes_received == GOODBYE_MSG)

Caching and parsing the stream for this could get messy. And is the
expected message clearly defined somewhere, or would it be device (and
firmware) dependent?

> or alternatively check for
>    if (!BOOTUP_MSG_RECEIVED)
>      - return success

This seems to suggest the only thing you worry about is the drivers idea
of the current state being out of sync (which could be addressed
differently and only once at probe) and not hibernation failing for some
other reason. And you'd still need to wait for ACTIVATION_TIMEOUT (as
well as allow the chip time to actually suspend).

> >    - wait(data-received, REPORT_CYCLE)
> >      - reception: failure
> >      - timeout: success
> > 
> > A problem with your implementation, is that you assume a report cycle
> > of one second, but this need to be the case. Judging from a quick look
> > at the sirf protocols (sirf nmea and sirf binary) report cycles can be
> > configured to be as long as 30s (sirf binary) or even 255s (sirf nmea).
> > [ To make things worse these numbers can be even higher in some
> > low-power modes it seems. ]
> > 
> As far as I see we will only might have a problem if 
>   - those settings are nonvolatile (or we come in with those
>     settings on another way)
>   - or a state change lateron fails which we cannot properly detect

So again, you only worry about getting the initial state right?

Otherwise, lowering the message rate would at runtime would affect all
state changes (as currently implemented), regardless of whether these
changes are stored in NVRAM or not.

But re-reading your reply above, I guess that's what you mean by your
second point.

> > Adding just a one-second timeout (the minimum supported cycle length)
> > seems way too low, even if whatever value we choose will be reflected
> > directly in the time it takes to hibernate (suspend) the device.
> > 
> > And since this is configurable at runtime, the only safe value would be
> > the maximum one...
> > 
> > Perhaps we can say that no-wakeup support depends on having reasonably
> > short report cycles, but this needs to be documented.
> > 
> Where should it be documented? Comment in code?
> devicetree bindings?

Commit message and code at least (not devicetree binding).

> So in general I see these possibilities:
> 1. just document tho problem with low cycle length requirement
>    lateron we might improve the solution
> 
> 2. do the data parsing like above just for the first or/and last bytes
>    if these things really come reliable, we would catch the low-power
>    corner cases like only data reports if you move around.
>    I have to do some research here.
>
> 3. monitor serial output in off state and send a pulse if data arrives
>   this would require the serial device to do aggressive power saving
>   in that time(you send an example patch for that) or giving a gpio interrupt
>   coming from the rx line to the gnss driver.
>   Those things look like more restructuring work (or having a separate driver
>   which is not that practical for further extensions) and would not
>   catch low-power modes

It would also be relying the serial driver to actually support
aggressive PM (e.g. currently only usable on OMAP).

> My personal opinion here is first 1.  later improve to 2. (and do some
> more tests in regards of 2.)

Yeah, I'd guess this is a fairly unusual configuration, so we shouldn't
overdo this, and option 1 is fine with me too. I just want to make sure
that the underlying assumptions are understood and spelled out.

Thanks,
Johan

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

[Johan Hovold]

On Sun, Jan 13, 2019 at 09:50:36PM +0100, Andreas Kemnade wrote:
> On Thu, 10 Jan 2019 13:02:28 +0100
> Johan Hovold <johan@kernel.org> wrote:
> 
> > 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@kemnade.info>
> > > ---
> > > 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?
> 
> No. we cannot. The problem here is that we would take the same mutex
> in a serdev callback and around a serdev call. Then we have things like
> that:
> 
> [   36.700408] ======================================================
> [   36.706970] WARNING: possible circular locking dependency detected

Right, we need to be able to flush as part of close. Thanks for
investigating, though.

Johan

Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Andreas Kemnade]

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

On Mon, 14 Jan 2019 11:51:29 +0100
Johan Hovold <johan@kernel.org> wrote:

> On Thu, Jan 10, 2019 at 11:02:23PM +0100, Andreas Kemnade wrote:
> > Hi Johan,
> > 
> > On Thu, 10 Jan 2019 13:10:38 +0100
> > Johan Hovold <johan@kernel.org> wrote:  
> 
> > > On Sun, Dec 09, 2018 at 08:51:47PM +0100, Andreas Kemnade wrote:  
> 
> > > > Additionally it checks for the initial state of the device during
> > > > probing to ensure it is off.    
> > > 
> > > Is this really needed? If so, it should probably go in a separate patch.
> > >   
> > Well, it is the main motivation for the new try of upstreaming this again.
> > You know the loooong history...
> > It has several times messed up my power consumption statistics. As I try
> > to test patches on top of mainline, this has often led to false alarms
> > regarding pm bugs in other drivers.
> > 
> > We could also come from another kernel here via kexec having the
> > device in another state. 
> > 
> > And why we do not want to check for uncommon things here? We e.g. do
> > multiple tries for changing power state.   
> 
> You still need to argue why it is needed (e.g. the kexec case) and that
> needs to go in the commit message of a separate patch adding something
> like that as it is orthogonal to supporting configurations without
> wakeup.
> 
yes, totally agreed, there is already a separate patch with an extensive
commit message.

> This may also be better handled by a shutdown() callback which is where
> such kexec concerns are supposed to be handled, and that would also take
> care of the reboot case. This way, not everyone has to pay a penalty on
> every boot for the arguable rare use case of kexec.
> 
there are more possibilities than kexec.

> > GPS chips will have usually some boot messages. So it is not the
> > "send nmea data set every X seconds" for the wakeup case, it is
> > another situation deserving IMHO another name.  
> 
> Ok, but unless all supported (sirf-star-based) chips have boot messages,
> we'd still need to wait that long for wakeup.
> 
I have never seen one without. But that could also be attached
to the dtb compatible name or a separate property.

> Are these messages you refer to output also on wake from hibernate, and
> not just on boot?
> 
also wake from hibernate.
I mean
$PSRF150,1*3E


> > > In pseudo code we have:
> > > 
> > >   activate:
> > >    - toggle on-off
> > >    - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
> > >      - reception: success   
> > 
> > Note: we can also get the goodbye/shutdown message from the chip here
> > so there are chances of a false success, but since we initially power down,
> > we will rule out wrong state here.  
> 
> Good point. Unless we know the current state, we'd need to sleep for
> HIBERNATE_TIMEOUT before waiting for data reception.
> 
> > >      - timeout: failure
> > > 
> > >   hibernate:
> > >    - toggle on-off
> > >    - sleep(HIBERNATE_TIMEOUT)  
> > we could also additionally check here for 
> >    if (last_bytes_received == GOODBYE_MSG)  
> 
> Caching and parsing the stream for this could get messy. And is the
> expected message clearly defined somewhere, or would it be device (and
> firmware) dependent?
> 
I think so but I must check.
$PSRF150.0

But as said, these ideas are be for a possibly later patchset.

Regards,
Andreas

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

Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Andreas Kemnade]

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

On Mon, 14 Jan 2019 11:51:29 +0100
Johan Hovold <johan@kernel.org> wrote:

here is a second part of a reply.

[...]
> > > In pseudo code we have:
> > > 
> > >   activate:
> > >    - toggle on-off
> > >    - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
> > >      - reception: success   
> > 
> > Note: we can also get the goodbye/shutdown message from the chip here
> > so there are chances of a false success, but since we initially power down,
> > we will rule out wrong state here.  
> 
> Good point. Unless we know the current state, we'd need to sleep for
> HIBERNATE_TIMEOUT before waiting for data reception.
> 

And probably this also magically (together with my
runtime_get/put()-pair) in _probe()) worked around the
problems fixed by the.
gnss: sirf: fix activation retry handling

I was not aware of this problem while writing that code. Just
discovered this fact after the submission.
The current state feels a bit wonky. So I would prefer to bring it
into a stable thing with clear limitations.

> > >      - timeout: failure
> > > 
> > >   hibernate:
> > >    - toggle on-off
> > >    - sleep(HIBERNATE_TIMEOUT)  
> > we could also additionally check here for 
> >    if (last_bytes_received == GOODBYE_MSG)  
> 
> Caching and parsing the stream for this could get messy. And is the
> expected message clearly defined somewhere, or would it be device (and
> firmware) dependent?
> 
> > or alternatively check for
> >    if (!BOOTUP_MSG_RECEIVED)
> >      - return success  
> 
> This seems to suggest the only thing you worry about is the drivers idea
> of the current state being out of sync (which could be addressed
> differently and only once at probe) and not hibernation failing for some
> other reason. And you'd still need to wait for ACTIVATION_TIMEOUT (as
> well as allow the chip time to actually suspend).
> 

States being out of sync is one problem. Hibernation/activation can also
fail. I would try to catch that also. The question was just how to get
the best for the long report cycle time problem and where we are now.

> > >    - wait(data-received, REPORT_CYCLE)
> > >      - reception: failure
> > >      - timeout: success
> > > 
> > > A problem with your implementation, is that you assume a report cycle
> > > of one second, but this need to be the case. Judging from a quick look
> > > at the sirf protocols (sirf nmea and sirf binary) report cycles can be
> > > configured to be as long as 30s (sirf binary) or even 255s (sirf nmea).
> > > [ To make things worse these numbers can be even higher in some
> > > low-power modes it seems. ]
> > >   
> > As far as I see we will only might have a problem if 
> >   - those settings are nonvolatile (or we come in with those
> >     settings on another way)
> >   - or a state change lateron fails which we cannot properly detect  
> 
> So again, you only worry about getting the initial state right?
> 
The point is that we have at least some chances if we get the initial
state right.

> Otherwise, lowering the message rate would at runtime would affect all
> state changes (as currently implemented), regardless of whether these
> changes are stored in NVRAM or not.
> 
Well, with the last patchset and short report cycle we can detect state
changes to off state reliably but state changes to on state
only unreliably (which was, as said, not the intention). If the GPS chip
behaves well enough, we will not see trouble.

Now with long report cycles: Off state detection will always return
success. On state detection (in its current wonky form) will see the
state change messages and will also always return success. If initial
state is correct, this works at least in a wonky way.

I do not like these wonky things too much. So I would rather see
well-defined behavior with well-known limitations.

State change failures are probably not only a theoretical thing,
so it is a good idea to track that.

Regards,
Andreas

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

Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Johan Hovold]

On Mon, Jan 14, 2019 at 10:58:02PM +0100, Andreas Kemnade wrote:
> On Mon, 14 Jan 2019 11:51:29 +0100
> Johan Hovold <johan@kernel.org> wrote:
> 
> here is a second part of a reply.

I'm not sure I received the first part if you're saying you replied to
my mail in two parts?

> [...]
> > > > In pseudo code we have:
> > > > 
> > > >   activate:
> > > >    - toggle on-off
> > > >    - wait(data-received, ACTIVATE_TIMEOUT + REPORT_CYCLE)
> > > >      - reception: success   
> > > 
> > > Note: we can also get the goodbye/shutdown message from the chip here
> > > so there are chances of a false success, but since we initially power down,
> > > we will rule out wrong state here.  
> > 
> > Good point. Unless we know the current state, we'd need to sleep for
> > HIBERNATE_TIMEOUT before waiting for data reception.
> 
> And probably this also magically (together with my
> runtime_get/put()-pair) in _probe()) worked around the
> problems fixed by the.
> gnss: sirf: fix activation retry handling

The retry-handling fix would only avoid a successful last retry attempt
incorrectly being reported as a failure, so I don't think that would
change much here.

> Well, with the last patchset and short report cycle we can detect state
> changes to off state reliably but state changes to on state
> only unreliably (which was, as said, not the intention). If the GPS chip
> behaves well enough, we will not see trouble.
> 
> Now with long report cycles: Off state detection will always return
> success. On state detection (in its current wonky form) will see the
> state change messages and will also always return success. If initial
> state is correct, this works at least in a wonky way.
> 
> I do not like these wonky things too much. So I would rather see
> well-defined behavior with well-known limitations.
> 
> State change failures are probably not only a theoretical thing,
> so it is a good idea to track that.

Good, sounds like we're on the same page then.

Thanks,
Johan

Re: [PATCH v2 2/5] gnss: sirf: power on logic for devices without wakeup signal

[Johan Hovold]

On Mon, Jan 14, 2019 at 01:13:46PM +0100, Andreas Kemnade wrote:
> On Mon, 14 Jan 2019 11:51:29 +0100
> Johan Hovold <johan@kernel.org> wrote:

> > Good point. Unless we know the current state, we'd need to sleep for
> > HIBERNATE_TIMEOUT before waiting for data reception.
> > 
> > > >      - timeout: failure
> > > > 
> > > >   hibernate:
> > > >    - toggle on-off
> > > >    - sleep(HIBERNATE_TIMEOUT)  
> > > we could also additionally check here for 
> > >    if (last_bytes_received == GOODBYE_MSG)  
> > 
> > Caching and parsing the stream for this could get messy. And is the
> > expected message clearly defined somewhere, or would it be device (and
> > firmware) dependent?
> > 
> I think so but I must check.
> $PSRF150.0
> 
> But as said, these ideas are be for a possibly later patchset.

Yeah, and remember that the receiver can be in binary sirf mode as well,
so this do indeed seem like something that will need to wait.

Thanks,
Johan