On Thu, Jul 21, 2022 at 10:50:43PM -0700, Christoph Hellwig wrote:
> On Fri, Jul 22, 2022 at 01:09:30PM +0800, Ming Lei wrote:
> > + unsigned long *map = (unsigned long *)&ub->dev_info.flags[0];
> > +
> > + /* We are not ready to support zero copy */
> > + ub->dev_info.flags[0] &= ~UBLK_F_SUPPORT_ZERO_COPY;
> > +
> > + /*
> > + * 128bit flags will be copied back to userspace as feature
> > + * negotiation result, so have to clear flags which driver
> > + * doesn't support yet, then userspace can get correct flags
> > + * (features) to handle.
> > + */
> > + bitmap_clear(map, __UBLK_F_NR_BITS, 128 - __UBLK_F_NR_BITS);
>
> Please don't do the cast and bitmap ops. In fact I think the current
> ABI is rather nasty. To make everyones life easier just use a single
> u64 flags, an mark the second one reserved so that we can extent into
> it with extra flags or something else. That way normal C operator
> leve bitops just work.
OK.
>
> > +enum ublk_flag_bits {
> > + __UBLK_F_SUPPORT_ZERO_COPY,
> > + __UBLK_F_URING_CMD_COMP_IN_TASK,
> > + __UBLK_F_NR_BITS,
> > +};
>
> Please make these #defines so that userspace can detect if they
> exist in a header using #ifdef.
userspace is supposed to only use UBLK_F_* instead of __UBLK_F_*, one
benefit of using enum is that UBLK_F_NR_BITS can be figured out
automatically, otherwise how can we figure out the max bits?
thanks,
Ming
