Re: [PATCH v7 09/22] s390: vfio-ap: register matrix device with VFIO mdev framework

[Tony Krowiak <akrowiak@xxxxxxxxxxxxx>]

On 07/27/2018 10:17 AM, Halil Pasic wrote:
> On 07/26/2018 09:54 PM, Christian Borntraeger wrote:
> > @@ -65,6 +66,21 @@ static int vfio_ap_matrix_dev_create(void)
> >   {
> >       int ret;
> >   +    mutex_init(&matrix_dev.lock);
> > +    INIT_LIST_HEAD(&matrix_dev.mdev_list);
> > +
> > +    /* Test if PQAP(QCI) instruction is available */
> > +    if (test_facility(12)) {
> > +        ret = ap_qci(&matrix_dev.info);
> > +        if (ret && (ret != -EOPNOTSUPP)) {
> After Connie's curiosity was piqued I gave this another look. If
> I read the ap_qci() documentation and code correctly, it can return
> either 0 or -EOPNOTSUPP, but nothing else. So basically this
> is a dead branch.
>
> Either removing the && (ret != -EOPNOTSUPP) and adding some fancy
> error reporting, or removing the check alltoghether would make
> more sense than what we have right now. Should we do something?
>
> But don't misunderstand me, what we have right now is safe.

I think we should change it to:

    if (ret) {
        vfio_ap_mdev_unregister();

        return ret;
    }

> > +            vfio_ap_mdev_unregister();
> > +
> > +            return ret;
> > +        }
> > +    }