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

[Chocron, Jonathan]

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.

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

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

>         return dest_ptr;
> }
> 
> BR,
> Dejin
> 
> > Thanks,
> >    Jonathan