Messages in this thread | | | Date | Tue, 24 May 2022 10:49:56 -0400 | Subject | Re: [PATCH v19 02/20] s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c | From | "Jason J. Herne" <> |
| |
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@linux.ibm.com> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > --- > 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.
-- -- Jason J. Herne (jjherne@linux.ibm.com)
| |