| Message ID | 20180118183525.GG53542@bhelgaas-glaptop.roam.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote: > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote: > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote: > > > platform_get_resource_byname() may fail and return NULL, so we should > > > better check it's return value to avoid a NULL pointer dereference a > > > bit later in the code. > > > > > > This is detected by Coccinelle semantic patch. > > > > > > @@ > > > expression pdev, res, n, t, e, e1, e2; > > > @@ > > > > > > res = platform_get_resource_byname(pdev, t, n); > > > + if (!res) > > > + return -EINVAL; > > > ... when != res == NULL > > > e = devm_ioremap(e1, res->start, e2); > > > > Well, then it should be replaced with devm_ioremap_resource() > > which already checks for NULL and the right resource type > > (IORESOURCE_MEM). > > That's probably a better idea. Maybe we should add a comment like this > to help avoid this in the future: > > --- a/lib/devres.c > +++ b/lib/devres.c > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev, void *res, void *match_data) > * @size: Size of map > * > * Managed ioremap(). Map is automatically unmapped on driver detach. > + * > + * When possible, use devm_ioremap_resource() instead. > */ > void __iomem *devm_ioremap(struct device *dev, resource_size_t offset, > resource_size_t size) Yes, please. It would be nice first patch in the serie converting existing users of devm_ioremap into devm_ioremap_resource: find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size | wc -l 82 I know, that was dumb, Coccinelle would certainly do better job. And from a quick look a lot of if (!res) { print error return -EINVAL; } code blocks could be deleted (and many cases where check for NULL resource is missing fixed). > > > Fixes: 608793e27b33 ("PCI: dwc: dra7xx: Add EP mode support") > > > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com> > > > --- > > > drivers/pci/dwc/pci-dra7xx.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c > > > index 8bf7c27..aafded8 100644 > > > --- a/drivers/pci/dwc/pci-dra7xx.c > > > +++ b/drivers/pci/dwc/pci-dra7xx.c > > > @@ -409,11 +409,15 @@ static int __init dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx, > > > ep->ops = &pcie_ep_ops; > > > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics"); > > > + if (!res) > > > + return -EINVAL; > > > pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res)); > > > if (!pci->dbi_base) > > > return -ENOMEM; > > > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics2"); > > > + if (!res) > > > + return -EINVAL; > > > pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res)); > > > if (!pci->dbi_base2) > > > return -ENOMEM; > > > @@ -462,6 +466,8 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, > > > return ret; > > > > > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbics"); > > > + if (!res) > > > + return -EINVAL; > > > pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res)); > > > if (!pci->dbi_base) > > > return -ENOMEM; > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote: > > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote: > > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote: > > > > platform_get_resource_byname() may fail and return NULL, so we > should > > > > better check it's return value to avoid a NULL pointer dereference a > > > > bit later in the code. > > > > > > > > This is detected by Coccinelle semantic patch. > > > > > > > > @@ > > > > expression pdev, res, n, t, e, e1, e2; > > > > @@ > > > > > > > > res = platform_get_resource_byname(pdev, t, n); > > > > + if (!res) > > > > + return -EINVAL; > > > > ... when != res == NULL > > > > e = devm_ioremap(e1, res->start, e2); > > > > > > Well, then it should be replaced with devm_ioremap_resource() > > > which already checks for NULL and the right resource type > > > (IORESOURCE_MEM). > > > > That's probably a better idea. Maybe we should add a comment like this > > to help avoid this in the future: Not all of the place using devm_ioremap() can be replaced with devm_ioremap_resource(), devices share the memory resource for example. So maybe you should also add an exception list to the comment, otherwise many people still not know how to use devm_ioremap_resource() or devm_ioremap(). > > > > --- a/lib/devres.c > > +++ b/lib/devres.c > > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev, > void *res, void *match_data) > > * @size: Size of map > > * > > * Managed ioremap(). Map is automatically unmapped on driver detach. > > + * > > + * When possible, use devm_ioremap_resource() instead. > > */ > > void __iomem *devm_ioremap(struct device *dev, resource_size_t offset, > > resource_size_t size) > > Yes, please. It would be nice first patch in the serie converting existing > users of devm_ioremap into devm_ioremap_resource: > find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size > | wc -l > 82 > I know, that was dumb, Coccinelle would certainly do better job. > And from a quick look a lot of > if (!res) { > print error > return -EINVAL; > } > code blocks could be deleted (and many cases where check for NULL > resource > is missing fixed). >
On Fri, 19 Jan 2018, weiyongjun (A) wrote: > > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote: > > > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote: > > > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote: > > > > > platform_get_resource_byname() may fail and return NULL, so we > > should > > > > > better check it's return value to avoid a NULL pointer dereference a > > > > > bit later in the code. > > > > > > > > > > This is detected by Coccinelle semantic patch. > > > > > > > > > > @@ > > > > > expression pdev, res, n, t, e, e1, e2; > > > > > @@ > > > > > > > > > > res = platform_get_resource_byname(pdev, t, n); > > > > > + if (!res) > > > > > + return -EINVAL; > > > > > ... when != res == NULL > > > > > e = devm_ioremap(e1, res->start, e2); > > > > > > > > Well, then it should be replaced with devm_ioremap_resource() > > > > which already checks for NULL and the right resource type > > > > (IORESOURCE_MEM). > > > > > > That's probably a better idea. Maybe we should add a comment like this > > > to help avoid this in the future: > > Not all of the place using devm_ioremap() can be replaced with > devm_ioremap_resource(), devices share the memory resource for example. > > So maybe you should also add an exception list to the comment, otherwise > many people still not know how to use devm_ioremap_resource() or devm_ioremap(). I believe that there is a semantic patch in the kernel to remove the test when devm_ioremap_reource is used. Maybe that should be extended or another one should be added to ensure that there is a test when devm_ioremap is used, since there seems to be a potential for confusion. julia > > > > > > > --- a/lib/devres.c > > > +++ b/lib/devres.c > > > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev, > > void *res, void *match_data) > > > * @size: Size of map > > > * > > > * Managed ioremap(). Map is automatically unmapped on driver detach. > > > + * > > > + * When possible, use devm_ioremap_resource() instead. > > > */ > > > void __iomem *devm_ioremap(struct device *dev, resource_size_t offset, > > > resource_size_t size) > > > > Yes, please. It would be nice first patch in the serie converting existing > > users of devm_ioremap into devm_ioremap_resource: > > find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size > > | wc -l > > 82 > > I know, that was dumb, Coccinelle would certainly do better job. > > And from a quick look a lot of > > if (!res) { > > print error > > return -EINVAL; > > } > > code blocks could be deleted (and many cases where check for NULL > > resource > > is missing fixed). > > > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Fri, Jan 19, 2018 at 01:54:58AM +0000, weiyongjun (A) wrote: > > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote: > > > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote: > > > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote: > > > > > platform_get_resource_byname() may fail and return NULL, so we > > should > > > > > better check it's return value to avoid a NULL pointer dereference a > > > > > bit later in the code. > > > > > > > > > > This is detected by Coccinelle semantic patch. > > > > > > > > > > @@ > > > > > expression pdev, res, n, t, e, e1, e2; > > > > > @@ > > > > > > > > > > res = platform_get_resource_byname(pdev, t, n); > > > > > + if (!res) > > > > > + return -EINVAL; > > > > > ... when != res == NULL > > > > > e = devm_ioremap(e1, res->start, e2); > > > > > > > > Well, then it should be replaced with devm_ioremap_resource() > > > > which already checks for NULL and the right resource type > > > > (IORESOURCE_MEM). > > > > > > That's probably a better idea. Maybe we should add a comment like this > > > to help avoid this in the future: > > Not all of the place using devm_ioremap() can be replaced with > devm_ioremap_resource(), devices share the memory resource for example. That's probably what "when possible" means. Also, how does sharing memory resource changes that? As long as 'struct resource' is an argument to devm_ioremap, devm_ioremap_resource can be used. > So maybe you should also add an exception list to the comment, otherwise > many people still not know how to use devm_ioremap_resource() or devm_ioremap(). Care to elaborate how should such an exception list look like? Thank you. > > > > > > --- a/lib/devres.c > > > +++ b/lib/devres.c > > > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev, > > void *res, void *match_data) > > > * @size: Size of map > > > * > > > * Managed ioremap(). Map is automatically unmapped on driver detach. > > > + * > > > + * When possible, use devm_ioremap_resource() instead. > > > */ > > > void __iomem *devm_ioremap(struct device *dev, resource_size_t offset, > > > resource_size_t size) > > > > Yes, please. It would be nice first patch in the serie converting existing > > users of devm_ioremap into devm_ioremap_resource: > > find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size > > | wc -l > > 82 > > I know, that was dumb, Coccinelle would certainly do better job. > > And from a quick look a lot of > > if (!res) { > > print error > > return -EINVAL; > > } > > code blocks could be deleted (and many cases where check for NULL > > resource > > is missing fixed). > >
On Fri, Jan 19, 2018 at 08:03:38AM +0100, Ladislav Michl wrote: > On Fri, Jan 19, 2018 at 01:54:58AM +0000, weiyongjun (A) wrote: > > > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote: > > > > On Thu, Jan 18, 2018 at 03:54:20PM +0100, Ladislav Michl wrote: > > > > > On Thu, Jan 18, 2018 at 02:00:37PM +0000, Wei Yongjun wrote: > > > > > > platform_get_resource_byname() may fail and return NULL, so we > > > should > > > > > > better check it's return value to avoid a NULL pointer dereference a > > > > > > bit later in the code. > > > > > > > > > > > > This is detected by Coccinelle semantic patch. > > > > > > > > > > > > @@ > > > > > > expression pdev, res, n, t, e, e1, e2; > > > > > > @@ > > > > > > > > > > > > res = platform_get_resource_byname(pdev, t, n); > > > > > > + if (!res) > > > > > > + return -EINVAL; > > > > > > ... when != res == NULL > > > > > > e = devm_ioremap(e1, res->start, e2); > > > > > > > > > > Well, then it should be replaced with devm_ioremap_resource() > > > > > which already checks for NULL and the right resource type > > > > > (IORESOURCE_MEM). > > > > > > > > That's probably a better idea. Maybe we should add a comment like this > > > > to help avoid this in the future: > > > > Not all of the place using devm_ioremap() can be replaced with > > devm_ioremap_resource(), devices share the memory resource for example. > > That's probably what "when possible" means. Also, how does sharing memory > resource changes that? As long as 'struct resource' is an argument to > devm_ioremap, devm_ioremap_resource can be used. > > > So maybe you should also add an exception list to the comment, otherwise > > many people still not know how to use devm_ioremap_resource() or devm_ioremap(). > > Care to elaborate how should such an exception list look like? What about: "When possible (for example when memory region was not already requested), use devm_ioremap_resource() instead."? And here I would propose something like devm_ioremap_resource_norequest() To be honest, such a name looks too long for me, so suggestions welcome :) > Thank you. > > > > > > > > > --- a/lib/devres.c > > > > +++ b/lib/devres.c > > > > @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev, > > > void *res, void *match_data) > > > > * @size: Size of map > > > > * > > > > * Managed ioremap(). Map is automatically unmapped on driver detach. > > > > + * > > > > + * When possible, use devm_ioremap_resource() instead. > > > > */ > > > > void __iomem *devm_ioremap(struct device *dev, resource_size_t offset, > > > > resource_size_t size) > > > > > > Yes, please. It would be nice first patch in the serie converting existing > > > users of devm_ioremap into devm_ioremap_resource: > > > find drivers -name "*.c" | xargs grep "devm_ioremap(" | grep resource_size > > > | wc -l > > > 82 > > > I know, that was dumb, Coccinelle would certainly do better job. > > > And from a quick look a lot of > > > if (!res) { > > > print error > > > return -EINVAL; > > > } > > > code blocks could be deleted (and many cases where check for NULL > > > resource > > > is missing fixed). > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/lib/devres.c +++ b/lib/devres.c @@ -22,6 +22,8 @@ static int devm_ioremap_match(struct device *dev, void *res, void *match_data) * @size: Size of map * * Managed ioremap(). Map is automatically unmapped on driver detach. + * + * When possible, use devm_ioremap_resource() instead. */ void __iomem *devm_ioremap(struct device *dev, resource_size_t offset, resource_size_t size)