[PATCH 1/2] ALSA: ppc: drop if block with always false condition

[PATCH 1/2] ALSA: ppc: drop if block with always false condition

[Uwe Kleine-König]

The remove callback is only called for devices that were probed
successfully before. As the matching probe function cannot complete
without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to
check this here.

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

diff --git a/sound/ppc/snd_ps3.c b/sound/ppc/snd_ps3.c
index 58bb49fff184..6ab796a5d936 100644
--- a/sound/ppc/snd_ps3.c
+++ b/sound/ppc/snd_ps3.c
@@ -1053,8 +1053,6 @@ static int snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
 {
 	int ret;
 	pr_info("%s:start id=%d\n", __func__,  dev->match_id);
-	if (dev->match_id != PS3_MATCH_ID_SOUND)
-		return -ENXIO;
 
 	/*
 	 * ctl and preallocate buffer will be freed in
-- 
2.29.2

[PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Uwe Kleine-König]

The driver core ignores the return value of struct device_driver::remove
because there is only little that can be done. For the shutdown callback
it's ps3_system_bus_shutdown() which ignores the return value.

To simplify the quest to make struct device_driver::remove return void,
let struct ps3_system_bus_driver::remove return void, too. All users
already unconditionally return 0, this commit makes it obvious that
returning an error code is a bad idea and ensures future users behave
accordingly.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/powerpc/include/asm/ps3.h               |  4 ++--
 arch/powerpc/platforms/ps3/system-bus.c      |  5 ++---
 drivers/block/ps3disk.c                      |  3 +--
 drivers/block/ps3vram.c                      |  3 +--
 drivers/char/ps3flash.c                      |  3 +--
 drivers/net/ethernet/toshiba/ps3_gelic_net.c |  3 +--
 drivers/ps3/ps3-lpm.c                        |  3 +--
 drivers/ps3/ps3-vuart.c                      | 10 ++++------
 drivers/scsi/ps3rom.c                        |  3 +--
 drivers/usb/host/ehci-ps3.c                  |  4 +---
 drivers/usb/host/ohci-ps3.c                  |  4 +---
 drivers/video/fbdev/ps3fb.c                  |  4 +---
 sound/ppc/snd_ps3.c                          |  3 +--
 13 files changed, 18 insertions(+), 34 deletions(-)

diff --git a/arch/powerpc/include/asm/ps3.h b/arch/powerpc/include/asm/ps3.h
index cb89e4bf55ce..e646c7f218bc 100644
--- a/arch/powerpc/include/asm/ps3.h
+++ b/arch/powerpc/include/asm/ps3.h
@@ -378,8 +378,8 @@ struct ps3_system_bus_driver {
 	enum ps3_match_sub_id match_sub_id;
 	struct device_driver core;
 	int (*probe)(struct ps3_system_bus_device *);
-	int (*remove)(struct ps3_system_bus_device *);
-	int (*shutdown)(struct ps3_system_bus_device *);
+	void (*remove)(struct ps3_system_bus_device *);
+	void (*shutdown)(struct ps3_system_bus_device *);
 /*	int (*suspend)(struct ps3_system_bus_device *, pm_message_t); */
 /*	int (*resume)(struct ps3_system_bus_device *); */
 };
diff --git a/arch/powerpc/platforms/ps3/system-bus.c b/arch/powerpc/platforms/ps3/system-bus.c
index c62aaa29a9d5..b431f41c6cb5 100644
--- a/arch/powerpc/platforms/ps3/system-bus.c
+++ b/arch/powerpc/platforms/ps3/system-bus.c
@@ -382,7 +382,6 @@ static int ps3_system_bus_probe(struct device *_dev)
 
 static int ps3_system_bus_remove(struct device *_dev)
 {
-	int result = 0;
 	struct ps3_system_bus_device *dev = ps3_dev_to_system_bus_dev(_dev);
 	struct ps3_system_bus_driver *drv;
 
@@ -393,13 +392,13 @@ static int ps3_system_bus_remove(struct device *_dev)
 	BUG_ON(!drv);
 
 	if (drv->remove)
-		result = drv->remove(dev);
+		drv->remove(dev);
 	else
 		dev_dbg(&dev->core, "%s:%d %s: no remove method\n",
 			__func__, __LINE__, drv->core.name);
 
 	pr_debug(" <- %s:%d: %s\n", __func__, __LINE__, dev_name(&dev->core));
-	return result;
+	return 0;
 }
 
 static void ps3_system_bus_shutdown(struct device *_dev)
diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
index 7b55811c2a81..ba3ece56cbb3 100644
--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -507,7 +507,7 @@ static int ps3disk_probe(struct ps3_system_bus_device *_dev)
 	return error;
 }
 
-static int ps3disk_remove(struct ps3_system_bus_device *_dev)
+static void ps3disk_remove(struct ps3_system_bus_device *_dev)
 {
 	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
 	struct ps3disk_private *priv = ps3_system_bus_get_drvdata(&dev->sbd);
@@ -526,7 +526,6 @@ static int ps3disk_remove(struct ps3_system_bus_device *_dev)
 	kfree(dev->bounce_buf);
 	kfree(priv);
 	ps3_system_bus_set_drvdata(_dev, NULL);
-	return 0;
 }
 
 static struct ps3_system_bus_driver ps3disk = {
diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c
index 1088798c8dd0..b71d28372ef3 100644
--- a/drivers/block/ps3vram.c
+++ b/drivers/block/ps3vram.c
@@ -797,7 +797,7 @@ static int ps3vram_probe(struct ps3_system_bus_device *dev)
 	return error;
 }
 
-static int ps3vram_remove(struct ps3_system_bus_device *dev)
+static void ps3vram_remove(struct ps3_system_bus_device *dev)
 {
 	struct ps3vram_priv *priv = ps3_system_bus_get_drvdata(dev);
 
@@ -817,7 +817,6 @@ static int ps3vram_remove(struct ps3_system_bus_device *dev)
 	free_pages((unsigned long) priv->xdr_buf, get_order(XDR_BUF_SIZE));
 	kfree(priv);
 	ps3_system_bus_set_drvdata(dev, NULL);
-	return 0;
 }
 
 static struct ps3_system_bus_driver ps3vram = {
diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c
index 1a07fee33f66..23871cde41fb 100644
--- a/drivers/char/ps3flash.c
+++ b/drivers/char/ps3flash.c
@@ -403,7 +403,7 @@ static int ps3flash_probe(struct ps3_system_bus_device *_dev)
 	return error;
 }
 
-static int ps3flash_remove(struct ps3_system_bus_device *_dev)
+static void ps3flash_remove(struct ps3_system_bus_device *_dev)
 {
 	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
 
@@ -413,7 +413,6 @@ static int ps3flash_remove(struct ps3_system_bus_device *_dev)
 	kfree(ps3_system_bus_get_drvdata(&dev->sbd));
 	ps3_system_bus_set_drvdata(&dev->sbd, NULL);
 	ps3flash_dev = NULL;
-	return 0;
 }
 
 
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index d9a5722f561b..3d1fc8d2ca66 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -1791,7 +1791,7 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
  * ps3_gelic_driver_remove - remove a device from the control of this driver
  */
 
-static int ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
+static void ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
 {
 	struct gelic_card *card = ps3_system_bus_get_drvdata(dev);
 	struct net_device *netdev0;
@@ -1840,7 +1840,6 @@ static int ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
 	ps3_close_hv_device(dev);
 
 	pr_debug("%s: done\n", __func__);
-	return 0;
 }
 
 static struct ps3_system_bus_driver ps3_gelic_driver = {
diff --git a/drivers/ps3/ps3-lpm.c b/drivers/ps3/ps3-lpm.c
index e54aa2d82f50..65512b6cc6fd 100644
--- a/drivers/ps3/ps3-lpm.c
+++ b/drivers/ps3/ps3-lpm.c
@@ -1196,7 +1196,7 @@ static int ps3_lpm_probe(struct ps3_system_bus_device *dev)
 	return 0;
 }
 
-static int ps3_lpm_remove(struct ps3_system_bus_device *dev)
+static void ps3_lpm_remove(struct ps3_system_bus_device *dev)
 {
 	dev_dbg(&dev->core, " -> %s:%u:\n", __func__, __LINE__);
 
@@ -1206,7 +1206,6 @@ static int ps3_lpm_remove(struct ps3_system_bus_device *dev)
 	lpm_priv = NULL;
 
 	dev_info(&dev->core, " <- %s:%u:\n", __func__, __LINE__);
-	return 0;
 }
 
 static struct ps3_system_bus_driver ps3_lpm_driver = {
diff --git a/drivers/ps3/ps3-vuart.c b/drivers/ps3/ps3-vuart.c
index 4ed131eaff51..e34ae6a442c7 100644
--- a/drivers/ps3/ps3-vuart.c
+++ b/drivers/ps3/ps3-vuart.c
@@ -1102,7 +1102,7 @@ static int ps3_vuart_cleanup(struct ps3_system_bus_device *dev)
  * device can no longer be used.
  */
 
-static int ps3_vuart_remove(struct ps3_system_bus_device *dev)
+static void ps3_vuart_remove(struct ps3_system_bus_device *dev)
 {
 	struct ps3_vuart_port_priv *priv = to_port_priv(dev);
 	struct ps3_vuart_port_driver *drv;
@@ -1118,7 +1118,7 @@ static int ps3_vuart_remove(struct ps3_system_bus_device *dev)
 		dev_dbg(&dev->core, "%s:%d: no driver bound\n", __func__,
 			__LINE__);
 		mutex_unlock(&vuart_bus_priv.probe_mutex);
-		return 0;
+		return;
 	}
 
 	drv = ps3_system_bus_dev_to_vuart_drv(dev);
@@ -1141,7 +1141,6 @@ static int ps3_vuart_remove(struct ps3_system_bus_device *dev)
 
 	dev_dbg(&dev->core, " <- %s:%d\n", __func__, __LINE__);
 	mutex_unlock(&vuart_bus_priv.probe_mutex);
-	return 0;
 }
 
 /**
@@ -1154,7 +1153,7 @@ static int ps3_vuart_remove(struct ps3_system_bus_device *dev)
  * sequence.
  */
 
-static int ps3_vuart_shutdown(struct ps3_system_bus_device *dev)
+static void ps3_vuart_shutdown(struct ps3_system_bus_device *dev)
 {
 	struct ps3_vuart_port_driver *drv;
 
@@ -1169,7 +1168,7 @@ static int ps3_vuart_shutdown(struct ps3_system_bus_device *dev)
 		dev_dbg(&dev->core, "%s:%d: no driver bound\n", __func__,
 			__LINE__);
 		mutex_unlock(&vuart_bus_priv.probe_mutex);
-		return 0;
+		return;
 	}
 
 	drv = ps3_system_bus_dev_to_vuart_drv(dev);
@@ -1193,7 +1192,6 @@ static int ps3_vuart_shutdown(struct ps3_system_bus_device *dev)
 	dev_dbg(&dev->core, " <- %s:%d\n", __func__, __LINE__);
 
 	mutex_unlock(&vuart_bus_priv.probe_mutex);
-	return 0;
 }
 
 static int __init ps3_vuart_bus_init(void)
diff --git a/drivers/scsi/ps3rom.c b/drivers/scsi/ps3rom.c
index f75c0b5cd587..ccb5771f1cb7 100644
--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -402,7 +402,7 @@ static int ps3rom_probe(struct ps3_system_bus_device *_dev)
 	return error;
 }
 
-static int ps3rom_remove(struct ps3_system_bus_device *_dev)
+static void ps3rom_remove(struct ps3_system_bus_device *_dev)
 {
 	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
 	struct Scsi_Host *host = ps3_system_bus_get_drvdata(&dev->sbd);
@@ -412,7 +412,6 @@ static int ps3rom_remove(struct ps3_system_bus_device *_dev)
 	scsi_host_put(host);
 	ps3_system_bus_set_drvdata(&dev->sbd, NULL);
 	kfree(dev->bounce_buf);
-	return 0;
 }
 
 static struct ps3_system_bus_driver ps3rom = {
diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
index fb52133c3557..98568b046a1a 100644
--- a/drivers/usb/host/ehci-ps3.c
+++ b/drivers/usb/host/ehci-ps3.c
@@ -200,7 +200,7 @@ static int ps3_ehci_probe(struct ps3_system_bus_device *dev)
 	return result;
 }
 
-static int ps3_ehci_remove(struct ps3_system_bus_device *dev)
+static void ps3_ehci_remove(struct ps3_system_bus_device *dev)
 {
 	unsigned int tmp;
 	struct usb_hcd *hcd = ps3_system_bus_get_drvdata(dev);
@@ -227,8 +227,6 @@ static int ps3_ehci_remove(struct ps3_system_bus_device *dev)
 
 	ps3_dma_region_free(dev->d_region);
 	ps3_close_hv_device(dev);
-
-	return 0;
 }
 
 static int __init ps3_ehci_driver_register(struct ps3_system_bus_driver *drv)
diff --git a/drivers/usb/host/ohci-ps3.c b/drivers/usb/host/ohci-ps3.c
index f77cd6af0ccf..4f5af929c3e4 100644
--- a/drivers/usb/host/ohci-ps3.c
+++ b/drivers/usb/host/ohci-ps3.c
@@ -184,7 +184,7 @@ static int ps3_ohci_probe(struct ps3_system_bus_device *dev)
 	return result;
 }
 
-static int ps3_ohci_remove(struct ps3_system_bus_device *dev)
+static void ps3_ohci_remove(struct ps3_system_bus_device *dev)
 {
 	unsigned int tmp;
 	struct usb_hcd *hcd = ps3_system_bus_get_drvdata(dev);
@@ -212,8 +212,6 @@ static int ps3_ohci_remove(struct ps3_system_bus_device *dev)
 
 	ps3_dma_region_free(dev->d_region);
 	ps3_close_hv_device(dev);
-
-	return 0;
 }
 
 static int __init ps3_ohci_driver_register(struct ps3_system_bus_driver *drv)
diff --git a/drivers/video/fbdev/ps3fb.c b/drivers/video/fbdev/ps3fb.c
index 203c254f8f6c..2fe08b67eda7 100644
--- a/drivers/video/fbdev/ps3fb.c
+++ b/drivers/video/fbdev/ps3fb.c
@@ -1208,7 +1208,7 @@ static int ps3fb_probe(struct ps3_system_bus_device *dev)
 	return retval;
 }
 
-static int ps3fb_shutdown(struct ps3_system_bus_device *dev)
+static void ps3fb_shutdown(struct ps3_system_bus_device *dev)
 {
 	struct fb_info *info = ps3_system_bus_get_drvdata(dev);
 	u64 xdr_lpar = ps3_mm_phys_to_lpar(__pa(ps3fb_videomemory.address));
@@ -1241,8 +1241,6 @@ static int ps3fb_shutdown(struct ps3_system_bus_device *dev)
 	lv1_gpu_memory_free(ps3fb.memory_handle);
 	ps3_close_hv_device(dev);
 	dev_dbg(&dev->core, " <- %s:%d\n", __func__, __LINE__);
-
-	return 0;
 }
 
 static struct ps3_system_bus_driver ps3fb_driver = {
diff --git a/sound/ppc/snd_ps3.c b/sound/ppc/snd_ps3.c
index 6ab796a5d936..8e44fa5d4dc7 100644
--- a/sound/ppc/snd_ps3.c
+++ b/sound/ppc/snd_ps3.c
@@ -1049,7 +1049,7 @@ static int snd_ps3_driver_probe(struct ps3_system_bus_device *dev)
 }; /* snd_ps3_probe */
 
 /* called when module removal */
-static int snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
+static void snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
 {
 	int ret;
 	pr_info("%s:start id=%d\n", __func__,  dev->match_id);
@@ -1075,7 +1075,6 @@ static int snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
 	lv1_gpu_device_unmap(2);
 	ps3_close_hv_device(dev);
 	pr_info("%s:end id=%d\n", __func__, dev->match_id);
-	return 0;
 } /* snd_ps3_remove */
 
 static struct ps3_system_bus_driver snd_ps3_bus_driver_info = {
-- 
2.29.2

Re: [PATCH 1/2] ALSA: ppc: drop if block with always false condition

[Geert Uytterhoeven]

Hi Uwe,

On Thu, Nov 26, 2020 at 6:03 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> The remove callback is only called for devices that were probed
> successfully before. As the matching probe function cannot complete
> without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to
> check this here.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Note that there are similar checks in snd_ps3_driver_probe(), which
can be removed, too:

        if (WARN_ON(!firmware_has_feature(FW_FEATURE_PS3_LV1)))
                return -ENODEV;
        if (WARN_ON(dev->match_id != PS3_MATCH_ID_SOUND))
                return -ENODEV;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Geert Uytterhoeven]

Hi Uwe,

On Thu, Nov 26, 2020 at 6:03 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> The driver core ignores the return value of struct device_driver::remove
> because there is only little that can be done. For the shutdown callback
> it's ps3_system_bus_shutdown() which ignores the return value.
>
> To simplify the quest to make struct device_driver::remove return void,
> let struct ps3_system_bus_driver::remove return void, too. All users
> already unconditionally return 0, this commit makes it obvious that
> returning an error code is a bad idea and ensures future users behave
> accordingly.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

Note that the same can be done for ps3_vuart_port_driver.remove().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] ALSA: ppc: drop if block with always false condition

[Uwe Kleine-König]

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

On Fri, Nov 27, 2020 at 09:35:39AM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
> 
> On Thu, Nov 26, 2020 at 6:03 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > The remove callback is only called for devices that were probed
> > successfully before. As the matching probe function cannot complete
> > without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to
> > check this here.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Thanks for your patch!
> 
> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> Note that there are similar checks in snd_ps3_driver_probe(), which
> can be removed, too:
> 
>         if (WARN_ON(!firmware_has_feature(FW_FEATURE_PS3_LV1)))
>                 return -ENODEV;
>         if (WARN_ON(dev->match_id != PS3_MATCH_ID_SOUND))
>                 return -ENODEV;

I had to invest some brain cycles here. For the first:

Assuming firmware_has_feature(FW_FEATURE_PS3_LV1) always returns the
same value, snd_ps3_driver_probe is only used after this check succeeds
because the driver is registered only after this check in
snd_ps3_init().

The second is superflous because ps3_system_bus_match() yields false if
this doesn't match the driver's match_id.

Best regards
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 1/2] ALSA: ppc: drop if block with always false condition

[Geoff Levand]

Hi Uwe,

On 11/26/20 8:59 AM, Uwe Kleine-König wrote:
> The remove callback is only called for devices that were probed
> successfully before. As the matching probe function cannot complete
> without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to
> check this here.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I tested your two patches plus Leonard's patch 'ALSA: ppc: remove
redundant checks in PS3 driver probe' applied to v5.9 on the PS3,
and they seem to work fine.

Thanks for both your efforts.

Tested by: Geoff Levand <geoff@infradead.org>

Re: [PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Geoff Levand]

On 11/26/20 8:59 AM, Uwe Kleine-König wrote:
> The driver core ignores the return value of struct device_driver::remove
> because there is only little that can be done. For the shutdown callback
> it's ps3_system_bus_shutdown() which ignores the return value.
> 
> To simplify the quest to make struct device_driver::remove return void,
> let struct ps3_system_bus_driver::remove return void, too. All users
> already unconditionally return 0, this commit makes it obvious that
> returning an error code is a bad idea and ensures future users behave
> accordingly.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Seems OK with v5.9 on PS3.

Tested by: Geoff Levand <geoff@infradead.org>

Re: [PATCH 1/2] ALSA: ppc: drop if block with always false condition

[Takashi Iwai]

On Thu, 26 Nov 2020 17:59:49 +0100,
Uwe Kleine-König wrote:
> 
> The remove callback is only called for devices that were probed
> successfully before. As the matching probe function cannot complete
> without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to
> check this here.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied this one now.  Thanks.


Takashi

Re: [PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Takashi Iwai]

On Thu, 26 Nov 2020 17:59:50 +0100,
Uwe Kleine-König wrote:
> 
> The driver core ignores the return value of struct device_driver::remove
> because there is only little that can be done. For the shutdown callback
> it's ps3_system_bus_shutdown() which ignores the return value.
> 
> To simplify the quest to make struct device_driver::remove return void,
> let struct ps3_system_bus_driver::remove return void, too. All users
> already unconditionally return 0, this commit makes it obvious that
> returning an error code is a bad idea and ensures future users behave
> accordingly.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

For the sound bit:
Acked-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> ---
>  arch/powerpc/include/asm/ps3.h               |  4 ++--
>  arch/powerpc/platforms/ps3/system-bus.c      |  5 ++---
>  drivers/block/ps3disk.c                      |  3 +--
>  drivers/block/ps3vram.c                      |  3 +--
>  drivers/char/ps3flash.c                      |  3 +--
>  drivers/net/ethernet/toshiba/ps3_gelic_net.c |  3 +--
>  drivers/ps3/ps3-lpm.c                        |  3 +--
>  drivers/ps3/ps3-vuart.c                      | 10 ++++------
>  drivers/scsi/ps3rom.c                        |  3 +--
>  drivers/usb/host/ehci-ps3.c                  |  4 +---
>  drivers/usb/host/ohci-ps3.c                  |  4 +---
>  drivers/video/fbdev/ps3fb.c                  |  4 +---
>  sound/ppc/snd_ps3.c                          |  3 +--
>  13 files changed, 18 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/ps3.h b/arch/powerpc/include/asm/ps3.h
> index cb89e4bf55ce..e646c7f218bc 100644
> --- a/arch/powerpc/include/asm/ps3.h
> +++ b/arch/powerpc/include/asm/ps3.h
> @@ -378,8 +378,8 @@ struct ps3_system_bus_driver {
>  	enum ps3_match_sub_id match_sub_id;
>  	struct device_driver core;
>  	int (*probe)(struct ps3_system_bus_device *);
> -	int (*remove)(struct ps3_system_bus_device *);
> -	int (*shutdown)(struct ps3_system_bus_device *);
> +	void (*remove)(struct ps3_system_bus_device *);
> +	void (*shutdown)(struct ps3_system_bus_device *);
>  /*	int (*suspend)(struct ps3_system_bus_device *, pm_message_t); */
>  /*	int (*resume)(struct ps3_system_bus_device *); */
>  };
> diff --git a/arch/powerpc/platforms/ps3/system-bus.c b/arch/powerpc/platforms/ps3/system-bus.c
> index c62aaa29a9d5..b431f41c6cb5 100644
> --- a/arch/powerpc/platforms/ps3/system-bus.c
> +++ b/arch/powerpc/platforms/ps3/system-bus.c
> @@ -382,7 +382,6 @@ static int ps3_system_bus_probe(struct device *_dev)
>  
>  static int ps3_system_bus_remove(struct device *_dev)
>  {
> -	int result = 0;
>  	struct ps3_system_bus_device *dev = ps3_dev_to_system_bus_dev(_dev);
>  	struct ps3_system_bus_driver *drv;
>  
> @@ -393,13 +392,13 @@ static int ps3_system_bus_remove(struct device *_dev)
>  	BUG_ON(!drv);
>  
>  	if (drv->remove)
> -		result = drv->remove(dev);
> +		drv->remove(dev);
>  	else
>  		dev_dbg(&dev->core, "%s:%d %s: no remove method\n",
>  			__func__, __LINE__, drv->core.name);
>  
>  	pr_debug(" <- %s:%d: %s\n", __func__, __LINE__, dev_name(&dev->core));
> -	return result;
> +	return 0;
>  }
>  
>  static void ps3_system_bus_shutdown(struct device *_dev)
> diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
> index 7b55811c2a81..ba3ece56cbb3 100644
> --- a/drivers/block/ps3disk.c
> +++ b/drivers/block/ps3disk.c
> @@ -507,7 +507,7 @@ static int ps3disk_probe(struct ps3_system_bus_device *_dev)
>  	return error;
>  }
>  
> -static int ps3disk_remove(struct ps3_system_bus_device *_dev)
> +static void ps3disk_remove(struct ps3_system_bus_device *_dev)
>  {
>  	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
>  	struct ps3disk_private *priv = ps3_system_bus_get_drvdata(&dev->sbd);
> @@ -526,7 +526,6 @@ static int ps3disk_remove(struct ps3_system_bus_device *_dev)
>  	kfree(dev->bounce_buf);
>  	kfree(priv);
>  	ps3_system_bus_set_drvdata(_dev, NULL);
> -	return 0;
>  }
>  
>  static struct ps3_system_bus_driver ps3disk = {
> diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c
> index 1088798c8dd0..b71d28372ef3 100644
> --- a/drivers/block/ps3vram.c
> +++ b/drivers/block/ps3vram.c
> @@ -797,7 +797,7 @@ static int ps3vram_probe(struct ps3_system_bus_device *dev)
>  	return error;
>  }
>  
> -static int ps3vram_remove(struct ps3_system_bus_device *dev)
> +static void ps3vram_remove(struct ps3_system_bus_device *dev)
>  {
>  	struct ps3vram_priv *priv = ps3_system_bus_get_drvdata(dev);
>  
> @@ -817,7 +817,6 @@ static int ps3vram_remove(struct ps3_system_bus_device *dev)
>  	free_pages((unsigned long) priv->xdr_buf, get_order(XDR_BUF_SIZE));
>  	kfree(priv);
>  	ps3_system_bus_set_drvdata(dev, NULL);
> -	return 0;
>  }
>  
>  static struct ps3_system_bus_driver ps3vram = {
> diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c
> index 1a07fee33f66..23871cde41fb 100644
> --- a/drivers/char/ps3flash.c
> +++ b/drivers/char/ps3flash.c
> @@ -403,7 +403,7 @@ static int ps3flash_probe(struct ps3_system_bus_device *_dev)
>  	return error;
>  }
>  
> -static int ps3flash_remove(struct ps3_system_bus_device *_dev)
> +static void ps3flash_remove(struct ps3_system_bus_device *_dev)
>  {
>  	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
>  
> @@ -413,7 +413,6 @@ static int ps3flash_remove(struct ps3_system_bus_device *_dev)
>  	kfree(ps3_system_bus_get_drvdata(&dev->sbd));
>  	ps3_system_bus_set_drvdata(&dev->sbd, NULL);
>  	ps3flash_dev = NULL;
> -	return 0;
>  }
>  
>  
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index d9a5722f561b..3d1fc8d2ca66 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -1791,7 +1791,7 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
>   * ps3_gelic_driver_remove - remove a device from the control of this driver
>   */
>  
> -static int ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
> +static void ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
>  {
>  	struct gelic_card *card = ps3_system_bus_get_drvdata(dev);
>  	struct net_device *netdev0;
> @@ -1840,7 +1840,6 @@ static int ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
>  	ps3_close_hv_device(dev);
>  
>  	pr_debug("%s: done\n", __func__);
> -	return 0;
>  }
>  
>  static struct ps3_system_bus_driver ps3_gelic_driver = {
> diff --git a/drivers/ps3/ps3-lpm.c b/drivers/ps3/ps3-lpm.c
> index e54aa2d82f50..65512b6cc6fd 100644
> --- a/drivers/ps3/ps3-lpm.c
> +++ b/drivers/ps3/ps3-lpm.c
> @@ -1196,7 +1196,7 @@ static int ps3_lpm_probe(struct ps3_system_bus_device *dev)
>  	return 0;
>  }
>  
> -static int ps3_lpm_remove(struct ps3_system_bus_device *dev)
> +static void ps3_lpm_remove(struct ps3_system_bus_device *dev)
>  {
>  	dev_dbg(&dev->core, " -> %s:%u:\n", __func__, __LINE__);
>  
> @@ -1206,7 +1206,6 @@ static int ps3_lpm_remove(struct ps3_system_bus_device *dev)
>  	lpm_priv = NULL;
>  
>  	dev_info(&dev->core, " <- %s:%u:\n", __func__, __LINE__);
> -	return 0;
>  }
>  
>  static struct ps3_system_bus_driver ps3_lpm_driver = {
> diff --git a/drivers/ps3/ps3-vuart.c b/drivers/ps3/ps3-vuart.c
> index 4ed131eaff51..e34ae6a442c7 100644
> --- a/drivers/ps3/ps3-vuart.c
> +++ b/drivers/ps3/ps3-vuart.c
> @@ -1102,7 +1102,7 @@ static int ps3_vuart_cleanup(struct ps3_system_bus_device *dev)
>   * device can no longer be used.
>   */
>  
> -static int ps3_vuart_remove(struct ps3_system_bus_device *dev)
> +static void ps3_vuart_remove(struct ps3_system_bus_device *dev)
>  {
>  	struct ps3_vuart_port_priv *priv = to_port_priv(dev);
>  	struct ps3_vuart_port_driver *drv;
> @@ -1118,7 +1118,7 @@ static int ps3_vuart_remove(struct ps3_system_bus_device *dev)
>  		dev_dbg(&dev->core, "%s:%d: no driver bound\n", __func__,
>  			__LINE__);
>  		mutex_unlock(&vuart_bus_priv.probe_mutex);
> -		return 0;
> +		return;
>  	}
>  
>  	drv = ps3_system_bus_dev_to_vuart_drv(dev);
> @@ -1141,7 +1141,6 @@ static int ps3_vuart_remove(struct ps3_system_bus_device *dev)
>  
>  	dev_dbg(&dev->core, " <- %s:%d\n", __func__, __LINE__);
>  	mutex_unlock(&vuart_bus_priv.probe_mutex);
> -	return 0;
>  }
>  
>  /**
> @@ -1154,7 +1153,7 @@ static int ps3_vuart_remove(struct ps3_system_bus_device *dev)
>   * sequence.
>   */
>  
> -static int ps3_vuart_shutdown(struct ps3_system_bus_device *dev)
> +static void ps3_vuart_shutdown(struct ps3_system_bus_device *dev)
>  {
>  	struct ps3_vuart_port_driver *drv;
>  
> @@ -1169,7 +1168,7 @@ static int ps3_vuart_shutdown(struct ps3_system_bus_device *dev)
>  		dev_dbg(&dev->core, "%s:%d: no driver bound\n", __func__,
>  			__LINE__);
>  		mutex_unlock(&vuart_bus_priv.probe_mutex);
> -		return 0;
> +		return;
>  	}
>  
>  	drv = ps3_system_bus_dev_to_vuart_drv(dev);
> @@ -1193,7 +1192,6 @@ static int ps3_vuart_shutdown(struct ps3_system_bus_device *dev)
>  	dev_dbg(&dev->core, " <- %s:%d\n", __func__, __LINE__);
>  
>  	mutex_unlock(&vuart_bus_priv.probe_mutex);
> -	return 0;
>  }
>  
>  static int __init ps3_vuart_bus_init(void)
> diff --git a/drivers/scsi/ps3rom.c b/drivers/scsi/ps3rom.c
> index f75c0b5cd587..ccb5771f1cb7 100644
> --- a/drivers/scsi/ps3rom.c
> +++ b/drivers/scsi/ps3rom.c
> @@ -402,7 +402,7 @@ static int ps3rom_probe(struct ps3_system_bus_device *_dev)
>  	return error;
>  }
>  
> -static int ps3rom_remove(struct ps3_system_bus_device *_dev)
> +static void ps3rom_remove(struct ps3_system_bus_device *_dev)
>  {
>  	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
>  	struct Scsi_Host *host = ps3_system_bus_get_drvdata(&dev->sbd);
> @@ -412,7 +412,6 @@ static int ps3rom_remove(struct ps3_system_bus_device *_dev)
>  	scsi_host_put(host);
>  	ps3_system_bus_set_drvdata(&dev->sbd, NULL);
>  	kfree(dev->bounce_buf);
> -	return 0;
>  }
>  
>  static struct ps3_system_bus_driver ps3rom = {
> diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
> index fb52133c3557..98568b046a1a 100644
> --- a/drivers/usb/host/ehci-ps3.c
> +++ b/drivers/usb/host/ehci-ps3.c
> @@ -200,7 +200,7 @@ static int ps3_ehci_probe(struct ps3_system_bus_device *dev)
>  	return result;
>  }
>  
> -static int ps3_ehci_remove(struct ps3_system_bus_device *dev)
> +static void ps3_ehci_remove(struct ps3_system_bus_device *dev)
>  {
>  	unsigned int tmp;
>  	struct usb_hcd *hcd = ps3_system_bus_get_drvdata(dev);
> @@ -227,8 +227,6 @@ static int ps3_ehci_remove(struct ps3_system_bus_device *dev)
>  
>  	ps3_dma_region_free(dev->d_region);
>  	ps3_close_hv_device(dev);
> -
> -	return 0;
>  }
>  
>  static int __init ps3_ehci_driver_register(struct ps3_system_bus_driver *drv)
> diff --git a/drivers/usb/host/ohci-ps3.c b/drivers/usb/host/ohci-ps3.c
> index f77cd6af0ccf..4f5af929c3e4 100644
> --- a/drivers/usb/host/ohci-ps3.c
> +++ b/drivers/usb/host/ohci-ps3.c
> @@ -184,7 +184,7 @@ static int ps3_ohci_probe(struct ps3_system_bus_device *dev)
>  	return result;
>  }
>  
> -static int ps3_ohci_remove(struct ps3_system_bus_device *dev)
> +static void ps3_ohci_remove(struct ps3_system_bus_device *dev)
>  {
>  	unsigned int tmp;
>  	struct usb_hcd *hcd = ps3_system_bus_get_drvdata(dev);
> @@ -212,8 +212,6 @@ static int ps3_ohci_remove(struct ps3_system_bus_device *dev)
>  
>  	ps3_dma_region_free(dev->d_region);
>  	ps3_close_hv_device(dev);
> -
> -	return 0;
>  }
>  
>  static int __init ps3_ohci_driver_register(struct ps3_system_bus_driver *drv)
> diff --git a/drivers/video/fbdev/ps3fb.c b/drivers/video/fbdev/ps3fb.c
> index 203c254f8f6c..2fe08b67eda7 100644
> --- a/drivers/video/fbdev/ps3fb.c
> +++ b/drivers/video/fbdev/ps3fb.c
> @@ -1208,7 +1208,7 @@ static int ps3fb_probe(struct ps3_system_bus_device *dev)
>  	return retval;
>  }
>  
> -static int ps3fb_shutdown(struct ps3_system_bus_device *dev)
> +static void ps3fb_shutdown(struct ps3_system_bus_device *dev)
>  {
>  	struct fb_info *info = ps3_system_bus_get_drvdata(dev);
>  	u64 xdr_lpar = ps3_mm_phys_to_lpar(__pa(ps3fb_videomemory.address));
> @@ -1241,8 +1241,6 @@ static int ps3fb_shutdown(struct ps3_system_bus_device *dev)
>  	lv1_gpu_memory_free(ps3fb.memory_handle);
>  	ps3_close_hv_device(dev);
>  	dev_dbg(&dev->core, " <- %s:%d\n", __func__, __LINE__);
> -
> -	return 0;
>  }
>  
>  static struct ps3_system_bus_driver ps3fb_driver = {
> diff --git a/sound/ppc/snd_ps3.c b/sound/ppc/snd_ps3.c
> index 6ab796a5d936..8e44fa5d4dc7 100644
> --- a/sound/ppc/snd_ps3.c
> +++ b/sound/ppc/snd_ps3.c
> @@ -1049,7 +1049,7 @@ static int snd_ps3_driver_probe(struct ps3_system_bus_device *dev)
>  }; /* snd_ps3_probe */
>  
>  /* called when module removal */
> -static int snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
> +static void snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
>  {
>  	int ret;
>  	pr_info("%s:start id=%d\n", __func__,  dev->match_id);
> @@ -1075,7 +1075,6 @@ static int snd_ps3_driver_remove(struct ps3_system_bus_device *dev)
>  	lv1_gpu_device_unmap(2);
>  	ps3_close_hv_device(dev);
>  	pr_info("%s:end id=%d\n", __func__, dev->match_id);
> -	return 0;
>  } /* snd_ps3_remove */
>  
>  static struct ps3_system_bus_driver snd_ps3_bus_driver_info = {
> -- 
> 2.29.2
> 

Re: [PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Uwe Kleine-König]

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

Hello Michael,

On Sat, Nov 28, 2020 at 09:48:30AM +0100, Takashi Iwai wrote:
> On Thu, 26 Nov 2020 17:59:50 +0100,
> Uwe Kleine-König wrote:
> > 
> > The driver core ignores the return value of struct device_driver::remove
> > because there is only little that can be done. For the shutdown callback
> > it's ps3_system_bus_shutdown() which ignores the return value.
> > 
> > To simplify the quest to make struct device_driver::remove return void,
> > let struct ps3_system_bus_driver::remove return void, too. All users
> > already unconditionally return 0, this commit makes it obvious that
> > returning an error code is a bad idea and ensures future users behave
> > accordingly.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> For the sound bit:
> Acked-by: Takashi Iwai <tiwai@suse.de>

assuming that you are the one who will apply this patch: Note that it
depends on patch 1 that Takashi already applied to his tree. So you
either have to wait untils patch 1 appears in some tree that you merge
before applying, or you have to take patch 1, too. (With Takashi
optinally dropping it then.)

Best regards
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 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Michael Ellerman]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> Hello Michael,
>
> On Sat, Nov 28, 2020 at 09:48:30AM +0100, Takashi Iwai wrote:
>> On Thu, 26 Nov 2020 17:59:50 +0100,
>> Uwe Kleine-König wrote:
>> > 
>> > The driver core ignores the return value of struct device_driver::remove
>> > because there is only little that can be done. For the shutdown callback
>> > it's ps3_system_bus_shutdown() which ignores the return value.
>> > 
>> > To simplify the quest to make struct device_driver::remove return void,
>> > let struct ps3_system_bus_driver::remove return void, too. All users
>> > already unconditionally return 0, this commit makes it obvious that
>> > returning an error code is a bad idea and ensures future users behave
>> > accordingly.
>> > 
>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> 
>> For the sound bit:
>> Acked-by: Takashi Iwai <tiwai@suse.de>
>
> assuming that you are the one who will apply this patch: Note that it
> depends on patch 1 that Takashi already applied to his tree. So you
> either have to wait untils patch 1 appears in some tree that you merge
> before applying, or you have to take patch 1, too. (With Takashi
> optinally dropping it then.)

Thanks. I've picked up both patches.

If Takashi doesn't want to rebase his tree to drop patch 1 that's OK, it
will just arrive in mainline via two paths, but git should handle it.

cheers

Re: [PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void

[Takashi Iwai]

On Wed, 02 Dec 2020 13:14:06 +0100,
Michael Ellerman wrote:
> 
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > Hello Michael,
> >
> > On Sat, Nov 28, 2020 at 09:48:30AM +0100, Takashi Iwai wrote:
> >> On Thu, 26 Nov 2020 17:59:50 +0100,
> >> Uwe Kleine-König wrote:
> >> > 
> >> > The driver core ignores the return value of struct device_driver::remove
> >> > because there is only little that can be done. For the shutdown callback
> >> > it's ps3_system_bus_shutdown() which ignores the return value.
> >> > 
> >> > To simplify the quest to make struct device_driver::remove return void,
> >> > let struct ps3_system_bus_driver::remove return void, too. All users
> >> > already unconditionally return 0, this commit makes it obvious that
> >> > returning an error code is a bad idea and ensures future users behave
> >> > accordingly.
> >> > 
> >> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> 
> >> For the sound bit:
> >> Acked-by: Takashi Iwai <tiwai@suse.de>
> >
> > assuming that you are the one who will apply this patch: Note that it
> > depends on patch 1 that Takashi already applied to his tree. So you
> > either have to wait untils patch 1 appears in some tree that you merge
> > before applying, or you have to take patch 1, too. (With Takashi
> > optinally dropping it then.)
> 
> Thanks. I've picked up both patches.
> 
> If Takashi doesn't want to rebase his tree to drop patch 1 that's OK, it
> will just arrive in mainline via two paths, but git should handle it.

Yeah, I'd like to avoid rebasing, so let's get it merge from both
trees.  git can handle such a case gracefully.


thanks,

Takashi

Re: [PATCH 1/2] ALSA: ppc: drop if block with always false condition

[Michael Ellerman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

On Thu, 26 Nov 2020 17:59:49 +0100, Uwe Kleine-König wrote:
> The remove callback is only called for devices that were probed
> successfully before. As the matching probe function cannot complete
> without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to
> check this here.

Applied to powerpc/next.

[1/2] ALSA: ppc: drop if block with always false condition
      https://git.kernel.org/powerpc/c/7ff94669e7d8e50756cd57947283381ae9665759
[2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void
      https://git.kernel.org/powerpc/c/6d247e4d264961aa3b871290f9b11a48d5a567f2

cheers