[{"id":2912282,"web_url":"http://patchwork.ozlabs.org/comment/2912282/","msgid":"","list_archive_url":null,"date":"2022-06-14T08:23:33","subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT child\n nodes number","submitter":{"id":82259,"url":"http://patchwork.ozlabs.org/api/people/82259/?format=json","name":"Damien Le Moal","email":"damien.lemoal@opensource.wdc.com"},"content":"On 6/10/22 17:17, Serge Semin wrote:\n> Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical\n> from the further AHCI-platform initialization point of view since\n> exceeding the ports upper limit will cause allocating more resources than\n> will be used afterwards. But detecting too many child DT-nodes doesn't\n> seem right since it's very unlikely to have it on an ordinary platform. In\n> accordance with the AHCI specification there can't be more than 32 ports\n> implemented at least due to having the CAP.NP field of 5 bits wide and the\n> PI register of dword size. Thus if such situation is found the DTB must\n> have been corrupted and the data read from it shouldn't be reliable. Let's\n> consider that as an erroneous situation and halt further resources\n> allocation.\n> \n> Note it's logically more correct to have the nports set only after the\n> initialization value is checked for being sane. So while at it let's make\n> sure nports is assigned with a correct value.\n> \n> Signed-off-by: Serge Semin \n> Reviewed-by: Hannes Reinecke \n> \n> ---\n> \n> Changelog v2:\n> - Drop the else word from the child_nodes value checking if-else-if\n> statement (@Damien) and convert the after-else part into the ternary\n> operator-based statement.\n> \n> Changelog v4:\n> - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)\n> ---\n> drivers/ata/libahci_platform.c | 13 ++++++++++---\n> 1 file changed, 10 insertions(+), 3 deletions(-)\n> \n> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c\n> index 814804582d1d..8aed7b29c7ab 100644\n> --- a/drivers/ata/libahci_platform.c\n> +++ b/drivers/ata/libahci_platform.c\n> @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n> \t\t}\n> \t}\n> \n> -\thpriv->nports = child_nodes = of_get_child_count(dev->of_node);\n> +\t/*\n> +\t * Too many sub-nodes most likely means having something wrong with\n> +\t * the firmware.\n> +\t */\n> +\tchild_nodes = of_get_child_count(dev->of_node);\n> +\tif (child_nodes > AHCI_MAX_PORTS) {\n> +\t\trc = -EINVAL;\n> +\t\tgoto err_out;\n> +\t}\n> \n> \t/*\n> \t * If no sub-node was found, we still need to set nports to\n> \t * one in order to be able to use the\n> \t * ahci_platform_[en|dis]able_[phys|regulators] functions.\n> \t */\n> -\tif (!child_nodes)\n> -\t\thpriv->nports = 1;\n> +\thpriv->nports = child_nodes ?: 1;\n\nThis change is not necessary and makes the code far less easy to read.\n\n> \n> \thpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);\n> \tif (!hpriv->phys) {","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n unprotected) header.d=wdc.com header.i=@wdc.com header.a=rsa-sha256\n header.s=dkim.wdc.com header.b=QlL2rfWP;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=opensource.wdc.com header.i=@opensource.wdc.com\n header.a=rsa-sha256 header.s=dkim header.b=cZgBSiPG;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)","usg-ed-osssrv.wdc.com (amavisd-new); dkim=pass\n reason=\"pass (just generated, assumed good)\"\n header.d=opensource.wdc.com"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LMhKY08r7z9sGH\n\tfor ; Tue, 14 Jun 2022 18:23:45 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S240175AbiFNIXm (ORCPT );\n Tue, 14 Jun 2022 04:23:42 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:48292 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S1355013AbiFNIXk (ORCPT\n ); Tue, 14 Jun 2022 04:23:40 -0400","from esa2.hgst.iphmx.com (esa2.hgst.iphmx.com [68.232.143.124])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DD434366B7\n for ; Tue, 14 Jun 2022 01:23:38 -0700 (PDT)","from uls-op-cesaip01.wdc.com (HELO uls-op-cesaep01.wdc.com)\n ([199.255.45.14])\n by ob1.hgst.iphmx.com with ESMTP; 14 Jun 2022 16:23:38 +0800","from uls-op-cesaip01.wdc.com ([10.248.3.36])\n by uls-op-cesaep01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 14 Jun 2022 00:46:36 -0700","from usg-ed-osssrv.wdc.com ([10.3.10.180])\n by uls-op-cesaip01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 14 Jun 2022 01:23:38 -0700","from usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTP id 4LMhKP3dttz1Rwrw\n for ; Tue, 14 Jun 2022 01:23:37 -0700 (PDT)","from usg-ed-osssrv.wdc.com ([127.0.0.1])\n by usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n (amavisd-new, port 10026)\n with ESMTP id bk7wS6trIoMK for ;\n Tue, 14 Jun 2022 01:23:36 -0700 (PDT)","from [10.225.163.77] (unknown [10.225.163.77])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTPSA id 4LMhKM12P2z1Rvlc;\n Tue, 14 Jun 2022 01:23:34 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=simple/simple;\n d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com;\n t=1655195018; x=1686731018;\n h=message-id:date:mime-version:subject:to:cc:references:\n from:in-reply-to:content-transfer-encoding;\n bh=MmvF2pvUPJk+l0G2cFMgXO+laIHLHX4qzgsWbZlC+FM=;\n b=QlL2rfWPlcVuBEtpFfnKIiMCtGcw9n2i59nn4mhIOMmv25Qbw0VPzL3M\n i8xpIHbGKtCpZ/BKq/PAu9WtmJlna2dLzWJTPtuMNHsEBJV1XmClzhoaE\n /Uy2CjhZ/KqFl5TmGPsHP8eifacScD+R+zY/nuotXveeK05cxGtwtdDKw\n vwwiqZQKYdfpWwkb25hAwn5VhnrbF8W45mdh/z4J36U8+NG6F5VTGi/Yq\n C7d1YVeIN/8vD/TeDyN427C46OAPFjRWUJmb7QCTkiL01HtEUsNigkrHH\n o7QDPNlubCM9Rg2Wg2I+kTj0QNyB7HiUMDBjcWbmtkOjDmuxqDBjOdvw8\n w==;","v=1; a=rsa-sha256; c=relaxed/simple; d=\n opensource.wdc.com; h=content-transfer-encoding:content-type\n :in-reply-to:organization:from:references:to:content-language\n :subject:user-agent:mime-version:date:message-id; s=dkim; t=\n 1655195016; x=1657787017; bh=MmvF2pvUPJk+l0G2cFMgXO+laIHLHX4qzgs\n WbZlC+FM=; b=cZgBSiPGopdTy3/f3GexXyiQJGRLQCM5ePdjjqM+Mw5uensNmg5\n 02s/0vZmih6XWX3v/F4UyynmVBuVpMJ2CVW7sEVuq8T7d/YwePnO7JHp8VfZ+w+d\n JSNT7ngTk1d/PUJ8YQ40msTkvWOXYLjJdvPXV+7PuO2TLoVRNiMbnLgpLSZnHyv7\n A6BHlclzYoVKYxydhG30lwNfoZiVED4rP/waEx06tT1RuVq+FDfldH2bsjCGpntQ\n Bwwu1aIOQ3ZgrRiHm5hSpQEhLTjh1okSo1jCfDt5uC9ujFkmQMc9zxFNcNlFT0N6\n aEZ0ufp9l94E4+baq7rH9HpKd+88a+Era9g=="],"X-IronPort-AV":"E=Sophos;i=\"5.91,299,1647273600\";\n d=\"scan'208\";a=\"307384170\"","IronPort-SDR":["\n PrhKaXvVUYL1v64ld3+53kPWnndHkHttNCcz/ZY/aHh0zUR4d+lMLwt3JE/zGI37KgNo8UYQHW\n RZGVoF+XoN8/aQifhSJte//BDEkoWNc5FyJ3lecBG4KwS2gDOdZleHii0D9vBo4jICIf6Mqplm\n 6TlbnbXsFr5afPw/cAhtHqpg/w91VI8kFgqr0kK5OafeDuHlMrCJsVl501OJkZ7TJXdSxC1yr0\n F972lF8RtT8bvuyfLQmjOOYhYCkMskTcZn+kvqAvXkWREp6PKi/dJi9pKUKMSiKOFGyo42LFzF\n 9W0Efe3T0Y7v382zu75wyy6P","\n bHVajle4zaJxSR61fox3T+6OBecwou/xhW0PD5ouI1p1GzgsldUL3dcZMb9dIcIc4jQdEbUpmC\n 91Ol77V+aUVNquhifZf4byiGKpSIN1SqICcA2YRzNGIVVAqoINIWp3CV8DOZR0NlO19Y7fT4fl\n +B8m0UbbAe699hz5J3lyFAm09ndvd8cAgnH/MjVEZvwc7MI6zkObrmIqq8SJWm0kLxJ5OqJxVn\n 45nKg6eW+3KbgTcXoXXI57DZH8BIBwxtIY7zoZx7k+AxXCFd+YszL3rsE2sotbSsuhflSTPoMt\n D1g="],"WDCIronportException":"Internal","X-Virus-Scanned":"amavisd-new at usg-ed-osssrv.wdc.com","Message-ID":"","Date":"Tue, 14 Jun 2022 17:23:33 +0900","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n Thunderbird/91.10.0","Subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT child\n nodes number","Content-Language":"en-US","To":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ","Cc":"Serge Semin ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-9-Sergey.Semin@baikalelectronics.ru>","From":"Damien Le Moal ","Organization":"Western Digital Research","In-Reply-To":"<20220610081801.11854-9-Sergey.Semin@baikalelectronics.ru>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-Spam-Status":"No, score=-5.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,\n SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham\n autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2913719,"web_url":"http://patchwork.ozlabs.org/comment/2913719/","msgid":"<20220615205328.chwruabvksdbnaex@mobilestation>","list_archive_url":null,"date":"2022-06-15T20:53:28","subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT\n child nodes number","submitter":{"id":70038,"url":"http://patchwork.ozlabs.org/api/people/70038/?format=json","name":"Serge Semin","email":"fancer.lancer@gmail.com"},"content":"On Tue, Jun 14, 2022 at 05:23:33PM +0900, Damien Le Moal wrote:\n> On 6/10/22 17:17, Serge Semin wrote:\n> > Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical\n> > from the further AHCI-platform initialization point of view since\n> > exceeding the ports upper limit will cause allocating more resources than\n> > will be used afterwards. But detecting too many child DT-nodes doesn't\n> > seem right since it's very unlikely to have it on an ordinary platform. In\n> > accordance with the AHCI specification there can't be more than 32 ports\n> > implemented at least due to having the CAP.NP field of 5 bits wide and the\n> > PI register of dword size. Thus if such situation is found the DTB must\n> > have been corrupted and the data read from it shouldn't be reliable. Let's\n> > consider that as an erroneous situation and halt further resources\n> > allocation.\n> > \n> > Note it's logically more correct to have the nports set only after the\n> > initialization value is checked for being sane. So while at it let's make\n> > sure nports is assigned with a correct value.\n> > \n> > Signed-off-by: Serge Semin \n> > Reviewed-by: Hannes Reinecke \n> > \n> > ---\n> > \n> > Changelog v2:\n> > - Drop the else word from the child_nodes value checking if-else-if\n> > statement (@Damien) and convert the after-else part into the ternary\n> > operator-based statement.\n> > \n> > Changelog v4:\n> > - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)\n> > ---\n> > drivers/ata/libahci_platform.c | 13 ++++++++++---\n> > 1 file changed, 10 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c\n> > index 814804582d1d..8aed7b29c7ab 100644\n> > --- a/drivers/ata/libahci_platform.c\n> > +++ b/drivers/ata/libahci_platform.c\n> > @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n> > \t\t}\n> > \t}\n> > \n> > -\thpriv->nports = child_nodes = of_get_child_count(dev->of_node);\n> > +\t/*\n> > +\t * Too many sub-nodes most likely means having something wrong with\n> > +\t * the firmware.\n> > +\t */\n> > +\tchild_nodes = of_get_child_count(dev->of_node);\n> > +\tif (child_nodes > AHCI_MAX_PORTS) {\n> > +\t\trc = -EINVAL;\n> > +\t\tgoto err_out;\n> > +\t}\n> > \n> > \t/*\n> > \t * If no sub-node was found, we still need to set nports to\n> > \t * one in order to be able to use the\n> > \t * ahci_platform_[en|dis]able_[phys|regulators] functions.\n> > \t */\n> > -\tif (!child_nodes)\n> > -\t\thpriv->nports = 1;\n> > +\thpriv->nports = child_nodes ?: 1;\n> \n\n> This change is not necessary and makes the code far less easy to read.\n\nelaborate please. What change? What part of this change makes the code\nless easy to read?\n\n-Sergey\n\n> \n> > \n> > \thpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);\n> > \tif (!hpriv->phys) {\n> \n> \n> -- \n> Damien Le Moal\n> Western Digital Research","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256\n header.s=20210112 header.b=nyhckwPz;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LNcwQ5QMgz9sGF\n\tfor ; Thu, 16 Jun 2022 06:53:42 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S232547AbiFOUxk (ORCPT );\n Wed, 15 Jun 2022 16:53:40 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:49300 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S1349323AbiFOUxe (ORCPT\n ); Wed, 15 Jun 2022 16:53:34 -0400","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n [IPv6:2a00:1450:4864:20::12a])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A0ED054FA6;\n Wed, 15 Jun 2022 13:53:32 -0700 (PDT)","by mail-lf1-x12a.google.com with SMTP id c2so20811929lfk.0;\n Wed, 15 Jun 2022 13:53:32 -0700 (PDT)","from mobilestation ([95.79.189.214])\n by smtp.gmail.com with ESMTPSA id\n t20-20020a056512069400b004795311530asm1911636lfe.209.2022.06.15.13.53.29\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Wed, 15 Jun 2022 13:53:30 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=gmail.com; s=20210112;\n h=date:from:to:cc:subject:message-id:references:mime-version\n :content-disposition:in-reply-to;\n bh=dkNLjdDW3Fz/m7M441ZwWT9nGf+j42bVYyQKzjzvhUs=;\n b=nyhckwPzYvb5+DY7chXK08bpIkP5CR0LjriS4B82ISsX9equWKiptiscQ9eRxWMKtj\n k2SUVn3ZcCRcWEjCPmW/R4jsaQ0SzJ+zdGDYFkhCdGx3PU+CEluPuePcpULVe4NEbsvD\n 2v6N4iS8qH6gt9ZfZjsJy+fpuS6RJ+YWcmdP0gROM5lIXmTgANry/M07Df0z/+Zu7ejG\n 6tEQXc1stzbOEISFZHTNnZQnr9MIyrikEUiNO4KRgdhc82kDzLh28vJE3XGiSrDrSSP0\n 3n2nK18z0ckgsbnoB/RtgNRUUKm1z4TULbLOsQyOJrOD59p+613VtvJpI0+70aXfl3Lu\n x5+A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=dkNLjdDW3Fz/m7M441ZwWT9nGf+j42bVYyQKzjzvhUs=;\n b=Jsva56ilnqudemQY3Fw8qnDxcn7+1hGXAzhsA+oU3AL2b66V4x0tnbQj+OZfIG0/mW\n Uzn5zyYHaTqI9t27xRFNqbu4TDf/LSBwaHKJsUmg/cp5XwAEg9LS0JpO7x3+WkxwTzJB\n IE7dDQqtDyCVTqLcnLnYUZz5RE/4/BFynXdfs2l/uDffauZX87avzQXqbapeJhObR/VT\n 7YmeCHrU5qeVtRbCmSWCWaKh/FjWz2t7u2jjSsSHiAQE6zVsR6RSWrZb+FAVNUds+qNI\n glEkhyP5CSameuNOveWtk2cf1ChCCCuhKxOp2M3yKUJSptkjbyjqy5R3C0oSJhZOYTWC\n RM9A==","X-Gm-Message-State":"AJIora9TKzpLmQHmKSS6ORO+Pe0/MQz9aRrpR67Ty+GXJdkpc6l3omta\n IK7nYQHCo7KsPTWEQu6dBtE=","X-Google-Smtp-Source":"\n AGRyM1u51hJv1mQ/ajBoNSZ6zc7XEpSrk9yR0l23nwS1A6zSYkrB4ctYGJDOTzdXXP7wlbkJl672rQ==","X-Received":"by 2002:a19:2d51:0:b0:479:1269:a146 with SMTP id\n t17-20020a192d51000000b004791269a146mr780304lft.572.1655326411015;\n Wed, 15 Jun 2022 13:53:31 -0700 (PDT)","Date":"Wed, 15 Jun 2022 23:53:28 +0300","From":"Serge Semin ","To":"Damien Le Moal ","Cc":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT\n child nodes number","Message-ID":"<20220615205328.chwruabvksdbnaex@mobilestation>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-9-Sergey.Semin@baikalelectronics.ru>\n ","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,\n RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2913851,"web_url":"http://patchwork.ozlabs.org/comment/2913851/","msgid":"<6d16fe23-012d-39fb-21e5-39ce50f7b03a@opensource.wdc.com>","list_archive_url":null,"date":"2022-06-16T00:25:48","subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT child\n nodes number","submitter":{"id":82259,"url":"http://patchwork.ozlabs.org/api/people/82259/?format=json","name":"Damien Le Moal","email":"damien.lemoal@opensource.wdc.com"},"content":"On 2022/06/16 5:53, Serge Semin wrote:\n> On Tue, Jun 14, 2022 at 05:23:33PM +0900, Damien Le Moal wrote:\n>> On 6/10/22 17:17, Serge Semin wrote:\n>>> Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical\n>>> from the further AHCI-platform initialization point of view since\n>>> exceeding the ports upper limit will cause allocating more resources than\n>>> will be used afterwards. But detecting too many child DT-nodes doesn't\n>>> seem right since it's very unlikely to have it on an ordinary platform. In\n>>> accordance with the AHCI specification there can't be more than 32 ports\n>>> implemented at least due to having the CAP.NP field of 5 bits wide and the\n>>> PI register of dword size. Thus if such situation is found the DTB must\n>>> have been corrupted and the data read from it shouldn't be reliable. Let's\n>>> consider that as an erroneous situation and halt further resources\n>>> allocation.\n>>>\n>>> Note it's logically more correct to have the nports set only after the\n>>> initialization value is checked for being sane. So while at it let's make\n>>> sure nports is assigned with a correct value.\n>>>\n>>> Signed-off-by: Serge Semin \n>>> Reviewed-by: Hannes Reinecke \n>>>\n>>> ---\n>>>\n>>> Changelog v2:\n>>> - Drop the else word from the child_nodes value checking if-else-if\n>>> statement (@Damien) and convert the after-else part into the ternary\n>>> operator-based statement.\n>>>\n>>> Changelog v4:\n>>> - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)\n>>> ---\n>>> drivers/ata/libahci_platform.c | 13 ++++++++++---\n>>> 1 file changed, 10 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c\n>>> index 814804582d1d..8aed7b29c7ab 100644\n>>> --- a/drivers/ata/libahci_platform.c\n>>> +++ b/drivers/ata/libahci_platform.c\n>>> @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n>>> \t\t}\n>>> \t}\n>>> \n>>> -\thpriv->nports = child_nodes = of_get_child_count(dev->of_node);\n>>> +\t/*\n>>> +\t * Too many sub-nodes most likely means having something wrong with\n>>> +\t * the firmware.\n>>> +\t */\n>>> +\tchild_nodes = of_get_child_count(dev->of_node);\n>>> +\tif (child_nodes > AHCI_MAX_PORTS) {\n>>> +\t\trc = -EINVAL;\n>>> +\t\tgoto err_out;\n>>> +\t}\n>>> \n>>> \t/*\n>>> \t * If no sub-node was found, we still need to set nports to\n>>> \t * one in order to be able to use the\n>>> \t * ahci_platform_[en|dis]able_[phys|regulators] functions.\n>>> \t */\n>>> -\tif (!child_nodes)\n>>> -\t\thpriv->nports = 1;\n>>> +\thpriv->nports = child_nodes ?: 1;\n>>\n> \n>> This change is not necessary and makes the code far less easy to read.\n> \n> elaborate please. What change? What part of this change makes the code\n> less easy to read?\n\nYou changed:\n\n\tif (!child_nodes)\n\t\thpriv->nports = 1;\n\nto:\n\n\thpriv->nports = child_nodes ?: 1;\n\nThat is the same. So the change is not needed in the first place, and worse,\nmakes the code way harder to read for no good reason.\n\n> \n> -Sergey\n> \n>>\n>>> \n>>> \thpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);\n>>> \tif (!hpriv->phys) {\n>>\n>>\n>> -- \n>> Damien Le Moal\n>> Western Digital Research","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n unprotected) header.d=wdc.com header.i=@wdc.com header.a=rsa-sha256\n header.s=dkim.wdc.com header.b=ekgzRZ81;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=opensource.wdc.com header.i=@opensource.wdc.com\n header.a=rsa-sha256 header.s=dkim header.b=S+tY2y1w;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)","usg-ed-osssrv.wdc.com (amavisd-new); dkim=pass\n reason=\"pass (just generated, assumed good)\"\n header.d=opensource.wdc.com"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LNjdK3dBBz9sG2\n\tfor ; Thu, 16 Jun 2022 10:25:57 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S1343724AbiFPAZ4 (ORCPT );\n Wed, 15 Jun 2022 20:25:56 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:33844 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S241758AbiFPAZz (ORCPT\n ); Wed, 15 Jun 2022 20:25:55 -0400","from esa1.hgst.iphmx.com (esa1.hgst.iphmx.com [68.232.141.245])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7EB0F1F2D8\n for ; Wed, 15 Jun 2022 17:25:54 -0700 (PDT)","from h199-255-45-14.hgst.com (HELO uls-op-cesaep01.wdc.com)\n ([199.255.45.14])\n by ob1.hgst.iphmx.com with ESMTP; 16 Jun 2022 08:25:52 +0800","from uls-op-cesaip01.wdc.com ([10.248.3.36])\n by uls-op-cesaep01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 15 Jun 2022 16:48:42 -0700","from usg-ed-osssrv.wdc.com ([10.3.10.180])\n by uls-op-cesaip01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 15 Jun 2022 17:25:53 -0700","from usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTP id 4LNjdD32tGz1SVnx\n for ; Wed, 15 Jun 2022 17:25:52 -0700 (PDT)","from usg-ed-osssrv.wdc.com ([127.0.0.1])\n by usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n (amavisd-new, port 10026)\n with ESMTP id mm_StWzCclhK for ;\n Wed, 15 Jun 2022 17:25:51 -0700 (PDT)","from [10.89.84.185] (c02drav6md6t.dhcp.fujisawa.hgst.com\n [10.89.84.185])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTPSA id 4LNjd95DxZz1Rvlc;\n Wed, 15 Jun 2022 17:25:49 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=simple/simple;\n d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com;\n t=1655339154; x=1686875154;\n h=message-id:date:mime-version:subject:to:cc:references:\n from:in-reply-to:content-transfer-encoding;\n bh=3O+o768OC3iqAiJUZClTU8MbjeZHe54EszKexKJRWLg=;\n b=ekgzRZ81ABkg4U3PnfTwQ038W0Dpj5g+/WLqTFzS2jmrV4Bw7zQ1NTFJ\n 63PKTBv3eA8Vb+w31RYiXPOHw5EF/imCfYM3kp61uOUmMkRcprZQKPcYr\n pt67dVRWpss//hf7renmiwnSFbWwtRuRG6LPv0tTFezLcvOHlzD7SuykW\n be25awIxM25p+mALUFpkTLwl0CadioCWOGGWSvvX6764wS5/NGowUEff1\n UBBbi3jAi5aMFfuAxwU3xM7kWRlZMaqhmBP33kQJnAvlI6uGZe7kSuih4\n m9OCFKLtljxs/ngp4LW8OyrUMOB/U8UptmNAvYqoPimeROvjDQh6Z+xFJ\n g==;","v=1; a=rsa-sha256; c=relaxed/simple; d=\n opensource.wdc.com; h=content-transfer-encoding:content-type\n :in-reply-to:organization:from:references:to:content-language\n :subject:user-agent:mime-version:date:message-id; s=dkim; t=\n 1655339151; x=1657931152; bh=3O+o768OC3iqAiJUZClTU8MbjeZHe54EszK\n exKJRWLg=; b=S+tY2y1wHtR8fC9ecLVgckNzkJOyDYraBIvctmw2wMv3rIXC8gu\n Cb1CYDkv3mouD7iTx1w67ubCb3LLA3lM75hWyizqrJewNo1yMXv9nPcwRMafftgV\n Zwlvegz6UW9fyLBrwtD2C49EGiSokBjGr11QivKQBeQ6wUEh3k+XzsApf/+G/7an\n zmkijV2uiwTojsFfVDe2JR2xNOG0uQOBMyAu1451YN0CL33MXoWmQW4IcBYct0Ct\n q+xq6vCXG15RquGQRAd+eMnjZ1R9wONDwecWaLvK4I2RdMQAUdV0D70MWUVIBIsP\n smOjU3A4+n38d9Gn/2WWe6qfz2fKzoXq4PQ=="],"X-IronPort-AV":"E=Sophos;i=\"5.91,302,1647273600\";\n d=\"scan'208\";a=\"315352713\"","IronPort-SDR":["\n R6MH+nbSgnguAbxgeXUHpphsBFxrGqp7CL2KtTWVi1J2EdhtDlkXcmFJm/MMK1o6pps9FFdDGZ\n gOpUMpV8nWX+bds4p6v+YTwQTidAGPzbiNcUoQaM9n18w4srq1QbP7u1G68wkNhNzTmxiHYHo7\n pjd9GIx8pTLsnvspZYyc1SZ4W+AQxl3OWe5XSzxWJNHpcpM7r+w4lo/4RmNmE9RtduBGTzoSil\n DigW/40SaZMNdjXSWnj/t3nJgwg3MGQxFwNxUQjGaADp0JBCArI1Dy+C98v5mztysUeOTsTTkR\n yp+V3MD0u9wSpyc7y2DxYjmt","\n OhE28f2XbTlGd/FuUPgYW1P+0hzZlJyvbz9nXD6E6U6ulaWAxVCipHTciPkwPf0jZr5HHN9FMR\n Keb5lue9i2T8v8DLsEqTC36LUGv9gXDFsP+z+VVTlin65sdoOyaZejPmAyiX69vsJpjape8vqu\n 3iXFme7S996qVT/sQL6vyS772XuWieGVHRtWdhZ/CJlt+cvEx0N/C5LPKShwLjPDbzb/wsiu1M\n c+NwV3ciRjonjP12rJ6vZTporoede/kBVdhFGYaAj8GPwM0xqe20yk9OHlIl8bw6S6p51k/xTO\n jV0="],"WDCIronportException":"Internal","X-Virus-Scanned":"amavisd-new at usg-ed-osssrv.wdc.com","Message-ID":"<6d16fe23-012d-39fb-21e5-39ce50f7b03a@opensource.wdc.com>","Date":"Thu, 16 Jun 2022 09:25:48 +0900","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)\n Gecko/20100101 Thunderbird/91.10.0","Subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT child\n nodes number","Content-Language":"en-US","To":"Serge Semin ","Cc":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-9-Sergey.Semin@baikalelectronics.ru>\n \n <20220615205328.chwruabvksdbnaex@mobilestation>","From":"Damien Le Moal ","Organization":"Western Digital Research","In-Reply-To":"<20220615205328.chwruabvksdbnaex@mobilestation>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-Spam-Status":"No, score=-5.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,\n SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable\n autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2915215,"web_url":"http://patchwork.ozlabs.org/comment/2915215/","msgid":"<20220617201855.cf64vbhe6wk4hrcu@mobilestation>","list_archive_url":null,"date":"2022-06-17T20:18:55","subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT\n child nodes number","submitter":{"id":70038,"url":"http://patchwork.ozlabs.org/api/people/70038/?format=json","name":"Serge Semin","email":"fancer.lancer@gmail.com"},"content":"On Thu, Jun 16, 2022 at 09:25:48AM +0900, Damien Le Moal wrote:\n> On 2022/06/16 5:53, Serge Semin wrote:\n> > On Tue, Jun 14, 2022 at 05:23:33PM +0900, Damien Le Moal wrote:\n> >> On 6/10/22 17:17, Serge Semin wrote:\n> >>> Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical\n> >>> from the further AHCI-platform initialization point of view since\n> >>> exceeding the ports upper limit will cause allocating more resources than\n> >>> will be used afterwards. But detecting too many child DT-nodes doesn't\n> >>> seem right since it's very unlikely to have it on an ordinary platform. In\n> >>> accordance with the AHCI specification there can't be more than 32 ports\n> >>> implemented at least due to having the CAP.NP field of 5 bits wide and the\n> >>> PI register of dword size. Thus if such situation is found the DTB must\n> >>> have been corrupted and the data read from it shouldn't be reliable. Let's\n> >>> consider that as an erroneous situation and halt further resources\n> >>> allocation.\n> >>>\n> >>> Note it's logically more correct to have the nports set only after the\n> >>> initialization value is checked for being sane. So while at it let's make\n> >>> sure nports is assigned with a correct value.\n> >>>\n> >>> Signed-off-by: Serge Semin \n> >>> Reviewed-by: Hannes Reinecke \n> >>>\n> >>> ---\n> >>>\n> >>> Changelog v2:\n> >>> - Drop the else word from the child_nodes value checking if-else-if\n> >>> statement (@Damien) and convert the after-else part into the ternary\n> >>> operator-based statement.\n> >>>\n> >>> Changelog v4:\n> >>> - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)\n> >>> ---\n> >>> drivers/ata/libahci_platform.c | 13 ++++++++++---\n> >>> 1 file changed, 10 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c\n> >>> index 814804582d1d..8aed7b29c7ab 100644\n> >>> --- a/drivers/ata/libahci_platform.c\n> >>> +++ b/drivers/ata/libahci_platform.c\n> >>> @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n> >>> \t\t}\n> >>> \t}\n> >>> \n> >>> -\thpriv->nports = child_nodes = of_get_child_count(dev->of_node);\n> >>> +\t/*\n> >>> +\t * Too many sub-nodes most likely means having something wrong with\n> >>> +\t * the firmware.\n> >>> +\t */\n> >>> +\tchild_nodes = of_get_child_count(dev->of_node);\n> >>> +\tif (child_nodes > AHCI_MAX_PORTS) {\n> >>> +\t\trc = -EINVAL;\n> >>> +\t\tgoto err_out;\n> >>> +\t}\n> >>> \n> >>> \t/*\n> >>> \t * If no sub-node was found, we still need to set nports to\n> >>> \t * one in order to be able to use the\n> >>> \t * ahci_platform_[en|dis]able_[phys|regulators] functions.\n> >>> \t */\n> >>> -\tif (!child_nodes)\n> >>> -\t\thpriv->nports = 1;\n> >>> +\thpriv->nports = child_nodes ?: 1;\n> >>\n> > \n> >> This change is not necessary and makes the code far less easy to read.\n> > \n> > elaborate please. What change? What part of this change makes the code\n> > less easy to read?\n> \n\n> You changed:\n> \n> \tif (!child_nodes)\n> \t\thpriv->nports = 1;\n> \n> to:\n> \n> \thpriv->nports = child_nodes ?: 1;\n> \n> That is the same. So the change is not needed in the first place, and worse,\n> makes the code way harder to read for no good reason.\n\nNo, they aren't the same:\n+\tif (!child_nodes)\n+\t\thpriv->nports = 1;\nand\n+\thpriv->nports = child_nodes ?: 1;\naren't equivalent. The equivalent implementation would be:\n+\tif (child_nodes)\n+\t\thpriv->nports = child_nodes;\n+\telse\n+\t\thpriv->nports = 1;\n\nAs I said in the patchlog, hpriv->nports is updated now only if\nof_get_child_count() returns a valid number of the child nodes,\nports, which semantically is more correct. In the previous\nimplementation it was always set to the number of child nodes\nno matter whether that value was correct or not.\n\nRegarding the ternary operator with omitted operand. Well, it's not\nthat rare beast in the kernel:\n$ grep -r \"?:\" kernel/ drivers/ mm/ fs/ block/ | wc -l\n699\nBut if you insist in it being not that readable, I can replace it with\nmore bulky if-else statement. Do you?\n\n-Sergey\n\n> \n> > \n> > -Sergey\n> > \n> >>\n> >>> \n> >>> \thpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);\n> >>> \tif (!hpriv->phys) {\n> >>\n> >>\n> >> -- \n> >> Damien Le Moal\n> >> Western Digital Research\n> \n> \n> -- \n> Damien Le Moal\n> Western Digital Research","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256\n header.s=20210112 header.b=VGT133l2;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LPr3f4kdkz9sG0\n\tfor ; Sat, 18 Jun 2022 06:19:10 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S234545AbiFQUTF (ORCPT );\n Fri, 17 Jun 2022 16:19:05 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:53082 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S239060AbiFQUTE (ORCPT\n ); Fri, 17 Jun 2022 16:19:04 -0400","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n [IPv6:2a00:1450:4864:20::235])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3F4AD5001E;\n Fri, 17 Jun 2022 13:19:00 -0700 (PDT)","by mail-lj1-x235.google.com with SMTP id w2so2483785lji.5;\n Fri, 17 Jun 2022 13:19:00 -0700 (PDT)","from mobilestation ([95.79.189.214])\n by smtp.gmail.com with ESMTPSA id\n x16-20020a2e7c10000000b00253d84812edsm659117ljc.2.2022.06.17.13.18.56\n (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n Fri, 17 Jun 2022 13:18:57 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=gmail.com; s=20210112;\n h=date:from:to:cc:subject:message-id:references:mime-version\n :content-disposition:in-reply-to;\n bh=6CFz7/vywGyF9mFNfYc/+8/AiVUzu6xNEnAULo+5cj4=;\n b=VGT133l2psyPg5mTAIDEuZh+UdKd7/Z3XMRnRXFEyqDicC9hU6qUHURjGVWitncFWc\n PS+RD5nCh9rWet+wKtSL1Sckvr4PbNoTZCCM/nHwyu4oUe6Gio5fNH4TeBNTBpglLRmC\n IpLnE/spxqwprW0ANkKg4ZEporbixUPDLPNiFbfjTCv/rkF9oPFhifkw9yqlwfuhy7Kg\n F0LjHmz1WVn0g1zs5Exk7NwH6oDhQBw32xIvNPUdVSzivajvIwUJreqoy85X1HX3bgcx\n zmSnILFwLJ1Wz10B9bsyYfFRi2oegN7I0xjrhGJ+QUHcfL0/OwCk70njMjb4AdTVc5i7\n a93Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n d=1e100.net; s=20210112;\n h=x-gm-message-state:date:from:to:cc:subject:message-id:references\n :mime-version:content-disposition:in-reply-to;\n bh=6CFz7/vywGyF9mFNfYc/+8/AiVUzu6xNEnAULo+5cj4=;\n b=OC7Mm/BmKcsYh8tttdBhkxgTrUwWt+yxxdsx8Osnjq97jOCpRW9dsJp1vx5y7VVdhp\n 9jOoSqEkrqRc0wZ0fXMo86cwK+pX2nRCqfZKPlX0ZqfqOfezMk0tY/9x/TnCWPlxtFfM\n kSGKFn7rqDmSaoMGBACGmXmIMZcEg7rsSuCD0GNQO/z0GrBVv0asmNuTIec/sUXUkfUx\n kvTjj28Qow70PY8M9ryBjVo5oIwlE92uAhVxt3AFQcygi5bXPXo43fLquiMNcrIoxZQB\n JfQq5mF8hld1lIzFa7hAHk9gFZrYdcdBvT4iTeqmKt7L+dYOoRZvgmUZQcHCZXJbNZUa\n RbGA==","X-Gm-Message-State":"AJIora/KDXF7FZ+8d2UbEE/gmhfObIjzMG4VVeMRc2nK/TT9Zaxrq2xs\n 819j+ULuSlDiNWhosd5XNX4=","X-Google-Smtp-Source":"\n AGRyM1tjqWK/Ga0CmcTpFaI3vTdeTBUf256zFlYlxlZt3o9JxQF/7DgUjvJps7MYr9l7yjHiDaxshg==","X-Received":"by 2002:a2e:8897:0:b0:255:95f8:be0a with SMTP id\n k23-20020a2e8897000000b0025595f8be0amr5735176lji.303.1655497138625;\n Fri, 17 Jun 2022 13:18:58 -0700 (PDT)","Date":"Fri, 17 Jun 2022 23:18:55 +0300","From":"Serge Semin ","To":"Damien Le Moal ","Cc":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","Subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT\n child nodes number","Message-ID":"<20220617201855.cf64vbhe6wk4hrcu@mobilestation>","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-9-Sergey.Semin@baikalelectronics.ru>\n \n <20220615205328.chwruabvksdbnaex@mobilestation>\n <6d16fe23-012d-39fb-21e5-39ce50f7b03a@opensource.wdc.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<6d16fe23-012d-39fb-21e5-39ce50f7b03a@opensource.wdc.com>","X-Spam-Status":"No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,\n RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE\n autolearn=ham autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}},{"id":2915380,"web_url":"http://patchwork.ozlabs.org/comment/2915380/","msgid":"","list_archive_url":null,"date":"2022-06-18T06:49:08","subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT child\n nodes number","submitter":{"id":82259,"url":"http://patchwork.ozlabs.org/api/people/82259/?format=json","name":"Damien Le Moal","email":"damien.lemoal@opensource.wdc.com"},"content":"On 6/18/22 05:18, Serge Semin wrote:\n> On Thu, Jun 16, 2022 at 09:25:48AM +0900, Damien Le Moal wrote:\n>> On 2022/06/16 5:53, Serge Semin wrote:\n>>> On Tue, Jun 14, 2022 at 05:23:33PM +0900, Damien Le Moal wrote:\n>>>> On 6/10/22 17:17, Serge Semin wrote:\n>>>>> Having greater than AHCI_MAX_PORTS (32) ports detected isn't that critical\n>>>>> from the further AHCI-platform initialization point of view since\n>>>>> exceeding the ports upper limit will cause allocating more resources than\n>>>>> will be used afterwards. But detecting too many child DT-nodes doesn't\n>>>>> seem right since it's very unlikely to have it on an ordinary platform. In\n>>>>> accordance with the AHCI specification there can't be more than 32 ports\n>>>>> implemented at least due to having the CAP.NP field of 5 bits wide and the\n>>>>> PI register of dword size. Thus if such situation is found the DTB must\n>>>>> have been corrupted and the data read from it shouldn't be reliable. Let's\n>>>>> consider that as an erroneous situation and halt further resources\n>>>>> allocation.\n>>>>>\n>>>>> Note it's logically more correct to have the nports set only after the\n>>>>> initialization value is checked for being sane. So while at it let's make\n>>>>> sure nports is assigned with a correct value.\n>>>>>\n>>>>> Signed-off-by: Serge Semin \n>>>>> Reviewed-by: Hannes Reinecke \n>>>>>\n>>>>> ---\n>>>>>\n>>>>> Changelog v2:\n>>>>> - Drop the else word from the child_nodes value checking if-else-if\n>>>>> statement (@Damien) and convert the after-else part into the ternary\n>>>>> operator-based statement.\n>>>>>\n>>>>> Changelog v4:\n>>>>> - Fix some logical mistakes in the patch log. (@Sergei Shtylyov)\n>>>>> ---\n>>>>> drivers/ata/libahci_platform.c | 13 ++++++++++---\n>>>>> 1 file changed, 10 insertions(+), 3 deletions(-)\n>>>>>\n>>>>> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c\n>>>>> index 814804582d1d..8aed7b29c7ab 100644\n>>>>> --- a/drivers/ata/libahci_platform.c\n>>>>> +++ b/drivers/ata/libahci_platform.c\n>>>>> @@ -451,15 +451,22 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,\n>>>>> \t\t}\n>>>>> \t}\n>>>>> \n>>>>> -\thpriv->nports = child_nodes = of_get_child_count(dev->of_node);\n>>>>> +\t/*\n>>>>> +\t * Too many sub-nodes most likely means having something wrong with\n>>>>> +\t * the firmware.\n>>>>> +\t */\n>>>>> +\tchild_nodes = of_get_child_count(dev->of_node);\n>>>>> +\tif (child_nodes > AHCI_MAX_PORTS) {\n>>>>> +\t\trc = -EINVAL;\n>>>>> +\t\tgoto err_out;\n>>>>> +\t}\n>>>>> \n>>>>> \t/*\n>>>>> \t * If no sub-node was found, we still need to set nports to\n>>>>> \t * one in order to be able to use the\n>>>>> \t * ahci_platform_[en|dis]able_[phys|regulators] functions.\n>>>>> \t */\n>>>>> -\tif (!child_nodes)\n>>>>> -\t\thpriv->nports = 1;\n>>>>> +\thpriv->nports = child_nodes ?: 1;\n>>>>\n>>>\n>>>> This change is not necessary and makes the code far less easy to read.\n>>>\n>>> elaborate please. What change? What part of this change makes the code\n>>> less easy to read?\n>>\n> \n>> You changed:\n>>\n>> \tif (!child_nodes)\n>> \t\thpriv->nports = 1;\n>>\n>> to:\n>>\n>> \thpriv->nports = child_nodes ?: 1;\n>>\n>> That is the same. So the change is not needed in the first place, and worse,\n>> makes the code way harder to read for no good reason.\n> \n> No, they aren't the same:\n> +\tif (!child_nodes)\n> +\t\thpriv->nports = 1;\n> and\n> +\thpriv->nports = child_nodes ?: 1;\n> aren't equivalent. The equivalent implementation would be:\n> +\tif (child_nodes)\n> +\t\thpriv->nports = child_nodes;\n> +\telse\n> +\t\thpriv->nports = 1;\n\nThen use this code. That cryptic C code is hard to read.\n\n> \n> As I said in the patchlog, hpriv->nports is updated now only if\n> of_get_child_count() returns a valid number of the child nodes,\n> ports, which semantically is more correct. In the previous\n> implementation it was always set to the number of child nodes\n> no matter whether that value was correct or not.\n> \n> Regarding the ternary operator with omitted operand. Well, it's not\n> that rare beast in the kernel:\n> $ grep -r \"?:\" kernel/ drivers/ mm/ fs/ block/ | wc -l\n> 699\n> But if you insist in it being not that readable, I can replace it with\n> more bulky if-else statement. Do you?\n\nYes please, use the spelled out if/else. I prefer easy to read code rather\nthan loosing time trying to understand that cryptic C syntax, which I\nactually did not know about.\n\n> \n> -Sergey\n> \n>>\n>>>\n>>> -Sergey\n>>>\n>>>>\n>>>>> \n>>>>> \thpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);\n>>>>> \tif (!hpriv->phys) {\n>>>>\n>>>>\n>>>> -- \n>>>> Damien Le Moal\n>>>> Western Digital Research\n>>\n>>\n>> -- \n>> Damien Le Moal\n>> Western Digital Research","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["bilbo.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n unprotected) header.d=wdc.com header.i=@wdc.com header.a=rsa-sha256\n header.s=dkim.wdc.com header.b=mEXoN3ca;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=opensource.wdc.com header.i=@opensource.wdc.com\n header.a=rsa-sha256 header.s=dkim header.b=OpXI24xk;\n\tdkim-atps=neutral","ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2620:137:e000::1:20; helo=out1.vger.email;\n envelope-from=linux-ide-owner@vger.kernel.org; receiver=)","usg-ed-osssrv.wdc.com (amavisd-new); dkim=pass\n reason=\"pass (just generated, assumed good)\"\n header.d=opensource.wdc.com"],"Received":["from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20])\n\tby bilbo.ozlabs.org (Postfix) with ESMTP id 4LQ62j6mfdz9s1l\n\tfor ; Sat, 18 Jun 2022 16:49:17 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n id S232093AbiFRGtQ (ORCPT );\n Sat, 18 Jun 2022 02:49:16 -0400","from lindbergh.monkeyblade.net ([23.128.96.19]:54994 \"EHLO\n lindbergh.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n with ESMTP id S231425AbiFRGtP (ORCPT\n ); Sat, 18 Jun 2022 02:49:15 -0400","from esa3.hgst.iphmx.com (esa3.hgst.iphmx.com [216.71.153.141])\n by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DE443457A5\n for ; Fri, 17 Jun 2022 23:49:13 -0700 (PDT)","from uls-op-cesaip02.wdc.com (HELO uls-op-cesaep02.wdc.com)\n ([199.255.45.15])\n by ob1.hgst.iphmx.com with ESMTP; 18 Jun 2022 14:49:12 +0800","from uls-op-cesaip02.wdc.com ([10.248.3.37])\n by uls-op-cesaep02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 17 Jun 2022 23:07:24 -0700","from usg-ed-osssrv.wdc.com ([10.3.10.180])\n by uls-op-cesaip02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256;\n 17 Jun 2022 23:49:13 -0700","from usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTP id 4LQ62c2VbSz1Rwrw\n for ; Fri, 17 Jun 2022 23:49:12 -0700 (PDT)","from usg-ed-osssrv.wdc.com ([127.0.0.1])\n by usg-ed-osssrv.wdc.com (usg-ed-osssrv.wdc.com [127.0.0.1])\n (amavisd-new, port 10026)\n with ESMTP id GFLnBSVEOqM5 for ;\n Fri, 17 Jun 2022 23:49:11 -0700 (PDT)","from [10.225.163.84] (unknown [10.225.163.84])\n by usg-ed-osssrv.wdc.com (Postfix) with ESMTPSA id 4LQ62Y4Pwwz1Rvlc;\n Fri, 17 Jun 2022 23:49:09 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=simple/simple;\n d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com;\n t=1655534952; x=1687070952;\n h=message-id:date:mime-version:subject:to:cc:references:\n from:in-reply-to:content-transfer-encoding;\n bh=cJlFcpIzoeKzPz93xqBfimQuZjPgKMexkzSIElG5stA=;\n b=mEXoN3casbn87NcE7tbQDO8+Ul34MEl28sGQnKDzSExtV4fXAxBpLUg8\n iSfQd5yx+OAUxApMK0khwbPos9+LZwb0iZ9Whak8R4FjZrGUV1t38Hch6\n bmEUuSVz/MrZurOcHiO5+sABIsXDLhv3HkqaxMmVLIaFxKLIABRX8HOge\n pnblavXWw/VyiQ67sz6nZxchhvLiuHZjkL8VNck2ulCVwQXcjRwONSjVk\n 4g+o+xC04lnKNe+g82o3UkiPs/AGmRAVVZ5xv4EgKgQu19dJxSvJZSQmG\n x4QeExdlqCI885b6zgRtyoSljtdLyJkBcb24KyU5r6HVpwT4ZvaNwAlEv\n g==;","v=1; a=rsa-sha256; c=relaxed/simple; d=\n opensource.wdc.com; h=content-transfer-encoding:content-type\n :in-reply-to:organization:from:references:to:content-language\n :subject:user-agent:mime-version:date:message-id; s=dkim; t=\n 1655534951; x=1658126952; bh=cJlFcpIzoeKzPz93xqBfimQuZjPgKMexkzS\n IElG5stA=; b=OpXI24xkSEAwVND8bZRwnvkumDUdfGisI+1cKjvdhxiDVUM6Ad3\n BWfpyrbG+drUIN0M7XqAzeifo2nQNuOeo2FWmYCFdqIB9ZTo1OEGWH/m+brrVPxC\n YMAWcLkOiCs37xNte8Pb1Ocyq0UiPnKPA1OcvbvOP4y9Qg0LxJXQOP5sx2QqZkxL\n YrfkdkAC/6tA94bheRzPC9sT6UgoaG8nuVlcuiwuPvU06dEki90yGnX5U2E2DXJm\n CDMnIF3ASiBlQxgfWOUJwhIrUasektvh7FPwMpFp0/UB9uPKo9jo6XV1Teo8FpIr\n MxPK9qvSU8VCs3UeqZKCFw/1eHIyq+ZumqQ=="],"X-IronPort-AV":"E=Sophos;i=\"5.92,306,1650902400\";\n d=\"scan'208\";a=\"208360276\"","IronPort-SDR":["\n jDJf22DtEXJqCkz2y4eOQ8MbY0cyJqLd5JWnvXkIFYmbV1P+b7N/BEVGEOpCfvLFgRvRM6Ghb6\n JGcOgg7lVT9YaxrJARRTyTga5WkGNUdCNr/4VA4lSMMKXGfsCn2P6OKTsamt0NsVgj8wPJyIRu\n 2RZk5wc6ubKv8k+JYuiPip8oF9XkXCC4ZV3kPfRJXKJDyffD+F9niax+avSyBEZsQiXhUh70c5\n dB0GoGdSm5Yuqow4nIAqmplFHQW/b3bfo/cSf5b3ezvObaQpsgSqMNfrdNAVihSDZig2FowmN6\n 3BQrphmfFuKK3s6fowfYUXTd","\n y28Z+zrgUkjmjVMe0AxsPvXDC7RddjgeBGq/GI56iD/0IS1Ax9dJr82Nv7dtzl0UbpQCAaXjgg\n 2i/C8WKWtlM1AOZpmmooZcsLn98ORgzQ6HsK5btAednT3tJkzWssskg2a406kzV+yZQ7sq96Sq\n uYNbTy51vBrPyfQkQ4o+dPQWnzTZT5Cn7nRh0CifzCiDCYXvRlU9ftDSJhAswywxQr26PhemBE\n 3VHPij7Ejf8gVL58fXS9piMRNm/HACqjslJN9OjVqHvYyxyLmACsi/jLY5JWwKwuUnOVOOSoSa\n S9s="],"WDCIronportException":"Internal","X-Virus-Scanned":"amavisd-new at usg-ed-osssrv.wdc.com","Message-ID":"","Date":"Sat, 18 Jun 2022 15:49:08 +0900","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n Thunderbird/91.10.0","Subject":"Re: [PATCH v4 08/23] ata: libahci_platform: Sanity check the DT child\n nodes number","Content-Language":"en-US","To":"Serge Semin ","Cc":"Serge Semin ,\n Hans de Goede ,\n Jens Axboe , Hannes Reinecke ,\n Alexey Malahov ,\n Pavel Parkhomenko ,\n Rob Herring , linux-ide@vger.kernel.org,\n linux-kernel@vger.kernel.org, devicetree@vger.kernel.org","References":"<20220610081801.11854-1-Sergey.Semin@baikalelectronics.ru>\n <20220610081801.11854-9-Sergey.Semin@baikalelectronics.ru>\n \n <20220615205328.chwruabvksdbnaex@mobilestation>\n <6d16fe23-012d-39fb-21e5-39ce50f7b03a@opensource.wdc.com>\n <20220617201855.cf64vbhe6wk4hrcu@mobilestation>","From":"Damien Le Moal ","Organization":"Western Digital Research","In-Reply-To":"<20220617201855.cf64vbhe6wk4hrcu@mobilestation>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-Spam-Status":"No, score=-6.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,\n DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,\n SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham\n autolearn_force=no version=3.4.6","X-Spam-Checker-Version":"SpamAssassin 3.4.6 (2021-04-09) on\n lindbergh.monkeyblade.net","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ide@vger.kernel.org"}}]