| Message ID | 1368173847-5661-5-git-send-email-wsa@the-dreams.de |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 05/10/2013 02:16 AM, Wolfram Sang wrote: > devm_ioremap_resource does sanity checks on the given resource. No need to > duplicate this in the driver. > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!res) { > - dev_err(&pdev->dev, "No mem resource for DMA\n"); > - return -EINVAL; > - } > - > tdma->base_addr = devm_ioremap_resource(&pdev->dev, res); One issue here is that it's not obvious just from reading the code that's left behind that the "missing" error-checking of the platform_get_resource() return value is OK because devm_ioremap_resource() will check it "for us". Everyone now has to mentally maintain a list of exceptions where it's OK not to error-check. A similar situation exists for e.g. kzalloc already spewing an error when allocations fail, and so drivers don't need to print a diagnostic in that case, but it's helpful if they do in most other cases, but that's another issue. Would it be better to introduce a new devm_ioremap_pdev_resource(pdev, index) that replaced both those two API calls with a single one. That way, only the author/reader of the new devm_ioremap_pdev_resource() would have to remember that caveat, and a single comment could be added so people unfamiliar with the code remember, without duplicating the comment in every driver. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 10, 2013 at 10:35:32AM -0600, Stephen Warren wrote: > On 05/10/2013 02:16 AM, Wolfram Sang wrote: > > devm_ioremap_resource does sanity checks on the given resource. No need to > > duplicate this in the driver. > > > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > - if (!res) { > > - dev_err(&pdev->dev, "No mem resource for DMA\n"); > > - return -EINVAL; > > - } > > - > > tdma->base_addr = devm_ioremap_resource(&pdev->dev, res); > > One issue here is that it's not obvious just from reading the code > that's left behind that the "missing" error-checking of the > platform_get_resource() return value is OK because > devm_ioremap_resource() will check it "for us". Everyone now has to > mentally maintain a list of exceptions where it's OK not to error-check. My goal is to make not-checking the standard case with devm. > A similar situation exists for e.g. kzalloc already spewing an error > when allocations fail, and so drivers don't need to print a diagnostic > in that case, but it's helpful if they do in most other cases, but > that's another issue. My goal is to make drivers-not-printing-errors the standard case with devm :) > Would it be better to introduce a new devm_ioremap_pdev_resource(pdev, > index) that replaced both those two API calls with a single one. That > way, only the author/reader of the new devm_ioremap_pdev_resource() > would have to remember that caveat, and a single comment could be added > so people unfamiliar with the code remember, without duplicating the > comment in every driver. I see two steps here: 1) make devm interface consistent 2) ease the devm interface with platform drivers I am currently working on the first step. Thanks, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/2013 11:57 AM, Wolfram Sang wrote: > On Fri, May 10, 2013 at 10:35:32AM -0600, Stephen Warren wrote: >> On 05/10/2013 02:16 AM, Wolfram Sang wrote: >>> devm_ioremap_resource does sanity checks on the given resource. No need to >>> duplicate this in the driver. >> >>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c >> >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> - if (!res) { >>> - dev_err(&pdev->dev, "No mem resource for DMA\n"); >>> - return -EINVAL; >>> - } >>> - >>> tdma->base_addr = devm_ioremap_resource(&pdev->dev, res); >> >> One issue here is that it's not obvious just from reading the code >> that's left behind that the "missing" error-checking of the >> platform_get_resource() return value is OK because >> devm_ioremap_resource() will check it "for us". Everyone now has to >> mentally maintain a list of exceptions where it's OK not to error-check. > > My goal is to make not-checking the standard case with devm. OK, if no parameters passed to any devm function every need to be error-checked, that'll certainly be a bit easier to remember. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 10, 2013 at 01:25:36PM -0600, Stephen Warren wrote: > On 05/10/2013 11:57 AM, Wolfram Sang wrote: > > On Fri, May 10, 2013 at 10:35:32AM -0600, Stephen Warren wrote: > >> On 05/10/2013 02:16 AM, Wolfram Sang wrote: > >>> devm_ioremap_resource does sanity checks on the given resource. No need to > >>> duplicate this in the driver. > >> > >>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > >> > >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> - if (!res) { > >>> - dev_err(&pdev->dev, "No mem resource for DMA\n"); > >>> - return -EINVAL; > >>> - } > >>> - > >>> tdma->base_addr = devm_ioremap_resource(&pdev->dev, res); > >> > >> One issue here is that it's not obvious just from reading the code > >> that's left behind that the "missing" error-checking of the > >> platform_get_resource() return value is OK because > >> devm_ioremap_resource() will check it "for us". Everyone now has to > >> mentally maintain a list of exceptions where it's OK not to error-check. > > > > My goal is to make not-checking the standard case with devm. > > OK, if no parameters passed to any devm function every need to be > error-checked, that'll certainly be a bit easier to remember. Okay to remove the log message and move to devm_ but I dont agree with this patch not returning error above. We shouldnt supress the error.. -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 12, 2013 at 09:34:40PM +0530, Vinod Koul wrote: > On Fri, May 10, 2013 at 01:25:36PM -0600, Stephen Warren wrote: > > On 05/10/2013 11:57 AM, Wolfram Sang wrote: > > > On Fri, May 10, 2013 at 10:35:32AM -0600, Stephen Warren wrote: > > >> On 05/10/2013 02:16 AM, Wolfram Sang wrote: > > >>> devm_ioremap_resource does sanity checks on the given resource. No need to > > >>> duplicate this in the driver. > > >> > > >>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > > >> > > >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > >>> - if (!res) { > > >>> - dev_err(&pdev->dev, "No mem resource for DMA\n"); > > >>> - return -EINVAL; > > >>> - } > > >>> - > > >>> tdma->base_addr = devm_ioremap_resource(&pdev->dev, res); > > >> > > >> One issue here is that it's not obvious just from reading the code > > >> that's left behind that the "missing" error-checking of the > > >> platform_get_resource() return value is OK because > > >> devm_ioremap_resource() will check it "for us". Everyone now has to > > >> mentally maintain a list of exceptions where it's OK not to error-check. > > > > > > My goal is to make not-checking the standard case with devm. > > > > OK, if no parameters passed to any devm function every need to be > > error-checked, that'll certainly be a bit easier to remember. > Okay to remove the log message and move to devm_ but I dont agree with this > patch not returning error above. We shouldnt supress the error.. The error will be reported because devm_ioremap_resource will return an ERR_PTR. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 12, 2013 at 08:28:07PM +0200, Wolfram Sang wrote: > On Sun, May 12, 2013 at 09:34:40PM +0530, Vinod Koul wrote: > > On Fri, May 10, 2013 at 01:25:36PM -0600, Stephen Warren wrote: > > > On 05/10/2013 11:57 AM, Wolfram Sang wrote: > > > > On Fri, May 10, 2013 at 10:35:32AM -0600, Stephen Warren wrote: > > > >> On 05/10/2013 02:16 AM, Wolfram Sang wrote: > > > >>> devm_ioremap_resource does sanity checks on the given resource. No need to > > > >>> duplicate this in the driver. > > > >> > > > >>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > > > >> > > > >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > >>> - if (!res) { > > > >>> - dev_err(&pdev->dev, "No mem resource for DMA\n"); > > > >>> - return -EINVAL; > > > >>> - } > > > >>> - > > > >>> tdma->base_addr = devm_ioremap_resource(&pdev->dev, res); > > > >> > > > >> One issue here is that it's not obvious just from reading the code > > > >> that's left behind that the "missing" error-checking of the > > > >> platform_get_resource() return value is OK because > > > >> devm_ioremap_resource() will check it "for us". Everyone now has to > > > >> mentally maintain a list of exceptions where it's OK not to error-check. > > > > > > > > My goal is to make not-checking the standard case with devm. > > > > > > OK, if no parameters passed to any devm function every need to be > > > error-checked, that'll certainly be a bit easier to remember. > > Okay to remove the log message and move to devm_ but I dont agree with this > > patch not returning error above. We shouldnt supress the error.. > > The error will be reported because devm_ioremap_resource will return an > ERR_PTR. And this patch removed the check on 'res' pointer, so on failure we continue... -- ~Vinod > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > >>> devm_ioremap_resource does sanity checks on the given resource. No need to > > > > >>> duplicate this in the driver. > > > > >> > > > > >>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > > > > >> > > > > >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > >>> - if (!res) { > > > > >>> - dev_err(&pdev->dev, "No mem resource for DMA\n"); > > > > >>> - return -EINVAL; > > > > >>> - } > > > > >>> - > > > > >>> tdma->base_addr = devm_ioremap_resource(&pdev->dev, res); > > > > >> > > > > >> One issue here is that it's not obvious just from reading the code > > > > >> that's left behind that the "missing" error-checking of the > > > > >> platform_get_resource() return value is OK because > > > > >> devm_ioremap_resource() will check it "for us". Everyone now has to > > > > >> mentally maintain a list of exceptions where it's OK not to error-check. > > > > > > > > > > My goal is to make not-checking the standard case with devm. > > > > > > > > OK, if no parameters passed to any devm function every need to be > > > > error-checked, that'll certainly be a bit easier to remember. > > > Okay to remove the log message and move to devm_ but I dont agree with this > > > patch not returning error above. We shouldnt supress the error.. > > > > The error will be reported because devm_ioremap_resource will return an > > ERR_PTR. > And this patch removed the check on 'res' pointer, so on failure we continue... I don't get it. What is the difference between a manual check of res and the directly following devm_ioremap_resource returning ERR_PTR(-EINVAL) if res is NULL? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" 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/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c index fcee27e..53e1375 100644 --- a/drivers/dma/tegra20-apb-dma.c +++ b/drivers/dma/tegra20-apb-dma.c @@ -1279,11 +1279,6 @@ static int tegra_dma_probe(struct platform_device *pdev) platform_set_drvdata(pdev, tdma); res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - dev_err(&pdev->dev, "No mem resource for DMA\n"); - return -EINVAL; - } - tdma->base_addr = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(tdma->base_addr)) return PTR_ERR(tdma->base_addr);
devm_ioremap_resource does sanity checks on the given resource. No need to duplicate this in the driver. Signed-off-by: Wolfram Sang <wsa@the-dreams.de> --- drivers/dma/tegra20-apb-dma.c | 5 ----- 1 file changed, 5 deletions(-)