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
