Hi Cornelia,
A bit more on this RFC.
I think this is a required change in the CIO layer, especially because there
are lot of validations before we call the probe, which makes sure that we
are not generating the uevent on an invalid (without a ccw-device connected)
subchannel and we dont expect a uevent-storm with the current code-base.
So, in anyway, Removing the suppression in the uevent is a cleaner way
for the
css driver.
But, the more i look at this patch and discuss on this, i think this is
not complete.
i.e as you know, the main reason for this RFC was the the below thread.
https://marc.info/?l=linux-s390&m=158591045732735&w=2
We are still not solving the problem that was mentioned in that RFD.
There are couple of things which we needs to consider here. With this
patch, the uevents
are generated before doing the initialization or before finding the
ccw-device
connected. Which means, the udev-rules have to manage with a
non-initialized setup
compared to the previous version (Version without this patch). As you
mentioned, the
current user-space programs which works with this uevent, especially in
case of vfio-ccw
will have a problem.
Second point is, there is no guarantee on the current code that the
uevent generated
will be used by udev-rules of vfio-ccw instead of io-subchannel driver.
This means, we
need to do some more modifications on the udev-rules, which can then
decide which
driver should bind with the subchannel. I think that is the only way to
avoid the
problems we are facing with the driver_override.
I would like to get your expert-opinion on the modifications that can be
done on the
vfio-ccw udev-rules to make it sync with the current patch.
Regards
Vineeth
On 11/25/20 10:40 AM, Vineeth Vijayan wrote:
Thank you for looking in to this.
On 11/24/20 2:02 PM, Cornelia Huck wrote:
On Tue, 24 Nov 2020 10:34:07 +0100
Vineeth Vijayan <vneethv@xxxxxxxxxxxxx> wrote:
'commit fa1a8c23eb7d ("s390: cio: Delay uevents for subchannels")'
introduced the uevent suppression of subchannels. Even though there
are reasons for wanting to delay the uevent, it also introduces
problems. i.e On some platforms (qemu), where the udev-rule for the
subchannel needs to do driver_override to bind the vfio-ccw driver
instead of io_subchannel driver, but the suppressed uevent is
generated only when the device is found on the subchannel. By the
time it generates the uevent, it makes it difficult for the vfio-ccw
udev-rules to work.
This patch removes the uevent-suppress logic from the css driver.
The ADD uevent will be generated when there is a valid subchannel
and not after binding the valid device. The uevent generates while
device_add() during css_sch_device_register() function.
Signed-off-by: Vineeth Vijayan <vneethv@xxxxxxxxxxxxx>
---
drivers/s390/cio/chsc_sch.c | 5 -----
drivers/s390/cio/css.c | 19 -------------------
drivers/s390/cio/device.c | 18 ------------------
drivers/s390/cio/eadm_sch.c | 5 -----
drivers/s390/cio/vfio_ccw_drv.c | 5 -----
5 files changed, 52 deletions(-)
While I really like that diffstat, I hope that this is actually safe
for userspace programs processing uevents. Previously, we generated
the ADD uevent only when all parts were setup and ready to use
(including a child ccw_device, for example). Now, the ADD uevent is
created earlier, before drivers have done their setup. Do existing
udev rules still work as expected? (...)
I am still working on this. I have done minimal tests as of now.
Mostly with the available LPARs/zVMs. And it looks ok. But, i know
there are many fragile setups out there and the change in the "timing"
(The uevents generated earlier) could be a potential issue on that. I
will keep you updated on my findings.
About this RFC, i just wanted make sure that we are on same page with
regards to the RFD that you shared.
@@ -1055,16 +1047,6 @@ static int io_subchannel_probe(struct
subchannel *sch)
"attributes for subchannel "
"0.%x.%04x (rc=%d)\n",
sch->schid.ssid, sch->schid.sch_no, rc);
- /*
- * The console subchannel already has an associated
ccw_device.
- * Throw the delayed uevent for the subchannel, register
- * the ccw_device and exit.
I would keep the comment that we already have a ccw_device here. I.e.
/*
* The console subchannel already has an associated ccw_device.
* Register it and exit.
*/
- */
- if (dev_get_uevent_suppress(&sch->dev)) {
- /* should always be the case for the console */
- dev_set_uevent_suppress(&sch->dev, 0);
- kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
- }
cdev = sch_get_cdev(sch);
rc = ccw_device_add(cdev);
if (rc) {
(...)
