Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751732AbdGIHft (ORCPT ); Sun, 9 Jul 2017 03:35:49 -0400 Received: from mail-pg0-f65.google.com ([74.125.83.65]:34215 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750836AbdGIHfs (ORCPT ); Sun, 9 Jul 2017 03:35:48 -0400 Subject: Re: [PATCH v2 1/1] mux: mux-core: Add NULL check for dev->of_node To: Peter Rosin , sathyanarayanan.kuppuswamy@linux.intel.com Cc: linux-kernel@vger.kernel.org References: <4a9a39dd-86b0-9485-46f1-76f4f8f5a809@gmail.com> <591290e2-5c63-52c2-b5a3-5417bc16dc27@axentia.se> From: "Kuppuswamy, Sathyanarayanan" Message-ID: <27cdacb6-b775-f678-9ff0-6a3f2ad11e17@gmail.com> Date: Sun, 9 Jul 2017 00:35:45 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <591290e2-5c63-52c2-b5a3-5417bc16dc27@axentia.se> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2601 Lines: 71 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 >>>> >>>> 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 >>> 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 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 leave this decision to the MUX consumers because there use cases where MUX control can be optional > > 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); >>>>