Re: [PATCH] extcon: palmas: Fail gracefully if invalid configuration

[Roger Quadros <rogerq@ti.com>]

Hi Chanwoo,

On 07/12/16 14:46, Chanwoo Choi wrote:
> Hi Roger,
> 
> Looks good to me.
> But I have some comment.
> 
> How about changing the subject as following?
> 
> - old : Fail gracefully if invalid configuration
> - new : Check the parent instance to prevent the NULL pointer error

I'm OK with this.

> 
> On 2016년 12월 07일 21:12, Roger Quadros wrote:
>> extcon-palmas must be child of palmas and expects parent's
>> drvdata to be valid. Check for non NULL parent drvdata and
>> fail if it is NULL. Not doing so will result in a NULL
>> pointer dereference later in the probe() parent drvdata
>> is NULL (e.g. misplaced extcon-palmas node in device tree).
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  drivers/extcon/extcon-palmas.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>> index 634ba70..ec987ab 100644
>> --- a/drivers/extcon/extcon-palmas.c
>> +++ b/drivers/extcon/extcon-palmas.c
>> @@ -190,6 +190,11 @@ static int palmas_usb_probe(struct platform_device *pdev)
>>  	struct palmas_usb *palmas_usb;
>>  	int status;
>>  
>> +	if (!palmas) {
>> +		dev_err(&pdev->dev, "device has invalid parent\n");
> 
> How about changing the error message as following?
> because the extcon-palmas used the 'failed to ..' style for error message.
> - "failed to get valid parent"

This is also fine.

I'll send a v2.

> 
>> +		return -EINVAL;
>> +	}
>> +
>>  	palmas_usb = devm_kzalloc(&pdev->dev, sizeof(*palmas_usb), GFP_KERNEL);
>>  	if (!palmas_usb)
>>  		return -ENOMEM;
>>
> 
> 

--
cheers,
-roger