[PATCH 0/2] Add resume support for the mux mmio driver

[PATCH 0/2] Add resume support for the mux mmio driver

[Thomas Richard]

The patches of this series were originally in the series "Add suspend to
ram support for PCIe on J7200" [1].
There is no changes compared to the patches in the series [1].

These patches add resume support for the mmio driver.
The first patch adds a new function mux_chip_resume() in the mux subsystem.
The second patch adds the resume support for the mmio driver using this new
function.

[1] https://lore.kernel.org/all/20240102-j7200-pcie-s2r-v6-0-4656ef6e6d66@bootlin.com/

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
Thomas Richard (1):
      mux: add mux_chip_resume() function

Théo Lebrun (1):
      mux: mmio: add resume support

 drivers/mux/core.c         | 29 +++++++++++++++++++++++++++++
 drivers/mux/mmio.c         | 12 ++++++++++++
 include/linux/mux/driver.h |  1 +
 3 files changed, 42 insertions(+)
---
base-commit: 8e7767d07e04b89999d5adefb190f4d5e566d8d4
change-id: 20240613-mux-mmio-resume-support-4f3b2a34a32a

Best regards,
-- 
Thomas Richard <thomas.richard@bootlin.com>

[PATCH 1/2] mux: add mux_chip_resume() function

[Thomas Richard]

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         | 29 +++++++++++++++++++++++++++++
 include/linux/mux/driver.h |  1 +
 2 files changed, 30 insertions(+)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 78c0022697ec..0858cacae845 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -215,6 +215,35 @@ 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 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");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+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,

-- 
2.39.2

[PATCH 2/2] mux: mmio: add resume support

[Thomas Richard]

From: Théo Lebrun <theo.lebrun@bootlin.com>

No need to save something during the suspend stage, as the mux core has an
internal cache to store the state of muxes.

This cache is used by mux_chip_resume() to restore all muxes.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/mux/mmio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c
index 30a952c34365..00405abe3ce3 100644
--- a/drivers/mux/mmio.c
+++ b/drivers/mux/mmio.c
@@ -130,13 +130,25 @@ static int mux_mmio_probe(struct platform_device *pdev)
 
 	mux_chip->ops = &mux_mmio_ops;
 
+	dev_set_drvdata(dev, mux_chip);
+
 	return devm_mux_chip_register(dev, mux_chip);
 }
 
+static int mux_mmio_resume_noirq(struct device *dev)
+{
+	struct mux_chip *mux_chip = dev_get_drvdata(dev);
+
+	return mux_chip_resume(mux_chip);
+}
+
+static DEFINE_NOIRQ_DEV_PM_OPS(mux_mmio_pm_ops, NULL, mux_mmio_resume_noirq);
+
 static struct platform_driver mux_mmio_driver = {
 	.driver = {
 		.name = "mmio-mux",
 		.of_match_table	= mux_mmio_dt_ids,
+		.pm = pm_sleep_ptr(&mux_mmio_pm_ops),
 	},
 	.probe = mux_mmio_probe,
 };

-- 
2.39.2

Re: [PATCH 0/2] Add resume support for the mux mmio driver

[Thomas Richard]

On 6/13/24 15:07, Thomas Richard wrote:
> The patches of this series were originally in the series "Add suspend to
> ram support for PCIe on J7200" [1].
> There is no changes compared to the patches in the series [1].
> 
> These patches add resume support for the mmio driver.
> The first patch adds a new function mux_chip_resume() in the mux subsystem.
> The second patch adds the resume support for the mmio driver using this new
> function.
> 
> [1] https://lore.kernel.org/all/20240102-j7200-pcie-s2r-v6-0-4656ef6e6d66@bootlin.com/

Hello !!

Gentle ping.

This series has no remaining comment to address.
Is there any chance to get this series merged ?

Best Regards,

Thomas

> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
> Thomas Richard (1):
>       mux: add mux_chip_resume() function
> 
> Théo Lebrun (1):
>       mux: mmio: add resume support
> 
>  drivers/mux/core.c         | 29 +++++++++++++++++++++++++++++
>  drivers/mux/mmio.c         | 12 ++++++++++++
>  include/linux/mux/driver.h |  1 +
>  3 files changed, 42 insertions(+)
> ---
> base-commit: 8e7767d07e04b89999d5adefb190f4d5e566d8d4
> change-id: 20240613-mux-mmio-resume-support-4f3b2a34a32a
> 
> Best regards,

Re: [PATCH 0/2] Add resume support for the mux mmio driver

[Thomas Richard]

On 6/13/24 15:07, Thomas Richard wrote:
> The patches of this series were originally in the series "Add suspend to
> ram support for PCIe on J7200" [1].
> There is no changes compared to the patches in the series [1].
> 
> These patches add resume support for the mmio driver.
> The first patch adds a new function mux_chip_resume() in the mux subsystem.
> The second patch adds the resume support for the mmio driver using this new
> function.
> 
> [1] https://lore.kernel.org/all/20240102-j7200-pcie-s2r-v6-0-4656ef6e6d66@bootlin.com/

Hello maintainers!

There is no remaining comments to address since a log time for this series.
Is there any chance to get this series merged ?

Thanks,

Thomas

> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
> Thomas Richard (1):
>       mux: add mux_chip_resume() function
> 
> Théo Lebrun (1):
>       mux: mmio: add resume support
> 
>  drivers/mux/core.c         | 29 +++++++++++++++++++++++++++++
>  drivers/mux/mmio.c         | 12 ++++++++++++
>  include/linux/mux/driver.h |  1 +
>  3 files changed, 42 insertions(+)
> ---
> base-commit: 8e7767d07e04b89999d5adefb190f4d5e566d8d4
> change-id: 20240613-mux-mmio-resume-support-4f3b2a34a32a
> 
> Best regards,

Re: [PATCH 0/2] Add resume support for the mux mmio driver

[Greg KH]

On Tue, Sep 03, 2024 at 02:39:03PM +0200, Thomas Richard wrote:
> On 6/13/24 15:07, Thomas Richard wrote:
> > The patches of this series were originally in the series "Add suspend to
> > ram support for PCIe on J7200" [1].
> > There is no changes compared to the patches in the series [1].
> > 
> > These patches add resume support for the mmio driver.
> > The first patch adds a new function mux_chip_resume() in the mux subsystem.
> > The second patch adds the resume support for the mmio driver using this new
> > function.
> > 
> > [1] https://lore.kernel.org/all/20240102-j7200-pcie-s2r-v6-0-4656ef6e6d66@bootlin.com/
> 
> Hello maintainers!
> 
> There is no remaining comments to address since a log time for this series.
> Is there any chance to get this series merged ?

I'm not the maintainer, not much I can do here, sorry...

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Peter Rosin]

Hi!

Sorry for being unresponsive. And for first writing this in the older v4
thread instead of here.

2024-06-13 at 15:07, Thomas Richard wrote:
> 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         | 29 +++++++++++++++++++++++++++++
>  include/linux/mux/driver.h |  1 +
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
> index 78c0022697ec..0858cacae845 100644
> --- a/drivers/mux/core.c
> +++ b/drivers/mux/core.c
> @@ -215,6 +215,35 @@ 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 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);

mux_control_set() is an internal helper. It is called from
__mux_control_select() and mux_control_deselect() (and on init...)

In all those cases, there is no race to reach the mux_control_set()
function, by means of the mux->lock semaphore (or the mux not being
"published" yet).

I fail to see how resume is safe when mux->lock is ignored?

Cheers,
Peter

> +		if (ret < 0) {
> +			dev_err(&mux_chip->dev, "unable to restore state\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +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,
> 

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Thomas Richard]

On 9/3/24 15:22, Peter Rosin wrote:
> Hi!
> 
> Sorry for being unresponsive. And for first writing this in the older v4
> thread instead of here.
> 
> 2024-06-13 at 15:07, Thomas Richard wrote:
>> 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         | 29 +++++++++++++++++++++++++++++
>>  include/linux/mux/driver.h |  1 +
>>  2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>> index 78c0022697ec..0858cacae845 100644
>> --- a/drivers/mux/core.c
>> +++ b/drivers/mux/core.c
>> @@ -215,6 +215,35 @@ 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 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);
> 
> mux_control_set() is an internal helper. It is called from
> __mux_control_select() and mux_control_deselect() (and on init...)
> 
> In all those cases, there is no race to reach the mux_control_set()
> function, by means of the mux->lock semaphore (or the mux not being
> "published" yet).
> 
> I fail to see how resume is safe when mux->lock is ignored?

I think I should use mux_control_select() to use the lock.
If I ignore the lock, I could have a cache coherence issue.

I'll send a new version which use mux_control_select().
But if I use mux_control_select(), I have to clean the cache before to
call it, if not nothing happen [1].

[1]
https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L319

Regards,

Thomas

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Peter Rosin]

Hi!

2024-09-04 at 11:18, Thomas Richard wrote:
> On 9/3/24 15:22, Peter Rosin wrote:
>> Hi!
>>
>> Sorry for being unresponsive. And for first writing this in the older v4
>> thread instead of here.
>>
>> 2024-06-13 at 15:07, Thomas Richard wrote:
>>> 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         | 29 +++++++++++++++++++++++++++++
>>>  include/linux/mux/driver.h |  1 +
>>>  2 files changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>> index 78c0022697ec..0858cacae845 100644
>>> --- a/drivers/mux/core.c
>>> +++ b/drivers/mux/core.c
>>> @@ -215,6 +215,35 @@ 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 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);
>>
>> mux_control_set() is an internal helper. It is called from
>> __mux_control_select() and mux_control_deselect() (and on init...)
>>
>> In all those cases, there is no race to reach the mux_control_set()
>> function, by means of the mux->lock semaphore (or the mux not being
>> "published" yet).
>>
>> I fail to see how resume is safe when mux->lock is ignored?
> 
> I think I should use mux_control_select() to use the lock.
> If I ignore the lock, I could have a cache coherence issue.
> 
> I'll send a new version which use mux_control_select().
> But if I use mux_control_select(), I have to clean the cache before to
> call it, if not nothing happen [1].
> 
> [1]
> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L319

No, calling mux_control_select() in resume context is not an
option IIUC. That would dead-lock if there is a long-time client
who has locked the mux in some desired state.

I see no trivial solution to integrate suspend/resume, and do
not have enough time to think about what a working solutions
would look like. Sorry.

Cheers,
Peter

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Thomas Richard]

On 9/4/24 11:32, Peter Rosin wrote:
> Hi!
> 
> 2024-09-04 at 11:18, Thomas Richard wrote:
>> On 9/3/24 15:22, Peter Rosin wrote:
>>> Hi!
>>>
>>> Sorry for being unresponsive. And for first writing this in the older v4
>>> thread instead of here.
>>>
>>> 2024-06-13 at 15:07, Thomas Richard wrote:
>>>> 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         | 29 +++++++++++++++++++++++++++++
>>>>  include/linux/mux/driver.h |  1 +
>>>>  2 files changed, 30 insertions(+)
>>>>
>>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>>> index 78c0022697ec..0858cacae845 100644
>>>> --- a/drivers/mux/core.c
>>>> +++ b/drivers/mux/core.c
>>>> @@ -215,6 +215,35 @@ 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 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);
>>>
>>> mux_control_set() is an internal helper. It is called from
>>> __mux_control_select() and mux_control_deselect() (and on init...)
>>>
>>> In all those cases, there is no race to reach the mux_control_set()
>>> function, by means of the mux->lock semaphore (or the mux not being
>>> "published" yet).
>>>
>>> I fail to see how resume is safe when mux->lock is ignored?
>>
>> I think I should use mux_control_select() to use the lock.
>> If I ignore the lock, I could have a cache coherence issue.
>>
>> I'll send a new version which use mux_control_select().
>> But if I use mux_control_select(), I have to clean the cache before to
>> call it, if not nothing happen [1].
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L319
> 
> No, calling mux_control_select() in resume context is not an
> option IIUC. That would dead-lock if there is a long-time client
> who has locked the mux in some desired state.

Yes, I didn't thought about it.

> 
> I see no trivial solution to integrate suspend/resume, and do
> not have enough time to think about what a working solutions
> would look like. Sorry.
> 

We maybe have a solution.
Please let me know if it's relevant or not for you:

- Add a get operation in struct mux_control_ops.

- Implement mux_chip_suspend() which reads the state of each mux using
the get operation, and store it in a hardware_state variable (stored in
the mux_control struct).

- The mux_chip_resume uses the hardware_state value to restore all muxes
using mux_control_set().
So if a mux is locked with a long delay, there is no dead-lock.

- If the get operation is not defined, mux_chip_suspend() and
mux_chip_resume() do nothing (maybe a warning or info message could be
useful).

Regards,

Thomas

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Peter Rosin]

Hi!

2024-09-04 at 13:29, Thomas Richard wrote:
> On 9/4/24 11:32, Peter Rosin wrote:
>> Hi!
>>
>> 2024-09-04 at 11:18, Thomas Richard wrote:
>>> On 9/3/24 15:22, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> Sorry for being unresponsive. And for first writing this in the older v4
>>>> thread instead of here.
>>>>
>>>> 2024-06-13 at 15:07, Thomas Richard wrote:
>>>>> 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         | 29 +++++++++++++++++++++++++++++
>>>>>  include/linux/mux/driver.h |  1 +
>>>>>  2 files changed, 30 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>>>> index 78c0022697ec..0858cacae845 100644
>>>>> --- a/drivers/mux/core.c
>>>>> +++ b/drivers/mux/core.c
>>>>> @@ -215,6 +215,35 @@ 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 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);
>>>>
>>>> mux_control_set() is an internal helper. It is called from
>>>> __mux_control_select() and mux_control_deselect() (and on init...)
>>>>
>>>> In all those cases, there is no race to reach the mux_control_set()
>>>> function, by means of the mux->lock semaphore (or the mux not being
>>>> "published" yet).
>>>>
>>>> I fail to see how resume is safe when mux->lock is ignored?
>>>
>>> I think I should use mux_control_select() to use the lock.
>>> If I ignore the lock, I could have a cache coherence issue.
>>>
>>> I'll send a new version which use mux_control_select().
>>> But if I use mux_control_select(), I have to clean the cache before to
>>> call it, if not nothing happen [1].
>>>
>>> [1]
>>> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L319
>>
>> No, calling mux_control_select() in resume context is not an
>> option IIUC. That would dead-lock if there is a long-time client
>> who has locked the mux in some desired state.
> 
> Yes, I didn't thought about it.
> 
>>
>> I see no trivial solution to integrate suspend/resume, and do
>> not have enough time to think about what a working solutions
>> would look like. Sorry.
>>
> 
> We maybe have a solution.
> Please let me know if it's relevant or not for you:
> 
> - Add a get operation in struct mux_control_ops.
> 
> - Implement mux_chip_suspend() which reads the state of each mux using
> the get operation, and store it in a hardware_state variable (stored in
> the mux_control struct).
> 
> - The mux_chip_resume uses the hardware_state value to restore all muxes
> using mux_control_set().
> So if a mux is locked with a long delay, there is no dead-lock.
> 
> - If the get operation is not defined, mux_chip_suspend() and
> mux_chip_resume() do nothing (maybe a warning or info message could be
> useful).

What if a mux control is used to mux e.g. an SPI bus, and on that bus
sits some device that needs to be accessed during suspend/resume. What
part of the system ensures that the mux is supended late and resumed
early? Other things probably also want to be suspended late and resumed
early. But everything can be latest/earliest, there has to be some kind
of order, and I'm not sure that ordering is guaranteed to "fit".

Cheers,
Peter

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Thomas Richard]

On 9/4/24 14:52, Peter Rosin wrote:
> Hi!
> 
> 2024-09-04 at 13:29, Thomas Richard wrote:
>> On 9/4/24 11:32, Peter Rosin wrote:
>>> Hi!
>>>
>>> 2024-09-04 at 11:18, Thomas Richard wrote:
>>>> On 9/3/24 15:22, Peter Rosin wrote:
>>>>> Hi!
>>>>>
>>>>> Sorry for being unresponsive. And for first writing this in the older v4
>>>>> thread instead of here.
>>>>>
>>>>> 2024-06-13 at 15:07, Thomas Richard wrote:
>>>>>> 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         | 29 +++++++++++++++++++++++++++++
>>>>>>  include/linux/mux/driver.h |  1 +
>>>>>>  2 files changed, 30 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>>>>> index 78c0022697ec..0858cacae845 100644
>>>>>> --- a/drivers/mux/core.c
>>>>>> +++ b/drivers/mux/core.c
>>>>>> @@ -215,6 +215,35 @@ 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 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);
>>>>>
>>>>> mux_control_set() is an internal helper. It is called from
>>>>> __mux_control_select() and mux_control_deselect() (and on init...)
>>>>>
>>>>> In all those cases, there is no race to reach the mux_control_set()
>>>>> function, by means of the mux->lock semaphore (or the mux not being
>>>>> "published" yet).
>>>>>
>>>>> I fail to see how resume is safe when mux->lock is ignored?
>>>>
>>>> I think I should use mux_control_select() to use the lock.
>>>> If I ignore the lock, I could have a cache coherence issue.
>>>>
>>>> I'll send a new version which use mux_control_select().
>>>> But if I use mux_control_select(), I have to clean the cache before to
>>>> call it, if not nothing happen [1].
>>>>
>>>> [1]
>>>> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L319
>>>
>>> No, calling mux_control_select() in resume context is not an
>>> option IIUC. That would dead-lock if there is a long-time client
>>> who has locked the mux in some desired state.
>>
>> Yes, I didn't thought about it.
>>
>>>
>>> I see no trivial solution to integrate suspend/resume, and do
>>> not have enough time to think about what a working solutions
>>> would look like. Sorry.
>>>
>>
>> We maybe have a solution.
>> Please let me know if it's relevant or not for you:
>>
>> - Add a get operation in struct mux_control_ops.
>>
>> - Implement mux_chip_suspend() which reads the state of each mux using
>> the get operation, and store it in a hardware_state variable (stored in
>> the mux_control struct).
>>
>> - The mux_chip_resume uses the hardware_state value to restore all muxes
>> using mux_control_set().
>> So if a mux is locked with a long delay, there is no dead-lock.
>>
>> - If the get operation is not defined, mux_chip_suspend() and
>> mux_chip_resume() do nothing (maybe a warning or info message could be
>> useful).
> 
> What if a mux control is used to mux e.g. an SPI bus, and on that bus
> sits some device that needs to be accessed during suspend/resume. What
> part of the system ensures that the mux is supended late and resumed
> early? Other things probably also want to be suspended late and resumed
> early. But everything can be latest/earliest, there has to be some kind
> of order, and I'm not sure that ordering is guaranteed to "fit".

I experimented that it's ordered correctly for each stage, using
dependencies defined in the DT (I guess).
Of course if we suspend at suspend stage and resume at resume stage
whereas an SPI access is done at suspend_late (for example), it doesn't
work.
We could suspend/resume at noirq stages. It's the last suspend stage,
and the first resume stage, so we are sure to save and restore the right
states.

Thomas

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Peter Rosin]

Hi!

2024-09-04 at 18:07, Thomas Richard wrote:
> On 9/4/24 14:52, Peter Rosin wrote:
>> Hi!
>>
>> 2024-09-04 at 13:29, Thomas Richard wrote:
>>> On 9/4/24 11:32, Peter Rosin wrote:
>>>> Hi!
>>>>
>>>> 2024-09-04 at 11:18, Thomas Richard wrote:
>>>>> On 9/3/24 15:22, Peter Rosin wrote:
>>>>>> Hi!
>>>>>>
>>>>>> Sorry for being unresponsive. And for first writing this in the older v4
>>>>>> thread instead of here.
>>>>>>
>>>>>> 2024-06-13 at 15:07, Thomas Richard wrote:
>>>>>>> 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         | 29 +++++++++++++++++++++++++++++
>>>>>>>  include/linux/mux/driver.h |  1 +
>>>>>>>  2 files changed, 30 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>>>>>> index 78c0022697ec..0858cacae845 100644
>>>>>>> --- a/drivers/mux/core.c
>>>>>>> +++ b/drivers/mux/core.c
>>>>>>> @@ -215,6 +215,35 @@ 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 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);
>>>>>>
>>>>>> mux_control_set() is an internal helper. It is called from
>>>>>> __mux_control_select() and mux_control_deselect() (and on init...)
>>>>>>
>>>>>> In all those cases, there is no race to reach the mux_control_set()
>>>>>> function, by means of the mux->lock semaphore (or the mux not being
>>>>>> "published" yet).
>>>>>>
>>>>>> I fail to see how resume is safe when mux->lock is ignored?
>>>>>
>>>>> I think I should use mux_control_select() to use the lock.
>>>>> If I ignore the lock, I could have a cache coherence issue.
>>>>>
>>>>> I'll send a new version which use mux_control_select().
>>>>> But if I use mux_control_select(), I have to clean the cache before to
>>>>> call it, if not nothing happen [1].
>>>>>
>>>>> [1]
>>>>> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L319
>>>>
>>>> No, calling mux_control_select() in resume context is not an
>>>> option IIUC. That would dead-lock if there is a long-time client
>>>> who has locked the mux in some desired state.
>>>
>>> Yes, I didn't thought about it.
>>>
>>>>
>>>> I see no trivial solution to integrate suspend/resume, and do
>>>> not have enough time to think about what a working solutions
>>>> would look like. Sorry.
>>>>
>>>
>>> We maybe have a solution.
>>> Please let me know if it's relevant or not for you:
>>>
>>> - Add a get operation in struct mux_control_ops.
>>>
>>> - Implement mux_chip_suspend() which reads the state of each mux using
>>> the get operation, and store it in a hardware_state variable (stored in
>>> the mux_control struct).
>>>
>>> - The mux_chip_resume uses the hardware_state value to restore all muxes
>>> using mux_control_set().
>>> So if a mux is locked with a long delay, there is no dead-lock.
>>>
>>> - If the get operation is not defined, mux_chip_suspend() and
>>> mux_chip_resume() do nothing (maybe a warning or info message could be
>>> useful).
>>
>> What if a mux control is used to mux e.g. an SPI bus, and on that bus
>> sits some device that needs to be accessed during suspend/resume. What
>> part of the system ensures that the mux is supended late and resumed
>> early? Other things probably also want to be suspended late and resumed
>> early. But everything can be latest/earliest, there has to be some kind
>> of order, and I'm not sure that ordering is guaranteed to "fit".
> 
> I experimented that it's ordered correctly for each stage, using
> dependencies defined in the DT (I guess).
> Of course if we suspend at suspend stage and resume at resume stage
> whereas an SPI access is done at suspend_late (for example), it doesn't
> work.
> We could suspend/resume at noirq stages. It's the last suspend stage,
> and the first resume stage, so we are sure to save and restore the right
> states.

And what if the mux in turn sits on I2C? Is the ordering still guaranteed
to be correct? I.e. I'm not really intrested in just one case, but in the
general problem. I am resisting the attempt to add generic support for
something that, AFAICT, has no clear general solution.

Maybe you should simply implement resume locally in the driver itself and
have it reprogram the register, perhaps still based on mux->cached_state,
but "behind the back" of the mux core?

Cheers,
Peter

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Thomas Richard]

On 9/5/24 10:28, Peter Rosin wrote:
> Hi!
> 
> 2024-09-04 at 18:07, Thomas Richard wrote:
>> On 9/4/24 14:52, Peter Rosin wrote:
>>> Hi!
>>>
>>> 2024-09-04 at 13:29, Thomas Richard wrote:
>>>> On 9/4/24 11:32, Peter Rosin wrote:
>>>>> Hi!
>>>>>
>>>>> 2024-09-04 at 11:18, Thomas Richard wrote:
>>>>>> On 9/3/24 15:22, Peter Rosin wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Sorry for being unresponsive. And for first writing this in the older v4
>>>>>>> thread instead of here.
>>>>>>>
>>>>>>> 2024-06-13 at 15:07, Thomas Richard wrote:
>>>>>>>> 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         | 29 +++++++++++++++++++++++++++++
>>>>>>>>  include/linux/mux/driver.h |  1 +
>>>>>>>>  2 files changed, 30 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/mux/core.c b/drivers/mux/core.c
>>>>>>>> index 78c0022697ec..0858cacae845 100644
>>>>>>>> --- a/drivers/mux/core.c
>>>>>>>> +++ b/drivers/mux/core.c
>>>>>>>> @@ -215,6 +215,35 @@ 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 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);
>>>>>>>
>>>>>>> mux_control_set() is an internal helper. It is called from
>>>>>>> __mux_control_select() and mux_control_deselect() (and on init...)
>>>>>>>
>>>>>>> In all those cases, there is no race to reach the mux_control_set()
>>>>>>> function, by means of the mux->lock semaphore (or the mux not being
>>>>>>> "published" yet).
>>>>>>>
>>>>>>> I fail to see how resume is safe when mux->lock is ignored?
>>>>>>
>>>>>> I think I should use mux_control_select() to use the lock.
>>>>>> If I ignore the lock, I could have a cache coherence issue.
>>>>>>
>>>>>> I'll send a new version which use mux_control_select().
>>>>>> But if I use mux_control_select(), I have to clean the cache before to
>>>>>> call it, if not nothing happen [1].
>>>>>>
>>>>>> [1]
>>>>>> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L319
>>>>>
>>>>> No, calling mux_control_select() in resume context is not an
>>>>> option IIUC. That would dead-lock if there is a long-time client
>>>>> who has locked the mux in some desired state.
>>>>
>>>> Yes, I didn't thought about it.
>>>>
>>>>>
>>>>> I see no trivial solution to integrate suspend/resume, and do
>>>>> not have enough time to think about what a working solutions
>>>>> would look like. Sorry.
>>>>>
>>>>
>>>> We maybe have a solution.
>>>> Please let me know if it's relevant or not for you:
>>>>
>>>> - Add a get operation in struct mux_control_ops.
>>>>
>>>> - Implement mux_chip_suspend() which reads the state of each mux using
>>>> the get operation, and store it in a hardware_state variable (stored in
>>>> the mux_control struct).
>>>>
>>>> - The mux_chip_resume uses the hardware_state value to restore all muxes
>>>> using mux_control_set().
>>>> So if a mux is locked with a long delay, there is no dead-lock.
>>>>
>>>> - If the get operation is not defined, mux_chip_suspend() and
>>>> mux_chip_resume() do nothing (maybe a warning or info message could be
>>>> useful).
>>>
>>> What if a mux control is used to mux e.g. an SPI bus, and on that bus
>>> sits some device that needs to be accessed during suspend/resume. What
>>> part of the system ensures that the mux is supended late and resumed
>>> early? Other things probably also want to be suspended late and resumed
>>> early. But everything can be latest/earliest, there has to be some kind
>>> of order, and I'm not sure that ordering is guaranteed to "fit".
>>
>> I experimented that it's ordered correctly for each stage, using
>> dependencies defined in the DT (I guess).
>> Of course if we suspend at suspend stage and resume at resume stage
>> whereas an SPI access is done at suspend_late (for example), it doesn't
>> work.
>> We could suspend/resume at noirq stages. It's the last suspend stage,
>> and the first resume stage, so we are sure to save and restore the right
>> states.
> 
> And what if the mux in turn sits on I2C? Is the ordering still guaranteed
> to be correct? I.e. I'm not really intrested in just one case, but in the
> general problem. I am resisting the attempt to add generic support for
> something that, AFAICT, has no clear general solution.
> 
> Maybe you should simply implement resume locally in the driver itself and
> have it reprogram the register, perhaps still based on mux->cached_state,
> but "behind the back" of the mux core?

Ok, it's seems to be the best solution for now.
I'll send a patch.

Just a small comment, I think I should not use the cached_state.
I should implement a mux_mmio_get(), which is called during suspend, to
get the "real" state. Then use it during resume.
Because the cache is not coherent during is a very small period [1].

What do you think ?

[1]
https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L144

Thomas

Re: [PATCH 1/2] mux: add mux_chip_resume() function

[Peter Rosin]

Hi!

2024-09-06 at 18:17, Thomas Richard wrote:
> On 9/5/24 10:28, Peter Rosin wrote:
>> Maybe you should simply implement resume locally in the driver itself and
>> have it reprogram the register, perhaps still based on mux->cached_state,
>> but "behind the back" of the mux core?
> 
> Ok, it's seems to be the best solution for now.
> I'll send a patch.
> 
> Just a small comment, I think I should not use the cached_state.
> I should implement a mux_mmio_get(), which is called during suspend, to
> get the "real" state. Then use it during resume.
> Because the cache is not coherent during is a very small period [1].
> 
> What do you think ?
> 
> [1]
> https://elixir.bootlin.com/linux/v6.11-rc6/source/drivers/mux/core.c#L144

If you are worried about that, then I think you need a mutex in the
driver. Or why wouldn't a mux_mmio_get() be racy as well? (since you
are not able to grab the mux->lock)

Cheers,
Peter