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

[Anthony Krowiak]

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