Re: [PATCH v2] serial: uartlite: Move out-of-range port-numbers into ULITE_NR_UARTS range

[Michal Simek <michal.simek@xxxxxxxxxx>]

On 11/18/21 22:17, Ruediger Willenberg wrote:
> Find free uart_port struct in range 0 <= id < ULITE_NR_UARTS when
> the device tree port-number property is outside that range. This
> happens when there are other UART types in the system because the
> Xilinx device tree generator numbers all UARTs consecutively;
> as a result, not as many Uartlites as specified by the
> SERIAL_UARTLITE_NR_UARTS parameter could be successfully added.
>
> Signed-off-by: Ruediger Willenberg <r.willenberg@xxxxxxxxxxxxxx>
> ---
> Changes in v2:
>   - give KERN_NOTICE when changing the id,
>     with reference to the requested port-number
>
>   drivers/tty/serial/uartlite.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index d3d9566e5dbd..27c513c7350e 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -631,15 +631,17 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
>   {
>   	struct uart_port *port;
>   	int rc;
> +	int oor_id = -1;
>   
> -	/* if id = -1; then scan for a free id and use that */
> -	if (id < 0) {
> +	/* if id -1 or out of range; then scan for a free id and use that */
> +	if (id < 0 || id >= ULITE_NR_UARTS) {
> +		oor_id = id;
>   		for (id = 0; id < ULITE_NR_UARTS; id++)
>   			if (ulite_ports[id].mapbase == 0)
>   				break;
>   	}
> -	if (id < 0 || id >= ULITE_NR_UARTS) {
> -		dev_err(dev, "%s%i too large\n", ULITE_NAME, id);
> +	if (id == ULITE_NR_UARTS) {
> +		dev_err(dev, "maximum number of %s assigned\n", ULITE_NAME);
>   		return -EINVAL;
>   	}
>   
> @@ -676,7 +678,11 @@ static int ulite_assign(struct device *dev, int id, u32 base, int irq,
>   		dev_set_drvdata(dev, NULL);
>   		return rc;
>   	}

nit: please keep this newline here.

> -
> +	if (oor_id >= 0)
> +		dev_notice(dev,
> +			"assigned uartlite with device tree port-number=<%i> to %s%i\n",
> +			oor_id, ULITE_NAME, id);
> +



[linux](master)$ ./scripts/checkpatch.pl --strict 
0001-serial-uartlite-Move-out-of-range-port-numbers-into-.patch
CHECK: Alignment should match open parenthesis
#54: FILE: drivers/tty/serial/uartlite.c:669:
+		dev_notice(dev,
+			"assigned uartlite with device tree port-number=<%i> to %s%i\n",



>   	return 0;
>   }
>   

And there is one more issue with this. If you start to mix serial IPs 
which are partially recorded in aliases. For example like this.

aliases {
	serial0 = &uart1;
};

uart0 {
...
};

uart1 {
...
};

Uart0 is probed first. It is without alias and id 0 will be assigned to 
it. Then uart1 is probed. It looks at aliases you get id 0 but it is 
already taken and you end up with "cannot assign to %s%i; it is already 
in use"

It would be IMHO better to use of_alias_get_highest_id().
In xilinx_uartps I used in past of_alias_get_alias_list() to get 
bitfield of aliases which are taken and use only that one which are free 
but up2you. Having the same behavior across drivers would be good.

Thanks,
Michal