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

[Patrick Havelange <patrick.havelange@essensium.com>]

On 2020-12-03 09:44, Madalin Bucur wrote:
>> -----Original Message-----
>> From: Patrick Havelange <patrick.havelange@essensium.com>
>> Sent: 02 December 2020 18:16
>> To: Madalin Bucur <madalin.bucur@nxp.com>; David S. Miller
>> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
>> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
>> Cc: Patrick Havelange <patrick.havelange@essensium.com>
>> 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@essensium.com>
>> ---
>>   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.