| Message ID | 20220705063613.93770-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e57297fc0915e2f95de26d18ad8ab6f17c068658 |
| Headers | show |
| Series | ASoC: rsnd: Emit useful error messages in .remove() | expand |
Hi Uwe Thank you for your patch > for_each_rsnd_dai(rdai, priv, i) { > - ret |= rsnd_dai_call(remove, &rdai->playback, priv); > - ret |= rsnd_dai_call(remove, &rdai->capture, priv); > + int ret; > + > + ret = rsnd_dai_call(remove, &rdai->playback, priv); > + if (ret) > + dev_warn(&pdev->dev, "Failed to remove playback dai #%d\n", i); > + > + ret = rsnd_dai_call(remove, &rdai->capture, priv); > + if (ret) > + dev_warn(&pdev->dev, "Failed to remove capture dai #%d\n", i); > } > > for (i = 0; i < ARRAY_SIZE(remove_func); i++) > remove_func[i](priv); > > - return ret; > + return 0; > } I think we want to get return error ? The reason why it was using |= is that it should call all function without break even though it was error (Maybe first/last error is very OK). So, we want to is maybe like this (it uses last error) int ret = 0; for_each() { int __ret; __ret = xxx(); if (__ret < 0) { ... => ret = __ret; } } => return ret; Thank you for your help !! Best regards --- Kuninori Morimoto
On Tue, Jul 05, 2022 at 11:21:20PM +0000, Kuninori Morimoto wrote: > > Hi Uwe > > Thank you for your patch > > > for_each_rsnd_dai(rdai, priv, i) { > > - ret |= rsnd_dai_call(remove, &rdai->playback, priv); > > - ret |= rsnd_dai_call(remove, &rdai->capture, priv); > > + int ret; > > + > > + ret = rsnd_dai_call(remove, &rdai->playback, priv); > > + if (ret) > > + dev_warn(&pdev->dev, "Failed to remove playback dai #%d\n", i); > > + > > + ret = rsnd_dai_call(remove, &rdai->capture, priv); > > + if (ret) > > + dev_warn(&pdev->dev, "Failed to remove capture dai #%d\n", i); > > } > > > > for (i = 0; i < ARRAY_SIZE(remove_func); i++) > > remove_func[i](priv); > > > > - return ret; > > + return 0; > > } > > I think we want to get return error ? The motivation of my patch is to make the remove function return 0 and the eventual goal is to make the remove callback return void. The difference today between returning 0 and returning an error is only that the core emits an error message in the error case. In both cases the device is removed. See drivers/base/platform.c:platform_remove(). > The reason why it was using |= is that it should call all function > without break even though it was error It's right to call all cleanup functions also if some of them fail. But returning an error is useless. Best regards Uwe
Hi Uwe > The motivation of my patch is to make the remove function return 0 and > the eventual goal is to make the remove callback return void. > The difference today between returning 0 and returning an error is only > that the core emits an error message in the error case. In both cases > the device is removed. See drivers/base/platform.c:platform_remove(). Ah, OK. Because of "remove" funciton. Sorry, I was misunderstanding about it. Thank you for your help !! Best regards --- Kuninori Morimoto
Hello, On Wed, Jul 06, 2022 at 06:42:49AM +0000, Kuninori Morimoto wrote: > > The motivation of my patch is to make the remove function return 0 and > > the eventual goal is to make the remove callback return void. > > The difference today between returning 0 and returning an error is only > > that the core emits an error message in the error case. In both cases > > the device is removed. See drivers/base/platform.c:platform_remove(). > > Ah, OK. Because of "remove" funciton. > Sorry, I was misunderstanding about it. > > Thank you for your help !! Is this an ack now? :-) Best regards Uwe
On Tue, 5 Jul 2022 08:36:13 +0200, Uwe Kleine-König wrote: > If more than one call of rsnd_dai_call(remove, ...) fails the platform > remove callback returns all values orred together which then makes the > driver core emit a generic error message which is little helpful. > > Instead emit details of which call failed exactly and return 0. Note > returning 0 instead of an error code doesn't make a difference in the > driver core apart from the error message. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: rsnd: Emit useful error messages in .remove() commit: e57297fc0915e2f95de26d18ad8ab6f17c068658 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c index eb762ab94d3e..01225344001a 100644 --- a/sound/soc/sh/rcar/core.c +++ b/sound/soc/sh/rcar/core.c @@ -1968,19 +1968,26 @@ static int rsnd_remove(struct platform_device *pdev) rsnd_cmd_remove, rsnd_adg_remove, }; - int ret = 0, i; + int i; pm_runtime_disable(&pdev->dev); for_each_rsnd_dai(rdai, priv, i) { - ret |= rsnd_dai_call(remove, &rdai->playback, priv); - ret |= rsnd_dai_call(remove, &rdai->capture, priv); + int ret; + + ret = rsnd_dai_call(remove, &rdai->playback, priv); + if (ret) + dev_warn(&pdev->dev, "Failed to remove playback dai #%d\n", i); + + ret = rsnd_dai_call(remove, &rdai->capture, priv); + if (ret) + dev_warn(&pdev->dev, "Failed to remove capture dai #%d\n", i); } for (i = 0; i < ARRAY_SIZE(remove_func); i++) remove_func[i](priv); - return ret; + return 0; } static int __maybe_unused rsnd_suspend(struct device *dev)
If more than one call of rsnd_dai_call(remove, ...) fails the platform remove callback returns all values orred together which then makes the driver core emit a generic error message which is little helpful. Instead emit details of which call failed exactly and return 0. Note returning 0 instead of an error code doesn't make a difference in the driver core apart from the error message. This is a preparation for making platform remove callbacks return void. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- sound/soc/sh/rcar/core.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56