Re: [PATCH v3] spmi: mtk-pmif: Serialize PMIF status check and command submission

Re: [PATCH v3] spmi: mtk-pmif: Serialize PMIF status check and command submission

[Nícolas F. R. A. Prado]

On Mon, Jul 24, 2023 at 11:47:33AM -0400, Nícolas F. R. A. Prado wrote:
> Before writing the read or write command to the SPMI arbiter through the
> PMIF interface, the current status of the channel is checked to ensure
> it is idle. However, since the status only changes from idle when the
> command is written, it is possible for two concurrent calls to determine
> that the channel is idle and simultaneously send their commands. At this
> point the PMIF interface hangs, with the status register no longer being
> updated, and thus causing all subsequent operations to time out.
> 
> This was observed on the mt8195-cherry-tomato-r2 machine, particularly
> after commit 46600ab142f8 ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for
> drivers between 5.10 and 5.15") was applied, since then the two MT6315
> devices present on the SPMI bus would probe assynchronously and
> sometimes (during probe or at a later point) read the bus
> simultaneously, breaking the PMIF interface and consequently slowing
> down the whole system.
> 
> To fix the issue at its root cause, introduce locking around the channel
> status check and the command write, so that both become an atomic
> operation, preventing race conditions between two (or more) SPMI bus
> read/write operations. A spinlock is used since this is a fast bus, as
> indicated by the usage of the atomic variant of readl_poll, and
> '.fast_io = true' being used in the mt6315 driver, so spinlocks are
> already used for the regmap access.
> 
> Fixes: b45b3ccef8c0 ("spmi: mediatek: Add support for MT6873/8192")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Hi,

gentle ping on this one. MT8195 Chromebooks sometimes boot to a broken state
without it.

Thanks,
Nícolas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] spmi: mtk-pmif: Serialize PMIF status check and command submission

[Alexandre Mergnat]

Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>

On 24/07/2023 17:47, Nícolas F. R. A. Prado wrote:
> Before writing the read or write command to the SPMI arbiter through the
> PMIF interface, the current status of the channel is checked to ensure
> it is idle. However, since the status only changes from idle when the
> command is written, it is possible for two concurrent calls to determine
> that the channel is idle and simultaneously send their commands. At this
> point the PMIF interface hangs, with the status register no longer being
> updated, and thus causing all subsequent operations to time out.
> 
> This was observed on the mt8195-cherry-tomato-r2 machine, particularly
> after commit 46600ab142f8 ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for
> drivers between 5.10 and 5.15") was applied, since then the two MT6315
> devices present on the SPMI bus would probe assynchronously and
> sometimes (during probe or at a later point) read the bus
> simultaneously, breaking the PMIF interface and consequently slowing
> down the whole system.
> 
> To fix the issue at its root cause, introduce locking around the channel
> status check and the command write, so that both become an atomic
> operation, preventing race conditions between two (or more) SPMI bus
> read/write operations. A spinlock is used since this is a fast bus, as
> indicated by the usage of the atomic variant of readl_poll, and
> '.fast_io = true' being used in the mt6315 driver, so spinlocks are
> already used for the regmap access.

-- 
Regards,
Alexandre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] spmi: mtk-pmif: Serialize PMIF status check and command submission

[Nícolas F. R. A. Prado]

On Thu, Aug 10, 2023 at 06:03:53PM -0400, Nícolas F. R. A. Prado wrote:
> On Mon, Jul 24, 2023 at 11:47:33AM -0400, Nícolas F. R. A. Prado wrote:
> > Before writing the read or write command to the SPMI arbiter through the
> > PMIF interface, the current status of the channel is checked to ensure
> > it is idle. However, since the status only changes from idle when the
> > command is written, it is possible for two concurrent calls to determine
> > that the channel is idle and simultaneously send their commands. At this
> > point the PMIF interface hangs, with the status register no longer being
> > updated, and thus causing all subsequent operations to time out.
> > 
> > This was observed on the mt8195-cherry-tomato-r2 machine, particularly
> > after commit 46600ab142f8 ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for
> > drivers between 5.10 and 5.15") was applied, since then the two MT6315
> > devices present on the SPMI bus would probe assynchronously and
> > sometimes (during probe or at a later point) read the bus
> > simultaneously, breaking the PMIF interface and consequently slowing
> > down the whole system.
> > 
> > To fix the issue at its root cause, introduce locking around the channel
> > status check and the command write, so that both become an atomic
> > operation, preventing race conditions between two (or more) SPMI bus
> > read/write operations. A spinlock is used since this is a fast bus, as
> > indicated by the usage of the atomic variant of readl_poll, and
> > '.fast_io = true' being used in the mt6315 driver, so spinlocks are
> > already used for the regmap access.
> > 
> > Fixes: b45b3ccef8c0 ("spmi: mediatek: Add support for MT6873/8192")
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> Hi,
> 
> gentle ping on this one. MT8195 Chromebooks sometimes boot to a broken state
> without it.

Hi Stephen,

once again kindling reminding you of this important fix for MediaTek platforms,
solving a real issue impacting basic functionality observed on the MT8195
Chromebook.

Thanks,
Nícolas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] spmi: mtk-pmif: Serialize PMIF status check and command submission

[AngeloGioacchino Del Regno]

Il 19/09/23 22:12, Nícolas F. R. A. Prado ha scritto:
> On Thu, Aug 10, 2023 at 06:03:53PM -0400, Nícolas F. R. A. Prado wrote:
>> On Mon, Jul 24, 2023 at 11:47:33AM -0400, Nícolas F. R. A. Prado wrote:
>>> Before writing the read or write command to the SPMI arbiter through the
>>> PMIF interface, the current status of the channel is checked to ensure
>>> it is idle. However, since the status only changes from idle when the
>>> command is written, it is possible for two concurrent calls to determine
>>> that the channel is idle and simultaneously send their commands. At this
>>> point the PMIF interface hangs, with the status register no longer being
>>> updated, and thus causing all subsequent operations to time out.
>>>
>>> This was observed on the mt8195-cherry-tomato-r2 machine, particularly
>>> after commit 46600ab142f8 ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for
>>> drivers between 5.10 and 5.15") was applied, since then the two MT6315
>>> devices present on the SPMI bus would probe assynchronously and
>>> sometimes (during probe or at a later point) read the bus
>>> simultaneously, breaking the PMIF interface and consequently slowing
>>> down the whole system.
>>>
>>> To fix the issue at its root cause, introduce locking around the channel
>>> status check and the command write, so that both become an atomic
>>> operation, preventing race conditions between two (or more) SPMI bus
>>> read/write operations. A spinlock is used since this is a fast bus, as
>>> indicated by the usage of the atomic variant of readl_poll, and
>>> '.fast_io = true' being used in the mt6315 driver, so spinlocks are
>>> already used for the regmap access.
>>>
>>> Fixes: b45b3ccef8c0 ("spmi: mediatek: Add support for MT6873/8192")
>>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>
>> Hi,
>>
>> gentle ping on this one. MT8195 Chromebooks sometimes boot to a broken state
>> without it.
> 
> Hi Stephen,
> 
> once again kindling reminding you of this important fix for MediaTek platforms,
> solving a real issue impacting basic functionality observed on the MT8195
> Chromebook.
> 

Hello Stephen,

can you please pick this one ASAP?
MT8195 is broken without.

Thanks,
Angelo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] spmi: mtk-pmif: Serialize PMIF status check and command submission

[Stephen Boyd]

Quoting Nícolas F. R. A. Prado (2023-07-24 08:47:33)
> Before writing the read or write command to the SPMI arbiter through the
> PMIF interface, the current status of the channel is checked to ensure
> it is idle. However, since the status only changes from idle when the
> command is written, it is possible for two concurrent calls to determine
> that the channel is idle and simultaneously send their commands. At this
> point the PMIF interface hangs, with the status register no longer being
> updated, and thus causing all subsequent operations to time out.
> 
> This was observed on the mt8195-cherry-tomato-r2 machine, particularly
> after commit 46600ab142f8 ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for
> drivers between 5.10 and 5.15") was applied, since then the two MT6315
> devices present on the SPMI bus would probe assynchronously and
> sometimes (during probe or at a later point) read the bus
> simultaneously, breaking the PMIF interface and consequently slowing
> down the whole system.
> 
> To fix the issue at its root cause, introduce locking around the channel
> status check and the command write, so that both become an atomic
> operation, preventing race conditions between two (or more) SPMI bus
> read/write operations. A spinlock is used since this is a fast bus, as
> indicated by the usage of the atomic variant of readl_poll, and
> '.fast_io = true' being used in the mt6315 driver, so spinlocks are
> already used for the regmap access.
> 
> Fixes: b45b3ccef8c0 ("spmi: mediatek: Add support for MT6873/8192")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> ---

Applied to spmi-next

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel