[PATCH 0/9] remoteproc: Allow platform-specific drivers to request resources

[PATCH 0/9] remoteproc: Allow platform-specific drivers to request resources

[Lee Jones]

Once this patch-set has been applied; platform-specific remoteproc
drivers will be able to amend existing resource table entries,
provide new entries to be appended to an existing resource table
(if one already exists), start a new resource table (if one does
not already exist), and dump out resource table contents during
development/debugging.  It will also be possible to start remote
processors which do not require a resource table without being
forced to provide a fake one.

We're also taking the liberty to provide some simple fix-ups.

Lee Jones (9):
  remoteproc: core: Ensure error message is clear
  remoteproc: core: Trivial: Improve error checking, spelling and debug
    prints
  remoteproc: core: Remove pointless OOM print
  remoteproc: core: New API to add new resources to the resource table
  remoteproc: core: Add function to amend an existing resource table
    entry
  remoteproc: core: Add function to append a new resource table entry
  remoteproc: core: Add function to over-ride current resource table
  remoteproc: core: Skip resource table integrity checks if there are
    amendments
  remoteproc: core: Support empty resource tables

 drivers/remoteproc/remoteproc_core.c | 354 +++++++++++++++++++++++++++++++++--
 include/linux/remoteproc.h           |  21 +++
 2 files changed, 358 insertions(+), 17 deletions(-)

-- 
2.9.0

[PATCH 0/9] remoteproc: Allow platform-specific drivers to request resources

[Lee Jones]

Once this patch-set has been applied; platform-specific remoteproc
drivers will be able to amend existing resource table entries,
provide new entries to be appended to an existing resource table
(if one already exists), start a new resource table (if one does
not already exist), and dump out resource table contents during
development/debugging.  It will also be possible to start remote
processors which do not require a resource table without being
forced to provide a fake one.

We're also taking the liberty to provide some simple fix-ups.

Lee Jones (9):
  remoteproc: core: Ensure error message is clear
  remoteproc: core: Trivial: Improve error checking, spelling and debug
    prints
  remoteproc: core: Remove pointless OOM print
  remoteproc: core: New API to add new resources to the resource table
  remoteproc: core: Add function to amend an existing resource table
    entry
  remoteproc: core: Add function to append a new resource table entry
  remoteproc: core: Add function to over-ride current resource table
  remoteproc: core: Skip resource table integrity checks if there are
    amendments
  remoteproc: core: Support empty resource tables

 drivers/remoteproc/remoteproc_core.c | 354 +++++++++++++++++++++++++++++++++--
 include/linux/remoteproc.h           |  21 +++
 2 files changed, 358 insertions(+), 17 deletions(-)

-- 
2.9.0

[PATCH 1/9] remoteproc: core: Ensure error message is clear

[Lee Jones]

Before this patch, the dma_alloc_coherent() failure path printed out:

  "dma_alloc_coherent err: 16760832"

... alluding to the Linux error code being 16760832, but seeing as
Linux error codes are all negative, this looks like a signed/unsigned
issue.  In fact, the message is trying to print the length of the
requested memory region.  Let's clear that up.

While we're at it, let's standardise the way 'len' is printed.  In
all other locations 'len' is in hex prefixed by a '0x' for clarity.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index aea29a75c..3566dc9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		return -EINVAL;
 	}
 
-	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
+	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
 			rsc->da, rsc->pa, rsc->len, rsc->flags);
 
 	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
@@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
 
 	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
-		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
+		dev_err(dev->parent,
+			"failed to allocate dma memory: len 0x%x\n", rsc->len);
 		ret = -ENOMEM;
 		goto free_carv;
 	}
-- 
2.9.0

[PATCH 1/9] remoteproc: core: Ensure error message is clear

[Lee Jones]

Before this patch, the dma_alloc_coherent() failure path printed out:

  "dma_alloc_coherent err: 16760832"

... alluding to the Linux error code being 16760832, but seeing as
Linux error codes are all negative, this looks like a signed/unsigned
issue.  In fact, the message is trying to print the length of the
requested memory region.  Let's clear that up.

While we're at it, let's standardise the way 'len' is printed.  In
all other locations 'len' is in hex prefixed by a '0x' for clarity.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index aea29a75c..3566dc9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		return -EINVAL;
 	}
 
-	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
+	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
 			rsc->da, rsc->pa, rsc->len, rsc->flags);
 
 	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
@@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
 
 	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
-		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
+		dev_err(dev->parent,
+			"failed to allocate dma memory: len 0x%x\n", rsc->len);
 		ret = -ENOMEM;
 		goto free_carv;
 	}
-- 
2.9.0

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Bjorn Andersson]

On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:

> Before this patch, the dma_alloc_coherent() failure path printed out:
> 
>   "dma_alloc_coherent err: 16760832"
> 
> ... alluding to the Linux error code being 16760832, but seeing as
> Linux error codes are all negative, this looks like a signed/unsigned
> issue.  In fact, the message is trying to print the length of the
> requested memory region.  Let's clear that up.
> 
> While we're at it, let's standardise the way 'len' is printed.  In
> all other locations 'len' is in hex prefixed by a '0x' for clarity.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index aea29a75c..3566dc9 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  		return -EINVAL;
>  	}
>  
> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
>  
>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  
>  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
>  	if (!va) {
> -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
> +		dev_err(dev->parent,
> +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
>  		ret = -ENOMEM;
>  		goto free_carv;
>  	}
> -- 
> 2.9.0
> 

[PATCH 1/9] remoteproc: core: Ensure error message is clear

[Bjorn Andersson]

On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:

> Before this patch, the dma_alloc_coherent() failure path printed out:
> 
>   "dma_alloc_coherent err: 16760832"
> 
> ... alluding to the Linux error code being 16760832, but seeing as
> Linux error codes are all negative, this looks like a signed/unsigned
> issue.  In fact, the message is trying to print the length of the
> requested memory region.  Let's clear that up.
> 
> While we're at it, let's standardise the way 'len' is printed.  In
> all other locations 'len' is in hex prefixed by a '0x' for clarity.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index aea29a75c..3566dc9 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  		return -EINVAL;
>  	}
>  
> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
>  
>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  
>  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
>  	if (!va) {
> -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
> +		dev_err(dev->parent,
> +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
>  		ret = -ENOMEM;
>  		goto free_carv;
>  	}
> -- 
> 2.9.0
> 

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Lee Jones]

On Tue, 09 Aug 2016, Bjorn Andersson wrote:

> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> 
> > Before this patch, the dma_alloc_coherent() failure path printed out:
> > 
> >   "dma_alloc_coherent err: 16760832"
> > 
> > ... alluding to the Linux error code being 16760832, but seeing as
> > Linux error codes are all negative, this looks like a signed/unsigned
> > issue.  In fact, the message is trying to print the length of the
> > requested memory region.  Let's clear that up.
> > 
> > While we're at it, let's standardise the way 'len' is printed.  In
> > all other locations 'len' is in hex prefixed by a '0x' for clarity.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Again, this can just be applied.

> > ---
> >  drivers/remoteproc/remoteproc_core.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index aea29a75c..3566dc9 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >  		return -EINVAL;
> >  	}
> >  
> > -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> > +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> >  			rsc->da, rsc->pa, rsc->len, rsc->flags);
> >  
> >  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> > @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >  
> >  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
> >  	if (!va) {
> > -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
> > +		dev_err(dev->parent,
> > +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
> >  		ret = -ENOMEM;
> >  		goto free_carv;
> >  	}

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/9] remoteproc: core: Ensure error message is clear

[Lee Jones]

On Tue, 09 Aug 2016, Bjorn Andersson wrote:

> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> 
> > Before this patch, the dma_alloc_coherent() failure path printed out:
> > 
> >   "dma_alloc_coherent err: 16760832"
> > 
> > ... alluding to the Linux error code being 16760832, but seeing as
> > Linux error codes are all negative, this looks like a signed/unsigned
> > issue.  In fact, the message is trying to print the length of the
> > requested memory region.  Let's clear that up.
> > 
> > While we're at it, let's standardise the way 'len' is printed.  In
> > all other locations 'len' is in hex prefixed by a '0x' for clarity.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Again, this can just be applied.

> > ---
> >  drivers/remoteproc/remoteproc_core.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index aea29a75c..3566dc9 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >  		return -EINVAL;
> >  	}
> >  
> > -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> > +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> >  			rsc->da, rsc->pa, rsc->len, rsc->flags);
> >  
> >  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> > @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >  
> >  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
> >  	if (!va) {
> > -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
> > +		dev_err(dev->parent,
> > +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
> >  		ret = -ENOMEM;
> >  		goto free_carv;
> >  	}

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Suman Anna]

On 08/09/2016 01:12 PM, Lee Jones wrote:
> On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> 
>> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
>>
>>> Before this patch, the dma_alloc_coherent() failure path printed out:
>>>
>>>   "dma_alloc_coherent err: 16760832"
>>>
>>> ... alluding to the Linux error code being 16760832, but seeing as
>>> Linux error codes are all negative, this looks like a signed/unsigned
>>> issue.  In fact, the message is trying to print the length of the
>>> requested memory region.  Let's clear that up.
>>>
>>> While we're at it, let's standardise the way 'len' is printed.  In
>>> all other locations 'len' is in hex prefixed by a '0x' for clarity.
>>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>
>> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Again, this can just be applied.
> 
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>> index aea29a75c..3566dc9 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
>>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
>>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);

If you are modifying this trace, it's better to following the leading 0x
convention on all arguments rather than just the length.

regards
Suman

>>>  
>>>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
>>> @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>>  
>>>  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
>>>  	if (!va) {
>>> -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
>>> +		dev_err(dev->parent,
>>> +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
>>>  		ret = -ENOMEM;
>>>  		goto free_carv;
>>>  	}
> 

[PATCH 1/9] remoteproc: core: Ensure error message is clear

[Suman Anna]

On 08/09/2016 01:12 PM, Lee Jones wrote:
> On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> 
>> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
>>
>>> Before this patch, the dma_alloc_coherent() failure path printed out:
>>>
>>>   "dma_alloc_coherent err: 16760832"
>>>
>>> ... alluding to the Linux error code being 16760832, but seeing as
>>> Linux error codes are all negative, this looks like a signed/unsigned
>>> issue.  In fact, the message is trying to print the length of the
>>> requested memory region.  Let's clear that up.
>>>
>>> While we're at it, let's standardise the way 'len' is printed.  In
>>> all other locations 'len' is in hex prefixed by a '0x' for clarity.
>>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>
>> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Again, this can just be applied.
> 
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>> index aea29a75c..3566dc9 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
>>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
>>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);

If you are modifying this trace, it's better to following the leading 0x
convention on all arguments rather than just the length.

regards
Suman

>>>  
>>>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
>>> @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>>  
>>>  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
>>>  	if (!va) {
>>> -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
>>> +		dev_err(dev->parent,
>>> +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
>>>  		ret = -ENOMEM;
>>>  		goto free_carv;
>>>  	}
> 

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Suman Anna]

On 08/09/2016 01:12 PM, Lee Jones wrote:
> On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> 
>> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
>>
>>> Before this patch, the dma_alloc_coherent() failure path printed out:
>>>
>>>   "dma_alloc_coherent err: 16760832"
>>>
>>> ... alluding to the Linux error code being 16760832, but seeing as
>>> Linux error codes are all negative, this looks like a signed/unsigned
>>> issue.  In fact, the message is trying to print the length of the
>>> requested memory region.  Let's clear that up.
>>>
>>> While we're at it, let's standardise the way 'len' is printed.  In
>>> all other locations 'len' is in hex prefixed by a '0x' for clarity.
>>>
>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>
>> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Again, this can just be applied.
> 
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>> index aea29a75c..3566dc9 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
>>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
>>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);

If you are modifying this trace, it's better to following the leading 0x
convention on all arguments rather than just the length.

regards
Suman

>>>  
>>>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
>>> @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>>  
>>>  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
>>>  	if (!va) {
>>> -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
>>> +		dev_err(dev->parent,
>>> +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
>>>  		ret = -ENOMEM;
>>>  		goto free_carv;
>>>  	}
> 

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Lee Jones]

On Wed, 10 Aug 2016, Suman Anna wrote:

> On 08/09/2016 01:12 PM, Lee Jones wrote:
> > On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> > 
> >> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> >>
> >>> Before this patch, the dma_alloc_coherent() failure path printed out:
> >>>
> >>>   "dma_alloc_coherent err: 16760832"
> >>>
> >>> ... alluding to the Linux error code being 16760832, but seeing as
> >>> Linux error codes are all negative, this looks like a signed/unsigned
> >>> issue.  In fact, the message is trying to print the length of the
> >>> requested memory region.  Let's clear that up.
> >>>
> >>> While we're at it, let's standardise the way 'len' is printed.  In
> >>> all other locations 'len' is in hex prefixed by a '0x' for clarity.
> >>>
> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >>
> >> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Again, this can just be applied.
> > 
> >>> ---
> >>>  drivers/remoteproc/remoteproc_core.c | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >>> index aea29a75c..3566dc9 100644
> >>> --- a/drivers/remoteproc/remoteproc_core.c
> >>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>> @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >>>  		return -EINVAL;
> >>>  	}
> >>>  
> >>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> >>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> >>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
> 
> If you are modifying this trace, it's better to following the leading 0x
> convention on all arguments rather than just the length.

I'd be concerned if anyone thought it a good idea to print out memory
addresses !0x.  The length is the only parameter there which could
(and has) cause confusion.

However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
the final decision to Bjorn and fix-up if he sees it necessary.

> >>>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> >>> @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >>>  
> >>>  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
> >>>  	if (!va) {
> >>> -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
> >>> +		dev_err(dev->parent,
> >>> +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
> >>>  		ret = -ENOMEM;
> >>>  		goto free_carv;
> >>>  	}
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 1/9] remoteproc: core: Ensure error message is clear

[Lee Jones]

On Wed, 10 Aug 2016, Suman Anna wrote:

> On 08/09/2016 01:12 PM, Lee Jones wrote:
> > On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> > 
> >> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> >>
> >>> Before this patch, the dma_alloc_coherent() failure path printed out:
> >>>
> >>>   "dma_alloc_coherent err: 16760832"
> >>>
> >>> ... alluding to the Linux error code being 16760832, but seeing as
> >>> Linux error codes are all negative, this looks like a signed/unsigned
> >>> issue.  In fact, the message is trying to print the length of the
> >>> requested memory region.  Let's clear that up.
> >>>
> >>> While we're at it, let's standardise the way 'len' is printed.  In
> >>> all other locations 'len' is in hex prefixed by a '0x' for clarity.
> >>>
> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >>
> >> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Again, this can just be applied.
> > 
> >>> ---
> >>>  drivers/remoteproc/remoteproc_core.c | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >>> index aea29a75c..3566dc9 100644
> >>> --- a/drivers/remoteproc/remoteproc_core.c
> >>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>> @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >>>  		return -EINVAL;
> >>>  	}
> >>>  
> >>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> >>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> >>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
> 
> If you are modifying this trace, it's better to following the leading 0x
> convention on all arguments rather than just the length.

I'd be concerned if anyone thought it a good idea to print out memory
addresses !0x.  The length is the only parameter there which could
(and has) cause confusion.

However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
the final decision to Bjorn and fix-up if he sees it necessary.

> >>>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> >>> @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >>>  
> >>>  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
> >>>  	if (!va) {
> >>> -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
> >>> +		dev_err(dev->parent,
> >>> +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
> >>>  		ret = -ENOMEM;
> >>>  		goto free_carv;
> >>>  	}
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Lee Jones]

On Wed, 10 Aug 2016, Suman Anna wrote:

> On 08/09/2016 01:12 PM, Lee Jones wrote:
> > On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> > 
> >> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> >>
> >>> Before this patch, the dma_alloc_coherent() failure path printed out:
> >>>
> >>>   "dma_alloc_coherent err: 16760832"
> >>>
> >>> ... alluding to the Linux error code being 16760832, but seeing as
> >>> Linux error codes are all negative, this looks like a signed/unsigned
> >>> issue.  In fact, the message is trying to print the length of the
> >>> requested memory region.  Let's clear that up.
> >>>
> >>> While we're at it, let's standardise the way 'len' is printed.  In
> >>> all other locations 'len' is in hex prefixed by a '0x' for clarity.
> >>>
> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >>
> >> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Again, this can just be applied.
> > 
> >>> ---
> >>>  drivers/remoteproc/remoteproc_core.c | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >>> index aea29a75c..3566dc9 100644
> >>> --- a/drivers/remoteproc/remoteproc_core.c
> >>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>> @@ -581,7 +581,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >>>  		return -EINVAL;
> >>>  	}
> >>>  
> >>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> >>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> >>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
> 
> If you are modifying this trace, it's better to following the leading 0x
> convention on all arguments rather than just the length.

I'd be concerned if anyone thought it a good idea to print out memory
addresses !0x.  The length is the only parameter there which could
(and has) cause confusion.

However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
the final decision to Bjorn and fix-up if he sees it necessary.

> >>>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> >>> @@ -590,7 +590,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >>>  
> >>>  	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
> >>>  	if (!va) {
> >>> -		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
> >>> +		dev_err(dev->parent,
> >>> +			"failed to allocate dma memory: len 0x%x\n", rsc->len);
> >>>  		ret = -ENOMEM;
> >>>  		goto free_carv;
> >>>  	}
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Bjorn Andersson]

On Thu 11 Aug 00:36 PDT 2016, Lee Jones wrote:

> On Wed, 10 Aug 2016, Suman Anna wrote:
> 
> > On 08/09/2016 01:12 PM, Lee Jones wrote:
> > > On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> > > 
> > >> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
[..]
> > >>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> > >>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> > >>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
> > 
> > If you are modifying this trace, it's better to following the leading 0x
> > convention on all arguments rather than just the length.
> 
> I'd be concerned if anyone thought it a good idea to print out memory
> addresses !0x.  The length is the only parameter there which could
> (and has) cause confusion.
> 
> However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
> the final decision to Bjorn and fix-up if he sees it necessary.
> 

I applied this patch yesterday, I see both changes here related to the
issue presented (confusing printing of len) so I'm fine with not
touching the other parts.

That said, there are a few other format strings in this driver that
would benefit from some love as well, several cases where we should use
%pK or %pad rather than using %llx and a cast to unsigned long long.

Regards,
Bjorn

[PATCH 1/9] remoteproc: core: Ensure error message is clear

[Bjorn Andersson]

On Thu 11 Aug 00:36 PDT 2016, Lee Jones wrote:

> On Wed, 10 Aug 2016, Suman Anna wrote:
> 
> > On 08/09/2016 01:12 PM, Lee Jones wrote:
> > > On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> > > 
> > >> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
[..]
> > >>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> > >>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> > >>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
> > 
> > If you are modifying this trace, it's better to following the leading 0x
> > convention on all arguments rather than just the length.
> 
> I'd be concerned if anyone thought it a good idea to print out memory
> addresses !0x.  The length is the only parameter there which could
> (and has) cause confusion.
> 
> However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
> the final decision to Bjorn and fix-up if he sees it necessary.
> 

I applied this patch yesterday, I see both changes here related to the
issue presented (confusing printing of len) so I'm fine with not
touching the other parts.

That said, there are a few other format strings in this driver that
would benefit from some love as well, several cases where we should use
%pK or %pad rather than using %llx and a cast to unsigned long long.

Regards,
Bjorn

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Bjorn Andersson]

On Thu 11 Aug 00:36 PDT 2016, Lee Jones wrote:

> On Wed, 10 Aug 2016, Suman Anna wrote:
> 
> > On 08/09/2016 01:12 PM, Lee Jones wrote:
> > > On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> > > 
> > >> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
[..]
> > >>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
> > >>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> > >>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
> > 
> > If you are modifying this trace, it's better to following the leading 0x
> > convention on all arguments rather than just the length.
> 
> I'd be concerned if anyone thought it a good idea to print out memory
> addresses !0x.  The length is the only parameter there which could
> (and has) cause confusion.
> 
> However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
> the final decision to Bjorn and fix-up if he sees it necessary.
> 

I applied this patch yesterday, I see both changes here related to the
issue presented (confusing printing of len) so I'm fine with not
touching the other parts.

That said, there are a few other format strings in this driver that
would benefit from some love as well, several cases where we should use
%pK or %pad rather than using %llx and a cast to unsigned long long.

Regards,
Bjorn

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Suman Anna]

On 08/11/2016 02:19 PM, Bjorn Andersson wrote:
> On Thu 11 Aug 00:36 PDT 2016, Lee Jones wrote:
> 
>> On Wed, 10 Aug 2016, Suman Anna wrote:
>>
>>> On 08/09/2016 01:12 PM, Lee Jones wrote:
>>>> On Tue, 09 Aug 2016, Bjorn Andersson wrote:
>>>>
>>>>> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> [..]
>>>>>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
>>>>>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
>>>>>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
>>>
>>> If you are modifying this trace, it's better to following the leading 0x
>>> convention on all arguments rather than just the length.
>>
>> I'd be concerned if anyone thought it a good idea to print out memory
>> addresses !0x.  The length is the only parameter there which could
>> (and has) cause confusion.
>>
>> However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
>> the final decision to Bjorn and fix-up if he sees it necessary.
>>
> 
> I applied this patch yesterday, I see both changes here related to the
> issue presented (confusing printing of len) so I'm fine with not
> touching the other parts.
> 
> That said, there are a few other format strings in this driver that
> would benefit from some love as well, several cases where we should use
> %pK or %pad rather than using %llx and a cast to unsigned long long.

OK, no issues. I have the patches for some of these handy, will post
them soon.

regards
Suman

[PATCH 1/9] remoteproc: core: Ensure error message is clear

[Suman Anna]

On 08/11/2016 02:19 PM, Bjorn Andersson wrote:
> On Thu 11 Aug 00:36 PDT 2016, Lee Jones wrote:
> 
>> On Wed, 10 Aug 2016, Suman Anna wrote:
>>
>>> On 08/09/2016 01:12 PM, Lee Jones wrote:
>>>> On Tue, 09 Aug 2016, Bjorn Andersson wrote:
>>>>
>>>>> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> [..]
>>>>>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
>>>>>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
>>>>>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
>>>
>>> If you are modifying this trace, it's better to following the leading 0x
>>> convention on all arguments rather than just the length.
>>
>> I'd be concerned if anyone thought it a good idea to print out memory
>> addresses !0x.  The length is the only parameter there which could
>> (and has) cause confusion.
>>
>> However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
>> the final decision to Bjorn and fix-up if he sees it necessary.
>>
> 
> I applied this patch yesterday, I see both changes here related to the
> issue presented (confusing printing of len) so I'm fine with not
> touching the other parts.
> 
> That said, there are a few other format strings in this driver that
> would benefit from some love as well, several cases where we should use
> %pK or %pad rather than using %llx and a cast to unsigned long long.

OK, no issues. I have the patches for some of these handy, will post
them soon.

regards
Suman

Re: [PATCH 1/9] remoteproc: core: Ensure error message is clear

[Suman Anna]

On 08/11/2016 02:19 PM, Bjorn Andersson wrote:
> On Thu 11 Aug 00:36 PDT 2016, Lee Jones wrote:
> 
>> On Wed, 10 Aug 2016, Suman Anna wrote:
>>
>>> On 08/09/2016 01:12 PM, Lee Jones wrote:
>>>> On Tue, 09 Aug 2016, Bjorn Andersson wrote:
>>>>
>>>>> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> [..]
>>>>>> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
>>>>>> +	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
>>>>>>  			rsc->da, rsc->pa, rsc->len, rsc->flags);
>>>
>>> If you are modifying this trace, it's better to following the leading 0x
>>> convention on all arguments rather than just the length.
>>
>> I'd be concerned if anyone thought it a good idea to print out memory
>> addresses !0x.  The length is the only parameter there which could
>> (and has) cause confusion.
>>
>> However, the fix-up is trivial, so I'm happy to oblige.  I'll leave
>> the final decision to Bjorn and fix-up if he sees it necessary.
>>
> 
> I applied this patch yesterday, I see both changes here related to the
> issue presented (confusing printing of len) so I'm fine with not
> touching the other parts.
> 
> That said, there are a few other format strings in this driver that
> would benefit from some love as well, several cases where we should use
> %pK or %pad rather than using %llx and a cast to unsigned long long.

OK, no issues. I have the patches for some of these handy, will post
them soon.

regards
Suman

[PATCH 2/9] remoteproc: core: Trivial: Improve error checking, spelling and debug prints

[Lee Jones]

Trivial patch to clean up a couple of minor misgivings.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3566dc9..5654a81 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -457,8 +457,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
 
 	rproc->num_traces++;
 
-	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n", name, ptr,
-						rsc->da, rsc->len);
+	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n",
+		name, ptr, rsc->da, rsc->len);
 
 	return 0;
 }
@@ -581,8 +581,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		return -EINVAL;
 	}
 
-	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
-			rsc->da, rsc->pa, rsc->len, rsc->flags);
+	dev_dbg(dev, "carveout rsc: name: %s, da %x, pa %x, len 0x%x, flags %x\n",
+		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
 
 	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
 	if (!carveout)
@@ -700,7 +700,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
 	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
 	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
 	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
-	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
+	[RSC_VDEV] = NULL, /* VDEVs were handled upon registration */
 };
 
 static rproc_handle_resource_t rproc_vdev_handler[RSC_LAST] = {
@@ -918,7 +918,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	 * Create a copy of the resource table. When a virtio device starts
 	 * and calls vring_new_virtqueue() the address of the allocated vring
 	 * will be stored in the cached_table. Before the device is started,
-	 * cached_table will be copied into devic memory.
+	 * cached_table will be copied into device memory.
 	 */
 	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
 	if (!rproc->cached_table)
-- 
2.9.0

[PATCH 2/9] remoteproc: core: Trivial: Improve error checking, spelling and debug prints

[Lee Jones]

Trivial patch to clean up a couple of minor misgivings.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3566dc9..5654a81 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -457,8 +457,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
 
 	rproc->num_traces++;
 
-	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n", name, ptr,
-						rsc->da, rsc->len);
+	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n",
+		name, ptr, rsc->da, rsc->len);
 
 	return 0;
 }
@@ -581,8 +581,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		return -EINVAL;
 	}
 
-	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
-			rsc->da, rsc->pa, rsc->len, rsc->flags);
+	dev_dbg(dev, "carveout rsc: name: %s, da %x, pa %x, len 0x%x, flags %x\n",
+		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
 
 	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
 	if (!carveout)
@@ -700,7 +700,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
 	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
 	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
 	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
-	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
+	[RSC_VDEV] = NULL, /* VDEVs were handled upon registration */
 };
 
 static rproc_handle_resource_t rproc_vdev_handler[RSC_LAST] = {
@@ -918,7 +918,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	 * Create a copy of the resource table. When a virtio device starts
 	 * and calls vring_new_virtqueue() the address of the allocated vring
 	 * will be stored in the cached_table. Before the device is started,
-	 * cached_table will be copied into devic memory.
+	 * cached_table will be copied into device memory.
 	 */
 	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
 	if (!rproc->cached_table)
-- 
2.9.0

Re: [PATCH 2/9] remoteproc: core: Trivial: Improve error checking, spelling and debug prints

[Bjorn Andersson]

On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:

> Trivial patch to clean up a couple of minor misgivings.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Although as I hope to stack this series ontop of the auto-boot there
will be some minor conflicts here.

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3566dc9..5654a81 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -457,8 +457,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>  
>  	rproc->num_traces++;
>  
> -	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n", name, ptr,
> -						rsc->da, rsc->len);
> +	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n",
> +		name, ptr, rsc->da, rsc->len);
>  
>  	return 0;
>  }
> @@ -581,8 +581,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  		return -EINVAL;
>  	}
>  
> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> -			rsc->da, rsc->pa, rsc->len, rsc->flags);
> +	dev_dbg(dev, "carveout rsc: name: %s, da %x, pa %x, len 0x%x, flags %x\n",
> +		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
>  
>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
>  	if (!carveout)
> @@ -700,7 +700,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
>  	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
>  	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
>  	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
> -	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
> +	[RSC_VDEV] = NULL, /* VDEVs were handled upon registration */
>  };
>  
>  static rproc_handle_resource_t rproc_vdev_handler[RSC_LAST] = {
> @@ -918,7 +918,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>  	 * Create a copy of the resource table. When a virtio device starts
>  	 * and calls vring_new_virtqueue() the address of the allocated vring
>  	 * will be stored in the cached_table. Before the device is started,
> -	 * cached_table will be copied into devic memory.
> +	 * cached_table will be copied into device memory.
>  	 */
>  	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
>  	if (!rproc->cached_table)
> -- 
> 2.9.0
> 

[PATCH 2/9] remoteproc: core: Trivial: Improve error checking, spelling and debug prints

[Bjorn Andersson]

On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:

> Trivial patch to clean up a couple of minor misgivings.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Although as I hope to stack this series ontop of the auto-boot there
will be some minor conflicts here.

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3566dc9..5654a81 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -457,8 +457,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
>  
>  	rproc->num_traces++;
>  
> -	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n", name, ptr,
> -						rsc->da, rsc->len);
> +	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n",
> +		name, ptr, rsc->da, rsc->len);
>  
>  	return 0;
>  }
> @@ -581,8 +581,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  		return -EINVAL;
>  	}
>  
> -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> -			rsc->da, rsc->pa, rsc->len, rsc->flags);
> +	dev_dbg(dev, "carveout rsc: name: %s, da %x, pa %x, len 0x%x, flags %x\n",
> +		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
>  
>  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
>  	if (!carveout)
> @@ -700,7 +700,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
>  	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
>  	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
>  	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
> -	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
> +	[RSC_VDEV] = NULL, /* VDEVs were handled upon registration */
>  };
>  
>  static rproc_handle_resource_t rproc_vdev_handler[RSC_LAST] = {
> @@ -918,7 +918,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>  	 * Create a copy of the resource table. When a virtio device starts
>  	 * and calls vring_new_virtqueue() the address of the allocated vring
>  	 * will be stored in the cached_table. Before the device is started,
> -	 * cached_table will be copied into devic memory.
> +	 * cached_table will be copied into device memory.
>  	 */
>  	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
>  	if (!rproc->cached_table)
> -- 
> 2.9.0
> 

Re: [PATCH 2/9] remoteproc: core: Trivial: Improve error checking, spelling and debug prints

[Lee Jones]

On Tue, 09 Aug 2016, Bjorn Andersson wrote:

> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> 
> > Trivial patch to clean up a couple of minor misgivings.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

You can just take it.

> Although as I hope to stack this series ontop of the auto-boot there
> will be some minor conflicts here.

Sure, but they should be trivial to fix.

Or, just take this patch, since it is independent of the set.

> > ---
> >  drivers/remoteproc/remoteproc_core.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 3566dc9..5654a81 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -457,8 +457,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
> >  
> >  	rproc->num_traces++;
> >  
> > -	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n", name, ptr,
> > -						rsc->da, rsc->len);
> > +	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n",
> > +		name, ptr, rsc->da, rsc->len);
> >  
> >  	return 0;
> >  }
> > @@ -581,8 +581,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >  		return -EINVAL;
> >  	}
> >  
> > -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> > -			rsc->da, rsc->pa, rsc->len, rsc->flags);
> > +	dev_dbg(dev, "carveout rsc: name: %s, da %x, pa %x, len 0x%x, flags %x\n",
> > +		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
> >  
> >  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> >  	if (!carveout)
> > @@ -700,7 +700,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
> >  	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
> >  	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
> >  	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
> > -	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
> > +	[RSC_VDEV] = NULL, /* VDEVs were handled upon registration */
> >  };
> >  
> >  static rproc_handle_resource_t rproc_vdev_handler[RSC_LAST] = {
> > @@ -918,7 +918,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
> >  	 * Create a copy of the resource table. When a virtio device starts
> >  	 * and calls vring_new_virtqueue() the address of the allocated vring
> >  	 * will be stored in the cached_table. Before the device is started,
> > -	 * cached_table will be copied into devic memory.
> > +	 * cached_table will be copied into device memory.
> >  	 */
> >  	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
> >  	if (!rproc->cached_table)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 2/9] remoteproc: core: Trivial: Improve error checking, spelling and debug prints

[Lee Jones]

On Tue, 09 Aug 2016, Bjorn Andersson wrote:

> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> 
> > Trivial patch to clean up a couple of minor misgivings.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

You can just take it.

> Although as I hope to stack this series ontop of the auto-boot there
> will be some minor conflicts here.

Sure, but they should be trivial to fix.

Or, just take this patch, since it is independent of the set.

> > ---
> >  drivers/remoteproc/remoteproc_core.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 3566dc9..5654a81 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -457,8 +457,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
> >  
> >  	rproc->num_traces++;
> >  
> > -	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n", name, ptr,
> > -						rsc->da, rsc->len);
> > +	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n",
> > +		name, ptr, rsc->da, rsc->len);
> >  
> >  	return 0;
> >  }
> > @@ -581,8 +581,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >  		return -EINVAL;
> >  	}
> >  
> > -	dev_dbg(dev, "carveout rsc: da %x, pa %x, len 0x%x, flags %x\n",
> > -			rsc->da, rsc->pa, rsc->len, rsc->flags);
> > +	dev_dbg(dev, "carveout rsc: name: %s, da %x, pa %x, len 0x%x, flags %x\n",
> > +		rsc->name, rsc->da, rsc->pa, rsc->len, rsc->flags);
> >  
> >  	carveout = kzalloc(sizeof(*carveout), GFP_KERNEL);
> >  	if (!carveout)
> > @@ -700,7 +700,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
> >  	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
> >  	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
> >  	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
> > -	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
> > +	[RSC_VDEV] = NULL, /* VDEVs were handled upon registration */
> >  };
> >  
> >  static rproc_handle_resource_t rproc_vdev_handler[RSC_LAST] = {
> > @@ -918,7 +918,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
> >  	 * Create a copy of the resource table. When a virtio device starts
> >  	 * and calls vring_new_virtqueue() the address of the allocated vring
> >  	 * will be stored in the cached_table. Before the device is started,
> > -	 * cached_table will be copied into devic memory.
> > +	 * cached_table will be copied into device memory.
> >  	 */
> >  	rproc->cached_table = kmemdup(table, tablesz, GFP_KERNEL);
> >  	if (!rproc->cached_table)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/9] remoteproc: core: Remove pointless OOM print

[Lee Jones]

These types of error prints are superfluous.  The system will
pick up on OOM issues and let the user know.

Signed-off-by: Lee Jones <lee.jones@linaro.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 5654a81..4914482 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -619,7 +619,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	if (rproc->domain) {
 		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
 		if (!mapping) {
-			dev_err(dev, "kzalloc mapping failed\n");
 			ret = -ENOMEM;
 			goto dma_free;
 		}
-- 
2.9.0

[PATCH 3/9] remoteproc: core: Remove pointless OOM print

[Lee Jones]

These types of error prints are superfluous.  The system will
pick up on OOM issues and let the user know.

Signed-off-by: Lee Jones <lee.jones@linaro.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 5654a81..4914482 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -619,7 +619,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	if (rproc->domain) {
 		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
 		if (!mapping) {
-			dev_err(dev, "kzalloc mapping failed\n");
 			ret = -ENOMEM;
 			goto dma_free;
 		}
-- 
2.9.0

Re: [PATCH 3/9] remoteproc: core: Remove pointless OOM print

[Bjorn Andersson]

On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:

> These types of error prints are superfluous.  The system will
> pick up on OOM issues and let the user know.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Although, I think we should drop the "mapping" object. We have all the
information in the carveout object to reverse the mapping at cleanup.

Regards,
Bjorn

> ---
>  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 5654a81..4914482 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -619,7 +619,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	if (rproc->domain) {
>  		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
>  		if (!mapping) {
> -			dev_err(dev, "kzalloc mapping failed\n");
>  			ret = -ENOMEM;
>  			goto dma_free;
>  		}
> -- 
> 2.9.0
> 

[PATCH 3/9] remoteproc: core: Remove pointless OOM print

[Bjorn Andersson]

On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:

> These types of error prints are superfluous.  The system will
> pick up on OOM issues and let the user know.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Although, I think we should drop the "mapping" object. We have all the
information in the carveout object to reverse the mapping at cleanup.

Regards,
Bjorn

> ---
>  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 5654a81..4914482 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -619,7 +619,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  	if (rproc->domain) {
>  		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
>  		if (!mapping) {
> -			dev_err(dev, "kzalloc mapping failed\n");
>  			ret = -ENOMEM;
>  			goto dma_free;
>  		}
> -- 
> 2.9.0
> 

Re: [PATCH 3/9] remoteproc: core: Remove pointless OOM print

[Lee Jones]

On Tue, 09 Aug 2016, Bjorn Andersson wrote:

> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> 
> > These types of error prints are superfluous.  The system will
> > pick up on OOM issues and let the user know.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

This doesn't rely on anything else, so you can just take it.

> Although, I think we should drop the "mapping" object. We have all the
> information in the carveout object to reverse the mapping at cleanup.

Let's fix it for now, then work on 'nice to have's in a subsequent set.
> > ---
> >  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 5654a81..4914482 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -619,7 +619,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >  	if (rproc->domain) {
> >  		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> >  		if (!mapping) {
> > -			dev_err(dev, "kzalloc mapping failed\n");
> >  			ret = -ENOMEM;
> >  			goto dma_free;
> >  		}

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/9] remoteproc: core: Remove pointless OOM print

[Lee Jones]

On Tue, 09 Aug 2016, Bjorn Andersson wrote:

> On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> 
> > These types of error prints are superfluous.  The system will
> > pick up on OOM issues and let the user know.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

This doesn't rely on anything else, so you can just take it.

> Although, I think we should drop the "mapping" object. We have all the
> information in the carveout object to reverse the mapping at cleanup.

Let's fix it for now, then work on 'nice to have's in a subsequent set.
> > ---
> >  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 5654a81..4914482 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -619,7 +619,6 @@ static int rproc_handle_carveout(struct rproc *rproc,
> >  	if (rproc->domain) {
> >  		mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
> >  		if (!mapping) {
> > -			dev_err(dev, "kzalloc mapping failed\n");
> >  			ret = -ENOMEM;
> >  			goto dma_free;
> >  		}

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/9] remoteproc: core: Remove pointless OOM print

[Bjorn Andersson]

On Tue 09 Aug 11:10 PDT 2016, Lee Jones wrote:

> On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> 
> > On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> > 
> > > These types of error prints are superfluous.  The system will
> > > pick up on OOM issues and let the user know.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > 
> > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> This doesn't rely on anything else, so you can just take it.
> 

Of course...

Applied patch 1 through 3.

Regards,
Bjorn

[PATCH 3/9] remoteproc: core: Remove pointless OOM print

[Bjorn Andersson]

On Tue 09 Aug 11:10 PDT 2016, Lee Jones wrote:

> On Tue, 09 Aug 2016, Bjorn Andersson wrote:
> 
> > On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:
> > 
> > > These types of error prints are superfluous.  The system will
> > > pick up on OOM issues and let the user know.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > 
> > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> This doesn't rely on anything else, so you can just take it.
> 

Of course...

Applied patch 1 through 3.

Regards,
Bjorn

[PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[Lee Jones]

In order to amend or add a new resource table entry we need a method
for a platform-specific to submit them. rproc_request_resource() is a
new public API which provides this functionality.

It is to be called between rproc_alloc() and rproc_add().

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
 include/linux/remoteproc.h           |  21 ++++++
 2 files changed, 146 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 4914482..0abfa2b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 	}
 }
 
+static void rproc_dump_resource_table(struct rproc *rproc,
+				      struct resource_table *table, int size)
+{
+	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
+	struct device *dev = &rproc->dev;
+	struct fw_rsc_carveout *c;
+	struct fw_rsc_devmem *d;
+	struct fw_rsc_trace *t;
+	struct fw_rsc_vdev *v;
+	int i, j;
+
+	if (!table) {
+		dev_dbg(dev, "No resource table found\n");
+		return;
+	}
+
+	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
+		table->ver, table->num, size);
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			c = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
+			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
+			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
+			dev_dbg(dev, "  Name %s\n\n", c->name);
+			break;
+		case RSC_DEVMEM:
+			d = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
+			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
+			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
+			dev_dbg(dev, "  Name %s\n\n", d->name);
+			break;
+		case RSC_TRACE:
+			t = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
+			dev_dbg(dev, "  Name %s\n\n", t->name);
+			break;
+		case RSC_VDEV:
+			v = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+
+			dev_dbg(dev, "  ID %d\n", v->id);
+			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
+			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
+			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
+			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
+			dev_dbg(dev, "  Status 0x%x\n", v->status);
+			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
+			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
+				v->reserved[0], v->reserved[1]);
+
+			for (j = 0; j < v->num_of_vrings; j++) {
+				dev_dbg(dev, "  Vring %d\n", j);
+				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
+				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
+				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
+				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
+				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
+					v->vring[j].reserved);
+			}
+			break;
+		default:
+			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
+				hdr->type, hdr);
+			return;
+		}
+	}
+}
+
+int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
+{
+	struct device *dev = &rproc->dev;
+	struct rproc_request_resource *request;
+	int size;
+
+	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	switch (type) {
+	case RSC_CARVEOUT:
+		size = sizeof(struct fw_rsc_carveout);
+		break;
+	case RSC_DEVMEM:
+		size = sizeof(struct fw_rsc_devmem);
+		break;
+	case RSC_TRACE:
+		size = sizeof(struct fw_rsc_trace);
+		break;
+	default:
+		dev_err(dev, "Unsupported resource type: %d\n", type);
+		return -EINVAL;
+	}
+
+	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (!request->resource)
+		return -ENOMEM;
+
+	memcpy(request->resource, resource, size);
+	request->type = type;
+	request->size = size;
+
+	list_add_tail(&request->node, &rproc->override_resources);
+
+	return 0;
+}
+EXPORT_SYMBOL(rproc_request_resource);
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1452,6 +1576,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->mappings);
 	INIT_LIST_HEAD(&rproc->traces);
 	INIT_LIST_HEAD(&rproc->rvdevs);
+	INIT_LIST_HEAD(&rproc->override_resources);
 
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
 	init_completion(&rproc->crash_comp);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3608d20..c620177 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -323,6 +323,25 @@ struct rproc_mem_entry {
 	struct list_head node;
 };
 
+/**
+ * struct rproc_requested_resources - add a resource to the resource table
+ *
+ * @resource:	pointer to a 'struct fw_rsc_*' resource
+ * @type:	'fw_resource_type' resource type
+ * @size:	size of resource
+ * @node:	list node
+ *
+ * Resources can be added by platform-specific rproc drivers calling
+ * rproc_request_resource()
+ *
+ */
+struct rproc_request_resource {
+	void *resource;
+	u32 type;
+	u32 size;
+	struct list_head node;
+};
+
 struct rproc;
 
 /**
@@ -429,6 +448,7 @@ struct rproc {
 	int num_traces;
 	struct list_head carveouts;
 	struct list_head mappings;
+	struct list_head override_resources;
 	struct completion firmware_loading_complete;
 	u32 bootaddr;
 	struct list_head rvdevs;
@@ -487,6 +507,7 @@ struct rproc_vdev {
 	u32 rsc_offset;
 };
 
+int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
 struct rproc *rproc_alloc(struct device *dev, const char *name,
 				const struct rproc_ops *ops,
 				const char *firmware, int len);
-- 
2.9.0

[PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[Lee Jones]

In order to amend or add a new resource table entry we need a method
for a platform-specific to submit them. rproc_request_resource() is a
new public API which provides this functionality.

It is to be called between rproc_alloc() and rproc_add().

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
 include/linux/remoteproc.h           |  21 ++++++
 2 files changed, 146 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 4914482..0abfa2b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 	}
 }
 
+static void rproc_dump_resource_table(struct rproc *rproc,
+				      struct resource_table *table, int size)
+{
+	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
+	struct device *dev = &rproc->dev;
+	struct fw_rsc_carveout *c;
+	struct fw_rsc_devmem *d;
+	struct fw_rsc_trace *t;
+	struct fw_rsc_vdev *v;
+	int i, j;
+
+	if (!table) {
+		dev_dbg(dev, "No resource table found\n");
+		return;
+	}
+
+	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
+		table->ver, table->num, size);
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			c = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
+			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
+			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
+			dev_dbg(dev, "  Name %s\n\n", c->name);
+			break;
+		case RSC_DEVMEM:
+			d = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
+			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
+			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
+			dev_dbg(dev, "  Name %s\n\n", d->name);
+			break;
+		case RSC_TRACE:
+			t = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
+			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
+			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
+			dev_dbg(dev, "  Name %s\n\n", t->name);
+			break;
+		case RSC_VDEV:
+			v = rsc;
+			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
+
+			dev_dbg(dev, "  ID %d\n", v->id);
+			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
+			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
+			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
+			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
+			dev_dbg(dev, "  Status 0x%x\n", v->status);
+			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
+			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
+				v->reserved[0], v->reserved[1]);
+
+			for (j = 0; j < v->num_of_vrings; j++) {
+				dev_dbg(dev, "  Vring %d\n", j);
+				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
+				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
+				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
+				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
+				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
+					v->vring[j].reserved);
+			}
+			break;
+		default:
+			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
+				hdr->type, hdr);
+			return;
+		}
+	}
+}
+
+int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
+{
+	struct device *dev = &rproc->dev;
+	struct rproc_request_resource *request;
+	int size;
+
+	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	switch (type) {
+	case RSC_CARVEOUT:
+		size = sizeof(struct fw_rsc_carveout);
+		break;
+	case RSC_DEVMEM:
+		size = sizeof(struct fw_rsc_devmem);
+		break;
+	case RSC_TRACE:
+		size = sizeof(struct fw_rsc_trace);
+		break;
+	default:
+		dev_err(dev, "Unsupported resource type: %d\n", type);
+		return -EINVAL;
+	}
+
+	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (!request->resource)
+		return -ENOMEM;
+
+	memcpy(request->resource, resource, size);
+	request->type = type;
+	request->size = size;
+
+	list_add_tail(&request->node, &rproc->override_resources);
+
+	return 0;
+}
+EXPORT_SYMBOL(rproc_request_resource);
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1452,6 +1576,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->mappings);
 	INIT_LIST_HEAD(&rproc->traces);
 	INIT_LIST_HEAD(&rproc->rvdevs);
+	INIT_LIST_HEAD(&rproc->override_resources);
 
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
 	init_completion(&rproc->crash_comp);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3608d20..c620177 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -323,6 +323,25 @@ struct rproc_mem_entry {
 	struct list_head node;
 };
 
+/**
+ * struct rproc_requested_resources - add a resource to the resource table
+ *
+ * @resource:	pointer to a 'struct fw_rsc_*' resource
+ * @type:	'fw_resource_type' resource type
+ * @size:	size of resource
+ * @node:	list node
+ *
+ * Resources can be added by platform-specific rproc drivers calling
+ * rproc_request_resource()
+ *
+ */
+struct rproc_request_resource {
+	void *resource;
+	u32 type;
+	u32 size;
+	struct list_head node;
+};
+
 struct rproc;
 
 /**
@@ -429,6 +448,7 @@ struct rproc {
 	int num_traces;
 	struct list_head carveouts;
 	struct list_head mappings;
+	struct list_head override_resources;
 	struct completion firmware_loading_complete;
 	u32 bootaddr;
 	struct list_head rvdevs;
@@ -487,6 +507,7 @@ struct rproc_vdev {
 	u32 rsc_offset;
 };
 
+int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
 struct rproc *rproc_alloc(struct device *dev, const char *name,
 				const struct rproc_ops *ops,
 				const char *firmware, int len);
-- 
2.9.0

Re: [PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[Lee Jones]

On Thu, 04 Aug 2016, Lee Jones wrote:

> In order to amend or add a new resource table entry we need a method
> for a platform-specific to submit them. rproc_request_resource() is a
> new public API which provides this functionality.
> 
> It is to be called between rproc_alloc() and rproc_add().
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
>  include/linux/remoteproc.h           |  21 ++++++
>  2 files changed, 146 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4914482..0abfa2b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>  	}
>  }
>  
> +static void rproc_dump_resource_table(struct rproc *rproc,
> +				      struct resource_table *table, int size)
> +{

Ah!  Looks like I mistakenly squashed two patches together.  I won't
re-send, but please treat these two functions as separate patches.  I
will fix this during v2, once I've received some feedback.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[Lee Jones]

On Thu, 04 Aug 2016, Lee Jones wrote:

> In order to amend or add a new resource table entry we need a method
> for a platform-specific to submit them. rproc_request_resource() is a
> new public API which provides this functionality.
> 
> It is to be called between rproc_alloc() and rproc_add().
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
>  include/linux/remoteproc.h           |  21 ++++++
>  2 files changed, 146 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4914482..0abfa2b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>  	}
>  }
>  
> +static void rproc_dump_resource_table(struct rproc *rproc,
> +				      struct resource_table *table, int size)
> +{

Ah!  Looks like I mistakenly squashed two patches together.  I won't
re-send, but please treat these two functions as separate patches.  I
will fix this during v2, once I've received some feedback.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[loic pallardy]

Hi Lee,

After splitting this patch in 2, it may be good to add a debugfs entry 
to display current rsc associated to one rproc.

Regards,
Loic

On 08/04/2016 11:21 AM, Lee Jones wrote:
> In order to amend or add a new resource table entry we need a method
> for a platform-specific to submit them. rproc_request_resource() is a
> new public API which provides this functionality.
>
> It is to be called between rproc_alloc() and rproc_add().
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
>   include/linux/remoteproc.h           |  21 ++++++
>   2 files changed, 146 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4914482..0abfa2b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>   	}
>   }
>
> +static void rproc_dump_resource_table(struct rproc *rproc,
> +				      struct resource_table *table, int size)
> +{
> +	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
> +	struct device *dev = &rproc->dev;
> +	struct fw_rsc_carveout *c;
> +	struct fw_rsc_devmem *d;
> +	struct fw_rsc_trace *t;
> +	struct fw_rsc_vdev *v;
> +	int i, j;
> +
> +	if (!table) {
> +		dev_dbg(dev, "No resource table found\n");
> +		return;
> +	}
> +
> +	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
> +		table->ver, table->num, size);
> +
> +	for (i = 0; i < table->num; i++) {
> +		int offset = table->offset[i];
> +		struct fw_rsc_hdr *hdr = (void *)table + offset;
> +		void *rsc = (void *)hdr + sizeof(*hdr);
> +
> +		switch (hdr->type) {
> +		case RSC_CARVEOUT:
> +			c = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
> +			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
> +			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", c->name);
> +			break;
> +		case RSC_DEVMEM:
> +			d = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
> +			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
> +			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", d->name);
> +			break;
> +		case RSC_TRACE:
> +			t = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", t->name);
> +			break;
> +		case RSC_VDEV:
> +			v = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +
> +			dev_dbg(dev, "  ID %d\n", v->id);
> +			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
> +			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
> +			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
> +			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
> +			dev_dbg(dev, "  Status 0x%x\n", v->status);
> +			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
> +				v->reserved[0], v->reserved[1]);
> +
> +			for (j = 0; j < v->num_of_vrings; j++) {
> +				dev_dbg(dev, "  Vring %d\n", j);
> +				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
> +				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
> +				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
> +				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
> +				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
> +					v->vring[j].reserved);
> +			}
> +			break;
> +		default:
> +			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
> +				hdr->type, hdr);
> +			return;
> +		}
> +	}
> +}
> +
> +int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
> +{
> +	struct device *dev = &rproc->dev;
> +	struct rproc_request_resource *request;
> +	int size;
> +
> +	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
> +	if (!request)
> +		return -ENOMEM;
> +
> +	switch (type) {
> +	case RSC_CARVEOUT:
> +		size = sizeof(struct fw_rsc_carveout);
> +		break;
> +	case RSC_DEVMEM:
> +		size = sizeof(struct fw_rsc_devmem);
> +		break;
> +	case RSC_TRACE:
> +		size = sizeof(struct fw_rsc_trace);
> +		break;
> +	default:
> +		dev_err(dev, "Unsupported resource type: %d\n", type);
> +		return -EINVAL;
> +	}
> +
> +	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
> +	if (!request->resource)
> +		return -ENOMEM;
> +
> +	memcpy(request->resource, resource, size);
> +	request->type = type;
> +	request->size = size;
> +
> +	list_add_tail(&request->node, &rproc->override_resources);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(rproc_request_resource);
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
> @@ -1452,6 +1576,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   	INIT_LIST_HEAD(&rproc->mappings);
>   	INIT_LIST_HEAD(&rproc->traces);
>   	INIT_LIST_HEAD(&rproc->rvdevs);
> +	INIT_LIST_HEAD(&rproc->override_resources);
>
>   	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
>   	init_completion(&rproc->crash_comp);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 3608d20..c620177 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -323,6 +323,25 @@ struct rproc_mem_entry {
>   	struct list_head node;
>   };
>
> +/**
> + * struct rproc_requested_resources - add a resource to the resource table
> + *
> + * @resource:	pointer to a 'struct fw_rsc_*' resource
> + * @type:	'fw_resource_type' resource type
> + * @size:	size of resource
> + * @node:	list node
> + *
> + * Resources can be added by platform-specific rproc drivers calling
> + * rproc_request_resource()
> + *
> + */
> +struct rproc_request_resource {
> +	void *resource;
> +	u32 type;
> +	u32 size;
> +	struct list_head node;
> +};
> +
>   struct rproc;
>
>   /**
> @@ -429,6 +448,7 @@ struct rproc {
>   	int num_traces;
>   	struct list_head carveouts;
>   	struct list_head mappings;
> +	struct list_head override_resources;
>   	struct completion firmware_loading_complete;
>   	u32 bootaddr;
>   	struct list_head rvdevs;
> @@ -487,6 +507,7 @@ struct rproc_vdev {
>   	u32 rsc_offset;
>   };
>
> +int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
>   struct rproc *rproc_alloc(struct device *dev, const char *name,
>   				const struct rproc_ops *ops,
>   				const char *firmware, int len);
>

[PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[loic pallardy]

Hi Lee,

After splitting this patch in 2, it may be good to add a debugfs entry 
to display current rsc associated to one rproc.

Regards,
Loic

On 08/04/2016 11:21 AM, Lee Jones wrote:
> In order to amend or add a new resource table entry we need a method
> for a platform-specific to submit them. rproc_request_resource() is a
> new public API which provides this functionality.
>
> It is to be called between rproc_alloc() and rproc_add().
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
>   include/linux/remoteproc.h           |  21 ++++++
>   2 files changed, 146 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4914482..0abfa2b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>   	}
>   }
>
> +static void rproc_dump_resource_table(struct rproc *rproc,
> +				      struct resource_table *table, int size)
> +{
> +	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
> +	struct device *dev = &rproc->dev;
> +	struct fw_rsc_carveout *c;
> +	struct fw_rsc_devmem *d;
> +	struct fw_rsc_trace *t;
> +	struct fw_rsc_vdev *v;
> +	int i, j;
> +
> +	if (!table) {
> +		dev_dbg(dev, "No resource table found\n");
> +		return;
> +	}
> +
> +	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
> +		table->ver, table->num, size);
> +
> +	for (i = 0; i < table->num; i++) {
> +		int offset = table->offset[i];
> +		struct fw_rsc_hdr *hdr = (void *)table + offset;
> +		void *rsc = (void *)hdr + sizeof(*hdr);
> +
> +		switch (hdr->type) {
> +		case RSC_CARVEOUT:
> +			c = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
> +			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
> +			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", c->name);
> +			break;
> +		case RSC_DEVMEM:
> +			d = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
> +			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
> +			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", d->name);
> +			break;
> +		case RSC_TRACE:
> +			t = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", t->name);
> +			break;
> +		case RSC_VDEV:
> +			v = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +
> +			dev_dbg(dev, "  ID %d\n", v->id);
> +			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
> +			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
> +			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
> +			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
> +			dev_dbg(dev, "  Status 0x%x\n", v->status);
> +			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
> +				v->reserved[0], v->reserved[1]);
> +
> +			for (j = 0; j < v->num_of_vrings; j++) {
> +				dev_dbg(dev, "  Vring %d\n", j);
> +				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
> +				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
> +				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
> +				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
> +				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
> +					v->vring[j].reserved);
> +			}
> +			break;
> +		default:
> +			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
> +				hdr->type, hdr);
> +			return;
> +		}
> +	}
> +}
> +
> +int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
> +{
> +	struct device *dev = &rproc->dev;
> +	struct rproc_request_resource *request;
> +	int size;
> +
> +	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
> +	if (!request)
> +		return -ENOMEM;
> +
> +	switch (type) {
> +	case RSC_CARVEOUT:
> +		size = sizeof(struct fw_rsc_carveout);
> +		break;
> +	case RSC_DEVMEM:
> +		size = sizeof(struct fw_rsc_devmem);
> +		break;
> +	case RSC_TRACE:
> +		size = sizeof(struct fw_rsc_trace);
> +		break;
> +	default:
> +		dev_err(dev, "Unsupported resource type: %d\n", type);
> +		return -EINVAL;
> +	}
> +
> +	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
> +	if (!request->resource)
> +		return -ENOMEM;
> +
> +	memcpy(request->resource, resource, size);
> +	request->type = type;
> +	request->size = size;
> +
> +	list_add_tail(&request->node, &rproc->override_resources);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(rproc_request_resource);
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
> @@ -1452,6 +1576,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   	INIT_LIST_HEAD(&rproc->mappings);
>   	INIT_LIST_HEAD(&rproc->traces);
>   	INIT_LIST_HEAD(&rproc->rvdevs);
> +	INIT_LIST_HEAD(&rproc->override_resources);
>
>   	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
>   	init_completion(&rproc->crash_comp);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 3608d20..c620177 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -323,6 +323,25 @@ struct rproc_mem_entry {
>   	struct list_head node;
>   };
>
> +/**
> + * struct rproc_requested_resources - add a resource to the resource table
> + *
> + * @resource:	pointer to a 'struct fw_rsc_*' resource
> + * @type:	'fw_resource_type' resource type
> + * @size:	size of resource
> + * @node:	list node
> + *
> + * Resources can be added by platform-specific rproc drivers calling
> + * rproc_request_resource()
> + *
> + */
> +struct rproc_request_resource {
> +	void *resource;
> +	u32 type;
> +	u32 size;
> +	struct list_head node;
> +};
> +
>   struct rproc;
>
>   /**
> @@ -429,6 +448,7 @@ struct rproc {
>   	int num_traces;
>   	struct list_head carveouts;
>   	struct list_head mappings;
> +	struct list_head override_resources;
>   	struct completion firmware_loading_complete;
>   	u32 bootaddr;
>   	struct list_head rvdevs;
> @@ -487,6 +507,7 @@ struct rproc_vdev {
>   	u32 rsc_offset;
>   };
>
> +int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
>   struct rproc *rproc_alloc(struct device *dev, const char *name,
>   				const struct rproc_ops *ops,
>   				const char *firmware, int len);
>

Re: [PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[loic pallardy]

Hi Lee,

After splitting this patch in 2, it may be good to add a debugfs entry 
to display current rsc associated to one rproc.

Regards,
Loic

On 08/04/2016 11:21 AM, Lee Jones wrote:
> In order to amend or add a new resource table entry we need a method
> for a platform-specific to submit them. rproc_request_resource() is a
> new public API which provides this functionality.
>
> It is to be called between rproc_alloc() and rproc_add().
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
>   include/linux/remoteproc.h           |  21 ++++++
>   2 files changed, 146 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 4914482..0abfa2b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
>   	}
>   }
>
> +static void rproc_dump_resource_table(struct rproc *rproc,
> +				      struct resource_table *table, int size)
> +{
> +	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
> +	struct device *dev = &rproc->dev;
> +	struct fw_rsc_carveout *c;
> +	struct fw_rsc_devmem *d;
> +	struct fw_rsc_trace *t;
> +	struct fw_rsc_vdev *v;
> +	int i, j;
> +
> +	if (!table) {
> +		dev_dbg(dev, "No resource table found\n");
> +		return;
> +	}
> +
> +	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
> +		table->ver, table->num, size);
> +
> +	for (i = 0; i < table->num; i++) {
> +		int offset = table->offset[i];
> +		struct fw_rsc_hdr *hdr = (void *)table + offset;
> +		void *rsc = (void *)hdr + sizeof(*hdr);
> +
> +		switch (hdr->type) {
> +		case RSC_CARVEOUT:
> +			c = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
> +			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
> +			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", c->name);
> +			break;
> +		case RSC_DEVMEM:
> +			d = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
> +			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
> +			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", d->name);
> +			break;
> +		case RSC_TRACE:
> +			t = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
> +			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
> +			dev_dbg(dev, "  Name %s\n\n", t->name);
> +			break;
> +		case RSC_VDEV:
> +			v = rsc;
> +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> +
> +			dev_dbg(dev, "  ID %d\n", v->id);
> +			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
> +			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
> +			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
> +			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
> +			dev_dbg(dev, "  Status 0x%x\n", v->status);
> +			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
> +			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
> +				v->reserved[0], v->reserved[1]);
> +
> +			for (j = 0; j < v->num_of_vrings; j++) {
> +				dev_dbg(dev, "  Vring %d\n", j);
> +				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
> +				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
> +				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
> +				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
> +				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
> +					v->vring[j].reserved);
> +			}
> +			break;
> +		default:
> +			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
> +				hdr->type, hdr);
> +			return;
> +		}
> +	}
> +}
> +
> +int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
> +{
> +	struct device *dev = &rproc->dev;
> +	struct rproc_request_resource *request;
> +	int size;
> +
> +	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
> +	if (!request)
> +		return -ENOMEM;
> +
> +	switch (type) {
> +	case RSC_CARVEOUT:
> +		size = sizeof(struct fw_rsc_carveout);
> +		break;
> +	case RSC_DEVMEM:
> +		size = sizeof(struct fw_rsc_devmem);
> +		break;
> +	case RSC_TRACE:
> +		size = sizeof(struct fw_rsc_trace);
> +		break;
> +	default:
> +		dev_err(dev, "Unsupported resource type: %d\n", type);
> +		return -EINVAL;
> +	}
> +
> +	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
> +	if (!request->resource)
> +		return -ENOMEM;
> +
> +	memcpy(request->resource, resource, size);
> +	request->type = type;
> +	request->size = size;
> +
> +	list_add_tail(&request->node, &rproc->override_resources);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(rproc_request_resource);
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
> @@ -1452,6 +1576,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>   	INIT_LIST_HEAD(&rproc->mappings);
>   	INIT_LIST_HEAD(&rproc->traces);
>   	INIT_LIST_HEAD(&rproc->rvdevs);
> +	INIT_LIST_HEAD(&rproc->override_resources);
>
>   	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
>   	init_completion(&rproc->crash_comp);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 3608d20..c620177 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -323,6 +323,25 @@ struct rproc_mem_entry {
>   	struct list_head node;
>   };
>
> +/**
> + * struct rproc_requested_resources - add a resource to the resource table
> + *
> + * @resource:	pointer to a 'struct fw_rsc_*' resource
> + * @type:	'fw_resource_type' resource type
> + * @size:	size of resource
> + * @node:	list node
> + *
> + * Resources can be added by platform-specific rproc drivers calling
> + * rproc_request_resource()
> + *
> + */
> +struct rproc_request_resource {
> +	void *resource;
> +	u32 type;
> +	u32 size;
> +	struct list_head node;
> +};
> +
>   struct rproc;
>
>   /**
> @@ -429,6 +448,7 @@ struct rproc {
>   	int num_traces;
>   	struct list_head carveouts;
>   	struct list_head mappings;
> +	struct list_head override_resources;
>   	struct completion firmware_loading_complete;
>   	u32 bootaddr;
>   	struct list_head rvdevs;
> @@ -487,6 +507,7 @@ struct rproc_vdev {
>   	u32 rsc_offset;
>   };
>
> +int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
>   struct rproc *rproc_alloc(struct device *dev, const char *name,
>   				const struct rproc_ops *ops,
>   				const char *firmware, int len);
>

Re: [PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[Lee Jones]

On Mon, 08 Aug 2016, loic pallardy wrote:

> Hi Lee,
> 
> After splitting this patch in 2, it may be good to add a debugfs entry to
> display current rsc associated to one rproc.

Happy to take suggestions to add functionality, but I would like to
prevent *this* set from suffering from feature creep.  Thus, I would
like to code-up your suggestion in a subsequent submission if it's all
the same to you.

> On 08/04/2016 11:21 AM, Lee Jones wrote:
> > In order to amend or add a new resource table entry we need a method
> > for a platform-specific to submit them. rproc_request_resource() is a
> > new public API which provides this functionality.
> > 
> > It is to be called between rproc_alloc() and rproc_add().
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >   drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
> >   include/linux/remoteproc.h           |  21 ++++++
> >   2 files changed, 146 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 4914482..0abfa2b 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
> >   	}
> >   }
> > 
> > +static void rproc_dump_resource_table(struct rproc *rproc,
> > +				      struct resource_table *table, int size)
> > +{
> > +	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
> > +	struct device *dev = &rproc->dev;
> > +	struct fw_rsc_carveout *c;
> > +	struct fw_rsc_devmem *d;
> > +	struct fw_rsc_trace *t;
> > +	struct fw_rsc_vdev *v;
> > +	int i, j;
> > +
> > +	if (!table) {
> > +		dev_dbg(dev, "No resource table found\n");
> > +		return;
> > +	}
> > +
> > +	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
> > +		table->ver, table->num, size);
> > +
> > +	for (i = 0; i < table->num; i++) {
> > +		int offset = table->offset[i];
> > +		struct fw_rsc_hdr *hdr = (void *)table + offset;
> > +		void *rsc = (void *)hdr + sizeof(*hdr);
> > +
> > +		switch (hdr->type) {
> > +		case RSC_CARVEOUT:
> > +			c = rsc;
> > +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> > +			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
> > +			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
> > +			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
> > +			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
> > +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
> > +			dev_dbg(dev, "  Name %s\n\n", c->name);
> > +			break;
> > +		case RSC_DEVMEM:
> > +			d = rsc;
> > +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> > +			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
> > +			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
> > +			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
> > +			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
> > +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
> > +			dev_dbg(dev, "  Name %s\n\n", d->name);
> > +			break;
> > +		case RSC_TRACE:
> > +			t = rsc;
> > +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> > +			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
> > +			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
> > +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
> > +			dev_dbg(dev, "  Name %s\n\n", t->name);
> > +			break;
> > +		case RSC_VDEV:
> > +			v = rsc;
> > +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> > +
> > +			dev_dbg(dev, "  ID %d\n", v->id);
> > +			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
> > +			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
> > +			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
> > +			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
> > +			dev_dbg(dev, "  Status 0x%x\n", v->status);
> > +			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
> > +			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
> > +				v->reserved[0], v->reserved[1]);
> > +
> > +			for (j = 0; j < v->num_of_vrings; j++) {
> > +				dev_dbg(dev, "  Vring %d\n", j);
> > +				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
> > +				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
> > +				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
> > +				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
> > +				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
> > +					v->vring[j].reserved);
> > +			}
> > +			break;
> > +		default:
> > +			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
> > +				hdr->type, hdr);
> > +			return;
> > +		}
> > +	}
> > +}
> > +
> > +int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
> > +{
> > +	struct device *dev = &rproc->dev;
> > +	struct rproc_request_resource *request;
> > +	int size;
> > +
> > +	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
> > +	if (!request)
> > +		return -ENOMEM;
> > +
> > +	switch (type) {
> > +	case RSC_CARVEOUT:
> > +		size = sizeof(struct fw_rsc_carveout);
> > +		break;
> > +	case RSC_DEVMEM:
> > +		size = sizeof(struct fw_rsc_devmem);
> > +		break;
> > +	case RSC_TRACE:
> > +		size = sizeof(struct fw_rsc_trace);
> > +		break;
> > +	default:
> > +		dev_err(dev, "Unsupported resource type: %d\n", type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
> > +	if (!request->resource)
> > +		return -ENOMEM;
> > +
> > +	memcpy(request->resource, resource, size);
> > +	request->type = type;
> > +	request->size = size;
> > +
> > +	list_add_tail(&request->node, &rproc->override_resources);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(rproc_request_resource);
> > +
> >   /*
> >    * take a firmware and boot a remote processor with it.
> >    */
> > @@ -1452,6 +1576,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> >   	INIT_LIST_HEAD(&rproc->mappings);
> >   	INIT_LIST_HEAD(&rproc->traces);
> >   	INIT_LIST_HEAD(&rproc->rvdevs);
> > +	INIT_LIST_HEAD(&rproc->override_resources);
> > 
> >   	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
> >   	init_completion(&rproc->crash_comp);
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 3608d20..c620177 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -323,6 +323,25 @@ struct rproc_mem_entry {
> >   	struct list_head node;
> >   };
> > 
> > +/**
> > + * struct rproc_requested_resources - add a resource to the resource table
> > + *
> > + * @resource:	pointer to a 'struct fw_rsc_*' resource
> > + * @type:	'fw_resource_type' resource type
> > + * @size:	size of resource
> > + * @node:	list node
> > + *
> > + * Resources can be added by platform-specific rproc drivers calling
> > + * rproc_request_resource()
> > + *
> > + */
> > +struct rproc_request_resource {
> > +	void *resource;
> > +	u32 type;
> > +	u32 size;
> > +	struct list_head node;
> > +};
> > +
> >   struct rproc;
> > 
> >   /**
> > @@ -429,6 +448,7 @@ struct rproc {
> >   	int num_traces;
> >   	struct list_head carveouts;
> >   	struct list_head mappings;
> > +	struct list_head override_resources;
> >   	struct completion firmware_loading_complete;
> >   	u32 bootaddr;
> >   	struct list_head rvdevs;
> > @@ -487,6 +507,7 @@ struct rproc_vdev {
> >   	u32 rsc_offset;
> >   };
> > 
> > +int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
> >   struct rproc *rproc_alloc(struct device *dev, const char *name,
> >   				const struct rproc_ops *ops,
> >   				const char *firmware, int len);
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/9] remoteproc: core: New API to add new resources to the resource table

[Lee Jones]

On Mon, 08 Aug 2016, loic pallardy wrote:

> Hi Lee,
> 
> After splitting this patch in 2, it may be good to add a debugfs entry to
> display current rsc associated to one rproc.

Happy to take suggestions to add functionality, but I would like to
prevent *this* set from suffering from feature creep.  Thus, I would
like to code-up your suggestion in a subsequent submission if it's all
the same to you.

> On 08/04/2016 11:21 AM, Lee Jones wrote:
> > In order to amend or add a new resource table entry we need a method
> > for a platform-specific to submit them. rproc_request_resource() is a
> > new public API which provides this functionality.
> > 
> > It is to be called between rproc_alloc() and rproc_add().
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >   drivers/remoteproc/remoteproc_core.c | 125 +++++++++++++++++++++++++++++++++++
> >   include/linux/remoteproc.h           |  21 ++++++
> >   2 files changed, 146 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 4914482..0abfa2b 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -793,6 +793,130 @@ static void rproc_resource_cleanup(struct rproc *rproc)
> >   	}
> >   }
> > 
> > +static void rproc_dump_resource_table(struct rproc *rproc,
> > +				      struct resource_table *table, int size)
> > +{
> > +	const char *types[] = {"carveout", "devmem", "trace", "vdev"};
> > +	struct device *dev = &rproc->dev;
> > +	struct fw_rsc_carveout *c;
> > +	struct fw_rsc_devmem *d;
> > +	struct fw_rsc_trace *t;
> > +	struct fw_rsc_vdev *v;
> > +	int i, j;
> > +
> > +	if (!table) {
> > +		dev_dbg(dev, "No resource table found\n");
> > +		return;
> > +	}
> > +
> > +	dev_dbg(dev, "Resource Table: Version %d with %d entries [size: %x]\n",
> > +		table->ver, table->num, size);
> > +
> > +	for (i = 0; i < table->num; i++) {
> > +		int offset = table->offset[i];
> > +		struct fw_rsc_hdr *hdr = (void *)table + offset;
> > +		void *rsc = (void *)hdr + sizeof(*hdr);
> > +
> > +		switch (hdr->type) {
> > +		case RSC_CARVEOUT:
> > +			c = rsc;
> > +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> > +			dev_dbg(dev, "  Device Address 0x%x\n", c->da);
> > +			dev_dbg(dev, "  Physical Address 0x%x\n", c->pa);
> > +			dev_dbg(dev, "  Length 0x%x Bytes\n", c->len);
> > +			dev_dbg(dev, "  Flags 0x%x\n", c->flags);
> > +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", c->reserved);
> > +			dev_dbg(dev, "  Name %s\n\n", c->name);
> > +			break;
> > +		case RSC_DEVMEM:
> > +			d = rsc;
> > +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> > +			dev_dbg(dev, "  Device Address 0x%x\n", d->da);
> > +			dev_dbg(dev, "  Physical Address 0x%x\n", d->pa);
> > +			dev_dbg(dev, "  Length 0x%x Bytes\n", d->len);
> > +			dev_dbg(dev, "  Flags 0x%x\n", d->flags);
> > +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", d->reserved);
> > +			dev_dbg(dev, "  Name %s\n\n", d->name);
> > +			break;
> > +		case RSC_TRACE:
> > +			t = rsc;
> > +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> > +			dev_dbg(dev, "  Device Address 0x%x\n", t->da);
> > +			dev_dbg(dev, "  Length 0x%x Bytes\n", t->len);
> > +			dev_dbg(dev, "  Reserved (should be zero) [%d]\n", t->reserved);
> > +			dev_dbg(dev, "  Name %s\n\n", t->name);
> > +			break;
> > +		case RSC_VDEV:
> > +			v = rsc;
> > +			dev_dbg(dev, "Entry %d is of type %s\n", i, types[hdr->type]);
> > +
> > +			dev_dbg(dev, "  ID %d\n", v->id);
> > +			dev_dbg(dev, "  Notify ID %d\n", v->notifyid);
> > +			dev_dbg(dev, "  Device features 0x%x\n", v->dfeatures);
> > +			dev_dbg(dev, "  Guest features 0x%x\n", v->gfeatures);
> > +			dev_dbg(dev, "  Config length 0x%x\n", v->config_len);
> > +			dev_dbg(dev, "  Status 0x%x\n", v->status);
> > +			dev_dbg(dev, "  Number of vrings %d\n", v->num_of_vrings);
> > +			dev_dbg(dev, "  Reserved (should be zero) [%d][%d]\n\n",
> > +				v->reserved[0], v->reserved[1]);
> > +
> > +			for (j = 0; j < v->num_of_vrings; j++) {
> > +				dev_dbg(dev, "  Vring %d\n", j);
> > +				dev_dbg(dev, "    Device Address 0x%x\n", v->vring[j].da);
> > +				dev_dbg(dev, "    Alignment %d\n", v->vring[j].align);
> > +				dev_dbg(dev, "    Number of buffers %d\n", v->vring[j].num);
> > +				dev_dbg(dev, "    Notify ID %d\n", v->vring[j].notifyid);
> > +				dev_dbg(dev, "    Reserved (should be zero) [%d]\n\n",
> > +					v->vring[j].reserved);
> > +			}
> > +			break;
> > +		default:
> > +			dev_dbg(dev, "Invalid resource type found: %d [hdr: %p]\n",
> > +				hdr->type, hdr);
> > +			return;
> > +		}
> > +	}
> > +}
> > +
> > +int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
> > +{
> > +	struct device *dev = &rproc->dev;
> > +	struct rproc_request_resource *request;
> > +	int size;
> > +
> > +	request = devm_kzalloc(dev, sizeof(*request), GFP_KERNEL);
> > +	if (!request)
> > +		return -ENOMEM;
> > +
> > +	switch (type) {
> > +	case RSC_CARVEOUT:
> > +		size = sizeof(struct fw_rsc_carveout);
> > +		break;
> > +	case RSC_DEVMEM:
> > +		size = sizeof(struct fw_rsc_devmem);
> > +		break;
> > +	case RSC_TRACE:
> > +		size = sizeof(struct fw_rsc_trace);
> > +		break;
> > +	default:
> > +		dev_err(dev, "Unsupported resource type: %d\n", type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	request->resource = devm_kzalloc(dev, size, GFP_KERNEL);
> > +	if (!request->resource)
> > +		return -ENOMEM;
> > +
> > +	memcpy(request->resource, resource, size);
> > +	request->type = type;
> > +	request->size = size;
> > +
> > +	list_add_tail(&request->node, &rproc->override_resources);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(rproc_request_resource);
> > +
> >   /*
> >    * take a firmware and boot a remote processor with it.
> >    */
> > @@ -1452,6 +1576,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> >   	INIT_LIST_HEAD(&rproc->mappings);
> >   	INIT_LIST_HEAD(&rproc->traces);
> >   	INIT_LIST_HEAD(&rproc->rvdevs);
> > +	INIT_LIST_HEAD(&rproc->override_resources);
> > 
> >   	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
> >   	init_completion(&rproc->crash_comp);
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 3608d20..c620177 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -323,6 +323,25 @@ struct rproc_mem_entry {
> >   	struct list_head node;
> >   };
> > 
> > +/**
> > + * struct rproc_requested_resources - add a resource to the resource table
> > + *
> > + * @resource:	pointer to a 'struct fw_rsc_*' resource
> > + * @type:	'fw_resource_type' resource type
> > + * @size:	size of resource
> > + * @node:	list node
> > + *
> > + * Resources can be added by platform-specific rproc drivers calling
> > + * rproc_request_resource()
> > + *
> > + */
> > +struct rproc_request_resource {
> > +	void *resource;
> > +	u32 type;
> > +	u32 size;
> > +	struct list_head node;
> > +};
> > +
> >   struct rproc;
> > 
> >   /**
> > @@ -429,6 +448,7 @@ struct rproc {
> >   	int num_traces;
> >   	struct list_head carveouts;
> >   	struct list_head mappings;
> > +	struct list_head override_resources;
> >   	struct completion firmware_loading_complete;
> >   	u32 bootaddr;
> >   	struct list_head rvdevs;
> > @@ -487,6 +507,7 @@ struct rproc_vdev {
> >   	u32 rsc_offset;
> >   };
> > 
> > +int rproc_request_resource(struct rproc *rproc, u32 type, void *res);
> >   struct rproc *rproc_alloc(struct device *dev, const char *name,
> >   				const struct rproc_ops *ops,
> >   				const char *firmware, int len);
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 5/9] remoteproc: core: Add function to amend an existing resource table entry

[Lee Jones]

Sometimes the firmware does not know best.

When a firmware is built, it can be loaded with a resource table, usually
detailing shared; memory, virtual device, trace log information etc.
However, some vendors require this hard-coded information to be amended
with new/improved information obtained from Device Tree for instance.

Until now, no method exists which allows the resource table to be amended.
The addition of this function changes that.  It is now possible to pull in
a resource table and amend it before it is finally shared with the remote
device.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0abfa2b..3318ebd 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -917,6 +917,69 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 }
 EXPORT_SYMBOL(rproc_request_resource);
 
+static int rproc_update_resource_table_entry(struct rproc *rproc,
+				struct rproc_request_resource *request,
+				struct resource_table *table, int size)
+{
+	struct fw_rsc_carveout *tblc, *newc;
+	struct fw_rsc_devmem *tbld, *newd;
+	struct fw_rsc_trace *tblt, *newt;
+	int updated = true;
+	int i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		if (request->type != hdr->type)
+			continue;
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			tblc = rsc;
+			newc = request->resource;
+
+			if (strncmp(newc->name, tblc->name,
+				    sizeof(*tblc->name)))
+				break;
+
+			memcpy(tblc, newc, request->size);
+
+			return updated;
+		case RSC_DEVMEM:
+			tbld = rsc;
+			newd = request->resource;
+
+			if (strncmp(newd->name, tbld->name,
+				    sizeof(*tbld->name)))
+				break;
+
+			memcpy(tbld, newd, request->size);
+
+			return updated;
+		case RSC_TRACE:
+			tblt = rsc;
+			newt = request->resource;
+
+			if (strncmp(newt->name, tblt->name,
+				    sizeof(*tblt->name)))
+				break;
+
+			memcpy(tblt, newt, request->size);
+
+			return updated;
+		default:
+			dev_err(&rproc->dev,
+				"Unsupported resource type: %d\n",
+				hdr->type);
+			return -EINVAL;
+		}
+	}
+
+	return !updated;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
-- 
2.9.0

[PATCH 5/9] remoteproc: core: Add function to amend an existing resource table entry

[Lee Jones]

Sometimes the firmware does not know best.

When a firmware is built, it can be loaded with a resource table, usually
detailing shared; memory, virtual device, trace log information etc.
However, some vendors require this hard-coded information to be amended
with new/improved information obtained from Device Tree for instance.

Until now, no method exists which allows the resource table to be amended.
The addition of this function changes that.  It is now possible to pull in
a resource table and amend it before it is finally shared with the remote
device.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 0abfa2b..3318ebd 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -917,6 +917,69 @@ int rproc_request_resource(struct rproc *rproc, u32 type, void *resource)
 }
 EXPORT_SYMBOL(rproc_request_resource);
 
+static int rproc_update_resource_table_entry(struct rproc *rproc,
+				struct rproc_request_resource *request,
+				struct resource_table *table, int size)
+{
+	struct fw_rsc_carveout *tblc, *newc;
+	struct fw_rsc_devmem *tbld, *newd;
+	struct fw_rsc_trace *tblt, *newt;
+	int updated = true;
+	int i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		if (request->type != hdr->type)
+			continue;
+
+		switch (hdr->type) {
+		case RSC_CARVEOUT:
+			tblc = rsc;
+			newc = request->resource;
+
+			if (strncmp(newc->name, tblc->name,
+				    sizeof(*tblc->name)))
+				break;
+
+			memcpy(tblc, newc, request->size);
+
+			return updated;
+		case RSC_DEVMEM:
+			tbld = rsc;
+			newd = request->resource;
+
+			if (strncmp(newd->name, tbld->name,
+				    sizeof(*tbld->name)))
+				break;
+
+			memcpy(tbld, newd, request->size);
+
+			return updated;
+		case RSC_TRACE:
+			tblt = rsc;
+			newt = request->resource;
+
+			if (strncmp(newt->name, tblt->name,
+				    sizeof(*tblt->name)))
+				break;
+
+			memcpy(tblt, newt, request->size);
+
+			return updated;
+		default:
+			dev_err(&rproc->dev,
+				"Unsupported resource type: %d\n",
+				hdr->type);
+			return -EINVAL;
+		}
+	}
+
+	return !updated;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
-- 
2.9.0

[PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[Lee Jones]

A new function now exists to pull in and amend and existing resource
table entry.  But what if we wish to provide a new resource?  This
function provides functionality to append a brand new resource entry
onto the resource table.  All complexity related to shuffling parts
of the table around, providing new offsets and incriminating number
of entries in the resource table's top-level header is taken care of
here.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3318ebd..111350e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -980,6 +980,61 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
 	return !updated;
 }
 
+static struct resource_table*
+rproc_add_resource_table_entry(struct rproc *rproc,
+			       struct rproc_request_resource *request,
+			       struct resource_table *old_table, int *tablesz)
+{
+	struct resource_table *table;
+	struct fw_rsc_hdr h;
+	void *new_rsc_loc;
+	void *fw_header_loc;
+	void *start_of_rscs;
+	int new_rsc_offset;
+	int size = *tablesz;
+	int i;
+
+	h.type = request->type;
+
+	new_rsc_offset = size;
+
+	/*
+	 * Allocate another contiguous chunk of memory, large enough to
+	 * contain the new, expanded resource table.
+	 *
+	 * The +4 is for the extra offset[] element in the top level header
+	 */
+	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
+	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
+	if (!table)
+		return ERR_PTR(-ENOMEM);
+
+	/* Shunt table by 4 Bytes to account for the extra offset[] element */
+	start_of_rscs = (void *)table + table->offset[0];
+	memmove(start_of_rscs + 4,
+		start_of_rscs, new_rsc_offset - table->offset[0]);
+	new_rsc_offset += 4;
+
+	/* Update existing resource entry's offsets */
+	for (i = 0; i < table->num; i++)
+		table->offset[i] += 4;
+
+	/* Update the top level 'resource table' header */
+	table->offset[table->num] = new_rsc_offset;
+	table->num++;
+
+	/* Copy new firmware header into table */
+	fw_header_loc = (void *)table + new_rsc_offset;
+	memcpy(fw_header_loc, &h, sizeof(h));
+
+	/* Copy new resource entry into table */
+	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
+	memcpy(new_rsc_loc, request->resource, request->size);
+
+	*tablesz = size;
+	return table;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
-- 
2.9.0

[PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[Lee Jones]

A new function now exists to pull in and amend and existing resource
table entry.  But what if we wish to provide a new resource?  This
function provides functionality to append a brand new resource entry
onto the resource table.  All complexity related to shuffling parts
of the table around, providing new offsets and incriminating number
of entries in the resource table's top-level header is taken care of
here.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3318ebd..111350e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -980,6 +980,61 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
 	return !updated;
 }
 
+static struct resource_table*
+rproc_add_resource_table_entry(struct rproc *rproc,
+			       struct rproc_request_resource *request,
+			       struct resource_table *old_table, int *tablesz)
+{
+	struct resource_table *table;
+	struct fw_rsc_hdr h;
+	void *new_rsc_loc;
+	void *fw_header_loc;
+	void *start_of_rscs;
+	int new_rsc_offset;
+	int size = *tablesz;
+	int i;
+
+	h.type = request->type;
+
+	new_rsc_offset = size;
+
+	/*
+	 * Allocate another contiguous chunk of memory, large enough to
+	 * contain the new, expanded resource table.
+	 *
+	 * The +4 is for the extra offset[] element in the top level header
+	 */
+	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
+	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
+	if (!table)
+		return ERR_PTR(-ENOMEM);
+
+	/* Shunt table by 4 Bytes to account for the extra offset[] element */
+	start_of_rscs = (void *)table + table->offset[0];
+	memmove(start_of_rscs + 4,
+		start_of_rscs, new_rsc_offset - table->offset[0]);
+	new_rsc_offset += 4;
+
+	/* Update existing resource entry's offsets */
+	for (i = 0; i < table->num; i++)
+		table->offset[i] += 4;
+
+	/* Update the top level 'resource table' header */
+	table->offset[table->num] = new_rsc_offset;
+	table->num++;
+
+	/* Copy new firmware header into table */
+	fw_header_loc = (void *)table + new_rsc_offset;
+	memcpy(fw_header_loc, &h, sizeof(h));
+
+	/* Copy new resource entry into table */
+	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
+	memcpy(new_rsc_loc, request->resource, request->size);
+
+	*tablesz = size;
+	return table;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
-- 
2.9.0

Re: [PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[loic pallardy]

Hi Lee,

I just tested your series and found issue with append mechanism.
There is no problem to add resources when working on Linux side, but the 
resource table is growing and when copying it at loaded location (ie 
overwriting existing prebuilt resource table of firmware), you have an 
overflow corrupting part of firmware code.

Moreover firmware code is in general tuned to a feature set. Resource 
table is created according to supported features. In most of the cases, 
new resource won't be handled by firmware.

Regards,
Loic


On 08/04/2016 11:21 AM, Lee Jones wrote:
> A new function now exists to pull in and amend and existing resource
> table entry.  But what if we wish to provide a new resource?  This
> function provides functionality to append a brand new resource entry
> onto the resource table.  All complexity related to shuffling parts
> of the table around, providing new offsets and incriminating number
> of entries in the resource table's top-level header is taken care of
> here.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3318ebd..111350e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -980,6 +980,61 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>   	return !updated;
>   }
>
> +static struct resource_table*
> +rproc_add_resource_table_entry(struct rproc *rproc,
> +			       struct rproc_request_resource *request,
> +			       struct resource_table *old_table, int *tablesz)
> +{
> +	struct resource_table *table;
> +	struct fw_rsc_hdr h;
> +	void *new_rsc_loc;
> +	void *fw_header_loc;
> +	void *start_of_rscs;
> +	int new_rsc_offset;
> +	int size = *tablesz;
> +	int i;
> +
> +	h.type = request->type;
> +
> +	new_rsc_offset = size;
> +
> +	/*
> +	 * Allocate another contiguous chunk of memory, large enough to
> +	 * contain the new, expanded resource table.
> +	 *
> +	 * The +4 is for the extra offset[] element in the top level header
> +	 */
> +	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
> +	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
> +	if (!table)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Shunt table by 4 Bytes to account for the extra offset[] element */
> +	start_of_rscs = (void *)table + table->offset[0];
> +	memmove(start_of_rscs + 4,
> +		start_of_rscs, new_rsc_offset - table->offset[0]);
> +	new_rsc_offset += 4;
> +
> +	/* Update existing resource entry's offsets */
> +	for (i = 0; i < table->num; i++)
> +		table->offset[i] += 4;
> +
> +	/* Update the top level 'resource table' header */
> +	table->offset[table->num] = new_rsc_offset;
> +	table->num++;
> +
> +	/* Copy new firmware header into table */
> +	fw_header_loc = (void *)table + new_rsc_offset;
> +	memcpy(fw_header_loc, &h, sizeof(h));
> +
> +	/* Copy new resource entry into table */
> +	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
> +	memcpy(new_rsc_loc, request->resource, request->size);
> +
> +	*tablesz = size;
> +	return table;
> +}
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
>

[PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[loic pallardy]

Hi Lee,

I just tested your series and found issue with append mechanism.
There is no problem to add resources when working on Linux side, but the 
resource table is growing and when copying it at loaded location (ie 
overwriting existing prebuilt resource table of firmware), you have an 
overflow corrupting part of firmware code.

Moreover firmware code is in general tuned to a feature set. Resource 
table is created according to supported features. In most of the cases, 
new resource won't be handled by firmware.

Regards,
Loic


On 08/04/2016 11:21 AM, Lee Jones wrote:
> A new function now exists to pull in and amend and existing resource
> table entry.  But what if we wish to provide a new resource?  This
> function provides functionality to append a brand new resource entry
> onto the resource table.  All complexity related to shuffling parts
> of the table around, providing new offsets and incriminating number
> of entries in the resource table's top-level header is taken care of
> here.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3318ebd..111350e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -980,6 +980,61 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>   	return !updated;
>   }
>
> +static struct resource_table*
> +rproc_add_resource_table_entry(struct rproc *rproc,
> +			       struct rproc_request_resource *request,
> +			       struct resource_table *old_table, int *tablesz)
> +{
> +	struct resource_table *table;
> +	struct fw_rsc_hdr h;
> +	void *new_rsc_loc;
> +	void *fw_header_loc;
> +	void *start_of_rscs;
> +	int new_rsc_offset;
> +	int size = *tablesz;
> +	int i;
> +
> +	h.type = request->type;
> +
> +	new_rsc_offset = size;
> +
> +	/*
> +	 * Allocate another contiguous chunk of memory, large enough to
> +	 * contain the new, expanded resource table.
> +	 *
> +	 * The +4 is for the extra offset[] element in the top level header
> +	 */
> +	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
> +	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
> +	if (!table)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Shunt table by 4 Bytes to account for the extra offset[] element */
> +	start_of_rscs = (void *)table + table->offset[0];
> +	memmove(start_of_rscs + 4,
> +		start_of_rscs, new_rsc_offset - table->offset[0]);
> +	new_rsc_offset += 4;
> +
> +	/* Update existing resource entry's offsets */
> +	for (i = 0; i < table->num; i++)
> +		table->offset[i] += 4;
> +
> +	/* Update the top level 'resource table' header */
> +	table->offset[table->num] = new_rsc_offset;
> +	table->num++;
> +
> +	/* Copy new firmware header into table */
> +	fw_header_loc = (void *)table + new_rsc_offset;
> +	memcpy(fw_header_loc, &h, sizeof(h));
> +
> +	/* Copy new resource entry into table */
> +	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
> +	memcpy(new_rsc_loc, request->resource, request->size);
> +
> +	*tablesz = size;
> +	return table;
> +}
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
>

Re: [PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[loic pallardy]

Hi Lee,

I just tested your series and found issue with append mechanism.
There is no problem to add resources when working on Linux side, but the 
resource table is growing and when copying it at loaded location (ie 
overwriting existing prebuilt resource table of firmware), you have an 
overflow corrupting part of firmware code.

Moreover firmware code is in general tuned to a feature set. Resource 
table is created according to supported features. In most of the cases, 
new resource won't be handled by firmware.

Regards,
Loic


On 08/04/2016 11:21 AM, Lee Jones wrote:
> A new function now exists to pull in and amend and existing resource
> table entry.  But what if we wish to provide a new resource?  This
> function provides functionality to append a brand new resource entry
> onto the resource table.  All complexity related to shuffling parts
> of the table around, providing new offsets and incriminating number
> of entries in the resource table's top-level header is taken care of
> here.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 55 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3318ebd..111350e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -980,6 +980,61 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,
>   	return !updated;
>   }
>
> +static struct resource_table*
> +rproc_add_resource_table_entry(struct rproc *rproc,
> +			       struct rproc_request_resource *request,
> +			       struct resource_table *old_table, int *tablesz)
> +{
> +	struct resource_table *table;
> +	struct fw_rsc_hdr h;
> +	void *new_rsc_loc;
> +	void *fw_header_loc;
> +	void *start_of_rscs;
> +	int new_rsc_offset;
> +	int size = *tablesz;
> +	int i;
> +
> +	h.type = request->type;
> +
> +	new_rsc_offset = size;
> +
> +	/*
> +	 * Allocate another contiguous chunk of memory, large enough to
> +	 * contain the new, expanded resource table.
> +	 *
> +	 * The +4 is for the extra offset[] element in the top level header
> +	 */
> +	size += sizeof(struct fw_rsc_hdr) + request->size + 4;
> +	table = devm_kmemdup(&rproc->dev, old_table, size, GFP_KERNEL);
> +	if (!table)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Shunt table by 4 Bytes to account for the extra offset[] element */
> +	start_of_rscs = (void *)table + table->offset[0];
> +	memmove(start_of_rscs + 4,
> +		start_of_rscs, new_rsc_offset - table->offset[0]);
> +	new_rsc_offset += 4;
> +
> +	/* Update existing resource entry's offsets */
> +	for (i = 0; i < table->num; i++)
> +		table->offset[i] += 4;
> +
> +	/* Update the top level 'resource table' header */
> +	table->offset[table->num] = new_rsc_offset;
> +	table->num++;
> +
> +	/* Copy new firmware header into table */
> +	fw_header_loc = (void *)table + new_rsc_offset;
> +	memcpy(fw_header_loc, &h, sizeof(h));
> +
> +	/* Copy new resource entry into table */
> +	new_rsc_loc = (void *)fw_header_loc + sizeof(h);
> +	memcpy(new_rsc_loc, request->resource, request->size);
> +
> +	*tablesz = size;
> +	return table;
> +}
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
>

Re: [PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[Bjorn Andersson]

On Thu 11 Aug 00:51 PDT 2016, loic pallardy wrote:

> Hi Lee,
> 

Loic, please don't top-post.

> I just tested your series and found issue with append mechanism.
> There is no problem to add resources when working on Linux side, but the
> resource table is growing and when copying it at loaded location (ie
> overwriting existing prebuilt resource table of firmware), you have an
> overflow corrupting part of firmware code.
> 

Suman brought up the same concern. For one it shows that we must check
the size of the .resource_table to know if we can fit an expanded table
before installing it.

> Moreover firmware code is in general tuned to a feature set. Resource table
> is created according to supported features. In most of the cases, new
> resource won't be handled by firmware.
> 

For the case behind this implementation, where you have resource
information from e.g. DT and build up a resource table (to be installed)
from that, how would you deal with this? Would you build your firmware
with room for some amount of resources?



As my (not the maintainer-me) need for this is purely on the Linux side
I originally envisioned something where we during firmware load parse
the resource_table into some Linux side data structures; we would allow
for these to be merged with additional data or new ones added and Linux
would handle these.

At the end we would have modified the referenced resource_table (through
references as it isn't the primary data structure) and could copy this.

Or alternatively, in the case you described with an empty start and
resources only from DT, we could have a resource-table-installer that
would make up a resource table from these Linux-side lists of resources.


This path would solve the case that we would not automatically grow the
table with new resources, but for the case where we generate a resource
table at the end we would still have the same issues to conclude on.

Regards,
Bjorn

[PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[Bjorn Andersson]

On Thu 11 Aug 00:51 PDT 2016, loic pallardy wrote:

> Hi Lee,
> 

Loic, please don't top-post.

> I just tested your series and found issue with append mechanism.
> There is no problem to add resources when working on Linux side, but the
> resource table is growing and when copying it at loaded location (ie
> overwriting existing prebuilt resource table of firmware), you have an
> overflow corrupting part of firmware code.
> 

Suman brought up the same concern. For one it shows that we must check
the size of the .resource_table to know if we can fit an expanded table
before installing it.

> Moreover firmware code is in general tuned to a feature set. Resource table
> is created according to supported features. In most of the cases, new
> resource won't be handled by firmware.
> 

For the case behind this implementation, where you have resource
information from e.g. DT and build up a resource table (to be installed)
from that, how would you deal with this? Would you build your firmware
with room for some amount of resources?



As my (not the maintainer-me) need for this is purely on the Linux side
I originally envisioned something where we during firmware load parse
the resource_table into some Linux side data structures; we would allow
for these to be merged with additional data or new ones added and Linux
would handle these.

At the end we would have modified the referenced resource_table (through
references as it isn't the primary data structure) and could copy this.

Or alternatively, in the case you described with an empty start and
resources only from DT, we could have a resource-table-installer that
would make up a resource table from these Linux-side lists of resources.


This path would solve the case that we would not automatically grow the
table with new resources, but for the case where we generate a resource
table at the end we would still have the same issues to conclude on.

Regards,
Bjorn

Re: [PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[loic pallardy]

On 08/11/2016 10:20 PM, Bjorn Andersson wrote:
> On Thu 11 Aug 00:51 PDT 2016, loic pallardy wrote:
>
>> Hi Lee,
>>
>
> Loic, please don't top-post.

Hi Bjorn,

Thanks for the advice, I'll.

>
>> I just tested your series and found issue with append mechanism.
>> There is no problem to add resources when working on Linux side, but the
>> resource table is growing and when copying it at loaded location (ie
>> overwriting existing prebuilt resource table of firmware), you have an
>> overflow corrupting part of firmware code.
>>
>
> Suman brought up the same concern. For one it shows that we must check
> the size of the .resource_table to know if we can fit an expanded table
> before installing it.
>
Yes I saw Suman's reply just after my answer.

>> Moreover firmware code is in general tuned to a feature set. Resource table
>> is created according to supported features. In most of the cases, new
>> resource won't be handled by firmware.
>>
>
> For the case behind this implementation, where you have resource
> information from e.g. DT and build up a resource table (to be installed)
> from that, how would you deal with this? Would you build your firmware
> with room for some amount of resources?
>
In general host is filling existing resources defined in firmware 
resource table (using DT information).
Resource table is built according to firmware capabilities. No room with 
current firmware (current status on ST side).

>
>
> As my (not the maintainer-me) need for this is purely on the Linux side
> I originally envisioned something where we during firmware load parse
> the resource_table into some Linux side data structures; we would allow
> for these to be merged with additional data or new ones added and Linux
> would handle these.
>
Yes agree, working on this point to differentiate:
- resource we want to modify in firmware resource table,
- resource we want to handle on Linux side,
- resource we want to append is possible to existing resource table

> At the end we would have modified the referenced resource_table (through
> references as it isn't the primary data structure) and could copy this.

Copy is an issue if not spare bytes are reserved on firmware side.
Moreover firmware should be able to handle new resources on it side.
In general, resource table fits firmware capabilities. If a resource is 
not present, that means firmware won't be able to handle it.
(I'm speaking about basic firmware with limited feature set)

>
> Or alternatively, in the case you described with an empty start and
> resources only from DT, we could have a resource-table-installer that
> would make up a resource table from these Linux-side lists of resources.
>
I had long discussion with Lee about firmware resource table extension. 
Indeed we can create some "free" resource slots that host can 
dynamically fill, or a new resource type (like RSV_SPARE) providing 
information about spare bytes which can be used by host to append 
resource table.
Lee should work on a proposal...

Regards,
Loic
>
> This path would solve the case that we would not automatically grow the
> table with new resources, but for the case where we generate a resource
> table at the end we would still have the same issues to conclude on.
>
> Regards,
> Bjorn
>

[PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[loic pallardy]

On 08/11/2016 10:20 PM, Bjorn Andersson wrote:
> On Thu 11 Aug 00:51 PDT 2016, loic pallardy wrote:
>
>> Hi Lee,
>>
>
> Loic, please don't top-post.

Hi Bjorn,

Thanks for the advice, I'll.

>
>> I just tested your series and found issue with append mechanism.
>> There is no problem to add resources when working on Linux side, but the
>> resource table is growing and when copying it at loaded location (ie
>> overwriting existing prebuilt resource table of firmware), you have an
>> overflow corrupting part of firmware code.
>>
>
> Suman brought up the same concern. For one it shows that we must check
> the size of the .resource_table to know if we can fit an expanded table
> before installing it.
>
Yes I saw Suman's reply just after my answer.

>> Moreover firmware code is in general tuned to a feature set. Resource table
>> is created according to supported features. In most of the cases, new
>> resource won't be handled by firmware.
>>
>
> For the case behind this implementation, where you have resource
> information from e.g. DT and build up a resource table (to be installed)
> from that, how would you deal with this? Would you build your firmware
> with room for some amount of resources?
>
In general host is filling existing resources defined in firmware 
resource table (using DT information).
Resource table is built according to firmware capabilities. No room with 
current firmware (current status on ST side).

>
>
> As my (not the maintainer-me) need for this is purely on the Linux side
> I originally envisioned something where we during firmware load parse
> the resource_table into some Linux side data structures; we would allow
> for these to be merged with additional data or new ones added and Linux
> would handle these.
>
Yes agree, working on this point to differentiate:
- resource we want to modify in firmware resource table,
- resource we want to handle on Linux side,
- resource we want to append is possible to existing resource table

> At the end we would have modified the referenced resource_table (through
> references as it isn't the primary data structure) and could copy this.

Copy is an issue if not spare bytes are reserved on firmware side.
Moreover firmware should be able to handle new resources on it side.
In general, resource table fits firmware capabilities. If a resource is 
not present, that means firmware won't be able to handle it.
(I'm speaking about basic firmware with limited feature set)

>
> Or alternatively, in the case you described with an empty start and
> resources only from DT, we could have a resource-table-installer that
> would make up a resource table from these Linux-side lists of resources.
>
I had long discussion with Lee about firmware resource table extension. 
Indeed we can create some "free" resource slots that host can 
dynamically fill, or a new resource type (like RSV_SPARE) providing 
information about spare bytes which can be used by host to append 
resource table.
Lee should work on a proposal...

Regards,
Loic
>
> This path would solve the case that we would not automatically grow the
> table with new resources, but for the case where we generate a resource
> table at the end we would still have the same issues to conclude on.
>
> Regards,
> Bjorn
>

Re: [PATCH 6/9] remoteproc: core: Add function to append a new resource table entry

[loic pallardy]

On 08/11/2016 10:20 PM, Bjorn Andersson wrote:
> On Thu 11 Aug 00:51 PDT 2016, loic pallardy wrote:
>
>> Hi Lee,
>>
>
> Loic, please don't top-post.

Hi Bjorn,

Thanks for the advice, I'll.

>
>> I just tested your series and found issue with append mechanism.
>> There is no problem to add resources when working on Linux side, but the
>> resource table is growing and when copying it at loaded location (ie
>> overwriting existing prebuilt resource table of firmware), you have an
>> overflow corrupting part of firmware code.
>>
>
> Suman brought up the same concern. For one it shows that we must check
> the size of the .resource_table to know if we can fit an expanded table
> before installing it.
>
Yes I saw Suman's reply just after my answer.

>> Moreover firmware code is in general tuned to a feature set. Resource table
>> is created according to supported features. In most of the cases, new
>> resource won't be handled by firmware.
>>
>
> For the case behind this implementation, where you have resource
> information from e.g. DT and build up a resource table (to be installed)
> from that, how would you deal with this? Would you build your firmware
> with room for some amount of resources?
>
In general host is filling existing resources defined in firmware 
resource table (using DT information).
Resource table is built according to firmware capabilities. No room with 
current firmware (current status on ST side).

>
>
> As my (not the maintainer-me) need for this is purely on the Linux side
> I originally envisioned something where we during firmware load parse
> the resource_table into some Linux side data structures; we would allow
> for these to be merged with additional data or new ones added and Linux
> would handle these.
>
Yes agree, working on this point to differentiate:
- resource we want to modify in firmware resource table,
- resource we want to handle on Linux side,
- resource we want to append is possible to existing resource table

> At the end we would have modified the referenced resource_table (through
> references as it isn't the primary data structure) and could copy this.

Copy is an issue if not spare bytes are reserved on firmware side.
Moreover firmware should be able to handle new resources on it side.
In general, resource table fits firmware capabilities. If a resource is 
not present, that means firmware won't be able to handle it.
(I'm speaking about basic firmware with limited feature set)

>
> Or alternatively, in the case you described with an empty start and
> resources only from DT, we could have a resource-table-installer that
> would make up a resource table from these Linux-side lists of resources.
>
I had long discussion with Lee about firmware resource table extension. 
Indeed we can create some "free" resource slots that host can 
dynamically fill, or a new resource type (like RSV_SPARE) providing 
information about spare bytes which can be used by host to append 
resource table.
Lee should work on a proposal...

Regards,
Loic
>
> This path would solve the case that we would not automatically grow the
> table with new resources, but for the case where we generate a resource
> table at the end we would still have the same issues to conclude on.
>
> Regards,
> Bjorn
>

[PATCH 7/9] remoteproc: core: Add function to over-ride current resource table

[Lee Jones]

Most of the new resource table handling function are now in place, so
it's time to put it all together.  Once new resource table information
has been requested, the structures will be held in a holding pen until
boot-time.  During boot-time rproc_apply_resource_overrides() will be
invoked which in turn will pull the new information out of the holding
pen and edit the table accordingly.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 111350e..6b4e29a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1035,6 +1035,65 @@ rproc_add_resource_table_entry(struct rproc *rproc,
 	return table;
 }
 
+static struct resource_table*
+rproc_apply_resource_overrides(struct rproc *rproc,
+			       struct resource_table **orig_table,
+			       int *tablesz)
+{
+	struct rproc_request_resource *resource;
+	struct resource_table *table = *orig_table;
+	int size = *tablesz;
+
+	if (!table && size != 0) {
+		dev_err(&rproc->dev, "No table present but table size is set\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	mutex_lock(&rproc->lock);
+
+	rproc_dump_resource_table(rproc, table, size);
+
+	if (!table) {
+		size = sizeof(*table);
+		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
+		if (!table) {
+			table = ERR_PTR(-ENOMEM);
+			goto out;
+		}
+		table->ver = 1;
+	}
+
+	list_for_each_entry(resource, &rproc->override_resources, node) {
+		int updated = 0;
+
+		/* If we already have a table, update it with the new values. */
+		updated = rproc_update_resource_table_entry(rproc, resource,
+							    table, size);
+		if (updated < 0) {
+			table = ERR_PTR(updated);
+			goto out;
+		}
+		if (updated)
+			continue;
+
+		/* Didn't find matching resource entry -- creating a new one. */
+		table = rproc_add_resource_table_entry(rproc, resource,
+						       table, &size);
+		if (IS_ERR(table))
+			goto out;
+
+		*orig_table = table;
+	}
+
+	rproc_dump_resource_table(rproc, table, size);
+
+	*tablesz = size;
+
+ out:
+	mutex_unlock(&rproc->lock);
+	return table;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1153,6 +1212,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	if (!table)
 		goto out;
 
+	if (!list_empty(&rproc->override_resources)) {
+		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
+		if (IS_ERR(table))
+			goto out;
+	}
+
 	rproc->table_csum = crc32(0, table, tablesz);
 
 	/*
-- 
2.9.0

[PATCH 7/9] remoteproc: core: Add function to over-ride current resource table

[Lee Jones]

Most of the new resource table handling function are now in place, so
it's time to put it all together.  Once new resource table information
has been requested, the structures will be held in a holding pen until
boot-time.  During boot-time rproc_apply_resource_overrides() will be
invoked which in turn will pull the new information out of the holding
pen and edit the table accordingly.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 111350e..6b4e29a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1035,6 +1035,65 @@ rproc_add_resource_table_entry(struct rproc *rproc,
 	return table;
 }
 
+static struct resource_table*
+rproc_apply_resource_overrides(struct rproc *rproc,
+			       struct resource_table **orig_table,
+			       int *tablesz)
+{
+	struct rproc_request_resource *resource;
+	struct resource_table *table = *orig_table;
+	int size = *tablesz;
+
+	if (!table && size != 0) {
+		dev_err(&rproc->dev, "No table present but table size is set\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	mutex_lock(&rproc->lock);
+
+	rproc_dump_resource_table(rproc, table, size);
+
+	if (!table) {
+		size = sizeof(*table);
+		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
+		if (!table) {
+			table = ERR_PTR(-ENOMEM);
+			goto out;
+		}
+		table->ver = 1;
+	}
+
+	list_for_each_entry(resource, &rproc->override_resources, node) {
+		int updated = 0;
+
+		/* If we already have a table, update it with the new values. */
+		updated = rproc_update_resource_table_entry(rproc, resource,
+							    table, size);
+		if (updated < 0) {
+			table = ERR_PTR(updated);
+			goto out;
+		}
+		if (updated)
+			continue;
+
+		/* Didn't find matching resource entry -- creating a new one. */
+		table = rproc_add_resource_table_entry(rproc, resource,
+						       table, &size);
+		if (IS_ERR(table))
+			goto out;
+
+		*orig_table = table;
+	}
+
+	rproc_dump_resource_table(rproc, table, size);
+
+	*tablesz = size;
+
+ out:
+	mutex_unlock(&rproc->lock);
+	return table;
+}
+
 /*
  * take a firmware and boot a remote processor with it.
  */
@@ -1153,6 +1212,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	if (!table)
 		goto out;
 
+	if (!list_empty(&rproc->override_resources)) {
+		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
+		if (IS_ERR(table))
+			goto out;
+	}
+
 	rproc->table_csum = crc32(0, table, tablesz);
 
 	/*
-- 
2.9.0

Re: [PATCH 7/9] remoteproc: core: Add function to over-ride current resource table

[loic pallardy]

Hi Lee

On 08/04/2016 11:21 AM, Lee Jones wrote:
> Most of the new resource table handling function are now in place, so
> it's time to put it all together.  Once new resource table information
> has been requested, the structures will be held in a holding pen until
> boot-time.  During boot-time rproc_apply_resource_overrides() will be
> invoked which in turn will pull the new information out of the holding
> pen and edit the table accordingly.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 65 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 111350e..6b4e29a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1035,6 +1035,65 @@ rproc_add_resource_table_entry(struct rproc *rproc,
>   	return table;
>   }
>
> +static struct resource_table*
> +rproc_apply_resource_overrides(struct rproc *rproc,
> +			       struct resource_table **orig_table,
> +			       int *tablesz)
> +{
> +	struct rproc_request_resource *resource;
> +	struct resource_table *table = *orig_table;
> +	int size = *tablesz;
> +
> +	if (!table && size != 0) {
> +		dev_err(&rproc->dev, "No table present but table size is set\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	mutex_lock(&rproc->lock);
> +
> +	rproc_dump_resource_table(rproc, table, size);
It will be good to find a way to not dump resource table if debug is not 
activated. For the same reason as you mentioned in your patch 8, 
resource table parsing will waste CPUS cycles.

> +
> +	if (!table) {
> +		size = sizeof(*table);
> +		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
> +		if (!table) {
> +			table = ERR_PTR(-ENOMEM);
> +			goto out;
> +		}
> +		table->ver = 1;
> +	}
> +
> +	list_for_each_entry(resource, &rproc->override_resources, node) {
> +		int updated = 0;
> +
> +		/* If we already have a table, update it with the new values. */
> +		updated = rproc_update_resource_table_entry(rproc, resource,
> +							    table, size);
> +		if (updated < 0) {
> +			table = ERR_PTR(updated);
> +			goto out;
> +		}
> +		if (updated)
> +			continue;
> +
> +		/* Didn't find matching resource entry -- creating a new one. */
> +		table = rproc_add_resource_table_entry(rproc, resource,
> +						       table, &size);
> +		if (IS_ERR(table))
> +			goto out;
> +
> +		*orig_table = table;
> +	}
> +
> +	rproc_dump_resource_table(rproc, table, size);
ditto

Regards,
Loic
> +
> +	*tablesz = size;
> +
> + out:
> +	mutex_unlock(&rproc->lock);
> +	return table;
> +}
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
> @@ -1153,6 +1212,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>   	if (!table)
>   		goto out;
>
> +	if (!list_empty(&rproc->override_resources)) {
> +		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
> +		if (IS_ERR(table))
> +			goto out;
> +	}
> +
>   	rproc->table_csum = crc32(0, table, tablesz);
>
>   	/*
>

[PATCH 7/9] remoteproc: core: Add function to over-ride current resource table

[loic pallardy]

Hi Lee

On 08/04/2016 11:21 AM, Lee Jones wrote:
> Most of the new resource table handling function are now in place, so
> it's time to put it all together.  Once new resource table information
> has been requested, the structures will be held in a holding pen until
> boot-time.  During boot-time rproc_apply_resource_overrides() will be
> invoked which in turn will pull the new information out of the holding
> pen and edit the table accordingly.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 65 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 111350e..6b4e29a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1035,6 +1035,65 @@ rproc_add_resource_table_entry(struct rproc *rproc,
>   	return table;
>   }
>
> +static struct resource_table*
> +rproc_apply_resource_overrides(struct rproc *rproc,
> +			       struct resource_table **orig_table,
> +			       int *tablesz)
> +{
> +	struct rproc_request_resource *resource;
> +	struct resource_table *table = *orig_table;
> +	int size = *tablesz;
> +
> +	if (!table && size != 0) {
> +		dev_err(&rproc->dev, "No table present but table size is set\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	mutex_lock(&rproc->lock);
> +
> +	rproc_dump_resource_table(rproc, table, size);
It will be good to find a way to not dump resource table if debug is not 
activated. For the same reason as you mentioned in your patch 8, 
resource table parsing will waste CPUS cycles.

> +
> +	if (!table) {
> +		size = sizeof(*table);
> +		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
> +		if (!table) {
> +			table = ERR_PTR(-ENOMEM);
> +			goto out;
> +		}
> +		table->ver = 1;
> +	}
> +
> +	list_for_each_entry(resource, &rproc->override_resources, node) {
> +		int updated = 0;
> +
> +		/* If we already have a table, update it with the new values. */
> +		updated = rproc_update_resource_table_entry(rproc, resource,
> +							    table, size);
> +		if (updated < 0) {
> +			table = ERR_PTR(updated);
> +			goto out;
> +		}
> +		if (updated)
> +			continue;
> +
> +		/* Didn't find matching resource entry -- creating a new one. */
> +		table = rproc_add_resource_table_entry(rproc, resource,
> +						       table, &size);
> +		if (IS_ERR(table))
> +			goto out;
> +
> +		*orig_table = table;
> +	}
> +
> +	rproc_dump_resource_table(rproc, table, size);
ditto

Regards,
Loic
> +
> +	*tablesz = size;
> +
> + out:
> +	mutex_unlock(&rproc->lock);
> +	return table;
> +}
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
> @@ -1153,6 +1212,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>   	if (!table)
>   		goto out;
>
> +	if (!list_empty(&rproc->override_resources)) {
> +		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
> +		if (IS_ERR(table))
> +			goto out;
> +	}
> +
>   	rproc->table_csum = crc32(0, table, tablesz);
>
>   	/*
>

Re: [PATCH 7/9] remoteproc: core: Add function to over-ride current resource table

[loic pallardy]

Hi Lee

On 08/04/2016 11:21 AM, Lee Jones wrote:
> Most of the new resource table handling function are now in place, so
> it's time to put it all together.  Once new resource table information
> has been requested, the structures will be held in a holding pen until
> boot-time.  During boot-time rproc_apply_resource_overrides() will be
> invoked which in turn will pull the new information out of the holding
> pen and edit the table accordingly.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 65 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 111350e..6b4e29a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1035,6 +1035,65 @@ rproc_add_resource_table_entry(struct rproc *rproc,
>   	return table;
>   }
>
> +static struct resource_table*
> +rproc_apply_resource_overrides(struct rproc *rproc,
> +			       struct resource_table **orig_table,
> +			       int *tablesz)
> +{
> +	struct rproc_request_resource *resource;
> +	struct resource_table *table = *orig_table;
> +	int size = *tablesz;
> +
> +	if (!table && size != 0) {
> +		dev_err(&rproc->dev, "No table present but table size is set\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	mutex_lock(&rproc->lock);
> +
> +	rproc_dump_resource_table(rproc, table, size);
It will be good to find a way to not dump resource table if debug is not 
activated. For the same reason as you mentioned in your patch 8, 
resource table parsing will waste CPUS cycles.

> +
> +	if (!table) {
> +		size = sizeof(*table);
> +		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
> +		if (!table) {
> +			table = ERR_PTR(-ENOMEM);
> +			goto out;
> +		}
> +		table->ver = 1;
> +	}
> +
> +	list_for_each_entry(resource, &rproc->override_resources, node) {
> +		int updated = 0;
> +
> +		/* If we already have a table, update it with the new values. */
> +		updated = rproc_update_resource_table_entry(rproc, resource,
> +							    table, size);
> +		if (updated < 0) {
> +			table = ERR_PTR(updated);
> +			goto out;
> +		}
> +		if (updated)
> +			continue;
> +
> +		/* Didn't find matching resource entry -- creating a new one. */
> +		table = rproc_add_resource_table_entry(rproc, resource,
> +						       table, &size);
> +		if (IS_ERR(table))
> +			goto out;
> +
> +		*orig_table = table;
> +	}
> +
> +	rproc_dump_resource_table(rproc, table, size);
ditto

Regards,
Loic
> +
> +	*tablesz = size;
> +
> + out:
> +	mutex_unlock(&rproc->lock);
> +	return table;
> +}
> +
>   /*
>    * take a firmware and boot a remote processor with it.
>    */
> @@ -1153,6 +1212,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>   	if (!table)
>   		goto out;
>
> +	if (!list_empty(&rproc->override_resources)) {
> +		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
> +		if (IS_ERR(table))
> +			goto out;
> +	}
> +
>   	rproc->table_csum = crc32(0, table, tablesz);
>
>   	/*
>

Re: [PATCH 7/9] remoteproc: core: Add function to over-ride current resource table

[Lee Jones]

On Mon, 08 Aug 2016, loic pallardy wrote:

> Hi Lee
> 
> On 08/04/2016 11:21 AM, Lee Jones wrote:
> > Most of the new resource table handling function are now in place, so
> > it's time to put it all together.  Once new resource table information
> > has been requested, the structures will be held in a holding pen until
> > boot-time.  During boot-time rproc_apply_resource_overrides() will be
> > invoked which in turn will pull the new information out of the holding
> > pen and edit the table accordingly.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >   drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 65 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 111350e..6b4e29a 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1035,6 +1035,65 @@ rproc_add_resource_table_entry(struct rproc *rproc,
> >   	return table;
> >   }
> > 
> > +static struct resource_table*
> > +rproc_apply_resource_overrides(struct rproc *rproc,
> > +			       struct resource_table **orig_table,
> > +			       int *tablesz)
> > +{
> > +	struct rproc_request_resource *resource;
> > +	struct resource_table *table = *orig_table;
> > +	int size = *tablesz;
> > +
> > +	if (!table && size != 0) {
> > +		dev_err(&rproc->dev, "No table present but table size is set\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	mutex_lock(&rproc->lock);
> > +
> > +	rproc_dump_resource_table(rproc, table, size);
> It will be good to find a way to not dump resource table if debug is not
> activated. For the same reason as you mentioned in your patch 8, resource
> table parsing will waste CPUS cycles.

+1

I've been meaning to fix this, thanks.

> > +
> > +	if (!table) {
> > +		size = sizeof(*table);
> > +		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
> > +		if (!table) {
> > +			table = ERR_PTR(-ENOMEM);
> > +			goto out;
> > +		}
> > +		table->ver = 1;
> > +	}
> > +
> > +	list_for_each_entry(resource, &rproc->override_resources, node) {
> > +		int updated = 0;
> > +
> > +		/* If we already have a table, update it with the new values. */
> > +		updated = rproc_update_resource_table_entry(rproc, resource,
> > +							    table, size);
> > +		if (updated < 0) {
> > +			table = ERR_PTR(updated);
> > +			goto out;
> > +		}
> > +		if (updated)
> > +			continue;
> > +
> > +		/* Didn't find matching resource entry -- creating a new one. */
> > +		table = rproc_add_resource_table_entry(rproc, resource,
> > +						       table, &size);
> > +		if (IS_ERR(table))
> > +			goto out;
> > +
> > +		*orig_table = table;
> > +	}
> > +
> > +	rproc_dump_resource_table(rproc, table, size);
> ditto

Ditto

> > +
> > +	*tablesz = size;
> > +
> > + out:
> > +	mutex_unlock(&rproc->lock);
> > +	return table;
> > +}
> > +
> >   /*
> >    * take a firmware and boot a remote processor with it.
> >    */
> > @@ -1153,6 +1212,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
> >   	if (!table)
> >   		goto out;
> > 
> > +	if (!list_empty(&rproc->override_resources)) {
> > +		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
> > +		if (IS_ERR(table))
> > +			goto out;
> > +	}
> > +
> >   	rproc->table_csum = crc32(0, table, tablesz);
> > 
> >   	/*
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 7/9] remoteproc: core: Add function to over-ride current resource table

[Lee Jones]

On Mon, 08 Aug 2016, loic pallardy wrote:

> Hi Lee
> 
> On 08/04/2016 11:21 AM, Lee Jones wrote:
> > Most of the new resource table handling function are now in place, so
> > it's time to put it all together.  Once new resource table information
> > has been requested, the structures will be held in a holding pen until
> > boot-time.  During boot-time rproc_apply_resource_overrides() will be
> > invoked which in turn will pull the new information out of the holding
> > pen and edit the table accordingly.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >   drivers/remoteproc/remoteproc_core.c | 65 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 65 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 111350e..6b4e29a 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1035,6 +1035,65 @@ rproc_add_resource_table_entry(struct rproc *rproc,
> >   	return table;
> >   }
> > 
> > +static struct resource_table*
> > +rproc_apply_resource_overrides(struct rproc *rproc,
> > +			       struct resource_table **orig_table,
> > +			       int *tablesz)
> > +{
> > +	struct rproc_request_resource *resource;
> > +	struct resource_table *table = *orig_table;
> > +	int size = *tablesz;
> > +
> > +	if (!table && size != 0) {
> > +		dev_err(&rproc->dev, "No table present but table size is set\n");
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	mutex_lock(&rproc->lock);
> > +
> > +	rproc_dump_resource_table(rproc, table, size);
> It will be good to find a way to not dump resource table if debug is not
> activated. For the same reason as you mentioned in your patch 8, resource
> table parsing will waste CPUS cycles.

+1

I've been meaning to fix this, thanks.

> > +
> > +	if (!table) {
> > +		size = sizeof(*table);
> > +		table = devm_kzalloc(&rproc->dev, size, GFP_KERNEL);
> > +		if (!table) {
> > +			table = ERR_PTR(-ENOMEM);
> > +			goto out;
> > +		}
> > +		table->ver = 1;
> > +	}
> > +
> > +	list_for_each_entry(resource, &rproc->override_resources, node) {
> > +		int updated = 0;
> > +
> > +		/* If we already have a table, update it with the new values. */
> > +		updated = rproc_update_resource_table_entry(rproc, resource,
> > +							    table, size);
> > +		if (updated < 0) {
> > +			table = ERR_PTR(updated);
> > +			goto out;
> > +		}
> > +		if (updated)
> > +			continue;
> > +
> > +		/* Didn't find matching resource entry -- creating a new one. */
> > +		table = rproc_add_resource_table_entry(rproc, resource,
> > +						       table, &size);
> > +		if (IS_ERR(table))
> > +			goto out;
> > +
> > +		*orig_table = table;
> > +	}
> > +
> > +	rproc_dump_resource_table(rproc, table, size);
> ditto

Ditto

> > +
> > +	*tablesz = size;
> > +
> > + out:
> > +	mutex_unlock(&rproc->lock);
> > +	return table;
> > +}
> > +
> >   /*
> >    * take a firmware and boot a remote processor with it.
> >    */
> > @@ -1153,6 +1212,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
> >   	if (!table)
> >   		goto out;
> > 
> > +	if (!list_empty(&rproc->override_resources)) {
> > +		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
> > +		if (IS_ERR(table))
> > +			goto out;
> > +	}
> > +
> >   	rproc->table_csum = crc32(0, table, tablesz);
> > 
> >   	/*
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 8/9] remoteproc: core: Skip resource table integrity checks if there are amendments

[Lee Jones]

There is little point wasting CPU cycles completing integrity checking
(i.e. ensuring nothing has changed) on the resource table if we *know*
that it will be changed (by us).  In this patch we skip resource table
integrity checks if a platform-specific remoteproc driver has requested
an amendment or an appended entry.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6b4e29a..9a077e4 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1126,6 +1126,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
 	ret = -EINVAL;
 
+	/* If we've overridden the resource table, we know it's changed. */
+	if (!list_empty(&rproc->override_resources))
+		goto skip_table_check;
+
 	/* look for the resource table */
 	table = rproc_find_rsc_table(rproc, fw, &tablesz);
 	if (!table) {
@@ -1139,6 +1143,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+ skip_table_check:
+
 	/* handle fw resources which are required to boot rproc */
 	ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
 	if (ret) {
@@ -1216,10 +1222,15 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
 		if (IS_ERR(table))
 			goto out;
+
+		/* No point checking table if we know it *has* changed */
+		goto skip_table_check;
 	}
 
 	rproc->table_csum = crc32(0, table, tablesz);
 
+ skip_table_check:
+
 	/*
 	 * Create a copy of the resource table. When a virtio device starts
 	 * and calls vring_new_virtqueue() the address of the allocated vring
-- 
2.9.0

[PATCH 8/9] remoteproc: core: Skip resource table integrity checks if there are amendments

[Lee Jones]

There is little point wasting CPU cycles completing integrity checking
(i.e. ensuring nothing has changed) on the resource table if we *know*
that it will be changed (by us).  In this patch we skip resource table
integrity checks if a platform-specific remoteproc driver has requested
an amendment or an appended entry.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6b4e29a..9a077e4 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1126,6 +1126,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
 	ret = -EINVAL;
 
+	/* If we've overridden the resource table, we know it's changed. */
+	if (!list_empty(&rproc->override_resources))
+		goto skip_table_check;
+
 	/* look for the resource table */
 	table = rproc_find_rsc_table(rproc, fw, &tablesz);
 	if (!table) {
@@ -1139,6 +1143,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+ skip_table_check:
+
 	/* handle fw resources which are required to boot rproc */
 	ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
 	if (ret) {
@@ -1216,10 +1222,15 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
 		if (IS_ERR(table))
 			goto out;
+
+		/* No point checking table if we know it *has* changed */
+		goto skip_table_check;
 	}
 
 	rproc->table_csum = crc32(0, table, tablesz);
 
+ skip_table_check:
+
 	/*
 	 * Create a copy of the resource table. When a virtio device starts
 	 * and calls vring_new_virtqueue() the address of the allocated vring
-- 
2.9.0

Re: [PATCH 8/9] remoteproc: core: Skip resource table integrity checks if there are amendments

[Bjorn Andersson]

On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:

> There is little point wasting CPU cycles completing integrity checking
> (i.e. ensuring nothing has changed) on the resource table if we *know*
> that it will be changed (by us).  In this patch we skip resource table
> integrity checks if a platform-specific remoteproc driver has requested
> an amendment or an appended entry.
> 

If we apply the auto-boot series first, I believe this dance goes away.

Regards,
Bjorn

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 6b4e29a..9a077e4 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1126,6 +1126,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>  	ret = -EINVAL;
>  
> +	/* If we've overridden the resource table, we know it's changed. */
> +	if (!list_empty(&rproc->override_resources))
> +		goto skip_table_check;
> +
>  	/* look for the resource table */
>  	table = rproc_find_rsc_table(rproc, fw, &tablesz);
>  	if (!table) {
> @@ -1139,6 +1143,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  		goto clean_up;
>  	}
>  
> + skip_table_check:
> +
>  	/* handle fw resources which are required to boot rproc */
>  	ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
>  	if (ret) {
> @@ -1216,10 +1222,15 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>  		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
>  		if (IS_ERR(table))
>  			goto out;
> +
> +		/* No point checking table if we know it *has* changed */
> +		goto skip_table_check;
>  	}
>  
>  	rproc->table_csum = crc32(0, table, tablesz);
>  
> + skip_table_check:
> +
>  	/*
>  	 * Create a copy of the resource table. When a virtio device starts
>  	 * and calls vring_new_virtqueue() the address of the allocated vring
> -- 
> 2.9.0
> 

[PATCH 8/9] remoteproc: core: Skip resource table integrity checks if there are amendments

[Bjorn Andersson]

On Thu 04 Aug 02:21 PDT 2016, Lee Jones wrote:

> There is little point wasting CPU cycles completing integrity checking
> (i.e. ensuring nothing has changed) on the resource table if we *know*
> that it will be changed (by us).  In this patch we skip resource table
> integrity checks if a platform-specific remoteproc driver has requested
> an amendment or an appended entry.
> 

If we apply the auto-boot series first, I believe this dance goes away.

Regards,
Bjorn

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 6b4e29a..9a077e4 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1126,6 +1126,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	rproc->bootaddr = rproc_get_boot_addr(rproc, fw);
>  	ret = -EINVAL;
>  
> +	/* If we've overridden the resource table, we know it's changed. */
> +	if (!list_empty(&rproc->override_resources))
> +		goto skip_table_check;
> +
>  	/* look for the resource table */
>  	table = rproc_find_rsc_table(rproc, fw, &tablesz);
>  	if (!table) {
> @@ -1139,6 +1143,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  		goto clean_up;
>  	}
>  
> + skip_table_check:
> +
>  	/* handle fw resources which are required to boot rproc */
>  	ret = rproc_handle_resources(rproc, tablesz, rproc_loading_handlers);
>  	if (ret) {
> @@ -1216,10 +1222,15 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
>  		table = rproc_apply_resource_overrides(rproc, &table, &tablesz);
>  		if (IS_ERR(table))
>  			goto out;
> +
> +		/* No point checking table if we know it *has* changed */
> +		goto skip_table_check;
>  	}
>  
>  	rproc->table_csum = crc32(0, table, tablesz);
>  
> + skip_table_check:
> +
>  	/*
>  	 * Create a copy of the resource table. When a virtio device starts
>  	 * and calls vring_new_virtqueue() the address of the allocated vring
> -- 
> 2.9.0
> 

[PATCH 9/9] remoteproc: core: Support empty resource tables

[Lee Jones]

Currently, when a remote processor does not require resources, the
platform-specific remoteproc driver has to create a fake resource
table in order to by-pass the strict checking.  But there is no hard
requirement for a remote processor so require or support shared
resources.  This patch removes the strict checking and skips
resource table related operations if one is not provided.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9a077e4..366e197 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1104,9 +1104,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	struct resource_table *table, *loaded_table;
 	int ret, tablesz;
 
-	if (!rproc->table_ptr)
-		return -ENOMEM;
-
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
 		return ret;
@@ -1130,12 +1127,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	if (!list_empty(&rproc->override_resources))
 		goto skip_table_check;
 
-	/* look for the resource table */
+	/* Resource tables aren't compulsory. */
 	table = rproc_find_rsc_table(rproc, fw, &tablesz);
-	if (!table) {
-		dev_err(dev, "Failed to find resource table\n");
-		goto clean_up;
-	}
+	if (!table)
+		goto skip_resources;
 
 	/* Verify that resource table in loaded fw is unchanged */
 	if (rproc->table_csum != crc32(0, table, tablesz)) {
@@ -1152,6 +1147,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+ skip_resources:
+
 	/* load the ELF segments to memory */
 	ret = rproc_load_segments(rproc, fw);
 	if (ret) {
@@ -1213,8 +1210,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	if (rproc_fw_sanity_check(rproc, fw) < 0)
 		goto out;
 
-	/* look for the resource table */
 	table = rproc_find_rsc_table(rproc, fw,  &tablesz);
+	if (IS_ERR(table)) {
+		dev_err(&rproc->dev, "Resource table expected, but failed\n");
+		goto out;
+	}
+
 	if (!table)
 		goto out;
 
-- 
2.9.0

[PATCH 9/9] remoteproc: core: Support empty resource tables

[Lee Jones]

Currently, when a remote processor does not require resources, the
platform-specific remoteproc driver has to create a fake resource
table in order to by-pass the strict checking.  But there is no hard
requirement for a remote processor so require or support shared
resources.  This patch removes the strict checking and skips
resource table related operations if one is not provided.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9a077e4..366e197 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1104,9 +1104,6 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	struct resource_table *table, *loaded_table;
 	int ret, tablesz;
 
-	if (!rproc->table_ptr)
-		return -ENOMEM;
-
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
 		return ret;
@@ -1130,12 +1127,10 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	if (!list_empty(&rproc->override_resources))
 		goto skip_table_check;
 
-	/* look for the resource table */
+	/* Resource tables aren't compulsory. */
 	table = rproc_find_rsc_table(rproc, fw, &tablesz);
-	if (!table) {
-		dev_err(dev, "Failed to find resource table\n");
-		goto clean_up;
-	}
+	if (!table)
+		goto skip_resources;
 
 	/* Verify that resource table in loaded fw is unchanged */
 	if (rproc->table_csum != crc32(0, table, tablesz)) {
@@ -1152,6 +1147,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 		goto clean_up;
 	}
 
+ skip_resources:
+
 	/* load the ELF segments to memory */
 	ret = rproc_load_segments(rproc, fw);
 	if (ret) {
@@ -1213,8 +1210,12 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	if (rproc_fw_sanity_check(rproc, fw) < 0)
 		goto out;
 
-	/* look for the resource table */
 	table = rproc_find_rsc_table(rproc, fw,  &tablesz);
+	if (IS_ERR(table)) {
+		dev_err(&rproc->dev, "Resource table expected, but failed\n");
+		goto out;
+	}
+
 	if (!table)
 		goto out;
 
-- 
2.9.0

Re: [PATCH 9/9] remoteproc: core: Support empty resource tables

[Lee Jones]

On Thu, 04 Aug 2016, Lee Jones wrote:

> Currently, when a remote processor does not require resources, the
> platform-specific remoteproc driver has to create a fake resource
> table in order to by-pass the strict checking.  But there is no hard
> requirement for a remote processor so require or support shared
> resources.  This patch removes the strict checking and skips
> resource table related operations if one is not provided.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

Due to the fact that a) this patch conflicts heavily with Bjorn's set
and b) it isn't fully tested with real resourceless firmware, I'm
pulling it from the set.  I will submit it as part of a subsequent set
-- please ignore.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 9/9] remoteproc: core: Support empty resource tables

[Lee Jones]

On Thu, 04 Aug 2016, Lee Jones wrote:

> Currently, when a remote processor does not require resources, the
> platform-specific remoteproc driver has to create a fake resource
> table in order to by-pass the strict checking.  But there is no hard
> requirement for a remote processor so require or support shared
> resources.  This patch removes the strict checking and skips
> resource table related operations if one is not provided.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

Due to the fact that a) this patch conflicts heavily with Bjorn's set
and b) it isn't fully tested with real resourceless firmware, I'm
pulling it from the set.  I will submit it as part of a subsequent set
-- please ignore.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog