| Message ID | 20180202154636.1624079-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | net: mlx5: remove pointless memcpy | expand |
On Fri, Feb 2, 2018 at 8:06 PM, Jason Gunthorpe <jgg@mellanox.com> wrote: > On Fri, Feb 02, 2018 at 04:46:30PM +0100, Arnd Bergmann wrote: >> gcc-8 notices that the memcpy in mlx5_core_query_xsrq() makes no >> sense because the source and destination variables are identical: >> >> drivers/net/ethernet/mellanox/mlx5/core/transobj.c: In function 'mlx5_core_query_xsrq': >> drivers/net/ethernet/mellanox/mlx5/core/transobj.c:347:3: error: 'memcpy' source argument is the same as destination [-Werror=restrict] >> >> Either one of the pointers should be something else, or the code is >> completely bogus. Removing the memcpy() won't change the behavior >> but gets rid of the warning. >> >> Fixes: 01949d0109ee ("net/mlx5_core: Enable XRCs and SRQs when using ISSI > 0") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> Please review carefully, I have no idea what the author actually >> intended here. > > I think they intended to adjust the command return between > mlx5_ifc_query_srq_out_bits and mlx5_ifc_query_xrc_srq_out_bits? > >> index 9e38343a951f..75450f7d53bf 100644 >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/transobj.c >> @@ -332,20 +332,12 @@ int mlx5_core_destroy_xsrq(struct mlx5_core_dev *dev, u32 xsrqn) >> int mlx5_core_query_xsrq(struct mlx5_core_dev *dev, u32 xsrqn, u32 *out) >> { >> u32 in[MLX5_ST_SZ_DW(query_xrc_srq_in)] = {0}; >> - void *srqc; >> - void *xrc_srqc; >> int err; >> >> MLX5_SET(query_xrc_srq_in, in, opcode, MLX5_CMD_OP_QUERY_XRC_SRQ); >> MLX5_SET(query_xrc_srq_in, in, xrc_srqn, xsrqn); >> err = mlx5_cmd_exec(dev, in, sizeof(in), out, >> MLX5_ST_SZ_BYTES(query_xrc_srq_out)); >> - if (!err) { >> - xrc_srqc = MLX5_ADDR_OF(query_xrc_srq_out, out, >> - xrc_srq_context_entry); >> - srqc = MLX5_ADDR_OF(query_srq_out, out, srq_context_entry); >> - memcpy(srqc, xrc_srqc, MLX5_ST_SZ_BYTES(srqc)); >> - } > > Probably should add a > > BUILD_BUG_ON(MLX5_BYTE_OFF(query_xrc_srq_out, xrc_srq_context_entry) == MLX5_BYTE_OFF(query_srq_out, srq_context_entry)); > > Just for clarity that the SRQ and XRC_SRQ are being used interchangeably. > > and the 'err' variable can be eliminated. > > Curious though that I can't find a call site for it, and removing the > prototype doesn't break the build.. Seems like dead code. I checked the git history and don't see any user ever added after the function first showed up in the kernel, same for a couple of other functions from commit 01949d0109ee ("net/mlx5_core: Enable XRCs and SRQs when using ISSI > 0"). Can you come up with a proper patch for this isse, either removing the dead code, or fixing it appropriately? You clearly understand what this file is about, and I don't ;-) Arnd
On 02/02/2018 12:26 PM, Arnd Bergmann wrote: > On Fri, Feb 2, 2018 at 8:06 PM, Jason Gunthorpe <jgg@mellanox.com> wrote: >> On Fri, Feb 02, 2018 at 04:46:30PM +0100, Arnd Bergmann wrote: >>> gcc-8 notices that the memcpy in mlx5_core_query_xsrq() makes no >>> sense because the source and destination variables are identical: >>> >>> drivers/net/ethernet/mellanox/mlx5/core/transobj.c: In function 'mlx5_core_query_xsrq': >>> drivers/net/ethernet/mellanox/mlx5/core/transobj.c:347:3: error: 'memcpy' source argument is the same as destination [-Werror=restrict] >>> >>> Either one of the pointers should be something else, or the code is >>> completely bogus. Removing the memcpy() won't change the behavior >>> but gets rid of the warning. >>> >>> Fixes: 01949d0109ee ("net/mlx5_core: Enable XRCs and SRQs when using ISSI > 0") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> Please review carefully, I have no idea what the author actually >>> intended here. >> >> I think they intended to adjust the command return between >> mlx5_ifc_query_srq_out_bits and mlx5_ifc_query_xrc_srq_out_bits? >> >>> index 9e38343a951f..75450f7d53bf 100644 >>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/transobj.c >>> @@ -332,20 +332,12 @@ int mlx5_core_destroy_xsrq(struct mlx5_core_dev *dev, u32 xsrqn) >>> int mlx5_core_query_xsrq(struct mlx5_core_dev *dev, u32 xsrqn, u32 *out) >>> { >>> u32 in[MLX5_ST_SZ_DW(query_xrc_srq_in)] = {0}; >>> - void *srqc; >>> - void *xrc_srqc; >>> int err; >>> >>> MLX5_SET(query_xrc_srq_in, in, opcode, MLX5_CMD_OP_QUERY_XRC_SRQ); >>> MLX5_SET(query_xrc_srq_in, in, xrc_srqn, xsrqn); >>> err = mlx5_cmd_exec(dev, in, sizeof(in), out, >>> MLX5_ST_SZ_BYTES(query_xrc_srq_out)); >>> - if (!err) { >>> - xrc_srqc = MLX5_ADDR_OF(query_xrc_srq_out, out, >>> - xrc_srq_context_entry); >>> - srqc = MLX5_ADDR_OF(query_srq_out, out, srq_context_entry); >>> - memcpy(srqc, xrc_srqc, MLX5_ST_SZ_BYTES(srqc)); >>> - } OMG! >> >> Probably should add a >> >> BUILD_BUG_ON(MLX5_BYTE_OFF(query_xrc_srq_out, xrc_srq_context_entry) == MLX5_BYTE_OFF(query_srq_out, srq_context_entry)); >> >> Just for clarity that the SRQ and XRC_SRQ are being used interchangeably. >> >> and the 'err' variable can be eliminated. >> >> Curious though that I can't find a call site for it, and removing the >> prototype doesn't break the build.. Seems like dead code. > > I checked the git history and don't see any user ever added after the function > first showed up in the kernel, same for a couple of other functions from > commit 01949d0109ee ("net/mlx5_core: Enable XRCs and SRQs when > using ISSI > 0"). > > Can you come up with a proper patch for this isse, either removing the > dead code, or fixing it appropriately? You clearly understand what this > file is about, and I don't ;-) Simply this is just pointless dead code, will remove it, there is no point of trying to figure out what the author was thinking the day he wrote that patch :) Thank you Arnd for spotting this. > > Arnd >
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/transobj.c b/drivers/net/ethernet/mellanox/mlx5/core/transobj.c index 9e38343a951f..75450f7d53bf 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/transobj.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/transobj.c @@ -332,20 +332,12 @@ int mlx5_core_destroy_xsrq(struct mlx5_core_dev *dev, u32 xsrqn) int mlx5_core_query_xsrq(struct mlx5_core_dev *dev, u32 xsrqn, u32 *out) { u32 in[MLX5_ST_SZ_DW(query_xrc_srq_in)] = {0}; - void *srqc; - void *xrc_srqc; int err; MLX5_SET(query_xrc_srq_in, in, opcode, MLX5_CMD_OP_QUERY_XRC_SRQ); MLX5_SET(query_xrc_srq_in, in, xrc_srqn, xsrqn); err = mlx5_cmd_exec(dev, in, sizeof(in), out, MLX5_ST_SZ_BYTES(query_xrc_srq_out)); - if (!err) { - xrc_srqc = MLX5_ADDR_OF(query_xrc_srq_out, out, - xrc_srq_context_entry); - srqc = MLX5_ADDR_OF(query_srq_out, out, srq_context_entry); - memcpy(srqc, xrc_srqc, MLX5_ST_SZ_BYTES(srqc)); - } return err; }
gcc-8 notices that the memcpy in mlx5_core_query_xsrq() makes no sense because the source and destination variables are identical: drivers/net/ethernet/mellanox/mlx5/core/transobj.c: In function 'mlx5_core_query_xsrq': drivers/net/ethernet/mellanox/mlx5/core/transobj.c:347:3: error: 'memcpy' source argument is the same as destination [-Werror=restrict] Either one of the pointers should be something else, or the code is completely bogus. Removing the memcpy() won't change the behavior but gets rid of the warning. Fixes: 01949d0109ee ("net/mlx5_core: Enable XRCs and SRQs when using ISSI > 0") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- Please review carefully, I have no idea what the author actually intended here. --- drivers/net/ethernet/mellanox/mlx5/core/transobj.c | 8 -------- 1 file changed, 8 deletions(-) -- 2.9.0