Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <c0e07dbf-e19e-43b7-9c95-8025a12323b8@cornelisnetworks.com>
Date:   Fri, 15 Sep 2023 08:51:01 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 04/10] drm/IB/hfi1: Use RMW accessors for changing
 LNKCTL2
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        linux-pci@vger.kernel.org, Bjorn Helgaas <helgaas@kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Rob Herring <robh@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Lukas Wunner <lukas@wunner.de>,
        Alexandru Gagniuc <mr.nuke.me@gmail.com>,
        Krishna chaitanya chundru <quic_krichai@quicinc.com>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        "Rafael J . Wysocki" <rafael@kernel.org>, linux-pm@vger.kernel.org,
        Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Leon Romanovsky <leon@kernel.org>, linux-rdma@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     Alex Deucher <alexdeucher@gmail.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>
References: <20230915120142.32987-1-ilpo.jarvinen@linux.intel.com>
 <20230915120142.32987-5-ilpo.jarvinen@linux.intel.com>
Content-Language: en-US
From:   Dean Luick <dean.luick@cornelisnetworks.com>
In-Reply-To: <20230915120142.32987-5-ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WjRVQnVNcUpNRGhuVGRhRnptbTFiaEhIVTZING1LZzhrMDIyRzNiR2tnRVkw?=
 =?utf-8?B?TEgwbmI2K3R1dkgrTkRzZUptRDZKa3VOanVkR0VkV0ZYdS9VWUJyQlM0TWpI?=
 =?utf-8?B?UXMwbU1hdU5ycFdaS2dpNXVXVGM1OFBmWTAzL3dHclEvOTZPNGZyRDBWWkR5?=
 =?utf-8?B?UW1SV3NrUkJySmdkNnNKWDJpb01hbjJwcTRnMjRqdWNIUFZOYmFtL1p1a3JG?=
 =?utf-8?B?R1ExWjJ3dnFvN1Q1eUt2cDZwenRHRkRWb0xlRHlTMzNscmFqenRpb0drQVBt?=
 =?utf-8?B?KzZuODVDT1BGbmpqNVRmV09UaU45ZVJMQTh2eUU5cDltSDNaUlpGaWdJTDZj?=
 =?utf-8?B?S0xiYnZqVmJhQmRjbGk0T2tNdEVVd3QvajdmZDcydUR4Y0UrSVhLQUVBVGRE?=
 =?utf-8?B?NVZqSGlhZGgvcmFZblI2ZG1xMUNkYzRiRWZuZkZHNld3UjNKUHB1dXdIenQ0?=
 =?utf-8?B?TnJIbnVhMlJNbC9LbXphYzF5RjRiVjdhZTJSMjEzSXhrVGU1a05Wa3ZSZk5L?=
 =?utf-8?B?aWpnZkk0cWJVeTI2MHUzK25FcHhIR0V1QzRFeFpaK2xqN2M2MG04SnZIVVBK?=
 =?utf-8?B?YXhYajA5eStKTUc4MW1pK0RBQzBOMWsyYm9oYktBSTMzejVIUy9ZRTJyUDMw?=
 =?utf-8?B?anJNa0sxWHhOekFtRUJoQXI4SmI3NDBvN09Sd2N3MXEzb0FYUlNYTmNrbnF4?=
 =?utf-8?B?ZURhMlBqejltbDI4TURPOTdhSFk3TGhwM0tEVFlIMGtjMmd5YVM5b0c1cnBU?=
 =?utf-8?B?UTB0bmxpTjgvbGxEdzY0K3lXTVVyQksxdlNKUlZMNHRYdnFrNENUQkl6MWV0?=
 =?utf-8?B?MnE0VXNtemswd2JUUkYxS1kvL0tzS0pzTnRrNWRxNExWclFKZE5VVG50NThR?=
 =?utf-8?B?MnJuTzZ1R1BnNVdsd0tDaS9CUFIwNmFyc1dJaHpicHprbUY2eFdhQUFrQzdJ?=
 =?utf-8?B?T2Fhc1FtWDFKMGlCbzh1cG1HQWMyaDgwSkdINXNCLzRTdHIxSDllS1MrdDZq?=
 =?utf-8?B?V3l6MTV5bDRYdnBuSnlLWDFsNmpQMlV5OWV6WE11V0JCalRacU4vbmVUSHRY?=
 =?utf-8?B?b0dlOGNHbFRUL1dWZU5TWTljSU5ibTY0VEJiMnQ2WVFyUGhPcFFEclArcXNM?=
 =?utf-8?B?TVVLWWxnNlFVNCtTK09TMjFTU3BwcmJmK05tbmZOeFVOS0EzY0NxY1VJTmo0?=
 =?utf-8?B?UjhsRHZrRG92Z1krU0VOVUd5RDU2R3RleE9sRGkraWVyWWZGYVlUaE96N01p?=
 =?utf-8?B?MEVFaXgyYVAvS0lHVktMK3E4YmtGbWVCdDExM1FBVCt3R1pwTXJFa2NmY3hV?=
 =?utf-8?B?Y2tRMzgxM2lKL3ZJRVU0MHlFZFBqTXpCMGI0NHpWeVFaZmpRVW5qQnVndWVG?=
 =?utf-8?B?VEh1R3hPSGlTYUxoeVFuUk90ZldIdVJ2dmdzRWYwYjU0U0NtTytqWEpYWEhS?=
 =?utf-8?B?SnJ6TXN1cGxVN0ljWGljMCtpZGxQVll1QVlBVW1sRE9WSGNwUHZqQU5Kcjlk?=
 =?utf-8?B?MXVUM1IwQUNqa0M0M0FxNlNwRk04WWFOcnBBV1diUlFZcHlIVDF4SDFtemI1?=
 =?utf-8?B?aFl3bHFFZlh6cUxUajlGYVV0aXB3NnRIOE53cTFEbHNrak9uVHE4YyszZDEz?=
 =?utf-8?B?YmN6YnRESEVJMk9MZUN4d2FNdE1lRE8rWVRESXlaeDhBZ0FNUmg4VjFRdDdT?=
 =?utf-8?B?Z0FjWk9FV3IyMFNjUkhHVytlY0x0N0FpU2ozRGg2RjNMWFVaYXpFSzVwSlpm?=
 =?utf-8?B?T0hsQkE5amlBeWppMks1NW9PSUs2K0E5L2JWNzdnVEVzdVFRTVNBOW5VdW1L?=
 =?utf-8?B?ZUFuQ2xKcFhrSDJYNGoxbS9QMFUxbUlJNjlMOEI3YXc1SE9LTXMyTytCZTY0?=
 =?utf-8?B?UnFsRExzblN5OXhTRks2NHg1UTUrSnRHVFladllBc0U2eTVIeVZZR2N5eFJC?=
 =?utf-8?B?YklQTkdWdmw0QXZTMFMwYjJ2MU9MWVhDdjc3N3duZlpnQ3ZSdEZLRUJDK24v?=
 =?utf-8?B?NzZzcXMxWG9ZTFRLSy9Nek1aYUExbEVxVTZmNWhMRUFPWmlZMG5sWDV5emRJ?=
 =?utf-8?B?Q3d5d2VUQjJLSGZ0d1VsOGFYOEpuYTBGZ1JWS2g2dUZvc0NkV3dxYnd0TFZp?=
 =?utf-8?B?UnRVeS9sZnArcUN2bUxOTjM5Smt6eWRKMmlYQ2V2NFFEWUxTbThldGl3elRt?=
 =?utf-8?B?UXc9PQ==?=
X-OriginatorOrg: cornelisnetworks.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0bb9da8d-81de-4de8-2294-08dbb5f2ce6f
X-MS-Exchange-CrossTenant-AuthSource: BL0PR01MB4131.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Sep 2023 13:51:05.6634
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4dbdb7da-74ee-4b45-8747-ef5ce5ebe68a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +MkS+czzN0LgqM4A+peT3QNcD5PhTeqqu1+Afq/2qBqtAY86ZH0zgEHzaWCTGK5rixqRrQyHLSpp0znOdWfHD5ddX3I4v1sP/in2eAyaeLQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR01MB7479
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Fri, 15 Sep 2023 06:51:31 -0700 (PDT)

On 9/15/2023 7:01 AM, Ilpo J=C3=A4rvinen wrote:
> Don't assume that only the driver would be accessing LNKCTL2. In the
> case of upstream (parent), the driver does not even own the device it's
> changing the registers for.
>
> Use RMW capability accessors which do proper locking to avoid losing
> concurrent updates to the register value. This change is also useful as
> a cleanup.
>
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com>

I believe the subject should begin with "RDMA/hfi1:", the current expectati=
on for all devices in drivers/infiniband.

> ---
>  drivers/infiniband/hw/hfi1/pcie.c | 30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/infiniband/hw/hfi1/pcie.c b/drivers/infiniband/hw/hf=
i1/pcie.c
> index 08732e1ac966..60a177f52eb5 100644
> --- a/drivers/infiniband/hw/hfi1/pcie.c
> +++ b/drivers/infiniband/hw/hfi1/pcie.c
> @@ -1212,14 +1212,11 @@ int do_pcie_gen3_transition(struct hfi1_devdata *=
dd)
>                   (u32)lnkctl2);
>       /* only write to parent if target is not as high as ours */
>       if ((lnkctl2 & PCI_EXP_LNKCTL2_TLS) < target_vector) {
> -             lnkctl2 &=3D ~PCI_EXP_LNKCTL2_TLS;
> -             lnkctl2 |=3D target_vector;
> -             dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__=
,
> -                         (u32)lnkctl2);
> -             ret =3D pcie_capability_write_word(parent,
> -                                              PCI_EXP_LNKCTL2, lnkctl2);
> +             ret =3D pcie_capability_clear_and_set_word(parent, PCI_EXP_=
LNKCTL2,
> +                                                      PCI_EXP_LNKCTL2_TL=
S,
> +                                                      target_vector);
>               if (ret) {
> -                     dd_dev_err(dd, "Unable to write to PCI config\n");
> +                     dd_dev_err(dd, "Unable to change PCI target speed\n=
");

To differentiate from the dev_err below, add "parent", i.e. "Unable to chan=
ge parent PCI target speed".


>                       return_error =3D 1;
>                       goto done;
>               }
> @@ -1228,22 +1225,11 @@ int do_pcie_gen3_transition(struct hfi1_devdata *=
dd)
>       }
>
>       dd_dev_info(dd, "%s: setting target link speed\n", __func__);
> -     ret =3D pcie_capability_read_word(dd->pcidev, PCI_EXP_LNKCTL2, &lnk=
ctl2);
> +     ret =3D pcie_capability_clear_and_set_word(dd->pcidev, PCI_EXP_LNKC=
TL2,
> +                                              PCI_EXP_LNKCTL2_TLS,
> +                                              target_vector);
>       if (ret) {
> -             dd_dev_err(dd, "Unable to read from PCI config\n");
> -             return_error =3D 1;
> -             goto done;
> -     }
> -
> -     dd_dev_info(dd, "%s: ..old link control2: 0x%x\n", __func__,
> -                 (u32)lnkctl2);
> -     lnkctl2 &=3D ~PCI_EXP_LNKCTL2_TLS;
> -     lnkctl2 |=3D target_vector;
> -     dd_dev_info(dd, "%s: ..new link control2: 0x%x\n", __func__,
> -                 (u32)lnkctl2);
> -     ret =3D pcie_capability_write_word(dd->pcidev, PCI_EXP_LNKCTL2, lnk=
ctl2);
> -     if (ret) {
> -             dd_dev_err(dd, "Unable to write to PCI config\n");
> +             dd_dev_err(dd, "Unable to change PCI target speed\n");

To differentiate from the dev_err above, add "device", i.e. "Unable to chan=
ge device PCI target speed".


>               return_error =3D 1;
>               goto done;
>       }

External recipient