s390: vfio-ap: sysfs interfaces to configure adapters
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 34 lines · Cornelia Huck
Quoted 17 lines · On Thu, 26 Jul 2018 21:54:17 +0200 Christian Borntraeger…
(quoted diff) +17
-0+/** + * 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) +{Review
Quoted 10 lines
(quoted diff) +10
-0+ 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
Quoted 7 lines
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
Quoted 10 lines · Thomas Huth
On 07/26/2018 09:54 PM, Christian Borntraeger wrote:
[...]Quoted 8 lines
drivers/s390/crypto/vfio_ap_ops.c +0
-0@@ -15,7 +15,6 @@#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough"#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
Quoted 2 lines
drivers/s390/crypto/vfio_ap_ops.c line 18
Quoted 1 line
drivers/s390/crypto/vfio_ap_ops.c +0
-1-Unnecessary white space change.
It is indeed, to be fixed.
Quoted 26 lines
Quoted 26 lines
drivers/s390/crypto/vfio_ap_ops.c +18
-0static 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;
Quoted 8 lines
drivers/s390/crypto/vfio_ap_ops.c static struct attribute_group *vfio_ap_mdev_type_groups[] = { · line 129
Quoted 7 lines
drivers/s390/crypto/vfio_ap_ops.c +7
-0+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.
Quoted 44 lines
Quoted 44 lines
drivers/s390/crypto/vfio_ap_ops.c +43
-0+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;
Quoted 8 lines
drivers/s390/crypto/vfio_ap_ops.c static struct attribute_group *vfio_ap_mdev_type_groups[] = { · line 179
Quoted 7 lines
drivers/s390/crypto/vfio_ap_ops.c +7
-0+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
Quoted 53 lines
Quoted 41 lines
drivers/s390/crypto/vfio_ap_ops.c +39
-0+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,drivers/s390/crypto/vfio_ap_ops.c static struct attribute_group *vfio_ap_mdev_type_groups[] = { · line 228
Quoted 10 lines
drivers/s390/crypto/vfio_ap_ops.c +4
-0+ 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