[PATCH v2 1/4] s390/cio: Make struct css_driver::remove return void

[PATCH v2 1/4] s390/cio: Make struct css_driver::remove return void

[Uwe Kleine-König]

The driver core ignores the return value of css_remove()
(because there is only little it can do when a device disappears) and
there are no pci_epf_drivers with a remove callback.

So make it impossible for future drivers to return an unused error code
by changing the remove prototype to return void.

The real motivation for this change is the quest to make struct
bus_type::remove return void, too.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/s390/cio/chsc_sch.c     | 3 +--
 drivers/s390/cio/css.c          | 7 ++++---
 drivers/s390/cio/css.h          | 2 +-
 drivers/s390/cio/device.c       | 5 ++---
 drivers/s390/cio/eadm_sch.c     | 4 +---
 drivers/s390/cio/vfio_ccw_drv.c | 3 +--
 6 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/s390/cio/chsc_sch.c b/drivers/s390/cio/chsc_sch.c
index c42405c620b5..684348d82f08 100644
--- a/drivers/s390/cio/chsc_sch.c
+++ b/drivers/s390/cio/chsc_sch.c
@@ -100,7 +100,7 @@ static int chsc_subchannel_probe(struct subchannel *sch)
 	return ret;
 }
 
-static int chsc_subchannel_remove(struct subchannel *sch)
+static void chsc_subchannel_remove(struct subchannel *sch)
 {
 	struct chsc_private *private;
 
@@ -112,7 +112,6 @@ static int chsc_subchannel_remove(struct subchannel *sch)
 		put_device(&sch->dev);
 	}
 	kfree(private);
-	return 0;
 }
 
 static void chsc_subchannel_shutdown(struct subchannel *sch)
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index a974943c27da..092fd1ea5799 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -1374,12 +1374,13 @@ static int css_probe(struct device *dev)
 static int css_remove(struct device *dev)
 {
 	struct subchannel *sch;
-	int ret;
 
 	sch = to_subchannel(dev);
-	ret = sch->driver->remove ? sch->driver->remove(sch) : 0;
+	if (sch->driver->remove)
+		sch->driver->remove(sch);
 	sch->driver = NULL;
-	return ret;
+
+	return 0;
 }
 
 static void css_shutdown(struct device *dev)
diff --git a/drivers/s390/cio/css.h b/drivers/s390/cio/css.h
index 2eddfc47f687..c98522cbe276 100644
--- a/drivers/s390/cio/css.h
+++ b/drivers/s390/cio/css.h
@@ -81,7 +81,7 @@ struct css_driver {
 	int (*chp_event)(struct subchannel *, struct chp_link *, int);
 	int (*sch_event)(struct subchannel *, int);
 	int (*probe)(struct subchannel *);
-	int (*remove)(struct subchannel *);
+	void (*remove)(struct subchannel *);
 	void (*shutdown)(struct subchannel *);
 	int (*settle)(void);
 };
diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index 84f659cafe76..cd5d2d4d8e46 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -137,7 +137,7 @@ static int ccw_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 static void io_subchannel_irq(struct subchannel *);
 static int io_subchannel_probe(struct subchannel *);
-static int io_subchannel_remove(struct subchannel *);
+static void io_subchannel_remove(struct subchannel *);
 static void io_subchannel_shutdown(struct subchannel *);
 static int io_subchannel_sch_event(struct subchannel *, int);
 static int io_subchannel_chp_event(struct subchannel *, struct chp_link *,
@@ -1101,7 +1101,7 @@ static int io_subchannel_probe(struct subchannel *sch)
 	return 0;
 }
 
-static int io_subchannel_remove(struct subchannel *sch)
+static void io_subchannel_remove(struct subchannel *sch)
 {
 	struct io_subchannel_private *io_priv = to_io_private(sch);
 	struct ccw_device *cdev;
@@ -1120,7 +1120,6 @@ static int io_subchannel_remove(struct subchannel *sch)
 			  io_priv->dma_area, io_priv->dma_area_dma);
 	kfree(io_priv);
 	sysfs_remove_group(&sch->dev.kobj, &io_subchannel_attr_group);
-	return 0;
 }
 
 static void io_subchannel_verify(struct subchannel *sch)
diff --git a/drivers/s390/cio/eadm_sch.c b/drivers/s390/cio/eadm_sch.c
index c8964e0a23e7..15bdae5981ca 100644
--- a/drivers/s390/cio/eadm_sch.c
+++ b/drivers/s390/cio/eadm_sch.c
@@ -282,7 +282,7 @@ static void eadm_quiesce(struct subchannel *sch)
 	spin_unlock_irq(sch->lock);
 }
 
-static int eadm_subchannel_remove(struct subchannel *sch)
+static void eadm_subchannel_remove(struct subchannel *sch)
 {
 	struct eadm_private *private = get_eadm_private(sch);
 
@@ -297,8 +297,6 @@ static int eadm_subchannel_remove(struct subchannel *sch)
 	spin_unlock_irq(sch->lock);
 
 	kfree(private);
-
-	return 0;
 }
 
 static void eadm_subchannel_shutdown(struct subchannel *sch)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 9b61e9b131ad..76099bcb765b 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -234,7 +234,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	return ret;
 }
 
-static int vfio_ccw_sch_remove(struct subchannel *sch)
+static void vfio_ccw_sch_remove(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 	struct vfio_ccw_crw *crw, *temp;
@@ -257,7 +257,6 @@ static int vfio_ccw_sch_remove(struct subchannel *sch)
 	VFIO_CCW_MSG_EVENT(4, "unbound from subchannel %x.%x.%04x\n",
 			   sch->schid.cssid, sch->schid.ssid,
 			   sch->schid.sch_no);
-	return 0;
 }
 
 static void vfio_ccw_sch_shutdown(struct subchannel *sch)
-- 
2.30.2

Re: [PATCH v2 1/4] s390/cio: Make struct css_driver::remove return void

[Cornelia Huck]

On Tue, Jul 06 2021, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> The driver core ignores the return value of css_remove()
> (because there is only little it can do when a device disappears) and
> there are no pci_epf_drivers with a remove callback.

s/pci_epf/css/

>
> So make it impossible for future drivers to return an unused error code
> by changing the remove prototype to return void.
>
> The real motivation for this change is the quest to make struct
> bus_type::remove return void, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/s390/cio/chsc_sch.c     | 3 +--
>  drivers/s390/cio/css.c          | 7 ++++---
>  drivers/s390/cio/css.h          | 2 +-
>  drivers/s390/cio/device.c       | 5 ++---
>  drivers/s390/cio/eadm_sch.c     | 4 +---
>  drivers/s390/cio/vfio_ccw_drv.c | 3 +--
>  6 files changed, 10 insertions(+), 14 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v2 1/4] s390/cio: Make struct css_driver::remove return void

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

On Tue, Jul 06, 2021 at 05:58:25PM +0200, Cornelia Huck wrote:
> On Tue, Jul 06 2021, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > The driver core ignores the return value of css_remove()
> > (because there is only little it can do when a device disappears) and
> > there are no pci_epf_drivers with a remove callback.
> 
> s/pci_epf/css/

Argh, too much copy&paste. I make this:

	The driver core ignores the return value of css_remove()
	(because there is only little it can do when a device
	disappears) and all callbacks return 0 anyhow.

to make this actually correct.

> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] s390/cio: Make struct css_driver::remove return void

[Vineeth Vijayan]

Thank you. I will use the modified description. This will be picked up 
by Vasily/Heiko to the s390-tree.

Also Acked-by: Vineeth Vijayan <vneethv@linux.ibm.com>

One question, is this patchset supposed to have 4 patches ? Are we 
missing one ?

On 7/6/21 6:05 PM, Uwe Kleine-König wrote:
> Argh, too much copy&paste. I make this:
>
> 	The driver core ignores the return value of css_remove()
> 	(because there is only little it can do when a device
> 	disappears) and all callbacks return 0 anyhow.
>
> to make this actually correct.
>
>> Reviewed-by: Cornelia Huck<cohuck@redhat.com>

Re: [PATCH v2 1/4] s390/cio: Make struct css_driver::remove return void

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

Hello Vineeth,

On Wed, Jul 07, 2021 at 01:28:11PM +0200, Vineeth Vijayan wrote:
> Thank you. I will use the modified description. This will be picked up by
> Vasily/Heiko to the s390-tree.
> 
> Also Acked-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> 
> One question, is this patchset supposed to have 4 patches ? Are we missing
> one ?

Yes, the fourth patch[1] has the following shortstat:

	80 files changed, 83 insertions(+), 219 deletions(-)

and the affected files are distributed over the whole source tree.

Given that this fourth patch is the actual motivation for the first
three, and I'd like to get this in during the next merge window, I would
prefer if these patches were taken together. (Well unless the first
three make it into 5.14-rc1 of course.)

Best regards
Uwe

[1] https://lore.kernel.org/lkml/20210706154803.1631813-5-u.kleine-koenig@pengutronix.de/
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] s390/cio: Make struct css_driver::remove return void

[Heiko Carstens]

On Wed, Jul 07, 2021 at 04:34:31PM +0200, Uwe Kleine-König wrote:
> Hello Vineeth,
> 
> On Wed, Jul 07, 2021 at 01:28:11PM +0200, Vineeth Vijayan wrote:
> > Thank you. I will use the modified description. This will be picked up by
> > Vasily/Heiko to the s390-tree.
> > 
> > Also Acked-by: Vineeth Vijayan <vneethv@linux.ibm.com>
> > 
> > One question, is this patchset supposed to have 4 patches ? Are we missing
> > one ?
> 
> Yes, the fourth patch[1] has the following shortstat:
> 
> 	80 files changed, 83 insertions(+), 219 deletions(-)
> 
> and the affected files are distributed over the whole source tree.
> 
> Given that this fourth patch is the actual motivation for the first
> three, and I'd like to get this in during the next merge window, I would
> prefer if these patches were taken together. (Well unless the first
> three make it into 5.14-rc1 of course.)
> 
> Best regards
> Uwe
> 
> [1] https://lore.kernel.org/lkml/20210706154803.1631813-5-u.kleine-koenig@pengutronix.de/

In this case I think Greg should pick up all four patches.

FWIW: it's usually also not very helpful to cc people only on parts of
a patch series and let them figure out the bigger picture.

[PATCH v2 2/4] s390/ccwgroup: Drop if with an always false condition

[Uwe Kleine-König]

The driver core only calls a bus remove callback when there is a driver.
So dev->driver is never NULL and the check can safely be removed.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/s390/cio/ccwgroup.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/s390/cio/ccwgroup.c b/drivers/s390/cio/ccwgroup.c
index 9748165e08e9..a6aeab1ea0ae 100644
--- a/drivers/s390/cio/ccwgroup.c
+++ b/drivers/s390/cio/ccwgroup.c
@@ -444,8 +444,6 @@ static int ccwgroup_remove(struct device *dev)
 	struct ccwgroup_device *gdev = to_ccwgroupdev(dev);
 	struct ccwgroup_driver *gdrv = to_ccwgroupdrv(dev->driver);
 
-	if (!dev->driver)
-		return 0;
 	if (gdrv->remove)
 		gdrv->remove(gdev);
 
-- 
2.30.2

Re: [PATCH v2 2/4] s390/ccwgroup: Drop if with an always false condition

[Cornelia Huck]

On Tue, Jul 06 2021, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> The driver core only calls a bus remove callback when there is a driver.
> So dev->driver is never NULL and the check can safely be removed.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/s390/cio/ccwgroup.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v2 2/4] s390/ccwgroup: Drop if with an always false condition

[Vineeth Vijayan]

Thank you. Looks good to me.
Heiko/Vasily will pick this up and will be part of the next s390-tree 
patchset.

Also,

Acked-by: Vineeth Vijayan <vneethv@linux.ibm.com>


On 7/6/21 5:48 PM, Uwe Kleine-König wrote:
> The driver core only calls a bus remove callback when there is a driver.
> So dev->driver is never NULL and the check can safely be removed.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   drivers/s390/cio/ccwgroup.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/s390/cio/ccwgroup.c b/drivers/s390/cio/ccwgroup.c
> index 9748165e08e9..a6aeab1ea0ae 100644
> --- a/drivers/s390/cio/ccwgroup.c
> +++ b/drivers/s390/cio/ccwgroup.c
> @@ -444,8 +444,6 @@ static int ccwgroup_remove(struct device *dev)
>   	struct ccwgroup_device *gdev = to_ccwgroupdev(dev);
>   	struct ccwgroup_driver *gdrv = to_ccwgroupdrv(dev->driver);
>   
> -	if (!dev->driver)
> -		return 0;
>   	if (gdrv->remove)
>   		gdrv->remove(gdev);
>   

[PATCH v2 3/4] s390/scm: Make struct scm_driver::remove return void

[Uwe Kleine-König]

The driver core ignores the return value of scmdev_remove()
(because there is only little it can do when a device disappears).

So make it impossible for future drivers to return an unused error code
by changing the remove prototype to return void.

The real motivation for this change is the quest to make struct
bus_type::remove return void, too.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/s390/include/asm/eadm.h | 2 +-
 drivers/s390/block/scm_drv.c | 4 +---
 drivers/s390/cio/scm.c       | 5 ++++-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/s390/include/asm/eadm.h b/arch/s390/include/asm/eadm.h
index bb63b2afdf6f..445fe4c8184a 100644
--- a/arch/s390/include/asm/eadm.h
+++ b/arch/s390/include/asm/eadm.h
@@ -105,7 +105,7 @@ enum scm_event {SCM_CHANGE, SCM_AVAIL};
 struct scm_driver {
 	struct device_driver drv;
 	int (*probe) (struct scm_device *scmdev);
-	int (*remove) (struct scm_device *scmdev);
+	void (*remove) (struct scm_device *scmdev);
 	void (*notify) (struct scm_device *scmdev, enum scm_event event);
 	void (*handler) (struct scm_device *scmdev, void *data,
 			blk_status_t error);
diff --git a/drivers/s390/block/scm_drv.c b/drivers/s390/block/scm_drv.c
index 3134fd6e058e..69a845eb8b1f 100644
--- a/drivers/s390/block/scm_drv.c
+++ b/drivers/s390/block/scm_drv.c
@@ -60,15 +60,13 @@ static int scm_probe(struct scm_device *scmdev)
 	return ret;
 }
 
-static int scm_remove(struct scm_device *scmdev)
+static void scm_remove(struct scm_device *scmdev)
 {
 	struct scm_blk_dev *bdev = dev_get_drvdata(&scmdev->dev);
 
 	scm_blk_dev_cleanup(bdev);
 	dev_set_drvdata(&scmdev->dev, NULL);
 	kfree(bdev);
-
-	return 0;
 }
 
 static struct scm_driver scm_drv = {
diff --git a/drivers/s390/cio/scm.c b/drivers/s390/cio/scm.c
index 9f26d4310bb3..b31711307e5a 100644
--- a/drivers/s390/cio/scm.c
+++ b/drivers/s390/cio/scm.c
@@ -33,7 +33,10 @@ static int scmdev_remove(struct device *dev)
 	struct scm_device *scmdev = to_scm_dev(dev);
 	struct scm_driver *scmdrv = to_scm_drv(dev->driver);
 
-	return scmdrv->remove ? scmdrv->remove(scmdev) : -ENODEV;
+	if (scmdrv->remove)
+		scmdrv->remove(scmdev);
+
+	return 0;
 }
 
 static int scmdev_uevent(struct device *dev, struct kobj_uevent_env *env)
-- 
2.30.2

Re: [PATCH v2 3/4] s390/scm: Make struct scm_driver::remove return void

[Cornelia Huck]

On Tue, Jul 06 2021, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> The driver core ignores the return value of scmdev_remove()
> (because there is only little it can do when a device disappears).
>
> So make it impossible for future drivers to return an unused error code
> by changing the remove prototype to return void.
>
> The real motivation for this change is the quest to make struct
> bus_type::remove return void, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  arch/s390/include/asm/eadm.h | 2 +-
>  drivers/s390/block/scm_drv.c | 4 +---
>  drivers/s390/cio/scm.c       | 5 ++++-
>  3 files changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v2 3/4] s390/scm: Make struct scm_driver::remove return void

[Vineeth Vijayan]

Looks sane to me.
Acked-by: Vineeth Vijayan <vneethv@linux.ibm.com>

Vasily/Heiko will pick this up for the next-s390-tree.


On 7/6/21 5:48 PM, Uwe Kleine-König wrote:
> The driver core ignores the return value of scmdev_remove()
> (because there is only little it can do when a device disappears).
>
> So make it impossible for future drivers to return an unused error code
> by changing the remove prototype to return void.
>
> The real motivation for this change is the quest to make struct
> bus_type::remove return void, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>   arch/s390/include/asm/eadm.h | 2 +-
>   drivers/s390/block/scm_drv.c | 4 +---
>   drivers/s390/cio/scm.c       | 5 ++++-
>   3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/s390/include/asm/eadm.h b/arch/s390/include/asm/eadm.h
> index bb63b2afdf6f..445fe4c8184a 100644
> --- a/arch/s390/include/asm/eadm.h
> +++ b/arch/s390/include/asm/eadm.h
> @@ -105,7 +105,7 @@ enum scm_event {SCM_CHANGE, SCM_AVAIL};
>   struct scm_driver {
>   	struct device_driver drv;
>   	int (*probe) (struct scm_device *scmdev);
> -	int (*remove) (struct scm_device *scmdev);
> +	void (*remove) (struct scm_device *scmdev);
>   	void (*notify) (struct scm_device *scmdev, enum scm_event event);
>   	void (*handler) (struct scm_device *scmdev, void *data,
>   			blk_status_t error);
> diff --git a/drivers/s390/block/scm_drv.c b/drivers/s390/block/scm_drv.c
> index 3134fd6e058e..69a845eb8b1f 100644
> --- a/drivers/s390/block/scm_drv.c
> +++ b/drivers/s390/block/scm_drv.c
> @@ -60,15 +60,13 @@ static int scm_probe(struct scm_device *scmdev)
>   	return ret;
>   }
>   
> -static int scm_remove(struct scm_device *scmdev)
> +static void scm_remove(struct scm_device *scmdev)
>   {
>   	struct scm_blk_dev *bdev = dev_get_drvdata(&scmdev->dev);
>   
>   	scm_blk_dev_cleanup(bdev);
>   	dev_set_drvdata(&scmdev->dev, NULL);
>   	kfree(bdev);
> -
> -	return 0;
>   }
>   
>   static struct scm_driver scm_drv = {
> diff --git a/drivers/s390/cio/scm.c b/drivers/s390/cio/scm.c
> index 9f26d4310bb3..b31711307e5a 100644
> --- a/drivers/s390/cio/scm.c
> +++ b/drivers/s390/cio/scm.c
> @@ -33,7 +33,10 @@ static int scmdev_remove(struct device *dev)
>   	struct scm_device *scmdev = to_scm_dev(dev);
>   	struct scm_driver *scmdrv = to_scm_drv(dev->driver);
>   
> -	return scmdrv->remove ? scmdrv->remove(scmdev) : -ENODEV;
> +	if (scmdrv->remove)
> +		scmdrv->remove(scmdev);
> +
> +	return 0;
>   }
>   
>   static int scmdev_uevent(struct device *dev, struct kobj_uevent_env *env)