| Message ID | 1516284037-81537-1-git-send-email-weiyongjun1@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [-next] PCI: dra7xx: Fix potential NULL dereference | expand |
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); This pattern of: platform_get_resource_byname devm_ioremap is extremely common and could perhaps be factored out. There are already a few private versions, e.g., request_and_map(), msm_ioremap(), ssi_get_iomem(). request_and_map() also includes devm_request_mem_region(), which probably *should* be included but usually seems to be omitted. > 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; >
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). ladis > 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
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) > > 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
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
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: That seems to spot another a bit more serious problem (given how late release cycle is now). Both devm_ioremap() and devm_ioremap_resource() shares the same release function: devm_ioremap_release(). However this function is not aware of memory region previously requested by devm_request_mem_region() called from devm_ioremap_resource(). Bellow is just a quick hack, even untested as looking at devm_ioremap, devm_ioremap_wc and devm_ioremap_wc, there is some room for optimization. diff --git a/lib/devres.c b/lib/devres.c index 5f2aedd58bc5..6315b07a608f 100644 --- a/lib/devres.c +++ b/lib/devres.c @@ -10,6 +10,15 @@ void devm_ioremap_release(struct device *dev, void *res) iounmap(*(void __iomem **)res); } +void devm_ioremap_release_region(struct device *dev, void *res) +{ + resource_size_t offset = ((struct resource *)res)->start; + resource_size_t size = resource_size((struct resource *)res); + + iounmap(*(void __iomem **)res); + devm_release_mem_region(dev, offset, size); +} + static int devm_ioremap_match(struct device *dev, void *res, void *match_data) { return *(void **)res == match_data; @@ -136,7 +145,7 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res) { resource_size_t size; const char *name; - void __iomem *dest_ptr; + void __iomem *addr, **ptr; BUG_ON(!dev); @@ -153,14 +162,25 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res) return IOMEM_ERR_PTR(-EBUSY); } - dest_ptr = devm_ioremap(dev, res->start, size); - if (!dest_ptr) { + ptr = devres_alloc(devm_ioremap_release_region, sizeof(*ptr), GFP_KERNEL); + if (!ptr) { + dev_err(dev, "malloc failed for resource %pR\n", res); + devm_release_mem_region(dev, res->start, size); + return IOMEM_ERR_PTR(-ENOMEM); + } + + addr = ioremap(res->start, size); + if (addr) { + *ptr = addr; + devres_add(dev, ptr); + } else { dev_err(dev, "ioremap failed for resource %pR\n", res); devm_release_mem_region(dev, res->start, size); - dest_ptr = IOMEM_ERR_PTR(-ENOMEM); + devres_free(ptr); + addr = IOMEM_ERR_PTR(-ENOMEM); } - return dest_ptr; + return addr; } EXPORT_SYMBOL(devm_ioremap_resource); > --- 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) > > > > 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 Fri, Jan 19, 2018 at 10:58:57AM +0100, Ladislav Michl wrote: > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote: > > That's probably a better idea. Maybe we should add a comment like this > > to help avoid this in the future: > > That seems to spot another a bit more serious problem (given how late > release cycle is now). > > Both devm_ioremap() and devm_ioremap_resource() shares the same release > function: devm_ioremap_release(). However this function is not aware of > memory region previously requested by devm_request_mem_region() called > from devm_ioremap_resource(). > > Bellow is just a quick hack, even untested as looking at devm_ioremap, > devm_ioremap_wc and devm_ioremap_wc, there is some room for optimization. Okay, forget it, above analysis is not correct, however there is a bug (and also in PCI version). To show it, let's make following modification: diff --git a/lib/devres.c b/lib/devres.c index e9aad136f667..193e540eab23 100644 --- a/lib/devres.c +++ b/lib/devres.c @@ -153,6 +153,10 @@ void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res) return IOMEM_ERR_PTR(-EBUSY); } + if (res->start == 0x4809c000 || res->start == 0x480b4000 || res->start == 0x480ad000) { + dev_info(dev, "Setting size to madness\n"); + size = 1000000000; + } dest_ptr = devm_ioremap(dev, res->start, size); if (!dest_ptr) { dev_err(dev, "ioremap failed for resource %pR\n", res); Above patch will set insane resource size for omap_hsmmc driver which is using devm_ioremap_resource() and triggers following error: vmap allocation for size 1000005632 failed: use vmalloc=<size> to increase size omap_hsmmc 4809c000.mmc: ioremap failed for resource [mem 0x4809c000-0x4809c1ff] Trying to free nonexistent resource <000000004809c000-0000000083a489ff> ------------[ cut here ]------------ WARNING: CPU: 0 PID: 92 at kernel/resource.c:1477 __devm_release_region+0x44/0x58 Modules linked in: omap_aes_driver(+) omap_sham(+) crypto_engine omap_crypto phy_twl4030_usb omap2430(+) omap_hsmmc(+) musb_hdrc omap_mailbox ohci_platform(+) snd_soc_twl4030 ohci_hcd ehci_omap td CPU: 0 PID: 92 Comm: systemd-udevd Not tainted 4.15.0-rc8-next-20180118 #42 Hardware name: Generic OMAP36xx (Flattened Device Tree) [<c010d450>] (unwind_backtrace) from [<c010b510>] (show_stack+0x10/0x14) [<c010b510>] (show_stack) from [<c0129648>] (__warn+0xd4/0xec) [<c0129648>] (__warn) from [<c0129720>] (warn_slowpath_null+0x38/0x44) [<c0129720>] (warn_slowpath_null) from [<c012cd4c>] (__devm_release_region+0x44/0x58) [<c012cd4c>] (__devm_release_region) from [<c02b9904>] (devm_ioremap_resource+0x118/0x140) [<c02b9904>] (devm_ioremap_resource) from [<bf0f9f0c>] (omap_hsmmc_probe+0x15c/0x960 [omap_hsmmc]) [<bf0f9f0c>] (omap_hsmmc_probe [omap_hsmmc]) from [<c0339204>] (platform_drv_probe+0x50/0x9c) [<c0339204>] (platform_drv_probe) from [<c0337c04>] (driver_probe_device+0x330/0x478) [<c0337c04>] (driver_probe_device) from [<c0337dec>] (__driver_attach+0xa0/0x104) [<c0337dec>] (__driver_attach) from [<c0335fe8>] (bus_for_each_dev+0x54/0x78) [<c0335fe8>] (bus_for_each_dev) from [<c0336fbc>] (bus_add_driver+0x1b4/0x22c) [<c0336fbc>] (bus_add_driver) from [<c03384bc>] (driver_register+0xa0/0xe0) [<c03384bc>] (driver_register) from [<c0102620>] (do_one_initcall+0x124/0x14c) [<c0102620>] (do_one_initcall) from [<c016d300>] (do_init_module+0x54/0x1c0) [<c016d300>] (do_init_module) from [<c016ca48>] (load_module+0x1e90/0x1fb0) [<c016ca48>] (load_module) from [<c016cd80>] (SyS_finit_module+0xb4/0xc4) [<c016cd80>] (SyS_finit_module) from [<c0101000>] (ret_fast_syscall+0x0/0x54) Exception stack(0xdde2bfa8 to 0xdde2bff0) bfa0: 004f23b0 beb7cdc4 00000007 b6f7e0d8 00000000 004f22f8 bfc0: 004f23b0 beb7cdc4 beb7cdbc 0000017b 004faec8 00000000 00000000 004fd1d0 bfe0: beb7cca8 beb7cc98 b6f77cc5 b6edfba2 ---[ end trace b8768b734ce0c288 ]--- omap_hsmmc: probe of 4809c000.mmc failed with error -12 Please note that "Trying to free nonexistent resource" caused by calling devm_release_mem_region() twice. Fixes will be sent separately. Best regards, ladis
On Fri, Jan 19, 2018 at 06:06:57PM +0100, Ladislav Michl wrote: > On Fri, Jan 19, 2018 at 10:58:57AM +0100, Ladislav Michl wrote: > > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote: > > > That's probably a better idea. Maybe we should add a comment like this > > > to help avoid this in the future: > > > > That seems to spot another a bit more serious problem (given how late > > release cycle is now). > > > > Both devm_ioremap() and devm_ioremap_resource() shares the same release > > function: devm_ioremap_release(). However this function is not aware of > > memory region previously requested by devm_request_mem_region() called > > from devm_ioremap_resource(). > > > > Bellow is just a quick hack, even untested as looking at devm_ioremap, > > devm_ioremap_wc and devm_ioremap_wc, there is some room for optimization. > > Okay, forget it, above analysis is not correct, however there is a bug (and > also in PCI version). To show it, let's make following modification: I will never ever work in single tree for two different boards without full recompile (which should save time and caused opposite) as it makes debugging pointless - there is no bug. As a request forgiveness, please accept following draft as proposed solution for $subj Subject: [PATCH] PCI: dra7xx: Use devm_ioremap_resource() diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c index 8bf7c2714db6..7f422ae258ac 100644 --- a/drivers/pci/dwc/pci-dra7xx.c +++ b/drivers/pci/dwc/pci-dra7xx.c @@ -409,14 +409,14 @@ 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"); - pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res)); - if (!pci->dbi_base) - return -ENOMEM; + pci->dbi_base = devm_ioremap_resource(dev, res); + if (IS_ERR(pci->dbi_base)) + return PTR_ERR(pci->dbi_base); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics2"); - pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res)); - if (!pci->dbi_base2) - return -ENOMEM; + pci->dbi_base2 = devm_ioremap_resource(dev, res); + if (IS_ERR(pci->dbi_base2)) + return PTR_ERR(pci->dbi_base2); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); if (!res)
On Sat, Jan 20, 2018 at 01:16:45AM +0100, Ladislav Michl wrote: > On Fri, Jan 19, 2018 at 06:06:57PM +0100, Ladislav Michl wrote: > > On Fri, Jan 19, 2018 at 10:58:57AM +0100, Ladislav Michl wrote: > > > On Thu, Jan 18, 2018 at 12:35:25PM -0600, Bjorn Helgaas wrote: > > > > That's probably a better idea. Maybe we should add a comment like this > > > > to help avoid this in the future: > > > > > > That seems to spot another a bit more serious problem (given how late > > > release cycle is now). > > > > > > Both devm_ioremap() and devm_ioremap_resource() shares the same release > > > function: devm_ioremap_release(). However this function is not aware of > > > memory region previously requested by devm_request_mem_region() called > > > from devm_ioremap_resource(). > > > > > > Bellow is just a quick hack, even untested as looking at devm_ioremap, > > > devm_ioremap_wc and devm_ioremap_wc, there is some room for optimization. > > > > Okay, forget it, above analysis is not correct, however there is a bug (and > > also in PCI version). To show it, let's make following modification: > > I will never ever work in single tree for two different boards without full > recompile (which should save time and caused opposite) as it makes debugging > pointless - there is no bug. > > As a request forgiveness, please accept following draft as proposed solution > for $subj Wei, Ladislav, getting back to this old thread, I would mark it as "changes requested" and expect someone to post a follow-up patch, I do not think this is a solved problem. Lorenzo > Subject: [PATCH] PCI: dra7xx: Use devm_ioremap_resource() > > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c > index 8bf7c2714db6..7f422ae258ac 100644 > --- a/drivers/pci/dwc/pci-dra7xx.c > +++ b/drivers/pci/dwc/pci-dra7xx.c > @@ -409,14 +409,14 @@ 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"); > - pci->dbi_base = devm_ioremap(dev, res->start, resource_size(res)); > - if (!pci->dbi_base) > - return -ENOMEM; > + pci->dbi_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(pci->dbi_base)) > + return PTR_ERR(pci->dbi_base); > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ep_dbics2"); > - pci->dbi_base2 = devm_ioremap(dev, res->start, resource_size(res)); > - if (!pci->dbi_base2) > - return -ENOMEM; > + pci->dbi_base2 = devm_ioremap_resource(dev, res); > + if (IS_ERR(pci->dbi_base2)) > + return PTR_ERR(pci->dbi_base2); > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space"); > if (!res)
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;
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); 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(+)