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 X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 60663C433B4 for ; Wed, 14 Apr 2021 18:51:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 44879611AD for ; Wed, 14 Apr 2021 18:51:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230383AbhDNSvY (ORCPT ); Wed, 14 Apr 2021 14:51:24 -0400 Received: from mga17.intel.com ([192.55.52.151]:4142 "EHLO mga17.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229726AbhDNSvY (ORCPT ); Wed, 14 Apr 2021 14:51:24 -0400 IronPort-SDR: F+nQzQcD4ibIUb8q0AQ5clb36Ve8rL6WxO/uxe0HmTASRhJNQ3CjyI3zsCJVAq4lQYD/QqU4UR p7g/1M19lJYQ== X-IronPort-AV: E=McAfee;i="6200,9189,9954"; a="174814928" X-IronPort-AV: E=Sophos;i="5.82,223,1613462400"; d="scan'208";a="174814928" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Apr 2021 11:51:02 -0700 IronPort-SDR: N/Z7uWTWG25GI+a+l6wf/ZDt4RIRT12XOx7/5znwZdooN2gaSxlnO5PLHeSXra9NOy4vGOf4XQ 6F6+UgCMYXTQ== X-IronPort-AV: E=Sophos;i="5.82,223,1613462400"; d="scan'208";a="418434379" Received: from ljiang2-mobl3.amr.corp.intel.com (HELO intel.com) ([10.252.134.220]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Apr 2021 11:51:00 -0700 Date: Wed, 14 Apr 2021 11:50:58 -0700 From: Ben Widawsky To: Konrad Rzeszutek Wilk Cc: Jonathan Cameron , linux-cxl@vger.kernel.org, linux-pci@vger.kernel.org, Dan Williams , Bjorn Helgaas , Lorenzo Pieralisi , Chris Browy , linux-acpi@vger.kernel.org, alison.schofield@intel.com, vishal.l.verma@intel.com, ira.weiny@intel.com, linuxarm@huawei.com, Fangjian Subject: Re: [RFC PATCH v2 3/4] cxl/mem: Add CDAT table reading from DOE Message-ID: <20210414185058.ls5sbp3cow7synqe@intel.com> References: <20210413160159.935663-1-Jonathan.Cameron@huawei.com> <20210413160159.935663-4-Jonathan.Cameron@huawei.com> 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-pci@vger.kernel.org On 21-04-14 14:14:34, Konrad Rzeszutek Wilk wrote: > > +static int cdat_to_buffer(struct pcie_doe *doe, u32 *buffer, size_t length) > > +{ > > + int entry_handle = 0; > > + int rc; > > + > > + do { > > + u32 cdat_request_pl = CDAT_DOE_REQ(entry_handle); > > + u32 cdat_response_pl[32]; > > + struct pcie_doe_exchange ex = { > > + .vid = PCI_DVSEC_VENDOR_ID_CXL, > > + .protocol = CXL_DOE_PROTOCOL_TABLE_ACCESS, > > + .request_pl = &cdat_request_pl, > > + .request_pl_sz = sizeof(cdat_request_pl), > > + .response_pl = cdat_response_pl, > > + .response_pl_sz = sizeof(cdat_response_pl), > > + }; > > + size_t entry_dw; > > + u32 *entry; > > + > > + rc = pcie_doe_sync(doe, &ex); > > + if (rc < 0) > > + return rc; > > + > > + entry = cdat_response_pl + 1; > > + entry_dw = rc / sizeof(u32); > > Could you >> 2? What's the reasoning for >> 2? As someone who regularly does this because of habit, using divide is generally a lot more implicitly descriptive. > > + /* Skip Header */ > > + entry_dw -= 1; > > + entry_dw = min(length / 4, entry_dw); > > Ditto here on length? > > Also should you double check that length is not greater than > sizeof(cdat_respone_pl)? > > > + memcpy(buffer, entry, entry_dw * sizeof(u32)); > > + length -= entry_dw * sizeof(u32); > > Would it be just easier to convert length at the start to be >> 2 > instead of this * and / ? > Same as above. Assuming the compiler isn't braindead, I think it reads better as it is. > > > + buffer += entry_dw; > > + entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, cdat_response_pl[0]); > > + > > + } while (entry_handle != 0xFFFF); > > + > > + return 0; > > +} > > +