| Message ID | 20200526150954.4729-1-zhengdejin5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] PCI: controller: Remove duplicate error message | expand |
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? Thanks, Jonathan
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. 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); 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); } 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); } return dest_ptr; } BR, Dejin > Thanks, > Jonathan
On Tue, 26 May 2020 23:09:54 +0800, Dejin Zheng wrote: > It will print an error message by itself when > devm_pci_remap_cfg_resource() goes wrong. so remove the duplicate > error message. > > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > --- > drivers/pci/controller/cadence/pcie-cadence-host.c | 4 +--- > drivers/pci/controller/dwc/pcie-al.c | 13 +++---------- > drivers/pci/controller/dwc/pcie-armada8k.c | 1 - > drivers/pci/controller/dwc/pcie-spear13xx.c | 1 - > 4 files changed, 4 insertions(+), 15 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org>
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
On Tue, Jun 2, 2020 at 1:44 AM Chocron, Jonathan <jonnyc@amazon.com> 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
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
On Mon, Jul 06, 2020 at 04:58:47PM +0100, Lorenzo Pieralisi wrote: > On Tue, Jun 02, 2020 at 09:01:13AM -0600, Rob Herring wrote: > > [...] > > > > 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. Hi Lorenzo, Bob and Jonathan: Thank you very much for helping me review this patch, I sent a new patch for print the resource name when the request memory region or remapping of configuration space fails. and it is here: https://patchwork.kernel.org/patch/11657801/ BR, Dejin > > 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
On Sat, Jul 11, 2020 at 03:47:33PM +0800, Dejin Zheng wrote: > On Mon, Jul 06, 2020 at 04:58:47PM +0100, Lorenzo Pieralisi wrote: > > On Tue, Jun 02, 2020 at 09:01:13AM -0600, Rob Herring wrote: > > > > [...] > > > > > > 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. > > Hi Lorenzo, Bob and Jonathan: > > Thank you very much for helping me review this patch, I merge this patch in pci/misc, thanks. > I sent a new patch for print the resource name when the request memory > region or remapping of configuration space fails. and it is here: > https://patchwork.kernel.org/patch/11657801/ We will get to it soon. Lorenzo
On Wed, Jul 15, 2020 at 05:45:59PM +0100, Lorenzo Pieralisi wrote: > On Sat, Jul 11, 2020 at 03:47:33PM +0800, Dejin Zheng wrote: > > On Mon, Jul 06, 2020 at 04:58:47PM +0100, Lorenzo Pieralisi wrote: > > > On Tue, Jun 02, 2020 at 09:01:13AM -0600, Rob Herring wrote: > > > > > > [...] > > > > > > > > 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. > > > > Hi Lorenzo, Bob and Jonathan: > > > > Thank you very much for helping me review this patch, > > I merge this patch in pci/misc, thanks. > > > I sent a new patch for print the resource name when the request memory > > region or remapping of configuration space fails. and it is here: > > https://patchwork.kernel.org/patch/11657801/ > > We will get to it soon. > > Lorenzo Lorenzo, Thanks very much! Dejin
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c index 8c2543f28ba0..60bfb5bcbd37 100644 --- a/drivers/pci/controller/cadence/pcie-cadence-host.c +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c @@ -234,10 +234,8 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg"); rc->cfg_base = devm_pci_remap_cfg_resource(dev, res); - if (IS_ERR(rc->cfg_base)) { - dev_err(dev, "missing \"cfg\"\n"); + if (IS_ERR(rc->cfg_base)) return PTR_ERR(rc->cfg_base); - } rc->cfg_res = res; res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mem"); diff --git a/drivers/pci/controller/dwc/pcie-al.c b/drivers/pci/controller/dwc/pcie-al.c index 270868f3859a..d57d4ee15848 100644 --- a/drivers/pci/controller/dwc/pcie-al.c +++ b/drivers/pci/controller/dwc/pcie-al.c @@ -67,13 +67,8 @@ static int al_pcie_init(struct pci_config_window *cfg) dev_dbg(dev, "Root port dbi res: %pR\n", res); al_pcie->dbi_base = devm_pci_remap_cfg_resource(dev, res); - if (IS_ERR(al_pcie->dbi_base)) { - long err = PTR_ERR(al_pcie->dbi_base); - - dev_err(dev, "couldn't remap dbi base %pR (err:%ld)\n", - res, err); - return err; - } + if (IS_ERR(al_pcie->dbi_base)) + return PTR_ERR(al_pcie->dbi_base); cfg->priv = al_pcie; @@ -408,10 +403,8 @@ static int al_pcie_probe(struct platform_device *pdev) dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res); - if (IS_ERR(pci->dbi_base)) { - dev_err(dev, "couldn't remap dbi base %pR\n", dbi_res); + if (IS_ERR(pci->dbi_base)) return PTR_ERR(pci->dbi_base); - } ecam_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); if (!ecam_res) { diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c index 49596547e8c2..896b95d6917c 100644 --- a/drivers/pci/controller/dwc/pcie-armada8k.c +++ b/drivers/pci/controller/dwc/pcie-armada8k.c @@ -317,7 +317,6 @@ static int armada8k_pcie_probe(struct platform_device *pdev) base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl"); pci->dbi_base = devm_pci_remap_cfg_resource(dev, base); if (IS_ERR(pci->dbi_base)) { - dev_err(dev, "couldn't remap regs base %p\n", base); ret = PTR_ERR(pci->dbi_base); goto fail_clkreg; } diff --git a/drivers/pci/controller/dwc/pcie-spear13xx.c b/drivers/pci/controller/dwc/pcie-spear13xx.c index 7d0cdfd8138b..cdfde1bd7d8e 100644 --- a/drivers/pci/controller/dwc/pcie-spear13xx.c +++ b/drivers/pci/controller/dwc/pcie-spear13xx.c @@ -273,7 +273,6 @@ static int spear13xx_pcie_probe(struct platform_device *pdev) dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi"); pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base); if (IS_ERR(pci->dbi_base)) { - dev_err(dev, "couldn't remap dbi base %p\n", dbi_base); ret = PTR_ERR(pci->dbi_base); goto fail_clk; }
It will print an error message by itself when devm_pci_remap_cfg_resource() goes wrong. so remove the duplicate error message. Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> --- drivers/pci/controller/cadence/pcie-cadence-host.c | 4 +--- drivers/pci/controller/dwc/pcie-al.c | 13 +++---------- drivers/pci/controller/dwc/pcie-armada8k.c | 1 - drivers/pci/controller/dwc/pcie-spear13xx.c | 1 - 4 files changed, 4 insertions(+), 15 deletions(-)