[PATCH net v1] net: macb: fix an issue about leak related system resources

[PATCH net v1] net: macb: fix an issue about leak related system resources

[Dejin Zheng]

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 <andy.shevchenko@gmail.com>
Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
---
 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

RE: [PATCH net v1] net: macb: fix an issue about leak related system resources

[Yash Shah]

> -----Original Message-----
> From: Dejin Zheng <zhengdejin5@gmail.com>
> Sent: 25 April 2020 18:28
> To: davem@davemloft.net; Paul Walmsley <paul.walmsley@sifive.com>;
> palmer@dabbelt.com; nicolas.ferre@microchip.com; Yash Shah
> <yash.shah@sifive.com>; netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org; Dejin Zheng <zhengdejin5@gmail.com>;
> Andy Shevchenko <andy.shevchenko@gmail.com>
> 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 <andy.shevchenko@gmail.com>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
>  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 <yash.shah@sifive.com>

- Yash

Re: [PATCH net v1] net: macb: fix an issue about leak related system resources

[Andy Shevchenko]

On Sat, Apr 25, 2020 at 3:57 PM Dejin Zheng <zhengdejin5@gmail.com> 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 <andy.shevchenko@gmail.com>
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
>  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

Re: [PATCH net v1] net: macb: fix an issue about leak related system resources

[Dejin Zheng]

On Mon, Apr 27, 2020 at 01:33:41PM +0300, Andy Shevchenko wrote:
> On Sat, Apr 25, 2020 at 3:57 PM Dejin Zheng <zhengdejin5@gmail.com> 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 <andy.shevchenko@gmail.com>
> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> > ---
> >  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

Re: [PATCH net v1] net: macb: fix an issue about leak related system resources

[Dejin Zheng]

On Mon, Apr 27, 2020 at 08:25:41AM +0000, Yash Shah wrote:
> > -----Original Message-----
> > From: Dejin Zheng <zhengdejin5@gmail.com>
> > Sent: 25 April 2020 18:28
> > To: davem@davemloft.net; Paul Walmsley <paul.walmsley@sifive.com>;
> > palmer@dabbelt.com; nicolas.ferre@microchip.com; Yash Shah
> > <yash.shah@sifive.com>; netdev@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org; Dejin Zheng <zhengdejin5@gmail.com>;
> > Andy Shevchenko <andy.shevchenko@gmail.com>
> > 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 <andy.shevchenko@gmail.com>
> > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> > ---
> >  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 <yash.shah@sifive.com>
>
Thanks Yash!

> - Yash
> 

Re: [PATCH net v1] net: macb: fix an issue about leak related system resources

[Nicolas Ferre]

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 <zhengdejin5@gmail.com> 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 <andy.shevchenko@gmail.com>
>>> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
>>> ---
>>>   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

Re: [PATCH net v1] net: macb: fix an issue about leak related system resources

[Dejin Zheng]

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 <zhengdejin5@gmail.com> 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 <andy.shevchenko@gmail.com>
> > > > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> > > > ---
> > > >   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

Re: [PATCH net v1] net: macb: fix an issue about leak related system resources

[Andy Shevchenko]

On Tue, Apr 28, 2020 at 4:12 PM Dejin Zheng <zhengdejin5@gmail.com> 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 <zhengdejin5@gmail.com> 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

Re: [PATCH net v1] net: macb: fix an issue about leak related system resources

[Dejin Zheng]

On Tue, Apr 28, 2020 at 05:04:32PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 28, 2020 at 4:12 PM Dejin Zheng <zhengdejin5@gmail.com> 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 <zhengdejin5@gmail.com> 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/20200127135841.17935-1-zhengdejin5@gmail.com/

BR,
Dejin
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko