Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <24aaa840-7643-75c0-1517-5a357183bf07@linux.ibm.com>
Date:   Tue, 24 May 2022 13:41:13 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v19 02/20] s390/vfio-ap: move probe and remove callbacks
 to vfio_ap_ops.c
Content-Language: en-US
To:     jjherne@linux.ibm.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc:     freude@linux.ibm.com, borntraeger@de.ibm.com, cohuck@redhat.com,
        mjrosato@linux.ibm.com, pasic@linux.ibm.com,
        alex.williamson@redhat.com, kwankhede@nvidia.com,
        fiuczy@linux.ibm.com
References: <20220404221039.1272245-1-akrowiak@linux.ibm.com>
 <20220404221039.1272245-3-akrowiak@linux.ibm.com>
 <c705be10-bbe2-7d44-3057-005299ab6785@linux.ibm.com>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
In-Reply-To: <c705be10-bbe2-7d44-3057-005299ab6785@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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@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.

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.

>
>