Thu, Jul 04, 2024 at 01:28:03PM CEST, mst@xxxxxxxxxx wrote:
>On Thu, Jul 04, 2024 at 01:06:12PM +0200, Jiri Pirko wrote:
>> Thu, Jul 04, 2024 at 09:47:09AM CEST, mst@xxxxxxxxxx wrote:
>> >On Thu, Jul 04, 2024 at 08:43:35AM +0200, Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@xxxxxxxxxx>
>> >>
>> >> Introduce a structure virtio_queue_info to carry name, callback and ctx
>> >> together. In order to allow config implementations to accept config op
>> >> with array of virtio_queue_info structures, introduce a new
>> >> find_vqs_info() op. Do the needed conversion in virtio_find_vqs_ctx().
>> >> Note that whole virtio_find_vqs_ctx() is going to be eventually removed
>> >> at the and of this patchset.
>> >>
>> >> Signed-off-by: Jiri Pirko <jiri@xxxxxxxxxx>
>> >> ---
>> >> v1->v2:
>> >> - fixed comments for struct virtqueue_info
>> >> - s/virtio_queue_info/virtqueue_info/
>> >> ---
>> >> include/linux/virtio_config.h | 51 ++++++++++++++++++++++++++++++++---
>> >> 1 file changed, 48 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
>> >> index 82a1d798b2f1..9b975c2830f6 100644
>> >> --- a/include/linux/virtio_config.h
>> >> +++ b/include/linux/virtio_config.h
>> >> @@ -18,6 +18,16 @@ struct virtio_shm_region {
>> >>
>> >> typedef void vq_callback_t(struct virtqueue *);
>> >>
>> >> +struct virtqueue_info {
>> >> + const char *name; /* Mainly for debugging, may be NULL for a virtqueue
>> >> + * unused by driver.
>> >> + */
>> >> + vq_callback_t *callback; /* May be NULL for vq that does
>> >> + * not need a callback.
>> >> + */
>> >> + bool ctx;
>> >
>> >
>> > /*
>> > * Always
>> > * like this
>> > * outside of netdev code
>> > * and it comes before the code being documented
>> > */
>> >
>> > /* Never
>> > * like this
>> > */
>>
>>
>> How about to properly document the struct instead like this:
>> /**
>> * struct virtqueue_info - info for a virtqueue passed to find_vqs()
>> * @name: mainly for debugging, NULL for a virtqueue
>> * unused by the driver
>> * @callback: NULL for a virtqueue that does not need a callback
>> * @ctx: true to allow to maintain extra context per virtqueue
>> */
>> struct virtqueue_info {
>> const char *name;
>> vq_callback_t *callback;
>> bool ctx;
>> };
>
>Sure. And actually include the explanation what the fields are then?
>@name: virtqueue description
>@callback: a callback to invoke on a used buffer notification
Okay, how about:
/**
* struct virtqueue_info - Info for a virtqueue passed to find_vqs().
* @name: virtqueue description. Used mainly for debugging, NULL for
* a virtqueue unused by the driver.
* @callback: A callback to invoke on a used buffer notification.
* NULL for a virtqueue that does not need a callback.
* @ctx: A flag to indicate to maintain an extra context per virtqueue.
*/
struct virtqueue_info {
const char *name;
vq_callback_t *callback;
bool ctx;
};
?
>
>
>>
>>
>> >
>> >
>> >Also "may be null" is a bit confusing - it has to be null
>> >for such a vq.
>> >
>> >Thus:
>> >
>> > /*
>> > * name: mainly for debugging, NULL for a virtqueue
>> > * unused by the driver.
>> > */
>> > const char *name;
>> >
>> >and similarly for the callback.
>> >
>> >
>> >
>> >> +};
>> >> +
>> >> /**
>> >> * struct virtio_config_ops - operations for configuring a virtio device
>> >> * Note: Do not assume that a transport implements all of the operations
>> >> @@ -58,6 +68,12 @@ typedef void vq_callback_t(struct virtqueue *);
>> >> * names: array of virtqueue names (mainly for debugging)
>> >> * include a NULL entry for vqs unused by driver
>> >> * Returns 0 on success or error status
>> >> + * @find_vqs_info: find virtqueues and instantiate them.
>> >> + * vdev: the virtio_device
>> >> + * nvqs: the number of virtqueues to find
>> >> + * vqs: on success, includes new virtqueues
>> >> + * vqs_info: array of virtqueue info structures
>> >> + * Returns 0 on success or error status
>> >> * @del_vqs: free virtqueues found by find_vqs().
>> >> * @synchronize_cbs: synchronize with the virtqueue callbacks (optional)
>> >> * The function guarantees that all memory operations on the
>> >> @@ -109,6 +125,10 @@ struct virtio_config_ops {
>> >> struct virtqueue *vqs[], vq_callback_t *callbacks[],
>> >> const char * const names[], const bool *ctx,
>> >> struct irq_affinity *desc);
>> >> + int (*find_vqs_info)(struct virtio_device *vdev, unsigned int nvqs,
>> >> + struct virtqueue *vqs[],
>> >> + struct virtqueue_info vqs_info[],
>> >> + struct irq_affinity *desc);
>> >> void (*del_vqs)(struct virtio_device *);
>> >> void (*synchronize_cbs)(struct virtio_device *);
>> >> u64 (*get_features)(struct virtio_device *vdev);
>> >> @@ -117,7 +137,7 @@ struct virtio_config_ops {
>> >> int (*set_vq_affinity)(struct virtqueue *vq,
>> >> const struct cpumask *cpu_mask);
>> >> const struct cpumask *(*get_vq_affinity)(struct virtio_device *vdev,
>> >> - int index);
>> >> + int index);
>> >> bool (*get_shm_region)(struct virtio_device *vdev,
>> >> struct virtio_shm_region *region, u8 id);
>> >> int (*disable_vq_and_reset)(struct virtqueue *vq);
>> >> @@ -210,14 +230,39 @@ static inline bool virtio_has_dma_quirk(const struct virtio_device *vdev)
>> >> return !virtio_has_feature(vdev, VIRTIO_F_ACCESS_PLATFORM);
>> >> }
>> >>
>> >> +static inline
>> >> +int virtio_find_vqs_info(struct virtio_device *vdev, unsigned int nvqs,
>> >> + struct virtqueue *vqs[],
>> >> + struct virtqueue_info vqs_info[],
>> >> + struct irq_affinity *desc)
>> >> +{
>> >> + return vdev->config->find_vqs_info(vdev, nvqs, vqs, vqs_info, desc);
>> >> +}
>> >> +
>> >> static inline
>> >> int virtio_find_vqs_ctx(struct virtio_device *vdev, unsigned nvqs,
>> >> struct virtqueue *vqs[], vq_callback_t *callbacks[],
>> >> const char * const names[], const bool *ctx,
>> >> struct irq_affinity *desc)
>> >> {
>> >> - return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks, names, ctx,
>> >> - desc);
>> >> + struct virtqueue_info *vqs_info;
>> >> + int err, i;
>> >> +
>> >> + if (!vdev->config->find_vqs_info)
>> >> + return vdev->config->find_vqs(vdev, nvqs, vqs, callbacks,
>> >> + names, ctx, desc);
>> >> +
>> >> + vqs_info = kmalloc_array(nvqs, sizeof(*vqs_info), GFP_KERNEL);
>> >> + if (!vqs_info)
>> >> + return -ENOMEM;
>> >> + for (i = 0; i < nvqs; i++) {
>> >> + vqs_info[i].name = names[i];
>> >> + vqs_info[i].callback = callbacks[i];
>> >> + vqs_info[i].ctx = ctx ? ctx[i] : false;
>> >> + }
>> >> + err = virtio_find_vqs_info(vdev, nvqs, vqs, vqs_info, desc);
>> >> + kfree(vqs_info);
>> >> + return err;
>> >> }
>> >>
>> >> static inline
>> >> --
>> >> 2.45.2
>> >
>
