Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <99fab1eb-1092-4b7d-8b60-b03374883302@cornelisnetworks.com>
Date:   Fri, 29 Sep 2023 08:03:04 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 04/10] RDMA/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: <20230929115723.7864-1-ilpo.jarvinen@linux.intel.com>
 <20230929115723.7864-5-ilpo.jarvinen@linux.intel.com>
Content-Language: en-US
From:   Dean Luick <dean.luick@cornelisnetworks.com>
In-Reply-To: <20230929115723.7864-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?N1E3WmVPZHBYMnpSNGQvZjJMRTBjSW9FdnBwYStBbi9Oa3dnbUttV1VZcUZ1?=
 =?utf-8?B?TDhRQktxakkxSzZWWitFMmlldW9EcUx1N0FoZHVnaWVKbHNTQkczcHdSZXZG?=
 =?utf-8?B?dE9keXJrQ0d1QnE1ZFdLREI3cWxLVDVtRVIrN05DTVVvWi9aZlphM1pOTmll?=
 =?utf-8?B?cVpKYURqYUVtMEtqelBYcy8vNExOblNhU2V2eDBvTmp5MU1Ja2VFbC84YzAr?=
 =?utf-8?B?TUZBSGw1RDVUS0xZQm5ZSXF3ZzBlbTMzYzVRLzRrWE1PYnRyWGJxNTBBRzh3?=
 =?utf-8?B?VGZBQ21wYmNaeEdGOTl1NE5sSHNMcGhSc0NYemxDNG1DVGYvcHRkc1R6VUFN?=
 =?utf-8?B?Ukxmelh2K2U1SWp1UktNTXllbnNJOEJodVFVSnZhc3AzeTVBMVFjcUpBNnNC?=
 =?utf-8?B?N25tV0JUZDhScVN0L1VtREJ6Wmx5S21LbnNMOVlPUk5za0RxOG8rWXZzaURX?=
 =?utf-8?B?d2NhMkFxSEZHSWh6UGR4cy9EN2JxdVBUMzRqMGViMXNFT25vbUR1NmVva2o4?=
 =?utf-8?B?OVByZnF4SWZrUkdBMjQ4MEFPZFNkRkNITi9uZGcwSHF5RFQxMU4ybXY5Nmdw?=
 =?utf-8?B?bHJick5oUldyTGtHbHliUEJzWHVKNFVNTmNmMmZyYnUycTJtbTdvRGllZUhY?=
 =?utf-8?B?SldPaHFLeGZtV0dzaTBxNURHWGx4R2lVb09JSjdZRXgwZW5QRGZjb1ljMlUy?=
 =?utf-8?B?amF0ajNMT2lKbVJXS2FtOEdaQ1VnNUo5REF4MWxwQVZrTDRoa3RvU05IOW01?=
 =?utf-8?B?dURSckNZaVdDcC9tMDZmMmkvTWtBNWw4MHBEemQxeElIM1JrNVE1bUxBVDlZ?=
 =?utf-8?B?Vzk2ZmQ4TzdCRVFkU1BYTkM3Uk9RSDh2dmlEWDA0RHBEMGpvbXk3bHpxWnov?=
 =?utf-8?B?ZStZMzk1bHdmRGpFUjA4cmJNNVFoOTd3NGRvdnEyZSs4d2pydUgyUHBjZVd5?=
 =?utf-8?B?MjZmRWZkUVo4QWhkeXVibm4xaVp6ZHBVQnYwcVNCZDcwRWtGR3o4UFVJK1Rr?=
 =?utf-8?B?eVQ2MzNvVTluNStXZmFKenVRWERMVy85YUY2ZW1BenFYbHRnMWtRc1p2YmxW?=
 =?utf-8?B?cll1UjAvVDBBd09lOUxPcHhNM1prdXh4Uk5FenJ6OE0waS9VWTJxTlVidU14?=
 =?utf-8?B?VkVMa2g1YTJqVkREbG9Bak4rUlM0TCsrSG1TOVBrUVdDWVJCNTJkY0RIdjdl?=
 =?utf-8?B?S1FjNzYwRmlIRzBBeVJoaEh3RDc5eEp2dVhjUlFqZ1FqNGp6U3pQU2FFYlBG?=
 =?utf-8?B?MkpYMkp2Z2ZmQVZwZ1FDUWsrc3psenF3alIvbVRxcGhZNkN6bzFIMlI0Z21M?=
 =?utf-8?B?RHA2dWw0YVNheVdzVm9jZ2Q5RURWbFpacDlLME1SbVVGV3lXZmliOWhad0h4?=
 =?utf-8?B?YWdCVnF5MlozTEU1OEdhc041TUNwY3BGNVRqTnFSZXpsejRwV1B5ZjZJeXZJ?=
 =?utf-8?B?bkh3L2dsWU1EalFlclhleU1qM0FjU1VyZHUxN1BwL0Z1MTNJU2cvNTVlSXV1?=
 =?utf-8?B?NnZUSnYwNDB0eEZCNEgray9VUnhsYU9jeDJJekJNYXhiVUpBbFlnZ25hLy9J?=
 =?utf-8?B?K0RDZlVxakhQRUpEZmVQdXZRRDRTOGdHYkZUV29FbUgzaGRUYk9WZDVaWHh5?=
 =?utf-8?B?RSswNTdpTWVBWGxKSElNN0xnM3B6ZkxVZlNLcDNvMC9DZ1NOZmhCamR6Vi9W?=
 =?utf-8?B?ZFlwRS8yZExNeDdkZGtkZUNVWVBQRXNtZDVLNjJSZDk5WGVRdTMxdGxwVUsv?=
 =?utf-8?B?V0Uzb1ZSMnlpY0NSdnlBYnFFbHhkTjhzRk1wY0kvbTY3M2pTRmJSWUZQWEt3?=
 =?utf-8?B?TlhBb1Nmb1dGK1BSaFRLNlh2QzIwMlVobDVuTGxXS2htRzFVNnBsRTA4TUxP?=
 =?utf-8?B?RjE4NklvekNoU2EvTlg2VDArU2ZOaE1hNGxyNFY3MUVrazVib29oYlVrUWgz?=
 =?utf-8?B?SmRxOGpXYk9vVjIraUlSV3RzQWdRNm9VbnF3SjUzQXloQ0NpR3Y1QjIrSEFs?=
 =?utf-8?B?ekRYZmlJL053Nk1KS3JuVWtEOXY4b1lUZ29zbWd5OEZoUnQydklZVTM4SXNW?=
 =?utf-8?B?WDUrSWtLNENEUi9jUlUySE5wTElNRlcvMzNDYmtuV1owZHArZEgwRVpyUGFJ?=
 =?utf-8?B?WXE5d1BaYUVSeDR2QmRRVlh6SHNwTkw3eEtHOHRzK29lcUhJL1lDblFTVWIz?=
 =?utf-8?B?MHc9PQ==?=
X-OriginatorOrg: cornelisnetworks.com
X-MS-Exchange-CrossTenant-Network-Message-Id: adcd1129-b068-446e-c475-08dbc0ec6d60
X-MS-Exchange-CrossTenant-AuthSource: BL0PR01MB4131.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2023 13:03:08.9846
 (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: rnjf3sbwG06nlqV6S9p+B0kFglCq3PNWwejdxoQzKfbC+eJ9YiXYPfTyDqEcg4rep3/UYHH8mZgAle7I6C3aOs/QZHOKo9wCYhm08ALKS54=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR01MB8476
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
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 (howler.vger.email [0.0.0.0]); Fri, 29 Sep 2023 06:03:30 -0700 (PDT)

On 9/29/2023 6:57 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>
> ---
>  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..4487a05bea04 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 parent PCI target =
speed\n");
>                       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 device PCI target speed\n"=
);
>               return_error =3D 1;
>               goto done;
>       }

Looks good.  Thank you for my suggested changes.

Reviewed-by: Dean Luick <dean.luick@cornelisnetworks.com>

-Dean

External recipient