Re: [PATCH v19 02/20] s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c

From: Tony Krowiak
Date: Tue May 24 2022 - 13:41:39 EST



On 5/24/22 10:49 AM, Jason J. Herne wrote:
On 4/4/22 18:10, Tony Krowiak wrote:
Let's move the probe and remove callbacks into the vfio_ap_ops.c
file to keep all code related to managing queues in a single file. This
way, all functions related to queue management can be removed from the
vfio_ap_private.h header file defining the public interfaces for the
vfio_ap device driver.

Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
---
  drivers/s390/crypto/vfio_ap_drv.c     | 59 +--------------------------
  drivers/s390/crypto/vfio_ap_ops.c     | 31 +++++++++++++-
  drivers/s390/crypto/vfio_ap_private.h |  5 ++-
  3 files changed, 34 insertions(+), 61 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 29ebd54f8919..9a300dd3b6f7 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -104,64 +104,9 @@ static const struct attribute_group vfio_queue_attr_group = {
      .attrs = vfio_queue_attrs,
  };
  -/**
- * vfio_ap_queue_dev_probe: Allocate a vfio_ap_queue structure and associate it
- *                with the device as driver_data.
- *
- * @apdev: the AP device being probed
- *
- * Return: returns 0 if the probe succeeded; otherwise, returns an error if
- *       storage could not be allocated for a vfio_ap_queue object or the
- *       sysfs 'status' attribute could not be created for the queue device.
- */
-static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
-{
-    int ret;
-    struct vfio_ap_queue *q;
-
-    q = kzalloc(sizeof(*q), GFP_KERNEL);
-    if (!q)
-        return -ENOMEM;
-
-    mutex_lock(&matrix_dev->lock);
-    dev_set_drvdata(&apdev->device, q);
-    q->apqn = to_ap_queue(&apdev->device)->qid;
-    q->saved_isc = VFIO_AP_ISC_INVALID;
-
-    ret = sysfs_create_group(&apdev->device.kobj, &vfio_queue_attr_group);
-    if (ret) {
-        dev_set_drvdata(&apdev->device, NULL);
-        kfree(q);
-    }
-
-    mutex_unlock(&matrix_dev->lock);
-
-    return ret;
-}
-
-/**
- * vfio_ap_queue_dev_remove: Free the associated vfio_ap_queue structure.
- *
- * @apdev: the AP device being removed
- *
- * Takes the matrix lock to avoid actions on this device while doing the remove.
- */
-static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
-{
-    struct vfio_ap_queue *q;
-
-    mutex_lock(&matrix_dev->lock);
-    sysfs_remove_group(&apdev->device.kobj, &vfio_queue_attr_group);
-    q = dev_get_drvdata(&apdev->device);
-    vfio_ap_mdev_reset_queue(q, 1);
-    dev_set_drvdata(&apdev->device, NULL);
-    kfree(q);
-    mutex_unlock(&matrix_dev->lock);
-}
-
  static struct ap_driver vfio_ap_drv = {
-    .probe = vfio_ap_queue_dev_probe,
-    .remove = vfio_ap_queue_dev_remove,
+    .probe = vfio_ap_mdev_probe_queue,
+    .remove = vfio_ap_mdev_remove_queue,
      .ids = ap_queue_ids,
  };
  diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 2227919fde13..16220157dbe3 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1314,8 +1314,7 @@ static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
      return q;
  }
  -int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q,
-                 unsigned int retry)
+static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, unsigned int retry)
  {
      struct ap_queue_status status;
      int ret;
@@ -1524,3 +1523,31 @@ void vfio_ap_mdev_unregister(void)
      mdev_unregister_device(&matrix_dev->device);
      mdev_unregister_driver(&vfio_ap_matrix_driver);
  }
+
+int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
+{
+    struct vfio_ap_queue *q;
+
+    q = kzalloc(sizeof(*q), GFP_KERNEL);
+    if (!q)
+        return -ENOMEM;
+    mutex_lock(&matrix_dev->lock);
+    q->apqn = to_ap_queue(&apdev->device)->qid;
+    q->saved_isc = VFIO_AP_ISC_INVALID;
+    dev_set_drvdata(&apdev->device, q);
+    mutex_unlock(&matrix_dev->lock);
+
+    return 0;
+}
+
+void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
+{
+    struct vfio_ap_queue *q;
+
+    mutex_lock(&matrix_dev->lock);
+    q = dev_get_drvdata(&apdev->device);
+    vfio_ap_mdev_reset_queue(q, 1);
+    dev_set_drvdata(&apdev->device, NULL);
+    kfree(q);
+    mutex_unlock(&matrix_dev->lock);
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 648fcaf8104a..3cade25a1620 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -119,7 +119,8 @@ struct vfio_ap_queue {
    int vfio_ap_mdev_register(void);
  void vfio_ap_mdev_unregister(void);
-int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q,
-                 unsigned int retry);
+
+int vfio_ap_mdev_probe_queue(struct ap_device *queue);
+void vfio_ap_mdev_remove_queue(struct ap_device *queue);
    #endif /* _VFIO_AP_PRIVATE_H_ */


With this commit, you did more than just move the probe/remove functions. You also changed their behavior. The call to sysfs_create_group has been removed. So the following in vfop_ap_drv.c becomes dead code:

    vfio_ap_mdev_for_queue
    status_show
    static DEVICE_ATTR_RO(status);
    vfio_queue_attrs
    vfio_queue_attr_group

Is this what you intended? If so, I assume we can live without the status attribute?
If this is the case then you'll want to remove all the dead code.

This was not intended. The status attribute was added via commit f139862b92cf which
was merged into the KVM last October. I believe it may have been removed when this
was rebased on the release containing that patch. I'll reinstate that attribute as it is
necessary. Thanks and good catch.