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 CC6F5C63705 for ; Wed, 7 Dec 2022 16:49:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229969AbiLGQtV (ORCPT ); Wed, 7 Dec 2022 11:49:21 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38250 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229777AbiLGQtT (ORCPT ); Wed, 7 Dec 2022 11:49:19 -0500 Received: from mail-qk1-x735.google.com (mail-qk1-x735.google.com [IPv6:2607:f8b0:4864:20::735]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4494858BD0; Wed, 7 Dec 2022 08:49:18 -0800 (PST) Received: by mail-qk1-x735.google.com with SMTP id c2so10416540qko.1; Wed, 07 Dec 2022 08:49:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=y18DIpbfCukk25TWC8zheO2SUvaGDQaONDYCY89KTGU=; b=SMK2V74sU8D3TcIrRaSUNwZYnaaTCeoM4k+ZcZTFw/ydUkhwZLbo1yEKDxUMIBS4gA iwNJGXqzS6w1suvYGlU8ecRhl6NNxEKbu7JAvBOIviytxXOltcSBfBeWp+TnxyBxXr9j aMWtjDuBUNQOGk7gO4jGnxDWTQ8/yXDvz53Y4z4R9i7/KWUTuiCGkDsrHLRa/o9SuTb7 KqorQSVIORS6lv2KWgYWjY+eZry7XlEBoWrdcJ1POsluOkv3Gc8qESPlNGyPzJ94Jsq1 5AvRk0IRxLOSHpjNN6gxt9LAglz7AWcGJObOYuX4CKqkCJ0dV+a7TLBA1pKfycAlJzKC YqrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=y18DIpbfCukk25TWC8zheO2SUvaGDQaONDYCY89KTGU=; b=mtKrzpYlOQ6xEDqaXzPNF4YAcmytXfDrb+h/1hUvV1GVXU2/uSmFYdqBl4H0gIYzq9 wgmcWiAwFCYtG+yjdPyYXu3J83VPiev2NylKlotcMZKdVzaSOsqYt7/X18vgcm4zO+vY tn/6zrPdjBoJFNNqq4kh6r+ZQgd2mrIVukm3O9cltaN7sJL7vR/LNdj6o5k0T7Z5mmXM JD2Wgivev6TRLaldbkEBbhVkzb9PnywtADQj9XR4i3qgkb+EFITdXXPjK0K3rc/OFFcM PHTgcvkAfqp2K1W8nPlAhmC+19Ppoj0eD0JLxHbc1Ix/OVhLyvhcRSRAhPvPh8/OwBvr KkWQ== X-Gm-Message-State: ANoB5pl5eOAT6pfydNLuQ8J29ogf6Gjn+LdqxbVVdT7zH9QjqMrm75Pp 5wL6rcPRLapYiFhG3lcM55BEgyQee0RodUqFgmE= X-Google-Smtp-Source: AA0mqf6Z3cBlsiKEerUxS5aa9mOeKvWW6A2uIvZwb1fbjWfE4NyTIrR4VodmXjDwbwquQncYrV7lhOxx/sjwJ4ekXxA= X-Received: by 2002:a37:f504:0:b0:6cf:5fa1:15f8 with SMTP id l4-20020a37f504000000b006cf5fa115f8mr81140787qkk.748.1670431757330; Wed, 07 Dec 2022 08:49:17 -0800 (PST) MIME-Version: 1.0 References: <20221205085351.27566-1-tmaimon77@gmail.com> <20221205085351.27566-3-tmaimon77@gmail.com> In-Reply-To: From: Andy Shevchenko Date: Wed, 7 Dec 2022 18:48:41 +0200 Message-ID: Subject: Re: [PATCH v2 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver To: Adrian Hunter Cc: Tomer Maimon , 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 Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 7, 2022 at 3:49 PM Adrian Hunter wrote: > 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. Thank you! > > 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? -- With Best Regards, Andy Shevchenko 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 7F088C352A1 for ; Wed, 7 Dec 2022 16:50:24 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4NS3Dt5b7Dz3bgs for ; Thu, 8 Dec 2022 03:50:22 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20210112 header.b=SMK2V74s; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gmail.com (client-ip=2607:f8b0:4864:20::72b; helo=mail-qk1-x72b.google.com; envelope-from=andy.shevchenko@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20210112 header.b=SMK2V74s; dkim-atps=neutral Received: from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com [IPv6:2607:f8b0:4864:20::72b]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4NS3Cm4RhKz3bXt for ; Thu, 8 Dec 2022 03:49:22 +1100 (AEDT) Received: by mail-qk1-x72b.google.com with SMTP id j26so10399271qki.10 for ; Wed, 07 Dec 2022 08:49:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=y18DIpbfCukk25TWC8zheO2SUvaGDQaONDYCY89KTGU=; b=SMK2V74sU8D3TcIrRaSUNwZYnaaTCeoM4k+ZcZTFw/ydUkhwZLbo1yEKDxUMIBS4gA iwNJGXqzS6w1suvYGlU8ecRhl6NNxEKbu7JAvBOIviytxXOltcSBfBeWp+TnxyBxXr9j aMWtjDuBUNQOGk7gO4jGnxDWTQ8/yXDvz53Y4z4R9i7/KWUTuiCGkDsrHLRa/o9SuTb7 KqorQSVIORS6lv2KWgYWjY+eZry7XlEBoWrdcJ1POsluOkv3Gc8qESPlNGyPzJ94Jsq1 5AvRk0IRxLOSHpjNN6gxt9LAglz7AWcGJObOYuX4CKqkCJ0dV+a7TLBA1pKfycAlJzKC YqrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=y18DIpbfCukk25TWC8zheO2SUvaGDQaONDYCY89KTGU=; b=EQmIZuRGHr5iKoDAQXNKd/gphdRCPazWZwd7zptjDsfteDRD8amcCeHHPKFSbUUR5V ZXJOtIZH/dqOWacLNqDZ8HcvtIS2qav9retOEVBGrb44hgeUKin7gFnDsexZh557LA/V drU0shk2U3R6XPNyaaz2Em5ca+baxQSMegHC631lJajzVk09LLhv209oHK2i66WWU78B tWXH+ukjRy0lI1EU1Kmlgo0RQcLQrZHab0deALoj/iZGIijPY2RsOkltEdF0Sa2gvglR Mpoa3DoUjko8X+1+RRsEWtKIs8Dh/B7ogcpSQrXZU8cu9WjFdSCNlc4bZ0g5rd2dYUG9 Nr3w== X-Gm-Message-State: ANoB5pkBNmCo5wU8YmNMpKMZV2fK6qEb5D3YOnS7JttZZfy6oFghKZff /kwyd8Q9piOETaUPA126N/p+FwqPL1b8DW1lg3k= X-Google-Smtp-Source: AA0mqf6Z3cBlsiKEerUxS5aa9mOeKvWW6A2uIvZwb1fbjWfE4NyTIrR4VodmXjDwbwquQncYrV7lhOxx/sjwJ4ekXxA= X-Received: by 2002:a37:f504:0:b0:6cf:5fa1:15f8 with SMTP id l4-20020a37f504000000b006cf5fa115f8mr81140787qkk.748.1670431757330; Wed, 07 Dec 2022 08:49:17 -0800 (PST) MIME-Version: 1.0 References: <20221205085351.27566-1-tmaimon77@gmail.com> <20221205085351.27566-3-tmaimon77@gmail.com> In-Reply-To: From: Andy Shevchenko Date: Wed, 7 Dec 2022 18:48:41 +0200 Message-ID: Subject: Re: [PATCH v2 2/2] mmc: sdhci-npcm: Add NPCM SDHCI driver To: Adrian Hunter Content-Type: text/plain; charset="UTF-8" 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, Tomer Maimon , 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, benjaminfair@google.com Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" On Wed, Dec 7, 2022 at 3:49 PM Adrian Hunter wrote: > 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. Thank you! > > 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? -- With Best Regards, Andy Shevchenko