Re: [PATCH 1/1] virtio: Add support for the virtio suspend feature
On Thu, Apr 18, 2024 at 03:14:37PM +0800, Zhu, Lingshan wrote: > > > On 4/17/2024 4:54 PM, David Stevens wrote: > > Add support for the VIRTIO_F_SUSPEND feature. When this feature is > > negotiated, power management can use it to suspend virtio devices > > instead of resorting to resetting the devices entirely. > > > > Signed-off-by: David Stevens > > --- > > drivers/virtio/virtio.c| 32 ++ > > drivers/virtio/virtio_pci_common.c | 29 +++ > > drivers/virtio/virtio_pci_modern.c | 19 ++ > > include/linux/virtio.h | 2 ++ > > include/uapi/linux/virtio_config.h | 10 +- > > 5 files changed, 74 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index f4080692b351..cd11495a5098 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -1,5 +1,6 @@ > > // SPDX-License-Identifier: GPL-2.0-only > > #include > > +#include > > #include > > #include > > #include > > @@ -580,6 +581,37 @@ int virtio_device_restore(struct virtio_device *dev) > > return ret; > > } > > EXPORT_SYMBOL_GPL(virtio_device_restore); > > + > > +static int virtio_device_set_suspend_bit(struct virtio_device *dev, bool > > enabled) > > +{ > > + u8 status, target; > > + > > + status = dev->config->get_status(dev); > > + if (enabled) > > + target = status | VIRTIO_CONFIG_S_SUSPEND; > > + else > > + target = status & ~VIRTIO_CONFIG_S_SUSPEND; > > + dev->config->set_status(dev, target); > I think it is better to verify whether the device SUSPEND bit is > already set or clear, we can just return if status == target. > > Thanks > Zhu Lingshan > > + > > + while ((status = dev->config->get_status(dev)) != target) { > > + if (status & VIRTIO_CONFIG_S_NEEDS_RESET) > > + return -EIO; > > + mdelay(10); Bad device state (set by surprise removal) should also be handled here I think. > > + } > > + return 0; > > +} > > + > > +int virtio_device_suspend(struct virtio_device *dev) > > +{ > > + return virtio_device_set_suspend_bit(dev, true); > > +} > > +EXPORT_SYMBOL_GPL(virtio_device_suspend); > > + > > +int virtio_device_resume(struct virtio_device *dev) > > +{ > > + return virtio_device_set_suspend_bit(dev, false); > > +} > > +EXPORT_SYMBOL_GPL(virtio_device_resume); > > #endif > > static int virtio_init(void) > > diff --git a/drivers/virtio/virtio_pci_common.c > > b/drivers/virtio/virtio_pci_common.c > > index b655fccaf773..4d542de05970 100644 > > --- a/drivers/virtio/virtio_pci_common.c > > +++ b/drivers/virtio/virtio_pci_common.c > > @@ -495,31 +495,26 @@ static int virtio_pci_restore(struct device *dev) > > return virtio_device_restore(_dev->vdev); > > } > > -static bool vp_supports_pm_no_reset(struct device *dev) > > +static int virtio_pci_suspend(struct device *dev) > > { > > struct pci_dev *pci_dev = to_pci_dev(dev); > > - u16 pmcsr; > > - > > - if (!pci_dev->pm_cap) > > - return false; > > - > > - pci_read_config_word(pci_dev, pci_dev->pm_cap + PCI_PM_CTRL, ); > > - if (PCI_POSSIBLE_ERROR(pmcsr)) { > > - dev_err(dev, "Unable to query pmcsr"); > > - return false; > > - } > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); > > - return pmcsr & PCI_PM_CTRL_NO_SOFT_RESET; > > -} > > + if (virtio_has_feature(_dev->vdev, VIRTIO_F_SUSPEND)) > > + return virtio_device_suspend(_dev->vdev); > > -static int virtio_pci_suspend(struct device *dev) > > -{ > > - return vp_supports_pm_no_reset(dev) ? 0 : virtio_pci_freeze(dev); > > + return virtio_pci_freeze(dev); > > } > > static int virtio_pci_resume(struct device *dev) > > { > > - return vp_supports_pm_no_reset(dev) ? 0 : virtio_pci_restore(dev); > > + struct pci_dev *pci_dev = to_pci_dev(dev); > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); > > + > > + if (virtio_has_feature(_dev->vdev, VIRTIO_F_SUSPEND)) > > + return virtio_device_resume(_dev->vdev); > > + > > + return virtio_pci_restore(dev); > > } > > static const struct dev_pm_ops virtio_pci_pm_ops = { > > diff --git a/drivers/virtio/virtio_pci_modern.c > > b/drivers/virtio/virtio_pci_modern.c > > index f62b530aa3b5..ac8734526b8d 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -209,6 +209,22 @@ static void vp_modern_avq_deactivate(struct > > virtio_device *vdev) > > __virtqueue_break(admin_vq->info.vq); > > } > > +static bool vp_supports_pm_no_reset(struct pci_dev *pci_dev) > > +{ > > + u16 pmcsr; > > + > > + if (!pci_dev->pm_cap) > > + return false; > > + > > + pci_read_config_word(pci_dev, pci_dev->pm_cap + PCI_PM_CTRL, ); > > + if (PCI_POSSIBLE_ERROR(pmcsr)) { > > + dev_err(_dev->dev, "Unable to query pmcsr"); > > + return false;
Re: [PATCH 1/1] virtio: Add support for the virtio suspend feature
On 4/17/2024 4:54 PM, David Stevens wrote: Add support for the VIRTIO_F_SUSPEND feature. When this feature is negotiated, power management can use it to suspend virtio devices instead of resorting to resetting the devices entirely. Signed-off-by: David Stevens --- drivers/virtio/virtio.c| 32 ++ drivers/virtio/virtio_pci_common.c | 29 +++ drivers/virtio/virtio_pci_modern.c | 19 ++ include/linux/virtio.h | 2 ++ include/uapi/linux/virtio_config.h | 10 +- 5 files changed, 74 insertions(+), 18 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index f4080692b351..cd11495a5098 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only #include +#include #include #include #include @@ -580,6 +581,37 @@ int virtio_device_restore(struct virtio_device *dev) return ret; } EXPORT_SYMBOL_GPL(virtio_device_restore); + +static int virtio_device_set_suspend_bit(struct virtio_device *dev, bool enabled) +{ + u8 status, target; + + status = dev->config->get_status(dev); + if (enabled) + target = status | VIRTIO_CONFIG_S_SUSPEND; + else + target = status & ~VIRTIO_CONFIG_S_SUSPEND; + dev->config->set_status(dev, target); I think it is better to verify whether the device SUSPEND bit is already set or clear, we can just return if status == target. Thanks Zhu Lingshan + + while ((status = dev->config->get_status(dev)) != target) { + if (status & VIRTIO_CONFIG_S_NEEDS_RESET) + return -EIO; + mdelay(10); + } + return 0; +} + +int virtio_device_suspend(struct virtio_device *dev) +{ + return virtio_device_set_suspend_bit(dev, true); +} +EXPORT_SYMBOL_GPL(virtio_device_suspend); + +int virtio_device_resume(struct virtio_device *dev) +{ + return virtio_device_set_suspend_bit(dev, false); +} +EXPORT_SYMBOL_GPL(virtio_device_resume); #endif static int virtio_init(void) diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index b655fccaf773..4d542de05970 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -495,31 +495,26 @@ static int virtio_pci_restore(struct device *dev) return virtio_device_restore(_dev->vdev); } -static bool vp_supports_pm_no_reset(struct device *dev) +static int virtio_pci_suspend(struct device *dev) { struct pci_dev *pci_dev = to_pci_dev(dev); - u16 pmcsr; - - if (!pci_dev->pm_cap) - return false; - - pci_read_config_word(pci_dev, pci_dev->pm_cap + PCI_PM_CTRL, ); - if (PCI_POSSIBLE_ERROR(pmcsr)) { - dev_err(dev, "Unable to query pmcsr"); - return false; - } + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); - return pmcsr & PCI_PM_CTRL_NO_SOFT_RESET; -} + if (virtio_has_feature(_dev->vdev, VIRTIO_F_SUSPEND)) + return virtio_device_suspend(_dev->vdev); -static int virtio_pci_suspend(struct device *dev) -{ - return vp_supports_pm_no_reset(dev) ? 0 : virtio_pci_freeze(dev); + return virtio_pci_freeze(dev); } static int virtio_pci_resume(struct device *dev) { - return vp_supports_pm_no_reset(dev) ? 0 : virtio_pci_restore(dev); + struct pci_dev *pci_dev = to_pci_dev(dev); + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev); + + if (virtio_has_feature(_dev->vdev, VIRTIO_F_SUSPEND)) + return virtio_device_resume(_dev->vdev); + + return virtio_pci_restore(dev); } static const struct dev_pm_ops virtio_pci_pm_ops = { diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c index f62b530aa3b5..ac8734526b8d 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c @@ -209,6 +209,22 @@ static void vp_modern_avq_deactivate(struct virtio_device *vdev) __virtqueue_break(admin_vq->info.vq); } +static bool vp_supports_pm_no_reset(struct pci_dev *pci_dev) +{ + u16 pmcsr; + + if (!pci_dev->pm_cap) + return false; + + pci_read_config_word(pci_dev, pci_dev->pm_cap + PCI_PM_CTRL, ); + if (PCI_POSSIBLE_ERROR(pmcsr)) { + dev_err(_dev->dev, "Unable to query pmcsr"); + return false; + } + + return pmcsr & PCI_PM_CTRL_NO_SOFT_RESET; +} + static void vp_transport_features(struct virtio_device *vdev, u64 features) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); @@ -223,6 +239,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features) if (features & BIT_ULL(VIRTIO_F_ADMIN_VQ)) __virtio_set_bit(vdev, VIRTIO_F_ADMIN_VQ); + + if (features &