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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

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