| Message ID | 001001ceb816$5d1aecc0$1750c640$%han@samsung.com |
|---|---|
| State | Not Applicable |
| Headers | show |
Dear Jingoo Han, On Mon, 23 Sep 2013 13:35:42 +0900, Jingoo Han wrote: > Return NULL instead of ERR_PTR(ret) in order to fix the following > sparse warning: > > drivers/pci/host/pci-mvebu.c:744:31: warning: incorrect type in return expression (different address spaces) > drivers/pci/host/pci-mvebu.c:744:31: expected void [noderef] <asn:2>* > drivers/pci/host/pci-mvebu.c:744:31: got void * > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
On Sat, Nov 23, 2013 at 10:00:33PM -0500, Jason Cooper wrote: > And a small addendum: I currently have the following in mvebu/drivers > 058100a08be8 PCI: mvebu: return NULL instead of ERR_PTR(ret) Folks, I took a quick look at this, and it looks suspicious (sorry, I can't seem to find the thread to followup post) > PCI: mvebu: return NULL instead of ERR_PTR(ret) > > Return NULL instead of ERR_PTR(ret) in order to fix the following > sparse warning: > > drivers/pci/host/pci-mvebu.c:744:31: warning: incorrect type in return expression (different address > spaces) > drivers/pci/host/pci-mvebu.c:744:31: expected void [noderef] <asn:2>* > drivers/pci/host/pci-mvebu.c:744:31: got void * > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > Signed-off-by: Jason Cooper <jason@lakedaemon.net> > >--- a/drivers/pci/host/pci-mvebu.c >+++ b/drivers/pci/host/pci-mvebu.c >@@ -740,7 +740,7 @@ static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, > > ret = of_address_to_resource(np, 0, ®s); > if (ret) >- return ERR_PTR(ret); >+ return NULL; > > return devm_ioremap_resource(&pdev->dev, ®s); So we drop the ERR_PTR for that return but 'devm_ioremap_resource' returns ERR_PTR too: /** * devm_ioremap_resource() - check, request region, and ioremap resource * @dev: generic device to handle the resource for * @res: resource to be handled * * Checks that a resource is a valid memory region, requests the memory region * and ioremaps it either as cacheable or as non-cacheable memory depending on * the resource's flags. All operations are managed and will be undone on * driver detach. * * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code * on failure. Usage example: * * res = platform_get_resource(pdev, IORESOURCE_MEM, 0); * base = devm_ioremap_resource(&pdev->dev, res); * if (IS_ERR(base)) * return PTR_ERR(base); So this is clearly wrong: > port->base = mvebu_pcie_map_registers(pdev, child, port); >- if (IS_ERR(port->base)) { >+ if (!port->base) { > dev_err(&pdev->dev, NAK I guess? This looks like a sparse problem, doesn't it complain for devm_ioremap_resource too? void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res) { [..] return ERR_PTR(-EBUSY); Regards, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, November 26, 2013 5:03 AM, Jason Gunthorpe wrote: > On Sat, Nov 23, 2013 at 10:00:33PM -0500, Jason Cooper wrote: > > And a small addendum: I currently have the following in mvebu/drivers > > 058100a08be8 PCI: mvebu: return NULL instead of ERR_PTR(ret) > > Folks, I took a quick look at this, and it looks suspicious (sorry, I > can't seem to find the thread to followup post) > > > PCI: mvebu: return NULL instead of ERR_PTR(ret) > > > > Return NULL instead of ERR_PTR(ret) in order to fix the following > > sparse warning: > > > > drivers/pci/host/pci-mvebu.c:744:31: warning: incorrect type in return expression (different > address > > spaces) > > drivers/pci/host/pci-mvebu.c:744:31: expected void [noderef] <asn:2>* > > drivers/pci/host/pci-mvebu.c:744:31: got void * > > > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > > Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Signed-off-by: Jason Cooper <jason@lakedaemon.net> > > > >--- a/drivers/pci/host/pci-mvebu.c > >+++ b/drivers/pci/host/pci-mvebu.c > >@@ -740,7 +740,7 @@ static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, > > > > ret = of_address_to_resource(np, 0, ®s); > > if (ret) > >- return ERR_PTR(ret); > >+ return NULL; > > > > return devm_ioremap_resource(&pdev->dev, ®s); > > So we drop the ERR_PTR for that return but 'devm_ioremap_resource' > returns ERR_PTR too: Yes, you're right. It makes the problem. Thus, this commit "PCI: mvebu: return NULL instead of ERR_PTR(ret)" should be reverted. Previously, I sent the patch in order to fix sparse warning as below: How about this? static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, struct device_node *np, struct mvebu_pcie_port *port) { struct resource regs; int ret = 0; ret = of_address_to_resource(np, 0, ®s); if (ret) - return ERR_PTR(ret); + return (void __iomem *)ERR_PTR(ret); return devm_ioremap_resource(&pdev->dev, ®s); } static int mvebu_pcie_probe(struct platform_device *pdev) { ..... port->base = mvebu_pcie_map_registers(pdev, child, port); if (IS_ERR(port->base)) { dev_err(&pdev->dev, "PCIe%d.%d: cannot map registers\n", port->port, port->lane); port->base = NULL; clk_disable_unprepare(port->clk); continue; } Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 26, 2013 at 02:31:44PM +0900, Jingoo Han wrote: > > On Tuesday, November 26, 2013 5:03 AM, Jason Gunthorpe wrote: > > On Sat, Nov 23, 2013 at 10:00:33PM -0500, Jason Cooper wrote: > > > And a small addendum: I currently have the following in mvebu/drivers > > > 058100a08be8 PCI: mvebu: return NULL instead of ERR_PTR(ret) > > > > Folks, I took a quick look at this, and it looks suspicious (sorry, I > > can't seem to find the thread to followup post) > > > > > PCI: mvebu: return NULL instead of ERR_PTR(ret) > > > > > > Return NULL instead of ERR_PTR(ret) in order to fix the following > > > sparse warning: > > > > > > drivers/pci/host/pci-mvebu.c:744:31: warning: incorrect type in return expression (different > > address > > > spaces) > > > drivers/pci/host/pci-mvebu.c:744:31: expected void [noderef] <asn:2>* > > > drivers/pci/host/pci-mvebu.c:744:31: got void * > > > > > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > > > Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > > Signed-off-by: Jason Cooper <jason@lakedaemon.net> > > > > > >--- a/drivers/pci/host/pci-mvebu.c > > >+++ b/drivers/pci/host/pci-mvebu.c > > >@@ -740,7 +740,7 @@ static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, > > > > > > ret = of_address_to_resource(np, 0, ®s); > > > if (ret) > > >- return ERR_PTR(ret); > > >+ return NULL; > > > > > > return devm_ioremap_resource(&pdev->dev, ®s); > > > > So we drop the ERR_PTR for that return but 'devm_ioremap_resource' > > returns ERR_PTR too: > > Yes, you're right. > It makes the problem. > Thus, this commit "PCI: mvebu: return NULL instead of ERR_PTR(ret)" > should be reverted. It hasn't gone to mainline yet and I haven't sent a pull request for it yet. So I can just drop it. Since we have the discussion on all three of those patches re-ignited, I'll just drop the branch and Ack the resends for going through Bjorn. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 26, 2013 at 02:31:44PM +0900, Jingoo Han wrote: > Previously, I sent the patch in order to fix sparse warning as below: > How about this? > > static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, > struct device_node *np, struct mvebu_pcie_port *port) > { > struct resource regs; > int ret = 0; > > ret = of_address_to_resource(np, 0, ®s); > if (ret) > - return ERR_PTR(ret); > + return (void __iomem *)ERR_PTR(ret); You should probably ask the sparse folks for guidance 'git grep iomem.*ERR_PTR' returns nothing, so this isn't an established pattern. It seems like sparse should know that ERR_PTR functions can work with any pointer no matter the type? IS_ERR_PTR will have the same problem with implicitly dropping the iomem tag. Regards, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index 0bd3ba8..fb2474f 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -741,7 +741,7 @@ static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, ret = of_address_to_resource(np, 0, ®s); if (ret) - return ERR_PTR(ret); + return NULL; return devm_ioremap_resource(&pdev->dev, ®s); } @@ -940,10 +940,9 @@ static int mvebu_pcie_probe(struct platform_device *pdev) continue; port->base = mvebu_pcie_map_registers(pdev, child, port); - if (IS_ERR(port->base)) { + if (!port->base) { dev_err(&pdev->dev, "PCIe%d.%d: cannot map registers\n", port->port, port->lane); - port->base = NULL; clk_disable_unprepare(port->clk); continue; }