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

[loic pallardy <loic.pallardy@st.com>]

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