Re: [PATCH v13 02/15] s390/vfio-ap: No need to disable IRQ after queue reset
On Wed, 13 Jan 2021 19:46:03 -0500
Tony Krowiak wrote:
> On 1/13/21 4:21 PM, Halil Pasic wrote:
> > On Wed, 13 Jan 2021 12:06:28 -0500
> > Tony Krowiak wrote:
> >
> >> On 1/11/21 11:32 AM, Halil Pasic wrote:
> >>> On Tue, 22 Dec 2020 20:15:53 -0500
> >>> Tony Krowiak wrote:
> >>>
> The queues assigned to a matrix mediated device are currently reset when:
>
> * The VFIO_DEVICE_RESET ioctl is invoked
> * The mdev fd is closed by userspace (QEMU)
> * The mdev is removed from sysfs.
>
> Immediately after the reset of a queue, a call is made to disable
> interrupts for the queue. This is entirely unnecessary because the reset
> of
> a queue disables interrupts, so this will be removed.
>
> Signed-off-by: Tony Krowiak
> ---
> drivers/s390/crypto/vfio_ap_drv.c | 1 -
> drivers/s390/crypto/vfio_ap_ops.c | 40 +--
> drivers/s390/crypto/vfio_ap_private.h | 1 -
> 3 files changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c
> b/drivers/s390/crypto/vfio_ap_drv.c
> index be2520cc010b..ca18c91afec9 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device
> *apdev)
> apid = AP_QID_CARD(q->apqn);
> apqi = AP_QID_QUEUE(q->apqn);
> vfio_ap_mdev_reset_queue(apid, apqi, 1);
> -vfio_ap_irq_disable(q);
> kfree(q);
> mutex_unlock(_dev->lock);
> }
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index 7339043906cf..052f61391ec7 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -25,6 +25,7 @@
> #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
>
> static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
>
> static int match_apqn(struct device *dev, const void *data)
> {
> @@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
> int apqn)
> {
> struct vfio_ap_queue *q;
> -struct device *dev;
>
> if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
> return NULL;
> if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
> return NULL;
>
> -dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
> - , match_apqn);
> -if (!dev)
> -return NULL;
> -q = dev_get_drvdata(dev);
> -q->matrix_mdev = matrix_mdev;
> -put_device(dev);
> +q = vfio_ap_find_queue(apqn);
> +if (q)
> +q->matrix_mdev = matrix_mdev;
>
> return q;
> }
> @@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct
> notifier_block *nb,
> return notify_rc;
> }
>
> -static void (int apqn)
> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
> {
> struct device *dev;
> -struct vfio_ap_queue *q;
> +struct vfio_ap_queue *q = NULL;
>
> dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
> , match_apqn);
> if (dev) {
> q = dev_get_drvdata(dev);
> -vfio_ap_irq_disable(q);
> put_device(dev);
> }
> +
> +return q;
> }
> >>> This hunk and the previous one are a rewrite of vfio_ap_get_queue() and
> >>> have next to nothing to do with the patch's objective. If we were at an
> >>> earlier stage, I would ask to split it up.
> >> The rewrite of vfio_ap_get_queue() definitely is related to this
> >> patch's objective.
> > Definitively loosely related.
>
> A matter of opinion I suppose and I respect yours.
>
> >
> >> Below, in the vfio_ap_mdev_reset_queue()
> >> function, there is the label 'free_aqic_resources' which is where
> >> the call to vfio_ap_free_aqic_resources() function is called.
> >> That function takes a struct vfio_ap_queue as an argument,
> >> so the object needs to be retrieved prior to calling the function.
> >> We can't use the vfio_ap_get_queue() function for two reasons:
> >> 1. The vfio_ap_get_queue() function takes a struct ap_matrix_mdev
> >> as a parameter and we do not have a pointer to such at the time.
> >> 2. The vfio_ap_get_queue() function is used to link the mdev to the
> >>
Re: [PATCH v13 02/15] s390/vfio-ap: No need to disable IRQ after queue reset
On Wed, 13 Jan 2021 12:06:28 -0500
Tony Krowiak wrote:
> On 1/11/21 11:32 AM, Halil Pasic wrote:
> > On Tue, 22 Dec 2020 20:15:53 -0500
> > Tony Krowiak wrote:
> >
> >> The queues assigned to a matrix mediated device are currently reset when:
> >>
> >> * The VFIO_DEVICE_RESET ioctl is invoked
> >> * The mdev fd is closed by userspace (QEMU)
> >> * The mdev is removed from sysfs.
> >>
> >> Immediately after the reset of a queue, a call is made to disable
> >> interrupts for the queue. This is entirely unnecessary because the reset of
> >> a queue disables interrupts, so this will be removed.
> >>
> >> Signed-off-by: Tony Krowiak
> >> ---
> >> drivers/s390/crypto/vfio_ap_drv.c | 1 -
> >> drivers/s390/crypto/vfio_ap_ops.c | 40 +--
> >> drivers/s390/crypto/vfio_ap_private.h | 1 -
> >> 3 files changed, 26 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/s390/crypto/vfio_ap_drv.c
> >> b/drivers/s390/crypto/vfio_ap_drv.c
> >> index be2520cc010b..ca18c91afec9 100644
> >> --- a/drivers/s390/crypto/vfio_ap_drv.c
> >> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> >> @@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device
> >> *apdev)
> >>apid = AP_QID_CARD(q->apqn);
> >>apqi = AP_QID_QUEUE(q->apqn);
> >>vfio_ap_mdev_reset_queue(apid, apqi, 1);
> >> - vfio_ap_irq_disable(q);
> >>kfree(q);
> >>mutex_unlock(_dev->lock);
> >> }
> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> >> b/drivers/s390/crypto/vfio_ap_ops.c
> >> index 7339043906cf..052f61391ec7 100644
> >> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >> @@ -25,6 +25,7 @@
> >> #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
> >>
> >> static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
> >> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
> >>
> >> static int match_apqn(struct device *dev, const void *data)
> >> {
> >> @@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
> >>int apqn)
> >> {
> >>struct vfio_ap_queue *q;
> >> - struct device *dev;
> >>
> >>if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
> >>return NULL;
> >>if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
> >>return NULL;
> >>
> >> - dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
> >> - , match_apqn);
> >> - if (!dev)
> >> - return NULL;
> >> - q = dev_get_drvdata(dev);
> >> - q->matrix_mdev = matrix_mdev;
> >> - put_device(dev);
> >> + q = vfio_ap_find_queue(apqn);
> >> + if (q)
> >> + q->matrix_mdev = matrix_mdev;
> >>
> >>return q;
> >> }
> >> @@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct
> >> notifier_block *nb,
> >>return notify_rc;
> >> }
> >>
> >> -static void (int apqn)
> >> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
> >> {
> >>struct device *dev;
> >> - struct vfio_ap_queue *q;
> >> + struct vfio_ap_queue *q = NULL;
> >>
> >>dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
> >> , match_apqn);
> >>if (dev) {
> >>q = dev_get_drvdata(dev);
> >> - vfio_ap_irq_disable(q);
> >>put_device(dev);
> >>}
> >> +
> >> + return q;
> >> }
> > This hunk and the previous one are a rewrite of vfio_ap_get_queue() and
> > have next to nothing to do with the patch's objective. If we were at an
> > earlier stage, I would ask to split it up.
>
> The rewrite of vfio_ap_get_queue() definitely is related to this
> patch's objective.
Definitively loosely related.
> Below, in the vfio_ap_mdev_reset_queue()
> function, there is the label 'free_aqic_resources' which is where
> the call to vfio_ap_free_aqic_resources() function is called.
> That function takes a struct vfio_ap_queue as an argument,
> so the object needs to be retrieved prior to calling the function.
> We can't use the vfio_ap_get_queue() function for two reasons:
> 1. The vfio_ap_get_queue() function takes a struct ap_matrix_mdev
> as a parameter and we do not have a pointer to such at the time.
> 2. The vfio_ap_get_queue() function is used to link the mdev to the
> vfio_ap_queue object with the specified APQN.
> So, we needed a way to retrieve the vfio_ap_queue object by its
> APQN only, Rather than creating a function that retrieves the
> vfio_ap_queue object which duplicates the retrieval code in
> vfio_ap_get_queue(), I created the vfio_ap_find_queue()
> function to do just that and modified the vfio_ap_get_queue()
> function to call it (i.e., code reuse).
Please tell me what prevented you from doing a doing the splitting out
vfio_ap_find_queue() from vfio_ap_get_queue() in a separate patch, that
precedes this patch? It would have resulted in simpler diffs, because
the split out wouldn't be intermingled with other
Re: [PATCH v13 02/15] s390/vfio-ap: No need to disable IRQ after queue reset
On 1/13/21 4:21 PM, Halil Pasic wrote:
On Wed, 13 Jan 2021 12:06:28 -0500
Tony Krowiak wrote:
On 1/11/21 11:32 AM, Halil Pasic wrote:
On Tue, 22 Dec 2020 20:15:53 -0500
Tony Krowiak wrote:
The queues assigned to a matrix mediated device are currently reset when:
* The VFIO_DEVICE_RESET ioctl is invoked
* The mdev fd is closed by userspace (QEMU)
* The mdev is removed from sysfs.
Immediately after the reset of a queue, a call is made to disable
interrupts for the queue. This is entirely unnecessary because the reset of
a queue disables interrupts, so this will be removed.
Signed-off-by: Tony Krowiak
---
drivers/s390/crypto/vfio_ap_drv.c | 1 -
drivers/s390/crypto/vfio_ap_ops.c | 40 +--
drivers/s390/crypto/vfio_ap_private.h | 1 -
3 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_drv.c
b/drivers/s390/crypto/vfio_ap_drv.c
index be2520cc010b..ca18c91afec9 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
apid = AP_QID_CARD(q->apqn);
apqi = AP_QID_QUEUE(q->apqn);
vfio_ap_mdev_reset_queue(apid, apqi, 1);
- vfio_ap_irq_disable(q);
kfree(q);
mutex_unlock(_dev->lock);
}
diff --git a/drivers/s390/crypto/vfio_ap_ops.c
b/drivers/s390/crypto/vfio_ap_ops.c
index 7339043906cf..052f61391ec7 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -25,6 +25,7 @@
#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
static int match_apqn(struct device *dev, const void *data)
{
@@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
int apqn)
{
struct vfio_ap_queue *q;
- struct device *dev;
if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
return NULL;
if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
return NULL;
- dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
-, match_apqn);
- if (!dev)
- return NULL;
- q = dev_get_drvdata(dev);
- q->matrix_mdev = matrix_mdev;
- put_device(dev);
+ q = vfio_ap_find_queue(apqn);
+ if (q)
+ q->matrix_mdev = matrix_mdev;
return q;
}
@@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct
notifier_block *nb,
return notify_rc;
}
-static void (int apqn)
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
{
struct device *dev;
- struct vfio_ap_queue *q;
+ struct vfio_ap_queue *q = NULL;
dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
, match_apqn);
if (dev) {
q = dev_get_drvdata(dev);
- vfio_ap_irq_disable(q);
put_device(dev);
}
+
+ return q;
}
This hunk and the previous one are a rewrite of vfio_ap_get_queue() and
have next to nothing to do with the patch's objective. If we were at an
earlier stage, I would ask to split it up.
The rewrite of vfio_ap_get_queue() definitely is related to this
patch's objective.
Definitively loosely related.
A matter of opinion I suppose and I respect yours.
Below, in the vfio_ap_mdev_reset_queue()
function, there is the label 'free_aqic_resources' which is where
the call to vfio_ap_free_aqic_resources() function is called.
That function takes a struct vfio_ap_queue as an argument,
so the object needs to be retrieved prior to calling the function.
We can't use the vfio_ap_get_queue() function for two reasons:
1. The vfio_ap_get_queue() function takes a struct ap_matrix_mdev
as a parameter and we do not have a pointer to such at the time.
2. The vfio_ap_get_queue() function is used to link the mdev to the
vfio_ap_queue object with the specified APQN.
So, we needed a way to retrieve the vfio_ap_queue object by its
APQN only, Rather than creating a function that retrieves the
vfio_ap_queue object which duplicates the retrieval code in
vfio_ap_get_queue(), I created the vfio_ap_find_queue()
function to do just that and modified the vfio_ap_get_queue()
function to call it (i.e., code reuse).
Please tell me what prevented you from doing a doing the splitting out
vfio_ap_find_queue() from vfio_ap_get_queue() in a separate patch, that
precedes this patch? It would have resulted in simpler diffs, because
the split out wouldn't be intermingled with other stuff, i.e. getting
rid of vfio_ap_irq_disable_apqn(). Don't you see that the two are
intermingled in this diff?
I included this here for the reasons I stated above.
If I was reviewing these patches and saw this in a
Re: [PATCH v13 02/15] s390/vfio-ap: No need to disable IRQ after queue reset
On 1/11/21 11:32 AM, Halil Pasic wrote:
On Tue, 22 Dec 2020 20:15:53 -0500
Tony Krowiak wrote:
The queues assigned to a matrix mediated device are currently reset when:
* The VFIO_DEVICE_RESET ioctl is invoked
* The mdev fd is closed by userspace (QEMU)
* The mdev is removed from sysfs.
Immediately after the reset of a queue, a call is made to disable
interrupts for the queue. This is entirely unnecessary because the reset of
a queue disables interrupts, so this will be removed.
Signed-off-by: Tony Krowiak
---
drivers/s390/crypto/vfio_ap_drv.c | 1 -
drivers/s390/crypto/vfio_ap_ops.c | 40 +--
drivers/s390/crypto/vfio_ap_private.h | 1 -
3 files changed, 26 insertions(+), 16 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_drv.c
b/drivers/s390/crypto/vfio_ap_drv.c
index be2520cc010b..ca18c91afec9 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
apid = AP_QID_CARD(q->apqn);
apqi = AP_QID_QUEUE(q->apqn);
vfio_ap_mdev_reset_queue(apid, apqi, 1);
- vfio_ap_irq_disable(q);
kfree(q);
mutex_unlock(_dev->lock);
}
diff --git a/drivers/s390/crypto/vfio_ap_ops.c
b/drivers/s390/crypto/vfio_ap_ops.c
index 7339043906cf..052f61391ec7 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -25,6 +25,7 @@
#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
static int match_apqn(struct device *dev, const void *data)
{
@@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
int apqn)
{
struct vfio_ap_queue *q;
- struct device *dev;
if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
return NULL;
if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
return NULL;
- dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
-, match_apqn);
- if (!dev)
- return NULL;
- q = dev_get_drvdata(dev);
- q->matrix_mdev = matrix_mdev;
- put_device(dev);
+ q = vfio_ap_find_queue(apqn);
+ if (q)
+ q->matrix_mdev = matrix_mdev;
return q;
}
@@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct
notifier_block *nb,
return notify_rc;
}
-static void vfio_ap_irq_disable_apqn(int apqn)
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
{
struct device *dev;
- struct vfio_ap_queue *q;
+ struct vfio_ap_queue *q = NULL;
dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
, match_apqn);
if (dev) {
q = dev_get_drvdata(dev);
- vfio_ap_irq_disable(q);
put_device(dev);
}
+
+ return q;
}
This hunk and the previous one are a rewrite of vfio_ap_get_queue() and
have next to nothing to do with the patch's objective. If we were at an
earlier stage, I would ask to split it up.
The rewrite of vfio_ap_get_queue() definitely is related to this
patch's objective. Below, in the vfio_ap_mdev_reset_queue()
function, there is the label 'free_aqic_resources' which is where
the call to vfio_ap_free_aqic_resources() function is called.
That function takes a struct vfio_ap_queue as an argument,
so the object needs to be retrieved prior to calling the function.
We can't use the vfio_ap_get_queue() function for two reasons:
1. The vfio_ap_get_queue() function takes a struct ap_matrix_mdev
as a parameter and we do not have a pointer to such at the time.
2. The vfio_ap_get_queue() function is used to link the mdev to the
vfio_ap_queue object with the specified APQN.
So, we needed a way to retrieve the vfio_ap_queue object by its
APQN only, Rather than creating a function that retrieves the
vfio_ap_queue object which duplicates the retrieval code in
vfio_ap_get_queue(), I created the vfio_ap_find_queue()
function to do just that and modified the vfio_ap_get_queue()
function to call it (i.e., code reuse).
int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
unsigned int retry)
{
struct ap_queue_status status;
+ struct vfio_ap_queue *q;
+ int ret;
int retry2 = 2;
int apqn = AP_MKQID(apid, apqi);
@@ -1156,18 +1155,32 @@ int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
status = ap_tapq(apqn, NULL);
}
WARN_ON_ONCE(retry2 <= 0);
- return 0;
+ ret = 0;
+ goto free_aqic_resources;
case
Re: [PATCH v13 02/15] s390/vfio-ap: No need to disable IRQ after queue reset
On Tue, 22 Dec 2020 20:15:53 -0500
Tony Krowiak wrote:
> The queues assigned to a matrix mediated device are currently reset when:
>
> * The VFIO_DEVICE_RESET ioctl is invoked
> * The mdev fd is closed by userspace (QEMU)
> * The mdev is removed from sysfs.
>
> Immediately after the reset of a queue, a call is made to disable
> interrupts for the queue. This is entirely unnecessary because the reset of
> a queue disables interrupts, so this will be removed.
>
> Signed-off-by: Tony Krowiak
> ---
> drivers/s390/crypto/vfio_ap_drv.c | 1 -
> drivers/s390/crypto/vfio_ap_ops.c | 40 +--
> drivers/s390/crypto/vfio_ap_private.h | 1 -
> 3 files changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c
> b/drivers/s390/crypto/vfio_ap_drv.c
> index be2520cc010b..ca18c91afec9 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device
> *apdev)
> apid = AP_QID_CARD(q->apqn);
> apqi = AP_QID_QUEUE(q->apqn);
> vfio_ap_mdev_reset_queue(apid, apqi, 1);
> - vfio_ap_irq_disable(q);
> kfree(q);
> mutex_unlock(_dev->lock);
> }
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index 7339043906cf..052f61391ec7 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -25,6 +25,7 @@
> #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
>
> static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
>
> static int match_apqn(struct device *dev, const void *data)
> {
> @@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
> int apqn)
> {
> struct vfio_ap_queue *q;
> - struct device *dev;
>
> if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
> return NULL;
> if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
> return NULL;
>
> - dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
> - , match_apqn);
> - if (!dev)
> - return NULL;
> - q = dev_get_drvdata(dev);
> - q->matrix_mdev = matrix_mdev;
> - put_device(dev);
> + q = vfio_ap_find_queue(apqn);
> + if (q)
> + q->matrix_mdev = matrix_mdev;
>
> return q;
> }
> @@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct
> notifier_block *nb,
> return notify_rc;
> }
>
> -static void vfio_ap_irq_disable_apqn(int apqn)
> +static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
> {
> struct device *dev;
> - struct vfio_ap_queue *q;
> + struct vfio_ap_queue *q = NULL;
>
> dev = driver_find_device(_dev->vfio_ap_drv->driver, NULL,
>, match_apqn);
> if (dev) {
> q = dev_get_drvdata(dev);
> - vfio_ap_irq_disable(q);
> put_device(dev);
> }
> +
> + return q;
> }
This hunk and the previous one are a rewrite of vfio_ap_get_queue() and
have next to nothing to do with the patch's objective. If we were at an
earlier stage, I would ask to split it up.
>
> int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
>unsigned int retry)
> {
> struct ap_queue_status status;
> + struct vfio_ap_queue *q;
> + int ret;
> int retry2 = 2;
> int apqn = AP_MKQID(apid, apqi);
>
> @@ -1156,18 +1155,32 @@ int vfio_ap_mdev_reset_queue(unsigned int apid,
> unsigned int apqi,
> status = ap_tapq(apqn, NULL);
> }
> WARN_ON_ONCE(retry2 <= 0);
> - return 0;
> + ret = 0;
> + goto free_aqic_resources;
> case AP_RESPONSE_RESET_IN_PROGRESS:
> case AP_RESPONSE_BUSY:
> msleep(20);
> break;
> default:
> /* things are really broken, give up */
> - return -EIO;
> + ret = -EIO;
> + goto free_aqic_resources;
Do we really want the unpin here? I mean the reset did not work and
we are giving up. So the irqs are potentially still enabled.
Without this patch we try to disable the interrupts using AQIC, and
do the cleanup after that.
I'm aware, the comment says we should not take the default branch,
but if that's really the case we should IMHO log an error and leak the
page.
It's up to you if you want to change this. I don't want to delay the
series any further than absolutely necessary.
Acked-by: Halil Pasic
> }
> } while (retry--);
>
> return -EBUSY;
> +
> +free_aqic_resources:
> + /*
> + * In order to free the
