Re: [RFC 1/1] s390/cio: Remove uevent-suppress from css driver

[Vineeth Vijayan <vneethv@linux.ibm.com>]

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@linux.ibm.com> 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@linux.ibm.com>
>>> ---
>>>   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) {
>> (...)
>>