Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

From: Pierre Morel
Date: Thu Feb 14 2019 - 12:36:57 EST

On 14/02/2019 17:57, Cornelia Huck wrote:
On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
<pmorel@xxxxxxxxxxxxx> wrote:

On 14/02/2019 15:54, Cornelia Huck wrote:
On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
<pmorel@xxxxxxxxxxxxx> wrote:

+++ b/drivers/s390/crypto/vfio_ap_drv.c @@ -24,8 +24,9 @@

static struct ap_driver vfio_ap_drv;

-static struct device_type vfio_ap_dev_type = { - .name =
VFIO_AP_DEV_TYPE_NAME, +struct matrix_driver { + struct
device_driver drv; + int device_count;

This counter basically ensures that at most one device may bind
with this driver... you'd still have that device on the bus,

yes, this is what is wanted: this driver can only support one
device. May be another matrix driver can support one or more other

I should update comment message my be.


struct ap_matrix_dev *matrix_dev;

- matrix_dev->device.type = &vfio_ap_dev_type; dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); matrix_dev->device.parent = root_device; +
matrix_dev->device.bus = &matrix_bus; matrix_dev->device.release = vfio_ap_matrix_dev_release; -
matrix_dev->device.driver = &vfio_ap_drv.driver; +
matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver
instead when you need it in the function below?

Not anymore. We have two different drivers and devices matrix_drv
<-> matrix_dev and vfio_ap_drv <-> ap_devices

The driver behind the matrix_dev->dev->driver is matrix_drv what is
needed here is vfio_ap_drv.

Wait, we had tacked a driver for ap devices unto a matrix device,
which is not on the ap bus?

...yes -(

Maybe that's what trips libudev? >
(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

ret = device_register(&matrix_dev->device); if (ret) goto

+ ret = driver_register(&matrix_driver.drv); + if (ret) + goto
matrix_drv_err; +

As you already have several structures that can be registered
exactly once (the root device, the bus, the driver, ...), you can
already be sure that there's only one device on the bus, can't

hum, no I don't think so, no device can register before this module
is loaded, but what does prevent a device to register later from
another module?

Not unless you export the interface, I guess.

:) definitively right
thanks, this will simplify the code in the next version.
I will take the patch away from this series to get the way to stable as Christian requested.


Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany