Re: [XEN v1 1/9] xen/arm: Remove the extra assignment

[Julien Grall <julien@xxxxxxx>]

Hi,

In the previous version, I have suggested the following title:

xen/ns16550: Remove unneeded truncation check in the DT init code

This would also address Jan's comment.

On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> As "io_size" and "uart->io_size" are both u64, so there will be no truncation.
> Thus, one can remove the ASSERT() and extra assignment.
>
> In an earlier commit (7c1de0038895),
Why is this line shorter than the others?

> "ns16550.io_size" was u32 and "io_size" was u64. Thus, the ASSERT() was needed
> to check if the values are the same.
> However, in a later commit (c9f8e0aee507),
> "ns16550.io_size" was changed to u64. Thus, the ASSERT() became redundant.
You missed my comment here:

"Those two paragraphs are explaining why the truncation check is 
removed. So I think they should be moved first. Then you can add the 
initial paragraph to explain the resolution".
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
>
>   xen/drivers/char/ns16550.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 01a05c9aa8..58d0ccd889 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1747,7 +1747,6 @@ static int __init ns16550_uart_dt_init(struct 
> dt_device_node *dev,
>       struct ns16550 *uart;
>       int res;
>       u32 reg_shift, reg_width;
> -    u64 io_size;
>   
>       uart = &ns16550_com[0];
>   
> @@ -1758,14 +1757,10 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
>       uart->parity    = UART_PARITY_NONE;
>       uart->stop_bits = 1;
>   
> -    res = dt_device_get_address(dev, 0, &uart->io_base, &io_size);
> +    res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
>       if ( res )
>           return res;
>   
> -    uart->io_size = io_size;
> -
> -    ASSERT(uart->io_size == io_size); /* Detect truncation */
> -
>       res = dt_property_read_u32(dev, "reg-shift", &reg_shift);
>       if ( !res )
>           uart->reg_shift = 0;
Cheers,

--
Julien Grall