On Tue, May 31, 2022 at 05:44:11PM +0530, Abhishek Sahu wrote:
> On 5/30/2022 5:55 PM, Jason Gunthorpe wrote:
> > On Mon, May 30, 2022 at 04:45:59PM +0530, Abhishek Sahu wrote:
> >
> >> 1. In real use case, config or any other ioctl should not come along
> >> with VFIO_DEVICE_FEATURE_POWER_MANAGEMENT ioctl request.
> >>
> >> 2. Maintain some 'access_count' which will be incremented when we
> >> do any config space access or ioctl.
> >
> > Please don't open code locks - if you need a lock then write a proper
> > lock. You can use the 'try' variants to bail out in cases where that
> > is appropriate.
> >
> > Jason
>
> Thanks Jason for providing your inputs.
>
> In that case, should I introduce new rw_semaphore (For example
> power_lock) and move ‘platform_pm_engaged’ under ‘power_lock’ ?
Possibly, this is better than an atomic at least
> 1. At the beginning of config space access or ioctl, we can take the
> lock
>
> down_read(&vdev->power_lock);
You can also do down_read_trylock() here and bail out as you were
suggesting with the atomic.
trylock doesn't have lock odering rules because it can't sleep so it
gives a bit more flexability when designing the lock ordering.
Though userspace has to be able to tolerate the failure, or never make
the request.
> down_write(&vdev->power_lock);
> ...
> switch (vfio_pm.low_power_state) {
> case VFIO_DEVICE_LOW_POWER_STATE_ENTER:
> ...
> vfio_pci_zap_and_down_write_memory_lock(vdev);
> vdev->power_state_d3 = true;
> up_write(&vdev->memory_lock);
>
> ...
> up_write(&vdev->power_lock);
And something checks the power lock before allowing the memor to be
re-enabled?
> 4. For ioctl access, as mentioned previously I need to add two
> callbacks functions (one for start and one for end) in the struct
> vfio_device_ops and call the same at start and end of ioctl from
> vfio_device_fops_unl_ioctl().
Not sure I followed this..
Jason
