Re: [PATCH v1] PCI: controller: Remove duplicate error message

[Lorenzo Pieralisi]

On Tue, Jun 02, 2020 at 09:01:13AM -0600, Rob Herring wrote:

[...]

> > > In fact, I think its error handling is clear enough, It just goes
> > > wrong
> > > in three places, as follows:
> > >
> > > void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
> > >                                           struct resource *res)
> > > {
> > >         resource_size_t size;
> > >         const char *name;
> > >         void __iomem *dest_ptr;
> > >
> > >         BUG_ON(!dev);
> > >
> > >         if (!res || resource_type(res) != IORESOURCE_MEM) {
> > >                 dev_err(dev, "invalid resource\n");
> > >                 return IOMEM_ERR_PTR(-EINVAL);
> > >         }
> > >
> > In the above error case there is no indication of which resource failed
> > (mainly relevant if the resource name is missing in the devicetree,
> > since in the drivers you are changing platform_get_resource_byname() is
> > mostly used). In the existing drivers' code, on return from this
> > function in this case, the name would be printed by the caller.
> 
> A driver should only have one call to devm_pci_remap_cfg_resource() as
> there's only 1 config space. However, it looks like this function is
> frequently used on what is not config space which is a bigger issue.

That certainly is and should be fixed.

> If this error happens, it's almost always going to be a NULL ptr as
> platform_get_resource_byname() would have set IORESOURCE_MEM. Perhaps
> a WARN here so you get a backtrace to the caller location.

+1

> > >         size = resource_size(res);
> > >         name = res->name ?: dev_name(dev);
> > >
> > >         if (!devm_request_mem_region(dev, res->start, size, name)) {
> > >                 dev_err(dev, "can't request region for resource
> > > %pR\n", res);
> > >                 return IOMEM_ERR_PTR(-EBUSY);
> > >         }
> > >
> > >         dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size);
> > >         if (!dest_ptr) {
> > >                 dev_err(dev, "ioremap failed for resource %pR\n",
> > > res);
> > >                 devm_release_mem_region(dev, res->start, size);
> > >                 dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
> > >         }
> > >
> > The other 2 error cases as well don't print the resource name as far as
> > I recall (they will at least print the resource start/end).
> 
> Start/end are what are important for why either of these functions
> failed.
> 
> But sure, we could add 'name' here. That's a separate patch IMO.

I agree. In sum, I think it is OK to proceed with this patch, provided
we send follow-ups as discussed here, are we in agreement ?

Lorenzo