[V2] mtd: devices: docg3:- Handle return value of devm_ioremap.

On Mon, 12 Dec 2016 08:30:04 +0530
Arvind Yadav <arvind.yadav.cs@gmail.com> wrote:

> Here, If devm_ioremap will fail. It will return NULL.
> Kernel can run into a NULL-pointer dereference.
> 
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
>  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---

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 <arvind.yadav.cs@gmail.com> wrote:
>
>> Here, If devm_ioremap will fail. It will return NULL.
>> Kernel can run into a NULL-pointer dereference.
>>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>>   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 <arvind.yadav.cs@gmail.com> 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 <robert.jarzmik@free.fr> wrote:

> arvind Yadav <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com> 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