| Message ID | 000101ceeb14$480fee80$d82fcb80$%han@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2013-11-27 at 10:59 +0900, Jingoo Han wrote: [] > ./drivers/pci/host/pci-mvebu.c > static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, > struct device_node *np, struct mvebu_pcie_port *port) > { > ..... > > ret = of_address_to_resource(np, 0, ®s); > if (ret) > - return ERR_PTR(ret); > + return (void __iomem *)ERR_PTR(ret); > > However, other engineers said that "(void __iomem *)ERR_PTR(ret)" > is not a general pattern. I cannot find the proper method to resolve > this sparse warning. > > In this case, how can I resolve this sparse warning? I think there's no problem using the cast. It's not a pattern because it's not been required before as function returns have not previously been declared __iomem. Or, perhaps the arm|hexagon specific #define below could be made generic so it could be used. arch/arm/include/asm/io.h:#define IOMEM(x) ((void __force __iomem *)(x)) return IOMEM(ERR_PTR(ret)); There aren't any current uses of return IOMEM(foo) either though so the direct cast is probably more appropriate.
On Tue, Nov 26, 2013 at 06:26:36PM -0800, Joe Perches wrote: > On Wed, 2013-11-27 at 10:59 +0900, Jingoo Han wrote: > [] > > ./drivers/pci/host/pci-mvebu.c > > static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, > > struct device_node *np, struct mvebu_pcie_port *port) > > { > > ..... > > > > ret = of_address_to_resource(np, 0, ®s); > > if (ret) > > - return ERR_PTR(ret); > > + return (void __iomem *)ERR_PTR(ret); > > > > However, other engineers said that "(void __iomem *)ERR_PTR(ret)" > > is not a general pattern. I cannot find the proper method to resolve > > this sparse warning. > > > > In this case, how can I resolve this sparse warning? > > I think there's no problem using the cast. It's not a > pattern because it's not been required before as function > returns have not previously been declared __iomem. > > Or, perhaps the arm|hexagon specific #define below could > be made generic so it could be used. > > arch/arm/include/asm/io.h:#define IOMEM(x) ((void __force __iomem *)(x)) > > return IOMEM(ERR_PTR(ret)); > > There aren't any current uses of return IOMEM(foo) either > though so the direct cast is probably more appropriate. I don't think that #define is appropriate for any non-constant value; almost any instance of that __force cast applied to a variable would be better written by fixing types more appropriately. I'd suggest updating that #define for both architectures to use BUILD_BUG_ON to assert __builtin_constant_p on its argument. - Josh Triplett
On Tue, 2013-11-26 at 18:35 -0800, Josh Triplett wrote: > On Tue, Nov 26, 2013 at 06:26:36PM -0800, Joe Perches wrote: > > On Wed, 2013-11-27 at 10:59 +0900, Jingoo Han wrote: > > [] > > > ./drivers/pci/host/pci-mvebu.c > > > static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev, > > > struct device_node *np, struct mvebu_pcie_port *port) > > > { > > > ..... > > > > > > ret = of_address_to_resource(np, 0, ®s); > > > if (ret) > > > - return ERR_PTR(ret); > > > + return (void __iomem *)ERR_PTR(ret); > > > > > > However, other engineers said that "(void __iomem *)ERR_PTR(ret)" > > > is not a general pattern. I cannot find the proper method to resolve > > > this sparse warning. > > > > > > In this case, how can I resolve this sparse warning? > > > > I think there's no problem using the cast. It's not a > > pattern because it's not been required before as function > > returns have not previously been declared __iomem. > > > > Or, perhaps the arm|hexagon specific #define below could > > be made generic so it could be used. > > > > arch/arm/include/asm/io.h:#define IOMEM(x) ((void __force __iomem *)(x)) > > > > return IOMEM(ERR_PTR(ret)); > > > > There aren't any current uses of return IOMEM(foo) either > > though so the direct cast is probably more appropriate. > > I don't think that #define is appropriate for any non-constant value; > almost any instance of that __force cast applied to a variable would be > better written by fixing types more appropriately. I'd suggest > updating that #define for both architectures to use BUILD_BUG_ON to > assert __builtin_constant_p on its argument. That's probably a good idea. There are at least a couple things would likely need to be fixed up if the #define were changed. arch/arm/plat-omap/sram.c drivers/clocksource/bcm_kona_timer.c I've no issue with either a direct cast or your idea of a new static inline like ERR_PTR_IO or IOMEM_ERR_PTR or any other appropriate name.
--- 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); } @@ -939,7 +939,7 @@ 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 (IS_ERR_OR_NULL(port->base)) { dev_err(&pdev->dev, "PCIe%d.%d: cannot map registers\n", port->port, port->lane); port->base = NULL;