Moving the mlx5_ifc_query_qp_out_bits structure on the stack was a bit
excessive and now causes the compiler to complain on 32-bit architectures:
drivers/net/ethernet/mellanox/mlx5/core/debugfs.c: In function 'qp_read_field':
drivers/net/ethernet/mellanox/mlx5/core/debugfs.c:274:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
Revert the previous patch partially to use dynamically allocation as
the code did before. Unfortunately there is no good error handling
in case the allocation fails.
Fixes: 57a6c5e992f5 ("net/mlx5: Replace hand written QP context struct with automatic getters")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/net/ethernet/mellanox/mlx5/core/debugfs.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c b/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c
index 6409090b3ec5..d2d57213511b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c
@@ -202,18 +202,23 @@ void mlx5_cq_debugfs_cleanup(struct mlx5_core_dev *dev)
static u64 qp_read_field(struct mlx5_core_dev *dev, struct mlx5_core_qp *qp,
int index, int *is_str)
{
- u32 out[MLX5_ST_SZ_BYTES(query_qp_out)] = {};
+ int outlen = MLX5_ST_SZ_BYTES(query_qp_out);
u32 in[MLX5_ST_SZ_DW(query_qp_in)] = {};
u64 param = 0;
+ u32 *out;
int state;
u32 *qpc;
int err;
+ out = kzalloc(outlen, GFP_KERNEL);
+ if (!out)
+ return 0;
+
MLX5_SET(query_qp_in, in, opcode, MLX5_CMD_OP_QUERY_QP);
MLX5_SET(query_qp_in, in, qpn, qp->qpn);
err = mlx5_cmd_exec_inout(dev, query_qp, in, out);
if (err)
- return 0;
+ goto out;
*is_str = 0;
@@ -269,7 +274,8 @@ static u64 qp_read_field(struct mlx5_core_dev *dev, struct mlx5_core_qp *qp,
param = MLX5_GET(qpc, qpc, remote_qpn);
break;
}
-
+out:
+ kfree(out);
return param;
}
--
2.26.0
On Tue, 2020-04-28 at 23:23 +0200, Arnd Bergmann wrote:
> Moving the mlx5_ifc_query_qp_out_bits structure on the stack was a
> bit
> excessive and now causes the compiler to complain on 32-bit
> architectures:
>
> drivers/net/ethernet/mellanox/mlx5/core/debugfs.c: In function
> 'qp_read_field':
> drivers/net/ethernet/mellanox/mlx5/core/debugfs.c:274:1: error: the
> frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-
> larger-than=]
>
> Revert the previous patch partially to use dynamically allocation as
> the code did before. Unfortunately there is no good error handling
> in case the allocation fails.
>
I don't really mind this, since this is only for debugfs and 0 is not a
valid value anyway.
Acked-by: Saeed Mahameed <[email protected]>
this should go to mlx5-next, i will let Leon decide and pick this up.
> Fixes: 57a6c5e992f5 ("net/mlx5: Replace hand written QP context
> struct with automatic getters")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/debugfs.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c
> b/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c
> index 6409090b3ec5..d2d57213511b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/debugfs.c
> @@ -202,18 +202,23 @@ void mlx5_cq_debugfs_cleanup(struct
> mlx5_core_dev *dev)
> static u64 qp_read_field(struct mlx5_core_dev *dev, struct
> mlx5_core_qp *qp,
> int index, int *is_str)
> {
> - u32 out[MLX5_ST_SZ_BYTES(query_qp_out)] = {};
> + int outlen = MLX5_ST_SZ_BYTES(query_qp_out);
> u32 in[MLX5_ST_SZ_DW(query_qp_in)] = {};
> u64 param = 0;
> + u32 *out;
> int state;
> u32 *qpc;
> int err;
>
> + out = kzalloc(outlen, GFP_KERNEL);
> + if (!out)
> + return 0;
> +
> MLX5_SET(query_qp_in, in, opcode, MLX5_CMD_OP_QUERY_QP);
> MLX5_SET(query_qp_in, in, qpn, qp->qpn);
> err = mlx5_cmd_exec_inout(dev, query_qp, in, out);
> if (err)
> - return 0;
> + goto out;
>
> *is_str = 0;
>
> @@ -269,7 +274,8 @@ static u64 qp_read_field(struct mlx5_core_dev
> *dev, struct mlx5_core_qp *qp,
> param = MLX5_GET(qpc, qpc, remote_qpn);
> break;
> }
> -
> +out:
> + kfree(out);
> return param;
> }
>
On Tue, Apr 28, 2020 at 11:23:47PM +0200, Arnd Bergmann wrote:
> Moving the mlx5_ifc_query_qp_out_bits structure on the stack was a bit
> excessive and now causes the compiler to complain on 32-bit architectures:
>
> drivers/net/ethernet/mellanox/mlx5/core/debugfs.c: In function 'qp_read_field':
> drivers/net/ethernet/mellanox/mlx5/core/debugfs.c:274:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Revert the previous patch partially to use dynamically allocation as
> the code did before. Unfortunately there is no good error handling
> in case the allocation fails.
>
> Fixes: 57a6c5e992f5 ("net/mlx5: Replace hand written QP context struct with automatic getters")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/debugfs.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
Thanks Arnd, I'll pick it to mlx5-next.
I was under impression that the frame size was increased a long
time ago. Is this 1K limit still effective for all archs?
Or is it is 32-bit leftover?
Thanks
On Thu, Apr 30, 2020 at 7:22 AM Leon Romanovsky <[email protected]> wrote:
> On Tue, Apr 28, 2020 at 11:23:47PM +0200, Arnd Bergmann wrote:
> > Moving the mlx5_ifc_query_qp_out_bits structure on the stack was a bit
> > excessive and now causes the compiler to complain on 32-bit architectures:
> >
> > drivers/net/ethernet/mellanox/mlx5/core/debugfs.c: In function 'qp_read_field':
> > drivers/net/ethernet/mellanox/mlx5/core/debugfs.c:274:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> >
> > Revert the previous patch partially to use dynamically allocation as
> > the code did before. Unfortunately there is no good error handling
> > in case the allocation fails.
> >
> > Fixes: 57a6c5e992f5 ("net/mlx5: Replace hand written QP context struct with automatic getters")
> > Signed-off-by: Arnd Bergmann <[email protected]>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/debugfs.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
>
> Thanks Arnd, I'll pick it to mlx5-next.
>
> I was under impression that the frame size was increased a long
> time ago. Is this 1K limit still effective for all archs?
> Or is it is 32-bit leftover?
I got the output on a 32-bit build, but that doesn't make the code
right on 64-bit.
While warning limit is generally 1024 bytes for 32-bit architectures,
and 2048 bytes fro 64-bit architectures, we should probably
reduce the latter to something like 1280 bytes and fix up the
warnings that introduces.
Generally speaking, I'd say a function using more than a few hundred
bytes tends to be a bad idea, but we can't warn about those without
also warning about the handful of cases that do it for a good reason
and using close to 1024 bytes on 32 bit systems or a little more on
64-bit systems, in places that are known not to have deep call chains.
Arnd
On Thu, Apr 30, 2020 at 04:37:14PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 30, 2020 at 7:22 AM Leon Romanovsky <[email protected]> wrote:
> > On Tue, Apr 28, 2020 at 11:23:47PM +0200, Arnd Bergmann wrote:
> > > Moving the mlx5_ifc_query_qp_out_bits structure on the stack was a bit
> > > excessive and now causes the compiler to complain on 32-bit architectures:
> > >
> > > drivers/net/ethernet/mellanox/mlx5/core/debugfs.c: In function 'qp_read_field':
> > > drivers/net/ethernet/mellanox/mlx5/core/debugfs.c:274:1: error: the frame size of 1104 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> > >
> > > Revert the previous patch partially to use dynamically allocation as
> > > the code did before. Unfortunately there is no good error handling
> > > in case the allocation fails.
> > >
> > > Fixes: 57a6c5e992f5 ("net/mlx5: Replace hand written QP context struct with automatic getters")
> > > Signed-off-by: Arnd Bergmann <[email protected]>
> > > ---
> > > drivers/net/ethernet/mellanox/mlx5/core/debugfs.c | 12 +++++++++---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > Thanks Arnd, I'll pick it to mlx5-next.
> >
> > I was under impression that the frame size was increased a long
> > time ago. Is this 1K limit still effective for all archs?
> > Or is it is 32-bit leftover?
>
> I got the output on a 32-bit build, but that doesn't make the code
> right on 64-bit.
>
> While warning limit is generally 1024 bytes for 32-bit architectures,
> and 2048 bytes fro 64-bit architectures, we should probably
> reduce the latter to something like 1280 bytes and fix up the
> warnings that introduces.
It a chicken and an egg problem, I tried to use default frame size, but
the output of my kernel build was constantly flooded with those warnings
and made hard to spot real issues in the code I developed.
Thanks
>
> Generally speaking, I'd say a function using more than a few hundred
> bytes tends to be a bad idea, but we can't warn about those without
> also warning about the handful of cases that do it for a good reason
> and using close to 1024 bytes on 32 bit systems or a little more on
> 64-bit systems, in places that are known not to have deep call chains.
>
> Arnd
On Sun, May 3, 2020 at 7:30 AM Leon Romanovsky <[email protected]> wrote:
> On Thu, Apr 30, 2020 at 04:37:14PM +0200, Arnd Bergmann wrote:
> > On Thu, Apr 30, 2020 at 7:22 AM Leon Romanovsky <[email protected]> wrote:
> >
> > While warning limit is generally 1024 bytes for 32-bit architectures,
> > and 2048 bytes fro 64-bit architectures, we should probably
> > reduce the latter to something like 1280 bytes and fix up the
> > warnings that introduces.
>
> It a chicken and an egg problem, I tried to use default frame size, but
> the output of my kernel build was constantly flooded with those warnings
> and made hard to spot real issues in the code I developed.
>
When did you last try? I usually send patches whenever I see a new
warning, so there really shouldn't be any such warnings in the mainline
kernel except for cases where patches are still under discussion.
If you have a configuration in which you see lots of frame size warnings,
can you send me that .config file for that? It is possible that one or
more of the patches in my backlog fix a bunch of those issues
and need to be resent.
Arnd
On Mon, May 04, 2020 at 05:41:56PM +0200, Arnd Bergmann wrote:
> On Sun, May 3, 2020 at 7:30 AM Leon Romanovsky <[email protected]> wrote:
> > On Thu, Apr 30, 2020 at 04:37:14PM +0200, Arnd Bergmann wrote:
> > > On Thu, Apr 30, 2020 at 7:22 AM Leon Romanovsky <[email protected]> wrote:
> > >
> > > While warning limit is generally 1024 bytes for 32-bit architectures,
> > > and 2048 bytes fro 64-bit architectures, we should probably
> > > reduce the latter to something like 1280 bytes and fix up the
> > > warnings that introduces.
> >
> > It a chicken and an egg problem, I tried to use default frame size, but
> > the output of my kernel build was constantly flooded with those warnings
> > and made hard to spot real issues in the code I developed.
> >
>
> When did you last try? I usually send patches whenever I see a new
> warning, so there really shouldn't be any such warnings in the mainline
> kernel except for cases where patches are still under discussion.
>
> If you have a configuration in which you see lots of frame size warnings,
> can you send me that .config file for that? It is possible that one or
> more of the patches in my backlog fix a bunch of those issues
> and need to be resent.
I tried now (your patch is not in that branch yet).
➜ kernel git:(m/ece) mkt build
Start kernel compilation in silent mode
arch/x86/events/amd/ibs.c: In function ‘perf_ibs_handle_irq’:
arch/x86/events/amd/ibs.c:681:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
681 | }
| ^
fs/select.c: In function ‘do_select’:
fs/select.c:611:1: warning: the frame size of 1136 bytes is larger than 1024 bytes [-Wframe-larger-than=]
611 | }
| ^
fs/select.c: In function ‘do_sys_poll’:
fs/select.c:1023:1: warning: the frame size of 1312 bytes is larger than 1024 bytes [-Wframe-larger-than=]
1023 | }
| ^
drivers/video/fbdev/cirrusfb.o: warning: objtool: cirrusfb_set_par_foo.cold()+0x25f: sibling call from callable instruction with modified stack frame
net/core/rtnetlink.c: In function ‘__rtnl_newlink’:
net/core/rtnetlink.c:3379:1: warning: the frame size of 1280 bytes is larger than 1024 bytes [-Wframe-larger-than=]
3379 | }
| ^
q
drivers/acpi/processor_thermal.c: In function ‘cpu_has_cpufreq’:
drivers/acpi/processor_thermal.c:66:1: warning: the frame size of 1472 bytes is larger than 1024 bytes [-Wframe-larger-than=]
66 | }
| ^
drivers/tty/serial/8250/8250_core.c: In function ‘serial8250_probe’:
drivers/tty/serial/8250/8250_core.c:849:1: warning: the frame size of 1144 bytes is larger than 1024 bytes [-Wframe-larger-than=]
849 | }
| ^
drivers/tty/serial/8250/8250_pnp.c: In function ‘serial_pnp_probe’:
drivers/tty/serial/8250/8250_pnp.c:490:1: warning: the frame size of 1120 bytes is larger than 1024 bytes [-Wframe-larger-than=]
490 | }
| ^
drivers/tty/serial/8250/8250_pci.c: In function ‘pciserial_init_ports’:
drivers/tty/serial/8250/8250_pci.c:3952:1: warning: the frame size of 1160 bytes is larger than 1024 bytes [-Wframe-larger-than=]
3952 | }
| ^
drivers/tty/serial/8250/8250_exar.c: In function ‘exar_pci_probe’:
drivers/tty/serial/8250/8250_exar.c:641:1: warning: the frame size of 1168 bytes is larger than 1024 bytes [-Wframe-larger-than=]
641 | }
| ^
drivers/tty/serial/8250/8250_lpss.c: In function ‘lpss8250_probe.part.0’:
drivers/tty/serial/8250/8250_lpss.c:342:1: warning: the frame size of 1152 bytes is larger than 1024 bytes [-Wframe-larger-than=]
342 | }
| ^
drivers/tty/serial/8250/8250_mid.c: In function ‘mid8250_probe.part.0’:
drivers/tty/serial/8250/8250_mid.c:337:1: warning: the frame size of 1128 bytes is larger than 1024 bytes [-Wframe-larger-than=]
337 | }
| ^
net/ipv6/ipv6_sockglue.c: In function ‘do_ipv6_setsockopt.isra.0’:
net/ipv6/ipv6_sockglue.c:937:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
net/ipv6/ipv6_sockglue.c:937:1: warning: the frame size of 1088 bytes is larger than 1024 bytes [-Wframe-larger-than=]
937 | }
| ^
drivers/net/tun.o: warning: objtool: tun_chr_ioctl.cold()+0x107: sibling call from callable instruction with modified stack frame
drivers/net/ethernet/mellanox/mlx5/core/debugfs.c: In function ‘qp_read_field.isra.0’:
drivers/net/ethernet/mellanox/mlx5/core/debugfs.c:274:1: warning: the frame size of 1312 bytes is larger than 1024 bytes [-Wframe-larger-than=]
274 | }
| ^
drivers/infiniband/core/nldev.c: In function ‘nldev_newlink’:
drivers/infiniband/core/nldev.c:1547:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
1547 | }
| ^
drivers/infiniband/core/nldev.c: In function ‘nldev_get_chardev’:
drivers/infiniband/core/nldev.c:1658:1: warning: the frame size of 1080 bytes is larger than 1024 bytes [-Wframe-larger-than=]
1658 | }
| ^
drivers/infiniband/core/nldev.c: In function ‘res_get_common_dumpit’:
drivers/infiniband/core/nldev.c:1443:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
1443 | }
| ^
drivers/infiniband/sw/siw/siw_qp_tx.c: In function ‘siw_tx_hdt’:
drivers/infiniband/sw/siw/siw_qp_tx.c:658:1: warning: the frame size of 1232 bytes is larger than 1024 bytes [-Wframe-larger-than=]
658 | }
| ^
drivers/infiniband/hw/mlx5/qp.c: In function ‘_mlx5_ib_post_send’:
drivers/infiniband/hw/mlx5/qp.c:5636:1: warning: the frame size of 1384 bytes is larger than 1024 bytes [-Wframe-larger-than=]
5636 | }
| ^
>
> Arnd