On Tue, Oct 14, 2014 at 11:01:12AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@xxxxxxxxxx> writes:
> > Defer config changed notifications that arrive during
> > probe/scan/freeze/restore.
> >
> > This will allow drivers to set DRIVER_OK earlier, without worrying about
> > racing with config change interrupts.
> >
> > This change will also benefit old hypervisors (before 2009)
> > that send interrupts without checking DRIVER_OK: previously,
> > the callback could race with driver-specific initialization.
> >
> > This will also help simplify drivers.
>
> But AFAICT you never *read* dev->config_changed.
>
> You unconditionally trigger a config_changed event in
> virtio_config_enable(). That's a bit weird, but probably OK.
>
> How about the following change (on top of your patch). I
> think the renaming is clearer, and note the added if() test in
> virtio_config_enable().
>
> If you approve, I'll fold it in.
>
> Cheers,
> Rusty.
Hi Rusty,
I'm okay with both changes.
You can fold it in if you prefer, or just make it a patch on top.
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 2536701b098b..df598dd8c5c8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -122,7 +122,7 @@ static void __virtio_config_changed(struct virtio_device *dev)
> struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>
> if (!dev->config_enabled)
> - dev->config_changed = true;
> + dev->config_change_pending = true;
> else if (drv && drv->config_changed)
> drv->config_changed(dev);
> }
> @@ -148,8 +148,9 @@ static void virtio_config_enable(struct virtio_device *dev)
> {
> spin_lock_irq(&dev->config_lock);
> dev->config_enabled = true;
> - __virtio_config_changed(dev);
> - dev->config_changed = false;
> + if (dev->config_change_pending)
> + __virtio_config_changed(dev);
> + dev->config_change_pending = false;
> spin_unlock_irq(&dev->config_lock);
> }
>
> @@ -253,7 +254,7 @@ int register_virtio_device(struct virtio_device *dev)
>
> spin_lock_init(&dev->config_lock);
> dev->config_enabled = false;
> - dev->config_changed = false;
> + dev->config_change_pending = false;
>
> /* We always start by resetting the device, in case a previous
> * driver messed it up. This also tests that code path a little. */
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 5636b119dc25..65261a7244fc 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -80,7 +80,7 @@ bool virtqueue_is_broken(struct virtqueue *vq);
> * @index: unique position on the virtio bus
> * @failed: saved value for CONFIG_S_FAILED bit (for restore)
> * @config_enabled: configuration change reporting enabled
> - * @config_changed: configuration change reported while disabled
> + * @config_change_pending: configuration change reported while disabled
> * @config_lock: protects configuration change reporting
> * @dev: underlying device.
> * @id: the device type identification (used to match it with a driver).
> @@ -94,7 +94,7 @@ struct virtio_device {
> int index;
> bool failed;
> bool config_enabled;
> - bool config_changed;
> + bool config_change_pending;
> spinlock_t config_lock;
> struct device dev;
> struct virtio_device_id id;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
