From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DDA73C63705 for ; Wed, 7 Dec 2022 13:49:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230111AbiLGNtd (ORCPT ); Wed, 7 Dec 2022 08:49:33 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45786 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230097AbiLGNt3 (ORCPT ); Wed, 7 Dec 2022 08:49:29 -0500 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D0A8959876; Wed, 7 Dec 2022 05:49:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1670420967; x=1701956967; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=244oBt7BYcfp9OKLgDcoevrJ5DGlb+K1/OKrzuTdtw8=; b=dmNw4xSB8lAyuaDHob2osPr+3zEPleJt8cKRVNCLO/rfDT+VHrY24Yh8 5uh5r2bmgUBPetPXkI8SOIOwsDXIE48GuFbZEDywxzJum9GN9+wQxlBnL Unoe65vx2G9ow7ozmOPmceQwOqcLkLb5IEvV5lnz5S8pumqu85w5QcQBJ zY+N5RpfqaGo/SUmppFqO+lmvZXv/ZtfTzEMFV/qwJxEtc3vnQEytPc5J JE3SWAnRLYdvRXcq0OBxiBxTsboEygJeKnK/HIgtGu4sP9Zuk1EP4NZpO Na5hyJzkMcHdwY0eCVAAqTk25AP/2xe9YX9gtor2FhrP8rMKU7qAEIrG2 w==; X-IronPort-AV: E=McAfee;i="6500,9779,10553"; a="381185384" X-IronPort-AV: E=Sophos;i="5.96,225,1665471600"; d="scan'208";a="381185384" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Dec 2022 05:49:27 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10553"; a="710069060" X-IronPort-AV: E=Sophos;i="5.96,225,1665471600"; d="scan'208";a="710069060" Received: from ahunter6-mobl1.ger.corp.intel.com (HELO [10.0.2.15]) ([10.252.38.130]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Dec 2022 05:49:21 -0800 Message-ID: Date: Wed, 7 Dec 2022 15:49:16 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0 Thunderbird/102.5.1 Subject: Re: [PATCH v2 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver Content-Language: en-US To: Andy Shevchenko , Tomer Maimon Cc: ulf.hansson@linaro.org, avifishman70@gmail.com, tali.perry1@gmail.com, joel@jms.id.au, venture@google.com, yuenn@google.com, benjaminfair@google.com, skhan@linuxfoundation.org, davidgow@google.com, pbrobinson@gmail.com, gsomlo@gmail.com, briannorris@chromium.org, arnd@arndb.de, krakoczy@antmicro.com, openbmc@lists.ozlabs.org, linux-mmc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org References: <20221205085351.27566-1-tmaimon77@gmail.com> <20221205085351.27566-3-tmaimon77@gmail.com> From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 7/12/22 15:25, Andy Shevchenko wrote: > On Wed, Dec 7, 2022 at 3:01 PM Tomer Maimon wrote: >> On Mon, 5 Dec 2022 at 16:33, Adrian Hunter wrote: >>> On 5/12/22 16:17, Andy Shevchenko wrote: >>>> On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko >>>> wrote: >>>>> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter wrote: >>>>>> On 5/12/22 15:25, Andy Shevchenko wrote: >>>>>>> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon wrote: > > ... > >>>>>>> devm_ is problematic in your case. >>>>>>> TL;DR: you need to use clk_get_optional() and clk_put(). >>>>>> >>>>>> devm_ calls exactly those, so what is the issue? >>>>> >>>>> The issue is the error path or removal stage where it may or may be >>>>> not problematic. To be on the safe side, the best approach is to make >>>>> sure that allocated resources are being deallocated in the reversed >>>>> order. That said, the >>>>> >>>>> 1. call non-devm_func() >>>>> 2. call devm_func() >>>>> >>>>> is wrong strictly speaking. >>>> >>>> To elaborate more, the >>>> >>>> 1. call all devm_func() >>>> 2. call only non-devm_func() >>>> >>>> is the correct order. >>> >>> 1. WRT pltfm_host->clk, that is what is happening >>> 2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_ >>> e.g. mmc_alloc_host() / mmc_free_host() >> I little confused about what to decide, should I use only >> non-devm_func because mmc_alloc_host() / mmc_free_host() is not >> warrped with devm_? > > It is up to you how to proceed. I pointed out the problem with your > code which may or may not be fatal. > > If you want to solve it, there are several approaches: > 1) get rid of devm_ completely; > 2) properly shuffle the ordering in ->probe(), so all devm_ calls are > followed by non-devm_; > 3) wrap non-devm_ cals to become managed (see > devm_add_action_or_reset() approach); > 4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix > sdhci_pltfm_register() to handle the clock. I can take care of sdhci_pltfm when I next have some time. Otherwise it looks OK to me, so I am acking it. > > Personally, the list order is from the least, what I prefer, to the > most (i.o.w. I would like to see rather 4) than 1) to be implemented). > >>>> Hence in this case the driver can be worked around easily (by >>>> shuffling the order in ->probe() to call devm_ first), but as I said >>>> looking into implementation of the _unregister() I'm pretty sure that >>>> clock management should be in sdhci-pltfm, rather than in all callers >>>> who won't need the full customization. >>>> >>>> Hope this helps to understand my point. >>>> >>>>>>> Your ->remove() callback doesn't free resources in the reversed order >>>>>>> which may or, by luck, may not be the case of all possible crashes, >>>>>>> UAFs, races, etc during removal stage. All the same for error path in >>>>>>> ->probe(). >>>>> >>>>> I also pointed out above what would be the outcome of neglecting this rule. > > ... > >>>>>>>>> Why can't you use sdhci_pltfm_register()? >>>>>>>> two things are missing in sdhci_pltfm_register >>>>>>>> 1. clock. >>>>>>> >>>>>>> Taking into account the implementation of the corresponding >>>>>>> _unregister() I would add the clock handling to the _register() one. >>>>>>> Perhaps via a new member of the platform data that supplies the name >>>>>>> and index of the clock and hence all clk_get_optional() / clk_put will >>>>>>> be moved there. >> Do you mean to add it to sdhci_pltfm_register function? if yes I >> believe it will take some time to modify sdhci_pltfm_register >> I prefer not to use sdhci_pltfm_register. > > In the Linux kernel we are trying hard to avoid code duplication. Why > do you need it to be open coded? (Yes, I heard you, but somebody > should fix the issues with that funcion at some point, right?) > >>>>>>>> 2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities. >>>>>>> >>>>>>> All the same, why can't platform data be utilised for this? > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C80C4C352A1 for ; Wed, 7 Dec 2022 13:50:25 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4NRzFD2jcBz3bgv for ; Thu, 8 Dec 2022 00:50:24 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=Kyzqmt+N; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=intel.com (client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=adrian.hunter@intel.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=Kyzqmt+N; dkim-atps=neutral Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4NRzDB0qXKz2xHb for ; Thu, 8 Dec 2022 00:49:29 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1670420970; x=1701956970; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=244oBt7BYcfp9OKLgDcoevrJ5DGlb+K1/OKrzuTdtw8=; b=Kyzqmt+Nd+FSBIIWMPgmVy6natcPrdXW/RncHpc5R0y82/Ye7qsBRN3P oz3ObEhmob8CRbnvcQjWWUzCqd8Vbkjjzd1zjyxhwP71HLxEhloZFEeep MUNx+QnaxGI5fN/NnnteDJELNWWeO23ilABRVuN0NjU6IdJjoWBN2TVEV i64GLOpneHIrw3SYPMywBTjnsQSGLnG+JEI423WWOSF/DfTu5OrvlC5LI V3cfX0dlnh3QoK/SfPfFduUDNIcdZWzxJSBm/RT7J7jzihRn13m3o9zFn bp4a5QBK6soLbhKPu1MWPCZ4YuaaySoNHf2ZVr4/obTbQBFBHeAJR52qi Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10553"; a="381185387" X-IronPort-AV: E=Sophos;i="5.96,225,1665471600"; d="scan'208";a="381185387" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Dec 2022 05:49:27 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10553"; a="710069060" X-IronPort-AV: E=Sophos;i="5.96,225,1665471600"; d="scan'208";a="710069060" Received: from ahunter6-mobl1.ger.corp.intel.com (HELO [10.0.2.15]) ([10.252.38.130]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Dec 2022 05:49:21 -0800 Message-ID: Date: Wed, 7 Dec 2022 15:49:16 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0 Thunderbird/102.5.1 Subject: Re: [PATCH v2 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver Content-Language: en-US To: Andy Shevchenko , Tomer Maimon References: <20221205085351.27566-1-tmaimon77@gmail.com> <20221205085351.27566-3-tmaimon77@gmail.com> From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, ulf.hansson@linaro.org, benjaminfair@google.com, arnd@arndb.de, krakoczy@antmicro.com, avifishman70@gmail.com, venture@google.com, openbmc@lists.ozlabs.org, briannorris@chromium.org, linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org, tali.perry1@gmail.com, gsomlo@gmail.com, joel@jms.id.au, davidgow@google.com, skhan@linuxfoundation.org, pbrobinson@gmail.com Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" On 7/12/22 15:25, Andy Shevchenko wrote: > On Wed, Dec 7, 2022 at 3:01 PM Tomer Maimon wrote: >> On Mon, 5 Dec 2022 at 16:33, Adrian Hunter wrote: >>> On 5/12/22 16:17, Andy Shevchenko wrote: >>>> On Mon, Dec 5, 2022 at 4:14 PM Andy Shevchenko >>>> wrote: >>>>> On Mon, Dec 5, 2022 at 3:41 PM Adrian Hunter wrote: >>>>>> On 5/12/22 15:25, Andy Shevchenko wrote: >>>>>>> On Mon, Dec 5, 2022 at 1:20 PM Tomer Maimon wrote: > > ... > >>>>>>> devm_ is problematic in your case. >>>>>>> TL;DR: you need to use clk_get_optional() and clk_put(). >>>>>> >>>>>> devm_ calls exactly those, so what is the issue? >>>>> >>>>> The issue is the error path or removal stage where it may or may be >>>>> not problematic. To be on the safe side, the best approach is to make >>>>> sure that allocated resources are being deallocated in the reversed >>>>> order. That said, the >>>>> >>>>> 1. call non-devm_func() >>>>> 2. call devm_func() >>>>> >>>>> is wrong strictly speaking. >>>> >>>> To elaborate more, the >>>> >>>> 1. call all devm_func() >>>> 2. call only non-devm_func() >>>> >>>> is the correct order. >>> >>> 1. WRT pltfm_host->clk, that is what is happening >>> 2. WRT other resources that is simply not always possible because not every resource is wrapped by devm_ >>> e.g. mmc_alloc_host() / mmc_free_host() >> I little confused about what to decide, should I use only >> non-devm_func because mmc_alloc_host() / mmc_free_host() is not >> warrped with devm_? > > It is up to you how to proceed. I pointed out the problem with your > code which may or may not be fatal. > > If you want to solve it, there are several approaches: > 1) get rid of devm_ completely; > 2) properly shuffle the ordering in ->probe(), so all devm_ calls are > followed by non-devm_; > 3) wrap non-devm_ cals to become managed (see > devm_add_action_or_reset() approach); > 4) fix SDHCI / MMC layer by providing necessary devm_ calls and/or fix > sdhci_pltfm_register() to handle the clock. I can take care of sdhci_pltfm when I next have some time. Otherwise it looks OK to me, so I am acking it. > > Personally, the list order is from the least, what I prefer, to the > most (i.o.w. I would like to see rather 4) than 1) to be implemented). > >>>> Hence in this case the driver can be worked around easily (by >>>> shuffling the order in ->probe() to call devm_ first), but as I said >>>> looking into implementation of the _unregister() I'm pretty sure that >>>> clock management should be in sdhci-pltfm, rather than in all callers >>>> who won't need the full customization. >>>> >>>> Hope this helps to understand my point. >>>> >>>>>>> Your ->remove() callback doesn't free resources in the reversed order >>>>>>> which may or, by luck, may not be the case of all possible crashes, >>>>>>> UAFs, races, etc during removal stage. All the same for error path in >>>>>>> ->probe(). >>>>> >>>>> I also pointed out above what would be the outcome of neglecting this rule. > > ... > >>>>>>>>> Why can't you use sdhci_pltfm_register()? >>>>>>>> two things are missing in sdhci_pltfm_register >>>>>>>> 1. clock. >>>>>>> >>>>>>> Taking into account the implementation of the corresponding >>>>>>> _unregister() I would add the clock handling to the _register() one. >>>>>>> Perhaps via a new member of the platform data that supplies the name >>>>>>> and index of the clock and hence all clk_get_optional() / clk_put will >>>>>>> be moved there. >> Do you mean to add it to sdhci_pltfm_register function? if yes I >> believe it will take some time to modify sdhci_pltfm_register >> I prefer not to use sdhci_pltfm_register. > > In the Linux kernel we are trying hard to avoid code duplication. Why > do you need it to be open coded? (Yes, I heard you, but somebody > should fix the issues with that funcion at some point, right?) > >>>>>>>> 2. Adding SDHCI_CAN_DO_8BIT capability according the eMMC capabilities. >>>>>>> >>>>>>> All the same, why can't platform data be utilised for this? >