diff mbox

[v2,12/19] remoteproc: core: Add vdev support and force mode to resource amending function

Message ID 1472676622-32533-13-git-send-email-loic.pallardy@st.com
State New
Headers show

Commit Message

Loic Pallardy Aug. 31, 2016, 8:50 p.m. UTC 
This patch proposes diverse updates to rproc_update_resource_table_entry
function:
- rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to
  have shorter function name.
- add RSC_VDEV support
- add force mode resource even if resource already fixed on firmware side.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

---
 drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

-- 
1.9.1

Comments

Lee Jones Sept. 8, 2016, 8:48 a.m. UTC | #1 
On Wed, 31 Aug 2016, Loic Pallardy wrote:

> This patch proposes diverse updates to rproc_update_resource_table_entry

> function:

> - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to

>   have shorter function name.

> - add RSC_VDEV support

> - add force mode resource even if resource already fixed on firmware side.

> 

> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

> ---

>  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----

>  1 file changed, 22 insertions(+), 5 deletions(-)

> 

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c

> index 30e9c70..aff1a00 100644

> --- a/drivers/remoteproc/remoteproc_core.c

> +++ b/drivers/remoteproc/remoteproc_core.c

> @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,

>  	return -EINVAL;

>  }

>  

> -static int rproc_update_resource_table_entry(struct rproc *rproc,

> +static int __update_rsc_tbl_entry(struct rproc *rproc,


Unless the name is unruly, (which I don't think it is, you're still
having to line wrap at the call site), I tend to go for clarity over
brevity.

>  				struct rproc_request_resource *request,

> -				struct resource_table *table, int size)

> +				struct resource_table *table, int size,

> +				bool force)

>  {

>  	struct fw_rsc_carveout *tblc, *newc;

>  	struct fw_rsc_devmem *tbld, *newd;

>  	struct fw_rsc_trace *tblt, *newt;

> +	struct fw_rsc_vdev *tblv, *newv;

>  	int updated = true;

>  	int i;

>  

> @@ -1054,7 +1056,8 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,

>  				    sizeof(*tblc->name)))

>  				break;

>  

> -			memcpy(tblc, newc, request->size);

> +			if (tblc->pa == FW_RSC_ADDR_ANY || force)

> +				memcpy(tblc, newc, request->size);

>  

>  			return updated;

>  		case RSC_DEVMEM:

> @@ -1079,6 +1082,20 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,

>  			memcpy(tblt, newt, request->size);

>  

>  			return updated;

> +		case RSC_VDEV:

> +			tblv = rsc;

> +			newv = request->resource;

> +			if (newv->id != tblv->id)

> +				break;

> +

> +			if (request->size > (sizeof(*tblv) +

> +				    tblv->num_of_vrings * sizeof(struct fw_rsc_vdev_vring) +

> +				    tblv->config_len))

> +				return -ENOSPC;

> +

> +			memcpy(tblv, newv, request->size);

> +

> +			return updated;


Again, there is more than one functional change in this patch.  You're
(unnecessarily IMO) renaming things, adding a force argument and
supplying support for a new type of device, all in one patch.

If any one of those functional changes has to be reverted, the
Maintainer will have no choice but to either revert the whole thing,
or someone will have to physically write an anti-patch, which is more
time consuming.

>  		default:

>  			dev_err(&rproc->dev,

>  				"Unsupported resource type: %d\n",

> @@ -1176,8 +1193,8 @@ rproc_apply_resource_overrides(struct rproc *rproc,

>  		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);

> +		updated = __update_rsc_tbl_entry(rproc, resource, table, size,

> +						 false);

>  		if (updated < 0) {

>  			table = ERR_PTR(updated);

>  			goto out;


-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Loic Pallardy Sept. 8, 2016, 9:49 a.m. UTC | #2 
On 09/08/2016 10:48 AM, Lee Jones wrote:
> On Wed, 31 Aug 2016, Loic Pallardy wrote:

>

>> This patch proposes diverse updates to rproc_update_resource_table_entry

>> function:

>> - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to

>>   have shorter function name.

>> - add RSC_VDEV support

>> - add force mode resource even if resource already fixed on firmware side.

>>

>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

>> ---

>>  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----

>>  1 file changed, 22 insertions(+), 5 deletions(-)

>>

>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c

>> index 30e9c70..aff1a00 100644

>> --- a/drivers/remoteproc/remoteproc_core.c

>> +++ b/drivers/remoteproc/remoteproc_core.c

>> @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,

>>  	return -EINVAL;

>>  }

>>

>> -static int rproc_update_resource_table_entry(struct rproc *rproc,

>> +static int __update_rsc_tbl_entry(struct rproc *rproc,

>

> Unless the name is unruly, (which I don't think it is, you're still

> having to line wrap at the call site), I tend to go for clarity over

> brevity.

It was only to have reasonable line length. I can keept original name 
and see impact on rest of the code.

>

>>  				struct rproc_request_resource *request,

>> -				struct resource_table *table, int size)

>> +				struct resource_table *table, int size,

>> +				bool force)

>>  {

>>  	struct fw_rsc_carveout *tblc, *newc;

>>  	struct fw_rsc_devmem *tbld, *newd;

>>  	struct fw_rsc_trace *tblt, *newt;

>> +	struct fw_rsc_vdev *tblv, *newv;

>>  	int updated = true;

>>  	int i;

>>

>> @@ -1054,7 +1056,8 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,

>>  				    sizeof(*tblc->name)))

>>  				break;

>>

>> -			memcpy(tblc, newc, request->size);

>> +			if (tblc->pa == FW_RSC_ADDR_ANY || force)

>> +				memcpy(tblc, newc, request->size);

>>

>>  			return updated;

>>  		case RSC_DEVMEM:

>> @@ -1079,6 +1082,20 @@ static int rproc_update_resource_table_entry(struct rproc *rproc,

>>  			memcpy(tblt, newt, request->size);

>>

>>  			return updated;

>> +		case RSC_VDEV:

>> +			tblv = rsc;

>> +			newv = request->resource;

>> +			if (newv->id != tblv->id)

>> +				break;

>> +

>> +			if (request->size > (sizeof(*tblv) +

>> +				    tblv->num_of_vrings * sizeof(struct fw_rsc_vdev_vring) +

>> +				    tblv->config_len))

>> +				return -ENOSPC;

>> +

>> +			memcpy(tblv, newv, request->size);

>> +

>> +			return updated;

>

> Again, there is more than one functional change in this patch.  You're

> (unnecessarily IMO) renaming things, adding a force argument and

> supplying support for a new type of device, all in one patch.

>

> If any one of those functional changes has to be reverted, the

> Maintainer will have no choice but to either revert the whole thing,

> or someone will have to physically write an anti-patch, which is more

> time consuming.


Ok I'll split feature by feature

Thanks,
Loic
>

>>  		default:

>>  			dev_err(&rproc->dev,

>>  				"Unsupported resource type: %d\n",

>> @@ -1176,8 +1193,8 @@ rproc_apply_resource_overrides(struct rproc *rproc,

>>  		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);

>> +		updated = __update_rsc_tbl_entry(rproc, resource, table, size,

>> +						 false);

>>  		if (updated < 0) {

>>  			table = ERR_PTR(updated);

>>  			goto out;

>
Lee Jones Sept. 8, 2016, 11:02 a.m. UTC | #3 
On Thu, 08 Sep 2016, loic pallardy wrote:

> 

> 

> On 09/08/2016 10:48 AM, Lee Jones wrote:

> > On Wed, 31 Aug 2016, Loic Pallardy wrote:

> > 

> > > This patch proposes diverse updates to rproc_update_resource_table_entry

> > > function:

> > > - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to

> > >   have shorter function name.

> > > - add RSC_VDEV support

> > > - add force mode resource even if resource already fixed on firmware side.

> > > 

> > > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

> > > ---

> > >  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----

> > >  1 file changed, 22 insertions(+), 5 deletions(-)

> > > 

> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c

> > > index 30e9c70..aff1a00 100644

> > > --- a/drivers/remoteproc/remoteproc_core.c

> > > +++ b/drivers/remoteproc/remoteproc_core.c

> > > @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,

> > >  	return -EINVAL;

> > >  }

> > > 

> > > -static int rproc_update_resource_table_entry(struct rproc *rproc,

> > > +static int __update_rsc_tbl_entry(struct rproc *rproc,

> > 

> > Unless the name is unruly, (which I don't think it is, you're still

> > having to line wrap at the call site), I tend to go for clarity over

> > brevity.

> It was only to have reasonable line length. I can keept original name and

> see impact on rest of the code.


Reasonable line length is 80.  This line was 65.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Loic Pallardy Sept. 8, 2016, 1:11 p.m. UTC | #4 
On 09/08/2016 01:02 PM, Lee Jones wrote:
> On Thu, 08 Sep 2016, loic pallardy wrote:

>

>>

>>

>> On 09/08/2016 10:48 AM, Lee Jones wrote:

>>> On Wed, 31 Aug 2016, Loic Pallardy wrote:

>>>

>>>> This patch proposes diverse updates to rproc_update_resource_table_entry

>>>> function:

>>>> - rename rproc_update_resource_table_entry to __update_rsc_tbl_entry to

>>>>   have shorter function name.

>>>> - add RSC_VDEV support

>>>> - add force mode resource even if resource already fixed on firmware side.

>>>>

>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

>>>> ---

>>>>  drivers/remoteproc/remoteproc_core.c | 27 ++++++++++++++++++++++-----

>>>>  1 file changed, 22 insertions(+), 5 deletions(-)

>>>>

>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c

>>>> index 30e9c70..aff1a00 100644

>>>> --- a/drivers/remoteproc/remoteproc_core.c

>>>> +++ b/drivers/remoteproc/remoteproc_core.c

>>>> @@ -1027,13 +1027,15 @@ static int __verify_rsc_tbl_entry(struct rproc *rproc,

>>>>  	return -EINVAL;

>>>>  }

>>>>

>>>> -static int rproc_update_resource_table_entry(struct rproc *rproc,

>>>> +static int __update_rsc_tbl_entry(struct rproc *rproc,

>>>

>>> Unless the name is unruly, (which I don't think it is, you're still

>>> having to line wrap at the call site), I tend to go for clarity over

>>> brevity.

>> It was only to have reasonable line length. I can keept original name and

>> see impact on rest of the code.

>

> Reasonable line length is 80.  This line was 65.

This line yes. Name change proposal is to reduce line where this 
function is called (in rproc_apply_resource_overrides for exemple)
But OK to keep standard rproc_xxx naming
>
diff mbox

Patch

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 30e9c70..aff1a00 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1027,13 +1027,15 @@  static int __verify_rsc_tbl_entry(struct rproc *rproc,
 	return -EINVAL;
 }
 
-static int rproc_update_resource_table_entry(struct rproc *rproc,
+static int __update_rsc_tbl_entry(struct rproc *rproc,
 				struct rproc_request_resource *request,
-				struct resource_table *table, int size)
+				struct resource_table *table, int size,
+				bool force)
 {
 	struct fw_rsc_carveout *tblc, *newc;
 	struct fw_rsc_devmem *tbld, *newd;
 	struct fw_rsc_trace *tblt, *newt;
+	struct fw_rsc_vdev *tblv, *newv;
 	int updated = true;
 	int i;
 
@@ -1054,7 +1056,8 @@  static int rproc_update_resource_table_entry(struct rproc *rproc,
 				    sizeof(*tblc->name)))
 				break;
 
-			memcpy(tblc, newc, request->size);
+			if (tblc->pa == FW_RSC_ADDR_ANY || force)
+				memcpy(tblc, newc, request->size);
 
 			return updated;
 		case RSC_DEVMEM:
@@ -1079,6 +1082,20 @@  static int rproc_update_resource_table_entry(struct rproc *rproc,
 			memcpy(tblt, newt, request->size);
 
 			return updated;
+		case RSC_VDEV:
+			tblv = rsc;
+			newv = request->resource;
+			if (newv->id != tblv->id)
+				break;
+
+			if (request->size > (sizeof(*tblv) +
+				    tblv->num_of_vrings * sizeof(struct fw_rsc_vdev_vring) +
+				    tblv->config_len))
+				return -ENOSPC;
+
+			memcpy(tblv, newv, request->size);
+
+			return updated;
 		default:
 			dev_err(&rproc->dev,
 				"Unsupported resource type: %d\n",
@@ -1176,8 +1193,8 @@  rproc_apply_resource_overrides(struct rproc *rproc,
 		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);
+		updated = __update_rsc_tbl_entry(rproc, resource, table, size,
+						 false);
 		if (updated < 0) {
 			table = ERR_PTR(updated);
 			goto out;