Michael S. Tsirkin
2023-Mar-08 14:13 UTC
[PATCH 3/3] virtio_ring: Use const to annotate read-only pointer params
On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:> On 2023-03-07 04:14, David Edmondson wrote: > > External email: Use caution opening links or attachments > > > > > > Feng Liu via Virtualization <virtualization at lists.linux-foundation.org> > > writes: > > > >> Add const to make the read-only pointer parameters clear, similar to > >> many existing functions. > > > > In many of the modified functions the local variable that is a cast of > > the argument could also be const. Is there a reason not to do both at > > the same time? > > > > Hi,David > > In order to prevent the content of a pointer parameter from being > modified and increase the readability of the function, it is recommended > to add the 'const' keyword to the parameter. This is not necessary for > local variables and non-pointer parameters, as they are only stored on > the stack and do not affect the original value or structure member > passed into the function. Therefore, in this case, the 'const' keyword > is only added to pointer parameters.This makes no sense to me. If ytou cast away the const then it is pointless.> > >> Signed-off-by: Feng Liu <feliu at nvidia.com> > >> Reviewed-by: Jiri Pirko <jiri at nvidia.com> > >> Reviewed-by: Parav Pandit <parav at nvidia.com> > >> Reviewed-by: Gavin Li <gavinl at nvidia.com> > >> Reviewed-by: Bodong Wang <bodong at nvidia.com> > >> --- > >> drivers/virtio/virtio_ring.c | 25 ++++++++++++------------- > >> include/linux/virtio.h | 12 ++++++------ > >> 2 files changed, 18 insertions(+), 19 deletions(-) > >> > >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > >> index 806cc44eae64..8010fd1d2047 100644 > >> --- a/drivers/virtio/virtio_ring.c > >> +++ b/drivers/virtio/virtio_ring.c > >> @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq); > >> > >> #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq) > >> > >> -static bool virtqueue_use_indirect(struct vring_virtqueue *vq, > >> +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq, > >> unsigned int total_sg) > >> { > >> /* > >> @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue *vq, > >> * unconditionally on data path. > >> */ > >> > >> -static bool vring_use_dma_api(struct virtio_device *vdev) > >> +static bool vring_use_dma_api(const struct virtio_device *vdev) > >> { > >> if (!virtio_has_dma_quirk(vdev)) > >> return true; > >> @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev) > >> return false; > >> } > >> > >> -size_t virtio_max_dma_size(struct virtio_device *vdev) > >> +size_t virtio_max_dma_size(const struct virtio_device *vdev) > >> { > >> size_t max_segment_size = SIZE_MAX; > >> > >> @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num) > >> */ > >> > >> static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq, > >> - struct vring_desc *desc) > >> + const struct vring_desc *desc) > >> { > >> u16 flags; > >> > >> @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx) > >> } > >> > >> static void vring_unmap_extra_packed(const struct vring_virtqueue *vq, > >> - struct vring_desc_extra *extra) > >> + const struct vring_desc_extra *extra) > >> { > >> u16 flags; > >> > >> @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq, > >> } > >> > >> static void vring_unmap_desc_packed(const struct vring_virtqueue *vq, > >> - struct vring_packed_desc *desc) > >> + const struct vring_packed_desc *desc) > >> { > >> u16 flags; > >> > >> @@ -2786,7 +2786,7 @@ EXPORT_SYMBOL_GPL(vring_transport_features); > >> * Returns the size of the vring. This is mainly used for boasting to > >> * userspace. Unlike other operations, this need not be serialized. > >> */ > >> -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq) > >> +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq) > >> { > >> > >> struct vring_virtqueue *vq = to_vvq(_vq); > >> @@ -2819,7 +2819,7 @@ void __virtqueue_unbreak(struct virtqueue *_vq) > >> } > >> EXPORT_SYMBOL_GPL(__virtqueue_unbreak); > >> > >> -bool virtqueue_is_broken(struct virtqueue *_vq) > >> +bool virtqueue_is_broken(const struct virtqueue *_vq) > >> { > >> struct vring_virtqueue *vq = to_vvq(_vq); > >> > >> @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq) > >> } > >> EXPORT_SYMBOL_GPL(virtqueue_is_broken); > >> > >> -/* > >> - * This should prevent the device from being used, allowing drivers to > >> +/ This should prevent the device from being used, allowing drivers to > >> * recover. You may need to grab appropriate locks to flush. > >> */ > >> void virtio_break_device(struct virtio_device *dev) > >> @@ -2881,7 +2880,7 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq) > >> } > >> EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr); > >> > >> -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq) > >> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq) > >> { > >> struct vring_virtqueue *vq = to_vvq(_vq); > >> > >> @@ -2895,7 +2894,7 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq) > >> } > >> EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr); > >> > >> -dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq) > >> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq) > >> { > >> struct vring_virtqueue *vq = to_vvq(_vq); > >> > >> @@ -2910,7 +2909,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq) > >> EXPORT_SYMBOL_GPL(virtqueue_get_used_addr); > >> > >> /* Only available for split ring */ > >> -const struct vring *virtqueue_get_vring(struct virtqueue *vq) > >> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq) > >> { > >> return &to_vvq(vq)->split.vring; > >> } > >> diff --git a/include/linux/virtio.h b/include/linux/virtio.h > >> index 2b472514c49b..36a79374e735 100644 > >> --- a/include/linux/virtio.h > >> +++ b/include/linux/virtio.h > >> @@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq); > >> > >> void *virtqueue_detach_unused_buf(struct virtqueue *vq); > >> > >> -unsigned int virtqueue_get_vring_size(struct virtqueue *vq); > >> +unsigned int virtqueue_get_vring_size(const struct virtqueue *vq); > >> > >> -bool virtqueue_is_broken(struct virtqueue *vq); > >> +bool virtqueue_is_broken(const struct virtqueue *vq); > >> > >> -const struct vring *virtqueue_get_vring(struct virtqueue *vq); > >> -dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq); > >> -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq); > >> -dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq); > >> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq); > >> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq); > >> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq); > >> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq); > >> > >> int virtqueue_resize(struct virtqueue *vq, u32 num, > >> void (*recycle)(struct virtqueue *vq, void *buf)); > >> -- > >> 2.34.1 > >> > >> _______________________________________________ > >> Virtualization mailing list > >> Virtualization at lists.linux-foundation.org > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C6265cc6ac03e484167ba08db1eec5428%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638137772595544505%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=74eHEHUMF89nTG6hrrdMpkl43DJWvC9xCjwqNOny%2FQE%3D&reserved=0 > > -- > > Woke up in my clothes again this morning, don't know exactly where I am. >
Feng Liu
2023-Mar-08 15:59 UTC
[PATCH 3/3] virtio_ring: Use const to annotate read-only pointer params
On 2023-03-08 a.m.9:13, Michael S. Tsirkin wrote:> External email: Use caution opening links or attachments > > > On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote: >> On 2023-03-07 04:14, David Edmondson wrote: >>> External email: Use caution opening links or attachments >>> >>> >>> Feng Liu via Virtualization <virtualization at lists.linux-foundation.org> >>> writes: >>> >>>> Add const to make the read-only pointer parameters clear, similar to >>>> many existing functions. >>> >>> In many of the modified functions the local variable that is a cast of >>> the argument could also be const. Is there a reason not to do both at >>> the same time? >>> >> >> Hi,David >> >> In order to prevent the content of a pointer parameter from being >> modified and increase the readability of the function, it is recommended >> to add the 'const' keyword to the parameter. This is not necessary for >> local variables and non-pointer parameters, as they are only stored on >> the stack and do not affect the original value or structure member >> passed into the function. Therefore, in this case, the 'const' keyword >> is only added to pointer parameters. > > This makes no sense to me. If ytou cast away the const then it is > pointless. >Hi, Michael I really don't quite understand your point of view. Is a local variable that needs to be add const? Can you help to point out the specific problem/point ?>> >>>> Signed-off-by: Feng Liu <feliu at nvidia.com> >>>> Reviewed-by: Jiri Pirko <jiri at nvidia.com> >>>> Reviewed-by: Parav Pandit <parav at nvidia.com> >>>> Reviewed-by: Gavin Li <gavinl at nvidia.com> >>>> Reviewed-by: Bodong Wang <bodong at nvidia.com> >>>> --- >>>> drivers/virtio/virtio_ring.c | 25 ++++++++++++------------- >>>> include/linux/virtio.h | 12 ++++++------ >>>> 2 files changed, 18 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >>>> index 806cc44eae64..8010fd1d2047 100644 >>>> --- a/drivers/virtio/virtio_ring.c >>>> +++ b/drivers/virtio/virtio_ring.c >>>> @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq); >>>> >>>> #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq) >>>> >>>> -static bool virtqueue_use_indirect(struct vring_virtqueue *vq, >>>> +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq, >>>> unsigned int total_sg) >>>> { >>>> /* >>>> @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue *vq, >>>> * unconditionally on data path. >>>> */ >>>> >>>> -static bool vring_use_dma_api(struct virtio_device *vdev) >>>> +static bool vring_use_dma_api(const struct virtio_device *vdev) >>>> { >>>> if (!virtio_has_dma_quirk(vdev)) >>>> return true; >>>> @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev) >>>> return false; >>>> } >>>> >>>> -size_t virtio_max_dma_size(struct virtio_device *vdev) >>>> +size_t virtio_max_dma_size(const struct virtio_device *vdev) >>>> { >>>> size_t max_segment_size = SIZE_MAX; >>>> >>>> @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num) >>>> */ >>>> >>>> static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq, >>>> - struct vring_desc *desc) >>>> + const struct vring_desc *desc) >>>> { >>>> u16 flags; >>>> >>>> @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx) >>>> } >>>> >>>> static void vring_unmap_extra_packed(const struct vring_virtqueue *vq, >>>> - struct vring_desc_extra *extra) >>>> + const struct vring_desc_extra *extra) >>>> { >>>> u16 flags; >>>> >>>> @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq, >>>> } >>>> >>>> static void vring_unmap_desc_packed(const struct vring_virtqueue *vq, >>>> - struct vring_packed_desc *desc) >>>> + const struct vring_packed_desc *desc) >>>> { >>>> u16 flags; >>>> >>>> @@ -2786,7 +2786,7 @@ EXPORT_SYMBOL_GPL(vring_transport_features); >>>> * Returns the size of the vring. This is mainly used for boasting to >>>> * userspace. Unlike other operations, this need not be serialized. >>>> */ >>>> -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq) >>>> +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq) >>>> { >>>> >>>> struct vring_virtqueue *vq = to_vvq(_vq); >>>> @@ -2819,7 +2819,7 @@ void __virtqueue_unbreak(struct virtqueue *_vq) >>>> } >>>> EXPORT_SYMBOL_GPL(__virtqueue_unbreak); >>>> >>>> -bool virtqueue_is_broken(struct virtqueue *_vq) >>>> +bool virtqueue_is_broken(const struct virtqueue *_vq) >>>> { >>>> struct vring_virtqueue *vq = to_vvq(_vq); >>>> >>>> @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq) >>>> } >>>> EXPORT_SYMBOL_GPL(virtqueue_is_broken); >>>> >>>> -/* >>>> - * This should prevent the device from being used, allowing drivers to >>>> +/ This should prevent the device from being used, allowing drivers to >>>> * recover. You may need to grab appropriate locks to flush. >>>> */ >>>> void virtio_break_device(struct virtio_device *dev) >>>> @@ -2881,7 +2880,7 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq) >>>> } >>>> EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr); >>>> >>>> -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq) >>>> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq) >>>> { >>>> struct vring_virtqueue *vq = to_vvq(_vq); >>>> >>>> @@ -2895,7 +2894,7 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq) >>>> } >>>> EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr); >>>> >>>> -dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq) >>>> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq) >>>> { >>>> struct vring_virtqueue *vq = to_vvq(_vq); >>>> >>>> @@ -2910,7 +2909,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq) >>>> EXPORT_SYMBOL_GPL(virtqueue_get_used_addr); >>>> >>>> /* Only available for split ring */ >>>> -const struct vring *virtqueue_get_vring(struct virtqueue *vq) >>>> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq) >>>> { >>>> return &to_vvq(vq)->split.vring; >>>> } >>>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h >>>> index 2b472514c49b..36a79374e735 100644 >>>> --- a/include/linux/virtio.h >>>> +++ b/include/linux/virtio.h >>>> @@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq); >>>> >>>> void *virtqueue_detach_unused_buf(struct virtqueue *vq); >>>> >>>> -unsigned int virtqueue_get_vring_size(struct virtqueue *vq); >>>> +unsigned int virtqueue_get_vring_size(const struct virtqueue *vq); >>>> >>>> -bool virtqueue_is_broken(struct virtqueue *vq); >>>> +bool virtqueue_is_broken(const struct virtqueue *vq); >>>> >>>> -const struct vring *virtqueue_get_vring(struct virtqueue *vq); >>>> -dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq); >>>> -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq); >>>> -dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq); >>>> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq); >>>> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq); >>>> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq); >>>> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq); >>>> >>>> int virtqueue_resize(struct virtqueue *vq, u32 num, >>>> void (*recycle)(struct virtqueue *vq, void *buf)); >>>> -- >>>> 2.34.1 >>>> >>>> _______________________________________________ >>>> Virtualization mailing list >>>> Virtualization at lists.linux-foundation.org >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C0f6803f1797f448823ac08db1fdf67e5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638138816610707030%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FxggnD7U8o%2B%2BqcYnHN6nc%2BemGVRU1ia5sA4k%2FRTDD7U%3D&reserved=0 >>> -- >>> Woke up in my clothes again this morning, don't know exactly where I am. >> >