On 29.03.2023 16:35, Ayan Kumar Halder wrote:
Please let me know if the below patch looks fine.
Apart from the comments below there may be formatting issues, which
I can't sensibly comment on when the patch was mangled by your mailer
anyway. (Which in turn is why it is generally better to properly send
a new version, rather than replying with kind-of-a-new-version on an
earlier thread.)
Additionally, up front: I'm sorry for the extra requests, but I'm
afraid to sensibly make the changes you want to make some things need
sorting first, to avoid extending pre-existing clumsiness. This is
irrespective of the present state of things clearly not being your
fault.
@@ -1235,6 +1235,8 @@ pci_uart_config(struct ns16550 *uart, bool_t
skip_amt, unsigned int idx)
/* MMIO based */
if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
{
+ uint64_t pci_uart_io_base;
+
pci_conf_write32(PCI_SBDF(0, b, d, f),
PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
len = pci_conf_read32(PCI_SBDF(0, b, d, f),
@@ -1259,8 +1261,17 @@ pci_uart_config(struct ns16550 *uart, bool_t
skip_amt, unsigned int idx)
else
size = len & PCI_BASE_ADDRESS_MEM_MASK;
- uart->io_base = ((u64)bar_64 << 32) |
- (bar & PCI_BASE_ADDRESS_MEM_MASK);
+ pci_uart_io_base = ((uint64_t)bar_64 << 32) |
+ (bar & PCI_BASE_ADDRESS_MEM_MASK);
+
+ /* Truncation detected while converting to paddr_t */
+ if ( pci_uart_io_base != (paddr_t)pci_uart_io_base )
+ {
+ printk("ERROR: Truncation detected for io_base
address");
+ return -EINVAL;
+ }
Further down the function returns -1, so here you assume EINVAL != 1.
Such assumptions (and mixing of value spaces) is generally not a good
idea. Since there are other issues (see below), maybe you really want
to add a prereq patch addressing those? That would include changing the
"return -1" to either "return 1" or making it use some sensible and
properly distinguishable errno value.
@@ -1519,20 +1530,40 @@ 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) )
+ int ret;
+
+ ret = pci_uart_config(uart, 1/* skip AMT */, uart -
ns16550_com);
+
+ if ( ret == -EINVAL )
+ return false;
+ else if ( ret )
return true;
With skip_amt != 0 the function presently can only return 0. You're
therefore converting pre-existing dead code to another form of dead
code. Otoh (and as, I think, previously indicated) ...
+
conf += 3;
}
else if ( strncmp(conf, "amt", 3) == 0 )
{
- if ( pci_uart_config(uart, 0, uart - ns16550_com) )
+ int ret = pci_uart_config(uart, 0, uart - ns16550_com);
+
+ if ( ret == -EINVAL )
+ return false;
+ else if ( ret )
return true;
... the equivalent of this in parse_namevalue_pairs() not checking
the return value is bogus. But it is further bogus that the case
where skip_amt has passed 1 for it sets dev_set to true
unconditionally, i.e. even when no device was found. IOW I also
question the correctness of the final "return 0" in pci_uart_config().
I looks to me as if this wants to be a skip_amt-independent
"return -ENODEV". skip_amt would only control whether uart->io_base is
restored before returning (leaving aside the question of why that is).
