[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: Jan Beulich <jbeulich@xxxxxxxx>, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
- From: Ayan Kumar Halder <ayankuma@xxxxxxx>
- Date: Tue, 11 Jul 2023 16:43:17 +0100
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.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=toprCbtRqWWEFm51huvlokYccBkGzouj7gEFp/QXEYA=; b=XcGzXMRH8Lq06OObPfI/f0DKr2TY29H0PlQNYOfENx0CBSw3eqFyLO/fuZrgvGf4+DIM9rpTcDlzSs+L/f2gan+bxboa8/KSj/+uQT2HjwAQ1rqwjGVXKaPZWWk1L4wzl6mdc53ffc9PbEg4SiUiL/moWoYqGE6qZLOQTwjw72ZzdSQIS06d5hT/V17ZX/Tk9eU4upMDL0Ty5Cq1SJi6yXX+XsrqTILUfn0eGvtHSXPmt28vAxY1P9LFdrnCD5EI8uiYX6QkGMcgAjFhXcTEJc/H+NldyGE6dFVEybHTyz/EOIziMoDN5qZkZt9Z4IACkOwm1dp2eyL8VeDWs9oC/w==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=iqLqpXGfZV1XnOOZb4Vhc5TuFvnaXGMzBv46IsDQdp6FX9o9TwbW6WOEK/Kc5Ae1cX54vKMo+9LTl35oyEr3CZi3TakBz+lIC4YCC1UbmeRmWrhAwL589uC1QVRwTf1EA3mXOEFj62DvFc1pl14P4Ub94yw/GvHgxaaS8lTqTLHV8aQZVi89HOsL5BKpcENHkpL8dsKRhO7Ot3YsXogbh4xlEUJXX4B5aLsb1OoBpzY43IzLXpOLhmZeQjqabMjYO74dDYkN7uAIfinB6kfgIiK2El+LGAoBaWHuk4ZeYME0lCeBQuctssDxpeCGmqtEGGQE3tqoTyXqpSGrCR8wvQ==
- Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.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 15:43:55 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
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;
|
