| Message ID | 20161011042222.2366145-1-tom@herbertland.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 10/11/2016 01:22 PM, Tom Herbert wrote: > I am hitting this in mlx5: > > drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c: In function > ‘reclaim_pages_cmd.clone.0’: > drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c:346: error: call > to ‘__compiletime_assert_346’ declared with attribute error: > BUILD_BUG_ON failed: __mlx5_bit_off(manage_pages_out, pas[i]) % 64 > drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c: In function ‘give_pages’: > drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c:291: error: call > to ‘__compiletime_assert_291’ declared with attribute error: > BUILD_BUG_ON failed: __mlx5_bit_off(manage_pages_in, pas[i]) % 64 > > Problem is that this is doing a BUILD_BUG_ON on a non-constant > expression because of trying to take offset of pas[i] in the > structure. > > Fix is to create MLX5_SET64_VCHK that takes an additional argument > that is a constant. There are two callers of MLX5_SET64 that are > trying to get a variable offset, change those to call MLX5_SET64_VCHK > passing pas[0] as the argument to use in the offset check. > > Fixes: a533ed5e179cd ("net/mlx5: Pages management commands via mlx5 ifc") > Signed-off-by: Tom Herbert <tom@herbertland.com> Acked-by: Saeed Mahameed <saeedm@mellanox.com>
From: Tom Herbert > Sent: 11 October 2016 05:22 ... > Fix is to create MLX5_SET64_VCHK that takes an additional argument > that is a constant. There are two callers of MLX5_SET64 that are > trying to get a variable offset, change those to call MLX5_SET64_VCHK > passing pas[0] as the argument to use in the offset check. I think I'd separate the array index instead. Something like: #define MLX5_SET64_INDEXED(typ, p, fld, ndx, v) do { \ BUILD_BUG_ON(__mlx5_bit_off(typ, fld) % 64); \ __MLX5_SET64(typ, p, fld[ndx], v); \ } while (0) David
On Tue, Oct 11, 2016 at 7:50 PM, David Laight <David.Laight@aculab.com> wrote: > From: Tom Herbert >> Sent: 11 October 2016 05:22 > ... >> Fix is to create MLX5_SET64_VCHK that takes an additional argument >> that is a constant. There are two callers of MLX5_SET64 that are >> trying to get a variable offset, change those to call MLX5_SET64_VCHK >> passing pas[0] as the argument to use in the offset check. > > I think I'd separate the array index instead. > Something like: > > #define MLX5_SET64_INDEXED(typ, p, fld, ndx, v) do { \ > BUILD_BUG_ON(__mlx5_bit_off(typ, fld) % 64); \ > __MLX5_SET64(typ, p, fld[ndx], v); \ > } while (0) > > David Yes, I think this looks more natural, but instead MLX5_SET64_INDEXED, I prefer to have 2 macros MLX5_SET64(typ, p, fld, v) and MLX5_ARRAY_SET64(typ, p, fld, idx, v). Tom, do you want me to fix it ? Thanks, Saeed.
On Tue, Oct 11, 2016 at 4:57 AM, Saeed Mahameed <saeedm@dev.mellanox.co.il> wrote: > On Tue, Oct 11, 2016 at 7:50 PM, David Laight <David.Laight@aculab.com> wrote: >> From: Tom Herbert >>> Sent: 11 October 2016 05:22 >> ... >>> Fix is to create MLX5_SET64_VCHK that takes an additional argument >>> that is a constant. There are two callers of MLX5_SET64 that are >>> trying to get a variable offset, change those to call MLX5_SET64_VCHK >>> passing pas[0] as the argument to use in the offset check. >> >> I think I'd separate the array index instead. >> Something like: >> >> #define MLX5_SET64_INDEXED(typ, p, fld, ndx, v) do { \ >> BUILD_BUG_ON(__mlx5_bit_off(typ, fld) % 64); \ >> __MLX5_SET64(typ, p, fld[ndx], v); \ >> } while (0) >> >> David > > Yes, I think this looks more natural, but instead MLX5_SET64_INDEXED, > I prefer to have 2 macros > MLX5_SET64(typ, p, fld, v) and MLX5_ARRAY_SET64(typ, p, fld, idx, v). > > Tom, do you want me to fix it ? > Please do. > Thanks, > Saeed.
On Tue, Oct 11, 2016 at 08:46:45AM -0700, Tom Herbert wrote: > On Tue, Oct 11, 2016 at 4:57 AM, Saeed Mahameed > <saeedm@dev.mellanox.co.il> wrote: > > On Tue, Oct 11, 2016 at 7:50 PM, David Laight <David.Laight@aculab.com> wrote: > >> From: Tom Herbert > >>> Sent: 11 October 2016 05:22 > >> ... > >>> Fix is to create MLX5_SET64_VCHK that takes an additional argument > >>> that is a constant. There are two callers of MLX5_SET64 that are > >>> trying to get a variable offset, change those to call MLX5_SET64_VCHK > >>> passing pas[0] as the argument to use in the offset check. > >> > >> I think I'd separate the array index instead. > >> Something like: > >> > >> #define MLX5_SET64_INDEXED(typ, p, fld, ndx, v) do { \ > >> BUILD_BUG_ON(__mlx5_bit_off(typ, fld) % 64); \ > >> __MLX5_SET64(typ, p, fld[ndx], v); \ > >> } while (0) > >> > >> David > > > > Yes, I think this looks more natural, but instead MLX5_SET64_INDEXED, > > I prefer to have 2 macros > > MLX5_SET64(typ, p, fld, v) and MLX5_ARRAY_SET64(typ, p, fld, idx, v). > > > > Tom, do you want me to fix it ? > > > Please do. Saeed, Do you success to send this patch before -rc1 is released? So Linus's -rc1 will be clean from such build error. > > > Thanks, > > Saeed.
On Wed, Oct 12, 2016 at 4:40 AM, Leon Romanovsky <leon@kernel.org> wrote: > On Tue, Oct 11, 2016 at 08:46:45AM -0700, Tom Herbert wrote: >> On Tue, Oct 11, 2016 at 4:57 AM, Saeed Mahameed >> <saeedm@dev.mellanox.co.il> wrote: >> > On Tue, Oct 11, 2016 at 7:50 PM, David Laight <David.Laight@aculab.com> wrote: >> >> From: Tom Herbert >> >>> Sent: 11 October 2016 05:22 >> >> ... >> >>> Fix is to create MLX5_SET64_VCHK that takes an additional argument >> >>> that is a constant. There are two callers of MLX5_SET64 that are >> >>> trying to get a variable offset, change those to call MLX5_SET64_VCHK >> >>> passing pas[0] as the argument to use in the offset check. >> >> >> >> I think I'd separate the array index instead. >> >> Something like: >> >> >> >> #define MLX5_SET64_INDEXED(typ, p, fld, ndx, v) do { \ >> >> BUILD_BUG_ON(__mlx5_bit_off(typ, fld) % 64); \ >> >> __MLX5_SET64(typ, p, fld[ndx], v); \ >> >> } while (0) >> >> >> >> David >> > >> > Yes, I think this looks more natural, but instead MLX5_SET64_INDEXED, >> > I prefer to have 2 macros >> > MLX5_SET64(typ, p, fld, v) and MLX5_ARRAY_SET64(typ, p, fld, idx, v). >> > >> > Tom, do you want me to fix it ? >> > >> Please do. > > Saeed, > > Do you success to send this patch before -rc1 is released? So Linus's > -rc1 will be clean from such build error. > Just submitted the patch, it seems that i have issues with my other Mailer, sometimes e-mails take a while to appear in the mailing list. I Hope the patch will arrive on time for Dave to pick it it up. Thanks, -Saeed.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c index d458515..5ef5ae2 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c @@ -287,7 +287,7 @@ retry: goto retry; } - MLX5_SET64(manage_pages_in, in, pas[i], addr); + MLX5_SET64_VCHK(manage_pages_in, in, pas[i], pas[0], addr); } MLX5_SET(manage_pages_in, in, opcode, MLX5_CMD_OP_MANAGE_PAGES); @@ -344,7 +344,8 @@ static int reclaim_pages_cmd(struct mlx5_core_dev *dev, if (fwp->func_id != func_id) continue; - MLX5_SET64(manage_pages_out, out, pas[i], fwp->addr); + MLX5_SET64_VCHK(manage_pages_out, out, pas[i], pas[0], + fwp->addr); i++; } diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h index 77c1417..943eb18 100644 --- a/include/linux/mlx5/device.h +++ b/include/linux/mlx5/device.h @@ -92,12 +92,21 @@ __mlx5_mask(typ, fld)) ___t; \ }) -#define MLX5_SET64(typ, p, fld, v) do { \ +#define __MLX5_SET64(typ, p, fld, v) do { \ BUILD_BUG_ON(__mlx5_bit_sz(typ, fld) != 64); \ - BUILD_BUG_ON(__mlx5_bit_off(typ, fld) % 64); \ *((__be64 *)(p) + __mlx5_64_off(typ, fld)) = cpu_to_be64(v); \ } while (0) +#define MLX5_SET64(typ, p, fld, v) do { \ + BUILD_BUG_ON(__mlx5_bit_off(typ, fld) % 64); \ + __MLX5_SET64(typ, p, fld, v); \ +} while (0) + +#define MLX5_SET64_VCHK(typ, p, fld, const_fld, v) do { \ + BUILD_BUG_ON(__mlx5_bit_off(typ, const_fld) % 64); \ + __MLX5_SET64(typ, p, fld, v); \ +} while (0) + #define MLX5_GET64(typ, p, fld) be64_to_cpu(*((__be64 *)(p) + __mlx5_64_off(typ, fld))) #define MLX5_GET64_PR(typ, p, fld) ({ \
I am hitting this in mlx5: drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c: In function ‘reclaim_pages_cmd.clone.0’: drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c:346: error: call to ‘__compiletime_assert_346’ declared with attribute error: BUILD_BUG_ON failed: __mlx5_bit_off(manage_pages_out, pas[i]) % 64 drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c: In function ‘give_pages’: drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c:291: error: call to ‘__compiletime_assert_291’ declared with attribute error: BUILD_BUG_ON failed: __mlx5_bit_off(manage_pages_in, pas[i]) % 64 Problem is that this is doing a BUILD_BUG_ON on a non-constant expression because of trying to take offset of pas[i] in the structure. Fix is to create MLX5_SET64_VCHK that takes an additional argument that is a constant. There are two callers of MLX5_SET64 that are trying to get a variable offset, change those to call MLX5_SET64_VCHK passing pas[0] as the argument to use in the offset check. Fixes: a533ed5e179cd ("net/mlx5: Pages management commands via mlx5 ifc") Signed-off-by: Tom Herbert <tom@herbertland.com> --- drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c | 5 +++-- include/linux/mlx5/device.h | 13 +++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-)