On 07/31/2018 11:14 AM, Halil Pasic wrote:
>
>
> On 07/31/2018 10:38 AM, Cornelia Huck wrote:
>> On Thu, 26 Jul 2018 21:54:21 +0200
>> Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
>>
>> Regardless whether the activation approach is a good idea...
>>
>>> +static int vfio_ap_verify_queues_reserved(struct ap_matrix_mdev *matrix_mdev)
>>> +{
>>> + unsigned long apid, apqi;
>>> + int ret = 0;
>>> +
>>> + for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>>> + matrix_mdev->matrix.apm_max + 1) {
>>> + for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>>> + matrix_mdev->matrix.aqm_max + 1) {
>>> + if (!ap_owned_by_def_drv((int)apid, (int)apqi))
>>> + continue;
>>> +
>>> + /*
>>> + * We want to log every APQN that is not reserved by
>>> + * the driver, so record the return code, log a message
>>> + * and allow the loop to continue
>>> + */
>>> + ret = -EPERM;
>>> + pr_warn("%s: activate for %s failed: queue %02lx.%04lx owned by default driver\n",
>>> + VFIO_AP_MODULE_NAME, matrix_mdev->name, apid,
>>> + apqi);
>>
>> ...I do not think that the syslog is a good place to log those errors.
>> AFAICS these are setup/administration errors, and the admin may want to
>> inspect the data to find out what went wrong. Maybe better to log
>> attempts to assign APQNs reserved to the default drivers into a
>> dedicated dbf or somesuch, and log it in a format that can also be
>> digested by scripts?
>>
>
> I agree, the error reporting is suboptimal. My preferred solution is an
> API with proper error reporting though. Even with dbf the problem, that
> one has to read some log and figure out what error corresponds to what
> request persist. Optimally the user should get feedback via the same
> interface an interactive action was triggered.
>
> In fact, I had a proof of concept implementation but it would need some
> rebasing. Another IMHO desirable property of this interface is that it
> changes (or rejects change to) the matrix (masks) in one atomic operation.
> So there is no 'dirty' state and no transaction involved in specifying
> or changing the matrix.
lets just drop the printks for the time being. Its the normal way of kernel
things to return with -EINVAL and NOT telling why.
