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 212D7C43219 for ; Wed, 12 Oct 2022 20:02:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229548AbiJLUCF (ORCPT ); Wed, 12 Oct 2022 16:02:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45930 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229516AbiJLUCC (ORCPT ); Wed, 12 Oct 2022 16:02:02 -0400 Received: from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com [IPv6:2a00:1450:4864:20::12a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E18B51C40D; Wed, 12 Oct 2022 13:01:58 -0700 (PDT) Received: by mail-lf1-x12a.google.com with SMTP id bp15so27312099lfb.13; Wed, 12 Oct 2022 13:01:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=79SDMbgsMFe0kAcWZ8337yYSlLiSbukp1ZkX77xJUZE=; b=A0DaAUXrSk8uSYalBXlUqYnuljoQRW/N20juqG5exBub1TqJK8oaXCMUGResoK5v29 kJi6wTIgYWUfXLifEnMXSJwP+/NALgb7zqaUZ4sYWHGJXnvKQGEweeiQ7qWO2G8ztYXs 3qYOrllH8Ys/HjnlZNgLP0tODfSa2lj8uAdEyRc/2lUQ/ZR+hmzu7hzK2bd07vw9k+81 f+vZnvq6oGle7Ini9kNKhD1VBFIGcUlXKUvQi+Z2VqERXYhwt4lqW8JCyH+RkzuLup+O a1rcgMsVroAHhIT7EN4nPJAnVHnOZ+DZazDCaTN+P5O6/h8mG59xCD1z4VTdUsgeAC1g 5sKQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=79SDMbgsMFe0kAcWZ8337yYSlLiSbukp1ZkX77xJUZE=; b=YzHJuc1S+pHLY/IhTzWVglyr/Ppf7TSDr6fca4AWcCZgKCGtUV0KUcBxXPUx8OJNZo VWACNO1UYrleYUJIHJtWIfTSph0zqX5BokcvFnadb/I9nAz5Fd1UURDi8+FDk6y89FDf M3oSUEAdHvbzCzgT92Pe6hrT73Br0hIlgPJ0jBxA6sXBC4V/x4lRKDB6ycK96UAFGI+w EDjSW0gJ/UD/C9Aqgzrcwy+KySWrZQCMS3+IymolioQEjRzPRR0JER+w3479UX/w2OOz M3yJbvBV7zPodi3OuvEVCtskMqMP9JOJk3MMiX60JdDwpSOH/DeaeVBTpEBcqAtI1NOu j7Rw== X-Gm-Message-State: ACrzQf2QDD33nsSObOLxrCo/n/yfcMY1lS1phM8/VpYBk4/YmAIvDjTr RbdbAmJHHbrfgt2keNsxRDc= X-Google-Smtp-Source: AMsMyM56lNbIdMP/1IFUiefmVanFBnoRLeLA5Dn4YMgxV2CKC8uoqx68T07GaMTuQrPiWJ+lTNlK2Q== X-Received: by 2002:ac2:4d46:0:b0:4a2:473f:1fb3 with SMTP id 6-20020ac24d46000000b004a2473f1fb3mr10297325lfp.408.1665604917094; Wed, 12 Oct 2022 13:01:57 -0700 (PDT) Received: from mobilestation ([95.79.133.202]) by smtp.gmail.com with ESMTPSA id e11-20020a05651236cb00b004a44ffb1050sm84837lfs.171.2022.10.12.13.01.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Oct 2022 13:01:56 -0700 (PDT) Date: Wed, 12 Oct 2022 23:01:54 +0300 From: Serge Semin To: Borislav Petkov Cc: Serge Semin , Michal Simek , Mauro Carvalho Chehab , Tony Luck , James Morse , Robert Richter , Alexey Malahov , Michail Ivanov , Pavel Parkhomenko , Punnaiah Choudary Kalluri , Manish Narani , Dinh Nguyen , Rob Herring , Krzysztof Kozlowski , Rob Herring , Krzysztof Kozlowski , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure Message-ID: <20221012200154.7fq3i7igbgkcy2mx@mobilestation> References: <20220929232712.12202-1-Sergey.Semin@baikalelectronics.ru> <20220929232712.12202-14-Sergey.Semin@baikalelectronics.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Oct 12, 2022 at 07:29:22PM +0200, Borislav Petkov wrote: > On Fri, Sep 30, 2022 at 02:27:08AM +0300, Serge Semin wrote: > > In case of the unique index allocation it's not that optimal to always > > rely on the low-level device drivers (platform drivers), because they get > > to start to implement either the same design pattern (for instance global > > static MC counter) or may end-up with having non-unique index eventually > > at runtime. Needless to say that having a generic unique index > > allocation/tracking procedure will make code more readable and safer. > > I guess this is trying to say that the current memory controller index > thing doesn't work. But why doesn't it work? >From what have you got this? I said that the current MC indexing approach wasn't that optimal (always relying on the low-level driver to allocate the index) because it caused having the same IDx allocation pattern re-implemented in the drivers. It can be avoided by the provided patch. The unified approach makes code indeed more readable in the platform drivers and safer since they didn't have to bother with more coding. See for instance the drivers with the static variable-based IDs allocation. It doesn't seem like these drivers bother with the detected DDR devices order. If so then the automatic IDs allocation is perfect for them. Note the static variable increment isn't atomic. Thus the ID allocation algorithm there is prone to races should the devices probe is run concurrently. > > It works just fine with the x86 drivers - there the memory controller > number is the same as the node number where it is located so that works > just fine. > > If that scheme cannot work on other systems, then I need to see an > explanation why it cannot work first. > > > The suggested implementation is based on the kernel IDA infrastructure > > exposed by the lib/idr.c driver with API described in linux/idr.h header > > file. It's used to create an ID resource descriptor "mc_idr", which then > > is utilized either to track the custom MC idx specified by EDAC LLDDs or > > to allocate the next-free MC idx. > > This is talking about the "what" and not the "why". > > > A new special MC index is introduced here. It's defined by the > > EDAC_AUTO_MC_NUM macro with a value specifically chosen as the least > > probable value used for the real MC index. In case if the EDAC_AUTO_MC_NUM > > index is specified by the EDAC LLDD, the MC index will be either retrieved > > from the MC device OF-node alias index ("mc[:number:]") or automatically > > generated as the next-free MC index found by the ID allocation procedure. > > This is also talking about the "what" and not the "why". > > At best, what you're doing should be visible from the patch itself. > > Here's a longer explanation of how a commit message should look like: > > https://kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes Have you read it yourself? Here is a short excerpt from there: "Once the problem is established, describe what you are actually doing about it in technical detail. It's important to describe the change in plain English for the reviewer to verify that the code is behaving as you intend it to." So the "problem" is described in the first paragraph and the technical details in the later paragraphs. -Sergey > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 E8E2AC4332F for ; Wed, 12 Oct 2022 20:03:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=cFJgqPpkCwVcqqxOxSME7RW+qf+kCWPAbZvadK8MAsc=; b=J87mUfbDg/t5gn 8ujfckjtNZR3DxdpZi7ASR9aURVVHEsjhO7oV6nRxK5AUkzscSKJk3kUjAS87NQraT7LB2ZwH2lea p7nTbGe2TAtv4shtm8LOsfyemVGpzyiUUOB7mjFjwdJ48CZQc8qotnHsK6SzPPCGuOGCukHDicRAl t7aSPg898ga3afJaI1yKWZYUVrc1rfmgw56MBAxFajbDUUrkCt2uveJz9gG36GE0LlMrSAr2MUius 6ZtcZ2d8XYRzrWY6BzX35vaiCC1zqEJ6ADEayOel4b2h8IGVguO+lcu1fqlcUTTmoOIC2+9c3lAV3 UP685Vq1bctW4+0ELekg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oihvY-009E1z-Mq; Wed, 12 Oct 2022 20:02:04 +0000 Received: from mail-lf1-x134.google.com ([2a00:1450:4864:20::134]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oihvV-009E0w-3G for linux-arm-kernel@lists.infradead.org; Wed, 12 Oct 2022 20:02:02 +0000 Received: by mail-lf1-x134.google.com with SMTP id b1so22334608lfs.7 for ; Wed, 12 Oct 2022 13:01:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=79SDMbgsMFe0kAcWZ8337yYSlLiSbukp1ZkX77xJUZE=; b=A0DaAUXrSk8uSYalBXlUqYnuljoQRW/N20juqG5exBub1TqJK8oaXCMUGResoK5v29 kJi6wTIgYWUfXLifEnMXSJwP+/NALgb7zqaUZ4sYWHGJXnvKQGEweeiQ7qWO2G8ztYXs 3qYOrllH8Ys/HjnlZNgLP0tODfSa2lj8uAdEyRc/2lUQ/ZR+hmzu7hzK2bd07vw9k+81 f+vZnvq6oGle7Ini9kNKhD1VBFIGcUlXKUvQi+Z2VqERXYhwt4lqW8JCyH+RkzuLup+O a1rcgMsVroAHhIT7EN4nPJAnVHnOZ+DZazDCaTN+P5O6/h8mG59xCD1z4VTdUsgeAC1g 5sKQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=79SDMbgsMFe0kAcWZ8337yYSlLiSbukp1ZkX77xJUZE=; b=dtospG0H2gI4JknoDD0xLxaMc+VdFI5d3+SBMAep1T2aVFxUZUCOBO2oCE8slRkWkH pxCoTeSPzKwJcxO2Q/y2rNKp8l+27s2svrnpP0H8ujHx2pi4HnWLj4Dez3oz72RdOLQp GrOHzMwg0sM1/6jJCR/ZABKzVQC8/4bQnLFJSsm7i1hImbkni2k+SmKTsXAV+MJOLcfN CbxItZ9Xx9PBe1CuCYQfwAojErzI1g0e5BrnBQb8pI9gznFkLEMt5PpbVZdU3z+NTV1J f2zsk7iDMfhDmq83rw0sAchC9q8+vbCTaR3umKlfwx3ihp5SBIcUenPFZriaW296wLgb PdZg== X-Gm-Message-State: ACrzQf3q/do1cN+O+Lx+/KWEcKg9y0M0+Kvn5HmzpCqJVW0Qqr4WKGgm lUxvb7jWW55jM+Lh5gXAjR0= X-Google-Smtp-Source: AMsMyM56lNbIdMP/1IFUiefmVanFBnoRLeLA5Dn4YMgxV2CKC8uoqx68T07GaMTuQrPiWJ+lTNlK2Q== X-Received: by 2002:ac2:4d46:0:b0:4a2:473f:1fb3 with SMTP id 6-20020ac24d46000000b004a2473f1fb3mr10297325lfp.408.1665604917094; Wed, 12 Oct 2022 13:01:57 -0700 (PDT) Received: from mobilestation ([95.79.133.202]) by smtp.gmail.com with ESMTPSA id e11-20020a05651236cb00b004a44ffb1050sm84837lfs.171.2022.10.12.13.01.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Oct 2022 13:01:56 -0700 (PDT) Date: Wed, 12 Oct 2022 23:01:54 +0300 From: Serge Semin To: Borislav Petkov Cc: Serge Semin , Michal Simek , Mauro Carvalho Chehab , Tony Luck , James Morse , Robert Richter , Alexey Malahov , Michail Ivanov , Pavel Parkhomenko , Punnaiah Choudary Kalluri , Manish Narani , Dinh Nguyen , Rob Herring , Krzysztof Kozlowski , Rob Herring , Krzysztof Kozlowski , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure Message-ID: <20221012200154.7fq3i7igbgkcy2mx@mobilestation> References: <20220929232712.12202-1-Sergey.Semin@baikalelectronics.ru> <20220929232712.12202-14-Sergey.Semin@baikalelectronics.ru> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221012_130201_186214_1D53FA36 X-CRM114-Status: GOOD ( 36.05 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Oct 12, 2022 at 07:29:22PM +0200, Borislav Petkov wrote: > On Fri, Sep 30, 2022 at 02:27:08AM +0300, Serge Semin wrote: > > In case of the unique index allocation it's not that optimal to always > > rely on the low-level device drivers (platform drivers), because they get > > to start to implement either the same design pattern (for instance global > > static MC counter) or may end-up with having non-unique index eventually > > at runtime. Needless to say that having a generic unique index > > allocation/tracking procedure will make code more readable and safer. > > I guess this is trying to say that the current memory controller index > thing doesn't work. But why doesn't it work? >From what have you got this? I said that the current MC indexing approach wasn't that optimal (always relying on the low-level driver to allocate the index) because it caused having the same IDx allocation pattern re-implemented in the drivers. It can be avoided by the provided patch. The unified approach makes code indeed more readable in the platform drivers and safer since they didn't have to bother with more coding. See for instance the drivers with the static variable-based IDs allocation. It doesn't seem like these drivers bother with the detected DDR devices order. If so then the automatic IDs allocation is perfect for them. Note the static variable increment isn't atomic. Thus the ID allocation algorithm there is prone to races should the devices probe is run concurrently. > > It works just fine with the x86 drivers - there the memory controller > number is the same as the node number where it is located so that works > just fine. > > If that scheme cannot work on other systems, then I need to see an > explanation why it cannot work first. > > > The suggested implementation is based on the kernel IDA infrastructure > > exposed by the lib/idr.c driver with API described in linux/idr.h header > > file. It's used to create an ID resource descriptor "mc_idr", which then > > is utilized either to track the custom MC idx specified by EDAC LLDDs or > > to allocate the next-free MC idx. > > This is talking about the "what" and not the "why". > > > A new special MC index is introduced here. It's defined by the > > EDAC_AUTO_MC_NUM macro with a value specifically chosen as the least > > probable value used for the real MC index. In case if the EDAC_AUTO_MC_NUM > > index is specified by the EDAC LLDD, the MC index will be either retrieved > > from the MC device OF-node alias index ("mc[:number:]") or automatically > > generated as the next-free MC index found by the ID allocation procedure. > > This is also talking about the "what" and not the "why". > > At best, what you're doing should be visible from the patch itself. > > Here's a longer explanation of how a commit message should look like: > > https://kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes Have you read it yourself? Here is a short excerpt from there: "Once the problem is established, describe what you are actually doing about it in technical detail. It's important to describe the change in plain English for the reviewer to verify that the code is behaving as you intend it to." So the "problem" is described in the first paragraph and the technical details in the later paragraphs. -Sergey > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel