Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Madalin Bucur <madalin.bucur@nxp.com>
To:     Patrick Havelange <patrick.havelange@essensium.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 2/4] net: freescale/fman-port: remove direct use of
 __devm_request_region
Thread-Topic: [PATCH 2/4] net: freescale/fman-port: remove direct use of
 __devm_request_region
Thread-Index: AQHWyMZ42iwz0XCqx0e7//BYen7yN6nlDuWg
Date:   Thu, 3 Dec 2020 08:44:56 +0000
Message-ID: <AM6PR04MB3976E5A576C3E1AA157DA711ECF20@AM6PR04MB3976.eurprd04.prod.outlook.com>
References: <20201202161600.23738-1-patrick.havelange@essensium.com>
 <20201202161600.23738-2-patrick.havelange@essensium.com>
In-Reply-To: <20201202161600.23738-2-patrick.havelange@essensium.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 00be9344-810f-4c00-2a46-08d89767b66c
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Dec 2020 08:44:56.9413
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: L+YLyKGcU9XP41UNvmRqBcItOe10s0YNNwxZciobkfHSYH3BqNilUf2srmDMWjPjaNaDmoHLBrNwkjnwv+/hKQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM7PR04MB6869
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----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
>=20
> 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.
>=20
> 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
>=20
> 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.
>=20
> 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.
>=20
> 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(-)
>=20
> 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)
>=20
>  	of_node_put(port_node);
>=20
> -	dev_res =3D __devm_request_region(port->dev, &res, res.start,
> -					resource_size(&res), "fman-port");
> +	dev_res =3D 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 =3D -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.

Madalin