| Message ID | 20201126165950.2554997-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | 5c7797022fe9a95a4417c75fa59a1a2bfdc5a3be |
| Headers | show |
| Series | [1/2] ALSA: ppc: drop if block with always false condition | expand |
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
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
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
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>
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>
On 11/27/20 7:22 AM, Leonard Goehrs wrote: > The check for the FW_FEATURE_PS3_LV1 firmware feature is already performed > in ps3_system_bus_init() before registering the driver. So if the probe > function is actually used, this feature is already known to be available. > > The check for the match id is also superfluous; the condition is always > true because the bus' match function (ps3_system_bus_match()) only > considers this driver for devices having: > dev->match_id == snd_ps3_bus_driver_info.match_id. > > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Leonard Goehrs <l.goehrs@pengutronix.de> Seems OK with v5.9 on PS3. Tested by: Geoff Levand <geoff@infradead.org>
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
On Fri, 27 Nov 2020 16:22:59 +0100, Leonard Goehrs wrote: > > The check for the FW_FEATURE_PS3_LV1 firmware feature is already performed > in ps3_system_bus_init() before registering the driver. So if the probe > function is actually used, this feature is already known to be available. > > The check for the match id is also superfluous; the condition is always > true because the bus' match function (ps3_system_bus_match()) only > considers this driver for devices having: > dev->match_id == snd_ps3_bus_driver_info.match_id. > > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Leonard Goehrs <l.goehrs@pengutronix.de> Applied now. Thanks. Takashi
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
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
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
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
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(-)