[PATCH v2 0/2] spi, i2c: mark device nodes only in case of successful instantiation

[PATCH v2 0/2] spi, i2c: mark device nodes only in case of successful instantiation

[Ralf Ramsauer]

Hi,

this one fixes initialisation of I2C/SPI nodes.  Upon failure during
intialisation, nodes were erroneously populated and never unmarked.

This lead to the problem that re-loaded drivers will never probe those devices
again and can easily be fixed by clearing the OF_POPULATE flag when the node
doesn't successfully initialise.

For the discussion of v1, see
https://lkml.org/lkml/2016/10/14/483

  Ralf

changes since v1:
  - also fix I2C core driver
  - keep the atomic test-and-set, as Geert suggested

Ralf Ramsauer (2):
  spi: mark device nodes only in case of successful instantiation
  i2c: mark device nodes only in case of successful instantiation

 drivers/i2c/i2c-core.c | 11 ++++++++++-
 drivers/spi/spi.c      |  5 ++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

-- 
2.10.1

[PATCH v2 1/2] spi: mark device nodes only in case of successful instantiation

[Ralf Ramsauer]

Instantiated SPI device nodes are marked with OF_POPULATE. This was
introduced in bd6c164. On unloading, loaded device nodes will of course
be unmarked. The problem are nodes that fail during initialisation: If a
node fails, it won't be unloaded and hence not be unmarked.

If a SPI driver module is unloaded and reloaded, it will skip nodes that
failed before.

Skip device nodes that are already populated and mark them only in case
of success.

Note that the same issue exists for I2C.

Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
---
 drivers/spi/spi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 5787b72..838783c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1618,9 +1618,11 @@ static void of_register_spi_devices(struct spi_master *master)
 		if (of_node_test_and_set_flag(nc, OF_POPULATED))
 			continue;
 		spi = of_register_spi_device(master, nc);
-		if (IS_ERR(spi))
+		if (IS_ERR(spi)) {
 			dev_warn(&master->dev, "Failed to create SPI device for %s\n",
 				nc->full_name);
+			of_node_clear_flag(nc, OF_POPULATED);
+		}
 	}
 }
 #else
@@ -3131,6 +3133,7 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
 		if (IS_ERR(spi)) {
 			pr_err("%s: failed to create for '%s'\n",
 					__func__, rd->dn->full_name);
+			of_node_clear_flag(rd->dn, OF_POPULATED);
 			return notifier_from_errno(PTR_ERR(spi));
 		}
 		break;
-- 
2.10.1

[PATCH v2 2/2] i2c: mark device nodes only in case of successful instantiation

[Ralf Ramsauer]

Instantiated I2C device nodes are marked with OF_POPULATE. This was
introduced in 4f001fd. On unloading, loaded device nodes will of course
be unmarked. The problem are nodes that fail during initialisation: If a
node fails, it won't be unloaded and hence not be unmarked.

If a I2C driver module is unloaded and reloaded, it will skip nodes that
failed before.

Skip device nodes that are already populated and mark them only in case
of success.

Note that the same issue exists for SPI.

Fixes: 4f001fd ("i2c: Mark instantiated device nodes with OF_POPULATE")
Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
---
 drivers/i2c/i2c-core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 5ab6721..1704fc8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1681,6 +1681,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
 static void of_i2c_register_devices(struct i2c_adapter *adap)
 {
 	struct device_node *bus, *node;
+	struct i2c_client *client;
 
 	/* Only register child devices if the adapter has a node pointer set */
 	if (!adap->dev.of_node)
@@ -1695,7 +1696,14 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
 	for_each_available_child_of_node(bus, node) {
 		if (of_node_test_and_set_flag(node, OF_POPULATED))
 			continue;
-		of_i2c_register_device(adap, node);
+
+		client = of_i2c_register_device(adap, node);
+		if (IS_ERR(client)) {
+			dev_warn(&adap->dev,
+				 "Failed to create I2C device for %s\n",
+				 node->full_name);
+			of_node_clear_flag(node, OF_POPULATED);
+		}
 	}
 
 	of_node_put(bus);
@@ -2299,6 +2307,7 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
 		if (IS_ERR(client)) {
 			dev_err(&adap->dev, "failed to create client for '%s'\n",
 				 rd->dn->full_name);
+			of_node_clear_flag(rd->dn, OF_POPULATED);
 			return notifier_from_errno(PTR_ERR(client));
 		}
 		break;
-- 
2.10.1

Re: [PATCH v2 1/2] spi: mark device nodes only in case of successful instantiation

[Geert Uytterhoeven]

On Mon, Oct 17, 2016 at 3:59 PM, Ralf Ramsauer
<ralf@ramses-pyramidenbau.de> wrote:
> Instantiated SPI device nodes are marked with OF_POPULATE. This was
> introduced in bd6c164. On unloading, loaded device nodes will of course
> be unmarked. The problem are nodes that fail during initialisation: If a
> node fails, it won't be unloaded and hence not be unmarked.
>
> If a SPI driver module is unloaded and reloaded, it will skip nodes that
> failed before.
>
> Skip device nodes that are already populated and mark them only in case
> of success.
>
> Note that the same issue exists for I2C.
>
> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] i2c: mark device nodes only in case of successful instantiation

[Geert Uytterhoeven]

On Mon, Oct 17, 2016 at 3:59 PM, Ralf Ramsauer
<ralf@ramses-pyramidenbau.de> wrote:
> Instantiated I2C device nodes are marked with OF_POPULATE. This was
> introduced in 4f001fd. On unloading, loaded device nodes will of course
> be unmarked. The problem are nodes that fail during initialisation: If a
> node fails, it won't be unloaded and hence not be unmarked.
>
> If a I2C driver module is unloaded and reloaded, it will skip nodes that
> failed before.
>
> Skip device nodes that are already populated and mark them only in case
> of success.
>
> Note that the same issue exists for SPI.
>
> Fixes: 4f001fd ("i2c: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c

> @@ -1695,7 +1696,14 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
>         for_each_available_child_of_node(bus, node) {
>                 if (of_node_test_and_set_flag(node, OF_POPULATED))
>                         continue;
> -               of_i2c_register_device(adap, node);
> +
> +               client = of_i2c_register_device(adap, node);
> +               if (IS_ERR(client)) {
> +                       dev_warn(&adap->dev,
> +                                "Failed to create I2C device for %s\n",
> +                                node->full_name);

I don't think there's a need to add this warning, as of_i2c_register_device()
already prints messages in all failure paths.

> +                       of_node_clear_flag(node, OF_POPULATED);
> +               }
>         }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] i2c: mark device nodes only in case of successful instantiation

[Ralf Ramsauer]

On 10/17/2016 04:16 PM, Geert Uytterhoeven wrote:
> On Mon, Oct 17, 2016 at 3:59 PM, Ralf Ramsauer
> <ralf@ramses-pyramidenbau.de> wrote:
>> Instantiated I2C device nodes are marked with OF_POPULATE. This was
>> introduced in 4f001fd. On unloading, loaded device nodes will of course
>> be unmarked. The problem are nodes that fail during initialisation: If a
>> node fails, it won't be unloaded and hence not be unmarked.
>>
>> If a I2C driver module is unloaded and reloaded, it will skip nodes that
>> failed before.
>>
>> Skip device nodes that are already populated and mark them only in case
>> of success.
>>
>> Note that the same issue exists for SPI.
>>
>> Fixes: 4f001fd ("i2c: Mark instantiated device nodes with OF_POPULATE")
>> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
> 
>> @@ -1695,7 +1696,14 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
>>         for_each_available_child_of_node(bus, node) {
>>                 if (of_node_test_and_set_flag(node, OF_POPULATED))
>>                         continue;
>> -               of_i2c_register_device(adap, node);
>> +
>> +               client = of_i2c_register_device(adap, node);
>> +               if (IS_ERR(client)) {
>> +                       dev_warn(&adap->dev,
>> +                                "Failed to create I2C device for %s\n",
>> +                                node->full_name);
> 
> I don't think there's a need to add this warning, as of_i2c_register_device()
> already prints messages in all failure paths.
So does of_register_spi_device(). And there's a dev_warn() in
of_spi_register_devices() as well...
But yes, I was also thinking of this.

Nevertheless, I added it as I thought this would keep both drivers
somehow consistent. And it's a very rare error case, though.

  Ralf
> 
>> +                       of_node_clear_flag(node, OF_POPULATED);
>> +               }
>>         }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

-- 
Ralf Ramsauer
PGP: 0x8F10049B

Re: [PATCH v2 0/2] spi, i2c: mark device nodes only in case of successful instantiation

[Pantelis Antoniou]

Hi Ralf,

> On Oct 17, 2016, at 16:59 , Ralf Ramsauer <ralf@ramses-pyramidenbau.de> wrote:
> 
> Hi,
> 
> this one fixes initialisation of I2C/SPI nodes.  Upon failure during
> intialisation, nodes were erroneously populated and never unmarked.
> 
> This lead to the problem that re-loaded drivers will never probe those devices
> again and can easily be fixed by clearing the OF_POPULATE flag when the node
> doesn't successfully initialise.
> 
> For the discussion of v1, see
> https://lkml.org/lkml/2016/10/14/483
> 
>  Ralf
> 
> changes since v1:
>  - also fix I2C core driver
>  - keep the atomic test-and-set, as Geert suggested
> 
> Ralf Ramsauer (2):
>  spi: mark device nodes only in case of successful instantiation
>  i2c: mark device nodes only in case of successful instantiation
> 
> drivers/i2c/i2c-core.c | 11 ++++++++++-
> drivers/spi/spi.c      |  5 ++++-
> 2 files changed, 14 insertions(+), 2 deletions(-)
> 
> -- 
> 2.10.1
> 

Thanks for catching this.

Acked-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Re: [PATCH v2 1/2] spi: mark device nodes only in case of successful instantiation

[Pantelis Antoniou]

Hi Ralf,

> On Oct 17, 2016, at 16:59 , Ralf Ramsauer <ralf@ramses-pyramidenbau.de> wrote:
> 
> Instantiated SPI device nodes are marked with OF_POPULATE. This was
> introduced in bd6c164. On unloading, loaded device nodes will of course
> be unmarked. The problem are nodes that fail during initialisation: If a
> node fails, it won't be unloaded and hence not be unmarked.
> 
> If a SPI driver module is unloaded and reloaded, it will skip nodes that
> failed before.
> 
> Skip device nodes that are already populated and mark them only in case
> of success.
> 
> Note that the same issue exists for I2C.
> 
> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> ---
> drivers/spi/spi.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 5787b72..838783c 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1618,9 +1618,11 @@ static void of_register_spi_devices(struct spi_master *master)
> 		if (of_node_test_and_set_flag(nc, OF_POPULATED))
> 			continue;
> 		spi = of_register_spi_device(master, nc);
> -		if (IS_ERR(spi))
> +		if (IS_ERR(spi)) {
> 			dev_warn(&master->dev, "Failed to create SPI device for %s\n",
> 				nc->full_name);
> +			of_node_clear_flag(nc, OF_POPULATED);
> +		}
> 	}
> }
> #else
> @@ -3131,6 +3133,7 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
> 		if (IS_ERR(spi)) {
> 			pr_err("%s: failed to create for '%s'\n",
> 					__func__, rd->dn->full_name);
> +			of_node_clear_flag(rd->dn, OF_POPULATED);
> 			return notifier_from_errno(PTR_ERR(spi));
> 		}
> 		break;
> -- 
> 2.10.1
> 

Thanks for this.

Acked-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Re: [PATCH v2 2/2] i2c: mark device nodes only in case of successful instantiation

[Pantelis Antoniou]

Hi Ralf,

> On Oct 17, 2016, at 16:59 , Ralf Ramsauer <ralf@ramses-pyramidenbau.de> wrote:
> 
> Instantiated I2C device nodes are marked with OF_POPULATE. This was
> introduced in 4f001fd. On unloading, loaded device nodes will of course
> be unmarked. The problem are nodes that fail during initialisation: If a
> node fails, it won't be unloaded and hence not be unmarked.
> 
> If a I2C driver module is unloaded and reloaded, it will skip nodes that
> failed before.
> 
> Skip device nodes that are already populated and mark them only in case
> of success.
> 
> Note that the same issue exists for SPI.
> 
> Fixes: 4f001fd ("i2c: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> ---
> drivers/i2c/i2c-core.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 5ab6721..1704fc8 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1681,6 +1681,7 @@ static struct i2c_client *of_i2c_register_device(struct i2c_adapter *adap,
> static void of_i2c_register_devices(struct i2c_adapter *adap)
> {
> 	struct device_node *bus, *node;
> +	struct i2c_client *client;
> 
> 	/* Only register child devices if the adapter has a node pointer set */
> 	if (!adap->dev.of_node)
> @@ -1695,7 +1696,14 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
> 	for_each_available_child_of_node(bus, node) {
> 		if (of_node_test_and_set_flag(node, OF_POPULATED))
> 			continue;
> -		of_i2c_register_device(adap, node);
> +
> +		client = of_i2c_register_device(adap, node);
> +		if (IS_ERR(client)) {
> +			dev_warn(&adap->dev,
> +				 "Failed to create I2C device for %s\n",
> +				 node->full_name);
> +			of_node_clear_flag(node, OF_POPULATED);
> +		}
> 	}
> 
> 	of_node_put(bus);
> @@ -2299,6 +2307,7 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
> 		if (IS_ERR(client)) {
> 			dev_err(&adap->dev, "failed to create client for '%s'\n",
> 				 rd->dn->full_name);
> +			of_node_clear_flag(rd->dn, OF_POPULATED);
> 			return notifier_from_errno(PTR_ERR(client));
> 		}
> 		break;
> -- 
> 2.10.1
> 

Thanks for this

Acked-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

Re: [PATCH v2 1/2] spi: mark device nodes only in case of successful instantiation

[Mark Brown]

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

On Mon, Oct 17, 2016 at 03:59:56PM +0200, Ralf Ramsauer wrote:
> Instantiated SPI device nodes are marked with OF_POPULATE. This was
> introduced in bd6c164. On unloading, loaded device nodes will of course

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

Please also use longer hashes - the kernel repository is big enough that
we do actually have a few collisons on shorter hash lengths IIRC.  The
standard thing is 12 characthers (you can set core.abbrev to 12 to help
with this).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Applied "spi: mark device nodes only in case of successful instantiation" to the spi tree

[Mark Brown]

The patch

   spi: mark device nodes only in case of successful instantiation

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From e0af98a7e025a7263ae7e50264f6f79ed29642a7 Mon Sep 17 00:00:00 2001
From: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
Date: Mon, 17 Oct 2016 15:59:56 +0200
Subject: [PATCH] spi: mark device nodes only in case of successful
 instantiation

Instantiated SPI device nodes are marked with OF_POPULATE. This was
introduced in bd6c164. On unloading, loaded device nodes will of course
be unmarked. The problem are nodes that fail during initialisation: If a
node fails, it won't be unloaded and hence not be unmarked.

If a SPI driver module is unloaded and reloaded, it will skip nodes that
failed before.

Skip device nodes that are already populated and mark them only in case
of success.

Note that the same issue exists for I2C.

Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/spi/spi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 5787b723b593..838783c3fed0 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1618,9 +1618,11 @@ static void of_register_spi_devices(struct spi_master *master)
 		if (of_node_test_and_set_flag(nc, OF_POPULATED))
 			continue;
 		spi = of_register_spi_device(master, nc);
-		if (IS_ERR(spi))
+		if (IS_ERR(spi)) {
 			dev_warn(&master->dev, "Failed to create SPI device for %s\n",
 				nc->full_name);
+			of_node_clear_flag(nc, OF_POPULATED);
+		}
 	}
 }
 #else
@@ -3131,6 +3133,7 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
 		if (IS_ERR(spi)) {
 			pr_err("%s: failed to create for '%s'\n",
 					__func__, rd->dn->full_name);
+			of_node_clear_flag(rd->dn, OF_POPULATED);
 			return notifier_from_errno(PTR_ERR(spi));
 		}
 		break;
-- 
2.8.1

Re: [PATCH v2 2/2] i2c: mark device nodes only in case of successful instantiation

[Wolfram Sang]

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

On Mon, Oct 17, 2016 at 03:59:57PM +0200, Ralf Ramsauer wrote:
> Instantiated I2C device nodes are marked with OF_POPULATE. This was
> introduced in 4f001fd. On unloading, loaded device nodes will of course
> be unmarked. The problem are nodes that fail during initialisation: If a
> node fails, it won't be unloaded and hence not be unmarked.
> 
> If a I2C driver module is unloaded and reloaded, it will skip nodes that
> failed before.
> 
> Skip device nodes that are already populated and mark them only in case
> of success.
> 
> Note that the same issue exists for SPI.
> 
> Fixes: 4f001fd ("i2c: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]