Here, If devm_ioremap will fail. It will return NULL.
Kernel can run into a NULL-pointer dereference.
Signed-off-by: Arvind Yadav <[email protected]>
---
drivers/mtd/devices/docg3.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index b833e6c..ffe3db0 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -2083,9 +2083,14 @@ static int __init docg3_probe(struct platform_device *pdev)
dev_err(dev, "No I/O memory resource defined\n");
return ret;
}
- base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
ret = -ENOMEM;
+ base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
+ if (!base) {
+ dev_err(dev, "failed to map I/O memory\n");
+ return ret;
+ }
+
cascade = devm_kzalloc(dev, sizeof(*cascade) * DOC_MAX_NBFLOORS,
GFP_KERNEL);
if (!cascade)
--
2.7.4
On 12/12/2016 04:00 AM, Arvind Yadav wrote:
> Here, If devm_ioremap will fail. It will return NULL.
> Kernel can run into a NULL-pointer dereference.
>
> Signed-off-by: Arvind Yadav <[email protected]>
> ---
> drivers/mtd/devices/docg3.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index b833e6c..ffe3db0 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -2083,9 +2083,14 @@ static int __init docg3_probe(struct platform_device *pdev)
> dev_err(dev, "No I/O memory resource defined\n");
> return ret;
> }
> - base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
>
> ret = -ENOMEM;
> + base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
> + if (!base) {
> + dev_err(dev, "failed to map I/O memory\n");
> + return ret;
Um, return -ENOMEM right away ?
Otherwise,
Acked-by: Marek Vasut <[email protected]>
> + }
> +
> cascade = devm_kzalloc(dev, sizeof(*cascade) * DOC_MAX_NBFLOORS,
> GFP_KERNEL);
> if (!cascade)
>
--
Best regards,
Marek Vasut
On Mon, 12 Dec 2016 08:30:04 +0530
Arvind Yadav <[email protected]> wrote:
> Here, If devm_ioremap will fail. It will return NULL.
> Kernel can run into a NULL-pointer dereference.
>
> Signed-off-by: Arvind Yadav <[email protected]>
> ---
> drivers/mtd/devices/docg3.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index b833e6c..ffe3db0 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -2083,9 +2083,14 @@ static int __init docg3_probe(struct platform_device *pdev)
> dev_err(dev, "No I/O memory resource defined\n");
> return ret;
> }
> - base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
>
> ret = -ENOMEM;
> + base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
> + if (!base) {
> + dev_err(dev, "failed to map I/O memory\n");
> + return ret;
> + }
> +
> cascade = devm_kzalloc(dev, sizeof(*cascade) * DOC_MAX_NBFLOORS,
> GFP_KERNEL);
> if (!cascade)
How about going even further and doing the following?
This way you can drop the test on rres (it's done for you in
devm_ioremap_resource()), and you can directly return the error
returned by devm_ioremap_resource().
--->8---
diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index b833e6cc684c..c59b91734344 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -2077,13 +2077,10 @@ static int __init docg3_probe(struct
platform_device *pdev) int ret, floor;
struct docg3_cascade *cascade;
- ret = -ENXIO;
ress = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!ress) {
- dev_err(dev, "No I/O memory resource defined\n");
- return ret;
- }
- base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
+ base = devm_ioremap_resource(dev, ress);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
ret = -ENOMEM;
cascade = devm_kzalloc(dev, sizeof(*cascade) * DOC_MAX_NBFLOORS,
There is problem, if you will use devm_ioremap_resource instead of
devm_ioremap,
than devm_ioremap_resource will call request_mem_region().
request_mem_region() allows to tell the kernel that this driver is going
to use
this range of I/O addresses, which will prevent other drivers to make an
overlapping call to request_mem_region If other driver want to use same
address
space to access then it will not allow. Means we can not share same
address space
between two driver.
Thanks
-Arvind
On Monday 12 December 2016 02:12 PM, Boris Brezillon wrote:
> On Mon, 12 Dec 2016 08:30:04 +0530
> Arvind Yadav <[email protected]> wrote:
>
>> Here, If devm_ioremap will fail. It will return NULL.
>> Kernel can run into a NULL-pointer dereference.
>>
>> Signed-off-by: Arvind Yadav <[email protected]>
>> ---
>> drivers/mtd/devices/docg3.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
>> index b833e6c..ffe3db0 100644
>> --- a/drivers/mtd/devices/docg3.c
>> +++ b/drivers/mtd/devices/docg3.c
>> @@ -2083,9 +2083,14 @@ static int __init docg3_probe(struct platform_device *pdev)
>> dev_err(dev, "No I/O memory resource defined\n");
>> return ret;
>> }
>> - base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
>>
>> ret = -ENOMEM;
>> + base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
>> + if (!base) {
>> + dev_err(dev, "failed to map I/O memory\n");
>> + return ret;
>> + }
>> +
>> cascade = devm_kzalloc(dev, sizeof(*cascade) * DOC_MAX_NBFLOORS,
>> GFP_KERNEL);
>> if (!cascade)
> How about going even further and doing the following?
> This way you can drop the test on rres (it's done for you in
> devm_ioremap_resource()), and you can directly return the error
> returned by devm_ioremap_resource().
>
> --->8---
> diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
> index b833e6cc684c..c59b91734344 100644
> --- a/drivers/mtd/devices/docg3.c
> +++ b/drivers/mtd/devices/docg3.c
> @@ -2077,13 +2077,10 @@ static int __init docg3_probe(struct
> platform_device *pdev) int ret, floor;
> struct docg3_cascade *cascade;
>
> - ret = -ENXIO;
> ress = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!ress) {
> - dev_err(dev, "No I/O memory resource defined\n");
> - return ret;
> - }
> - base = devm_ioremap(dev, ress->start, DOC_IOSPACE_SIZE);
> + base = devm_ioremap_resource(dev, ress);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
>
> ret = -ENOMEM;
> cascade = devm_kzalloc(dev, sizeof(*cascade) * DOC_MAX_NBFLOORS,
arvind Yadav <[email protected]> writes:
> There is problem, if you will use devm_ioremap_resource instead of devm_ioremap,
> than devm_ioremap_resource will call request_mem_region().
> request_mem_region() allows to tell the kernel that this driver is going to use
> this range of I/O addresses, which will prevent other drivers to make an
> overlapping call to request_mem_region If other driver want to use same address
> space to access then it will not allow. Means we can not share same address
> space
> between two driver.
Hi,
You're right Arvind, and still, it's worth noticing that the docg3 access
semantics imply a "reserved" resource path (see how doc_register_readb() does a
write and how this cannot be shared with another driver).
Therefore I'll be willing to ack a mix of your both patches, the
devm_ioremap_resource() from Boris and the error message from your patch.
Cheers.
--
Robert
On Mon, 12 Dec 2016 17:32:58 +0100
Robert Jarzmik <[email protected]> wrote:
> arvind Yadav <[email protected]> writes:
>
> > There is problem, if you will use devm_ioremap_resource instead of devm_ioremap,
> > than devm_ioremap_resource will call request_mem_region().
> > request_mem_region() allows to tell the kernel that this driver is going to use
> > this range of I/O addresses, which will prevent other drivers to make an
> > overlapping call to request_mem_region If other driver want to use same address
> > space to access then it will not allow. Means we can not share same address
> > space
> > between two driver.
>
> Hi,
>
> You're right Arvind, and still, it's worth noticing that the docg3 access
> semantics imply a "reserved" resource path (see how doc_register_readb() does a
> write and how this cannot be shared with another driver).
>
> Therefore I'll be willing to ack a mix of your both patches, the
> devm_ioremap_resource() from Boris and the error message from your patch.
devm_ioremap_resource() already prints different error messages
depending on the error type [1], no need to duplicate it.
[1]http://lxr.free-electrons.com/source/lib/devres.c#L134
Hi Arvind,
On Mon, 12 Dec 2016 21:33:05 +0530
arvind Yadav <[email protected]> wrote:
> There is problem, if you will use devm_ioremap_resource instead of
> devm_ioremap,
> than devm_ioremap_resource will call request_mem_region().
> request_mem_region() allows to tell the kernel that this driver is going
> to use
> this range of I/O addresses, which will prevent other drivers to make an
> overlapping call to request_mem_region If other driver want to use same
> address
> space to access then it will not allow. Means we can not share same
> address space
> between two driver.
The question is, is it required here? In general, allowing 2 different
drivers from touching the same iomem region is a bad idea, so, if
there's a reason to allow that here, I'd like to know more about it.
Thanks,
Boris
Hi Boris,
Yes, It's possible that two driver can use same iomem region.
For example you can check
commit id - : 33cf75656923ff11d67a937a4f8e9344f58cea77
Here, It's not required.
Thanks
-Arvind
On Monday 12 December 2016 10:34 PM, Boris Brezillon wrote:
> Hi Arvind,
>
> On Mon, 12 Dec 2016 21:33:05 +0530
> arvind Yadav <[email protected]> wrote:
>
>> There is problem, if you will use devm_ioremap_resource instead of
>> devm_ioremap,
>> than devm_ioremap_resource will call request_mem_region().
>> request_mem_region() allows to tell the kernel that this driver is going
>> to use
>> this range of I/O addresses, which will prevent other drivers to make an
>> overlapping call to request_mem_region If other driver want to use same
>> address
>> space to access then it will not allow. Means we can not share same
>> address space
>> between two driver.
> The question is, is it required here? In general, allowing 2 different
> drivers from touching the same iomem region is a bad idea, so, if
> there's a reason to allow that here, I'd like to know more about it.
>
> Thanks,
>
> Boris