Re: [qemu-s390x] [RFC 04/19] s390/zcrypt: create an AP matrix device on the AP matrix bus

[Tony Krowiak]

On 10/18/2017 12:20 PM, Cornelia Huck wrote:
> On Fri, 13 Oct 2017 13:38:49 -0400
> Tony Krowiak <address@hidden> wrote:
>
> [Please take with a grain of salt as I did not yet have time to take
> more than a very superficial glance at the whole structure.]
>
> > Creates a single AP matrix device on the AP matrix bus.
> > The matrix device will be created as part of the AP matrix bus
> > initialization. The matrix device will hold the AP queues that
> > have been reserved for use by KVM guests. Mediated matrix devices
> > can then be created for each guest. The mediated matrix devices can
> > then be configured with a matrix of AP adapters, usage and
> > control domains that will be made accessible to the guest.
> >
> > The sysfs location of the matrix device is:
> >
> > /sys/bus/ap_matrix
> > ... [devices]
> > ...... [matrix]
> Also /sys/devices/ap_matrix/matrix, isn't it?
Yes
> > Signed-off-by: Tony Krowiak <address@hidden>
> > ---
> >   drivers/s390/crypto/ap_matrix_bus.c |   54 +++++++++++++++++++++++++++++++++++
> >   drivers/s390/crypto/ap_matrix_bus.h |    6 ++++
> >   2 files changed, 60 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/s390/crypto/ap_matrix_bus.c 
> > b/drivers/s390/crypto/ap_matrix_bus.c
> > index fbae175..4eb1e3c 100644
> > --- a/drivers/s390/crypto/ap_matrix_bus.c
> > +++ b/drivers/s390/crypto/ap_matrix_bus.c
> > @@ -12,6 +12,7 @@
> >   
> >   #include <linux/module.h>
> >   #include <linux/device.h>
> > +#include <linux/slab.h>
> >   #include <asm/ap.h>
> >   
> >   #include "ap_matrix_bus.h"
> > @@ -21,13 +22,59 @@
> >   MODULE_LICENSE("GPL v2");
> >   
> >   #define AP_MATRIX_BUS_NAME "ap_matrix"
> > +#define AP_MATRIX_DEV_TYPE_NAME "ap_matrix"
> > +#define AP_MATRIX_DEV_NAME "matrix"
> >   
> >   static struct device *ap_matrix_root_device;
> > +static struct ap_matrix *matrix;
> >   
> >   static struct bus_type ap_matrix_bus_type = {
> >         .name = AP_MATRIX_BUS_NAME,
> >   };
> >   
> > +static struct device_type ap_matrix_type = {
> > +       .name = AP_MATRIX_DEV_TYPE_NAME,
> > +};
> > +
> > +static void ap_matrix_dev_release(struct device *dev)
> > +{
> > +       struct ap_matrix *ap_matrix;
> > +
> > +       ap_matrix = container_of(dev, struct ap_matrix, device);
> > +
> > +       if (matrix == ap_matrix)
> > +               kfree(matrix);
> > +
> > +       matrix = NULL;
> This looks very, very odd. Refcounting should take care that the
> release function is only invoked if the device is really gone.
>
> Also, I don't think you need to keep a pointer to the device as it is a
> singleton: It's simply the only device on the list and you should be
> able to easily pick it that way. If your code does not register further
> devices on the bus, there won't be ambiguities.
Okay, I'll make that change
> > +}
> > +
> > +static int ap_matrix_dev_create(void)
> > +{
> > +       int ret;
> > +
> > +       matrix = kzalloc(sizeof(*matrix), GFP_KERNEL);
> > +       if (!matrix)
> > +               return -ENOMEM;
> > +
> > +       matrix->device.type = &ap_matrix_type;
> > +       dev_set_name(&matrix->device, "%s", AP_MATRIX_DEV_NAME);
> > +       matrix->device.bus = &ap_matrix_bus_type;
> > +       matrix->device.parent = ap_matrix_root_device;
> > +       matrix->device.release = ap_matrix_dev_release;
> > +
> > +       ret = device_register(&matrix->device);
> > +       if (ret) {
> > +               put_device(&matrix->device);
> > +               kfree(matrix);
> > +               matrix = NULL;
> That's wrong. The release function needs to clean up the embedding
> structure, so that kfree is at best useless and at worst wrong, if
> something else grabbed a reference.
Good point.
> > +               return ret;
> > +       }
> > +
> > +       get_device(&matrix->device);
> Why should you need an extra reference here? I'd expect the code
> hanging devices off this one to properly grab a reference, so you
> should be all good?
I'll remove this.
> > +
> > +       return 0;
> > +}
> > +
> >   int __init ap_matrix_init(void)
> >   {
> >         int ret;
> > @@ -41,8 +88,15 @@ int __init ap_matrix_init(void)
> >         if (ret)
> >                 goto bus_reg_err;
> >   
> > +	ret = ap_matrix_dev_create();
> > +       if (ret)
> > +               goto matrix_create_err;
> > +
> >         return 0;
> >   
> > +matrix_create_err:
> > +       bus_unregister(&ap_matrix_bus_type);
> > +
> >   bus_reg_err:
> >         root_device_unregister(ap_matrix_root_device);