| Message ID | 000001cf33a6$e0a57040$a1f050c0$%han@samsung.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jingoo Han <jg1.han@samsung.com> Date: Thu, 27 Feb 2014 19:30:04 +0900 > Use devm_ioremap_resource() in order to make the code simpler, > and remove redundant return value check of platform_get_resource() > because the value is checked by devm_ioremap_resource(). > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> I think the code with the NULL check removed is harder for semantic checkers to figure out, because of what will look like a blind dereference via resource_size(mem). Unless all semantic checkers are going to learn the complete semantics of devm_ioremap_resource(), in particular that it will signal an error on a NULL second argument, and then the semantic checker will see the complete control flow in the call site, I think we should keep the NULL check. Maybe recode this as: mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!mem) return -ENXIO; mem_size = resource_size(mem); priv->base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(priv->base)) return PTR_ERR(priv->base); Same goes for your other patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, February 28, 2014 7:07 AM, David Miller wrote: > From: Jingoo Han <jg1.han@samsung.com> > Date: Thu, 27 Feb 2014 19:30:04 +0900 > > > Use devm_ioremap_resource() in order to make the code simpler, > > and remove redundant return value check of platform_get_resource() > > because the value is checked by devm_ioremap_resource(). > > > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > > I think the code with the NULL check removed is harder for semantic > checkers to figure out, because of what will look like a blind > dereference via resource_size(mem). > > Unless all semantic checkers are going to learn the complete semantics > of devm_ioremap_resource(), in particular that it will signal an error > on a NULL second argument, and then the semantic checker will see > the complete control flow in the call site, I think we should keep > the NULL check. OK, I see. I agree with your opinion. I will send v2 patch, soon. Thank you for your comment. Best regards, Jingoo Han > > Maybe recode this as: > > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!mem) > return -ENXIO; > mem_size = resource_size(mem); > > priv->base = devm_ioremap_resource(&pdev->dev, mem); > if (IS_ERR(priv->base)) > return PTR_ERR(priv->base); > > Same goes for your other patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/net/ethernet/wiznet/w5300.c b/drivers/net/ethernet/wiznet/w5300.c index 71c27b3..1c3a5a5 100644 --- a/drivers/net/ethernet/wiznet/w5300.c +++ b/drivers/net/ethernet/wiznet/w5300.c @@ -558,14 +558,11 @@ static int w5300_hw_probe(struct platform_device *pdev) } mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!mem) - return -ENXIO; + priv->base = devm_ioremap_resource(&pdev->dev, mem); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); + mem_size = resource_size(mem); - if (!devm_request_mem_region(&pdev->dev, mem->start, mem_size, name)) - return -EBUSY; - priv->base = devm_ioremap(&pdev->dev, mem->start, mem_size); - if (!priv->base) - return -EBUSY; spin_lock_init(&priv->reg_lock); priv->indirect = mem_size < W5300_BUS_DIRECT_SIZE;
Use devm_ioremap_resource() in order to make the code simpler, and remove redundant return value check of platform_get_resource() because the value is checked by devm_ioremap_resource(). Signed-off-by: Jingoo Han <jg1.han@samsung.com> --- drivers/net/ethernet/wiznet/w5300.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)