Re: [PATCH v17 01/10] LIB: Introduce a generic PIO mapping method

[John Garry]

> > > +int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
> > > +{
> > > +	struct logic_pio_hwaddr *range;
> > > +	resource_size_t start = new_range->hw_start;
> > > +	resource_size_t end = new_range->hw_start + new_range->size;
> > > +	resource_size_t mmio_sz = 0;
> > > +	resource_size_t iio_sz = MMIO_UPPER_LIMIT;
> > > +	int ret = 0;
> > > +
> > > +	if (!new_range || !new_range->fwnode || !new_range->size)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&io_range_mutex);
> > > +	list_for_each_entry_rcu(range, &io_range_list, list) {
> > > +		if (range->fwnode == new_range->fwnode) {
> > > +			/* range already there */
> > > +			ret = -EFAULT;
> > > +			goto end_register;
> > > +		}

Hi Thierry,

> > This is the -EFAULT that propagates to pci-tegra.c's ->probe() and fails
> > to bind the driver.
> >
> > I'm not exactly sure what's causing the duplicate here because it's
> > rather difficult to get at something useful from just the ->fwnode, but
> > I'm fairly sure that the reason this breaks is because the Tegra driver
> > will defer probe due to some regulators that aren't available on the
> > first try. Given the above code and the rest of this file, I can't see a
> > way to "fix" the driver and remove the I/O range on failure.
> >
> > This is doubly bad because this doesn't only leak the ranges on probe
> > deferral, but also on driver unload, and we just added support for
> > building the Tegra driver as a loadable module, so these are actually
> > cases that can happen in regular uses of the driver.
> >
> > I have no idea on how to fix this. Anyone know of a quick fix to restore
> > PCI for Tegra other than reverting all of these changes?
> >
> > I suppose an API could be added to unregister the range, but the calling
> > sequence is rather obfuscated, so removing the range will look totally
> > asymmetric, I'm afraid.
> >
> > Here's the call stack:
> >
> > 	tegra_pcie_probe()
> > 	tegra_pcie_parse_dt()
> > 	of_pci_range_to_resource()
> > 	pci_register_io_range()
> > 	logic_pio_register_range()
> >
> > So the range here is registered as part of a resource parsing function,
> > which is supposed to not have any side-effects. There's no equivalent of
> > that parsing routine (i.e. no "unparse" function that would undo the
> > effects of parsing).
> >
> > Perhaps a cleaner way would be to decouple the parsing from the actual
> > request step that has the side-effect.

This could be added if we agreed that it would be useful.

> > Going back in history a little, it looks like even before this commit
> > the I/O range registration was triggered by the parsing code and even
> > the range leak was there, except that it caused pci_register_io_range()
> > to return 0 rather than -EFAULT. Perhaps the quickest fix for this would
> > be to do the same in the new code and restore drivers that accidentally
> > depend on this behaviour.
>
> I can confirm that the following fixes the issue for me, though I don't
> think it's a very clean fix given that the range will remain requested
> forever, even if the driver is gone. But since that's already been the
> case for quite a while, probably something that can be fixed separately.

Right, there was no way to deregister the range previously.  From 
looking at the history here I see no reason to not support it.

As for this patch, as you said, the only difference is that we fault on 
trying to register the same range again. So this solution seems reasonable.

On another point, for the tegra driver, is it possible to defer earlier 
in the probe, before these currently irreversible steps are taken?

Thanks very much,
John

> Cc'ing linux-tegra for visibility.
>
> Thierry
>
> --- >8 ---
> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> index 29cedeadb397..4664b87e1c5f 100644
> --- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -46,7 +46,6 @@ int logic_pio_register_range(struct logic_pio_hwaddr *new_range)
>  	list_for_each_entry_rcu(range, &io_range_list, list) {
>  		if (range->fwnode == new_range->fwnode) {
>  			/* range already there */
> -			ret = -EFAULT;
>  			goto end_register;
>  		}
>  		if (range->flags == LOGIC_PIO_CPU_MMIO &&