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).
Jan
