A call of the function macb_init() can fail in the function
fu540_c000_init. The related system resources were not released
then. use devm_ioremap() to replace ioremap() for fix it.
Fixes: c218ad559020ff9 ("macb: Add support for SiFive FU540-C000")
Cc: Andy Shevchenko <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
drivers/net/ethernet/cadence/macb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a0e8c5bbabc0..edba2eb56231 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4178,7 +4178,7 @@ static int fu540_c000_init(struct platform_device *pdev)
if (!res)
return -ENODEV;
- mgmt->reg = ioremap(res->start, resource_size(res));
+ mgmt->reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
if (!mgmt->reg)
return -ENOMEM;
--
2.25.0
> -----Original Message-----
> From: Dejin Zheng <[email protected]>
> Sent: 25 April 2020 18:28
> To: [email protected]; Paul Walmsley <[email protected]>;
> [email protected]; [email protected]; Yash Shah
> <[email protected]>; [email protected]
> Cc: [email protected]; Dejin Zheng <[email protected]>;
> Andy Shevchenko <[email protected]>
> Subject: [PATCH net v1] net: macb: fix an issue about leak related system
> resources
>
> [External Email] Do not click links or attachments unless you recognize the
> sender and know the content is safe
>
> A call of the function macb_init() can fail in the function fu540_c000_init. The
> related system resources were not released then. use devm_ioremap() to
> replace ioremap() for fix it.
>
> Fixes: c218ad559020ff9 ("macb: Add support for SiFive FU540-C000")
> Cc: Andy Shevchenko <[email protected]>
> Signed-off-by: Dejin Zheng <[email protected]>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index a0e8c5bbabc0..edba2eb56231 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4178,7 +4178,7 @@ static int fu540_c000_init(struct platform_device
> *pdev)
> if (!res)
> return -ENODEV;
>
> - mgmt->reg = ioremap(res->start, resource_size(res));
> + mgmt->reg = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> if (!mgmt->reg)
> return -ENOMEM;
>
> --
> 2.25.0
The change looks good to me.
Reviewed-by: Yash Shah <[email protected]>
- Yash
On Sat, Apr 25, 2020 at 3:57 PM Dejin Zheng <[email protected]> wrote:
>
> A call of the function macb_init() can fail in the function
> fu540_c000_init. The related system resources were not released
> then. use devm_ioremap() to replace ioremap() for fix it.
>
Why not to go further and convert to use devm_platform_ioremap_resource()?
> Fixes: c218ad559020ff9 ("macb: Add support for SiFive FU540-C000")
> Cc: Andy Shevchenko <[email protected]>
> Signed-off-by: Dejin Zheng <[email protected]>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a0e8c5bbabc0..edba2eb56231 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4178,7 +4178,7 @@ static int fu540_c000_init(struct platform_device *pdev)
> if (!res)
> return -ENODEV;
>
> - mgmt->reg = ioremap(res->start, resource_size(res));
> + mgmt->reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> if (!mgmt->reg)
> return -ENOMEM;
>
> --
> 2.25.0
>
--
With Best Regards,
Andy Shevchenko
On Mon, Apr 27, 2020 at 01:33:41PM +0300, Andy Shevchenko wrote:
> On Sat, Apr 25, 2020 at 3:57 PM Dejin Zheng <[email protected]> wrote:
> >
> > A call of the function macb_init() can fail in the function
> > fu540_c000_init. The related system resources were not released
> > then. use devm_ioremap() to replace ioremap() for fix it.
> >
>
> Why not to go further and convert to use devm_platform_ioremap_resource()?
>
devm_platform_ioremap_resource() will call devm_request_mem_region(),
and here did not do it.
> > Fixes: c218ad559020ff9 ("macb: Add support for SiFive FU540-C000")
> > Cc: Andy Shevchenko <[email protected]>
> > Signed-off-by: Dejin Zheng <[email protected]>
> > ---
> > drivers/net/ethernet/cadence/macb_main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index a0e8c5bbabc0..edba2eb56231 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -4178,7 +4178,7 @@ static int fu540_c000_init(struct platform_device *pdev)
> > if (!res)
> > return -ENODEV;
> >
> > - mgmt->reg = ioremap(res->start, resource_size(res));
> > + mgmt->reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > if (!mgmt->reg)
> > return -ENOMEM;
> >
> > --
> > 2.25.0
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko
On Mon, Apr 27, 2020 at 08:25:41AM +0000, Yash Shah wrote:
> > -----Original Message-----
> > From: Dejin Zheng <[email protected]>
> > Sent: 25 April 2020 18:28
> > To: [email protected]; Paul Walmsley <[email protected]>;
> > [email protected]; [email protected]; Yash Shah
> > <[email protected]>; [email protected]
> > Cc: [email protected]; Dejin Zheng <[email protected]>;
> > Andy Shevchenko <[email protected]>
> > Subject: [PATCH net v1] net: macb: fix an issue about leak related system
> > resources
> >
> > [External Email] Do not click links or attachments unless you recognize the
> > sender and know the content is safe
> >
> > A call of the function macb_init() can fail in the function fu540_c000_init. The
> > related system resources were not released then. use devm_ioremap() to
> > replace ioremap() for fix it.
> >
> > Fixes: c218ad559020ff9 ("macb: Add support for SiFive FU540-C000")
> > Cc: Andy Shevchenko <[email protected]>
> > Signed-off-by: Dejin Zheng <[email protected]>
> > ---
> > drivers/net/ethernet/cadence/macb_main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c
> > b/drivers/net/ethernet/cadence/macb_main.c
> > index a0e8c5bbabc0..edba2eb56231 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -4178,7 +4178,7 @@ static int fu540_c000_init(struct platform_device
> > *pdev)
> > if (!res)
> > return -ENODEV;
> >
> > - mgmt->reg = ioremap(res->start, resource_size(res));
> > + mgmt->reg = devm_ioremap(&pdev->dev, res->start,
> > + resource_size(res));
> > if (!mgmt->reg)
> > return -ENOMEM;
> >
> > --
> > 2.25.0
>
> The change looks good to me.
> Reviewed-by: Yash Shah <[email protected]>
>
Thanks Yash!
> - Yash
>
On 28/04/2020 at 05:24, Dejin Zheng wrote:
> On Mon, Apr 27, 2020 at 01:33:41PM +0300, Andy Shevchenko wrote:
>> On Sat, Apr 25, 2020 at 3:57 PM Dejin Zheng <[email protected]> wrote:
>>>
>>> A call of the function macb_init() can fail in the function
>>> fu540_c000_init. The related system resources were not released
>>> then. use devm_ioremap() to replace ioremap() for fix it.
>>>
>>
>> Why not to go further and convert to use devm_platform_ioremap_resource()?
>>
> devm_platform_ioremap_resource() will call devm_request_mem_region(),
> and here did not do it.
And what about devm_platform_get_and_ioremap_resource()? This would
streamline this whole fu540_c000_init() function.
Regards,
Nicolas
>>> Fixes: c218ad559020ff9 ("macb: Add support for SiFive FU540-C000")
>>> Cc: Andy Shevchenko <[email protected]>
>>> Signed-off-by: Dejin Zheng <[email protected]>
>>> ---
>>> drivers/net/ethernet/cadence/macb_main.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>> index a0e8c5bbabc0..edba2eb56231 100644
>>> --- a/drivers/net/ethernet/cadence/macb_main.c
>>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>>> @@ -4178,7 +4178,7 @@ static int fu540_c000_init(struct platform_device *pdev)
>>> if (!res)
>>> return -ENODEV;
>>>
>>> - mgmt->reg = ioremap(res->start, resource_size(res));
>>> + mgmt->reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>> if (!mgmt->reg)
>>> return -ENOMEM;
>>>
--
Nicolas Ferre
On Tue, Apr 28, 2020 at 10:42:56AM +0200, Nicolas Ferre wrote:
> On 28/04/2020 at 05:24, Dejin Zheng wrote:
> > On Mon, Apr 27, 2020 at 01:33:41PM +0300, Andy Shevchenko wrote:
> > > On Sat, Apr 25, 2020 at 3:57 PM Dejin Zheng <[email protected]> wrote:
> > > >
> > > > A call of the function macb_init() can fail in the function
> > > > fu540_c000_init. The related system resources were not released
> > > > then. use devm_ioremap() to replace ioremap() for fix it.
> > > >
> > >
> > > Why not to go further and convert to use devm_platform_ioremap_resource()?
> > >
> > devm_platform_ioremap_resource() will call devm_request_mem_region(),
> > and here did not do it.
>
> And what about devm_platform_get_and_ioremap_resource()? This would
> streamline this whole fu540_c000_init() function.
>
Nicolas, the function devm_platform_get_and_ioremap_resource() will also
call devm_request_mem_region(), after call it, These IO addresses will
be monopolized by this driver. the devm_ioremap() and ioremap() are not
do this. if this IO addresses will be shared with the other driver, call
devm_platform_get_and_ioremap_resource() may be fail.
BR,
Dejin
> Regards,
> Nicolas
>
> > > > Fixes: c218ad559020ff9 ("macb: Add support for SiFive FU540-C000")
> > > > Cc: Andy Shevchenko <[email protected]>
> > > > Signed-off-by: Dejin Zheng <[email protected]>
> > > > ---
> > > > drivers/net/ethernet/cadence/macb_main.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > > > index a0e8c5bbabc0..edba2eb56231 100644
> > > > --- a/drivers/net/ethernet/cadence/macb_main.c
> > > > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > > > @@ -4178,7 +4178,7 @@ static int fu540_c000_init(struct platform_device *pdev)
> > > > if (!res)
> > > > return -ENODEV;
> > > >
> > > > - mgmt->reg = ioremap(res->start, resource_size(res));
> > > > + mgmt->reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > > > if (!mgmt->reg)
> > > > return -ENOMEM;
> > > >
>
>
> --
> Nicolas Ferre
On Tue, Apr 28, 2020 at 4:12 PM Dejin Zheng <[email protected]> wrote:
> On Tue, Apr 28, 2020 at 10:42:56AM +0200, Nicolas Ferre wrote:
> > On 28/04/2020 at 05:24, Dejin Zheng wrote:
> > > On Mon, Apr 27, 2020 at 01:33:41PM +0300, Andy Shevchenko wrote:
> > > > On Sat, Apr 25, 2020 at 3:57 PM Dejin Zheng <[email protected]> wrote:
> > > > >
> > > > > A call of the function macb_init() can fail in the function
> > > > > fu540_c000_init. The related system resources were not released
> > > > > then. use devm_ioremap() to replace ioremap() for fix it.
> > > > >
> > > >
> > > > Why not to go further and convert to use devm_platform_ioremap_resource()?
> > > >
> > > devm_platform_ioremap_resource() will call devm_request_mem_region(),
> > > and here did not do it.
> >
> > And what about devm_platform_get_and_ioremap_resource()? This would
> > streamline this whole fu540_c000_init() function.
> >
> Nicolas, the function devm_platform_get_and_ioremap_resource() will also
> call devm_request_mem_region(), after call it, These IO addresses will
> be monopolized by this driver. the devm_ioremap() and ioremap() are not
> do this. if this IO addresses will be shared with the other driver, call
> devm_platform_get_and_ioremap_resource() may be fail.
I guess request region is a right thing to do. If driver is sharing
this IO region with something else, it is a delayed bomb attack and
has to be fixed.
--
With Best Regards,
Andy Shevchenko
On Tue, Apr 28, 2020 at 05:04:32PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 28, 2020 at 4:12 PM Dejin Zheng <[email protected]> wrote:
> > On Tue, Apr 28, 2020 at 10:42:56AM +0200, Nicolas Ferre wrote:
> > > On 28/04/2020 at 05:24, Dejin Zheng wrote:
> > > > On Mon, Apr 27, 2020 at 01:33:41PM +0300, Andy Shevchenko wrote:
> > > > > On Sat, Apr 25, 2020 at 3:57 PM Dejin Zheng <[email protected]> wrote:
> > > > > >
> > > > > > A call of the function macb_init() can fail in the function
> > > > > > fu540_c000_init. The related system resources were not released
> > > > > > then. use devm_ioremap() to replace ioremap() for fix it.
> > > > > >
> > > > >
> > > > > Why not to go further and convert to use devm_platform_ioremap_resource()?
> > > > >
> > > > devm_platform_ioremap_resource() will call devm_request_mem_region(),
> > > > and here did not do it.
> > >
> > > And what about devm_platform_get_and_ioremap_resource()? This would
> > > streamline this whole fu540_c000_init() function.
> > >
> > Nicolas, the function devm_platform_get_and_ioremap_resource() will also
> > call devm_request_mem_region(), after call it, These IO addresses will
> > be monopolized by this driver. the devm_ioremap() and ioremap() are not
> > do this. if this IO addresses will be shared with the other driver, call
> > devm_platform_get_and_ioremap_resource() may be fail.
>
> I guess request region is a right thing to do. If driver is sharing
> this IO region with something else, it is a delayed bomb attack and
> has to be fixed.
>
>
I did encounter IO address sharing, for example, some registers are shared
by both PHY and USB controllers on Tegra SoCs. See link[1] for details.
If many people think that devm_request_mem_region() should be added
here, I will send patch v2 and convert to use
devm_platform_ioremap_resource(). Thanks very much!
link[1]: https://patchwork.ozlabs.org/project/linux-tegra/patch/[email protected]/
BR,
Dejin
>
>
> --
> With Best Regards,
> Andy Shevchenko