Re: [PATCH 2/4] net: freescale/fman-port: remove direct use of __devm_request_region

[Patrick Havelange]

On 2020-12-03 09:44, Madalin Bucur wrote:
> > -----Original Message-----
> > From: Patrick Havelange <patrick.havelange@xxxxxxxxxxxxx>
> > Sent: 02 December 2020 18:16
> > To: Madalin Bucur <madalin.bucur@xxxxxxx>; David S. Miller
> > <davem@xxxxxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
> > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Cc: Patrick Havelange <patrick.havelange@xxxxxxxxxxxxx>
> > Subject: [PATCH 2/4] net: freescale/fman-port: remove direct use of
> > __devm_request_region
> >
> > This driver was directly calling __devm_request_region with a specific
> > resource on the stack as parameter. This is invalid as
> > __devm_request_region expects the given resource passed as parameter
> > to live longer than the call itself, as the pointer to the resource
> > will be stored inside the internal struct used by the devres
> > management.
> >
> > In addition to this issue, a related bug has been found by kmemleak
> > with this trace :
> > unreferenced object 0xc0000001efc01880 (size 64):
> >    comm "swapper/0", pid 1, jiffies 4294669078 (age 3620.536s)
> >    hex dump (first 32 bytes):
> >      00 00 00 0f fe 4a c0 00 00 00 00 0f fe 4a cf ff  .....J.......J..
> >      c0 00 00 00 00 ee 9d 98 00 00 00 00 80 00 02 00  ................
> >    backtrace:
> >      [<c000000000078874>] .alloc_resource+0xb8/0xe0
> >      [<c000000000079b50>] .__request_region+0x70/0x1c4
> >      [<c00000000007a010>] .__devm_request_region+0x8c/0x138
> >      [<c0000000006e0dc8>] .fman_port_probe+0x170/0x420
> >      [<c0000000005cecb8>] .platform_drv_probe+0x84/0x108
> >      [<c0000000005cc620>] .driver_probe_device+0x2c4/0x394
> >      [<c0000000005cc814>] .__driver_attach+0x124/0x128
> >      [<c0000000005c9ad4>] .bus_for_each_dev+0xb4/0x110
> >      [<c0000000005cca1c>] .driver_attach+0x34/0x4c
> >      [<c0000000005ca9b0>] .bus_add_driver+0x264/0x2a4
> >      [<c0000000005cd9e0>] .driver_register+0x94/0x160
> >      [<c0000000005cfea4>] .__platform_driver_register+0x60/0x7c
> >      [<c000000000f86a00>] .fman_port_load+0x28/0x64
> >      [<c000000000f4106c>] .do_one_initcall+0xd4/0x1a8
> >      [<c000000000f412fc>] .kernel_init_freeable+0x1bc/0x2a4
> >      [<c00000000000180c>] .kernel_init+0x24/0x138
> >
> > Indeed, the new resource (created in __request_region) will be linked
> > to the given resource living on the stack, which will end its lifetime
> > after the function calling __devm_request_region has finished.
> > Meaning the new resource allocated is no longer reachable.
> >
> > Now that the main fman driver is no longer reserving the region
> > used by fman-port, this previous hack is no longer needed
> > and we can use the regular call to devm_request_mem_region instead,
> > solving those bugs at the same time.
> >
> > Signed-off-by: Patrick Havelange <patrick.havelange@xxxxxxxxxxxxx>
> > ---
> >   drivers/net/ethernet/freescale/fman/fman_port.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c
> > b/drivers/net/ethernet/freescale/fman/fman_port.c
> > index d9baac0dbc7d..354974939d9d 100644
> > --- a/drivers/net/ethernet/freescale/fman/fman_port.c
> > +++ b/drivers/net/ethernet/freescale/fman/fman_port.c
> > @@ -1878,10 +1878,10 @@ static int fman_port_probe(struct platform_device
> > *of_dev)
> >
> >   	of_node_put(port_node);
> >
> > -	dev_res = __devm_request_region(port->dev, &res, res.start,
> > -					resource_size(&res), "fman-port");
> > +	dev_res = devm_request_mem_region(port->dev, res.start,
> > +					  resource_size(&res), "fman-port");
> >   	if (!dev_res) {
> > -		dev_err(port->dev, "%s: __devm_request_region() failed\n",
> > +		dev_err(port->dev, "%s: devm_request_mem_region() failed\n",
> >   			__func__);
> >   		err = -EINVAL;
> >   		goto free_port;
> > --
> > 2.17.1
>
> Hi Patrick,
>
> please send a fix for the issue, targeting the net tree, separate from the
> other change you are trying to introduce. We need a better explanation for
> the latter and it should go through the net-next tree, if accepted.

Hello,

I've resent the series with a cover letter having a bit more 
explanations. I think all the patches should be applied on net, as they 
are linked to the same issue/resolution, and are not independent.

BR,

Patrick H.