From: Alex Williamson <alex.williamson@redhat.com>
To: Abhishek Sahu <abhsahu@nvidia.com>
Cc: Cornelia Huck <cohuck@redhat.com>,
Yishai Hadas <yishaih@nvidia.com>,
Jason Gunthorpe <jgg@nvidia.com>,
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>,
Kevin Tian <kevin.tian@intel.com>,
"Rafael J . Wysocki" <rafael@kernel.org>,
Max Gurtovoy <mgurtovoy@nvidia.com>,
Bjorn Helgaas <bhelgaas@google.com>,
linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
linux-pm@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH v3 6/8] vfio: Invoke runtime PM API for IOCTL request
Date: Mon, 9 May 2022 16:30:02 -0600 [thread overview]
Message-ID: <20220509163002.57fe44fa.alex.williamson@redhat.com> (raw)
In-Reply-To: <0ba3d469-58af-64d3-514c-6d33c483f8fb@nvidia.com>
On Thu, 5 May 2022 15:10:43 +0530
Abhishek Sahu <abhsahu@nvidia.com> wrote:
> On 5/5/2022 1:12 AM, Alex Williamson wrote:
> > On Mon, 25 Apr 2022 14:56:13 +0530
> > Abhishek Sahu <abhsahu@nvidia.com> wrote:
> >
> >> The vfio/pci driver will have runtime power management support where the
> >> user can put the device low power state and then PCI devices can go into
> >> the D3cold state. If the device is in low power state and user issues any
> >> IOCTL, then the device should be moved out of low power state first. Once
> >> the IOCTL is serviced, then it can go into low power state again. The
> >> runtime PM framework manages this with help of usage count. One option
> >> was to add the runtime PM related API's inside vfio/pci driver but some
> >> IOCTL (like VFIO_DEVICE_FEATURE) can follow a different path and more
> >> IOCTL can be added in the future. Also, the runtime PM will be
> >> added for vfio/pci based drivers variant currently but the other vfio
> >> based drivers can use the same in the future. So, this patch adds the
> >> runtime calls runtime related API in the top level IOCTL function itself.
> >>
> >> For the vfio drivers which do not have runtime power management support
> >> currently, the runtime PM API's won't be invoked. Only for vfio/pci
> >> based drivers currently, the runtime PM API's will be invoked to increment
> >> and decrement the usage count. Taking this usage count incremented while
> >> servicing IOCTL will make sure that user won't put the device into low
> >> power state when any other IOCTL is being serviced in parallel.
> >>
> >> Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
> >> ---
> >> drivers/vfio/vfio.c | 44 +++++++++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 41 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >> index a4555014bd1e..4e65a127744e 100644
> >> --- a/drivers/vfio/vfio.c
> >> +++ b/drivers/vfio/vfio.c
> >> @@ -32,6 +32,7 @@
> >> #include <linux/vfio.h>
> >> #include <linux/wait.h>
> >> #include <linux/sched/signal.h>
> >> +#include <linux/pm_runtime.h>
> >> #include "vfio.h"
> >>
> >> #define DRIVER_VERSION "0.3"
> >> @@ -1536,6 +1537,30 @@ static const struct file_operations vfio_group_fops = {
> >> .release = vfio_group_fops_release,
> >> };
> >>
> >> +/*
> >> + * Wrapper around pm_runtime_resume_and_get().
> >> + * Return 0, if driver power management callbacks are not present i.e. the driver is not
> >
> > Mind the gratuitous long comment line here.
> >
>
> Thanks Alex.
>
> That was a miss. I will fix this.
>
> >> + * using runtime power management.
> >> + * Return 1 upon success, otherwise -errno
> >
> > Changing semantics vs the thing we're wrapping, why not provide a
> > wrapper for the `put` as well to avoid? The only cases where we return
> > zero are just as easy to detect on the other side.
> >
>
> Yes. Using wrapper function for put is better option.
> I will make the changes.
>
> >> + */
> >> +static inline int vfio_device_pm_runtime_get(struct device *dev)
> >
> > Given some of Jason's recent series, this should probably just accept a
> > vfio_device.
> >
>
> Sorry. I didn't get this part.
>
> Do I need to change it to
>
> static inline int vfio_device_pm_runtime_get(struct vfio_device *device)
> {
> struct device *dev = device->dev;
> ...
> }
Yes.
> >> +{
> >> +#ifdef CONFIG_PM
> >> + int ret;
> >> +
> >> + if (!dev->driver || !dev->driver->pm)
> >> + return 0;
I'm also wondering how we could ever get here with dev->driver == NULL.
If that were actually possible, the above would at best be racy. It
also really seems like there ought to be a better test than the
driver->pm pointer to check if runtime pm is enabled, but I haven't
spotted it yet.
> >> +
> >> + ret = pm_runtime_resume_and_get(dev);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return 1;
> >> +#else
> >> + return 0;
> >> +#endif
> >> +}
> >> +
> >> /*
> >> * VFIO Device fd
> >> */
> >> @@ -1845,15 +1870,28 @@ static long vfio_device_fops_unl_ioctl(struct file *filep,
> >> unsigned int cmd, unsigned long arg)
> >> {
> >> struct vfio_device *device = filep->private_data;
> >> + int pm_ret, ret = 0;
> >> +
> >> + pm_ret = vfio_device_pm_runtime_get(device->dev);
> >> + if (pm_ret < 0)
> >> + return pm_ret;
> >
> > I wonder if we might simply want to mask pm errors behind -EIO, maybe
> > with a rate limited dev_info(). My concern would be that we might mask
> > errnos that userspace has come to expect for certain ioctls. Thanks,
> >
> > Alex
> >
>
> I need to do something like following. Correct ?
>
> ret = vfio_device_pm_runtime_get(device);
> if (ret < 0) {
> dev_info_ratelimited(device->dev, "vfio: runtime resume failed %d\n", ret);
> return -EIO;
> }
Yeah, though I'd welcome other thoughts here. I don't necessarily like
the idea of squashing the errno, but at the same time, if
pm_runtime_resume_and_get() returns -EINVAL on user ioctl, that's not
really describing an invalid parameter relative to the ioctl itself.
Thanks,
Alex
next prev parent reply other threads:[~2022-05-09 22:30 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-25 9:26 [PATCH v3 0/8] vfio/pci: power management changes Abhishek Sahu
2022-04-25 9:26 ` [PATCH v3 1/8] vfio/pci: Invalidate mmaps and block the access in D3hot power state Abhishek Sahu
2022-04-26 1:42 ` kernel test robot
2022-04-26 14:14 ` Bjorn Helgaas
2022-04-25 9:26 ` [PATCH v3 2/8] vfio/pci: Change the PF power state to D0 before enabling VFs Abhishek Sahu
2022-04-25 9:26 ` [PATCH v3 3/8] vfio/pci: Virtualize PME related registers bits and initialize to zero Abhishek Sahu
2022-04-25 9:26 ` [PATCH v3 4/8] vfio/pci: Add support for setting driver data inside core layer Abhishek Sahu
2022-05-03 17:11 ` Alex Williamson
2022-05-04 0:20 ` Jason Gunthorpe
2022-05-04 10:32 ` Abhishek Sahu
2022-04-25 9:26 ` [PATCH v3 5/8] vfio/pci: Enable runtime PM for vfio_pci_core based drivers Abhishek Sahu
2022-05-04 19:42 ` Alex Williamson
2022-05-05 9:07 ` Abhishek Sahu
2022-04-25 9:26 ` [PATCH v3 6/8] vfio: Invoke runtime PM API for IOCTL request Abhishek Sahu
2022-05-04 19:42 ` Alex Williamson
2022-05-05 9:40 ` Abhishek Sahu
2022-05-09 22:30 ` Alex Williamson [this message]
2022-04-25 9:26 ` [PATCH v3 7/8] vfio/pci: Mask INTx during runtime suspend Abhishek Sahu
2022-04-25 9:26 ` [PATCH v3 8/8] vfio/pci: Add the support for PCI D3cold state Abhishek Sahu
2022-05-04 19:45 ` Alex Williamson
2022-05-05 12:16 ` Abhishek Sahu
2022-05-09 21:48 ` Alex Williamson
2022-05-10 13:26 ` Abhishek Sahu
2022-05-10 13:30 ` Jason Gunthorpe
2022-05-12 12:27 ` Abhishek Sahu
2022-05-12 12:47 ` Jason Gunthorpe
2022-05-30 11:15 ` Abhishek Sahu
2022-05-30 12:25 ` Jason Gunthorpe
2022-05-31 12:14 ` Abhishek Sahu
2022-05-31 19:43 ` Jason Gunthorpe
2022-05-31 22:52 ` Alex Williamson
2022-06-01 9:49 ` Abhishek Sahu
2022-06-01 16:21 ` Alex Williamson
2022-06-01 17:30 ` Jason Gunthorpe
2022-06-01 18:15 ` Alex Williamson
2022-06-01 23:17 ` Jason Gunthorpe
2022-06-02 11:52 ` Abhishek Sahu
2022-06-02 17:44 ` Alex Williamson
2022-06-03 10:19 ` Abhishek Sahu
2022-06-07 21:50 ` Alex Williamson
2022-06-08 10:12 ` Abhishek Sahu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220509163002.57fe44fa.alex.williamson@redhat.com \
--to=alex.williamson@redhat.com \
--cc=abhsahu@nvidia.com \
--cc=bhelgaas@google.com \
--cc=cohuck@redhat.com \
--cc=jgg@nvidia.com \
--cc=kevin.tian@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=mgurtovoy@nvidia.com \
--cc=rafael@kernel.org \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=yishaih@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).