diff mbox series

ns16550: Re-order the serial port address checking

Message ID 5E53B3E3.6070304@hisilicon.com (mailing list archive)
State New, archived
Headers show
Series ns16550: Re-order the serial port address checking | expand

Commit Message

Wei Xu Feb. 24, 2020, 11:30 a.m. UTC
Check the serial port address space before checking the address.

Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
---
 xen/drivers/char/ns16550.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Julien Grall Feb. 24, 2020, 10:03 p.m. UTC | #1
Hi Wei,

Thank you for sending a follow-up.

On 24/02/2020 11:30, Wei Xu wrote:
> Check the serial port address space before checking the address.

Could you explain in a sentence why you want to re-order?

Cheers,

> 
> Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
> ---
>   xen/drivers/char/ns16550.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index dc8ac4c..425e044 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1644,6 +1644,12 @@ static int __init ns16550_acpi_uart_init(const void *data)
> 
>       spcr = container_of(table, struct acpi_table_spcr, header);
> 
> +    if ( unlikely(spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) )
> +    {
> +        printk("ns16550: Address space type is not mmio\n");
> +        return -EINVAL;
> +    }
> +
>       /*
>        * The serial port address may be 0 for example
>        * if the console redirection is disabled.
> @@ -1654,12 +1660,6 @@ static int __init ns16550_acpi_uart_init(const void *data)
>           return -EINVAL;
>       }
> 
> -    if ( unlikely(spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) )
> -    {
> -        printk("ns16550: Address space type is not mmio\n");
> -        return -EINVAL;
> -    }
> -
>       ns16550_init_common(uart);
> 
>       /*
>
Wei Xu Feb. 25, 2020, 12:33 a.m. UTC | #2
Hi Julien,

On 2020/2/25 6:03, Julien Grall wrote:
> Hi Wei,
> 
> Thank you for sending a follow-up.
> 
> On 24/02/2020 11:30, Wei Xu wrote:
>> Check the serial port address space before checking the address.
> 
> Could you explain in a sentence why you want to re-order?

Do you think is it OK to explain as following:

The serial port address space ID qualifies the address.
Whether a value of zero for the serial port address can sensibly
mean "disabled" depends on the address space ID. Hence check the
address space ID before checking the address.

Thanks!

Best Regards,
Wei

> 
> Cheers,
> 
>>
>> Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
>> ---
>>   xen/drivers/char/ns16550.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index dc8ac4c..425e044 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1644,6 +1644,12 @@ static int __init ns16550_acpi_uart_init(const void *data)
>>
>>       spcr = container_of(table, struct acpi_table_spcr, header);
>>
>> +    if ( unlikely(spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) )
>> +    {
>> +        printk("ns16550: Address space type is not mmio\n");
>> +        return -EINVAL;
>> +    }
>> +
>>       /*
>>        * The serial port address may be 0 for example
>>        * if the console redirection is disabled.
>> @@ -1654,12 +1660,6 @@ static int __init ns16550_acpi_uart_init(const void *data)
>>           return -EINVAL;
>>       }
>>
>> -    if ( unlikely(spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) )
>> -    {
>> -        printk("ns16550: Address space type is not mmio\n");
>> -        return -EINVAL;
>> -    }
>> -
>>       ns16550_init_common(uart);
>>
>>       /*
>>
>
Jan Beulich Feb. 25, 2020, 1:10 p.m. UTC | #3
On 25.02.2020 01:33, Wei Xu wrote:
> On 2020/2/25 6:03, Julien Grall wrote:
>> Thank you for sending a follow-up.
>>
>> On 24/02/2020 11:30, Wei Xu wrote:
>>> Check the serial port address space before checking the address.
>>
>> Could you explain in a sentence why you want to re-order?
> 
> Do you think is it OK to explain as following:
> 
> The serial port address space ID qualifies the address.
> Whether a value of zero for the serial port address can sensibly
> mean "disabled" depends on the address space ID. Hence check the
> address space ID before checking the address.

With this description ...

>>> Signed-off-by: Wei Xu <xuwei5@hisilicon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

And I'm sorry for having forgotten to switch these around
while committing your earlier patch.

Jan
Wei Xu Feb. 26, 2020, 12:41 a.m. UTC | #4
Hi Jan,

On 2020/2/25 21:10, Jan Beulich wrote:
> On 25.02.2020 01:33, Wei Xu wrote:
>> On 2020/2/25 6:03, Julien Grall wrote:
>>> Thank you for sending a follow-up.
>>>
>>> On 24/02/2020 11:30, Wei Xu wrote:
>>>> Check the serial port address space before checking the address.
>>>
>>> Could you explain in a sentence why you want to re-order?
>>
>> Do you think is it OK to explain as following:
>>
>> The serial port address space ID qualifies the address.
>> Whether a value of zero for the serial port address can sensibly
>> mean "disabled" depends on the address space ID. Hence check the
>> address space ID before checking the address.
> 
> With this description ...
> 
>>>> Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks!

Best Regards,
Wei

> 
> And I'm sorry for having forgotten to switch these around
> while committing your earlier patch.
> 
> Jan
> 
> .
>
diff mbox series

Patch

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index dc8ac4c..425e044 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1644,6 +1644,12 @@  static int __init ns16550_acpi_uart_init(const void *data)

     spcr = container_of(table, struct acpi_table_spcr, header);

+    if ( unlikely(spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) )
+    {
+        printk("ns16550: Address space type is not mmio\n");
+        return -EINVAL;
+    }
+
     /*
      * The serial port address may be 0 for example
      * if the console redirection is disabled.
@@ -1654,12 +1660,6 @@  static int __init ns16550_acpi_uart_init(const void *data)
         return -EINVAL;
     }

-    if ( unlikely(spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) )
-    {
-        printk("ns16550: Address space type is not mmio\n");
-        return -EINVAL;
-    }
-
     ns16550_init_common(uart);

     /*