[PATCH] spi: mark device nodes only in case of successful instantiation

[PATCH] 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 the fail during initialisation: If a
node failed during registration, it won't be unloaded and hence never be
unmarked again.

So 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.

Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
---
Hi,

imagine the following situation: you loaded a spi driver as module, but
it fails to instantiate, because of some reasons (e.g. some resources,
like gpios, might be in use in userspace).

When reloading the driver, _all_ nodes, including previously failed
ones, should be probed again. This is not the case at the moment.
Current behaviour only re-registers nodes that were previously
successfully loaded.

This small patches fixes this behaviour. I stumbled over this while
working on a spi driver.

  Ralf

 drivers/spi/spi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 200ca22..f96a04e 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
 		return;
 
 	for_each_available_child_of_node(master->dev.of_node, nc) {
-		if (of_node_test_and_set_flag(nc, OF_POPULATED))
+		if (of_node_check_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);
+			continue;
+		}
+		of_node_set_flag(nc, OF_POPULATED);
 	}
 }
 #else
-- 
2.10.1

Re: [PATCH] spi: mark device nodes only in case of successful instantiation

[Geert Uytterhoeven]

Hi Ralf,

(Cc i2c)

On Fri, Oct 14, 2016 at 9:31 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 the fail during initialisation: If a
> node failed during registration, it won't be unloaded and hence never be
> unmarked again.
>
> So 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.
>
> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Hi,
>
> imagine the following situation: you loaded a spi driver as module, but
> it fails to instantiate, because of some reasons (e.g. some resources,
> like gpios, might be in use in userspace).
>
> When reloading the driver, _all_ nodes, including previously failed
> ones, should be probed again. This is not the case at the moment.
> Current behaviour only re-registers nodes that were previously
> successfully loaded.
>
> This small patches fixes this behaviour. I stumbled over this while
> working on a spi driver.

Thanks for your patch!

>  drivers/spi/spi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 200ca22..f96a04e 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
>                 return;
>
>         for_each_available_child_of_node(master->dev.of_node, nc) {
> -               if (of_node_test_and_set_flag(nc, OF_POPULATED))
> +               if (of_node_check_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);
> +                       continue;
> +               }
> +               of_node_set_flag(nc, OF_POPULATED);

I think it's safer to keep the atomic test-and-set, but clear the flag on
failure, cfr. of_platform_device_create_pdata() and of_amba_device_create().

Shouldn't of_spi_notify() be fixed, too?

The same issue exists for i2c in of_i2c_register_devices() and of_i2c_notify(),
which is what I had used as an example.

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] spi: mark device nodes only in case of successful instantiation

[Wolfram Sang]

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

On Sun, Oct 16, 2016 at 10:49:11AM +0200, Geert Uytterhoeven wrote:
> Hi Ralf,
> 
> (Cc i2c)

Thanks for letting me know! Adding Pantelis to CC, as he is the original
author of OF_DYNAMIC. Please keep me in the loop.

> 
> On Fri, Oct 14, 2016 at 9:31 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 the fail during initialisation: If a
> > node failed during registration, it won't be unloaded and hence never be
> > unmarked again.
> >
> > So 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.
> >
> > Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
> > Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Hi,
> >
> > imagine the following situation: you loaded a spi driver as module, but
> > it fails to instantiate, because of some reasons (e.g. some resources,
> > like gpios, might be in use in userspace).
> >
> > When reloading the driver, _all_ nodes, including previously failed
> > ones, should be probed again. This is not the case at the moment.
> > Current behaviour only re-registers nodes that were previously
> > successfully loaded.
> >
> > This small patches fixes this behaviour. I stumbled over this while
> > working on a spi driver.
> 
> Thanks for your patch!
> 
> >  drivers/spi/spi.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 200ca22..f96a04e 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
> >                 return;
> >
> >         for_each_available_child_of_node(master->dev.of_node, nc) {
> > -               if (of_node_test_and_set_flag(nc, OF_POPULATED))
> > +               if (of_node_check_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);
> > +                       continue;
> > +               }
> > +               of_node_set_flag(nc, OF_POPULATED);
> 
> I think it's safer to keep the atomic test-and-set, but clear the flag on
> failure, cfr. of_platform_device_create_pdata() and of_amba_device_create().
> 
> Shouldn't of_spi_notify() be fixed, too?
> 
> The same issue exists for i2c in of_i2c_register_devices() and of_i2c_notify(),
> which is what I had used as an example.
> 
> 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

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

Re: [PATCH] spi: mark device nodes only in case of successful instantiation

[Ralf Ramsauer]

Hi Geert,

On 10/16/2016 10:49 AM, Geert Uytterhoeven wrote:
> Hi Ralf,
> 
> (Cc i2c)
> 
> On Fri, Oct 14, 2016 at 9:31 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 the fail during initialisation: If a
>> node failed during registration, it won't be unloaded and hence never be
>> unmarked again.
>>
>> So 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.
>>
>> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
>> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> Hi,
>>
>> imagine the following situation: you loaded a spi driver as module, but
>> it fails to instantiate, because of some reasons (e.g. some resources,
>> like gpios, might be in use in userspace).
>>
>> When reloading the driver, _all_ nodes, including previously failed
>> ones, should be probed again. This is not the case at the moment.
>> Current behaviour only re-registers nodes that were previously
>> successfully loaded.
>>
>> This small patches fixes this behaviour. I stumbled over this while
>> working on a spi driver.
> 
> Thanks for your patch!
> 
>>  drivers/spi/spi.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 200ca22..f96a04e 100644
>> --- a/drivers/spi/spi.c
>> +++ b/drivers/spi/spi.c
>> @@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
>>                 return;
>>
>>         for_each_available_child_of_node(master->dev.of_node, nc) {
>> -               if (of_node_test_and_set_flag(nc, OF_POPULATED))
>> +               if (of_node_check_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);
>> +                       continue;
>> +               }
>> +               of_node_set_flag(nc, OF_POPULATED);
> 
> I think it's safer to keep the atomic test-and-set, but clear the flag on
> failure, cfr. of_platform_device_create_pdata() and of_amba_device_create().
Ack, no prob. Let me change this in the next version.
> 
> Shouldn't of_spi_notify() be fixed, too?
Right, that's almost the same path.
> 
> The same issue exists for i2c in of_i2c_register_devices() and of_i2c_notify(),
> which is what I had used as an example.
Good old c&p ;-)
I'll fix and test that tomorrow and come back with two patches, as it
touches different subsystems.

Best
  Ralf
> 
> 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
GPG: 0x8F10049B

Re: [PATCH] spi: mark device nodes only in case of successful instantiation

[Pantelis Antoniou]

Hi Ralf,

> On Oct 16, 2016, at 12:55 , Ralf Ramsauer <ralf@ramses-pyramidenbau.de> wrote:
> 
> Hi Geert,
> 
> On 10/16/2016 10:49 AM, Geert Uytterhoeven wrote:
>> Hi Ralf,
>> 
>> (Cc i2c)
>> 
>> On Fri, Oct 14, 2016 at 9:31 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 the fail during initialisation: If a
>>> node failed during registration, it won't be unloaded and hence never be
>>> unmarked again.
>>> 
>>> So 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.
>>> 
>>> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
>>> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
>>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
>>> ---
>>> Hi,
>>> 
>>> imagine the following situation: you loaded a spi driver as module, but
>>> it fails to instantiate, because of some reasons (e.g. some resources,
>>> like gpios, might be in use in userspace).
>>> 
>>> When reloading the driver, _all_ nodes, including previously failed
>>> ones, should be probed again. This is not the case at the moment.
>>> Current behaviour only re-registers nodes that were previously
>>> successfully loaded.
>>> 
>>> This small patches fixes this behaviour. I stumbled over this while
>>> working on a spi driver.
>> 
>> Thanks for your patch!
>> 
>>> drivers/spi/spi.c | 7 +++++--
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>>> index 200ca22..f96a04e 100644
>>> --- a/drivers/spi/spi.c
>>> +++ b/drivers/spi/spi.c
>>> @@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
>>>                return;
>>> 
>>>        for_each_available_child_of_node(master->dev.of_node, nc) {
>>> -               if (of_node_test_and_set_flag(nc, OF_POPULATED))
>>> +               if (of_node_check_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);
>>> +                       continue;
>>> +               }
>>> +               of_node_set_flag(nc, OF_POPULATED);
>> 
>> I think it's safer to keep the atomic test-and-set, but clear the flag on
>> failure, cfr. of_platform_device_create_pdata() and of_amba_device_create().
> Ack, no prob. Let me change this in the next version.
>> 
>> Shouldn't of_spi_notify() be fixed, too?
> Right, that's almost the same path.
>> 
>> The same issue exists for i2c in of_i2c_register_devices() and of_i2c_notify(),
>> which is what I had used as an example.
> Good old c&p ;-)
> I'll fix and test that tomorrow and come back with two patches, as it
> touches different subsystems.
> 

Thanks for this. This is a very rare case that’s easy to slip through.
It is good to be consistent :)

> Best
>  Ralf
>> 

Regards

— Pantelis

>> 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
> GPG: 0x8F10049B

Re: [PATCH] spi: mark device nodes only in case of successful instantiation

[Wolfram Sang]

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

On Mon, Oct 17, 2016 at 10:20:47PM +0300, Pantelis Antoniou wrote:
> Hi Ralf,
> 
> > On Oct 16, 2016, at 12:55 , Ralf Ramsauer <ralf@ramses-pyramidenbau.de> wrote:
> > 
> > Hi Geert,
> > 
> > On 10/16/2016 10:49 AM, Geert Uytterhoeven wrote:
> >> Hi Ralf,
> >> 
> >> (Cc i2c)
> >> 
> >> On Fri, Oct 14, 2016 at 9:31 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 the fail during initialisation: If a
> >>> node failed during registration, it won't be unloaded and hence never be
> >>> unmarked again.
> >>> 
> >>> So 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.
> >>> 
> >>> Fixes: bd6c164 ("spi: Mark instantiated device nodes with OF_POPULATE")
> >>> Signed-off-by: Ralf Ramsauer <ralf@ramses-pyramidenbau.de>
> >>> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> >>> ---
> >>> Hi,
> >>> 
> >>> imagine the following situation: you loaded a spi driver as module, but
> >>> it fails to instantiate, because of some reasons (e.g. some resources,
> >>> like gpios, might be in use in userspace).
> >>> 
> >>> When reloading the driver, _all_ nodes, including previously failed
> >>> ones, should be probed again. This is not the case at the moment.
> >>> Current behaviour only re-registers nodes that were previously
> >>> successfully loaded.
> >>> 
> >>> This small patches fixes this behaviour. I stumbled over this while
> >>> working on a spi driver.
> >> 
> >> Thanks for your patch!
> >> 
> >>> drivers/spi/spi.c | 7 +++++--
> >>> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> >>> index 200ca22..f96a04e 100644
> >>> --- a/drivers/spi/spi.c
> >>> +++ b/drivers/spi/spi.c
> >>> @@ -1604,12 +1604,15 @@ static void of_register_spi_devices(struct spi_master *master)
> >>>                return;
> >>> 
> >>>        for_each_available_child_of_node(master->dev.of_node, nc) {
> >>> -               if (of_node_test_and_set_flag(nc, OF_POPULATED))
> >>> +               if (of_node_check_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);
> >>> +                       continue;
> >>> +               }
> >>> +               of_node_set_flag(nc, OF_POPULATED);
> >> 
> >> I think it's safer to keep the atomic test-and-set, but clear the flag on
> >> failure, cfr. of_platform_device_create_pdata() and of_amba_device_create().
> > Ack, no prob. Let me change this in the next version.
> >> 

> Thanks for this. This is a very rare case that’s easy to slip through.
> It is good to be consistent :)

I read this as acked-by for the series?


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

Re: [PATCH] spi: mark device nodes only in case of successful instantiation

[Wolfram Sang]

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

> > Thanks for this. This is a very rare case that’s easy to slip through.
> > It is good to be consistent :)
> 
> I read this as acked-by for the series?

Ah, I mixed up. You acked V2 :)


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