[PATCH v2] serial: ns16550: Fix ordering of getting base address

[Andy Shevchenko]

On Fri, Apr 03, 2020 at 05:46:19AM -0700, Bin Meng wrote:
> Currently the driver gets ns16550 base address in the driver
> probe() routine, which may potentially break any ns16550 wrapper
> driver that does additional initialization before calling
> ns16550_serial_probe().
> 
> Things are complicated that we need consider ns16550 devices on
> both simple-bus and PCI bus. To fix the issue we move the base
> address assignment for simple-bus ns16550 device back to the
> ofdata_to_platdata(), and assign base address for PCI ns16550
> device in ns16550_serial_probe().
> 
> This is still not perfect. Ideally if any PCI bus based ns16550
> wrapper driver tries to access plat->base before calling probe(),
> it is subject to break.
> 
> Fixes: 720f9e1fdb0c9 ("serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()")
> Reported-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> Tested-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> Reviewed-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
> Tested-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>

...

> -	if (addr == FDT_ADDR_T_NONE)
> -		return -EINVAL;
> -
> -#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> -	plat->base = addr;
> -#else
> -	plat->base = (unsigned long)map_physmem(addr, 0, MAP_NOCACHE);
> -#endif
> -
> -	return 0;

(1) for below.

...


> +		addr = devfdt_get_addr_pci(dev);

> +		if (addr == FDT_ADDR_T_NONE)
> +			return -EINVAL;
> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +		plat->base = addr;
> +#else
> +		plat->base = (unsigned long)map_physmem(addr, 0, MAP_NOCACHE);
>  #endif


And now is the question why we can't use above as a helper in both cases
(either with check for address correctness or without)?

> +	}

...

> +	addr = dev_read_addr(dev);
> +	if (addr != FDT_ADDR_T_NONE) {
> +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
> +		plat->base = addr;
> +#else
> +		plat->base = (unsigned long)map_physmem(addr, 0, MAP_NOCACHE);
> +#endif
> +	} else if (!device_is_on_pci_bus(dev)) {
> +		return -EINVAL;
> +	}

Something like

	addr = dev_read_addr(dev);
	ret = ns16550_assign_base(addr, plat);
	if (ret && !device_is_on_pci_bus(...))
		return ret;

-- 
With Best Regards,
Andy Shevchenko