[RESEND PATCH v3 1/2] of: base: Introduce of_alias_get_alias_list() to check alias IDs

[RESEND PATCH v3 1/2] of: base: Introduce of_alias_get_alias_list() to check alias IDs

[Michal Simek]

The function travels the lookup table to record alias ids for the given
device match structures and alias stem.
This function will be used by serial drivers to check if requested alias
is allocated or free to use.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---

Changes in v3:
- Fix subject s/of_alias_check_id/of_alias_get_alias_list/
- And also fix commit message description to match
  of_alias_get_alias_list()
- mark empty function with inline

Changes in v2:
- Add empty function for !CONFIG_OF case
- Add return to that function
- Add Rob's Reviewed-by tag

Based on discussion with Rob
https://lkml.org/lkml/2018/4/27/397
nbits is passed to the function not to limit only to 32/64bit fields.

Greg: Please apply this together with serial: uartps: Change uart ID
port allocation. Rob is fine with that (link above)

---
 drivers/of/base.c  | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h | 10 ++++++++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 74eaedd5b860..33011b88ed3f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -16,6 +16,7 @@
 
 #define pr_fmt(fmt)	"OF: " fmt
 
+#include <linux/bitmap.h>
 #include <linux/console.h>
 #include <linux/ctype.h>
 #include <linux/cpu.h>
@@ -1943,6 +1944,57 @@ int of_alias_get_id(struct device_node *np, const char *stem)
 EXPORT_SYMBOL_GPL(of_alias_get_id);
 
 /**
+ * of_alias_get_alias_list - Get alias list for the given device driver
+ * @matches:	Array of OF device match structures to search in
+ * @stem:	Alias stem of the given device_node
+ * @bitmap:	Bitmap field pointer
+ * @nbits:	Maximum number of alias ID which can be recorded it bitmap
+ *
+ * The function travels the lookup table to record alias ids for the given
+ * device match structures and alias stem.
+ *
+ * Return:	0 or -ENOSYS when !CONFIG_OF
+ */
+int of_alias_get_alias_list(const struct of_device_id *matches,
+			     const char *stem, unsigned long *bitmap,
+			     unsigned int nbits)
+{
+	struct alias_prop *app;
+
+	/* Zero bitmap field to make sure that all the time it is clean */
+	bitmap_zero(bitmap, nbits);
+
+	mutex_lock(&of_mutex);
+	pr_debug("%s: Looking for stem: %s\n", __func__, stem);
+	list_for_each_entry(app, &aliases_lookup, link) {
+		pr_debug("%s: stem: %s, id: %d\n",
+			 __func__, app->stem, app->id);
+
+		if (strcmp(app->stem, stem) != 0) {
+			pr_debug("%s: stem comparison doesn't passed %s\n",
+				 __func__, app->stem);
+			continue;
+		}
+
+		if (app->id >= nbits) {
+			pr_debug("%s: ID %d greater then bitmap field %d\n",
+				__func__, app->id, nbits);
+			continue;
+		}
+
+		if (of_match_node(matches, app->np)) {
+			pr_debug("%s: Allocated ID %d\n", __func__, app->id);
+			set_bit(app->id, bitmap);
+		}
+		/* Alias exist but it not compatible with matches */
+	}
+	mutex_unlock(&of_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_alias_get_alias_list);
+
+/**
  * of_alias_get_highest_id - Get highest alias id for the given stem
  * @stem:	Alias stem to be examined
  *
diff --git a/include/linux/of.h b/include/linux/of.h
index 99b0ebf49632..d51457b40725 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -392,6 +392,9 @@ extern int of_phandle_iterator_args(struct of_phandle_iterator *it,
 extern void of_alias_scan(void * (*dt_alloc)(u64 size, u64 align));
 extern int of_alias_get_id(struct device_node *np, const char *stem);
 extern int of_alias_get_highest_id(const char *stem);
+extern int of_alias_get_alias_list(const struct of_device_id *matches,
+				   const char *stem, unsigned long *bitmap,
+				   unsigned int nbits);
 
 extern int of_machine_is_compatible(const char *compat);
 
@@ -893,6 +896,13 @@ static inline int of_alias_get_highest_id(const char *stem)
 	return -ENOSYS;
 }
 
+static inline int of_alias_get_alias_list(const struct of_device_id *matches,
+					  const char *stem, unsigned long *bitmap,
+					  unsigned int nbits)
+{
+	return -ENOSYS;
+}
+
 static inline int of_machine_is_compatible(const char *compat)
 {
 	return 0;
-- 
1.9.1

[RESEND PATCH v3 2/2] serial: uartps: Change uart ID port allocation

[Michal Simek]

For IPs which have alias algorightm all the time using that alias and
minor number. It means serial20 alias ends up as ttyPS20.

If alias is not setup for probed IP instance the first unused position is
used but that needs to be checked if it is really empty because another
instance doesn't need to be probed at that time. of_alias_get_alias_list()
fills alias bitmap which exactly shows which ID is free.
If alias pointing to different not compatible IP, it is free to use.

cdns_get_id() call is placed below structure allocation to simplify
error path.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v3: None
Changes in v2:
- Add handle of return value from of_alias_get_alias_list

Needs to be applied on the top of
https://lkml.org/lkml/2018/9/3/404
https://lkml.org/lkml/2018/9/3/400 (minor that's why just in case)

Greg: Please apply it with
"of: base: Introduce of_alias_get_alias_list() to check alias IDs
Rob is ok with that.

---
 drivers/tty/serial/xilinx_uartps.c | 109 ++++++++++++++++++++++++++++++++-----
 1 file changed, 96 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 71c032744dae..f77200a0f461 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -30,7 +30,6 @@
 #define CDNS_UART_TTY_NAME	"ttyPS"
 #define CDNS_UART_NAME		"xuartps"
 #define CDNS_UART_MAJOR		0	/* use dynamic node allocation */
-#define CDNS_UART_NR_PORTS	2
 #define CDNS_UART_FIFO_SIZE	64	/* FIFO size */
 #define CDNS_UART_REGISTER_SPACE	0x1000
 
@@ -1370,6 +1369,88 @@ static int __maybe_unused cdns_runtime_resume(struct device *dev)
 };
 MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
 
+/*
+ * Maximum number of instances without alias IDs but if there is alias
+ * which target "< MAX_UART_INSTANCES" range this ID can't be used.
+ */
+#define MAX_UART_INSTANCES	32
+
+/* Stores static aliases list */
+static DECLARE_BITMAP(alias_bitmap, MAX_UART_INSTANCES);
+static int alias_bitmap_initialized;
+
+/* Stores actual bitmap of allocated IDs with alias IDs together */
+static DECLARE_BITMAP(bitmap, MAX_UART_INSTANCES);
+/* Protect bitmap operations to have unique IDs */
+static DEFINE_MUTEX(bitmap_lock);
+
+static int cdns_get_id(struct platform_device *pdev)
+{
+	int id, ret;
+
+	mutex_lock(&bitmap_lock);
+
+	/* Alias list is stable that's why get alias bitmap only once */
+	if (!alias_bitmap_initialized) {
+		ret = of_alias_get_alias_list(cdns_uart_of_match, "serial",
+					      alias_bitmap, MAX_UART_INSTANCES);
+		if (ret)
+			return ret;
+
+		alias_bitmap_initialized++;
+	}
+
+	/* Make sure that alias ID is not taken by instance without alias */
+	bitmap_or(bitmap, bitmap, alias_bitmap, MAX_UART_INSTANCES);
+
+	dev_dbg(&pdev->dev, "Alias bitmap: %*pb\n",
+		MAX_UART_INSTANCES, bitmap);
+
+	/* Look for a serialN alias */
+	id = of_alias_get_id(pdev->dev.of_node, "serial");
+	if (id < 0) {
+		dev_warn(&pdev->dev,
+			 "No serial alias passed. Using the first free id\n");
+
+		/*
+		 * Start with id 0 and check if there is no serial0 alias
+		 * which points to device which is compatible with this driver.
+		 * If alias exists then try next free position.
+		 */
+		id = 0;
+
+		for (;;) {
+			dev_info(&pdev->dev, "Checking id %d\n", id);
+			id = find_next_zero_bit(bitmap, MAX_UART_INSTANCES, id);
+
+			/* No free empty instance */
+			if (id == MAX_UART_INSTANCES) {
+				dev_err(&pdev->dev, "No free ID\n");
+				mutex_unlock(&bitmap_lock);
+				return -EINVAL;
+			}
+
+			dev_dbg(&pdev->dev, "The empty id is %d\n", id);
+			/* Check if ID is empty */
+			if (!test_and_set_bit(id, bitmap)) {
+				/* Break the loop if bit is taken */
+				dev_dbg(&pdev->dev,
+					"Selected ID %d allocation passed\n",
+					id);
+				break;
+			}
+			dev_dbg(&pdev->dev,
+				"Selected ID %d allocation failed\n", id);
+			/* if taking bit fails then try next one */
+			id++;
+		}
+	}
+
+	mutex_unlock(&bitmap_lock);
+
+	return id;
+}
+
 /**
  * cdns_uart_probe - Platform driver probe
  * @pdev: Pointer to the platform device structure
@@ -1403,21 +1484,17 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	if (!cdns_uart_uart_driver)
 		return -ENOMEM;
 
-	/* Look for a serialN alias */
-	cdns_uart_data->id = of_alias_get_id(pdev->dev.of_node, "serial");
+	cdns_uart_data->id = cdns_get_id(pdev);
 	if (cdns_uart_data->id < 0)
-		cdns_uart_data->id = 0;
-
-	if (cdns_uart_data->id >= CDNS_UART_NR_PORTS) {
-		dev_err(&pdev->dev, "Cannot get uart_port structure\n");
-		return -ENODEV;
-	}
+		return cdns_uart_data->id;
 
 	/* There is a need to use unique driver name */
 	driver_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s%d",
 				     CDNS_UART_NAME, cdns_uart_data->id);
-	if (!driver_name)
-		return -ENOMEM;
+	if (!driver_name) {
+		rc = -ENOMEM;
+		goto err_out_id;
+	}
 
 	cdns_uart_uart_driver->owner = THIS_MODULE;
 	cdns_uart_uart_driver->driver_name = driver_name;
@@ -1446,7 +1523,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	rc = uart_register_driver(cdns_uart_uart_driver);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "Failed to register driver\n");
-		return rc;
+		goto err_out_id;
 	}
 
 	cdns_uart_data->cdns_uart_driver = cdns_uart_uart_driver;
@@ -1587,7 +1664,10 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	clk_disable_unprepare(cdns_uart_data->pclk);
 err_out_unregister_driver:
 	uart_unregister_driver(cdns_uart_data->cdns_uart_driver);
-
+err_out_id:
+	mutex_lock(&bitmap_lock);
+	clear_bit(cdns_uart_data->id, bitmap);
+	mutex_unlock(&bitmap_lock);
 	return rc;
 }
 
@@ -1610,6 +1690,9 @@ static int cdns_uart_remove(struct platform_device *pdev)
 #endif
 	rc = uart_remove_one_port(cdns_uart_data->cdns_uart_driver, port);
 	port->mapbase = 0;
+	mutex_lock(&bitmap_lock);
+	clear_bit(cdns_uart_data->id, bitmap);
+	mutex_unlock(&bitmap_lock);
 	clk_disable_unprepare(cdns_uart_data->uartclk);
 	clk_disable_unprepare(cdns_uart_data->pclk);
 	pm_runtime_disable(&pdev->dev);
-- 
1.9.1

Re: [RESEND PATCH v3 2/2] serial: uartps: Change uart ID port allocation

[Geert Uytterhoeven]

Hi Michal,

On Thu, Sep 20, 2018 at 1:42 PM Michal Simek <michal.simek@xilinx.com> wrote:
> For IPs which have alias algorightm all the time using that alias and
> minor number. It means serial20 alias ends up as ttyPS20.
>
> If alias is not setup for probed IP instance the first unused position is
> used but that needs to be checked if it is really empty because another
> instance doesn't need to be probed at that time. of_alias_get_alias_list()
> fills alias bitmap which exactly shows which ID is free.
> If alias pointing to different not compatible IP, it is free to use.
>
> cdns_get_id() call is placed below structure allocation to simplify
> error path.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

JFTR, for sh-sci, I used a different approach, as all ports in the static DTB
can have an alias (if aliases are needed at all), and only DT overlays cannot
have them. Cfr. commit 7678f4c20fa7670f ("serial: sh-sci: Add support for
dynamic instances").

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: [RESEND PATCH v3 1/2] of: base: Introduce of_alias_get_alias_list() to check alias IDs

[Geert Uytterhoeven]

Hi Michal,

On Thu, Sep 20, 2018 at 1:42 PM Michal Simek <michal.simek@xilinx.com> wrote:
> The function travels the lookup table to record alias ids for the given
> device match structures and alias stem.
> This function will be used by serial drivers to check if requested alias
> is allocated or free to use.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

I know this has already been applied, it just drew my attention.

> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -16,6 +16,7 @@
>
>  #define pr_fmt(fmt)    "OF: " fmt
>
> +#include <linux/bitmap.h>
>  #include <linux/console.h>
>  #include <linux/ctype.h>
>  #include <linux/cpu.h>
> @@ -1943,6 +1944,57 @@ int of_alias_get_id(struct device_node *np, const char *stem)
>  EXPORT_SYMBOL_GPL(of_alias_get_id);
>
>  /**
> + * of_alias_get_alias_list - Get alias list for the given device driver
> + * @matches:   Array of OF device match structures to search in
> + * @stem:      Alias stem of the given device_node
> + * @bitmap:    Bitmap field pointer
> + * @nbits:     Maximum number of alias ID which can be recorded it bitmap

IDs

> + *
> + * The function travels the lookup table to record alias ids for the given
> + * device match structures and alias stem.
> + *
> + * Return:     0 or -ENOSYS when !CONFIG_OF
> + */
> +int of_alias_get_alias_list(const struct of_device_id *matches,
> +                            const char *stem, unsigned long *bitmap,
> +                            unsigned int nbits)
> +{
> +       struct alias_prop *app;
> +
> +       /* Zero bitmap field to make sure that all the time it is clean */
> +       bitmap_zero(bitmap, nbits);
> +
> +       mutex_lock(&of_mutex);
> +       pr_debug("%s: Looking for stem: %s\n", __func__, stem);
> +       list_for_each_entry(app, &aliases_lookup, link) {
> +               pr_debug("%s: stem: %s, id: %d\n",
> +                        __func__, app->stem, app->id);
> +
> +               if (strcmp(app->stem, stem) != 0) {
> +                       pr_debug("%s: stem comparison doesn't passed %s\n",

didn't pass?

> +                                __func__, app->stem);
> +                       continue;
> +               }
> +
> +               if (app->id >= nbits) {
> +                       pr_debug("%s: ID %d greater then bitmap field %d\n",

than
%u for unsigned int

> +                               __func__, app->id, nbits);
> +                       continue;

Shouldn't this return an error, if of_match_node() below would have matched?

> +               }
> +
> +               if (of_match_node(matches, app->np)) {
> +                       pr_debug("%s: Allocated ID %d\n", __func__, app->id);
> +                       set_bit(app->id, bitmap);
> +               }
> +               /* Alias exist but it not compatible with matches */

exists but is npt

> +       }
> +       mutex_unlock(&of_mutex);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_alias_get_alias_list);
> +
> +/**
>   * of_alias_get_highest_id - Get highest alias id for the given stem
>   * @stem:      Alias stem to be examined
>   *

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: [RESEND PATCH v3 2/2] serial: uartps: Change uart ID port allocation

[Michal Simek]

Hi Geert,

On 24.9.2018 09:37, Geert Uytterhoeven wrote:
> Hi Michal,
> 
> On Thu, Sep 20, 2018 at 1:42 PM Michal Simek <michal.simek@xilinx.com> wrote:
>> For IPs which have alias algorightm all the time using that alias and
>> minor number. It means serial20 alias ends up as ttyPS20.
>>
>> If alias is not setup for probed IP instance the first unused position is
>> used but that needs to be checked if it is really empty because another
>> instance doesn't need to be probed at that time. of_alias_get_alias_list()
>> fills alias bitmap which exactly shows which ID is free.
>> If alias pointing to different not compatible IP, it is free to use.
>>
>> cdns_get_id() call is placed below structure allocation to simplify
>> error path.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> 
> JFTR, for sh-sci, I used a different approach, as all ports in the static DTB
> can have an alias (if aliases are needed at all), and only DT overlays cannot
> have them. Cfr. commit 7678f4c20fa7670f ("serial: sh-sci: Add support for
> dynamic instances").

if you look at all patches I have done for uartps you can find out that
I am creating uart_driver for every instance separately.
It means that there is still a limit for number of consoles exactly as
yours CONFIG_SERIAL_SH_SCI_NR_UARTS but every instance is separated.

Thanks,
Michal

Re: [RESEND PATCH v3 1/2] of: base: Introduce of_alias_get_alias_list() to check alias IDs

[Michal Simek]

On 24.9.2018 09:41, Geert Uytterhoeven wrote:
> Hi Michal,
> 
> On Thu, Sep 20, 2018 at 1:42 PM Michal Simek <michal.simek@xilinx.com> wrote:
>> The function travels the lookup table to record alias ids for the given
>> device match structures and alias stem.
>> This function will be used by serial drivers to check if requested alias
>> is allocated or free to use.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> I know this has already been applied, it just drew my attention.
> 
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -16,6 +16,7 @@
>>
>>  #define pr_fmt(fmt)    "OF: " fmt
>>
>> +#include <linux/bitmap.h>
>>  #include <linux/console.h>
>>  #include <linux/ctype.h>
>>  #include <linux/cpu.h>
>> @@ -1943,6 +1944,57 @@ int of_alias_get_id(struct device_node *np, const char *stem)
>>  EXPORT_SYMBOL_GPL(of_alias_get_id);
>>
>>  /**
>> + * of_alias_get_alias_list - Get alias list for the given device driver
>> + * @matches:   Array of OF device match structures to search in
>> + * @stem:      Alias stem of the given device_node
>> + * @bitmap:    Bitmap field pointer
>> + * @nbits:     Maximum number of alias ID which can be recorded it bitmap
> 
> IDs
> 
>> + *
>> + * The function travels the lookup table to record alias ids for the given
>> + * device match structures and alias stem.
>> + *
>> + * Return:     0 or -ENOSYS when !CONFIG_OF
>> + */
>> +int of_alias_get_alias_list(const struct of_device_id *matches,
>> +                            const char *stem, unsigned long *bitmap,
>> +                            unsigned int nbits)
>> +{
>> +       struct alias_prop *app;
>> +
>> +       /* Zero bitmap field to make sure that all the time it is clean */
>> +       bitmap_zero(bitmap, nbits);
>> +
>> +       mutex_lock(&of_mutex);
>> +       pr_debug("%s: Looking for stem: %s\n", __func__, stem);
>> +       list_for_each_entry(app, &aliases_lookup, link) {
>> +               pr_debug("%s: stem: %s, id: %d\n",
>> +                        __func__, app->stem, app->id);
>> +
>> +               if (strcmp(app->stem, stem) != 0) {
>> +                       pr_debug("%s: stem comparison doesn't passed %s\n",
> 
> didn't pass?
> 
>> +                                __func__, app->stem);
>> +                       continue;
>> +               }
>> +
>> +               if (app->id >= nbits) {
>> +                       pr_debug("%s: ID %d greater then bitmap field %d\n",
> 
> than
> %u for unsigned int

I won't fix this now because this can be done together with one change
described below.

> 
>> +                               __func__, app->id, nbits);
>> +                       continue;
> 
> Shouldn't this return an error, if of_match_node() below would have matched?

IIRC Above check is for "serial" name.
This one is for getting XX number from alias "serialXX".
And below is checking compatible string.

You are calling this function from uartps with 32 lines limit and there
is serial33 alias poiting to uartlite (for example) then you don't want
to break this call for that.

What can be done is that compatible string is checked first and if this
passed then ids are check and error can be returned or bit sets.

Thanks,
Michal

Re: [RESEND PATCH v3 1/2] of: base: Introduce of_alias_get_alias_list() to check alias IDs

[Geert Uytterhoeven]

Hi Michal,

On Wed, Sep 26, 2018 at 1:01 PM Michal Simek <michal.simek@xilinx.com> wrote:
> On 24.9.2018 09:41, Geert Uytterhoeven wrote:
> > On Thu, Sep 20, 2018 at 1:42 PM Michal Simek <michal.simek@xilinx.com> wrote:
> >> The function travels the lookup table to record alias ids for the given
> >> device match structures and alias stem.
> >> This function will be used by serial drivers to check if requested alias
> >> is allocated or free to use.
> >>
> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> >> Reviewed-by: Rob Herring <robh@kernel.org>
> >
> > I know this has already been applied, it just drew my attention.
> >
> >> --- a/drivers/of/base.c
> >> +++ b/drivers/of/base.c
> >> @@ -16,6 +16,7 @@
> >>
> >>  #define pr_fmt(fmt)    "OF: " fmt
> >>
> >> +#include <linux/bitmap.h>
> >>  #include <linux/console.h>
> >>  #include <linux/ctype.h>
> >>  #include <linux/cpu.h>
> >> @@ -1943,6 +1944,57 @@ int of_alias_get_id(struct device_node *np, const char *stem)
> >>  EXPORT_SYMBOL_GPL(of_alias_get_id);
> >>
> >>  /**
> >> + * of_alias_get_alias_list - Get alias list for the given device driver
> >> + * @matches:   Array of OF device match structures to search in
> >> + * @stem:      Alias stem of the given device_node
> >> + * @bitmap:    Bitmap field pointer
> >> + * @nbits:     Maximum number of alias ID which can be recorded it bitmap
> >
> > IDs
> >
> >> + *
> >> + * The function travels the lookup table to record alias ids for the given
> >> + * device match structures and alias stem.
> >> + *
> >> + * Return:     0 or -ENOSYS when !CONFIG_OF
> >> + */
> >> +int of_alias_get_alias_list(const struct of_device_id *matches,
> >> +                            const char *stem, unsigned long *bitmap,
> >> +                            unsigned int nbits)
> >> +{
> >> +       struct alias_prop *app;
> >> +
> >> +       /* Zero bitmap field to make sure that all the time it is clean */
> >> +       bitmap_zero(bitmap, nbits);
> >> +
> >> +       mutex_lock(&of_mutex);
> >> +       pr_debug("%s: Looking for stem: %s\n", __func__, stem);
> >> +       list_for_each_entry(app, &aliases_lookup, link) {
> >> +               pr_debug("%s: stem: %s, id: %d\n",
> >> +                        __func__, app->stem, app->id);
> >> +
> >> +               if (strcmp(app->stem, stem) != 0) {
> >> +                       pr_debug("%s: stem comparison doesn't passed %s\n",
> >
> > didn't pass?
> >
> >> +                                __func__, app->stem);
> >> +                       continue;
> >> +               }
> >> +
> >> +               if (app->id >= nbits) {
> >> +                       pr_debug("%s: ID %d greater then bitmap field %d\n",
> >
> > than
> > %u for unsigned int
>
> I won't fix this now because this can be done together with one change
> described below.
>
> >
> >> +                               __func__, app->id, nbits);
> >> +                       continue;
> >
> > Shouldn't this return an error, if of_match_node() below would have matched?
>
> IIRC Above check is for "serial" name.
> This one is for getting XX number from alias "serialXX".
> And below is checking compatible string.
>
> You are calling this function from uartps with 32 lines limit and there
> is serial33 alias poiting to uartlite (for example) then you don't want
> to break this call for that.
>
> What can be done is that compatible string is checked first and if this
> passed then ids are check and error can be returned or bit sets.

That's what I meant: currently any serialN with N >= nbits is ignored, even
if the alias applies to the driver that called the function.

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: [RESEND PATCH v3 1/2] of: base: Introduce of_alias_get_alias_list() to check alias IDs

[Michal Simek]

On 27.9.2018 09:19, Geert Uytterhoeven wrote:
> Hi Michal,
> 
> On Wed, Sep 26, 2018 at 1:01 PM Michal Simek <michal.simek@xilinx.com> wrote:
>> On 24.9.2018 09:41, Geert Uytterhoeven wrote:
>>> On Thu, Sep 20, 2018 at 1:42 PM Michal Simek <michal.simek@xilinx.com> wrote:
>>>> The function travels the lookup table to record alias ids for the given
>>>> device match structures and alias stem.
>>>> This function will be used by serial drivers to check if requested alias
>>>> is allocated or free to use.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>
>>> I know this has already been applied, it just drew my attention.
>>>
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -16,6 +16,7 @@
>>>>
>>>>  #define pr_fmt(fmt)    "OF: " fmt
>>>>
>>>> +#include <linux/bitmap.h>
>>>>  #include <linux/console.h>
>>>>  #include <linux/ctype.h>
>>>>  #include <linux/cpu.h>
>>>> @@ -1943,6 +1944,57 @@ int of_alias_get_id(struct device_node *np, const char *stem)
>>>>  EXPORT_SYMBOL_GPL(of_alias_get_id);
>>>>
>>>>  /**
>>>> + * of_alias_get_alias_list - Get alias list for the given device driver
>>>> + * @matches:   Array of OF device match structures to search in
>>>> + * @stem:      Alias stem of the given device_node
>>>> + * @bitmap:    Bitmap field pointer
>>>> + * @nbits:     Maximum number of alias ID which can be recorded it bitmap
>>>
>>> IDs
>>>
>>>> + *
>>>> + * The function travels the lookup table to record alias ids for the given
>>>> + * device match structures and alias stem.
>>>> + *
>>>> + * Return:     0 or -ENOSYS when !CONFIG_OF
>>>> + */
>>>> +int of_alias_get_alias_list(const struct of_device_id *matches,
>>>> +                            const char *stem, unsigned long *bitmap,
>>>> +                            unsigned int nbits)
>>>> +{
>>>> +       struct alias_prop *app;
>>>> +
>>>> +       /* Zero bitmap field to make sure that all the time it is clean */
>>>> +       bitmap_zero(bitmap, nbits);
>>>> +
>>>> +       mutex_lock(&of_mutex);
>>>> +       pr_debug("%s: Looking for stem: %s\n", __func__, stem);
>>>> +       list_for_each_entry(app, &aliases_lookup, link) {
>>>> +               pr_debug("%s: stem: %s, id: %d\n",
>>>> +                        __func__, app->stem, app->id);
>>>> +
>>>> +               if (strcmp(app->stem, stem) != 0) {
>>>> +                       pr_debug("%s: stem comparison doesn't passed %s\n",
>>>
>>> didn't pass?
>>>
>>>> +                                __func__, app->stem);
>>>> +                       continue;
>>>> +               }
>>>> +
>>>> +               if (app->id >= nbits) {
>>>> +                       pr_debug("%s: ID %d greater then bitmap field %d\n",
>>>
>>> than
>>> %u for unsigned int
>>
>> I won't fix this now because this can be done together with one change
>> described below.
>>
>>>
>>>> +                               __func__, app->id, nbits);
>>>> +                       continue;
>>>
>>> Shouldn't this return an error, if of_match_node() below would have matched?
>>
>> IIRC Above check is for "serial" name.
>> This one is for getting XX number from alias "serialXX".
>> And below is checking compatible string.
>>
>> You are calling this function from uartps with 32 lines limit and there
>> is serial33 alias poiting to uartlite (for example) then you don't want
>> to break this call for that.
>>
>> What can be done is that compatible string is checked first and if this
>> passed then ids are check and error can be returned or bit sets.
> 
> That's what I meant: currently any serialN with N >= nbits is ignored, even
> if the alias applies to the driver that called the function.

ok. Let me create a patch and check compatible string first and then
setup bit or fail if N >= nbits.

Thanks,
Michal

Re: [RESEND PATCH v3 1/2] of: base: Introduce of_alias_get_alias_list() to check alias IDs

[Michal Simek]

Hi Geert,

On 27.9.2018 09:22, Michal Simek wrote:
> On 27.9.2018 09:19, Geert Uytterhoeven wrote:
>> Hi Michal,
>>
>> On Wed, Sep 26, 2018 at 1:01 PM Michal Simek <michal.simek@xilinx.com> wrote:
>>> On 24.9.2018 09:41, Geert Uytterhoeven wrote:
>>>> On Thu, Sep 20, 2018 at 1:42 PM Michal Simek <michal.simek@xilinx.com> wrote:
>>>>> The function travels the lookup table to record alias ids for the given
>>>>> device match structures and alias stem.
>>>>> This function will be used by serial drivers to check if requested alias
>>>>> is allocated or free to use.
>>>>>
>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>>
>>>> I know this has already been applied, it just drew my attention.
>>>>
>>>>> --- a/drivers/of/base.c
>>>>> +++ b/drivers/of/base.c
>>>>> @@ -16,6 +16,7 @@
>>>>>
>>>>>  #define pr_fmt(fmt)    "OF: " fmt
>>>>>
>>>>> +#include <linux/bitmap.h>
>>>>>  #include <linux/console.h>
>>>>>  #include <linux/ctype.h>
>>>>>  #include <linux/cpu.h>
>>>>> @@ -1943,6 +1944,57 @@ int of_alias_get_id(struct device_node *np, const char *stem)
>>>>>  EXPORT_SYMBOL_GPL(of_alias_get_id);
>>>>>
>>>>>  /**
>>>>> + * of_alias_get_alias_list - Get alias list for the given device driver
>>>>> + * @matches:   Array of OF device match structures to search in
>>>>> + * @stem:      Alias stem of the given device_node
>>>>> + * @bitmap:    Bitmap field pointer
>>>>> + * @nbits:     Maximum number of alias ID which can be recorded it bitmap
>>>>
>>>> IDs
>>>>
>>>>> + *
>>>>> + * The function travels the lookup table to record alias ids for the given
>>>>> + * device match structures and alias stem.
>>>>> + *
>>>>> + * Return:     0 or -ENOSYS when !CONFIG_OF
>>>>> + */
>>>>> +int of_alias_get_alias_list(const struct of_device_id *matches,
>>>>> +                            const char *stem, unsigned long *bitmap,
>>>>> +                            unsigned int nbits)
>>>>> +{
>>>>> +       struct alias_prop *app;
>>>>> +
>>>>> +       /* Zero bitmap field to make sure that all the time it is clean */
>>>>> +       bitmap_zero(bitmap, nbits);
>>>>> +
>>>>> +       mutex_lock(&of_mutex);
>>>>> +       pr_debug("%s: Looking for stem: %s\n", __func__, stem);
>>>>> +       list_for_each_entry(app, &aliases_lookup, link) {
>>>>> +               pr_debug("%s: stem: %s, id: %d\n",
>>>>> +                        __func__, app->stem, app->id);
>>>>> +
>>>>> +               if (strcmp(app->stem, stem) != 0) {
>>>>> +                       pr_debug("%s: stem comparison doesn't passed %s\n",
>>>>
>>>> didn't pass?
>>>>
>>>>> +                                __func__, app->stem);
>>>>> +                       continue;
>>>>> +               }
>>>>> +
>>>>> +               if (app->id >= nbits) {
>>>>> +                       pr_debug("%s: ID %d greater then bitmap field %d\n",
>>>>
>>>> than
>>>> %u for unsigned int
>>>
>>> I won't fix this now because this can be done together with one change
>>> described below.
>>>
>>>>
>>>>> +                               __func__, app->id, nbits);
>>>>> +                       continue;
>>>>
>>>> Shouldn't this return an error, if of_match_node() below would have matched?
>>>
>>> IIRC Above check is for "serial" name.
>>> This one is for getting XX number from alias "serialXX".
>>> And below is checking compatible string.
>>>
>>> You are calling this function from uartps with 32 lines limit and there
>>> is serial33 alias poiting to uartlite (for example) then you don't want
>>> to break this call for that.
>>>
>>> What can be done is that compatible string is checked first and if this
>>> passed then ids are check and error can be returned or bit sets.
>>
>> That's what I meant: currently any serialN with N >= nbits is ignored, even
>> if the alias applies to the driver that called the function.
> 
> ok. Let me create a patch and check compatible string first and then
> setup bit or fail if N >= nbits.

I have sent a patch with this change. Let's have discussion in that new
thread.

Thanks,
Michal