Re: [PATCH] i2c: mux: demux-pinctrl: handle failure case of devm_kstrdup()

[Peter Rosin]

On 2018-12-04 13:13, Nicholas Mc Guire wrote:
> On Tue, Dec 04, 2018 at 11:16:59AM +0000, Peter Rosin wrote:
>> Hi!
>>
>> This patch looks like a good idea. However, a nitpick below.
>>
>> On 2018-12-01 11:01, Nicholas Mc Guire wrote:
>>> devm_kstrdup() may return NULL if internal allocation failed.
>>> Thus using  name, value  is unsafe without being checked. As
>>> i2c_demux_pinctrl_probe() can return -ENOMEM in other cases
>>> a dev_err() message is included to make the failure location
>>> clear.
>>>
>>> Signed-off-by: Nicholas Mc Guire <hofrat@xxxxxxxxx>
>>> Fixes: e35478eac030 ("i2c: mux: demux-pinctrl: run properly with multiple instances")
>>> ---
>>>
>>> Problem located with experimental coccinelle script
>>>
>>> Q: The use of devm_kstrdup() seems a bit odd while technically not wrong,
>>>    personally I think devm_kasprintf() would be more suitable here.
>>>
>>> Patch was compile tested with: multi_v7_defconfig 
>>> (implies I2C_DEMUX_PINCTRL=y)
>>>
>>> Patch is against 4.20-rc4 (localversion-next is next-20181130)
>>>
>>>  drivers/i2c/muxes/i2c-demux-pinctrl.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/i2c/muxes/i2c-demux-pinctrl.c b/drivers/i2c/muxes/i2c-demux-pinctrl.c
>>> index 035032e..c466999 100644
>>> --- a/drivers/i2c/muxes/i2c-demux-pinctrl.c
>>> +++ b/drivers/i2c/muxes/i2c-demux-pinctrl.c
>>> @@ -244,6 +244,12 @@ static int i2c_demux_pinctrl_probe(struct platform_device *pdev)
>>>  
>>>  		props[i].name = devm_kstrdup(&pdev->dev, "status", GFP_KERNEL);
>>>  		props[i].value = devm_kstrdup(&pdev->dev, "ok", GFP_KERNEL);
>>> +		if (!props[i].name || !props[i].value) {
>>> +			dev_err(&pdev->dev,
>>> +				"chan %d name, value allocation failed\n", i);
>>
>> Please drop this memory allocation failure message. You should get such a
>> message from devm_kstrdup.
>>
> 
> hm...tried to figure out where that message would be comming
> from - but I could not find any point in the call tree that
> would issue such a message ?
> 
>  devm_kstrdup() 
>    -> devm_kmalloc()
>         -> alloc_dr()
>              --> kmalloc_track_caller() (non-NUMA)
>              |     -> __kmalloc_node()
>              |        -> __do_kmalloc_node()
>              `-> __kmalloc_node_track_caller() (NUMA)
>                    -> __do_kmalloc_node()
> 
>  __do_kmalloc_node() seems like it simply returns NULL but
>  issues no failure message.
>  Am I overlooking something ? 

Well, I don't know the details, but checkpatch will warn about simple
error messages on devm_kstrdup failure (if I read the checkpatch source
correctly). But in this case there are two parallel conditions in the
if and hence checkpatch stumbles, but gist is the same, you should not
sprinkle messages on memory allocation failure.

Cheers,
Peter

> thx!
> hofrat 
>  
>> Cheers,
>> Peter
>>
>>> +			err = -ENOMEM;
>>> +			goto err_rollback;
>>> +		}
>>>  		props[i].length = 3;
>>>  
>>>  		of_changeset_init(&priv->chan[i].chgset);
>>>
>>