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

[Rob Herring]

On Tue, Jun 2, 2020 at 1:44 AM Chocron, Jonathan <jonnyc@xxxxxxxxxx> wrote:
>
> On Wed, 2020-05-27 at 21:20 +0800, Dejin Zheng wrote:
> > CAUTION: This email originated from outside of the organization. Do
> > not click links or open attachments unless you can confirm the sender
> > and know the content is safe.
> >
> >
> >
> > On Tue, May 26, 2020 at 06:22:56PM +0000, Chocron, Jonathan wrote:
> > > On Tue, 2020-05-26 at 23:09 +0800, Dejin Zheng wrote:
> > > > CAUTION: This email originated from outside of the organization.
> > > > Do
> > > > not click links or open attachments unless you can confirm the
> > > > sender
> > > > and know the content is safe.
> > > >
> > > >
> > > >
> > > > It will print an error message by itself when
> > > > devm_pci_remap_cfg_resource() goes wrong. so remove the duplicate
> > > > error message.
> > > >
> > >
> > > It seems like that in the first error case in
> > > devm_pci_remap_cfg_resource(), the print will be less indicative.
> > > Could
> > > you please share an example print log with the duplicate print?
> > >
> >
> > Hi Jonathan:
> >
> > Thank you very much for using your precious time to review my patch.
> >
> Sure, no problem.
>
> > I did not have this log and just found it by review codes. the
> > function
> > of devm_pci_remap_cfg_resource() is designed to handle error messages
> > by
> > itself. and Its recommended usage is as follows in the function
> > description
> >
> >         base = devm_pci_remap_cfg_resource(&pdev->dev, res);
> >         if (IS_ERR(base))
> >                 return PTR_ERR(base);
> >
> I assume that the recommended usage's main intent was to point out that
> IS_ERR() should be used, but this is mainly speculation.

It's generally preferred to print error messages in a called function
rather than in return error handling.

> > 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.

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.

> >         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.

Rob