Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau <Liviu.Dudau@arm.com>]

On Sat, Mar 08, 2014 at 05:15:08PM +0000, Arnd Bergmann wrote:
> On Wednesday 05 March 2014, Liviu Dudau wrote:
> > +
> > +	pr_debug("Parsing ranges property...\n");
> > +	for_each_of_pci_range(&parser, &range) {
> > +		/* Read next ranges element */
> > +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> > +				range.pci_space, range.pci_addr);
> > +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> > +					range.cpu_addr, range.size);
> > +
> > +		/*
> > +		 * If we failed translation or got a zero-sized region
> > +		 * then skip this range
> > +		 */
> > +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> > +			continue;
> > +
> > +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		of_pci_range_to_resource(&range, dev, res);
> > +
> > +		if (resource_type(res) == IORESOURCE_IO)
> > +			*io_base = range.cpu_addr;
> > +
> > +		pci_add_resource_offset(resources, res,
> > +				res->start - range.pci_addr);
> > +	}
> 
> As mentioned regarding the pci_register_io_range() helper, x86
> would not enter the 'resource_type(res) == IORESOURCE_IO' code path,
> which on the one hand is fine so we can return an error from
> pci_register_io_range() there, but I think it will lead to
> io_base getting an uninitialized content.
> 
> There could also be other reasons why pci_register_io_range() fails,
> e.g. because the space is exhausted, and I think we should try to
> catch that here and skip the pci_add_resource_offset() and io_base
> assignment then.

Hi Arnd,

I will try to improve the error handling in the next patchset version.
However I am still confused about the earlier discussion on
pci_register_io_range(). Your suggestion initially was to return an
error in the default weak implementation, but in your last email you
are talking about returning 'port'. My idea when I've introduced the
helper function was that it would return an error if it fails to
register the IO range and zero otherwise. I agree that we can treat
the default 'do nothing with the IO range' case as an error, with
the caveat that will force architectures that use this code to
provide their own implementation of pci_register_io_range() in order
to avoid failure, even for the cases where the architecture has a 1:1
mapping between IO and CPU addresses.

I've just noticed that my home server has silently dropped my reply
to you from the 7th of March, so I'm going to resend it using ARM's
setup.

Best regards,
Liviu


> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯