| Message ID | 1471987907-6336-7-git-send-email-leon@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
T24gV2VkLCAyMDE2LTA4LTI0IGF0IDAwOjMxICswMzAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6 DQo+IEZyb206IExlb24gUm9tYW5vdnNreSA8bGVvbnJvQG1lbGxhbm94LmNvbT4NCj7CoA0KPiAt I2RlZmluZSBIRkkxX0NNRF9BU1NJR05fQ1RYVMKgwqDCoMKgwqAxCS8qIGFsbG9jYXRlIEhGSSBh bmQNCj4gY29udGV4dCAqLw0KPiAtI2RlZmluZSBIRkkxX0NNRF9DVFhUX0lORk/CoMKgwqDCoMKg wqDCoDIJLyogZmluZCBvdXQgd2hhdCByZXNvdXJjZXMNCj4gd2UgZ290ICovDQo+IC0jZGVmaW5l IEhGSTFfQ01EX1VTRVJfSU5GT8KgwqDCoMKgwqDCoMKgMwkvKiBzZXQgdXAgdXNlcnNwYWNlICov DQo+IC0jZGVmaW5lIEhGSTFfQ01EX1RJRF9VUERBVEXCoMKgwqDCoMKgwqA0CS8qIHVwZGF0ZSBl eHBlY3RlZCBUSUQNCj4gZW50cmllcyAqLw0KPiAtI2RlZmluZSBIRkkxX0NNRF9USURfRlJFRcKg wqDCoMKgwqDCoMKgwqA1CS8qIGZyZWUgZXhwZWN0ZWQgVElEDQo+IGVudHJpZXMgKi8NCj4gLSNk ZWZpbmUgSEZJMV9DTURfQ1JFRElUX1VQRMKgwqDCoMKgwqDCoDYJLyogZm9yY2UgYW4gdXBkYXRl IG9mIFBJTw0KPiBjcmVkaXQgKi8NCj4gKy8qIGFsbG9jYXRlIEhGSSBhbmQgY29udGV4dCAqLw0K PiArI2RlZmluZSBIRkkxX0NNRF9BU1NJR05fQ1RYVMKgwqDCoMKgwqAoSEZJMV9DTURfQkFTRSAr IDB4MDEpDQo+ICsvKiBmaW5kIG91dCB3aGF0IHJlc291cmNlcyB3ZSBnb3QgKi8NCj4gKyNkZWZp bmUgSEZJMV9DTURfQ1RYVF9JTkZPwqDCoMKgwqDCoMKgwqAoSEZJMV9DTURfQkFTRSArIDB4MDIp DQo+ICsvKiBzZXQgdXAgdXNlcnNwYWNlICovDQo+ICsjZGVmaW5lIEhGSTFfQ01EX1VTRVJfSU5G T8KgwqDCoMKgwqDCoMKgKEhGSTFfQ01EX0JBU0UgKyAweDAzKQ0KPiArLyogdXBkYXRlIGV4cGVj dGVkIFRJRCBlbnRyaWVzICovDQo+ICsjZGVmaW5lIEhGSTFfQ01EX1RJRF9VUERBVEXCoMKgwqDC oMKgwqAoSEZJMV9DTURfQkFTRSArIDB4MDQpDQo+ICsvKiBmcmVlIGV4cGVjdGVkIFRJRCBlbnRy aWVzICovDQo+ICsjZGVmaW5lIEhGSTFfQ01EX1RJRF9GUkVFwqDCoMKgwqDCoMKgwqDCoChIRkkx X0NNRF9CQVNFICsgMHgwNSkNCj4gKy8qIGZvcmNlIGFuIHVwZGF0ZSBvZiBQSU8gY3JlZGl0ICov DQo+ICsjZGVmaW5lIEhGSTFfQ01EX0NSRURJVF9VUETCoMKgwqDCoMKgwqAoSEZJMV9DTURfQkFT RSArIDB4MDYpDQoNClRoaXMgaXMgYSBtaW5vciBpc3N1ZSwgYnV0IHRoZSBwcm9ibGVtIGhlcmUg aXMgd2hlbiB3ZSBidWlsZCBQU00NCmFnYWluc3QgdGhpcyBrZXJuZWwgaXQgd2lsbCBubyBsb25n ZXIgd29yayBmb3Igb2xkZXIga2VybmVscyBiZWNhdXNlDQp0aGUgdmFsdWUgb2YgSEZJMV9DTURf QVNTSUdOX0NUWFQgaGFzIGNoYW5nZWQgd2hlcmUgYXMgaXQgdXNlZCB0byBiZSAxLg0KUmlnaHQg bm93IFBTTSBpcyBiYWNrd2FyZHMgY29tcGF0aWJsZSwgdGhpcyBicmVha3MgdGhhdCBjb21wYXRp YmlsaXR5Lg0KDQpTbyB3aGlsZSBubyBvbmUgdXNlcyB0aGUgX19OVU0oKSBtYWNybyBkaXJlY3Rs eSBpdCBsZXRzIHVzIG5vdCBjaGFuZ2UNCnRoZSBQU00gY29tbWFuZCB2YWx1ZXMuIENhbiB3ZSBw dXQgdGhhdCBwYXJ0IGJhY2sgYW5kIGtlZXAgdGhlIGNvbW1hbmQNCnZhbHVlcyB1bmNoYW5nZWQ/ DQoNCi1EZW5ueQ0K -- 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 Thu, Sep 01, 2016 at 02:05:44PM +0000, Dalessandro, Dennis wrote: > On Wed, 2016-08-24 at 00:31 +0300, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@mellanox.com> > > > > -#define HFI1_CMD_ASSIGN_CTXT 1 /* allocate HFI and > > context */ > > -#define HFI1_CMD_CTXT_INFO 2 /* find out what resources > > we got */ > > -#define HFI1_CMD_USER_INFO 3 /* set up userspace */ > > -#define HFI1_CMD_TID_UPDATE 4 /* update expected TID > > entries */ > > -#define HFI1_CMD_TID_FREE 5 /* free expected TID > > entries */ > > -#define HFI1_CMD_CREDIT_UPD 6 /* force an update of PIO > > credit */ > > +/* allocate HFI and context */ > > +#define HFI1_CMD_ASSIGN_CTXT (HFI1_CMD_BASE + 0x01) > > +/* find out what resources we got */ > > +#define HFI1_CMD_CTXT_INFO (HFI1_CMD_BASE + 0x02) > > +/* set up userspace */ > > +#define HFI1_CMD_USER_INFO (HFI1_CMD_BASE + 0x03) > > +/* update expected TID entries */ > > +#define HFI1_CMD_TID_UPDATE (HFI1_CMD_BASE + 0x04) > > +/* free expected TID entries */ > > +#define HFI1_CMD_TID_FREE (HFI1_CMD_BASE + 0x05) > > +/* force an update of PIO credit */ > > +#define HFI1_CMD_CREDIT_UPD (HFI1_CMD_BASE + 0x06) > > This is a minor issue, but the problem here is when we build PSM > against this kernel it will no longer work for older kernels because > the value of HFI1_CMD_ASSIGN_CTXT has changed where as it used to be 1. > Right now PSM is backwards compatible, this breaks that compatibility. > > So while no one uses the __NUM() macro directly it lets us not change > the PSM command values. Can we put that part back and keep the command > values unchanged? Sure, I'll post new version. Just to be sure, you are not using __NUM() and I can redefine that macro, am I right? Thanks. > > -Denny
On Thu, Sep 01, 2016 at 02:05:44PM +0000, Dalessandro, Dennis wrote: > > +/* allocate HFI and context */ > > +#define HFI1_CMD_ASSIGN_CTXT (HFI1_CMD_BASE + 0x01) > > This is a minor issue, but the problem here is when we build PSM > against this kernel it will no longer work for older kernels because > the value of HFI1_CMD_ASSIGN_CTXT has changed where as it used to be 1. > Right now PSM is backwards compatible, this breaks that compatibility. What? Someone renumberd an ioctl? When? Why? How? Was this ioctl ever in a mainline non-staging kernel? If not, too bad, deal with it in your user space.. If yes, can we revert the renumbering? Why was that even done?? > So while no one uses the __NUM() macro directly it lets us not change > the PSM command values. Can we put that part back and keep the command > values unchanged? Please no, do not do crazy subtle things like this. If the ioctl has two valid numbers then you need two entries in the ioctl file. 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 Thu, 2016-09-01 at 19:20 +0300, Leon Romanovsky wrote: > On Thu, Sep 01, 2016 at 02:05:44PM +0000, Dalessandro, Dennis wrote: > > On Wed, 2016-08-24 at 00:31 +0300, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > > > -#define HFI1_CMD_ASSIGN_CTXT 1 /* allocate HFI and > > > context */ > > > -#define HFI1_CMD_CTXT_INFO 2 /* find out what > > > resources > > > we got */ > > > -#define HFI1_CMD_USER_INFO 3 /* set up userspace */ > > > -#define HFI1_CMD_TID_UPDATE 4 /* update expected TID > > > entries */ > > > -#define HFI1_CMD_TID_FREE 5 /* free expected TID > > > entries */ > > > -#define HFI1_CMD_CREDIT_UPD 6 /* force an update of > > > PIO > > > credit */ > > > +/* allocate HFI and context */ > > > +#define HFI1_CMD_ASSIGN_CTXT (HFI1_CMD_BASE + 0x01) > > > +/* find out what resources we got */ > > > +#define HFI1_CMD_CTXT_INFO (HFI1_CMD_BASE + 0x02) > > > +/* set up userspace */ > > > +#define HFI1_CMD_USER_INFO (HFI1_CMD_BASE + 0x03) > > > +/* update expected TID entries */ > > > +#define HFI1_CMD_TID_UPDATE (HFI1_CMD_BASE + 0x04) > > > +/* free expected TID entries */ > > > +#define HFI1_CMD_TID_FREE (HFI1_CMD_BASE + 0x05) > > > +/* force an update of PIO credit */ > > > +#define HFI1_CMD_CREDIT_UPD (HFI1_CMD_BASE + 0x06) > > > > This is a minor issue, but the problem here is when we build PSM > > against this kernel it will no longer work for older kernels > > because > > the value of HFI1_CMD_ASSIGN_CTXT has changed where as it used to > > be 1. > > Right now PSM is backwards compatible, this breaks that > > compatibility. > > > > So while no one uses the __NUM() macro directly it lets us not > > change > > the PSM command values. Can we put that part back and keep the > > command > > values unchanged? > > Sure, I'll post new version. > Just to be sure, you are not using __NUM() and I can redefine that > macro, am I right? > Yeah you can call it something else, or do it in a different way. I verified that the patch works with the current PSM reverting the CMD numbers. I'll grab your new patch and re-test it then. Thanks -Denny
On Thu, Sep 01, 2016 at 10:46:24AM -0600, Jason Gunthorpe wrote: > On Thu, Sep 01, 2016 at 02:05:44PM +0000, Dalessandro, Dennis wrote: > > > > +/* allocate HFI and context */ > > > +#define HFI1_CMD_ASSIGN_CTXT (HFI1_CMD_BASE + 0x01) > > > > This is a minor issue, but the problem here is when we build PSM > > against this kernel it will no longer work for older kernels because > > the value of HFI1_CMD_ASSIGN_CTXT has changed where as it used to be 1. > > Right now PSM is backwards compatible, this breaks that compatibility. > > What? Someone renumberd an ioctl? When? Why? How? > > Was this ioctl ever in a mainline non-staging kernel? If not, too bad, > deal with it in your user space.. > > If yes, can we revert the renumbering? Why was that even done?? > > > So while no one uses the __NUM() macro directly it lets us not change > > the PSM command values. Can we put that part back and keep the command > > values unchanged? > > Please no, do not do crazy subtle things like this. If the ioctl has > two valid numbers then you need two entries in the ioctl file. I didn't renumbered ioctls, but renumbered one of the internals number which is not used in kernel, but for any reasons used in their user-space. ➜ linux-rdma git:(master) grep -r HFI1_CMD_ASSIGN_CTXT drivers/infiniband/hw/hfi1/* include/* include/uapi/rdma/hfi/hfi1_user.h:#define HFI1_CMD_ASSIGN_CTXT 1 /* allocate HFI and context */ So what should I do? respin or not? > > Jason
On Thu, 2016-09-01 at 10:46 -0600, Jason Gunthorpe wrote: > On Thu, Sep 01, 2016 at 02:05:44PM +0000, Dalessandro, Dennis wrote: > > > > +/* allocate HFI and context */ > > > +#define HFI1_CMD_ASSIGN_CTXT (HFI1_CMD_BASE + 0x01) > > > > This is a minor issue, but the problem here is when we build PSM > > against this kernel it will no longer work for older kernels > > because > > the value of HFI1_CMD_ASSIGN_CTXT has changed where as it used to > > be 1. > > Right now PSM is backwards compatible, this breaks that > > compatibility. > > What? Someone renumberd an ioctl? When? Why? How? No ioctl number was still the same. It's the thing that is used to define the ioctl number. The ioctl was originally defined as: _IOWR(RDMA_IOCTL_MAGIC, __NUM(ASSIGN_CTXT), struct hfi1_user_info) Where __NUM(ASSIGN_CTXT) = HFI1_CMD_ASSIGN_CTXT + 0xE0 = 1 + 0xE0 It was changed to be: _IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_ASSIGN_CTXT, struct hfi1_user_info) Where HFI1_CMD_ASSIGN_CTXT = 0xE1. So the ioctl number doens't change. The value of HFI1_CMD_ASSIGN_CTXT does, it was 1 now it is 0xE1. HFI1_CMD_ASSIGN_CTXT is used by userspace. -Denny
T24gVGh1LCAyMDE2LTA5LTAxIGF0IDE5OjU1ICswMzAwLCBMZW9uIFJvbWFub3Zza3kgd3JvdGU6 DQo+wqANCj4gSSBkaWRuJ3QgcmVudW1iZXJlZCBpb2N0bHMsIGJ1dCByZW51bWJlcmVkIG9uZSBv ZiB0aGUgaW50ZXJuYWxzDQo+IG51bWJlcg0KPiB3aGljaCBpcyBub3QgdXNlZCBpbiBrZXJuZWws IGJ1dCBmb3IgYW55IHJlYXNvbnMgdXNlZCBpbiB0aGVpcg0KPiB1c2VyLXNwYWNlLg0KPiANCj4g 4p6cwqDCoGxpbnV4LXJkbWEgZ2l0OihtYXN0ZXIpIGdyZXAgLXIgSEZJMV9DTURfQVNTSUdOX0NU WFQNCj4gZHJpdmVycy9pbmZpbmliYW5kL2h3L2hmaTEvKiBpbmNsdWRlLyoNCj4gaW5jbHVkZS91 YXBpL3JkbWEvaGZpL2hmaTFfdXNlci5oOiNkZWZpbmUgSEZJMV9DTURfQVNTSUdOX0NUWFTCoMKg wqDCoMKgMQ0KPiAvKiBhbGxvY2F0ZSBIRkkgYW5kIGNvbnRleHQgKi8NCj4gDQo+IFNvIHdoYXQg c2hvdWxkIEkgZG8/IHJlc3BpbiBvciBub3Q/DQo+IA0KDQpJJ2QgcmF0aGVyIG5vdCBoYXZlIHRv IGdvIHJldG9vbCBQU00gZm9yIHNvbWV0aGluZyB0aGF0IHNob3VsZG4ndA0KcmVhbGx5IG1hdHRl ciB0byB0aGUga2VybmVsLiBTbyB3ZSdkIGFwcHJlY2lhdGUga2VlcGluZyB0aGUNCkhGSTFfQ01E X0FTU0lHTl9DVFhUIGFuZCBmcmllbmRzIHZhbHVlcyB0aGUgc2FtZS4NCg0KLURlbm55 -- 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 Thu, Sep 01, 2016 at 07:55:52PM +0300, Leon Romanovsky wrote: > On Thu, Sep 01, 2016 at 10:46:24AM -0600, Jason Gunthorpe wrote: > > On Thu, Sep 01, 2016 at 02:05:44PM +0000, Dalessandro, Dennis wrote: > > > > > > +/* allocate HFI and context */ > > > > +#define HFI1_CMD_ASSIGN_CTXT??????????(HFI1_CMD_BASE + 0x01) > > > > > > This is a minor issue, but the problem here is when we build PSM > > > against this kernel it will no longer work for older kernels because > > > the value of HFI1_CMD_ASSIGN_CTXT has changed where as it used to be 1. > > > Right now PSM is backwards compatible, this breaks that compatibility. > > > > What? Someone renumberd an ioctl? When? Why? How? > > > > Was this ioctl ever in a mainline non-staging kernel? If not, too bad, > > deal with it in your user space.. > > > > If yes, can we revert the renumbering? Why was that even done?? > > > > > So while no one uses the __NUM() macro directly it lets us not change > > > the PSM command values. Can we put that part back and keep the command > > > values unchanged? > > > > Please no, do not do crazy subtle things like this. If the ioctl has > > two valid numbers then you need two entries in the ioctl file. > > I didn't renumbered ioctls, but renumbered one of the internals number > which is not used in kernel, but for any reasons used in their > user-space. If it is not used in the kernel why is it in this header? Ah, I see, this was part of the staging clean up, this is part of the old non-ioctl UAPI that got trashed ?? > ??? linux-rdma git:(master) grep -r HFI1_CMD_ASSIGN_CTXT drivers/infiniband/hw/hfi1/* include/* > include/uapi/rdma/hfi/hfi1_user.h:#define HFI1_CMD_ASSIGN_CTXT 1 /* allocate HFI and context */ > > So what should I do? respin or not? Drop the cruft. These header file definitions should have been deleted when the API was dropped during the staging review. We do not promise anything for drivers during their time in staging, and we do not support ABIs that only existed during staging. Dennis should use an internal definition in PSM if he wishes to continue to support the staging kernel ABI. 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 Thu, 2016-09-01 at 11:07 -0600, Jason Gunthorpe wrote: > On Thu, Sep 01, 2016 at 07:55:52PM +0300, Leon Romanovsky wrote: > > On Thu, Sep 01, 2016 at 10:46:24AM -0600, Jason Gunthorpe wrote: > > > On Thu, Sep 01, 2016 at 02:05:44PM +0000, Dalessandro, Dennis > > > wrote: > > > > > > > > +/* allocate HFI and context */ > > > > > +#define HFI1_CMD_ASSIGN_CTXT??????????(HFI1_CMD_BASE + 0x01) > > > > > > > > This is a minor issue, but the problem here is when we build > > > > PSM > > > > against this kernel it will no longer work for older kernels > > > > because > > > > the value of HFI1_CMD_ASSIGN_CTXT has changed where as it used > > > > to be 1. > > > > Right now PSM is backwards compatible, this breaks that > > > > compatibility. > > > > > > What? Someone renumberd an ioctl? When? Why? How? > > > > > > Was this ioctl ever in a mainline non-staging kernel? If not, too > > > bad, > > > deal with it in your user space.. > > > > > > If yes, can we revert the renumbering? Why was that even done?? > > > > > > > So while no one uses the __NUM() macro directly it lets us not > > > > change > > > > the PSM command values. Can we put that part back and keep the > > > > command > > > > values unchanged? > > > > > > Please no, do not do crazy subtle things like this. If the ioctl > > > has > > > two valid numbers then you need two entries in the ioctl file. > > > > I didn't renumbered ioctls, but renumbered one of the internals > > number > > which is not used in kernel, but for any reasons used in their > > user-space. > > If it is not used in the kernel why is it in this header? > > Ah, I see, this was part of the staging clean up, this is part of the > old non-ioctl UAPI that got trashed ?? > > > ??? linux-rdma git:(master) grep -r HFI1_CMD_ASSIGN_CTXT > > drivers/infiniband/hw/hfi1/* include/* > > include/uapi/rdma/hfi/hfi1_user.h:#define > > HFI1_CMD_ASSIGN_CTXT 1 /* allocate HFI and context */ > > > > So what should I do? respin or not? > > Drop the cruft. These header file definitions should have been > deleted > when the API was dropped during the staging review. > > We do not promise anything for drivers during their time in staging, > and we do not support ABIs that only existed during staging. > > Dennis should use an internal definition in PSM if he wishes to > continue to support the staging kernel ABI. It's not just the backward compatibility. PSM uses these command definitions.So this breaks current support with current driver. -Denny
On Thu, Sep 01, 2016 at 05:09:31PM +0000, Dalessandro, Dennis wrote: > > Dennis should use an internal definition in PSM if he wishes to > > continue to support the staging kernel ABI. > > It's not just the backward compatibility. PSM uses these command > definitions.So this breaks current support with current driver. How exactly? The HFI1_CMD_ASSIGN_CTXT constant is never used in the kernel? 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 Thu, 2016-09-01 at 11:11 -0600, Jason Gunthorpe wrote: > On Thu, Sep 01, 2016 at 05:09:31PM +0000, Dalessandro, Dennis wrote: > > > > Dennis should use an internal definition in PSM if he wishes to > > > continue to support the staging kernel ABI. > > > > It's not just the backward compatibility. PSM uses these command > > definitions.So this breaks current support with current driver. > > How exactly? The HFI1_CMD_ASSIGN_CTXT constant is never used in the > kernel? This is used in PSM library. Agree it's not used in the kernel, so I can see the argument to get rid of it, or not care. However removing/changing it breaks PSM and requires changes. They are not hard changes to make, but as it stands now PSM will work with 4.7 but not with 4.8. Reverting this part of the patch doesn't change or detract from what the series does in the first place. Patch still accomplishes its job. -Denny
On Thu, Sep 01, 2016 at 05:17:26PM +0000, Dalessandro, Dennis wrote: > On Thu, 2016-09-01 at 11:11 -0600, Jason Gunthorpe wrote: > > On Thu, Sep 01, 2016 at 05:09:31PM +0000, Dalessandro, Dennis wrote: > > > > > > Dennis should use an internal definition in PSM if he wishes to > > > > continue to support the staging kernel ABI. > > > > > > It's not just the backward compatibility. PSM uses these command > > > definitions.So this breaks current support with current driver. > > > > How exactly? The HFI1_CMD_ASSIGN_CTXT constant is never used in the > > kernel? > > This is used in PSM library. Agree it's not used in the kernel, so I > can see the argument to get rid of it, or not care. Lets get rid of all the #defines. Leon you should just inline the ioctl numbers into the ioctl definition like normal and get rid of this extra layer of macros. This way Dennis will get a build failure when PSM is build with these headers instead of subtle runtime breakage. He can import the required definitions to support the staging compat ABI into PSM, where they belong, and make a new release to build with new kernel headers. Lots of time to do that before these headers hit the distros. Upstream is not the place to carry that stuff, and keeping strange subtleness with __NUM is just going to risk future breakage. 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 Thu, Sep 01, 2016 at 11:33:20AM -0600, Jason Gunthorpe wrote: > On Thu, Sep 01, 2016 at 05:17:26PM +0000, Dalessandro, Dennis wrote: > > On Thu, 2016-09-01 at 11:11 -0600, Jason Gunthorpe wrote: > > > On Thu, Sep 01, 2016 at 05:09:31PM +0000, Dalessandro, Dennis wrote: > > > > > > > > Dennis should use an internal definition in PSM if he wishes to > > > > > continue to support the staging kernel ABI. > > > > > > > > It's not just the backward compatibility. PSM uses these command > > > > definitions.So this breaks current support with current driver. > > > > > > How exactly? The HFI1_CMD_ASSIGN_CTXT constant is never used in the > > > kernel? > > > > This is used in PSM library. Agree it's not used in the kernel, so I > > can see the argument to get rid of it, or not care. > > Lets get rid of all the #defines. Leon you should just inline the > ioctl numbers into the ioctl definition like normal and get rid of > this extra layer of macros. Ohh, it looks like you returned to us filled with positive energy :) I tried to follow DRM subsystem [1] while converted these names with minimal impact on the current implementation. > This way Dennis will get a build failure when PSM is build with these > headers instead of subtle runtime breakage. He can import the required > definitions to support the staging compat ABI into PSM, where they > belong, and make a new release to build with new kernel headers. Lots > of time to do that before these headers hit the distros. > > Upstream is not the place to carry that stuff, and keeping strange > subtleness with __NUM is just going to risk future breakage. I would love to go in this direction, to get rid of unused defines (especially in UAPIs). Let's wait till Sunday, maybe we will see more discussions on the topic. [1] http://lxr.free-electrons.com/source/include/uapi/drm/i915_drm.h > > Jason
On Thu, Sep 01, 2016 at 08:52:22PM +0300, Leon Romanovsky wrote: > On Thu, Sep 01, 2016 at 11:33:20AM -0600, Jason Gunthorpe wrote: > > On Thu, Sep 01, 2016 at 05:17:26PM +0000, Dalessandro, Dennis wrote: > > > On Thu, 2016-09-01 at 11:11 -0600, Jason Gunthorpe wrote: > > > > On Thu, Sep 01, 2016 at 05:09:31PM +0000, Dalessandro, Dennis wrote: > > > > > > > > > > Dennis should use an internal definition in PSM if he wishes to > > > > > > continue to support the staging kernel ABI. > > > > > > > > > > It's not just the backward compatibility. PSM uses these command > > > > > definitions.So this breaks current support with current driver. > > > > > > > > How exactly? The HFI1_CMD_ASSIGN_CTXT constant is never used in the > > > > kernel? > > > > > > This is used in PSM library. Agree it's not used in the kernel, so I > > > can see the argument to get rid of it, or not care. > > > > Lets get rid of all the #defines. Leon you should just inline the > > ioctl numbers into the ioctl definition like normal and get rid of > > this extra layer of macros. > > Ohh, it looks like you returned to us filled with positive energy :) > > I tried to follow DRM subsystem [1] while converted these names with > minimal impact on the current implementation. I like the idea of borrowing from the DRM subsystem but that is not quite what I see here. That subsystem defines a constant command base and a common Command base (defined as an "end" for vendor specific): /** * Device specific ioctls should only be in their respective headers * The device specific ioctl range is from 0x40 to 0x9f. * Generic IOCTLS restart at 0xA0. * * \sa drmCommandNone(), drmCommandRead(), drmCommandWrite(), and * drmCommandReadWrite(). **/ #define DRM_COMMAND_BASE 0x40 #define DRM_COMMAND_END 0xA0 And then all the vendor specific commands are defined like: #define DRM_AMDGPU_GEM_CREATE 0x00 ... #define DRM_I810_INIT 0x00 ... #define DRM_I915_INIT 0x00 ... These defines are _not_ used anywhere in the kernel but rather define a device specific command _offset_ within the drm vendor ioctl range. When we agreed that HFI1 would use an 0x80 offset I thought that was the idea.[*] That IB would have an upper range which was device specific and HFI1 would be the first users of that range. Maybe it would be better to use: #define RDMA_VENDOR_BASE 0x80 Rather than HFI1_CMD_BASE? What I think is wrong is that your patch changes the HFI1_CMD_ASSIGN_CTXT define from being an offset from the base (0x01) to a hard coded RDMA define 0x81. This deviates from the idea that these defines are offsets from the common base. I can see the argument Jason presents. But I think we should leave the current defines for the following reasons: 1) that was not the agreed upon interface when hfi1 came out of staging. 2) This prevents other devices from utilizing this vendor range. 3) This allows us to use this range in the future for other devices 4) There is little reason to break userspace unnecessarily. All that said: I know that we are not heading down the path of vendor devices having the same IOCTLs but I don't think it is an idea we should abandon. The patch set from Matan has the following in ib_uverbs_ioctl: if (cmd == RDMA_DIRECT_IOCTL) { /* TODO? */ err = -ENOSYS; goto out; } else { Given that we already have a vendor ioctl range concept I think we should just keep this range for potential use instead of the above code. And keep the defines the way they are: as an HFI1 specific offset into the vendor ioctl range. Finally, I have to disagree with Jason that any define in a uapi file is required to be used in the kernel. Looking through the drm code that is clearly not the case. Ira [*] I can't find the email reference at this time... :-( > > > > This way Dennis will get a build failure when PSM is build with these > > headers instead of subtle runtime breakage. He can import the required > > definitions to support the staging compat ABI into PSM, where they > > belong, and make a new release to build with new kernel headers. Lots > > of time to do that before these headers hit the distros. > > > > Upstream is not the place to carry that stuff, and keeping strange > > subtleness with __NUM is just going to risk future breakage. > > I would love to go in this direction, to get rid of unused defines > (especially in UAPIs). > > Let's wait till Sunday, maybe we will see more discussions on the topic. > > [1] http://lxr.free-electrons.com/source/include/uapi/drm/i915_drm.h > > > > > 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 Tue, Sep 06, 2016 at 05:03:14PM -0400, ira.weiny wrote: > These defines are _not_ used anywhere in the kernel but rather define a > device specific command _offset_ within the drm vendor ioctl range. That is not completely true, the defines are used when setting up the kernel ioctl #. I have no idea why they did this, to me it is polluting a uapi header with unneeded defines which is a big no-no. Userspace should certainly *NEVER* use these constants, removing them is the best way to achieve that. > When we agreed that HFI1 would use an 0x80 offset I thought that was the > idea.[*] That IB would have an upper range which was device specific and HFI1 > would be the first users of that range. I'd rather not have non-unique ioctls if we can avoid it...... But even if we do, removing these indirection constants does nothing to change that - other drivers can still alias 0x80. That is doable, particularly if the struct is a different size, then we still have unique ioctl numbers. With some care other vendors can probably manage... 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 Tue, Sep 06, 2016 at 05:03:14PM -0400, ira.weiny wrote: > On Thu, Sep 01, 2016 at 08:52:22PM +0300, Leon Romanovsky wrote: > > On Thu, Sep 01, 2016 at 11:33:20AM -0600, Jason Gunthorpe wrote: > > > On Thu, Sep 01, 2016 at 05:17:26PM +0000, Dalessandro, Dennis wrote: > > > > On Thu, 2016-09-01 at 11:11 -0600, Jason Gunthorpe wrote: > > > > > On Thu, Sep 01, 2016 at 05:09:31PM +0000, Dalessandro, Dennis wrote: > > > > > > > > > > > > Dennis should use an internal definition in PSM if he wishes to > > > > > > > continue to support the staging kernel ABI. > > > > > > > > > > > > It's not just the backward compatibility. PSM uses these command > > > > > > definitions.So this breaks current support with current driver. > > > > > > > > > > How exactly? The HFI1_CMD_ASSIGN_CTXT constant is never used in the > > > > > kernel? > > > > > > > > This is used in PSM library. Agree it's not used in the kernel, so I > > > > can see the argument to get rid of it, or not care. > > > > > > Lets get rid of all the #defines. Leon you should just inline the > > > ioctl numbers into the ioctl definition like normal and get rid of > > > this extra layer of macros. > > > > Ohh, it looks like you returned to us filled with positive energy :) > > > > I tried to follow DRM subsystem [1] while converted these names with > > minimal impact on the current implementation. > > I like the idea of borrowing from the DRM subsystem but that is not quite what > I see here. "followed" != ("borrowed" || "copy-pasted") To make long story short. 1. You took 0xE0 as a base for HFI while claimed to take 0xF0. There is nothing about 0x80 as a base. See commit 8d970cf991a6c38a5566572979487b906d643740 +/* + * Make the ioctls occupy the last 0xf0-0xff portion of the IB range + */ +#define __NUM(cmd) (HFI1_CMD_##cmd + 0xe0) 2. In new ABI, you will define your special objects, mark them as a vendor specific and the driver will be called immediately. IMHO, there is no need in special ioctl. 3. The Jason's point do not pollute code with defines/code which not in use has very solid background. It is right time to stop misuse of UAPI which you did. I'm pretty sure that you was supposed to update your PSM library when you moved from staging and converted from write to ioctl. I didn't hear complains about it. Thanks.
On Tue, Sep 06, 2016 at 03:19:59PM -0600, Jason Gunthorpe wrote: > On Tue, Sep 06, 2016 at 05:03:14PM -0400, ira.weiny wrote: > > > These defines are _not_ used anywhere in the kernel but rather define a > > device specific command _offset_ within the drm vendor ioctl range. > > That is not completely true, the defines are used when setting up the > kernel ioctl #. I have no idea why they did this, to me it is > polluting a uapi header with unneeded defines which is a big no-no. Well... That is the same for the HFI1 defines as the __NUM macro expanded to those defines... #define __NUM(cmd) (HFI1_CMD_##cmd + 0xe0) But really aren't we just nit picking now? > > Userspace should certainly *NEVER* use these constants, removing them > is the best way to achieve that. I assume the DRM user space uses them to help distinguish between #define DRM_AMDGPU_GEM_CREATE 0x00 And #define DRM_I810_INIT 0x00 I'm not at all familiar with the drm user space code but I could see some sort of "provider" architecture where the providers pass these constants and some "core" layer adds the vendor offset automatically??? > > > When we agreed that HFI1 would use an 0x80 offset I thought that was the > > idea.[*] That IB would have an upper range which was device specific and HFI1 > > would be the first users of that range. > > I'd rather not have non-unique ioctls if we can avoid it...... I'm still mulling over the plan for this. > > But even if we do, removing these indirection constants does nothing to > change that - other drivers can still alias 0x80. Yes but having walked through the drm headers it is nice in how explicit they are. > > That is doable, particularly if the struct is a different size, then > we still have unique ioctl numbers. With some care other vendors can > probably manage... Yes, I am sure we can manage. There are always different ways to do things. In the end if Doug accepts this series we will manage in PSM. However, I really don't think what was done was "wrong". Ira > > 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 -- 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, Sep 07, 2016 at 08:55:19AM +0300, Leon Romanovsky wrote: > On Tue, Sep 06, 2016 at 05:03:14PM -0400, ira.weiny wrote: > > On Thu, Sep 01, 2016 at 08:52:22PM +0300, Leon Romanovsky wrote: > > > On Thu, Sep 01, 2016 at 11:33:20AM -0600, Jason Gunthorpe wrote: > > > > On Thu, Sep 01, 2016 at 05:17:26PM +0000, Dalessandro, Dennis wrote: > > > > > On Thu, 2016-09-01 at 11:11 -0600, Jason Gunthorpe wrote: > > > > > > On Thu, Sep 01, 2016 at 05:09:31PM +0000, Dalessandro, Dennis wrote: > > > > > > > > > > > > > > Dennis should use an internal definition in PSM if he wishes to > > > > > > > > continue to support the staging kernel ABI. > > > > > > > > > > > > > > It's not just the backward compatibility. PSM uses these command > > > > > > > definitions.So this breaks current support with current driver. > > > > > > > > > > > > How exactly? The HFI1_CMD_ASSIGN_CTXT constant is never used in the > > > > > > kernel? > > > > > > > > > > This is used in PSM library. Agree it's not used in the kernel, so I > > > > > can see the argument to get rid of it, or not care. > > > > > > > > Lets get rid of all the #defines. Leon you should just inline the > > > > ioctl numbers into the ioctl definition like normal and get rid of > > > > this extra layer of macros. > > > > > > Ohh, it looks like you returned to us filled with positive energy :) > > > > > > I tried to follow DRM subsystem [1] while converted these names with > > > minimal impact on the current implementation. > > > > I like the idea of borrowing from the DRM subsystem but that is not quite what > > I see here. > > "followed" != ("borrowed" || "copy-pasted") Understood. > > To make long story short. > > 1. You took 0xE0 as a base for HFI while claimed to take 0xF0. There is > nothing about 0x80 as a base. See commit 8d970cf991a6c38a5566572979487b906d643740 > > +/* > + * Make the ioctls occupy the last 0xf0-0xff portion of the IB range > + */ > +#define __NUM(cmd) (HFI1_CMD_##cmd + 0xe0) Ok the comment was wrong. Sorry. > > 2. In new ABI, you will define your special objects, mark them as a > vendor specific and the driver will be called immediately. IMHO, there > is no need in special ioctl. Well I'm still trying to get time to comment on that series. One thing which we are a bit worried about are the number of objects which may be tracked by for a device and the performance to get to a special driver object (like a PSM context). Right now PSM opens a FD and associates a single context with that FD so the access of that context is O(1). The addition of special objects and looking them up adds overhead which _MAY_ be ok but until we get a chance to evaluate better I can't commit that what you say here is true. > > 3. The Jason's point do not pollute code with defines/code which not in > use has very solid background. It is right time to stop misuse of UAPI > which you did. I'm pretty sure that you was supposed to update your PSM > library when you moved from staging and converted from write to ioctl. > I didn't hear complains about it. We did update PSM to move from staging to the ioctls. The fact that we are using these defines is because they were exported (just like the drm exports similar defines.) Perhaps drm does not use their defines I don't know. Perhaps these are "polluting" the header but generally I just can't agree that it is "wrong" to have defines like this. I think it is just a different style. As I said to Jason, we can adapt and I will not lose any sleep over this. Ira > > Thanks. -- 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, Sep 07, 2016 at 07:55:09PM -0400, ira.weiny wrote: > > > > 2. In new ABI, you will define your special objects, mark them as a > > vendor specific and the driver will be called immediately. IMHO, there > > is no need in special ioctl. > > Well I'm still trying to get time to comment on that series. One thing which > we are a bit worried about are the number of objects which may be tracked by > for a device and the performance to get to a special driver object (like a PSM > context). Right now PSM opens a FD and associates a single context with that > FD so the access of that context is O(1). The addition of special objects and > looking them up adds overhead which _MAY_ be ok but until we get a chance to > evaluate better I can't commit that what you say here is true. Good to know.
diff --git a/include/uapi/rdma/rdma_user_ioctl.h b/include/uapi/rdma/rdma_user_ioctl.h index 7ecf8cd..9abd6d1 100644 --- a/include/uapi/rdma/rdma_user_ioctl.h +++ b/include/uapi/rdma/rdma_user_ioctl.h @@ -39,71 +39,89 @@ #include <rdma/hfi/hfi1_ioctl.h> /* Documentation/ioctl/ioctl-number.txt */ -#define RDMA_IOCTL_MAGIC 0x1b +#define RDMA_IOCTL_MAGIC 0x1b /* Legacy name, for user space application which already use it */ -#define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC +#define IB_IOCTL_MAGIC RDMA_IOCTL_MAGIC -#define IB_USER_MAD_REGISTER_AGENT _IOWR(RDMA_IOCTL_MAGIC, 1, \ - struct ib_user_mad_reg_req) +/* General blocks assignments */ +#define MAD_CMD_BASE 0x00 +#define HFI1_CMD_BASE 0xE0 -#define IB_USER_MAD_UNREGISTER_AGENT _IOW(RDMA_IOCTL_MAGIC, 2, __u32) +/* MAD specific section */ +#define MAD_CMD_REG_AGENT (MAD_CMD_BASE + 0x01) +#define MAD_CMD_UNREG_AGENT (MAD_CMD_BASE + 0x02) +#define MAD_CMD_ENABLE_PKEY (MAD_CMD_BASE + 0x03) +#define MAD_CMD_REG_AGENT2 (MAD_CMD_BASE + 0x04) -#define IB_USER_MAD_ENABLE_PKEY _IO(RDMA_IOCTL_MAGIC, 3) - -#define IB_USER_MAD_REGISTER_AGENT2 _IOWR(RDMA_IOCTL_MAGIC, 4, \ - struct ib_user_mad_reg_req2) +#define IB_USER_MAD_REGISTER_AGENT \ + _IOWR(RDMA_IOCTL_MAGIC, MAD_CMD_REG_AGENT, struct ib_user_mad_reg_req) +#define IB_USER_MAD_UNREGISTER_AGENT \ + _IOW(RDMA_IOCTL_MAGIC, MAD_CMD_UNREG_AGENT, __u32) +#define IB_USER_MAD_ENABLE_PKEY \ + _IO(RDMA_IOCTL_MAGIC, MAD_CMD_ENABLE_PKEY) +#define IB_USER_MAD_REGISTER_AGENT2 \ + _IOWR(RDMA_IOCTL_MAGIC, MAD_CMD_REG_AGENT2, struct ib_user_mad_reg_req2) +/* HFI specific section */ /* User commands. */ -#define HFI1_CMD_ASSIGN_CTXT 1 /* allocate HFI and context */ -#define HFI1_CMD_CTXT_INFO 2 /* find out what resources we got */ -#define HFI1_CMD_USER_INFO 3 /* set up userspace */ -#define HFI1_CMD_TID_UPDATE 4 /* update expected TID entries */ -#define HFI1_CMD_TID_FREE 5 /* free expected TID entries */ -#define HFI1_CMD_CREDIT_UPD 6 /* force an update of PIO credit */ +/* allocate HFI and context */ +#define HFI1_CMD_ASSIGN_CTXT (HFI1_CMD_BASE + 0x01) +/* find out what resources we got */ +#define HFI1_CMD_CTXT_INFO (HFI1_CMD_BASE + 0x02) +/* set up userspace */ +#define HFI1_CMD_USER_INFO (HFI1_CMD_BASE + 0x03) +/* update expected TID entries */ +#define HFI1_CMD_TID_UPDATE (HFI1_CMD_BASE + 0x04) +/* free expected TID entries */ +#define HFI1_CMD_TID_FREE (HFI1_CMD_BASE + 0x05) +/* force an update of PIO credit */ +#define HFI1_CMD_CREDIT_UPD (HFI1_CMD_BASE + 0x06) -#define HFI1_CMD_RECV_CTRL 8 /* control receipt of packets */ -#define HFI1_CMD_POLL_TYPE 9 /* set the kind of polling we want */ -#define HFI1_CMD_ACK_EVENT 10 /* ack & clear user status bits */ -#define HFI1_CMD_SET_PKEY 11 /* set context's pkey */ -#define HFI1_CMD_CTXT_RESET 12 /* reset context's HW send context */ -#define HFI1_CMD_TID_INVAL_READ 13 /* read TID cache invalidations */ -#define HFI1_CMD_GET_VERS 14 /* get the version of the user cdev */ +/* control receipt of packets */ +#define HFI1_CMD_RECV_CTRL (HFI1_CMD_BASE + 0x08) +/* set the kind of polling we want */ +#define HFI1_CMD_POLL_TYPE (HFI1_CMD_BASE + 0x09) +/* ack & clear user status bits */ +#define HFI1_CMD_ACK_EVENT (HFI1_CMD_BASE + 0x0A) +/* set context's pkey */ +#define HFI1_CMD_SET_PKEY (HFI1_CMD_BASE + 0x0B) +/* reset context's HW send context */ +#define HFI1_CMD_CTXT_RESET (HFI1_CMD_BASE + 0x0C) +/* read TID cache invalidations */ +#define HFI1_CMD_TID_INVAL_READ (HFI1_CMD_BASE + 0x0D) +/* get the version of the user cdev */ +#define HFI1_CMD_GET_VERS (HFI1_CMD_BASE + 0x0E) /* * User IOCTLs can not go above 128 if they do then see common.h and change the * base for the snoop ioctl */ -/* - * Make the ioctls occupy the last 0xf0-0xff portion of the IB range - */ -#define __NUM(cmd) (HFI1_CMD_##cmd + 0xe0) - #define HFI1_IOCTL_ASSIGN_CTXT \ - _IOWR(RDMA_IOCTL_MAGIC, __NUM(ASSIGN_CTXT), struct hfi1_user_info) + _IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_ASSIGN_CTXT, struct hfi1_user_info) #define HFI1_IOCTL_CTXT_INFO \ - _IOW(RDMA_IOCTL_MAGIC, __NUM(CTXT_INFO), struct hfi1_ctxt_info) + _IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_CTXT_INFO, struct hfi1_ctxt_info) #define HFI1_IOCTL_USER_INFO \ - _IOW(RDMA_IOCTL_MAGIC, __NUM(USER_INFO), struct hfi1_base_info) + _IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_USER_INFO, struct hfi1_base_info) #define HFI1_IOCTL_TID_UPDATE \ - _IOWR(RDMA_IOCTL_MAGIC, __NUM(TID_UPDATE), struct hfi1_tid_info) + _IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_TID_UPDATE, struct hfi1_tid_info) #define HFI1_IOCTL_TID_FREE \ - _IOWR(RDMA_IOCTL_MAGIC, __NUM(TID_FREE), struct hfi1_tid_info) + _IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_TID_FREE, struct hfi1_tid_info) #define HFI1_IOCTL_CREDIT_UPD \ - _IO(RDMA_IOCTL_MAGIC, __NUM(CREDIT_UPD)) + _IO(RDMA_IOCTL_MAGIC, HFI1_CMD_CREDIT_UPD) #define HFI1_IOCTL_RECV_CTRL \ - _IOW(RDMA_IOCTL_MAGIC, __NUM(RECV_CTRL), int) + _IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_RECV_CTRL, int) #define HFI1_IOCTL_POLL_TYPE \ - _IOW(RDMA_IOCTL_MAGIC, __NUM(POLL_TYPE), int) + _IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_POLL_TYPE, int) #define HFI1_IOCTL_ACK_EVENT \ - _IOW(RDMA_IOCTL_MAGIC, __NUM(ACK_EVENT), unsigned long) + _IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_ACK_EVENT, unsigned long) #define HFI1_IOCTL_SET_PKEY \ - _IOW(RDMA_IOCTL_MAGIC, __NUM(SET_PKEY), __u16) + _IOW(RDMA_IOCTL_MAGIC, HFI1_CMD_SET_PKEY, __u16) #define HFI1_IOCTL_CTXT_RESET \ - _IO(RDMA_IOCTL_MAGIC, __NUM(CTXT_RESET)) + _IO(RDMA_IOCTL_MAGIC, HFI1_CMD_CTXT_RESET) #define HFI1_IOCTL_TID_INVAL_READ \ - _IOWR(RDMA_IOCTL_MAGIC, __NUM(TID_INVAL_READ), struct hfi1_tid_info) + _IOWR(RDMA_IOCTL_MAGIC, HFI1_CMD_TID_INVAL_READ, struct hfi1_tid_info) #define HFI1_IOCTL_GET_VERS \ - _IOR(RDMA_IOCTL_MAGIC, __NUM(GET_VERS), int) + _IOR(RDMA_IOCTL_MAGIC, HFI1_CMD_GET_VERS, int) #endif /* RDMA_USER_IOCTL_H */