lkml.org 
[lkml]   [2017]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v2 1/1] mux: mux-core: Add NULL check for dev->of_node
From
Date
Hi,


On 07/10/2017 12:40 AM, Peter Rosin wrote:
> On 2017-07-09 09:35, Kuppuswamy, Sathyanarayanan wrote:
>> Hi,
>>
>>
>> On 7/9/2017 12:07 AM, Peter Rosin wrote:
>>> On 2017-07-09 01:12, Kuppuswamy, Sathyanarayanan wrote:
>>>> Hi Peter,
>>>>
>>>>
>>>> On 7/8/2017 2:00 PM, Peter Rosin wrote:
>>>>> On 2017-07-07 23:46, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
>>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>>
>>>>>> If dev->of_node is NULL, then calling mux_control_get()
>>>>>> function can lead to NULL pointer exception. So adding
>>>>>> a NULL check for dev->of_node.
>>>>>>
>>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>> Do you have a driver that might call mux_control_get and not have any
>>>>> of_node?
>>>> For non-device tree drivers, this case is valid. I hit this issue when I
>>>> was working on Intel USB MUX driver.
>>>>> If not, I don't see the point of this check.
>>>> Since this is an API for other consumers, I think its better to have
>>>> some sanity checks.
>>>>
>>>> If a non device tree driver call this API , I think its better to fail
>>>> with some error no instead of creating null pointer exception.
>>> Is it? When authoring a new driver, and you make some error like this, why
>>> is a "nice" error better than a big fat fail? If you get a null deref,
>>> you will presumably also get a call stack etc, which will help you find
>>> where you made the error, w/o adding a bunch of traces to find out exactly
>>> what you did wrong.
>> In this case, I think the error can happen even if there is any
>> configuration mismatch in device tree blob. So its not only
> No, it cannot happen for any of the existing consumers. And seeing crashes
> if the DT blob is faulty isn't unexpected. I'm sure the ways to provoke a
> crash in that situation are abundant.
If we can prevent the crash and fail with some meaning full message, I think
we should adapt to it. if you want to leave a strong message, my
suggestion is to
use BUG or WARN_ON.
>
>> about API usage in driver. If you think that you need to fail the system
>> if the API is used without proper dt node configuration,
>> then we should use something like BUG or WARN_ON to explicitly mention
>> this dependency. But I think its better to
> But this is something that *cannot* happen unless you add some new code
> somewhere else. Why check at all?
Its not just kernel code here, DT tables are also involved.
>
>> leave this decision to the MUX consumers because there use cases where
>> MUX control can be optional
> This is only future-proofing for consumers that does not exist, and does
> nothing for the existing code. And the future where this is needed might
> never happen.
>
> Still skeptic...
I agree with philosophy of not bloating the kernel by merging un-used
code. But I think this concept should be limited to adding new APIs or
drivers. But for bugs in existing code I am not sure whether we should
follow the same principle.

I have checked some use cases of dev->of_node in kernel, and most of
them who uses it in
kernel APIs have some form of protection check for it. For example, take
a look at the
following functions.

int platform_get_irq(struct platform_device *dev, unsigned int num)
bool device_dma_supported(struct device *dev)
struct pwm_device *pwm_get(struct device *dev, const char *con_id)

These APIs have checks like,

if (IS_ENABLED(CONFIG_OF) && dev->of_node)

If you are still skeptic, I am out of arguments -:)
>
> Cheers,
> peda
>
>>> So, I'm skeptic...
>>>
>>> Cheers,
>>> peda
>>>
>>>>> Cheers,
>>>>> peda
>>>>>
>>>>>> ---
>>>>>> drivers/mux/mux-core.c | 3 +++
>>>>>> 1 file changed, 3 insertions(+)
>>>>>>
>>>>>> Changes since v1:
>>>>>> * Removed dummy new line.
>>>>>>
>>>>>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>>>>>> index 90b8995..924c983 100644
>>>>>> --- a/drivers/mux/mux-core.c
>>>>>> +++ b/drivers/mux/mux-core.c
>>>>>> @@ -438,6 +438,9 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>>>>> int index = 0;
>>>>>> int ret;
>>>>>>
>>>>>> + if (!np)
>>>>>> + return ERR_PTR(-ENODEV);
>>>>>> +
>>>>>> if (mux_name) {
>>>>>> index = of_property_match_string(np, "mux-control-names",
>>>>>> mux_name);
>>>>>>
>

--
Sathyanarayanan Kuppuswamy
Linux kernel developer

\
 
 \ /
  Last update: 2017-07-10 23:36    [W:0.681 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site