| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <69eb8bc7-a0ef-4f33-afbc-a5ff52e6f4f5@ti.com>
Date: Wed, 23 Apr 2025 13:54:44 +0530
From: Beleswar Prasad Padhi <b-padhi@...com>
To: Andrew Davis <afd@...com>, <andersson@...nel.org>,
<mathieu.poirier@...aro.org>
CC: <hnagalla@...com>, <u-kumar1@...com>, <jm@...com>,
<jan.kiszka@...mens.com>, <christophe.jaillet@...adoo.fr>,
<jkangas@...hat.com>, <eballetbo@...hat.com>,
<linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 15/33] remoteproc: k3: Refactor rproc_reset()
implementation into common driver
On 22/04/25 19:51, Andrew Davis wrote:
> On 4/22/25 12:53 AM, Beleswar Prasad Padhi wrote:
>> Hi Andrew,
>>
>> On 21/04/25 20:12, Andrew Davis wrote:
>>> On 4/17/25 1:19 PM, Beleswar Padhi wrote:
>>>> The rproc_reset() implementations in TI K3 DSP and M4 remoteproc drivers
>>>> assert reset in the same way. Refactor the above function into the
>>>> ti_k3_common.c driver as k3_rproc_reset() and use it throughout DSP and
>>>> M4 drivers for resetting the remote processor.
>>>>
>>>> Signed-off-by: Beleswar Padhi <b-padhi@...com>
>>>> ---
>>>> v10: Changelog:
>>>> 1. Split [v9 12/26] into [v10 14/33] and [v10 15/33] patches.
>>>>
>>>> Link to v9:
>>>> https://lore.kernel.org/all/20250317120622.1746415-13-b-padhi@ti.com/
>>>>
>>>> drivers/remoteproc/ti_k3_common.c | 25 ++++++++++++++++++++
>>>> drivers/remoteproc/ti_k3_common.h | 1 +
>>>> drivers/remoteproc/ti_k3_dsp_remoteproc.c | 28 ++---------------------
>>>> drivers/remoteproc/ti_k3_m4_remoteproc.c | 16 +++----------
>>>> 4 files changed, 31 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/ti_k3_common.c b/drivers/remoteproc/ti_k3_common.c
>>>> index aace308b49b0e..19bb6c337af77 100644
>>>> --- a/drivers/remoteproc/ti_k3_common.c
>>>> +++ b/drivers/remoteproc/ti_k3_common.c
>>>> @@ -105,5 +105,30 @@ void k3_rproc_kick(struct rproc *rproc, int vqid)
>>>> }
>>>> EXPORT_SYMBOL_GPL(k3_rproc_kick);
>>>> +/* Put the remote processor into reset */
>>>> +int k3_rproc_reset(struct k3_rproc *kproc)
>>>> +{
>>>> + struct device *dev = kproc->dev;
>>>> + int ret;
>>>> +
>>>> + if (kproc->data->uses_lreset) {
>>>> + ret = reset_control_assert(kproc->reset);
>>>> + if (ret)
>>>> + dev_err(dev, "local-reset assert failed (%pe)\n", ERR_PTR(ret));
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>>>> + kproc->ti_sci_id);
>>>> + if (ret) {
>>>> + dev_err(dev, "module-reset assert failed (%pe)\n", ERR_PTR(ret));
>>>> + if (reset_control_deassert(kproc->reset))
>>>> + dev_warn(dev, "local-reset deassert back failed\n");
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(k3_rproc_reset);
>>>> +
>>>> MODULE_LICENSE("GPL");
>>>> MODULE_DESCRIPTION("TI K3 common Remoteproc code");
>>>> diff --git a/drivers/remoteproc/ti_k3_common.h b/drivers/remoteproc/ti_k3_common.h
>>>> index 6ae7ac4ec5696..f3400fc774766 100644
>>>> --- a/drivers/remoteproc/ti_k3_common.h
>>>> +++ b/drivers/remoteproc/ti_k3_common.h
>>>> @@ -90,4 +90,5 @@ struct k3_rproc {
>>>> void k3_rproc_mbox_callback(struct mbox_client *client, void *data);
>>>> void k3_rproc_kick(struct rproc *rproc, int vqid);
>>>> +int k3_rproc_reset(struct k3_rproc *kproc);
>>>> #endif /* REMOTEPROC_TI_K3_COMMON_H */
>>>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> index 0a8c9e61393d2..f8a5282df5b71 100644
>>>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>>> @@ -24,30 +24,6 @@
>>>> #define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK (SZ_16M - 1)
>>>> -/* Put the DSP processor into reset */
>>>> -static int k3_dsp_rproc_reset(struct k3_rproc *kproc)
>>>> -{
>>>> - struct device *dev = kproc->dev;
>>>> - int ret;
>>>> -
>>>> - if (kproc->data->uses_lreset) {
>>>> - ret = reset_control_assert(kproc->reset);
>>>> - if (ret)
>>>> - dev_err(dev, "local-reset assert failed (%pe)\n", ERR_PTR(ret));
>>>> - return ret;
>>>> - }
>>>> -
>>>> - ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>>>> - kproc->ti_sci_id);
>>>> - if (ret) {
>>>> - dev_err(dev, "module-reset assert failed (%pe)\n", ERR_PTR(ret));
>>>> - if (reset_control_deassert(kproc->reset))
>>>> - dev_warn(dev, "local-reset deassert back failed\n");
>>>> - }
>>>> -
>>>> - return ret;
>>>> -}
>>>> -
>>>> /* Release the DSP processor from reset */
>>>> static int k3_dsp_rproc_release(struct k3_rproc *kproc)
>>>> {
>>>> @@ -201,7 +177,7 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
>>>> {
>>>> struct k3_rproc *kproc = rproc->priv;
>>>> - k3_dsp_rproc_reset(kproc);
>>>> + k3_rproc_reset(kproc);
>>>> return 0;
>>>> }
>>>> @@ -565,7 +541,7 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
>>>> return dev_err_probe(dev, ret, "failed to get reset status\n");
>>>> } else if (ret == 0) {
>>>> dev_warn(dev, "local reset is deasserted for device\n");
>>>> - k3_dsp_rproc_reset(kproc);
>>>> + k3_rproc_reset(kproc);
>>>> }
>>>> }
>>>> }
>>>> diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>>> index 8a6917259ce60..7d5b75be2e4f8 100644
>>>> --- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>>> +++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>>> @@ -65,11 +65,9 @@ static int k3_m4_rproc_prepare(struct rproc *rproc)
>>>> * Ensure the local reset is asserted so the core doesn't
>>>> * execute bogus code when the module reset is released.
>>>> */
>>>> - ret = reset_control_assert(kproc->reset);
>>>> - if (ret) {
>>>> - dev_err(dev, "could not assert local reset\n");
>>>> + ret = k3_rproc_reset(kproc);
>>>> + if (ret)
>>>> return ret;
>>>> - }
>>>> ret = reset_control_status(kproc->reset);
>>>> if (ret <= 0) {
>>>> @@ -374,16 +372,8 @@ static int k3_m4_rproc_start(struct rproc *rproc)
>>>> static int k3_m4_rproc_stop(struct rproc *rproc)
>>>> {
>>>> struct k3_rproc *kproc = rproc->priv;
>>>> - struct device *dev = kproc->dev;
>>>> - int ret;
>>>> - ret = reset_control_assert(kproc->reset);
>>>> - if (ret) {
>>>> - dev_err(dev, "local-reset assert failed, ret = %d\n", ret);
>>>> - return ret;
>>>> - }
>>>> -
>>>> - return 0;
>>>> + return k3_rproc_reset(kproc);
>>>
>>> This doesn't feel right. The new common k3_rproc_reset() function
>>> matches what ti_k3_dsp_remoteproc.c did for reset, you made it that
>>> way in the previous patch [14/33]. But it doesn't match what this
>>> M4 version does (yes I know logically they are the same as `uses_lreset`
>>> will be always true for M4). Maybe you want to do the same as you
>>> did for DSP to the M4 driver first, before you make this change so
>>> it is 100% clear the code is the same (and so bisect lands on the
>>> right patch should someday this be an issue).
>>
>>
>> Sure, I can make that change in next revision...
>>
>>>
>>> Also, the common k3_rproc_reset() calls put_device() unconditionally.
>>> Something that wasn't done at all here in the M4 prepare() and stop()
>>> functions.
>>
>>
>> There is a 'return ret' in the 'if (kproc->data->uses_lreset)' condition flow in k3_rproc_reset().
>>
>> put_device() should not be unconditional...
>>
>
> Ah, I must have looked right past the return statement. So it is one or
> the other, but not both? Might be good to put the put_device() side into
> an `else` block.
Sure I will do that in revision.
>
> Then it seems in the !uses_lreset path you still undo the reset_control_assert()
> by calling reset_control_deassert() if put_device() fails. Think you messed
> this one up in back in [14/33].
My bad. Will address in revision.. Do you have any other review comments for the series before I re-spin?
Thanks,
Beleswar
>
> Andrew
>
>>>
>>> These two changes make this patch not strictly a pure "refactor"
>>> patch, which IMHO should in no way change the calls being made nor
>>> the logical flow, only the code structure.
>>
>>
>> Got it. Will address in revision. Will wait for more reviews (if any) before re-spinning.
>>
>> Thanks,
>> Beleswar
>>
>>>
>>> Andrew
>>>
>>>> }
>>>> /*
Powered by blists - more mailing lists