diff mbox series

[v2,2/2] misc: sram: Generate unique names for subpools

Message ID 20230417-ux500-sram-v2-2-6e62ad551faa@linaro.org
State New
Headers
Series SRAM dt binding and fix |

Commit Message

Linus Walleij April 20, 2023, 9:17 p.m. UTC 
  The current code will, if we do not specify unique labels
for the SRAM subnodes, fail to register several nodes named
the same.

Example:

sram@40020000 {
  (...)
  sram@0 {
    (...)
  };
  sram@1000 {
    (...)
  };
};

Since the child->name in both cases will be "sram" the
gen_pool_create() will fail because the name is not unique.

Use dev_name() for the device as this will have bus ID
set to the fully translated address for the node, and that
will always be unique.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Stop complicating things and just use dev_name()
---
 drivers/misc/sram.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Dmitry Osipenko June 18, 2023, 9:33 p.m. UTC | #1 
21.04.2023 00:17, Linus Walleij пишет:
> The current code will, if we do not specify unique labels
> for the SRAM subnodes, fail to register several nodes named
> the same.
> 
> Example:
> 
> sram@40020000 {
>   (...)
>   sram@0 {
>     (...)
>   };
>   sram@1000 {
>     (...)
>   };
> };
> 
> Since the child->name in both cases will be "sram" the
> gen_pool_create() will fail because the name is not unique.
> 
> Use dev_name() for the device as this will have bus ID
> set to the fully translated address for the node, and that
> will always be unique.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Stop complicating things and just use dev_name()
> ---
>  drivers/misc/sram.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> index f0e7f02605eb..f80c3adddf0b 100644
> --- a/drivers/misc/sram.c
> +++ b/drivers/misc/sram.c
> @@ -240,10 +240,11 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
>  				goto err_chunks;
>  			}
>  			if (!label)
> -				label = child->name;
> -
> -			block->label = devm_kstrdup(sram->dev,
> -						    label, GFP_KERNEL);
> +				block->label = devm_kasprintf(sram->dev, GFP_KERNEL,
> +							      "%s", dev_name(sram->dev));

This broke device-trees that have no label property. The SRAM DT binding
says:

"
label:
description:
	The name for the reserved partition, if omitted, the label is taken
	from the node name excluding the unit address.
"

Not sure whether breakage was on purpose, otherwise doc needs to be
updated or there should be explicit check for the duplicated node names.

Secondly, AFAICS, the dev_name(sram->dev) is the name of the parent SRAM
device and not of the children sub-nodes, hence it's now always the same
dev_name(sram->dev) for all sub-nodes.
Linus Walleij June 19, 2023, 7:11 a.m. UTC | #2 
On Sun, Jun 18, 2023 at 11:33 PM Dmitry Osipenko <digetx@gmail.com> wrote:

> >                       if (!label)
> > -                             label = child->name;
> > -
> > -                     block->label = devm_kstrdup(sram->dev,
> > -                                                 label, GFP_KERNEL);
> > +                             block->label = devm_kasprintf(sram->dev, GFP_KERNEL,
> > +                                                           "%s", dev_name(sram->dev));
>
> This broke device-trees that have no label property.

Which system is affected? Asking so I can inspect the DTS file
and figure out how this needs to work.

>  The SRAM DT binding says:
>
> "
> label:
> description:
>         The name for the reserved partition, if omitted, the label is taken
>         from the node name excluding the unit address.
> "
>
> Not sure whether breakage was on purpose, otherwise doc needs to be
> updated or there should be explicit check for the duplicated node names.
>
> Secondly, AFAICS, the dev_name(sram->dev) is the name of the parent SRAM
> device and not of the children sub-nodes, hence it's now always the same
> dev_name(sram->dev) for all sub-nodes.

Sounds like I should go back to the original approach in patch v1:
https://lore.kernel.org/linux-devicetree/20230417-ux500-sram-v1-2-5924988bb835@linaro.org/

and also augment the DTS binding text to say it uses the full node name
including the address.

Does that look OK to you, or will this regress your system as well?

Yours,
Linus Walleij
Dmitry Osipenko June 20, 2023, 8:23 a.m. UTC | #3 
19.06.2023 10:11, Linus Walleij пишет:
> On Sun, Jun 18, 2023 at 11:33 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 
>>>                       if (!label)
>>> -                             label = child->name;
>>> -
>>> -                     block->label = devm_kstrdup(sram->dev,
>>> -                                                 label, GFP_KERNEL);
>>> +                             block->label = devm_kasprintf(sram->dev, GFP_KERNEL,
>>> +                                                           "%s", dev_name(sram->dev));
>>
>> This broke device-trees that have no label property.
> 
> Which system is affected? Asking so I can inspect the DTS file
> and figure out how this needs to work.

NVIDIA Tegra2/3 video decoder driver fails to probe with this change.

https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/nvidia/tegra-vde/vde.c#L312

https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/tegra20.dtsi#L347

>>  The SRAM DT binding says:
>>
>> "
>> label:
>> description:
>>         The name for the reserved partition, if omitted, the label is taken
>>         from the node name excluding the unit address.
>> "
>>
>> Not sure whether breakage was on purpose, otherwise doc needs to be
>> updated or there should be explicit check for the duplicated node names.
>>
>> Secondly, AFAICS, the dev_name(sram->dev) is the name of the parent SRAM
>> device and not of the children sub-nodes, hence it's now always the same
>> dev_name(sram->dev) for all sub-nodes.
> 
> Sounds like I should go back to the original approach in patch v1:
> https://lore.kernel.org/linux-devicetree/20230417-ux500-sram-v1-2-5924988bb835@linaro.org/
> 
> and also augment the DTS binding text to say it uses the full node name
> including the address.
> 
> Does that look OK to you, or will this regress your system as well?

That may work, but then seems you'll also need to update
of_gen_pool_get() to use np_pool->full_name instead of np_pool->name.

https://elixir.bootlin.com/linux/latest/source/lib/genalloc.c#L898
diff mbox series

Patch

diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index f0e7f02605eb..f80c3adddf0b 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -240,10 +240,11 @@  static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
 				goto err_chunks;
 			}
 			if (!label)
-				label = child->name;
-
-			block->label = devm_kstrdup(sram->dev,
-						    label, GFP_KERNEL);
+				block->label = devm_kasprintf(sram->dev, GFP_KERNEL,
+							      "%s", dev_name(sram->dev));
+			else
+				block->label = devm_kstrdup(sram->dev,
+							    label, GFP_KERNEL);
 			if (!block->label) {
 				ret = -ENOMEM;
 				goto err_chunks;