Hi Jason, Thanks for the review. On Wed, 10 Mar 2021 15:23:01 -0400, Jason Gunthorpe <jgg@xxxxxxxxxx> wrote: > On Sat, Feb 27, 2021 at 02:01:26PM -0800, Jacob Pan wrote: > > > +/* -------- IOCTLs for IOASID file descriptor (/dev/ioasid) -------- */ > > + > > +/** > > + * IOASID_GET_API_VERSION - _IO(IOASID_TYPE, IOASID_BASE + 0) > > + * > > + * Report the version of the IOASID API. This allows us to bump the > > entire > > + * API version should we later need to add or change features in > > incompatible > > + * ways. > > + * Return: IOASID_API_VERSION > > + * Availability: Always > > + */ > > +#define IOASID_GET_API_VERSION _IO(IOASID_TYPE, > > IOASID_BASE + 0) > > I think this is generally a bad idea, if you change the API later then > also change the ioctl numbers and everything should work out > > eg use the 4th argument to IOC to specify something about the ABI > Let me try to understand the idea, do you mean something like this? #define IOASID_GET_INFO _IOC(_IOC_NONE, IOASID_TYPE, IOASID_BASE + 1, sizeof(struct ioasid_info)) If we later change the size of struct ioasid_info, IOASID_GET_INFO would be a different ioctl number. Then we will break the existing user space that uses the old number. So I am guessing you meant we need to have a different name also. i.e. #define IOASID_GET_INFO_V2 _IOC(_IOC_NONE, IOASID_TYPE, IOASID_BASE + 1, sizeof(struct ioasid_info_v2)) We can get rid of the API version, just have individual IOCTL version. Is that right? > Jason Thanks, Jacob
