[PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Wei Yongjun]

platform_get_resource() may return NULL, add proper check to
avoid potential NULL dereferencing.

This is detected by Coccinelle semantic patch.

@@
expression pdev, res, n, t, e, e1, e2;
@@

res = platform_get_resource(pdev, t, n);
+ if (!res)
+   return -EINVAL;
... when != res == NULL
e = devm_ioremap(e1, res->start, e2);

Fixes: 4f16f7ff3bc0 ("clk: hisilicon: Add support for Hi3660 stub clocks")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 drivers/clk/hisilicon/clk-hi3660-stub.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
index 9b6c72b..e8b2c43 100644
--- a/drivers/clk/hisilicon/clk-hi3660-stub.c
+++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
@@ -149,6 +149,8 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
 		return PTR_ERR(stub_clk_chan.mbox);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
 	freq_reg = devm_ioremap(dev, res->start, resource_size(res));
 	if (!freq_reg)
 		return -ENOMEM;

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Masahiro Yamada]

2018-01-04 15:36 GMT+09:00 Wei Yongjun <weiyongjun1@huawei.com>:
> platform_get_resource() may return NULL, add proper check to
> avoid potential NULL dereferencing.
>
> This is detected by Coccinelle semantic patch.
>
> @@
> expression pdev, res, n, t, e, e1, e2;
> @@
>
> res = platform_get_resource(pdev, t, n);
> + if (!res)
> +   return -EINVAL;
> ... when != res == NULL
> e = devm_ioremap(e1, res->start, e2);
>
> Fixes: 4f16f7ff3bc0 ("clk: hisilicon: Add support for Hi3660 stub clocks")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
>  drivers/clk/hisilicon/clk-hi3660-stub.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
> index 9b6c72b..e8b2c43 100644
> --- a/drivers/clk/hisilicon/clk-hi3660-stub.c
> +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
> @@ -149,6 +149,8 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
>                 return PTR_ERR(stub_clk_chan.mbox);
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -EINVAL;
>         freq_reg = devm_ioremap(dev, res->start, resource_size(res));
>         if (!freq_reg)
>                 return -ENOMEM;


Probably, it is better to use devm_ioremap_resource().
This checks NULL input.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Leo Yan]

On Fri, Jan 05, 2018 at 12:59:38PM +0900, Masahiro Yamada wrote:
> 2018-01-04 15:36 GMT+09:00 Wei Yongjun <weiyongjun1@huawei.com>:
> > platform_get_resource() may return NULL, add proper check to
> > avoid potential NULL dereferencing.
> >
> > This is detected by Coccinelle semantic patch.
> >
> > @@
> > expression pdev, res, n, t, e, e1, e2;
> > @@
> >
> > res = platform_get_resource(pdev, t, n);
> > + if (!res)
> > +   return -EINVAL;
> > ... when != res == NULL
> > e = devm_ioremap(e1, res->start, e2);
> >
> > Fixes: 4f16f7ff3bc0 ("clk: hisilicon: Add support for Hi3660 stub clocks")
> > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> > ---
> >  drivers/clk/hisilicon/clk-hi3660-stub.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
> > index 9b6c72b..e8b2c43 100644
> > --- a/drivers/clk/hisilicon/clk-hi3660-stub.c
> > +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
> > @@ -149,6 +149,8 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
> >                 return PTR_ERR(stub_clk_chan.mbox);
> >
> >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!res)
> > +               return -EINVAL;
> >         freq_reg = devm_ioremap(dev, res->start, resource_size(res));
> >         if (!freq_reg)
> >                 return -ENOMEM;
> 
> 
> Probably, it is better to use devm_ioremap_resource().
> This checks NULL input.

Thanks for suggestion, Masahiro. This is good suggestion.

Yongjun, could you spin new patch by using devm_ioremap_resource()
to replace devm_ioremap()?

Thanks,
Leo Yan

RE: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[weiyongjun (A)]

PiBPbiBGcmksIEphbiAwNSwgMjAxOCBhdCAxMjo1OTozOFBNICswOTAwLCBNYXNhaGlybyBZYW1h
ZGEgd3JvdGU6DQo+ID4gMjAxOC0wMS0wNCAxNTozNiBHTVQrMDk6MDAgV2VpIFlvbmdqdW4gPHdl
aXlvbmdqdW4xQGh1YXdlaS5jb20+Og0KPiA+ID4gcGxhdGZvcm1fZ2V0X3Jlc291cmNlKCkgbWF5
IHJldHVybiBOVUxMLCBhZGQgcHJvcGVyIGNoZWNrIHRvDQo+ID4gPiBhdm9pZCBwb3RlbnRpYWwg
TlVMTCBkZXJlZmVyZW5jaW5nLg0KPiA+ID4NCj4gPiA+IFRoaXMgaXMgZGV0ZWN0ZWQgYnkgQ29j
Y2luZWxsZSBzZW1hbnRpYyBwYXRjaC4NCj4gPiA+DQo+ID4gPiBAQA0KPiA+ID4gZXhwcmVzc2lv
biBwZGV2LCByZXMsIG4sIHQsIGUsIGUxLCBlMjsNCj4gPiA+IEBADQo+ID4gPg0KPiA+ID4gcmVz
ID0gcGxhdGZvcm1fZ2V0X3Jlc291cmNlKHBkZXYsIHQsIG4pOw0KPiA+ID4gKyBpZiAoIXJlcykN
Cj4gPiA+ICsgICByZXR1cm4gLUVJTlZBTDsNCj4gPiA+IC4uLiB3aGVuICE9IHJlcyA9PSBOVUxM
DQo+ID4gPiBlID0gZGV2bV9pb3JlbWFwKGUxLCByZXMtPnN0YXJ0LCBlMik7DQo+ID4gPg0KPiA+
ID4gRml4ZXM6IDRmMTZmN2ZmM2JjMCAoImNsazogaGlzaWxpY29uOiBBZGQgc3VwcG9ydCBmb3Ig
SGkzNjYwIHN0dWIgY2xvY2tzIikNCj4gPiA+IFNpZ25lZC1vZmYtYnk6IFdlaSBZb25nanVuIDx3
ZWl5b25nanVuMUBodWF3ZWkuY29tPg0KPiA+ID4gLS0tDQo+ID4gPiAgZHJpdmVycy9jbGsvaGlz
aWxpY29uL2Nsay1oaTM2NjAtc3R1Yi5jIHwgMiArKw0KPiA+ID4gIDEgZmlsZSBjaGFuZ2VkLCAy
IGluc2VydGlvbnMoKykNCj4gPiA+DQo+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9jbGsvaGlz
aWxpY29uL2Nsay1oaTM2NjAtc3R1Yi5jDQo+IGIvZHJpdmVycy9jbGsvaGlzaWxpY29uL2Nsay1o
aTM2NjAtc3R1Yi5jDQo+ID4gPiBpbmRleCA5YjZjNzJiLi5lOGIyYzQzIDEwMDY0NA0KPiA+ID4g
LS0tIGEvZHJpdmVycy9jbGsvaGlzaWxpY29uL2Nsay1oaTM2NjAtc3R1Yi5jDQo+ID4gPiArKysg
Yi9kcml2ZXJzL2Nsay9oaXNpbGljb24vY2xrLWhpMzY2MC1zdHViLmMNCj4gPiA+IEBAIC0xNDks
NiArMTQ5LDggQEAgc3RhdGljIGludCBoaTM2NjBfc3R1Yl9jbGtfcHJvYmUoc3RydWN0DQo+IHBs
YXRmb3JtX2RldmljZSAqcGRldikNCj4gPiA+ICAgICAgICAgICAgICAgICByZXR1cm4gUFRSX0VS
UihzdHViX2Nsa19jaGFuLm1ib3gpOw0KPiA+ID4NCj4gPiA+ICAgICAgICAgcmVzID0gcGxhdGZv
cm1fZ2V0X3Jlc291cmNlKHBkZXYsIElPUkVTT1VSQ0VfTUVNLCAwKTsNCj4gPiA+ICsgICAgICAg
aWYgKCFyZXMpDQo+ID4gPiArICAgICAgICAgICAgICAgcmV0dXJuIC1FSU5WQUw7DQo+ID4gPiAg
ICAgICAgIGZyZXFfcmVnID0gZGV2bV9pb3JlbWFwKGRldiwgcmVzLT5zdGFydCwgcmVzb3VyY2Vf
c2l6ZShyZXMpKTsNCj4gPiA+ICAgICAgICAgaWYgKCFmcmVxX3JlZykNCj4gPiA+ICAgICAgICAg
ICAgICAgICByZXR1cm4gLUVOT01FTTsNCj4gPg0KPiA+DQo+ID4gUHJvYmFibHksIGl0IGlzIGJl
dHRlciB0byB1c2UgZGV2bV9pb3JlbWFwX3Jlc291cmNlKCkuDQo+ID4gVGhpcyBjaGVja3MgTlVM
TCBpbnB1dC4NCj4gDQo+IFRoYW5rcyBmb3Igc3VnZ2VzdGlvbiwgTWFzYWhpcm8uIFRoaXMgaXMg
Z29vZCBzdWdnZXN0aW9uLg0KPiANCj4gWW9uZ2p1biwgY291bGQgeW91IHNwaW4gbmV3IHBhdGNo
IGJ5IHVzaW5nIGRldm1faW9yZW1hcF9yZXNvdXJjZSgpDQo+IHRvIHJlcGxhY2UgZGV2bV9pb3Jl
bWFwKCk/DQoNCg0KSSB3aWxsIHNlbmQgdjIgcGF0Y2ggdXNpbmcgZGV2bV9pb3JlbWFwX3Jlc291
cmNlLiBBbmQgd291bGQgcGxlYXNlIHRlc3QgaXQNCnNpbmNlIHNvbWV0aW1lcyBjaGFuZ2UgZGV2
bV9pb3JlbWFwKCkgdG8gZGV2bV9pb3JlbWFwX3Jlc291cmNlKCkgbWF5DQpjYXVzZSBkZXZpY2Ug
Y2Fubm90IHdvcms/DQoNClJlZ2FyZHMsDQpXZWkgWW9uZ2p1bg0K

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Wangtao (Kevin, Kirin)]

在 2018/1/5 16:40, Leo Yan 写道:
> On Fri, Jan 05, 2018 at 12:59:38PM +0900, Masahiro Yamada wrote:
>> 2018-01-04 15:36 GMT+09:00 Wei Yongjun <weiyongjun1@huawei.com>:
>>> platform_get_resource() may return NULL, add proper check to
>>> avoid potential NULL dereferencing.
>>>
>>> This is detected by Coccinelle semantic patch.
>>>
>>> @@
>>> expression pdev, res, n, t, e, e1, e2;
>>> @@
>>>
>>> res = platform_get_resource(pdev, t, n);
>>> + if (!res)
>>> +   return -EINVAL;
>>> ... when != res == NULL
>>> e = devm_ioremap(e1, res->start, e2);
>>>
>>> Fixes: 4f16f7ff3bc0 ("clk: hisilicon: Add support for Hi3660 stub clocks")
>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>> ---
>>>   drivers/clk/hisilicon/clk-hi3660-stub.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
>>> index 9b6c72b..e8b2c43 100644
>>> --- a/drivers/clk/hisilicon/clk-hi3660-stub.c
>>> +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
>>> @@ -149,6 +149,8 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
>>>                  return PTR_ERR(stub_clk_chan.mbox);
>>>
>>>          res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +       if (!res)
>>> +               return -EINVAL;
>>>          freq_reg = devm_ioremap(dev, res->start, resource_size(res));
>>>          if (!freq_reg)
>>>                  return -ENOMEM;
>>
>>
>> Probably, it is better to use devm_ioremap_resource().
>> This checks NULL input.
> 
> Thanks for suggestion, Masahiro. This is good suggestion.
> 
> Yongjun, could you spin new patch by using devm_ioremap_resource()
> to replace devm_ioremap()?
clk-stub's memory region has intersection with mailbox, so we can not use
devm_ioremap_resource here, it will cause problem when mailbox driver be
accepted by mainline.
> 
> Thanks,
> Leo Yan
> 
> .
> 

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Masahiro Yamada]

2018-01-05 18:47 GMT+09:00 Wangtao (Kevin, Kirin) <kevin.wangtao@hisilicon.=
com>:
>
>
> =E5=9C=A8 2018/1/5 16:40, Leo Yan =E5=86=99=E9=81=93:
>>
>> On Fri, Jan 05, 2018 at 12:59:38PM +0900, Masahiro Yamada wrote:
>>>
>>> 2018-01-04 15:36 GMT+09:00 Wei Yongjun <weiyongjun1@huawei.com>:
>>>>
>>>> platform_get_resource() may return NULL, add proper check to
>>>> avoid potential NULL dereferencing.
>>>>
>>>> This is detected by Coccinelle semantic patch.
>>>>
>>>> @@
>>>> expression pdev, res, n, t, e, e1, e2;
>>>> @@
>>>>
>>>> res =3D platform_get_resource(pdev, t, n);
>>>> + if (!res)
>>>> +   return -EINVAL;
>>>> ... when !=3D res =3D=3D NULL
>>>> e =3D devm_ioremap(e1, res->start, e2);
>>>>
>>>> Fixes: 4f16f7ff3bc0 ("clk: hisilicon: Add support for Hi3660 stub
>>>> clocks")
>>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>>> ---
>>>>   drivers/clk/hisilicon/clk-hi3660-stub.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c
>>>> b/drivers/clk/hisilicon/clk-hi3660-stub.c
>>>> index 9b6c72b..e8b2c43 100644
>>>> --- a/drivers/clk/hisilicon/clk-hi3660-stub.c
>>>> +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
>>>> @@ -149,6 +149,8 @@ static int hi3660_stub_clk_probe(struct
>>>> platform_device *pdev)
>>>>                  return PTR_ERR(stub_clk_chan.mbox);
>>>>
>>>>          res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +       if (!res)
>>>> +               return -EINVAL;
>>>>          freq_reg =3D devm_ioremap(dev, res->start, resource_size(res)=
);
>>>>          if (!freq_reg)
>>>>                  return -ENOMEM;
>>>
>>>
>>>
>>> Probably, it is better to use devm_ioremap_resource().
>>> This checks NULL input.
>>
>>
>> Thanks for suggestion, Masahiro. This is good suggestion.
>>
>> Yongjun, could you spin new patch by using devm_ioremap_resource()
>> to replace devm_ioremap()?
>
> clk-stub's memory region has intersection with mailbox, so we can not use
> devm_ioremap_resource here, it will cause problem when mailbox driver be
> accepted by mainline.

Hmm, that sounds odd.

syscon in this case?




--=20
Best Regards
Masahiro Yamada

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Wangtao (Kevin, Kirin)]

在 2018/1/5 17:50, Masahiro Yamada 写道:
> 2018-01-05 18:47 GMT+09:00 Wangtao (Kevin, Kirin) <kevin.wangtao@hisilicon.com>:
>>
>>
>> 在 2018/1/5 16:40, Leo Yan 写道:
>>>
>>> On Fri, Jan 05, 2018 at 12:59:38PM +0900, Masahiro Yamada wrote:
>>>>
>>>> 2018-01-04 15:36 GMT+09:00 Wei Yongjun <weiyongjun1@huawei.com>:
>>>>>
>>>>> platform_get_resource() may return NULL, add proper check to
>>>>> avoid potential NULL dereferencing.
>>>>>
>>>>> This is detected by Coccinelle semantic patch.
>>>>>
>>>>> @@
>>>>> expression pdev, res, n, t, e, e1, e2;
>>>>> @@
>>>>>
>>>>> res = platform_get_resource(pdev, t, n);
>>>>> + if (!res)
>>>>> +   return -EINVAL;
>>>>> ... when != res == NULL
>>>>> e = devm_ioremap(e1, res->start, e2);
>>>>>
>>>>> Fixes: 4f16f7ff3bc0 ("clk: hisilicon: Add support for Hi3660 stub
>>>>> clocks")
>>>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>>>> ---
>>>>>    drivers/clk/hisilicon/clk-hi3660-stub.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c
>>>>> b/drivers/clk/hisilicon/clk-hi3660-stub.c
>>>>> index 9b6c72b..e8b2c43 100644
>>>>> --- a/drivers/clk/hisilicon/clk-hi3660-stub.c
>>>>> +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
>>>>> @@ -149,6 +149,8 @@ static int hi3660_stub_clk_probe(struct
>>>>> platform_device *pdev)
>>>>>                   return PTR_ERR(stub_clk_chan.mbox);
>>>>>
>>>>>           res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> +       if (!res)
>>>>> +               return -EINVAL;
>>>>>           freq_reg = devm_ioremap(dev, res->start, resource_size(res));
>>>>>           if (!freq_reg)
>>>>>                   return -ENOMEM;
>>>>
>>>>
>>>>
>>>> Probably, it is better to use devm_ioremap_resource().
>>>> This checks NULL input.
>>>
>>>
>>> Thanks for suggestion, Masahiro. This is good suggestion.
>>>
>>> Yongjun, could you spin new patch by using devm_ioremap_resource()
>>> to replace devm_ioremap()?
>>
>> clk-stub's memory region has intersection with mailbox, so we can not use
>> devm_ioremap_resource here, it will cause problem when mailbox driver be
>> accepted by mainline.
> 
> Hmm, that sounds odd.
> 
> syscon in this case?
actually it belongs to mailbox, but we use part of it as shared memory
> 
> 
> 
> 

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Leo Yan]

On Fri, Jan 05, 2018 at 06:50:21PM +0900, Masahiro Yamada wrote:
> 2018-01-05 18:47 GMT+09:00 Wangtao (Kevin, Kirin) <kevin.wangtao@hisilicon.com>:
> >
> >
> > 在 2018/1/5 16:40, Leo Yan 写道:
> >>
> >> On Fri, Jan 05, 2018 at 12:59:38PM +0900, Masahiro Yamada wrote:
> >>>
> >>> 2018-01-04 15:36 GMT+09:00 Wei Yongjun <weiyongjun1@huawei.com>:
> >>>>
> >>>> platform_get_resource() may return NULL, add proper check to
> >>>> avoid potential NULL dereferencing.
> >>>>
> >>>> This is detected by Coccinelle semantic patch.
> >>>>
> >>>> @@
> >>>> expression pdev, res, n, t, e, e1, e2;
> >>>> @@
> >>>>
> >>>> res = platform_get_resource(pdev, t, n);
> >>>> + if (!res)
> >>>> +   return -EINVAL;
> >>>> ... when != res == NULL
> >>>> e = devm_ioremap(e1, res->start, e2);
> >>>>
> >>>> Fixes: 4f16f7ff3bc0 ("clk: hisilicon: Add support for Hi3660 stub
> >>>> clocks")
> >>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> >>>> ---
> >>>>   drivers/clk/hisilicon/clk-hi3660-stub.c | 2 ++
> >>>>   1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c
> >>>> b/drivers/clk/hisilicon/clk-hi3660-stub.c
> >>>> index 9b6c72b..e8b2c43 100644
> >>>> --- a/drivers/clk/hisilicon/clk-hi3660-stub.c
> >>>> +++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
> >>>> @@ -149,6 +149,8 @@ static int hi3660_stub_clk_probe(struct
> >>>> platform_device *pdev)
> >>>>                  return PTR_ERR(stub_clk_chan.mbox);
> >>>>
> >>>>          res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>> +       if (!res)
> >>>> +               return -EINVAL;
> >>>>          freq_reg = devm_ioremap(dev, res->start, resource_size(res));
> >>>>          if (!freq_reg)
> >>>>                  return -ENOMEM;
> >>>
> >>>
> >>> Probably, it is better to use devm_ioremap_resource().
> >>> This checks NULL input.
> >>
> >>
> >> Thanks for suggestion, Masahiro. This is good suggestion.
> >>
> >> Yongjun, could you spin new patch by using devm_ioremap_resource()
> >> to replace devm_ioremap()?
> >
> > clk-stub's memory region has intersection with mailbox, so we can not use
> > devm_ioremap_resource here, it will cause problem when mailbox driver be
> > accepted by mainline.
> 
> Hmm, that sounds odd.
> 
> syscon in this case?

I verified Yongjun patch v2 for using devm_ioremap_resource(), just as
Kevin mentioned after using devm_ioremap_resource() the kernel has
memory mapping conflict between stub clock driver and mailbox driver.

Following Masahiro suggestion, I changed the stub clock driver to use
syscon & regmap to access the memory, I can confirm this can work
well, you could review in below change.

But this change should note two things, one thing is it is dependent on
mailbox driver upstreaming, so I prefer we can commit one patch for this
change along with mailbox driver patch set; the second thing is we
need change DT binding document.

If you have any better solution for this, also very welcome.

Thanks,
Leo Yan

---8<---

diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
index e8dde4f..40ee63b 100644
--- a/drivers/clk/hisilicon/clk-hi3660-stub.c
+++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
@@ -25,12 +25,14 @@
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/mailbox_client.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <dt-bindings/clock/hi3660-clock.h>
 
-#define HI3660_STUB_CLOCK_DATA		(0x70)
+#define HI3660_STUB_CLOCK_DATA		(0x570)
 #define MHZ				(1000 * 1000)
 
 #define DEFINE_CLK_STUB(_id, _cmd, _name)			\
@@ -60,7 +62,7 @@ struct hi3660_stub_clk {
 	unsigned int rate;
 };
 
-static void __iomem *freq_reg;
+static struct regmap *freq_reg;
 static struct hi3660_stub_clk_chan stub_clk_chan;
 
 static unsigned long hi3660_stub_clk_recalc_rate(struct clk_hw *hw,
@@ -72,7 +74,9 @@ static unsigned long hi3660_stub_clk_recalc_rate(struct clk_hw *hw,
 	 * LPM3 writes back the CPU frequency in shared SRAM so read
 	 * back the frequency.
 	 */
-	stub_clk->rate = readl(freq_reg + (stub_clk->id << 2)) * MHZ;
+	regmap_read(freq_reg, HI3660_STUB_CLOCK_DATA + (stub_clk->id << 2),
+		    &stub_clk->rate);
+	stub_clk->rate *= MHZ;
 	return stub_clk->rate;
 }
 
@@ -133,7 +137,7 @@ static struct clk_hw *hi3660_stub_clk_hw_get(struct of_phandle_args *clkspec,
 static int hi3660_stub_clk_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct resource *res;
+	struct device_node *np = pdev->dev.of_node;
 	unsigned int i;
 	int ret;
 
@@ -148,12 +152,12 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
 	if (IS_ERR(stub_clk_chan.mbox))
 		return PTR_ERR(stub_clk_chan.mbox);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	freq_reg = devm_ioremap_resource(dev, res);
-	if (IS_ERR(freq_reg))
+	freq_reg = syscon_regmap_lookup_by_phandle(np,
+				"hisilicon,hi3660-clk-sram");
+	if (IS_ERR(freq_reg)) {
+		dev_err(dev, "failed to get sram regmap\n");
 		return PTR_ERR(freq_reg);
-
-	freq_reg += HI3660_STUB_CLOCK_DATA;
+	}
 
 	for (i = 0; i < HI3660_CLK_STUB_NUM; i++) {
 		ret = devm_clk_hw_register(&pdev->dev, &hi3660_stub_clks[i].hw);

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Stephen Boyd]

On 01/08, Leo Yan wrote:
> 
> I verified Yongjun patch v2 for using devm_ioremap_resource(), just as
> Kevin mentioned after using devm_ioremap_resource() the kernel has
> memory mapping conflict between stub clock driver and mailbox driver.
> 
> Following Masahiro suggestion, I changed the stub clock driver to use
> syscon & regmap to access the memory, I can confirm this can work
> well, you could review in below change.
> 
> But this change should note two things, one thing is it is dependent on
> mailbox driver upstreaming, so I prefer we can commit one patch for this
> change along with mailbox driver patch set; the second thing is we
> need change DT binding document.
> 
> If you have any better solution for this, also very welcome.
> 

So I'll apply the original patch it sounds like.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Leo Yan]

On Wed, Jan 10, 2018 at 02:48:20PM -0800, Stephen Boyd wrote:
> On 01/08, Leo Yan wrote:
> > 
> > I verified Yongjun patch v2 for using devm_ioremap_resource(), just as
> > Kevin mentioned after using devm_ioremap_resource() the kernel has
> > memory mapping conflict between stub clock driver and mailbox driver.
> > 
> > Following Masahiro suggestion, I changed the stub clock driver to use
> > syscon & regmap to access the memory, I can confirm this can work
> > well, you could review in below change.
> > 
> > But this change should note two things, one thing is it is dependent on
> > mailbox driver upstreaming, so I prefer we can commit one patch for this
> > change along with mailbox driver patch set; the second thing is we
> > need change DT binding document.
> > 
> > If you have any better solution for this, also very welcome.
> > 
> 
> So I'll apply the original patch it sounds like.

Thanks for this and it's good for me, this is simple way to fix the
NULL checking issue.

Thanks,
Leo Yan

[PATCH -next v2] clk: hisilicon: Using devm_ioremap_resource() instead of devm_ioremap()

[Wei Yongjun]

Using devm_ioremap_resource() which can handle NULL res instead of
devm_ioremap().

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
v1 -> v2: using devm_ioremap_resource instead of devm_ioremap
---
 drivers/clk/hisilicon/clk-hi3660-stub.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/hisilicon/clk-hi3660-stub.c b/drivers/clk/hisilicon/clk-hi3660-stub.c
index 9b6c72b..e8dde4f 100644
--- a/drivers/clk/hisilicon/clk-hi3660-stub.c
+++ b/drivers/clk/hisilicon/clk-hi3660-stub.c
@@ -149,9 +149,9 @@ static int hi3660_stub_clk_probe(struct platform_device *pdev)
 		return PTR_ERR(stub_clk_chan.mbox);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	freq_reg = devm_ioremap(dev, res->start, resource_size(res));
-	if (!freq_reg)
-		return -ENOMEM;
+	freq_reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(freq_reg))
+		return PTR_ERR(freq_reg);
 
 	freq_reg += HI3660_STUB_CLOCK_DATA;

Re: [PATCH -next] clk: hisilicon: hi3660：Fix potential NULL dereference in hi3660_stub_clk_probe()

[Stephen Boyd]

Quoting Wei Yongjun (2018-01-03 22:36:34)
> platform_get_resource() may return NULL, add proper check to
> avoid potential NULL dereferencing.
> =

> This is detected by Coccinelle semantic patch.
> =

> @@
> expression pdev, res, n, t, e, e1, e2;
> @@
> =

> res =3D platform_get_resource(pdev, t, n);
> + if (!res)
> +   return -EINVAL;
> ... when !=3D res =3D=3D NULL
> e =3D devm_ioremap(e1, res->start, e2);
> =

> Fixes: 4f16f7ff3bc0 ("clk: hisilicon: Add support for Hi3660 stub clocks")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>

Applied to clk-fixes