Re: [PATCH v7 10/22] s390: vfio-ap: sysfs interfaces to configure adapters

Re: [PATCH v7 10/22] s390: vfio-ap: sysfs interfaces to configure adapters

[Christian Borntraeger]

On 07/30/2018 03:18 PM, Cornelia Huck wrote:
> On Thu, 26 Jul 2018 21:54:17 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> +/**
>> + * assign_adapter_store
>> + *
>> + * @dev: the matrix device
>> + * @attr: a mediated matrix device attribute
>> + * @buf: a buffer containing the adapter ID (APID) to be assigned
>> + * @count: the number of bytes in @buf
>> + *
>> + * Parses the APID from @buf and assigns it to the mediated matrix device.
>> + *
>> + * Returns the number of bytes processed if the APID is valid; otherwise returns
>> + * an error.
>> + */
>> +static ssize_t assign_adapter_store(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    const char *buf, size_t count)
>> +{
>> +	int ret;
>> +	unsigned long apid;
>> +	struct mdev_device *mdev = mdev_from_dev(dev);
>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +	unsigned long max_apid = matrix_mdev->matrix.apm_max;
>> +
>> +	ret = kstrtoul(buf, 0, &apid);
>> +	if (ret || (apid > max_apid)) {
>> +		pr_warn("%s: %s: adapter id '%s' not a value from 0 to %02lu(%#04lx)\n",
>> +		       VFIO_AP_MODULE_NAME, __func__, buf, max_apid, max_apid);
> 
> I'm not sure how helpful a message to the syslog is if you are writing
> to sysfs attributes. If you decide to drop the message, you could do
> 
> if (apid > max_apid)
> 	ret = -EINVAL;
> 
> if (ret)
> 	return ret;
> 
> which looks more straightforward to me.

Yes, makes sense. pr_* usage should be reduced in this patch series

Re: [PATCH v7 10/22] s390: vfio-ap: sysfs interfaces to configure adapters

[Tony Krowiak]

On 07/27/2018 11:04 AM, Thomas Huth wrote:
> On 07/26/2018 09:54 PM, Christian Borntraeger wrote:
> [...]
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index e4e8d738a043..07911fb9cdce 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -15,7 +15,6 @@
>>   #define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough"
>>   #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
>>   
>> -
> Unnecessary white space change.

It is indeed, to be fixed.

>
>>   static void vfio_ap_matrix_init(struct ap_config_info *info,
>>   				struct ap_matrix *matrix)
>>   {
>> @@ -103,9 +102,127 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
>>   	NULL,
>>   };
>>   
>> +/**
>> + * assign_adapter_store
>> + *
>> + * @dev: the matrix device
>> + * @attr: a mediated matrix device attribute
>> + * @buf: a buffer containing the adapter ID (APID) to be assigned
>> + * @count: the number of bytes in @buf
>> + *
>> + * Parses the APID from @buf and assigns it to the mediated matrix device.
>> + *
>> + * Returns the number of bytes processed if the APID is valid; otherwise returns
>> + * an error.
>> + */
>> +static ssize_t assign_adapter_store(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    const char *buf, size_t count)
>> +{
>> +	int ret;
>> +	unsigned long apid;
>> +	struct mdev_device *mdev = mdev_from_dev(dev);
>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +	unsigned long max_apid = matrix_mdev->matrix.apm_max;
>> +
>> +	ret = kstrtoul(buf, 0, &apid);
>> +	if (ret || (apid > max_apid)) {
> You can remove the parentheses around "apid > max_apid".

My personal opinion is that the parenthesis make it easier to
understand .... there is no need to scour the mind to recall
operator precedence rules.

>
>> +		pr_warn("%s: %s: adapter id '%s' not a value from 0 to %02lu(%#04lx)\n",
>> +		       VFIO_AP_MODULE_NAME, __func__, buf, max_apid, max_apid);
>> +
>> +		if (!ret)
>> +			ret = -EINVAL;
>> +
>> +		return ret;
>> +	}
>> +
>> +	/* Set the bit in the AP mask (APM) corresponding to the AP adapter
>> +	 * number (APID). The bits in the mask, from most significant to least
>> +	 * significant bit, correspond to APIDs 0-255.
>> +	 */
>> +	mutex_lock(&matrix_dev.lock);
>> +	set_bit_inv(apid, matrix_mdev->matrix.apm);
>> +	ret = count;
>> +
>> +	mutex_unlock(&matrix_dev.lock);
>> +
>> +	return ret;
>> +}
>> +static DEVICE_ATTR_WO(assign_adapter);
>> +
>> +/**
>> + * unassign_adapter_store
>> + *
>> + * @dev: the matrix device
>> + * @attr: a mediated matrix device attribute
>> + * @buf: a buffer containing the adapter ID (APID) to be assigned
>> + * @count: the number of bytes in @buf
>> + *
>> + * Parses the APID from @buf and unassigns it from the mediated matrix device.
>> + * The APID must be a valid value
>> + *
>> + * Returns the number of bytes processed if the APID is valid; otherwise returns
>> + * an error.
>> + */
>> +static ssize_t unassign_adapter_store(struct device *dev,
>> +				      struct device_attribute *attr,
>> +				      const char *buf, size_t count)
>> +{
>> +	int ret;
>> +	unsigned long apid;
>> +	struct mdev_device *mdev = mdev_from_dev(dev);
>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +
>> +	unsigned long max_apid = matrix_mdev->matrix.apm_max;
>> +
>> +	ret = kstrtoul(buf, 0, &apid);
>> +	if (ret || (apid > max_apid)) {
> dito

ditto

>
>> +		pr_warn("%s: %s: adapter id '%s' must be a value from 0 to %02lu(%#04lx)\n",
>> +		       VFIO_AP_MODULE_NAME, __func__, buf, max_apid, max_apid);
>> +
>> +		if (!ret)
>> +			ret = -EINVAL;
>> +
>> +		return ret;
>> +	}
>> +
>> +	mutex_lock(&matrix_dev.lock);
>> +	if (!test_bit_inv(apid, matrix_mdev->matrix.apm)) {
>> +		pr_warn("%s: %s: adapter id %02lu(%#04lx) not assigned\n",
>> +		       VFIO_AP_MODULE_NAME, __func__, apid, apid);
>> +		ret = -ENODEV;
>> +		goto done;
>> +	}
>> +
>> +	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
>> +	ret = count;
>> +
>> +done:
>> +	mutex_unlock(&matrix_dev.lock);
>> +
>> +	return ret;
>> +}
>> +DEVICE_ATTR_WO(unassign_adapter);
>> +
>> +static struct attribute *vfio_ap_mdev_attrs[] = {
>> +	&dev_attr_assign_adapter.attr,
>> +	&dev_attr_unassign_adapter.attr,
>> +	NULL
>> +};
>> +
>> +static struct attribute_group vfio_ap_mdev_attr_group = {
>> +	.attrs = vfio_ap_mdev_attrs
>> +};
>> +
>> +static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
>> +	&vfio_ap_mdev_attr_group,
>> +	NULL
>> +};
>> +
>>   static const struct mdev_parent_ops vfio_ap_matrix_ops = {
>>   	.owner			= THIS_MODULE,
>>   	.supported_type_groups	= vfio_ap_mdev_type_groups,
>> +	.mdev_attr_groups	= vfio_ap_mdev_attr_groups,
>>   	.create			= vfio_ap_mdev_create,
>>   	.remove			= vfio_ap_mdev_remove,
>>   };
>>
>   Thomas
>