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

[Heiko Carstens <hca@linux.ibm.com>]

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.

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

Same here.

>  	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:

  	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.

> +
> +/**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.

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

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

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