pci_uart_config() should return 0 when it probes the correct uart device and
sets the properties of "struct ns16550". Else, it should return -ENODEV.
Also before returning -ENODEV, it should restore the value of uart->io_base.
The callers should check if pci_uart_config() has returned 0 (for success) or
not.
Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
This was based on a discussion "[XEN v4 04/11] xen/drivers: ns16550: Use paddr_t for io_base/io_size"
https://patchew.org/Xen/20230321140357.24094-1-ayan.kumar.halder@amd.com/20230321140357.24094-5-ayan.kumar.halder@amd.com/
xen/drivers/char/ns16550.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 212a9c49ae..5a35498a06 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1342,13 +1342,9 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
}
}
- if ( !skip_amt )
- return -1;
-
- /* No AMT found, fallback to the defaults. */
uart->io_base = orig_base;
- return 0;
+ return -ENODEV;
}
static void enable_exar_enhanced_bits(const struct ns16550 *uart)
@@ -1527,13 +1523,13 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
#ifdef CONFIG_HAS_PCI
if ( strncmp(conf, "pci", 3) == 0 )
{
- if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
+ if ( !pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
return true;
conf += 3;
}
else if ( strncmp(conf, "amt", 3) == 0 )
{
- if ( pci_uart_config(uart, 0, uart - ns16550_com) )
+ if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
return true;
conf += 3;
}
@@ -1642,13 +1638,17 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
case device:
if ( strncmp(param_value, "pci", 3) == 0 )
{
- pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
- dev_set = true;
+ if ( !pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
+ dev_set = true;
+ else
+ return false;
}
else if ( strncmp(param_value, "amt", 3) == 0 )
{
- pci_uart_config(uart, 0, uart - ns16550_com);
- dev_set = true;
+ if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
+ dev_set = true;
+ else
+ return false;
}
break;
--
2.25.1On 07.07.2023 13:35, Ayan Kumar Halder wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1342,13 +1342,9 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
> }
> }
>
> - if ( !skip_amt )
> - return -1;
This special case probably needs retaining in the new model (with an
altered return value), such that ...
> - /* No AMT found, fallback to the defaults. */
> uart->io_base = orig_base;
>
> - return 0;
> + return -ENODEV;
> }
>
> static void enable_exar_enhanced_bits(const struct ns16550 *uart)
> @@ -1527,13 +1523,13 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
> #ifdef CONFIG_HAS_PCI
> if ( strncmp(conf, "pci", 3) == 0 )
> {
> - if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
> + if ( !pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
> return true;
> conf += 3;
> }
> else if ( strncmp(conf, "amt", 3) == 0 )
> {
> - if ( pci_uart_config(uart, 0, uart - ns16550_com) )
> + if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
> return true;
> conf += 3;
> }
... e.g. here you don't suddenly change behavior in unintended ways.
Prior to your change the earlier of the return paths was impossible
to be taken. That's likely wrong, but you now returning in the success
case can't be correct either: Further items may need parsing, first
and foremost the IRQ to use.
Jan
> @@ -1642,13 +1638,17 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
> case device:
> if ( strncmp(param_value, "pci", 3) == 0 )
> {
> - pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
> - dev_set = true;
> + if ( !pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
> + dev_set = true;
> + else
> + return false;
> }
> else if ( strncmp(param_value, "amt", 3) == 0 )
> {
> - pci_uart_config(uart, 0, uart - ns16550_com);
> - dev_set = true;
> + if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
> + dev_set = true;
> + else
> + return false;
> }
> break;
>
Hi Jan,
On 10/07/2023 11:08, Jan Beulich wrote:
> On 07.07.2023 13:35, Ayan Kumar Halder wrote:
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -1342,13 +1342,9 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>> }
>> }
>>
>> - if ( !skip_amt )
>> - return -1;
> This special case probably needs retaining in the new model (with an
> altered return value), such that ...
Does this look correct ?
if ( !skip_amt )
- return -1;
+ return -EINVAL;
>
>> - /* No AMT found, fallback to the defaults. */
>> uart->io_base = orig_base;
>>
>> - return 0;
>> + return -ENODEV;
>> }
>>
>> static void enable_exar_enhanced_bits(const struct ns16550 *uart)
>> @@ -1527,13 +1523,13 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>> #ifdef CONFIG_HAS_PCI
>> if ( strncmp(conf, "pci", 3) == 0 )
>> {
>> - if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
>> + if ( !pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
>> return true;
>> conf += 3;
>> }
>> else if ( strncmp(conf, "amt", 3) == 0 )
>> {
>> - if ( pci_uart_config(uart, 0, uart - ns16550_com) )
>> + if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
>> return true;
>> conf += 3;
>> }
> ... e.g. here you don't suddenly change behavior in unintended ways.
> Prior to your change the earlier of the return paths was impossible
> to be taken. That's likely wrong, but you now returning in the success
> case can't be correct either:
I am afraid I don't follow your comments very well.
pci_uart_config() returns 0 for success. So we need to check "!(pci_uart_config(...)" to return true.
> Further items may need parsing, first
> and foremost the IRQ to use.
I see the irq is parsed here
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/drivers/char/ns16550.c;h=212a9c49ae8e5c40dc6cd05da07a6652c8405935;hb=refs/heads/master#l1558
.
Do we need to do something more ?
- Ayan
>
> Jan
>
>> @@ -1642,13 +1638,17 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
>> case device:
>> if ( strncmp(param_value, "pci", 3) == 0 )
>> {
>> - pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
>> - dev_set = true;
>> + if ( !pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
>> + dev_set = true;
>> + else
>> + return false;
>> }
>> else if ( strncmp(param_value, "amt", 3) == 0 )
>> {
>> - pci_uart_config(uart, 0, uart - ns16550_com);
>> - dev_set = true;
>> + if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
>> + dev_set = true;
>> + else
>> + return false;
>> }
>> break;
>>
On 11.07.2023 17:43, Ayan Kumar Halder wrote:
> On 10/07/2023 11:08, Jan Beulich wrote:
>> On 07.07.2023 13:35, Ayan Kumar Halder wrote:
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -1342,13 +1342,9 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>>> }
>>> }
>>>
>>> - if ( !skip_amt )
>>> - return -1;
>> This special case probably needs retaining in the new model (with an
>> altered return value), such that ...
>
> Does this look correct ?
>
> if ( !skip_amt )
> - return -1;
> + return -EINVAL;
It's hard to say without seeing what else changes are done to the patch,
but at the first glance this looks wrong. If you change the function
along the lines of the initial patch, then likely this wants to be a
positive return value (to tell "failure" from "success" as well as from
this special case).
>>> @@ -1527,13 +1523,13 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>>> #ifdef CONFIG_HAS_PCI
>>> if ( strncmp(conf, "pci", 3) == 0 )
>>> {
>>> - if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
>>> + if ( !pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
>>> return true;
>>> conf += 3;
>>> }
>>> else if ( strncmp(conf, "amt", 3) == 0 )
>>> {
>>> - if ( pci_uart_config(uart, 0, uart - ns16550_com) )
>>> + if ( !pci_uart_config(uart, 0, uart - ns16550_com) )
>>> return true;
>>> conf += 3;
>>> }
>> ... e.g. here you don't suddenly change behavior in unintended ways.
>> Prior to your change the earlier of the return paths was impossible
>> to be taken. That's likely wrong, but you now returning in the success
>> case can't be correct either:
> I am afraid I don't follow your comments very well.
>
> pci_uart_config() returns 0 for success. So we need to check "!(pci_uart_config(...)" to return true.
But you cannot return here in the success case. You need to acknowledge
that in the original code a kind-of-error indication from the function
is converted to a success return here (which was impossible to happen
in one of the two cases, in turn causing extra confusion). So changing
this is a two-step process: First it needs to be understood what the
behavior ought to be at each of the four call sites. That behavior
likely isn't going to be the same in all four cases. And then this
needs transforming into the intended code change.
Jan
© 2016 - 2024 Red Hat, Inc.