Re: [PATCH v4 1/1] s390/vfio-ap: fix circular lockdep when setting/clearing crypto masks

[Halil Pasic <pasic@linux.ibm.com>]

On Thu, 18 Mar 2021 14:38:53 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 3/17/21 7:17 PM, Halil Pasic wrote:
> > On Wed, 10 Mar 2021 10:05:59 -0500
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >  
> >> -		ret = vfio_ap_mdev_reset_queues(mdev);
> >> +		matrix_mdev = mdev_get_drvdata(mdev);  
> > Is it guaranteed that matrix_mdev can't be NULL here? If yes, please
> > remind me of the mechanism that ensures this.
> >  
> >> +
> >> +		/*
> >> +		 * If the KVM pointer is in the process of being set, wait until
> >> +		 * the process has completed.
> >> +		 */
> >> +		wait_event_cmd(matrix_mdev->wait_for_kvm,
> >> +			       matrix_mdev->kvm_busy == false,
> >> +			       mutex_unlock(&matrix_dev->lock),
> >> +			       mutex_lock(&matrix_dev->lock));
> >> +
> >> +		if (matrix_mdev->kvm)
> >> +			ret = vfio_ap_mdev_reset_queues(mdev);
> >> +		else
> >> +			ret = -ENODEV;  
> > Didn't we agree to make the call to vfio_ap_mdev_reset_queues()
> > unconditional again (for reference please take look at
> > Message-ID: <64afa72c-2d6a-2ca1-e576-34e15fa579ed@linux.ibm.com>)?  
> 
> How about this:

Looks good. I will check the mdev code if the checkeck is really
needed. I'm curious when the sysfs files associated with a new mdev are
created. My guess is that this one comes in via a device specific file
(not the parent like in case of the create), and that those may be
created after the create. But we can get rid of the check any time so I
really don't see it as something that would preclude merging this.

Regards,
Halil

> 
> static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
>                      unsigned int cmd, unsigned long arg)
> {
>      int ret = 0;
>      struct ap_matrix_mdev *matrix_mdev;
> 
>      ...
>      case VFIO_DEVICE_RESET:
>          matrix_mdev = mdev_get_drvdata(mdev);
>          WARN(!matrix_mdev, "Driver data missing from mdev!!");
> 
>          if (matrix_mdev) {
>              /*
>               * If the KVM pointer is in the process of being set, wait 
> until
>               * the process has completed.
>               */
>              wait_event_cmd(matrix_mdev->wait_for_kvm,
>                         matrix_mdev->kvm_busy == false,
> mutex_unlock(&matrix_dev->lock),
> mutex_lock(&matrix_dev->lock));
> 
>              ret = vfio_ap_mdev_reset_queues(mdev);
>          }
>          break;
>      ...
> 
>      return ret;
> }
> 
> >
> > Regards,
> > Halil  
>