Re: [PATCH] s390/vio-ap: Fix no AP queue sharing allowed message written to kernel log

Anthony Krowiak
2025-02-21T15:56:01Z

On 2/21/25 4:57 AM, Heiko Carstens wrote:
> On Wed, Feb 19, 2025 at 07:07:38PM -0500, Anthony Krowiak wrote:
>> -#define MDEV_SHARING_ERR "Userspace may not re-assign queue %02lx.%04lx " \
>> - "already assigned to %s"
>> +#define MDEV_SHARING_ERR "Userspace may not assign queue %02lx.%04lx " \
>> + "to mdev: already assigned to %s"
> Please do not split error messages across several lines, so it is easy
> to grep such for messages. If this would have been used for printk
> directly checkpatch would have emitted a message.

fixed

>
>> +#define MDEV_IN_USE_ERR "Can not reserve queue %02lx.%04lx for host driver: " \
>> + "in use by mdev"
> Same here.

fixed

>
>> for_each_set_bit_inv(apid, apm, AP_DEVICES)
>> for_each_set_bit_inv(apqi, aqm, AP_DOMAINS)
>> - dev_warn(dev, MDEV_SHARING_ERR, apid, apqi, mdev_name);
>> + dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR,
>> + apid, apqi, dev_name(mdev_dev(assigned_to->mdev)));
> Braces are missing. Even it the above is not a bug: bodies of for
> statements must be enclosed with braces if they have more than one
> line:

fixed

>
> for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
> dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR,
> apid, apqi, dev_name(mdev_dev(assigned_to->mdev))
> }
> }
>
>> +static void vfio_ap_mdev_log_in_use_err(struct ap_matrix_mdev *assignee,
>> + unsigned long *apm, unsigned long *aqm)
>> +{
>> + unsigned long apid, apqi;
>> +
>> + for_each_set_bit_inv(apid, apm, AP_DEVICES)
>> + for_each_set_bit_inv(apqi, aqm, AP_DOMAINS)
>> + dev_warn(mdev_dev(assignee->mdev), MDEV_IN_USE_ERR,
>> + apid, apqi);
>> +}
> Same here.

fixed

>
>> +
>> +/**assigned
>> * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by matrix mdevs
> Stray "assigned" - as a result this is not kernel doc anymore.

fixed

>
>> + * @assignee the matrix mdev to which @mdev_apm and @mdev_aqm are being
>> + * assigned; or, NULL if this function was called by the AP bus driver
>> + * in_use callback to verify none of the APQNs being reserved for the
>> + * host device driver are in use by a vfio_ap mediated device
>> * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
>> * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
> Missing ":" behind @assignee. Please keep this consistent.

fixed

>
>> @@ -912,17 +930,21 @@ static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
>>
>> /*
>> * We work on full longs, as we can only exclude the leftover
>> - * bits in non-inverse order. The leftover is all zeros.
>> + * bits in non-inverse order. The leftover is all zeros.assigned
>> */
> Another random "assigned" word.

My IDE sometimes randomly pastes things in the clipboard.
fixed

>
>> + if (assignee)
>> + vfio_ap_mdev_log_sharing_err(assignee, assigned_to,
>> + apm, aqm);
>> + else
>> + vfio_ap_mdev_log_in_use_err(assigned_to, apm, aqm);
> if body with multiple lines -> braces. Or better make that
> vfio_ap_mdev_log_sharing_err() call a long line. If you want to keep
> the line-break add braces to both the if and else branch.

fixed