[PATCH 0/2] s390/cio: correct some uevent issues

[PATCH 0/2] s390/cio: correct some uevent issues

[Cornelia Huck]

For subchannels, we currently delay the initial ADD uevent to a
point in time controlled by the driver bound to it (this is to
avoid a storm of useless uevents for I/O subchannels that do not
have an operational device behind it and will get deregistered
again, which are potentially a lot on LPARs.)

If we unbind from the io_subchannel driver and rebind again later,
we'll get a duplicate ADD uevent -- not a common workflow, but might
happen e.g. when you use a device in the host, then pass it to a
guest via vfio-ccw, and then want to use it in the host again. Fixed
by the first patch.

The vfio_ccw subchannel driver did not generate any ADD uevent at
all -- currently not a problem, as every I/O subchannel will have been
bound to the io_subchannel driver before, but let's fix this anyway
(second patch).

[As an aside, setting driver_override via a udev rule does not work
as expected, due to the uevent delaying -- a specified driver_override
works as designed, but userspace won't get the ADD uevent until after
the io_subchannel driver has been bound to the device already... we
may want to rethink this whole uevent mechanism for subchannels later,
but I don't think it's too pressing an issue.]

Probably easiest for both patches to go via the s390 arch maintainers.

Cornelia Huck (2):
  s390/cio: avoid duplicated 'ADD' uevents
  s390/cio: generate delayed uevent for vfio-ccw subchannels

 drivers/s390/cio/device.c       | 13 +++++++++----
 drivers/s390/cio/vfio_ccw_drv.c |  5 +++++
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.21.1

[PATCH 1/2] s390/cio: avoid duplicated 'ADD' uevents

[Cornelia Huck]

The common I/O layer delays the ADD uevent for subchannels and
delegates generating this uevent to the individual subchannel
drivers. The io_subchannel driver will do so when the associated
ccw_device has been registered -- but unconditionally, so more
ADD uevents will be generated if a subchannel has been unbound
from the io_subchannel driver and later rebound.

To fix this, only generate the ADD event if uevents were still
suppressed for the device.

Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Fixes: fa1a8c23eb7d ("s390: cio: Delay uevents for subchannels")
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/s390/cio/device.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index 0c6245fc7706..983f9c9e08de 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -849,8 +849,10 @@ static void io_subchannel_register(struct ccw_device *cdev)
 	 * Now we know this subchannel will stay, we can throw
 	 * our delayed uevent.
 	 */
-	dev_set_uevent_suppress(&sch->dev, 0);
-	kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
+	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) {
@@ -1058,8 +1060,11 @@ static int io_subchannel_probe(struct subchannel *sch)
 		 * Throw the delayed uevent for the subchannel, register
 		 * the ccw_device and exit.
 		 */
-		dev_set_uevent_suppress(&sch->dev, 0);
-		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
+		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) {
-- 
2.21.1

[PATCH 2/2] s390/cio: generate delayed uevent for vfio-ccw subchannels

[Cornelia Huck]

The common I/O layer delays the ADD uevent for subchannels and
delegates generating this uevent to the individual subchannel
drivers. The vfio-ccw I/O subchannel driver, however, did not
do that, and will not generate an ADD uevent for subchannels
that had not been bound to a different driver (or none at all,
which also triggers the uevent).

Generate the ADD uevent at the end of the probe function if
uevents were still suppressed for the device.

Fixes: 63f1934d562d ("vfio: ccw: basic implementation for vfio_ccw driver")
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index e401a3d0aa57..339a6bc0339b 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -167,6 +167,11 @@ 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.21.1

Re: [PATCH 1/2] s390/cio: avoid duplicated 'ADD' uevents

[Peter Oberparleiter]

On 27.03.2020 13:45, Cornelia Huck wrote:
> The common I/O layer delays the ADD uevent for subchannels and
> delegates generating this uevent to the individual subchannel
> drivers. The io_subchannel driver will do so when the associated
> ccw_device has been registered -- but unconditionally, so more
> ADD uevents will be generated if a subchannel has been unbound
> from the io_subchannel driver and later rebound.
> 
> To fix this, only generate the ADD event if uevents were still
> suppressed for the device.
> 
> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Fixes: fa1a8c23eb7d ("s390: cio: Delay uevents for subchannels")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Please also add Vineeth on cc of future patches against core CIO code.

This patch looks sane and makes handling of suppressed UDEV events
consistent to what is already implemented for CHSC and EADM subchannel
types.

Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>

> ---
>  drivers/s390/cio/device.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
> index 0c6245fc7706..983f9c9e08de 100644
> --- a/drivers/s390/cio/device.c
> +++ b/drivers/s390/cio/device.c
> @@ -849,8 +849,10 @@ static void io_subchannel_register(struct ccw_device *cdev)
>  	 * Now we know this subchannel will stay, we can throw
>  	 * our delayed uevent.
>  	 */
> -	dev_set_uevent_suppress(&sch->dev, 0);
> -	kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
> +	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) {
> @@ -1058,8 +1060,11 @@ static int io_subchannel_probe(struct subchannel *sch)
>  		 * Throw the delayed uevent for the subchannel, register
>  		 * the ccw_device and exit.
>  		 */
> -		dev_set_uevent_suppress(&sch->dev, 0);
> -		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
> +		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) {
> 


-- 
Peter Oberparleiter
Linux on Z Development - IBM Germany

Re: [PATCH 2/2] s390/cio: generate delayed uevent for vfio-ccw subchannels

[Eric Farman]

On 3/27/20 8:45 AM, Cornelia Huck wrote:
> The common I/O layer delays the ADD uevent for subchannels and
> delegates generating this uevent to the individual subchannel
> drivers. The vfio-ccw I/O subchannel driver, however, did not
> do that, and will not generate an ADD uevent for subchannels
> that had not been bound to a different driver (or none at all,
> which also triggers the uevent).
> 
> Generate the ADD uevent at the end of the probe function if
> uevents were still suppressed for the device.
> 
> Fixes: 63f1934d562d ("vfio: ccw: basic implementation for vfio_ccw driver")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

This seems to line up with the other drivers.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index e401a3d0aa57..339a6bc0339b 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -167,6 +167,11 @@ 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);
> 

Re: [PATCH 1/2] s390/cio: avoid duplicated 'ADD' uevents

[Boris Fiuczynski]

The code changes look good and I was able to test this fix successfully.
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>


On 3/27/20 1:45 PM, Cornelia Huck wrote:
> The common I/O layer delays the ADD uevent for subchannels and
> delegates generating this uevent to the individual subchannel
> drivers. The io_subchannel driver will do so when the associated
> ccw_device has been registered -- but unconditionally, so more
> ADD uevents will be generated if a subchannel has been unbound
> from the io_subchannel driver and later rebound.
> 
> To fix this, only generate the ADD event if uevents were still
> suppressed for the device.
> 
> Reported-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> Fixes: fa1a8c23eb7d ("s390: cio: Delay uevents for subchannels")
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   drivers/s390/cio/device.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
> index 0c6245fc7706..983f9c9e08de 100644
> --- a/drivers/s390/cio/device.c
> +++ b/drivers/s390/cio/device.c
> @@ -849,8 +849,10 @@ static void io_subchannel_register(struct ccw_device *cdev)
>   	 * Now we know this subchannel will stay, we can throw
>   	 * our delayed uevent.
>   	 */
> -	dev_set_uevent_suppress(&sch->dev, 0);
> -	kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
> +	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) {
> @@ -1058,8 +1060,11 @@ static int io_subchannel_probe(struct subchannel *sch)
>   		 * Throw the delayed uevent for the subchannel, register
>   		 * the ccw_device and exit.
>   		 */
> -		dev_set_uevent_suppress(&sch->dev, 0);
> -		kobject_uevent(&sch->dev.kobj, KOBJ_ADD);
> +		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) {
> 


-- 
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: [PATCH 0/2] s390/cio: correct some uevent issues

[Cornelia Huck]

On Fri, 27 Mar 2020 13:45:01 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> For subchannels, we currently delay the initial ADD uevent to a
> point in time controlled by the driver bound to it (this is to
> avoid a storm of useless uevents for I/O subchannels that do not
> have an operational device behind it and will get deregistered
> again, which are potentially a lot on LPARs.)
> 
> If we unbind from the io_subchannel driver and rebind again later,
> we'll get a duplicate ADD uevent -- not a common workflow, but might
> happen e.g. when you use a device in the host, then pass it to a
> guest via vfio-ccw, and then want to use it in the host again. Fixed
> by the first patch.
> 
> The vfio_ccw subchannel driver did not generate any ADD uevent at
> all -- currently not a problem, as every I/O subchannel will have been
> bound to the io_subchannel driver before, but let's fix this anyway
> (second patch).
> 
> [As an aside, setting driver_override via a udev rule does not work
> as expected, due to the uevent delaying -- a specified driver_override
> works as designed, but userspace won't get the ADD uevent until after
> the io_subchannel driver has been bound to the device already... we
> may want to rethink this whole uevent mechanism for subchannels later,
> but I don't think it's too pressing an issue.]
> 
> Probably easiest for both patches to go via the s390 arch maintainers.

Friendly ping. Anyone taking these?

> 
> Cornelia Huck (2):
>   s390/cio: avoid duplicated 'ADD' uevents
>   s390/cio: generate delayed uevent for vfio-ccw subchannels
> 
>  drivers/s390/cio/device.c       | 13 +++++++++----
>  drivers/s390/cio/vfio_ccw_drv.c |  5 +++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 

Re: [PATCH 0/2] s390/cio: correct some uevent issues

[Vasily Gorbik]

On Mon, Apr 06, 2020 at 01:54:41PM +0200, Cornelia Huck wrote:
> On Fri, 27 Mar 2020 13:45:01 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > For subchannels, we currently delay the initial ADD uevent to a
> > point in time controlled by the driver bound to it (this is to
> > avoid a storm of useless uevents for I/O subchannels that do not
> > have an operational device behind it and will get deregistered
> > again, which are potentially a lot on LPARs.)
> > 
> > If we unbind from the io_subchannel driver and rebind again later,
> > we'll get a duplicate ADD uevent -- not a common workflow, but might
> > happen e.g. when you use a device in the host, then pass it to a
> > guest via vfio-ccw, and then want to use it in the host again. Fixed
> > by the first patch.
> > 
> > The vfio_ccw subchannel driver did not generate any ADD uevent at
> > all -- currently not a problem, as every I/O subchannel will have been
> > bound to the io_subchannel driver before, but let's fix this anyway
> > (second patch).
> > 
> > [As an aside, setting driver_override via a udev rule does not work
> > as expected, due to the uevent delaying -- a specified driver_override
> > works as designed, but userspace won't get the ADD uevent until after
> > the io_subchannel driver has been bound to the device already... we
> > may want to rethink this whole uevent mechanism for subchannels later,
> > but I don't think it's too pressing an issue.]
> > 
> > Probably easiest for both patches to go via the s390 arch maintainers.
> 
> Friendly ping. Anyone taking these?

I've just applied them, thank you!

Re: [PATCH 0/2] s390/cio: correct some uevent issues

[Cornelia Huck]

On Mon, 6 Apr 2020 13:58:07 +0200
Vasily Gorbik <gor@linux.ibm.com> wrote:

> On Mon, Apr 06, 2020 at 01:54:41PM +0200, Cornelia Huck wrote:
> > On Fri, 27 Mar 2020 13:45:01 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > For subchannels, we currently delay the initial ADD uevent to a
> > > point in time controlled by the driver bound to it (this is to
> > > avoid a storm of useless uevents for I/O subchannels that do not
> > > have an operational device behind it and will get deregistered
> > > again, which are potentially a lot on LPARs.)
> > > 
> > > If we unbind from the io_subchannel driver and rebind again later,
> > > we'll get a duplicate ADD uevent -- not a common workflow, but might
> > > happen e.g. when you use a device in the host, then pass it to a
> > > guest via vfio-ccw, and then want to use it in the host again. Fixed
> > > by the first patch.
> > > 
> > > The vfio_ccw subchannel driver did not generate any ADD uevent at
> > > all -- currently not a problem, as every I/O subchannel will have been
> > > bound to the io_subchannel driver before, but let's fix this anyway
> > > (second patch).
> > > 
> > > [As an aside, setting driver_override via a udev rule does not work
> > > as expected, due to the uevent delaying -- a specified driver_override
> > > works as designed, but userspace won't get the ADD uevent until after
> > > the io_subchannel driver has been bound to the device already... we
> > > may want to rethink this whole uevent mechanism for subchannels later,
> > > but I don't think it's too pressing an issue.]
> > > 
> > > Probably easiest for both patches to go via the s390 arch maintainers.  
> > 
> > Friendly ping. Anyone taking these?  
> 
> I've just applied them, thank you!
> 

Wonderful, thanks!