Michael S. Tsirkin
2014-Nov-27 20:07 UTC
[PATCH v6 01/46] virtio: add low-level APIs for feature bits
Add low level APIs to test/set/clear feature bits. For use by transports, to make it easier to write code independent of feature bit array format. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> --- include/linux/virtio_config.h | 53 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 7f4ef66..249fcd6 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h @@ -77,11 +77,47 @@ void virtio_check_driver_offered_feature(const struct virtio_device *vdev, unsigned int fbit); /** - * virtio_has_feature - helper to determine if this device has this feature. + * __virtio_test_bit - helper to test feature bits. For use by transports. + * Devices should normally use virtio_has_feature, + * which includes more checks. * @vdev: the device * @fbit: the feature bit */ -static inline bool virtio_has_feature(const struct virtio_device *vdev, +static inline bool __virtio_test_bit(const struct virtio_device *vdev, + unsigned int fbit) +{ + /* Did you forget to fix assumptions on max features? */ + if (__builtin_constant_p(fbit)) + BUILD_BUG_ON(fbit >= 32); + else + BUG_ON(fbit >= 32); + + return test_bit(fbit, vdev->features); +} + +/** + * __virtio_set_bit - helper to set feature bits. For use by transports. + * @vdev: the device + * @fbit: the feature bit + */ +static inline void __virtio_set_bit(struct virtio_device *vdev, + unsigned int fbit) +{ + /* Did you forget to fix assumptions on max features? */ + if (__builtin_constant_p(fbit)) + BUILD_BUG_ON(fbit >= 32); + else + BUG_ON(fbit >= 32); + + set_bit(fbit, vdev->features); +} + +/** + * __virtio_clear_bit - helper to set feature bits. For use by transports. + * @vdev: the device + * @fbit: the feature bit + */ +static inline void __virtio_clear_bit(struct virtio_device *vdev, unsigned int fbit) { /* Did you forget to fix assumptions on max features? */ @@ -90,10 +126,21 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev, else BUG_ON(fbit >= 32); + clear_bit(fbit, vdev->features); +} + +/** + * virtio_has_feature - helper to determine if this device has this feature. + * @vdev: the device + * @fbit: the feature bit + */ +static inline bool virtio_has_feature(const struct virtio_device *vdev, + unsigned int fbit) +{ if (fbit < VIRTIO_TRANSPORT_F_START) virtio_check_driver_offered_feature(vdev, fbit); - return test_bit(fbit, vdev->features); + return __virtio_test_bit(vdev, fbit); } static inline -- MST
Cornelia Huck
2014-Nov-28 10:02 UTC
[PATCH v6 01/46] virtio: add low-level APIs for feature bits
On Thu, 27 Nov 2014 22:07:36 +0200 "Michael S. Tsirkin" <mst at redhat.com> wrote:> Add low level APIs to test/set/clear feature bits. > For use by transports, to make it easier to > write code independent of feature bit array format. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > include/linux/virtio_config.h | 53 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 50 insertions(+), 3 deletions(-) > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h > index 7f4ef66..249fcd6 100644 > --- a/include/linux/virtio_config.h > +++ b/include/linux/virtio_config.h > @@ -77,11 +77,47 @@ void virtio_check_driver_offered_feature(const struct virtio_device *vdev, > unsigned int fbit); > > /** > - * virtio_has_feature - helper to determine if this device has this feature. > + * __virtio_test_bit - helper to test feature bits. For use by transports. > + * Devices should normally use virtio_has_feature, > + * which includes more checks. > * @vdev: the device > * @fbit: the feature bit > */ > -static inline bool virtio_has_feature(const struct virtio_device *vdev, > +static inline bool __virtio_test_bit(const struct virtio_device *vdev, > + unsigned int fbit) > +{ > + /* Did you forget to fix assumptions on max features? */ > + if (__builtin_constant_p(fbit)) > + BUILD_BUG_ON(fbit >= 32); > + else > + BUG_ON(fbit >= 32);Does this want to be a helper? Might be overkill, but I'm always wary of changes that need to be applied manually in many different places :)> + > + return test_bit(fbit, vdev->features); > +} > + > +/** > + * __virtio_set_bit - helper to set feature bits. For use by transports. > + * @vdev: the device > + * @fbit: the feature bit > + */ > +static inline void __virtio_set_bit(struct virtio_device *vdev, > + unsigned int fbit) > +{ > + /* Did you forget to fix assumptions on max features? */ > + if (__builtin_constant_p(fbit)) > + BUILD_BUG_ON(fbit >= 32); > + else > + BUG_ON(fbit >= 32); > + > + set_bit(fbit, vdev->features); > +} > + > +/** > + * __virtio_clear_bit - helper to set feature bits. For use by transports.s/set/clear/> + * @vdev: the device > + * @fbit: the feature bit > + */ > +static inline void __virtio_clear_bit(struct virtio_device *vdev, > unsigned int fbit) > { > /* Did you forget to fix assumptions on max features? */ > @@ -90,10 +126,21 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev, > else > BUG_ON(fbit >= 32); > > + clear_bit(fbit, vdev->features); > +} > + > +/** > + * virtio_has_feature - helper to determine if this device has this feature. > + * @vdev: the device > + * @fbit: the feature bit > + */ > +static inline bool virtio_has_feature(const struct virtio_device *vdev, > + unsigned int fbit) > +{ > if (fbit < VIRTIO_TRANSPORT_F_START) > virtio_check_driver_offered_feature(vdev, fbit); > > - return test_bit(fbit, vdev->features); > + return __virtio_test_bit(vdev, fbit); > } > > static inline
David Hildenbrand
2014-Nov-28 12:10 UTC
[PATCH v6 01/46] virtio: add low-level APIs for feature bits
> Add low level APIs to test/set/clear feature bits. > For use by transports, to make it easier to > write code independent of feature bit array format. > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > include/linux/virtio_config.h | 53 ++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 50 insertions(+), 3 deletions(-) >As these functions really set feature bits, I would rather name them: virtio_has_feature ... virtio_set_feature ... virtio_clear_feature ... But in general, this looks good to me!
Michael S. Tsirkin
2014-Nov-29 17:30 UTC
[PATCH v6 01/46] virtio: add low-level APIs for feature bits
On Fri, Nov 28, 2014 at 11:02:20AM +0100, Cornelia Huck wrote:> On Thu, 27 Nov 2014 22:07:36 +0200 > "Michael S. Tsirkin" <mst at redhat.com> wrote: > > > Add low level APIs to test/set/clear feature bits. > > For use by transports, to make it easier to > > write code independent of feature bit array format. > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > --- > > include/linux/virtio_config.h | 53 ++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h > > index 7f4ef66..249fcd6 100644 > > --- a/include/linux/virtio_config.h > > +++ b/include/linux/virtio_config.h > > @@ -77,11 +77,47 @@ void virtio_check_driver_offered_feature(const struct virtio_device *vdev, > > unsigned int fbit); > > > > /** > > - * virtio_has_feature - helper to determine if this device has this feature. > > + * __virtio_test_bit - helper to test feature bits. For use by transports. > > + * Devices should normally use virtio_has_feature, > > + * which includes more checks. > > * @vdev: the device > > * @fbit: the feature bit > > */ > > -static inline bool virtio_has_feature(const struct virtio_device *vdev, > > +static inline bool __virtio_test_bit(const struct virtio_device *vdev, > > + unsigned int fbit) > > +{ > > + /* Did you forget to fix assumptions on max features? */ > > + if (__builtin_constant_p(fbit)) > > + BUILD_BUG_ON(fbit >= 32); > > + else > > + BUG_ON(fbit >= 32); > > Does this want to be a helper? Might be overkill, but I'm always wary > of changes that need to be applied manually in many different places :)I'm not sure. It's just an assert, if it's not obvious what it checks then it doesn't serve a useful purpose.> > + > > + return test_bit(fbit, vdev->features); > > +} > > + > > +/** > > + * __virtio_set_bit - helper to set feature bits. For use by transports. > > + * @vdev: the device > > + * @fbit: the feature bit > > + */ > > +static inline void __virtio_set_bit(struct virtio_device *vdev, > > + unsigned int fbit) > > +{ > > + /* Did you forget to fix assumptions on max features? */ > > + if (__builtin_constant_p(fbit)) > > + BUILD_BUG_ON(fbit >= 32); > > + else > > + BUG_ON(fbit >= 32); > > + > > + set_bit(fbit, vdev->features); > > +} > > + > > +/** > > + * __virtio_clear_bit - helper to set feature bits. For use by transports. > > s/set/clear/Good catch, thanks.> > + * @vdev: the device > > + * @fbit: the feature bit > > + */ > > +static inline void __virtio_clear_bit(struct virtio_device *vdev, > > unsigned int fbit) > > { > > /* Did you forget to fix assumptions on max features? */ > > @@ -90,10 +126,21 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev, > > else > > BUG_ON(fbit >= 32); > > > > + clear_bit(fbit, vdev->features); > > +} > > + > > +/** > > + * virtio_has_feature - helper to determine if this device has this feature. > > + * @vdev: the device > > + * @fbit: the feature bit > > + */ > > +static inline bool virtio_has_feature(const struct virtio_device *vdev, > > + unsigned int fbit) > > +{ > > if (fbit < VIRTIO_TRANSPORT_F_START) > > virtio_check_driver_offered_feature(vdev, fbit); > > > > - return test_bit(fbit, vdev->features); > > + return __virtio_test_bit(vdev, fbit); > > } > > > > static inline
Michael S. Tsirkin
2014-Nov-29 17:32 UTC
[PATCH v6 01/46] virtio: add low-level APIs for feature bits
On Fri, Nov 28, 2014 at 01:10:57PM +0100, David Hildenbrand wrote:> > Add low level APIs to test/set/clear feature bits. > > For use by transports, to make it easier to > > write code independent of feature bit array format. > > > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > --- > > include/linux/virtio_config.h | 53 ++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 50 insertions(+), 3 deletions(-) > > > > As these functions really set feature bits, I would rather name them: > > virtio_has_feature ... > virtio_set_feature ... > virtio_clear_feature ...We have these as high level APIs for drivers. These are low level ones so need a different name.> But in general, this looks good to me!
Apparently Analagous Threads
- [PATCH v6 01/46] virtio: add low-level APIs for feature bits
- [PATCH v6 01/46] virtio: add low-level APIs for feature bits
- [PATCH v6 01/46] virtio: add low-level APIs for feature bits
- [PATCH v6 01/46] virtio: add low-level APIs for feature bits
- [PATCH v6 01/46] virtio: add low-level APIs for feature bits