| Message ID | 20180702172822.27827-1-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
On Mon, Jul 02, 2018 at 10:28:22AM -0700, Bart Van Assche wrote: > This patch avoids that sparse reports the following: > > drivers/infiniband/core/uverbs_std_types_cq.c:206:1: error: directive in argument list > drivers/infiniband/core/uverbs_std_types_cq.c:209:1: error: directive in argument list Huh. Yes, that should be fixed.. > Additionally, this patch also avoids that sparse warns about defined > but not used functions with CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI=n. But I don't think this should be.. With kconfig We want to compile as much code as possible and have the compiler eliminate it - so we get stable compilation coverage.. > Fixes: 185899ee8d00 ("IB/uverbs: Enable ioctl() uAPI by default for new verbs") > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Matan Barak <matanb@mellanox.com> > Cc: Leon Romanovsky <leonro@mellanox.com> > drivers/infiniband/core/uverbs_std_types_cq.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c > index 3d293d01afea..3fc1838d6cb1 100644 > +++ b/drivers/infiniband/core/uverbs_std_types_cq.c > @@ -53,6 +53,7 @@ static int uverbs_free_cq(struct ib_uobject *uobject, > return ret; > } > > +#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(struct ib_device *ib_dev, > struct ib_uverbs_file *file, > struct uverbs_attr_bundle *attrs) > @@ -199,13 +200,18 @@ static DECLARE_UVERBS_NAMED_METHOD(UVERBS_METHOD_CQ_DESTROY, > &UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_DESTROY_CQ_RESP, > UVERBS_ATTR_TYPE(struct ib_uverbs_destroy_cq_resp), > UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY))); > +#endif > > +#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_CQ, > &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0, > uverbs_free_cq), > -#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > &UVERBS_METHOD(UVERBS_METHOD_CQ_CREATE), > &UVERBS_METHOD(UVERBS_METHOD_CQ_DESTROY) > -#endif > ); > - > +#else > +DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_CQ, > + &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0, > + uverbs_free_cq), > + ); > +#endif Humm. I have a series that reworks these macros some more and eliminates the need for DECLARE_UVERBS_NAMED_OBJECT and handles CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI automatically. However maybe that is still a bit far off.. Do you think this is urgent to fix? Jason -- 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 Mon, Jul 02, 2018 at 10:28:22AM -0700, Bart Van Assche wrote: > This patch avoids that sparse reports the following: > > drivers/infiniband/core/uverbs_std_types_cq.c:206:1: error: directive in argument list > drivers/infiniband/core/uverbs_std_types_cq.c:209:1: error: directive in argument list > > Additionally, this patch also avoids that sparse warns about defined > but not used functions with CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI=n. > > Fixes: 185899ee8d00 ("IB/uverbs: Enable ioctl() uAPI by default for new verbs") > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Matan Barak <matanb@mellanox.com> > Cc: Leon Romanovsky <leonro@mellanox.com> > --- > drivers/infiniband/core/uverbs_std_types_cq.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) Bart, The patch looks good for me. Just for my general knowledge, can you point which part of C standard the original code violated? Thanks. > > diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c > index 3d293d01afea..3fc1838d6cb1 100644 > --- a/drivers/infiniband/core/uverbs_std_types_cq.c > +++ b/drivers/infiniband/core/uverbs_std_types_cq.c > @@ -53,6 +53,7 @@ static int uverbs_free_cq(struct ib_uobject *uobject, > return ret; > } > > +#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(struct ib_device *ib_dev, > struct ib_uverbs_file *file, > struct uverbs_attr_bundle *attrs) > @@ -199,13 +200,18 @@ static DECLARE_UVERBS_NAMED_METHOD(UVERBS_METHOD_CQ_DESTROY, > &UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_DESTROY_CQ_RESP, > UVERBS_ATTR_TYPE(struct ib_uverbs_destroy_cq_resp), > UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY))); > +#endif > > +#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_CQ, > &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0, > uverbs_free_cq), > -#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > &UVERBS_METHOD(UVERBS_METHOD_CQ_CREATE), > &UVERBS_METHOD(UVERBS_METHOD_CQ_DESTROY) > -#endif > ); > - > +#else > +DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_CQ, > + &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0, > + uverbs_free_cq), > + ); > +#endif > -- > 2.18.0 > > -- > 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 Tue, 2018-07-03 at 14:49 -0600, Jason Gunthorpe wrote: > On Mon, Jul 02, 2018 at 10:28:22AM -0700, Bart Van Assche wrote: > > This patch avoids that sparse reports the following: > > > > drivers/infiniband/core/uverbs_std_types_cq.c:206:1: error: directive in argument list > > drivers/infiniband/core/uverbs_std_types_cq.c:209:1: error: directive in argument list > > Huh. Yes, that should be fixed.. > > > Additionally, this patch also avoids that sparse warns about defined > > but not used functions with CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI=n. > > But I don't think this should be.. With kconfig We want to compile as > much code as possible and have the compiler eliminate it - so we get > stable compilation coverage.. > > > Fixes: 185899ee8d00 ("IB/uverbs: Enable ioctl() uAPI by default for new verbs") > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > > Cc: Matan Barak <matanb@mellanox.com> > > Cc: Leon Romanovsky <leonro@mellanox.com> > > drivers/infiniband/core/uverbs_std_types_cq.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c > > index 3d293d01afea..3fc1838d6cb1 100644 > > +++ b/drivers/infiniband/core/uverbs_std_types_cq.c > > @@ -53,6 +53,7 @@ static int uverbs_free_cq(struct ib_uobject *uobject, > > return ret; > > } > > > > +#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > > static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(struct ib_device *ib_dev, > > struct ib_uverbs_file *file, > > struct uverbs_attr_bundle *attrs) > > @@ -199,13 +200,18 @@ static DECLARE_UVERBS_NAMED_METHOD(UVERBS_METHOD_CQ_DESTROY, > > &UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_DESTROY_CQ_RESP, > > UVERBS_ATTR_TYPE(struct ib_uverbs_destroy_cq_resp), > > UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY))); > > +#endif > > > > +#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > > DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_CQ, > > &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0, > > uverbs_free_cq), > > -#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) > > &UVERBS_METHOD(UVERBS_METHOD_CQ_CREATE), > > &UVERBS_METHOD(UVERBS_METHOD_CQ_DESTROY) > > -#endif > > ); > > - > > +#else > > +DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_CQ, > > + &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0, > > + uverbs_free_cq), > > + ); > > +#endif > > Humm. > > I have a series that reworks these macros some more and eliminates the > need for DECLARE_UVERBS_NAMED_OBJECT and handles > CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI automatically. > > However maybe that is still a bit far off.. > > Do you think this is urgent to fix? No, this is not urgent to fix. I can keep this patch in my local tree until your work is ready to go upstream. This is something I came across while analyzing the sparse output for the RDMA code. Bart.
On Wed, 2018-07-04 at 08:27 +0300, Leon Romanovsky wrote: > On Mon, Jul 02, 2018 at 10:28:22AM -0700, Bart Van Assche wrote: > > This patch avoids that sparse reports the following: > > > > drivers/infiniband/core/uverbs_std_types_cq.c:206:1: error: directive in argument list > > drivers/infiniband/core/uverbs_std_types_cq.c:209:1: error: directive in argument list > > > > Additionally, this patch also avoids that sparse warns about defined > > but not used functions with CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI=n. > > > > Fixes: 185899ee8d00 ("IB/uverbs: Enable ioctl() uAPI by default for new verbs") > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > > Cc: Matan Barak <matanb@mellanox.com> > > Cc: Leon Romanovsky <leonro@mellanox.com> > > --- > > drivers/infiniband/core/uverbs_std_types_cq.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > Bart, > > The patch looks good for me. > Just for my general knowledge, can you point which part of C standard > the original code violated? Hello Leon, From ISO/IEC 9899:201x Committee Draft (N1570), section "6.10.3.4 Rescanning and further replacement": "The resulting completely macro-replaced preprocessing token sequence is not processed as a preprocessing directive even if it resembles one, but all pragma unary operator expressions within it are then processed as specified in 6.10.9 below." See also http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf. Bart.
On Thu, Jul 05, 2018 at 03:16:48PM +0000, Bart Van Assche wrote: > On Wed, 2018-07-04 at 08:27 +0300, Leon Romanovsky wrote: > > On Mon, Jul 02, 2018 at 10:28:22AM -0700, Bart Van Assche wrote: > > > This patch avoids that sparse reports the following: > > > > > > drivers/infiniband/core/uverbs_std_types_cq.c:206:1: error: directive in argument list > > > drivers/infiniband/core/uverbs_std_types_cq.c:209:1: error: directive in argument list > > > > > > Additionally, this patch also avoids that sparse warns about defined > > > but not used functions with CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI=n. > > > > > > Fixes: 185899ee8d00 ("IB/uverbs: Enable ioctl() uAPI by default for new verbs") > > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > > > Cc: Matan Barak <matanb@mellanox.com> > > > Cc: Leon Romanovsky <leonro@mellanox.com> > > > --- > > > drivers/infiniband/core/uverbs_std_types_cq.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > Bart, > > > > The patch looks good for me. > > Just for my general knowledge, can you point which part of C standard > > the original code violated? > > Hello Leon, > > From ISO/IEC 9899:201x Committee Draft (N1570), section "6.10.3.4 Rescanning > and further replacement": "The resulting completely macro-replaced preprocessing > token sequence is not processed as a preprocessing directive even if it resembles > one, but all pragma unary operator expressions within it are then processed as > specified in 6.10.9 below." > > See also http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf. Thanks Bart > > Bart. >
diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c b/drivers/infiniband/core/uverbs_std_types_cq.c index 3d293d01afea..3fc1838d6cb1 100644 --- a/drivers/infiniband/core/uverbs_std_types_cq.c +++ b/drivers/infiniband/core/uverbs_std_types_cq.c @@ -53,6 +53,7 @@ static int uverbs_free_cq(struct ib_uobject *uobject, return ret; } +#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) static int UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(struct ib_device *ib_dev, struct ib_uverbs_file *file, struct uverbs_attr_bundle *attrs) @@ -199,13 +200,18 @@ static DECLARE_UVERBS_NAMED_METHOD(UVERBS_METHOD_CQ_DESTROY, &UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_DESTROY_CQ_RESP, UVERBS_ATTR_TYPE(struct ib_uverbs_destroy_cq_resp), UA_FLAGS(UVERBS_ATTR_SPEC_F_MANDATORY))); +#endif +#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_CQ, &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0, uverbs_free_cq), -#if IS_ENABLED(CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI) &UVERBS_METHOD(UVERBS_METHOD_CQ_CREATE), &UVERBS_METHOD(UVERBS_METHOD_CQ_DESTROY) -#endif ); - +#else +DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_CQ, + &UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0, + uverbs_free_cq), + ); +#endif
This patch avoids that sparse reports the following: drivers/infiniband/core/uverbs_std_types_cq.c:206:1: error: directive in argument list drivers/infiniband/core/uverbs_std_types_cq.c:209:1: error: directive in argument list Additionally, this patch also avoids that sparse warns about defined but not used functions with CONFIG_INFINIBAND_EXP_LEGACY_VERBS_NEW_UAPI=n. Fixes: 185899ee8d00 ("IB/uverbs: Enable ioctl() uAPI by default for new verbs") Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: Matan Barak <matanb@mellanox.com> Cc: Leon Romanovsky <leonro@mellanox.com> --- drivers/infiniband/core/uverbs_std_types_cq.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)