| Message ID | 20170207174600.27218-1-leon@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Works for me. Tested-by: Slava Shwartsman <slavash@mellanox.com> On Tue, Feb 7, 2017 at 7:46 PM, Leon Romanovsky <leon@kernel.org> wrote: > Remove references to private kernel header and defines from exported > ib_user_verb.h file. > > The code snippet below is used to reproduce the issue: > > #include <stdio.h> > #include <rdma/ib_user_verb.h> > > int main(void) > { > printf("IB_USER_VERBS_ABI_VERSION = %d\n", IB_USER_VERBS_ABI_VERSION); > return 0; > } > > It fails during compilation phase with an error: > ➜ /tmp gcc main.c > main.c:2:31: fatal error: rdma/ib_user_verb.h: No such file or directory > #include <rdma/ib_user_verb.h> > ^ > compilation terminated. > > Fixes: 189aba99e700 ("IB/uverbs: Extend modify_qp and support packet pacing") > CC: Bodong Wang <bodong@mellanox.com> > CC: Matan Barak <matanb@mellanox.com> > Reported-by: Slava Shwartsman <slavash@mellanox.com> > Signed-off-by: Leon Romanovsky <leon@kernel.org> > --- > include/uapi/rdma/ib_user_verbs.h | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h > index dfdfe4e92d31..c56f525b041d 100644 > --- a/include/uapi/rdma/ib_user_verbs.h > +++ b/include/uapi/rdma/ib_user_verbs.h > @@ -37,7 +37,6 @@ > #define IB_USER_VERBS_H > > #include <linux/types.h> > -#include <rdma/ib_verbs.h> > > /* > * Increment this value if any changes that break userspace ABI > @@ -548,11 +547,11 @@ enum { > }; > > enum { > - IB_USER_LEGACY_LAST_QP_ATTR_MASK = IB_QP_DEST_QPN > + IB_USER_LEGACY_LAST_QP_ATTR_MASK = 1ULL << 20, > }; > > enum { > - IB_USER_LAST_QP_ATTR_MASK = IB_QP_RATE_LIMIT > + IB_USER_LAST_QP_ATTR_MASK = 1ULL << 25, > }; > > struct ib_uverbs_ex_create_qp { > -- > 2.11.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> #include <linux/types.h> > -#include <rdma/ib_verbs.h> > > /* > * Increment this value if any changes that break userspace ABI > @@ -548,11 +547,11 @@ enum { > }; > > enum { > - IB_USER_LEGACY_LAST_QP_ATTR_MASK = IB_QP_DEST_QPN > + IB_USER_LEGACY_LAST_QP_ATTR_MASK = 1ULL << 20, > }; > > enum { > - IB_USER_LAST_QP_ATTR_MASK = IB_QP_RATE_LIMIT > + IB_USER_LAST_QP_ATTR_MASK = 1ULL << 25, I disagree with the upendcoding. These constant should be moved to the user verbs header instead. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 08, 2017 at 12:28:50AM -0800, Christoph Hellwig wrote: > > #include <linux/types.h> > > -#include <rdma/ib_verbs.h> > > > > /* > > * Increment this value if any changes that break userspace ABI > > @@ -548,11 +547,11 @@ enum { > > }; > > > > enum { > > - IB_USER_LEGACY_LAST_QP_ATTR_MASK = IB_QP_DEST_QPN > > + IB_USER_LEGACY_LAST_QP_ATTR_MASK = 1ULL << 20, > > }; > > > > enum { > > - IB_USER_LAST_QP_ATTR_MASK = IB_QP_RATE_LIMIT > > + IB_USER_LAST_QP_ATTR_MASK = 1ULL << 25, > > I disagree with the upendcoding. These constant should be moved to > the user verbs header instead. These constants are part of much larger enum ib_qp_attr_mask. IMHO copy two values from that enum isn't good, but copy whole enum (mostly not needed for the users) is bad either. So I decided to open code it as a fix for -rc7. Thanks
On Wed, Feb 08, 2017 at 12:00:43PM +0200, Leon Romanovsky wrote: > These constants are part of much larger enum ib_qp_attr_mask. IMHO copy > two values from that enum isn't good, but copy whole enum (mostly not > needed for the users) is bad either. So I decided to open code it as a fix > for -rc7. At least add comments mentioning the proper symbolic values then. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/02/2017 12:00, Leon Romanovsky wrote: > On Wed, Feb 08, 2017 at 12:28:50AM -0800, Christoph Hellwig wrote: >>> #include <linux/types.h> >>> -#include <rdma/ib_verbs.h> >>> >>> /* >>> * Increment this value if any changes that break userspace ABI >>> @@ -548,11 +547,11 @@ enum { >>> }; >>> >>> enum { >>> - IB_USER_LEGACY_LAST_QP_ATTR_MASK = IB_QP_DEST_QPN >>> + IB_USER_LEGACY_LAST_QP_ATTR_MASK = 1ULL << 20, >>> }; >>> >>> enum { >>> - IB_USER_LAST_QP_ATTR_MASK = IB_QP_RATE_LIMIT >>> + IB_USER_LAST_QP_ATTR_MASK = 1ULL << 25, >> >> I disagree with the upendcoding. These constant should be moved to >> the user verbs header instead. > > These constants are part of much larger enum ib_qp_attr_mask. IMHO copy > two values from that enum isn't good, but copy whole enum (mostly not > needed for the users) is bad either. So I decided to open code it as a fix > for -rc7. > > Thanks > IMHO, since these enum values are actually passed between user-space to kernel (attr_mask), it's acceptable to expose all enum values. Otherwise, the user-space should define all these symbols by by itself, so why bother introduce only this explicit symbol? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 08, 2017 at 03:53:49AM -0800, Christoph Hellwig wrote: > On Wed, Feb 08, 2017 at 12:00:43PM +0200, Leon Romanovsky wrote: > > These constants are part of much larger enum ib_qp_attr_mask. IMHO copy > > two values from that enum isn't good, but copy whole enum (mostly not > > needed for the users) is bad either. So I decided to open code it as a fix > > for -rc7. > > At least add comments mentioning the proper symbolic values then. Sure, I'll send an update. Thanks
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h index dfdfe4e92d31..c56f525b041d 100644 --- a/include/uapi/rdma/ib_user_verbs.h +++ b/include/uapi/rdma/ib_user_verbs.h @@ -37,7 +37,6 @@ #define IB_USER_VERBS_H #include <linux/types.h> -#include <rdma/ib_verbs.h> /* * Increment this value if any changes that break userspace ABI @@ -548,11 +547,11 @@ enum { }; enum { - IB_USER_LEGACY_LAST_QP_ATTR_MASK = IB_QP_DEST_QPN + IB_USER_LEGACY_LAST_QP_ATTR_MASK = 1ULL << 20, }; enum { - IB_USER_LAST_QP_ATTR_MASK = IB_QP_RATE_LIMIT + IB_USER_LAST_QP_ATTR_MASK = 1ULL << 25, }; struct ib_uverbs_ex_create_qp {
Remove references to private kernel header and defines from exported ib_user_verb.h file. The code snippet below is used to reproduce the issue: #include <stdio.h> #include <rdma/ib_user_verb.h> int main(void) { printf("IB_USER_VERBS_ABI_VERSION = %d\n", IB_USER_VERBS_ABI_VERSION); return 0; } It fails during compilation phase with an error: ➜ /tmp gcc main.c main.c:2:31: fatal error: rdma/ib_user_verb.h: No such file or directory #include <rdma/ib_user_verb.h> ^ compilation terminated. Fixes: 189aba99e700 ("IB/uverbs: Extend modify_qp and support packet pacing") CC: Bodong Wang <bodong@mellanox.com> CC: Matan Barak <matanb@mellanox.com> Reported-by: Slava Shwartsman <slavash@mellanox.com> Signed-off-by: Leon Romanovsky <leon@kernel.org> --- include/uapi/rdma/ib_user_verbs.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) -- 2.11.1 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html