diff mbox series

Revert "remoteproc: core: Clear table_sz when rproc_shutdown"

Message ID 20250513-revert-rproc-table-sz-v1-1-a8c6b5d6f8a7@kernel.org
State New
Headers show
Series Revert "remoteproc: core: Clear table_sz when rproc_shutdown" | expand

Commit Message

Bjorn Andersson May 13, 2025, 3:52 p.m. UTC
Clearing the table_sz on cleanup seemed reasonable, but further
discussions concluded that this merely working around the issue
and that the fix is incomplete.

As such, revert commit efdde3d73ab2 ("remoteproc: core: Clear table_sz
when rproc_shutdown") to avoid carrying a partial fix.

Signed-off-by: Bjorn Andersson <andersson@kernel.org>
---
 drivers/remoteproc/remoteproc_core.c | 1 -
 1 file changed, 1 deletion(-)


---
base-commit: aa94665adc28f3fdc3de2979ac1e98bae961d6ca
change-id: 20250513-revert-rproc-table-sz-53ecf24726ae

Best regards,

Comments

Andrew Davis May 15, 2025, 5:21 p.m. UTC | #1
On 5/13/25 10:52 AM, Bjorn Andersson wrote:
> Clearing the table_sz on cleanup seemed reasonable, but further
> discussions concluded that this merely working around the issue
> and that the fix is incomplete.
> 
> As such, revert commit efdde3d73ab2 ("remoteproc: core: Clear table_sz
> when rproc_shutdown") to avoid carrying a partial fix.
> 

Setting table_sz to 0 still seems like a good idea from a defensive
programming perspective. Both table_ptr and table_sz should be set
and cleared together in all spots.

In addition to this, another fix would be to also update
both table_ptr and table_sz to 0 when loading firmware without
a resource table. Both should be done, no need to revert this.

Andrew

> Signed-off-by: Bjorn Andersson <andersson@kernel.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 48d146e1fa560397c11eeb8f824ae0fb844a022b..81b2ccf988e852ac79cee375c7e3f118c2a4b41a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2025,7 +2025,6 @@ int rproc_shutdown(struct rproc *rproc)
>   	kfree(rproc->cached_table);
>   	rproc->cached_table = NULL;
>   	rproc->table_ptr = NULL;
> -	rproc->table_sz = 0;
>   out:
>   	mutex_unlock(&rproc->lock);
>   	return ret;
> 
> ---
> base-commit: aa94665adc28f3fdc3de2979ac1e98bae961d6ca
> change-id: 20250513-revert-rproc-table-sz-53ecf24726ae
> 
> Best regards,
Mathieu Poirier May 15, 2025, 5:57 p.m. UTC | #2
On Thu, May 15, 2025 at 12:21:14PM -0500, Andrew Davis wrote:
> On 5/13/25 10:52 AM, Bjorn Andersson wrote:
> > Clearing the table_sz on cleanup seemed reasonable, but further
> > discussions concluded that this merely working around the issue
> > and that the fix is incomplete.
> > 
> > As such, revert commit efdde3d73ab2 ("remoteproc: core: Clear table_sz
> > when rproc_shutdown") to avoid carrying a partial fix.
> > 
> 
> Setting table_sz to 0 still seems like a good idea from a defensive
> programming perspective. Both table_ptr and table_sz should be set
> and cleared together in all spots.
> 
> In addition to this, another fix would be to also update
> both table_ptr and table_sz to 0 when loading firmware without
> a resource table. Both should be done, no need to revert this.
>

As mentioned by Bjorn, this is a partial fix.  I'm all good with setting
table_sz to 0, but as long as the real solution to the problem is part of the
same work.  Once the patch is reverted, which I'm about to apply, work can
continue.

> Andrew
> 
> > Signed-off-by: Bjorn Andersson <andersson@kernel.org>
> > ---
> >   drivers/remoteproc/remoteproc_core.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 48d146e1fa560397c11eeb8f824ae0fb844a022b..81b2ccf988e852ac79cee375c7e3f118c2a4b41a 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2025,7 +2025,6 @@ int rproc_shutdown(struct rproc *rproc)
> >   	kfree(rproc->cached_table);
> >   	rproc->cached_table = NULL;
> >   	rproc->table_ptr = NULL;
> > -	rproc->table_sz = 0;
> >   out:
> >   	mutex_unlock(&rproc->lock);
> >   	return ret;
> > 
> > ---
> > base-commit: aa94665adc28f3fdc3de2979ac1e98bae961d6ca
> > change-id: 20250513-revert-rproc-table-sz-53ecf24726ae
> > 
> > Best regards,
Mathieu Poirier May 15, 2025, 7:16 p.m. UTC | #3
On Tue, May 13, 2025 at 04:52:46PM +0100, Bjorn Andersson wrote:
> Clearing the table_sz on cleanup seemed reasonable, but further
> discussions concluded that this merely working around the issue
> and that the fix is incomplete.
> 
> As such, revert commit efdde3d73ab2 ("remoteproc: core: Clear table_sz
> when rproc_shutdown") to avoid carrying a partial fix.
> 
> Signed-off-by: Bjorn Andersson <andersson@kernel.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 1 -
>  1 file changed, 1 deletion(-)
>

I have applied this patch.

Thanks,
Mathieu

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 48d146e1fa560397c11eeb8f824ae0fb844a022b..81b2ccf988e852ac79cee375c7e3f118c2a4b41a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2025,7 +2025,6 @@ int rproc_shutdown(struct rproc *rproc)
>  	kfree(rproc->cached_table);
>  	rproc->cached_table = NULL;
>  	rproc->table_ptr = NULL;
> -	rproc->table_sz = 0;
>  out:
>  	mutex_unlock(&rproc->lock);
>  	return ret;
> 
> ---
> base-commit: aa94665adc28f3fdc3de2979ac1e98bae961d6ca
> change-id: 20250513-revert-rproc-table-sz-53ecf24726ae
> 
> Best regards,
> -- 
> Bjorn Andersson <andersson@kernel.org>
>
diff mbox series

Patch

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 48d146e1fa560397c11eeb8f824ae0fb844a022b..81b2ccf988e852ac79cee375c7e3f118c2a4b41a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2025,7 +2025,6 @@  int rproc_shutdown(struct rproc *rproc)
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
 	rproc->table_ptr = NULL;
-	rproc->table_sz = 0;
 out:
 	mutex_unlock(&rproc->lock);
 	return ret;