Re: [PATCH 1/2] virtio_balloon: Give the balloon its own wakeup source

[David Hildenbrand <david@redhat.com>]

On 18.03.24 10:10, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> Wakeup sources don't support nesting multiple events, so sharing a
> single object between multiple drivers can result in one driver
> overriding the wakeup event processing period specified by another
> driver. Have the virtio balloon driver use the wakeup source of the
> device it is bound to rather than the wakeup source of the parent
> device, to avoid conflicts with the transport layer.
> 
> Note that although the virtio balloon's virtio_device itself isn't what
> actually wakes up the device, it is responsible for processing wakeup
> events. In the same way that EPOLLWAKEUP uses a dedicated wakeup_source
> to prevent suspend when userspace is processing wakeup events, a
> dedicated wakeup_source is necessary when processing wakeup events in a
> higher layer in the kernel.
> 
> Fixes: b12fbc3f787e ("virtio_balloon: stay awake while adjusting balloon")
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>   drivers/virtio/virtio_balloon.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 1f5b3dd31fcf..7fe7ef5f1c77 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -450,7 +450,7 @@ static void start_update_balloon_size(struct virtio_balloon *vb)
>   	vb->adjustment_signal_pending = true;
>   	if (!vb->adjustment_in_progress) {
>   		vb->adjustment_in_progress = true;
> -		pm_stay_awake(vb->vdev->dev.parent);
> +		pm_stay_awake(&vb->vdev->dev);
>   	}
>   	spin_unlock_irqrestore(&vb->adjustment_lock, flags);
>   
> @@ -462,7 +462,7 @@ static void end_update_balloon_size(struct virtio_balloon *vb)
>   	spin_lock_irq(&vb->adjustment_lock);
>   	if (!vb->adjustment_signal_pending && vb->adjustment_in_progress) {
>   		vb->adjustment_in_progress = false;
> -		pm_relax(vb->vdev->dev.parent);
> +		pm_relax(&vb->vdev->dev);
>   	}
>   	spin_unlock_irq(&vb->adjustment_lock);
>   }
> @@ -1028,6 +1028,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
>   	}
>   
>   	spin_lock_init(&vb->adjustment_lock);

Can we add a comment here why we have to do that?

> +	device_set_wakeup_capable(&vb->vdev->dev, true);
>   
>   	virtio_device_ready(vdev);
>   

Absolutely not an expert on the details, but I assume this is fine.

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb