[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Ayan Kumar Halder <ayankuma@xxxxxxx>, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 11 Jul 2023 18:16:37 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=N4dg9Szd5vVdHhz2bL9kxNl27QPYC3Gkivg4U6lpTrg=; b=FmSDk3Sz/jeEzRSuj4+UU7h+ciGAGuIip70Ehxu5t+awkZDmhrs6R8Jgis7Jh0NPXdM9b2i3SAqS5E+v09uZQrD84PXJFf0OK/lqjmfrgooPHcCseivu+h8D7+1dJevbHRXuKqLNufzEf/BSx/Q0Rlj0pV972/P+ENrPDH/sX1Pw+iFzK0eHkK7iWOtxqeJ6mGiEwoNmXV5UIsfYVFfRhvleowoaV+WMHMJqzRoo884ljEkGt4gF8Qvz0v8EgcsfGFhcItDg9NfEV00A5PyLfSeKZS66C1jVG7eq90cXFi0KXqnqL3LhlNazDs62Lu+udFoljYj9F4uqbwTssAs53A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Wsd0hh14K4JpzKQy3zu1MDxZvaKb6iGmemXPfnY1hBShli2u8DdKEr2NTuJRFrDcCRyue9j9foG5jngzV0l7VA+rcGVIkIyw7d3vfqBzvuTLFTkP0DJVLXtHIS3lOxMfiKDTw4OCrMAN09F6z5cUwXCH8Me7z496oCX3TcKZen8p3ACFTL7Fl2J7iEwK34TWUjmIA1ad7A3SOkE1dItgLjKjlzJaccBdMIz4i7ONfhJ9d4/bfSjP7DAuJCdtv05TsvfVlwxEshv0pvIu5wx9m7wmTN/dwT9GD8OwP9nE5F+opy3WyEOuOB6tfAo0cmqApCZVP9FrfAzxSiakfJg2Yg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: sstabellini@xxxxxxxxxx, stefano.stabellini@xxxxxxx, andrew.cooper3@xxxxxxxxxx, george.dunlap@xxxxxxxxxx, julien@xxxxxxx, wl@xxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 11 Jul 2023 16:16:57 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.