[RFC 0/1] Remove uevent suppression logic from css driver

[RFC 0/1] Remove uevent suppression logic from css driver

[Vineeth Vijayan]

This RFC is a follow-up for the below mentioned thread.
https://marc.info/?l=linux-s390&m=158591045732735&w=2

As mentioned in the RFD, we tried to remove the uevent suppression
logic from the css driver. I think this would help the vfio-cww 
udev-rules to bind to the subchannel before the io-subchanneli driver.

Here, the uevent is generated after,stsch() and css_sch_is_valid() 
which makes sure that the subchannel is valid.


Vineeth Vijayan (1):
  s390/cio: Remove uevent-suppress from css driver

 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(-)

-- 
2.17.1

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

[Vineeth Vijayan]

'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(-)

diff --git a/drivers/s390/cio/chsc_sch.c b/drivers/s390/cio/chsc_sch.c
index 8f080d3fd380..b4057d781fb1 100644
--- a/drivers/s390/cio/chsc_sch.c
+++ b/drivers/s390/cio/chsc_sch.c
@@ -91,11 +91,6 @@ static int chsc_subchannel_probe(struct subchannel *sch)
 			 sch->schid.ssid, sch->schid.sch_no, ret);
 		dev_set_drvdata(&sch->dev, NULL);
 		kfree(private);
-	} else {
-		if (dev_get_uevent_suppress(&sch->dev)) {
-			dev_set_uevent_suppress(&sch->dev, 0);
-			kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-		}
 	}
 	return ret;
 }
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index cca1a7c4bb33..6ebc0f7152e9 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -449,16 +449,6 @@ int css_register_subchannel(struct subchannel *sch)
 	if (sch->st == SUBCHANNEL_TYPE_IO)
 		sch->dev.type = &io_subchannel_type;
 
-	/*
-	 * We don't want to generate uevents for I/O subchannels that don't
-	 * have a working ccw device behind them since they will be
-	 * unregistered before they can be used anyway, so we delay the add
-	 * uevent until after device recognition was successful.
-	 * Note that we suppress the uevent for all subchannel types;
-	 * the subchannel driver can decide itself when it wants to inform
-	 * userspace of its existence.
-	 */
-	dev_set_uevent_suppress(&sch->dev, 1);
 	css_update_ssd_info(sch);
 	/* make it known to the system */
 	ret = css_sch_device_register(sch);
@@ -467,15 +457,6 @@ int css_register_subchannel(struct subchannel *sch)
 			      sch->schid.ssid, sch->schid.sch_no, ret);
 		return ret;
 	}
-	if (!sch->driver) {
-		/*
-		 * No driver matched. Generate the uevent now so that
-		 * a fitting driver module may be loaded based on the
-		 * modalias.
-		 */
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-	}
 	return ret;
 }
 
diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index aab13c78db9f..363c9bbc4a24 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -845,14 +845,6 @@ static void io_subchannel_register(struct ccw_device *cdev)
 		adjust_init_count = 0;
 		goto out;
 	}
-	/*
-	 * Now we know this subchannel will stay, we can throw
-	 * our delayed uevent.
-	 */
-	if (dev_get_uevent_suppress(&sch->dev)) {
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-	}
 	/* make it known to the system */
 	ret = ccw_device_add(cdev);
 	if (ret) {
@@ -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.
-		 */
-		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) {
diff --git a/drivers/s390/cio/eadm_sch.c b/drivers/s390/cio/eadm_sch.c
index 53468ae64b99..f8b43583b487 100644
--- a/drivers/s390/cio/eadm_sch.c
+++ b/drivers/s390/cio/eadm_sch.c
@@ -243,11 +243,6 @@ static int eadm_subchannel_probe(struct subchannel *sch)
 	spin_lock_irq(&list_lock);
 	list_add(&private->head, &eadm_list);
 	spin_unlock_irq(&list_lock);
-
-	if (dev_get_uevent_suppress(&sch->dev)) {
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-	}
 out:
 	return ret;
 }
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 8c625b530035..7ccc5035c8fd 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -206,11 +206,6 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	if (ret)
 		goto out_disable;
 
-	if (dev_get_uevent_suppress(&sch->dev)) {
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
-	}
-
 	VFIO_CCW_MSG_EVENT(4, "bound to subchannel %x.%x.%04x\n",
 			   sch->schid.cssid, sch->schid.ssid,
 			   sch->schid.sch_no);
-- 
2.17.1

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

[Cornelia Huck]

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?

(...)

> @@ -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) {

(...)

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

[Vineeth Vijayan]

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) {
> (...)
>

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

[Vineeth Vijayan]

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) {
>> (...)
>>

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

[Cornelia Huck]

On Mon, 7 Dec 2020 09:09:48 +0100
Vineeth Vijayan <vneethv@linux.ibm.com> wrote:

> 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.

Agreed. A device being found not operational during actual ccw device
setup but not earlier is probably an edge case.

> 
> 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.

IIUC, we'll get the "normal" ADD uevent when the subchannel device is
registered (i.e. made visible). For the vfio-ccw case, we want the
driverctl rule to match in this case, so that the driver override can
be set before the subchannel device ends up being bound to the I/O
subchannel driver. So I think that removing the suppression is giving
us exactly what we want? Modulo any errors in the initialization
sequence we might currently have in the css bus code, of course.

I'm not sure how many rules actually care about events for the
subchannel device; the ccw device seems like the more helpful device to
watch out for.

> 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.

Looking on my LPAR, it seems the driverctl rule is the second in
priority; i.e., it will be called before nearly everything else. This
should suffice, I hope?

> 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.

I think the vfio-ccw specific rules are those we need to worry about
the least. Again, from a quick browse on my LPAR, the existing rules
seem pretty sane AFAICS. I cannot speak for all the different distros,
though :)

One thing that probably should be changed together with the removal of
the uevent suppression is the de-registration of the subchannel device
by the ccw bus. Likely an edge case, but might cause confusion when a
subchannel is immediately gone again.

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

[Halil Pasic]

On Tue, 8 Dec 2020 18:30:54 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> > 
> > 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.  
> 
> IIUC, we'll get the "normal" ADD uevent when the subchannel device is
> registered (i.e. made visible). For the vfio-ccw case, we want the
> driverctl rule to match in this case, so that the driver override can
> be set before the subchannel device ends up being bound to the I/O
> subchannel driver. So I think that removing the suppression is giving
> us exactly what we want? Modulo any errors in the initialization
> sequence we might currently have in the css bus code, of course.
>

I believe, I'm the originator of these concerns, yet I find my
concern hard to recognize in the comment of Vineeth, so let me
please try to explain this in a different way.

AFAIK the uevent handling is asynchronous with regards to matching and
probing, in a sense that there is no synchronization mechanism that
ensures, the userspace has had the ADD event handled (e.g.
driver_override set_ before the kernel proceeds with matching and
probing of the device. Am I wrong about this?

If I'm, with the suppression gone we end up with race, where userspace
may or may not set driver_override in time.

The man page of driverctl
(https://manpages.debian.org/testing/driverctl/driverctl.8.en.html)
claims that: "driverctl integrates with udev to support overriding driver
selection for both cold- and hotplugged devices from the moment of discovery, ..."
and "The driver overrides created by driverctl are persistent across
system reboots by default."

Writing to the driver_override sysfs attribute does not auto-rebind. So
if we can't ensure being in time to set driver_override for the
subchannel before the io_subchannel driver binds, then the userspace
should handle this situation (by unbind and bind) to ensure the
effectiveness of 'driver override'. I couldn't find that code in
driverctl, and I assume if we had that, driver override would work
without this patch. Conny, does that sound about right?

My argument is purely speculative. I didn't try this out, but trying
stuff out is of limited value with races anyway. Vineeth did you try?
If not, I could check this out myself some time later.
 
> I'm not sure how many rules actually care about events for the
> subchannel device; the ccw device seems like the more helpful device to
> watch out for.

I tend to agree, but the problem with vfio-ccw is that (currently) we
don't have a ccw device in the host, because we pass-through the
subchannel. When we interrogate the subchannel, we do learn if there
is a device and if, what is its devno. If I were to run a system with
vfio-ccw passthrough, I would want to passthrough the subchannel that
talks to the DASD (identified by devno) I need passed through to my
guest.

Regards,
Halil

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

[Cornelia Huck]

On Wed, 9 Dec 2020 13:52:03 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 8 Dec 2020 18:30:54 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > > 
> > > 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.    
> > 
> > IIUC, we'll get the "normal" ADD uevent when the subchannel device is
> > registered (i.e. made visible). For the vfio-ccw case, we want the
> > driverctl rule to match in this case, so that the driver override can
> > be set before the subchannel device ends up being bound to the I/O
> > subchannel driver. So I think that removing the suppression is giving
> > us exactly what we want? Modulo any errors in the initialization
> > sequence we might currently have in the css bus code, of course.
> >  
> 
> I believe, I'm the originator of these concerns, yet I find my
> concern hard to recognize in the comment of Vineeth, so let me
> please try to explain this in a different way.
> 
> AFAIK the uevent handling is asynchronous with regards to matching and
> probing, in a sense that there is no synchronization mechanism that
> ensures, the userspace has had the ADD event handled (e.g.
> driver_override set_ before the kernel proceeds with matching and
> probing of the device. Am I wrong about this?
> 
> If I'm, with the suppression gone we end up with race, where userspace
> may or may not set driver_override in time.
> 
> The man page of driverctl
> (https://manpages.debian.org/testing/driverctl/driverctl.8.en.html)
> claims that: "driverctl integrates with udev to support overriding driver
> selection for both cold- and hotplugged devices from the moment of discovery, ..."
> and "The driver overrides created by driverctl are persistent across
> system reboots by default."
> 
> Writing to the driver_override sysfs attribute does not auto-rebind. So
> if we can't ensure being in time to set driver_override for the
> subchannel before the io_subchannel driver binds, then the userspace
> should handle this situation (by unbind and bind) to ensure the
> effectiveness of 'driver override'. I couldn't find that code in
> driverctl, and I assume if we had that, driver override would work
> without this patch. Conny, does that sound about right?
> 
> My argument is purely speculative. I didn't try this out, but trying
> stuff out is of limited value with races anyway. Vineeth did you try?
> If not, I could check this out myself some time later.

Whenever we delegate policy decisions like that to userspace, we'll end
up with uncertainty depending on timing. I don't think that there's any
way around it. (FWIW, driver_override for pci behaves just the same,
unless I misread the code.)

What removing the uevent suppression does give us is at least a chance
to influence the driver that is being bound and not wait until we have
a fully setup ccw device as a child as well.

>  
> > I'm not sure how many rules actually care about events for the
> > subchannel device; the ccw device seems like the more helpful device to
> > watch out for.  
> 
> I tend to agree, but the problem with vfio-ccw is that (currently) we
> don't have a ccw device in the host, because we pass-through the
> subchannel. When we interrogate the subchannel, we do learn if there
> is a device and if, what is its devno. If I were to run a system with
> vfio-ccw passthrough, I would want to passthrough the subchannel that
> talks to the DASD (identified by devno) I need passed through to my
> guest.

I think that can be solved by simply adding the devno as a variable to
the uevent (valid if it's an I/O subchannel; we don't register the
subchannel in the first place if dnv is not set.)

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

[Halil Pasic]

On Tue, 15 Dec 2020 19:13:07 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 9 Dec 2020 13:52:03 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 8 Dec 2020 18:30:54 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > > 
> > > > 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.    
> > > 
> > > IIUC, we'll get the "normal" ADD uevent when the subchannel device is
> > > registered (i.e. made visible). For the vfio-ccw case, we want the
> > > driverctl rule to match in this case, so that the driver override can
> > > be set before the subchannel device ends up being bound to the I/O
> > > subchannel driver. So I think that removing the suppression is giving
> > > us exactly what we want? Modulo any errors in the initialization
> > > sequence we might currently have in the css bus code, of course.
> > >  
> > 
> > I believe, I'm the originator of these concerns, yet I find my
> > concern hard to recognize in the comment of Vineeth, so let me
> > please try to explain this in a different way.
> > 
> > AFAIK the uevent handling is asynchronous with regards to matching and
> > probing, in a sense that there is no synchronization mechanism that
> > ensures, the userspace has had the ADD event handled (e.g.
> > driver_override set_ before the kernel proceeds with matching and
> > probing of the device. Am I wrong about this?
> > 
> > If I'm, with the suppression gone we end up with race, where userspace
> > may or may not set driver_override in time.
> > 
> > The man page of driverctl
> > (https://manpages.debian.org/testing/driverctl/driverctl.8.en.html)
> > claims that: "driverctl integrates with udev to support overriding driver
> > selection for both cold- and hotplugged devices from the moment of discovery, ..."
> > and "The driver overrides created by driverctl are persistent across
> > system reboots by default."
> > 
> > Writing to the driver_override sysfs attribute does not auto-rebind. So
> > if we can't ensure being in time to set driver_override for the
> > subchannel before the io_subchannel driver binds, then the userspace
> > should handle this situation (by unbind and bind) to ensure the
> > effectiveness of 'driver override'. I couldn't find that code in
> > driverctl, and I assume if we had that, driver override would work
> > without this patch. Conny, does that sound about right?
> > 
> > My argument is purely speculative. I didn't try this out, but trying
> > stuff out is of limited value with races anyway. Vineeth did you try?
> > If not, I could check this out myself some time later.
> 
> Whenever we delegate policy decisions like that to userspace, we'll end
> up with uncertainty depending on timing. I don't think that there's any
> way around it. (FWIW, driver_override for pci behaves just the same,
> unless I misread the code.)
> 
> What removing the uevent suppression does give us is at least a chance
> to influence the driver that is being bound and not wait until we have
> a fully setup ccw device as a child as well.

I finally came around to test this. In my experience driverctl works for
subchannels and vfio_ccw without this patch, and continues to work with
it. I found the code in driverctl that does the unbind and the implicit
bind (via drivers_probe after after driver_override was set).

So now I have to ask, how exactly was the original problem diagnosed?

In https://marc.info/?l=linux-s390&m=158591045732735&w=2 there is a
paragraph like:

"""
So while there's definitely a good reason for wanting to delay uevents,
it is also introducing problems. One is udev rules for subchannels that
are supposed to do something before a driver binds (e.g. setting
driver_override to bind an I/O subchannel to vfio_ccw instead of
io_subchannel) are not effective, as the ADD uevent will only be
generated when the io_subchannel driver is already done with doing all
setup. Another one is that only the ADD uevent is generated after
uevent suppression is lifted; any other uevents that might have been
generated are lost.
"""

This is not how driverclt works! I.e. it deals with the situation that
the I/O subchannel was already bound to the io_subchannel driver at
the time the udev rule installed by driverctl activates (via the
mechanism I described above).

Regards,
Halil

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

[Halil Pasic]

On Wed, 16 Dec 2020 13:07:10 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 16 Dec 2020 12:53:41 +0100
> Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
> 
> > On 12/15/20 7:13 PM, Cornelia Huck wrote:
> > >>     
> > >>> I'm not sure how many rules actually care about events for the
> > >>> subchannel device; the ccw device seems like the more helpful device to
> > >>> watch out for.  
> > >> I tend to agree, but the problem with vfio-ccw is that (currently) we
> > >> don't have a ccw device in the host, because we pass-through the
> > >> subchannel. When we interrogate the subchannel, we do learn if there
> > >> is a device and if, what is its devno. If I were to run a system with
> > >> vfio-ccw passthrough, I would want to passthrough the subchannel that
> > >> talks to the DASD (identified by devno) I need passed through to my
> > >> guest.  
> > > I think that can be solved by simply adding the devno as a variable to
> > > the uevent (valid if it's an I/O subchannel; we don't register the
> > > subchannel in the first place if dnv is not set.)
> > >   
> > Providing the devno in the context of the udev event certainly helps if 
> > the event consumer would base its actions on it.
> > As far as I understand the driver_override mechanics driverctl sets the 
> > override based on a specified device. In that case the devno would not 
> > be looked at and the subchannel would end up with a vfio-ccw driver even 
> > so the ccw device might not be the one we want to use as pass-through 
> > device.
> 
> Hm, maybe we need to make a change in driverctl that allows per-bus
> custom rules?
> 

The issue with that is, that this problem ain't bus specific. I.e. it
could make perfect sense to driver_override a certain ccw tape device to
an alternative tape driver.

The problem is, that the only way driverctl can identify a device is a
(name_of_the_bus), device_name_on_the bus) pair. Currently the udev rule
installed by driverctl simply ooks fora file 
/etc/driverctl.d/$env{SUBSYSTEM}-$kernel
which basically encodes the current selection criteria.

Can yo please elaborate on your idea? How would you extend the driverctl
cli and how would persistence look like for these custom rules? Would
you make driverctl write an udev rule for each such device/custom rule
on 'set-override' command instead of file in /etc/driverctl.d?

Regards,
Halil

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

[Cornelia Huck]

On Sat, 19 Dec 2020 07:33:16 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> I finally came around to test this. In my experience driverctl works for
> subchannels and vfio_ccw without this patch, and continues to work with
> it. I found the code in driverctl that does the unbind and the implicit
> bind (via drivers_probe after after driver_override was set).
> 
> So now I have to ask, how exactly was the original problem diagnosed?
> 
> In https://marc.info/?l=linux-s390&m=158591045732735&w=2 there is a
> paragraph like:
> 
> """
> So while there's definitely a good reason for wanting to delay uevents,
> it is also introducing problems. One is udev rules for subchannels that
> are supposed to do something before a driver binds (e.g. setting
> driver_override to bind an I/O subchannel to vfio_ccw instead of
> io_subchannel) are not effective, as the ADD uevent will only be
> generated when the io_subchannel driver is already done with doing all
> setup. Another one is that only the ADD uevent is generated after
> uevent suppression is lifted; any other uevents that might have been
> generated are lost.
> """
> 
> This is not how driverclt works! I.e. it deals with the situation that
> the I/O subchannel was already bound to the io_subchannel driver at
> the time the udev rule installed by driverctl activates (via the
> mechanism I described above).

That's... weird. It definitely did not work on the LPAR I initially
tried it out on!

However, I think removing the suppression still looks like a good idea:
we still have the "any uevent other than ADD will have been lost"
problem.

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

[Cornelia Huck]

On Sat, 19 Dec 2020 08:20:06 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 16 Dec 2020 13:07:10 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 16 Dec 2020 12:53:41 +0100
> > Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
> >   
> > > On 12/15/20 7:13 PM, Cornelia Huck wrote:  
> > > >>       
> > > >>> I'm not sure how many rules actually care about events for the
> > > >>> subchannel device; the ccw device seems like the more helpful device to
> > > >>> watch out for.    
> > > >> I tend to agree, but the problem with vfio-ccw is that (currently) we
> > > >> don't have a ccw device in the host, because we pass-through the
> > > >> subchannel. When we interrogate the subchannel, we do learn if there
> > > >> is a device and if, what is its devno. If I were to run a system with
> > > >> vfio-ccw passthrough, I would want to passthrough the subchannel that
> > > >> talks to the DASD (identified by devno) I need passed through to my
> > > >> guest.    
> > > > I think that can be solved by simply adding the devno as a variable to
> > > > the uevent (valid if it's an I/O subchannel; we don't register the
> > > > subchannel in the first place if dnv is not set.)
> > > >     
> > > Providing the devno in the context of the udev event certainly helps if 
> > > the event consumer would base its actions on it.
> > > As far as I understand the driver_override mechanics driverctl sets the 
> > > override based on a specified device. In that case the devno would not 
> > > be looked at and the subchannel would end up with a vfio-ccw driver even 
> > > so the ccw device might not be the one we want to use as pass-through 
> > > device.  
> > 
> > Hm, maybe we need to make a change in driverctl that allows per-bus
> > custom rules?
> >   
> 
> The issue with that is, that this problem ain't bus specific. I.e. it
> could make perfect sense to driver_override a certain ccw tape device to
> an alternative tape driver.

But ccw does not provide driver_override? Confused.

> 
> The problem is, that the only way driverctl can identify a device is a
> (name_of_the_bus), device_name_on_the bus) pair. Currently the udev rule
> installed by driverctl simply ooks fora file 
> /etc/driverctl.d/$env{SUBSYSTEM}-$kernel
> which basically encodes the current selection criteria.
> 
> Can yo please elaborate on your idea? How would you extend the driverctl
> cli and how would persistence look like for these custom rules? Would
> you make driverctl write an udev rule for each such device/custom rule
> on 'set-override' command instead of file in /etc/driverctl.d?

I have not really looked at how to implement this. But we could have
driverctl support an optional "additional_parameters" option, which
allows to specify key/value pairs that have to match. I guess that
should be dropped into the driverctl config directory, and generate an
additional check?

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

[Halil Pasic]

On Mon, 21 Dec 2020 16:46:34 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Sat, 19 Dec 2020 07:33:16 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > I finally came around to test this. In my experience driverctl works for
> > subchannels and vfio_ccw without this patch, and continues to work with
> > it. I found the code in driverctl that does the unbind and the implicit
> > bind (via drivers_probe after after driver_override was set).
> > 
> > So now I have to ask, how exactly was the original problem diagnosed?
> > 
> > In https://marc.info/?l=linux-s390&m=158591045732735&w=2 there is a
> > paragraph like:
> > 
> > """
> > So while there's definitely a good reason for wanting to delay uevents,
> > it is also introducing problems. One is udev rules for subchannels that
> > are supposed to do something before a driver binds (e.g. setting
> > driver_override to bind an I/O subchannel to vfio_ccw instead of
> > io_subchannel) are not effective, as the ADD uevent will only be
> > generated when the io_subchannel driver is already done with doing all
> > setup. Another one is that only the ADD uevent is generated after
> > uevent suppression is lifted; any other uevents that might have been
> > generated are lost.
> > """
> > 
> > This is not how driverclt works! I.e. it deals with the situation that
> > the I/O subchannel was already bound to the io_subchannel driver at
> > the time the udev rule installed by driverctl activates (via the
> > mechanism I described above).
> 
> That's... weird. It definitely did not work on the LPAR I initially
> tried it out on!
> 

I think Boris told me some weeks ago that it didn't work for him either.
I will check with him after the winter sleep.

> However, I think removing the suppression still looks like a good idea:
> we still have the "any uevent other than ADD will have been lost"
> problem.
> 

I agree. I didn't look into the details, in general I think removing
quirks specific to 390 (when possible) is a good thing.

Regards,
Halil

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

[Halil Pasic]

On Mon, 21 Dec 2020 16:52:19 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Sat, 19 Dec 2020 08:20:06 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Wed, 16 Dec 2020 13:07:10 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Wed, 16 Dec 2020 12:53:41 +0100
> > > Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
> > >   
> > > > On 12/15/20 7:13 PM, Cornelia Huck wrote:  
> > > > >>       
> > > > >>> I'm not sure how many rules actually care about events for the
> > > > >>> subchannel device; the ccw device seems like the more helpful device to
> > > > >>> watch out for.    
> > > > >> I tend to agree, but the problem with vfio-ccw is that (currently) we
> > > > >> don't have a ccw device in the host, because we pass-through the
> > > > >> subchannel. When we interrogate the subchannel, we do learn if there
> > > > >> is a device and if, what is its devno. If I were to run a system with
> > > > >> vfio-ccw passthrough, I would want to passthrough the subchannel that
> > > > >> talks to the DASD (identified by devno) I need passed through to my
> > > > >> guest.    
> > > > > I think that can be solved by simply adding the devno as a variable to
> > > > > the uevent (valid if it's an I/O subchannel; we don't register the
> > > > > subchannel in the first place if dnv is not set.)
> > > > >     
> > > > Providing the devno in the context of the udev event certainly helps if 
> > > > the event consumer would base its actions on it.
> > > > As far as I understand the driver_override mechanics driverctl sets the 
> > > > override based on a specified device. In that case the devno would not 
> > > > be looked at and the subchannel would end up with a vfio-ccw driver even 
> > > > so the ccw device might not be the one we want to use as pass-through 
> > > > device.  
> > > 
> > > Hm, maybe we need to make a change in driverctl that allows per-bus
> > > custom rules?
> > >   
> > 
> > The issue with that is, that this problem ain't bus specific. I.e. it
> > could make perfect sense to driver_override a certain ccw tape device to
> > an alternative tape driver.
> 
> But ccw does not provide driver_override? Confused.
> 

Right, but it could. That is not my point. I was arguing with 'per-bus
custom rules'.

> > 
> > The problem is, that the only way driverctl can identify a device is a
> > (name_of_the_bus), device_name_on_the bus) pair. Currently the udev rule
> > installed by driverctl simply ooks fora file 
> > /etc/driverctl.d/$env{SUBSYSTEM}-$kernel
> > which basically encodes the current selection criteria.
> > 
> > Can yo please elaborate on your idea? How would you extend the driverctl
> > cli and how would persistence look like for these custom rules? Would
> > you make driverctl write an udev rule for each such device/custom rule
> > on 'set-override' command instead of file in /etc/driverctl.d?
> 
> I have not really looked at how to implement this. But we could have
> driverctl support an optional "additional_parameters" option, which
> allows to specify key/value pairs that have to match. I guess that
> should be dropped into the driverctl config directory, and generate an
> additional check?
> 

The devil is always in the details. I'm sure something can be done :).
Key/value pairs for matching fits IMHO better with new udev rules than
with files in the driverctl config dir that somehow generate additiona
checks. One thing I didn't understand is, are you proposing an invocation
like 
driverctl -b css --additional_params="DEVNO=<devno>" \
set-override <subchannel_no> vfio_ccw
or 
driverctl -b css --additional_params="DEVNO=<devno>" \
set-override vfio_ccw
?

The first one would safeguard against passing through the wrong devno,
but I think, we actually want to be oblivious about the subchannel
number.

Regards,
Halil

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

[Boris Fiuczynski]

On 12/21/20 5:51 PM, Halil Pasic wrote:
> On Mon, 21 Dec 2020 16:46:34 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Sat, 19 Dec 2020 07:33:16 +0100
>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>
>>> I finally came around to test this. In my experience driverctl works for
>>> subchannels and vfio_ccw without this patch, and continues to work with
>>> it. I found the code in driverctl that does the unbind and the implicit
>>> bind (via drivers_probe after after driver_override was set).
>>>
>>> So now I have to ask, how exactly was the original problem diagnosed?
>>>
>>> In https://marc.info/?l=linux-s390&m=158591045732735&w=2 there is a
>>> paragraph like:
>>>
>>> """
>>> So while there's definitely a good reason for wanting to delay uevents,
>>> it is also introducing problems. One is udev rules for subchannels that
>>> are supposed to do something before a driver binds (e.g. setting
>>> driver_override to bind an I/O subchannel to vfio_ccw instead of
>>> io_subchannel) are not effective, as the ADD uevent will only be
>>> generated when the io_subchannel driver is already done with doing all
>>> setup. Another one is that only the ADD uevent is generated after
>>> uevent suppression is lifted; any other uevents that might have been
>>> generated are lost.
>>> """
>>>
>>> This is not how driverclt works! I.e. it deals with the situation that
>>> the I/O subchannel was already bound to the io_subchannel driver at
>>> the time the udev rule installed by driverctl activates (via the
>>> mechanism I described above).
>>
>> That's... weird. It definitely did not work on the LPAR I initially
>> tried it out on!
>>
> 
> I think Boris told me some weeks ago that it didn't work for him either.
> I will check with him after the winter sleep.

Yesterday I used driverctl successfully for a subchannel on F33.

Not sure what went wrong a couple of months ago but I cannot reproduce 
driverctl not working now.

> 
>> However, I think removing the suppression still looks like a good idea:
>> we still have the "any uevent other than ADD will have been lost"
>> problem.
>>
I totally agree with this.

> 
> I agree. I didn't look into the details, in general I think removing
> quirks specific to 390 (when possible) is a good thing.
> 
> Regards,
> Halil
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Halil Pasic]

On Thu, 14 Jan 2021 14:03:25 +0100
Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:

> On 12/21/20 5:51 PM, Halil Pasic wrote:
> > On Mon, 21 Dec 2020 16:46:34 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> >> On Sat, 19 Dec 2020 07:33:16 +0100
> >> Halil Pasic <pasic@linux.ibm.com> wrote:
> >>  
> >>> I finally came around to test this. In my experience driverctl works for
> >>> subchannels and vfio_ccw without this patch, and continues to work with
> >>> it. I found the code in driverctl that does the unbind and the implicit
> >>> bind (via drivers_probe after after driver_override was set).
> >>>
> >>> So now I have to ask, how exactly was the original problem diagnosed?
> >>>
> >>> In https://marc.info/?l=linux-s390&m=158591045732735&w=2 there is a
> >>> paragraph like:
> >>>
> >>> """
> >>> So while there's definitely a good reason for wanting to delay uevents,
> >>> it is also introducing problems. One is udev rules for subchannels that
> >>> are supposed to do something before a driver binds (e.g. setting
> >>> driver_override to bind an I/O subchannel to vfio_ccw instead of
> >>> io_subchannel) are not effective, as the ADD uevent will only be
> >>> generated when the io_subchannel driver is already done with doing all
> >>> setup. Another one is that only the ADD uevent is generated after
> >>> uevent suppression is lifted; any other uevents that might have been
> >>> generated are lost.
> >>> """
> >>>
> >>> This is not how driverclt works! I.e. it deals with the situation that
> >>> the I/O subchannel was already bound to the io_subchannel driver at
> >>> the time the udev rule installed by driverctl activates (via the
> >>> mechanism I described above).  
> >>
> >> That's... weird. It definitely did not work on the LPAR I initially
> >> tried it out on!
> >>  
> > 
> > I think Boris told me some weeks ago that it didn't work for him either.
> > I will check with him after the winter sleep.  
> 
> Yesterday I used driverctl successfully for a subchannel on F33.
> 
> Not sure what went wrong a couple of months ago but I cannot reproduce 
> driverctl not working now.

Thanks Boris!

@Conny: IMHO driver_override has to work without this patch. Can you
figure out, why did you claim it does not (and provide instructions
on how to reproduce the problem)?

> 
> >   
> >> However, I think removing the suppression still looks like a good idea:
> >> we still have the "any uevent other than ADD will have been lost"
> >> problem.
> >>  
> I totally agree with this.

@Vineeth: I think the best way to move forward is to respin this patch
with a commit message, that doesn't argue about driver_override.

Regards,
Halil

[..]

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

[Cornelia Huck]

On Tue, 19 Jan 2021 12:47:24 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 14 Jan 2021 14:03:25 +0100
> Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
> 
> > On 12/21/20 5:51 PM, Halil Pasic wrote:  
> > > On Mon, 21 Dec 2020 16:46:34 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >     
> > >> On Sat, 19 Dec 2020 07:33:16 +0100
> > >> Halil Pasic <pasic@linux.ibm.com> wrote:
> > >>    
> > >>> I finally came around to test this. In my experience driverctl works for
> > >>> subchannels and vfio_ccw without this patch, and continues to work with
> > >>> it. I found the code in driverctl that does the unbind and the implicit
> > >>> bind (via drivers_probe after after driver_override was set).
> > >>>
> > >>> So now I have to ask, how exactly was the original problem diagnosed?
> > >>>
> > >>> In https://marc.info/?l=linux-s390&m=158591045732735&w=2 there is a
> > >>> paragraph like:
> > >>>
> > >>> """
> > >>> So while there's definitely a good reason for wanting to delay uevents,
> > >>> it is also introducing problems. One is udev rules for subchannels that
> > >>> are supposed to do something before a driver binds (e.g. setting
> > >>> driver_override to bind an I/O subchannel to vfio_ccw instead of
> > >>> io_subchannel) are not effective, as the ADD uevent will only be
> > >>> generated when the io_subchannel driver is already done with doing all
> > >>> setup. Another one is that only the ADD uevent is generated after
> > >>> uevent suppression is lifted; any other uevents that might have been
> > >>> generated are lost.
> > >>> """
> > >>>
> > >>> This is not how driverclt works! I.e. it deals with the situation that
> > >>> the I/O subchannel was already bound to the io_subchannel driver at
> > >>> the time the udev rule installed by driverctl activates (via the
> > >>> mechanism I described above).    
> > >>
> > >> That's... weird. It definitely did not work on the LPAR I initially
> > >> tried it out on!
> > >>    
> > > 
> > > I think Boris told me some weeks ago that it didn't work for him either.
> > > I will check with him after the winter sleep.    
> > 
> > Yesterday I used driverctl successfully for a subchannel on F33.
> > 
> > Not sure what went wrong a couple of months ago but I cannot reproduce 
> > driverctl not working now.  
> 
> Thanks Boris!
> 
> @Conny: IMHO driver_override has to work without this patch. Can you
> figure out, why did you claim it does not (and provide instructions
> on how to reproduce the problem)?

This may have been due to other reasons and only looking like a uevent
issue at the first glance; however, I do not have that particular setup
anymore, so I guess we'll never know.

> 
> >   
> > >     
> > >> However, I think removing the suppression still looks like a good idea:
> > >> we still have the "any uevent other than ADD will have been lost"
> > >> problem.
> > >>    
> > I totally agree with this.  
> 
> @Vineeth: I think the best way to move forward is to respin this patch
> with a commit message, that doesn't argue about driver_override.

That sounds good to me.

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

[Vineeth Vijayan]

On 1/19/21 12:59 PM, Cornelia Huck wrote:
> On Tue, 19 Jan 2021 12:47:24 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
>
>> On Thu, 14 Jan 2021 14:03:25 +0100
>> Boris Fiuczynski <fiuczy@linux.ibm.com> wrote:
>>
>>> On 12/21/20 5:51 PM, Halil Pasic wrote:
>>>> On Mon, 21 Dec 2020 16:46:34 +0100
>>>> Cornelia Huck <cohuck@redhat.com> wrote:
>>>>      
>>>>> On Sat, 19 Dec 2020 07:33:16 +0100
>>>>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>>>>     
>>>>>> I finally came around to test this. In my experience driverctl works for
>>>>>> subchannels and vfio_ccw without this patch, and continues to work with
>>>>>> it. I found the code in driverctl that does the unbind and the implicit
>>>>>> bind (via drivers_probe after after driver_override was set).
>>>>>>
>>>>>> So now I have to ask, how exactly was the original problem diagnosed?
>>>>>>
>>>>>> In https://marc.info/?l=linux-s390&m=158591045732735&w=2 there is a
>>>>>> paragraph like:
>>>>>>
>>>>>> """
>>>>>> So while there's definitely a good reason for wanting to delay uevents,
>>>>>> it is also introducing problems. One is udev rules for subchannels that
>>>>>> are supposed to do something before a driver binds (e.g. setting
>>>>>> driver_override to bind an I/O subchannel to vfio_ccw instead of
>>>>>> io_subchannel) are not effective, as the ADD uevent will only be
>>>>>> generated when the io_subchannel driver is already done with doing all
>>>>>> setup. Another one is that only the ADD uevent is generated after
>>>>>> uevent suppression is lifted; any other uevents that might have been
>>>>>> generated are lost.
>>>>>> """
>>>>>>
>>>>>> This is not how driverclt works! I.e. it deals with the situation that
>>>>>> the I/O subchannel was already bound to the io_subchannel driver at
>>>>>> the time the udev rule installed by driverctl activates (via the
>>>>>> mechanism I described above).
>>>>> That's... weird. It definitely did not work on the LPAR I initially
>>>>> tried it out on!
>>>>>     
>>>> I think Boris told me some weeks ago that it didn't work for him either.
>>>> I will check with him after the winter sleep.
>>> Yesterday I used driverctl successfully for a subchannel on F33.
>>>
>>> Not sure what went wrong a couple of months ago but I cannot reproduce
>>> driverctl not working now.
>> Thanks Boris!
>>
>> @Conny: IMHO driver_override has to work without this patch. Can you
>> figure out, why did you claim it does not (and provide instructions
>> on how to reproduce the problem)?
> This may have been due to other reasons and only looking like a uevent
> issue at the first glance; however, I do not have that particular setup
> anymore, so I guess we'll never know.
>
>>>    
>>>>      
>>>>> However, I think removing the suppression still looks like a good idea:
>>>>> we still have the "any uevent other than ADD will have been lost"
>>>>> problem.
>>>>>     
>>> I totally agree with this.
>> @Vineeth: I think the best way to move forward is to respin this patch
>> with a commit message, that doesn't argue about driver_override.
> That sounds good to me. 
Thank you @Conny, @Halil and @Boris on the insights.
I see that the driver_override works with/without this patch anyway.
But, as mentioned previously, this is more like a cleanup patch.
I shall respin the patch as a cleanup patch for CIO layer.

Regards
Vineeth