Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754909AbdGJVgb (ORCPT ); Mon, 10 Jul 2017 17:36:31 -0400 Received: from mga04.intel.com ([192.55.52.120]:27498 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754550AbdGJVga (ORCPT ); Mon, 10 Jul 2017 17:36:30 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,342,1496127600"; d="scan'208";a="285220699" Reply-To: sathyanarayanan.kuppuswamy@linux.intel.com Subject: Re: [PATCH v2 1/1] mux: mux-core: Add NULL check for dev->of_node To: Peter Rosin , "Kuppuswamy, Sathyanarayanan" Cc: linux-kernel@vger.kernel.org References: <4a9a39dd-86b0-9485-46f1-76f4f8f5a809@gmail.com> <591290e2-5c63-52c2-b5a3-5417bc16dc27@axentia.se> <27cdacb6-b775-f678-9ff0-6a3f2ad11e17@gmail.com> From: sathyanarayanan kuppuswamy Organization: Intel Message-ID: <75e9c7b1-f9b7-e0e0-7917-1fab3e2ac6bb@linux.intel.com> Date: Mon, 10 Jul 2017 14:36:13 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: 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: 4424 Lines: 121 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 >>>>>> >>>>>> 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 > 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