On Tue, Mar 13, 2018 at 3:38 PM, Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx> wrote: >> -----Original Message----- >> From: Matan Barak [mailto:matanb@xxxxxxxxxxxxxxxxxx] >> Sent: Sunday, March 11, 2018 12:35 PM >> To: Jason Gunthorpe <jgg@xxxxxxxx> >> Cc: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; Leon Romanovsky >> <leon@xxxxxxxxxx>; Doug Ledford <dledford@xxxxxxxxxx>; Leon >> Romanovsky <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux- >> rdma@xxxxxxxxxxxxxxx>; Aviad Yehezkel <aviadye@xxxxxxxxxxxx>; Boris >> Pismenny <borisp@xxxxxxxxxxxx>; Matan Barak <matanb@xxxxxxxxxxxx>; >> Yishai Hadas <yishaih@xxxxxxxxxxxx> >> Subject: Re: [PATCH rdma-next 02/12] IB/uverbs: Move to new headers and >> make naming consistent >> >> On Thu, Mar 8, 2018 at 11:45 PM, Jason Gunthorpe <jgg@xxxxxxxx> wrote: >> > On Thu, Mar 08, 2018 at 09:16:32PM +0000, Ruhl, Michael J wrote: >> >> > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- >> >> > owner@xxxxxxxxxxxxxxx] On Behalf Of Leon Romanovsky >> >> > Sent: Thursday, March 8, 2018 12:19 PM >> >> > To: Doug Ledford <dledford@xxxxxxxxxx>; Jason Gunthorpe >> >> > <jgg@xxxxxxxxxxxx> >> >> > Cc: Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA mailing list >> <linux- >> >> > rdma@xxxxxxxxxxxxxxx>; Aviad Yehezkel <aviadye@xxxxxxxxxxxx>; >> Boris >> >> > Pismenny <borisp@xxxxxxxxxxxx>; Matan Barak >> <matanb@xxxxxxxxxxxx>; >> >> > Yishai Hadas <yishaih@xxxxxxxxxxxx> >> >> > Subject: [PATCH rdma-next 02/12] IB/uverbs: Move to new headers and >> >> > make naming consistent >> >> > >> >> > From: Matan Barak <matanb@xxxxxxxxxxxx> >> >> > >> >> > Use macros to make names consistent in ioctl() uAPI: >> >> > The ioctl() uAPI works with object-method hierarchy. The method part >> >> > also states which handler should be executed when this method is >> called >> >> > from user-space. Therefore, we need to tie method, method's id, >> method's >> >> > handler and the object owning this method together. >> >> > Previously, this was done through explicit developer chosen names. >> >> > This makes grepping the code harder. Changing the method's name, >> >> > method's handler and object's name to be automatically generated >> based >> >> > on the ids. >> >> > >> >> > The headers are split in a way so they be included and used by >> >> > user-space. One header strictly contains structures that are used >> >> > directly by user-space applications, where another header is used for >> >> > internal library (i.e. libibverbs) to form the ioctl() commands. >> >> > Other header simply contains the required general command structure. >> >> > >> >> > Reviewed-by: Yishai Hadas <yishaih@xxxxxxxxxxxx> >> >> > Signed-off-by: Matan Barak <matanb@xxxxxxxxxxxx> >> >> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> >> >> > drivers/infiniband/core/uverbs_std_types.c | 139 ++++++++++++-- >> ------ >> >> > >> >> > diff --git a/drivers/infiniband/core/uverbs_std_types.c >> >> > b/drivers/infiniband/core/uverbs_std_types.c >> >> > index df1360e6774f..99f971b6cc67 100644 >> >> > +++ b/drivers/infiniband/core/uverbs_std_types.c >> >> > @@ -210,15 +210,24 @@ static int >> >> > uverbs_hot_unplug_completion_event_file(struct ib_uobject_file >> *uobj_ >> >> > return 0; >> >> > }; >> >> > >> >> > +#define UVERBS_METHOD(id) uverbs_method_##id >> >> > +#define UVERBS_HANDLER(id) uverbs_handler_##id >> >> > + >> >> > +#define DECLARE_COMMON_METHOD(id, ...) \ >> >> > + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, >> >> > UVERBS_HANDLER(id), ##__VA_ARGS__) >> >> > + >> >> > +#define DECLARE_COMMON_OBJECT(id, ...) \ >> >> > + DECLARE_UVERBS_OBJECT(UVERBS_OBJECT(id), id, >> >> > ##__VA_ARGS__) >> >> >> >> I applied this patch to my tree and noticed that the declarations for the >> >> METHODS and OBJECTS and HANDLERs had changed. >> >> >> >> I assumed that the above macros had been updated in the appropriate >> >> header files. When I tried to apply these "new" macros to my tree, I >> couldn't >> >> compile. >> >> >> >> So I cannot (easily) use the example of uverbs_std_verbs.c to set up my >> own >> >> stuff, and if I do, I have to copy the above macros to my files, or decipher >> these >> >> macros to figure out what I should use. >> > >> > Yeah, the normal DECLARE_UVERBS_METHOD/etc should just do this, we >> shouldn't >> > wrapper them in uvrebs_std_types.c >> > >> >> Ok, so this is the proposed change: >> >> diff --git a/include/rdma/uverbs_named_ioctl.h >> b/include/rdma/uverbs_named_ioctl.h >> new file mode 100644 >> index 000000000000..f9dff179a740 >> --- /dev/null >> +++ b/include/rdma/uverbs_named_ioctl.h >> @@ -0,0 +1,59 @@ >> +/* >> + * Copyright (c) 2018, Mellanox Technologies inc. All rights reserved. >> + * >> + * This software is available to you under a choice of one of two >> + * licenses. You may choose to be licensed under the terms of the GNU >> + * General Public License (GPL) Version 2, available from the file >> + * COPYING in the main directory of this source tree, or the >> + * OpenIB.org BSD license below: >> + * >> + * Redistribution and use in source and binary forms, with or >> + * without modification, are permitted provided that the following >> + * conditions are met: >> + * >> + * - Redistributions of source code must retain the above >> + * copyright notice, this list of conditions and the following >> + * disclaimer. >> + * >> + * - Redistributions in binary form must reproduce the above >> + * copyright notice, this list of conditions and the following >> + * disclaimer in the documentation and/or other materials >> + * provided with the distribution. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES >> OF >> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND >> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT >> HOLDERS >> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN >> AN >> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR >> IN >> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN >> THE >> + * SOFTWARE. >> + */ >> + >> +#ifndef _UVERBS_NAMED_IOCTL_ >> +#define _UVERBS_NAMED_IOCTL_ >> + >> +#include <rdma/uverbs_ioctl.h> >> + >> +#ifndef UVERBS_MODULE_NAME >> +#error "Please #define UVERBS_MODULE_NAME before including >> rdma/uverbs_named_ioctl.h" >> +#endif >> + >> +#define _UVERBS_PASTE(x, y) x ## y >> +#define _UVERBS_NAME(x, y) _UVERBS_PASTE(x, y) >> +#define UVERBS_METHOD(id) >> _UVERBS_NAME(UVERBS_MODULE_NAME, _method_##id) >> +#define UVERBS_HANDLER(id) >> _UVERBS_NAME(UVERBS_MODULE_NAME, _handler_##id) >> + >> +#define DECLARE_NAMED_METHOD(id, ...) \ >> + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, >> UVERBS_HANDLER(id), ##__VA_ARGS__) >> + >> +#define DECLARE_NAMED_METHOD_WITH_HANDLER(id, handler, ...) \ >> + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, handler, >> ##__VA_ARGS__) >> + >> +#define DECLARE_NAMED_METHOD_NO_HANDLER(id, handler, ...) \ > > Should there be a handler in this parameter list? > This is for the cases where the provider driver just want to add attributes to a common method. Previously, this was done by extending the ib_udata given to the provider driver callbacks. However, this makes it harder to extend and put more burden on the provider driver to validate. I wanted to use the standard parser to validate everything, that's why I preferred this approach. Therefore, when you call merge_trees(T1, T2), you'll get the handler of the last tree which is != NULL. >> + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, NULL, >> ##__VA_ARGS__) >> + >> + >> +#define DECLARE_NAMED_OBJECT(id, ...) \ >> + DECLARE_UVERBS_OBJECT(UVERBS_OBJECT(id), id, ##__VA_ARGS__) >> + >> +#endif >> >> In order to use this layer of macros, you'll have to define your >> module name (UVERBS_MODULE_NAME). >> The other names follow automatically. > > I am still trying to understand the mechanism that will allow me to "extend" > standard objects in a driver specific way (i.e. if I want to add new methods > to a standard object that are driver specific). > > I am assuming that I will include a declaration of the standard object in the > driver specific code, and then add the driver specific methods to it, and then > the spec_merge function will do its magic to put everything in place. > That's correct. > If this is the correct way to do this, then I would use the "unnamed" version > to specify the standard object in the drive code (and have to unraveled the > macro in the standard area to get the derived name)? > > Or am I misunderstanding this feature? > You can use the named one. You just have to make sure the id(s) are the same. The name parameter is only used for the macros reference. >> I first tried to use KBUILD_MODNAME, but since it's a stringifyed >> version of our module name, it doesn't work :( >> >> Later on I intend to try adding here some easy-to-use macros to >> declare a parsing tree with one argument (that extends one of the >> common methods). > > Is this easy-to-use macro going to "fix" my comment above? > Yeah, I just declared another UVERBS_MODULE_NAME macro that takes allows you to use the named version. > Thanks, > > Mike > >> Regards, Matan >> Are you ok with that? >> >> > Jason >> >> Matan >> >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> > the body of a message to majordomo@xxxxxxxxxxxxxxx >> > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
