Re: [PATCH 2/4] remoteproc: Split firmware name allocation from rproc_alloc()

[Alex Elder]

On 4/13/20 7:55 PM, Bjorn Andersson wrote:
> On Mon 13 Apr 13:56 PDT 2020, Alex Elder wrote:
> 
>> On 4/13/20 2:33 PM, Mathieu Poirier wrote:
>>> Make the firmware name allocation a function on its own in order to
>>> introduce more flexibility to function rproc_alloc().
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

. . .

>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 66 ++++++++++++++++------------
>>>  1 file changed, 39 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>> index 80056513ae71..4dee63f319ba 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -1979,6 +1979,33 @@ static const struct device_type rproc_type = {
>>>  	.release	= rproc_type_release,
>>>  };
>>>  
>>> +static int rproc_alloc_firmware(struct rproc *rproc,
>>> +				const char *name, const char *firmware)
>>> +{
>>> +	char *p, *template = "rproc-%s-fw";
>>> +	int name_len;
>>
>> Not a big deal (and maybe it's not consistent with other nearby
>> style) but template and name_len could be defined inside the
>> "if (!firmware)" block.
>>
> 
> I prefer variables declared in the beginning of the function, so I'm
> happy with this.

It should be obvious that this is fine with me.

>>> +	if (!firmware) {
>>> +		/*
>>> +		 * If the caller didn't pass in a firmware name then
>>> +		 * construct a default name.
>>> +		 */
>>> +		name_len = strlen(name) + strlen(template) - 2 + 1;
>>> +		p = kmalloc(name_len, GFP_KERNEL);
>>
>>
>> I don't know if it would be an improvement, but you could
>> check for a null p value below for both cases.  I.e.:
>>
>> 		if (p)
>> 			snprintf(p, ...);
>>
> 
> Moving the common NULL check and return out seems nice, but given that
> we then have to have this positive conditional I think the end result is
> more complex.
> 
> That said, if we're not just doing a verbatim copy from rproc_alloc() I
> think we should make this function:
> 
> 	if (!firmware)
> 		p = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);
> 	else
> 		p = kstrdup_const(firmware, GFP_KERNEL);

You know, I wanted to suggest this but I didn't know the
name of the function (kasprintf()) and didn't take the time
to find it.  I wholly agree with your suggestion.

The only additional minor tweak I'd add is that I prefer
using a non-negated condition where possible, though it
doesn't always "look right."  So:

	if (firmware)
		rproc->firmware = kstrdup_const(firmware, GFP_KERNEL);
	else
		rproc->firmware = kasprintf(GFP_KERNEL, "rproc-%s-fw", name);

					-Alex

> 	rproc->firmware = p;
> 
> 	return p ? 0 : -ENOMEM;
> 
> Regards,
> Bjorn
> 
>> (more below)
>>
>>> +		if (!p)
>>> +			return -ENOMEM;
>>> +		snprintf(p, name_len, template, name);
>>> +	} else {
>>> +		p = kstrdup(firmware, GFP_KERNEL);
>>> +		if (!p)
>>> +			return -ENOMEM;
>>> +	}
>>> +
>>
>> 	if (!p)
>> 		return -ENOMEM;
>> 	
>>> +	rproc->firmware = p;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  /**
>>>   * rproc_alloc() - allocate a remote processor handle
>>>   * @dev: the underlying device
>>> @@ -2007,42 +2034,21 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>>>  			  const char *firmware, int len)
>>>  {
>>>  	struct rproc *rproc;
>>> -	char *p, *template = "rproc-%s-fw";
>>> -	int name_len;
>>>  
>>>  	if (!dev || !name || !ops)
>>>  		return NULL;
>>>  
>>> -	if (!firmware) {
>>> -		/*
>>> -		 * If the caller didn't pass in a firmware name then
>>> -		 * construct a default name.
>>> -		 */
>>> -		name_len = strlen(name) + strlen(template) - 2 + 1;
>>> -		p = kmalloc(name_len, GFP_KERNEL);
>>> -		if (!p)
>>> -			return NULL;
>>> -		snprintf(p, name_len, template, name);
>>> -	} else {
>>> -		p = kstrdup(firmware, GFP_KERNEL);
>>> -		if (!p)
>>> -			return NULL;
>>> -	}
>>> -
>>>  	rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
>>> -	if (!rproc) {
>>> -		kfree(p);
>>> +	if (!rproc)
>>>  		return NULL;
>>> -	}
>>> +
>>> +	if (rproc_alloc_firmware(rproc, name, firmware))
>>> +		goto free_rproc;
>>>  
>>>  	rproc->ops = kmemdup(ops, sizeof(*ops), GFP_KERNEL);
>>> -	if (!rproc->ops) {
>>> -		kfree(p);
>>> -		kfree(rproc);
>>> -		return NULL;
>>> -	}
>>> +	if (!rproc->ops)
>>> +		goto free_firmware;
>>>  
>>> -	rproc->firmware = p;
>>>  	rproc->name = name;
>>>  	rproc->priv = &rproc[1];
>>>  	rproc->auto_boot = true;
>>> @@ -2091,6 +2097,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>>>  	rproc->state = RPROC_OFFLINE;
>>>  
>>>  	return rproc;
>>> +
>>> +free_firmware:
>>> +	kfree(rproc->firmware);
>>> +free_rproc:
>>> +	kfree(rproc);
>>> +	return NULL;
>>>  }
>>>  EXPORT_SYMBOL(rproc_alloc);
>>>  
>>>
>>