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’ ? I was mainly concerned about locking rules w.r.t. existing ‘memory_lock’ and the code present in vfio_pci_zap_and_down_write_memory_lock() which is internally taking ‘mmap_lock’ and ‘vma_lock’. But from the initial analysis, it seems this should not cause any issue since we should not need ‘power_lock’ in the mmap fault handler or any read/write functions. We can maintain following locking order power_lock => memory_lock 1. At the beginning of config space access or ioctl, we can take the lock down_read(&vdev->power_lock); if (vdev->platform_pm_engaged) { up_read(&vdev->power_lock); return -EIO; } And before returning from config or ioctl, we can release the lock. 2. Now ‘platform_pm_engaged’ is not protected with memory_lock and we need to support the case where VFIO_DEVICE_FEATURE_POWER_MANAGEMENT can be called without putting the device into D3hot explicitly. So, I need to introduce a second variable which tracks the memory disablement (like power_state_d3 in this patch) and will be protected with 'memory_lock'. It will be set for both the cases, where users change the power state to D3hot by config write or user makes this ioctl. Inside vfio_pci_core_feature_pm(), now the code will become 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); 3. Inside __vfio_pci_memory_enabled(), we can check vdev->power_state_d3 instead of current_state. 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(). Thanks, Abhishek