Re: [PATCH v1] xen/drivers: ns16550: Fix the return logic for pci_uart_config()

[Jan Beulich <jbeulich@xxxxxxxx>]

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