On Thu, Mar 02, 2023 at 12:34:19PM +0100, Stefano Garzarella wrote:
> Let's use our own kthread to run device jobs.
> This allows us more flexibility, especially we can attach the kthread
> to the user address space when vDPA uses user's VA.
>
> Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> ---
> drivers/vdpa/vdpa_sim/vdpa_sim.h | 3 ++-
> drivers/vdpa/vdpa_sim/vdpa_sim.c | 17 ++++++++++++-----
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> index acee20faaf6a..ce83f9130a5d 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> @@ -57,7 +57,8 @@ struct vdpasim_dev_attr {
> struct vdpasim {
> struct vdpa_device vdpa;
> struct vdpasim_virtqueue *vqs;
> - struct work_struct work;
> + struct kthread_worker *worker;
> + struct kthread_work work;
> struct vdpasim_dev_attr dev_attr;
> /* spinlock to synchronize virtqueue state */
> spinlock_t lock;
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index a6ee830efc38..6feb29726c2a 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -11,8 +11,8 @@
> #include <linux/module.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> +#include <linux/kthread.h>
> #include <linux/slab.h>
> -#include <linux/sched.h>
> #include <linux/dma-map-ops.h>
> #include <linux/vringh.h>
> #include <linux/vdpa.h>
> @@ -116,7 +116,7 @@ static void vdpasim_do_reset(struct vdpasim *vdpasim)
> static const struct vdpa_config_ops vdpasim_config_ops;
> static const struct vdpa_config_ops vdpasim_batch_config_ops;
>
> -static void vdpasim_work_fn(struct work_struct *work)
> +static void vdpasim_work_fn(struct kthread_work *work)
> {
> struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
>
> @@ -159,7 +159,13 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr,
>
> vdpasim = vdpa_to_sim(vdpa);
> vdpasim->dev_attr = *dev_attr;
> - INIT_WORK(&vdpasim->work, vdpasim_work_fn);
> +
> + kthread_init_work(&vdpasim->work, vdpasim_work_fn);
> + vdpasim->worker = kthread_create_worker(0, "vDPA sim worker: %s",
> + dev_attr->name);
> + if (IS_ERR(vdpasim->worker))
> + goto err_iommu;
Branching to err_iommu will result in a call to put_device(dev)...
> +
> spin_lock_init(&vdpasim->lock);
> spin_lock_init(&vdpasim->iommu_lock);
... but dev is not initialised until the line following this hunk,
which is:
dev = &vdpasim->vdpa.dev;
In order to avoid leaking dev I _think_ the correct approach
is to move the initialisation of dev above the branch to
err_iommu, perhaps above the call to kthread_init_work()
is a good place.
This does move the assignment outside the locks above.
But I _think_ that is ok.
> @@ -212,7 +218,7 @@ EXPORT_SYMBOL_GPL(vdpasim_create);
>
> void vdpasim_schedule_work(struct vdpasim *vdpasim)
> {
> - schedule_work(&vdpasim->work);
> + kthread_queue_work(vdpasim->worker, &vdpasim->work);
> }
> EXPORT_SYMBOL_GPL(vdpasim_schedule_work);
>
> @@ -612,7 +618,8 @@ static void vdpasim_free(struct vdpa_device *vdpa)
> struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
> int i;
>
> - cancel_work_sync(&vdpasim->work);
> + kthread_cancel_work_sync(&vdpasim->work);
> + kthread_destroy_worker(vdpasim->worker);
>
> for (i = 0; i < vdpasim->dev_attr.nvqs; i++) {
> vringh_kiov_cleanup(&vdpasim->vqs[i].out_iov);
> --
> 2.39.2
>
