| Message ID | 20240102-j7200-pcie-s2r-v3-5-5c2e4a3fac1f@bootlin.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote: > The mux_chip_resume() function restores a mux_chip using the cached state > of each mux. ... > +int mux_chip_resume(struct mux_chip *mux_chip) > +{ > + int global_ret = 0; > + int ret, i; > + > + for (i = 0; i < mux_chip->controllers; ++i) { > + struct mux_control *mux = &mux_chip->mux[i]; > + > + if (mux->cached_state == MUX_CACHE_UNKNOWN) > + continue; > + > + ret = mux_control_set(mux, mux->cached_state); > + if (ret < 0) { > + dev_err(&mux_chip->dev, "unable to restore state\n"); > + if (!global_ret) > + global_ret = ret; Hmm... This will record the first error and continue. > + } > + } > + return global_ret; So here, we actually will get stale data in case there are > 1 failures. > +}
On 2/15/24 16:29, Andy Shevchenko wrote: > On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote: >> The mux_chip_resume() function restores a mux_chip using the cached state >> of each mux. > > ... > >> +int mux_chip_resume(struct mux_chip *mux_chip) >> +{ >> + int global_ret = 0; >> + int ret, i; >> + >> + for (i = 0; i < mux_chip->controllers; ++i) { >> + struct mux_control *mux = &mux_chip->mux[i]; >> + >> + if (mux->cached_state == MUX_CACHE_UNKNOWN) >> + continue; >> + >> + ret = mux_control_set(mux, mux->cached_state); >> + if (ret < 0) { >> + dev_err(&mux_chip->dev, "unable to restore state\n"); >> + if (!global_ret) >> + global_ret = ret; > > Hmm... This will record the first error and continue. In the v2 we talked about this with Peter Rosin. In fact, in the v1 (mux_chip_resume() didn't exists yet, everything was done in the mmio driver) I had the same behavior: try to restore all muxes and in case of error restore the first one. I don't know what is the right solution. I just restored the behavior I had in v1. > >> + } >> + } >> + return global_ret; > > So here, we actually will get stale data in case there are > 1 failures. Yes, indeed. But we will have an error message for each failure. > >> +} >
On Fri, Feb 16, 2024 at 08:52:17AM +0100, Thomas Richard wrote: > On 2/15/24 16:29, Andy Shevchenko wrote: > > On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote: ... > >> +int mux_chip_resume(struct mux_chip *mux_chip) > >> +{ > >> + int global_ret = 0; > >> + int ret, i; > >> + > >> + for (i = 0; i < mux_chip->controllers; ++i) { > >> + struct mux_control *mux = &mux_chip->mux[i]; > >> + > >> + if (mux->cached_state == MUX_CACHE_UNKNOWN) > >> + continue; > >> + > >> + ret = mux_control_set(mux, mux->cached_state); > >> + if (ret < 0) { > >> + dev_err(&mux_chip->dev, "unable to restore state\n"); > >> + if (!global_ret) > >> + global_ret = ret; > > > > Hmm... This will record the first error and continue. > > In the v2 we talked about this with Peter Rosin. > > In fact, in the v1 (mux_chip_resume() didn't exists yet, everything was > done in the mmio driver) I had the same behavior: try to restore all > muxes and in case of error restore the first one. > > I don't know what is the right solution. I just restored the behavior I > had in v1. Okay, I believe you know what you are doing, folks. But to me this approach sounds at bare minimum "unusual". Because the failures here are not fatal and recording the first one may or may not make sense and it's so fragile as it completely implementation-dependent. > >> + } > >> + } > >> + return global_ret; > > > > So here, we actually will get stale data in case there are > 1 failures. > > Yes, indeed. But we will have an error message for each failure. Which is also problematic. PM calls may easily spam the logs and outshadow really important messages (like oopses).
On 2/16/24 16:07, Andy Shevchenko wrote: > On Fri, Feb 16, 2024 at 08:52:17AM +0100, Thomas Richard wrote: >> On 2/15/24 16:29, Andy Shevchenko wrote: >>> On Thu, Feb 15, 2024 at 04:17:50PM +0100, Thomas Richard wrote: > > ... > >>>> +int mux_chip_resume(struct mux_chip *mux_chip) >>>> +{ >>>> + int global_ret = 0; >>>> + int ret, i; >>>> + >>>> + for (i = 0; i < mux_chip->controllers; ++i) { >>>> + struct mux_control *mux = &mux_chip->mux[i]; >>>> + >>>> + if (mux->cached_state == MUX_CACHE_UNKNOWN) >>>> + continue; >>>> + >>>> + ret = mux_control_set(mux, mux->cached_state); >>>> + if (ret < 0) { >>>> + dev_err(&mux_chip->dev, "unable to restore state\n"); >>>> + if (!global_ret) >>>> + global_ret = ret; >>> >>> Hmm... This will record the first error and continue. >> >> In the v2 we talked about this with Peter Rosin. >> >> In fact, in the v1 (mux_chip_resume() didn't exists yet, everything was >> done in the mmio driver) I had the same behavior: try to restore all >> muxes and in case of error restore the first one. >> >> I don't know what is the right solution. I just restored the behavior I >> had in v1. > > Okay, I believe you know what you are doing, folks. But to me this approach > sounds at bare minimum "unusual". Because the failures here are not fatal > and recording the first one may or may not make sense and it's so fragile > as it completely implementation-dependent. I guess if there is an error, the resume is completely dead so no need to continue. If it's okay for Peter I can return on first failure. Regards,
diff --git a/drivers/mux/core.c b/drivers/mux/core.c index 775816112932..5db5a7698ad1 100644 --- a/drivers/mux/core.c +++ b/drivers/mux/core.c @@ -215,6 +215,36 @@ void mux_chip_free(struct mux_chip *mux_chip) } EXPORT_SYMBOL_GPL(mux_chip_free); +/** + * mux_chip_resume() - restores the mux-chip state + * @mux_chip: The mux-chip to resume. + * + * Restores the mux-chip state. + * + * Return: Zero on success or a negative errno on error. + */ +int mux_chip_resume(struct mux_chip *mux_chip) +{ + int global_ret = 0; + int ret, i; + + for (i = 0; i < mux_chip->controllers; ++i) { + struct mux_control *mux = &mux_chip->mux[i]; + + if (mux->cached_state == MUX_CACHE_UNKNOWN) + continue; + + ret = mux_control_set(mux, mux->cached_state); + if (ret < 0) { + dev_err(&mux_chip->dev, "unable to restore state\n"); + if (!global_ret) + global_ret = ret; + } + } + return global_ret; +} +EXPORT_SYMBOL_GPL(mux_chip_resume); + static void devm_mux_chip_release(struct device *dev, void *res) { struct mux_chip *mux_chip = *(struct mux_chip **)res; diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h index 18824064f8c0..2a7e5ec5d540 100644 --- a/include/linux/mux/driver.h +++ b/include/linux/mux/driver.h @@ -88,6 +88,7 @@ struct mux_chip *mux_chip_alloc(struct device *dev, int mux_chip_register(struct mux_chip *mux_chip); void mux_chip_unregister(struct mux_chip *mux_chip); void mux_chip_free(struct mux_chip *mux_chip); +int mux_chip_resume(struct mux_chip *mux_chip); struct mux_chip *devm_mux_chip_alloc(struct device *dev, unsigned int controllers,
The mux_chip_resume() function restores a mux_chip using the cached state of each mux. Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- drivers/mux/core.c | 30 ++++++++++++++++++++++++++++++ include/linux/mux/driver.h | 1 + 2 files changed, 31 insertions(+)