Received-SPF: pass (google.com: domain of linux-kernel+bounces-68558-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 16 Feb 2024 13:10:14 +0100
From: Robert Richter <rrichter@amd.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Davidlohr Bueso <dave@stgolabs.net>, linux-cxl@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>, Lukas Wunner <lukas@wunner.de>,
	Fan Ni <nifan.cxl@gmail.com>
Subject: Re: [PATCH v3 2/3] cxl/pci: Get rid of pointer arithmetic reading
 CDAT table
Message-ID: <Zc9QpsNa2kNQsQsR@rric.localdomain>
References: <20240209192647.163042-1-rrichter@amd.com>
 <20240209192647.163042-3-rrichter@amd.com>
 <20240214173158.000005c0@Huawei.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240214173158.000005c0@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?ZEkLrThcQmYBhKKs9fX3CPaqpUmNCLZDeWNe1Jcc52SXabmGgOagYuGbmmKk?=
 =?us-ascii?Q?qEQCnlULzLRFR7Pc6HAJ0MT5TlAzPh4/cD2asdefDGPTgkBUU1M4A2MOtBY0?=
 =?us-ascii?Q?EWR/1HR8QCvuccjdx7MTFi0yWo81n4VLnF4d3PrVknSsCxlu2NLElqupxiLX?=
 =?us-ascii?Q?a5EWG6yQnnnKtF6EEuAM4PGldsZ8pgME4dHDB/E4jYe7qpdGXYUqbt2YbhO5?=
 =?us-ascii?Q?5/eUse1kJ2ZgcPqZkxAGVfthP51pelk/P/+rU7czT57ExGq128p/gPUtyaRn?=
 =?us-ascii?Q?zYmvGWJyOekL5d4ZkWEetCh9tUMJN9xuJVTAH9OAxryqlZHOmLmLbBUuDzeS?=
 =?us-ascii?Q?aCG1qex3MeanMxV/qRpoozSlczUMorSIZowl4rBrfqKCK3Xya71fI0i2sZXP?=
 =?us-ascii?Q?KKJXGz/2WSBPbCFpSvRqTheuUo8RiECxji3uSef3OCSJKz/XKJ2kws6ire9X?=
 =?us-ascii?Q?/rBphY39oEruQYnl7R1JNpk7nTaQGeR8WJMtBAN3JJXTIVWGRiKV7/rxufQk?=
 =?us-ascii?Q?6IsEKYNWmGXo3BtlVbvmvTNkxfaqL5tOHmYUFqw+UUYgV+k/dspCoxGGHgoT?=
 =?us-ascii?Q?C4uOlMx223ymx9DBLzphkqWtXB7KGnDKju5jBLGb2lGsod3PxttTXY/kayIM?=
 =?us-ascii?Q?J4/gVmCYCfQ1tA9nRuIayAoRhUEWyA0uZ04zyC+JgRhehAn89JzpQGzDEuSM?=
 =?us-ascii?Q?2DrTkPIt74lTPIhQ7FI2K6opUXElxjOVQQvA/CRFRCHKzr/Lubo00Y8tfBqd?=
 =?us-ascii?Q?pF03GIxDC48gESu2k17nOvhncv58nJrfbYcGHkT1dzxxWloBEW4UOwaO3K8w?=
 =?us-ascii?Q?mWKC5ayKHX+p5UDakRu3ZyPlv0OvdFb46huvdMinmXrwSUcY/3tiUMb9JubR?=
 =?us-ascii?Q?O2uHS7L/EXW3Dtv6t7i8iM8CnPGQOCtjURooelM3tkp0IFF/uzPhLX8XWh/K?=
 =?us-ascii?Q?b0spvWSGIzA2FAiDvN/wnttKdpUrnrDEa6LDhM90U+uhdeNaiclfjfN9IEDm?=
 =?us-ascii?Q?W+j+T/4Qb/Z1oVw4kxslGSNy1g2iwUUqQgEq1MUCWDAjMkTBGnAolR523Hn5?=
 =?us-ascii?Q?ZnRdZ6qlzpn43eUeCRSVJYSs34/E3W4n7loT+XTCufaLo75D62AWszu7gZaZ?=
 =?us-ascii?Q?C0+QRVwJhvU7pXtRbtDp6F5JXdsA5WXUMJt1ipvgA7wOac8uef2GzBH0v1uE?=
 =?us-ascii?Q?O5kZAP3261K5P3BCo5HkPDKBgEFsFTiag7XCEhMJzsst3A1sNyfJDmgvfy88?=
 =?us-ascii?Q?9Z6DOyk5EClMLWMb9OcSQiQYnQ6V+6uYeybQ8mfzNhih74M7hHUpNUfRBwCB?=
 =?us-ascii?Q?8KS/q85PK6mWwhll4AFGMnzi7D6FGcDqYv5V+RvjDCoEDj/RVH3AV6bEtYKo?=
 =?us-ascii?Q?0PyToZJpmSEtaCcCCpXlQGKVrwWw6Qnhd4qubzc17IJLghVYUXJg/fQvJWek?=
 =?us-ascii?Q?ZNg+mRq4TNLwZ7Ac2bAdrf873t/3MaZNyvN2PFcCkb0UdbrKDYsv4ebSeKRd?=
 =?us-ascii?Q?1ULkTJY3C018/paqg0f7rGCoQlpTqIxak/g/+Ty/TQp0DPVZNjnpviXqT9FM?=
 =?us-ascii?Q?Vl2MtMG8beMt86K87IR28qGAwRfCQ/tNnPr/AkF2?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 48613418-08c0-477b-d23f-08dc2ee83edd
X-MS-Exchange-CrossTenant-AuthSource: CYYPR12MB8750.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Feb 2024 12:10:20.4296
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vXja+0wzQKXa8sdHIKDjo3dwsbdHdUL5WjcJxtq/psJi8kQV48qJJmcZCUtpVORXLZQ2GbCOODykb3Z0Hvl+pw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR12MB8280

Hi Jonathan,

thanks for your review.

On 14.02.24 17:31:58, Jonathan Cameron wrote:
> On Fri, 9 Feb 2024 20:26:46 +0100
> Robert Richter <rrichter@amd.com> wrote:
> 
> > Reading the CDAT table using DOE requires a Table Access Response
> > Header in addition to the CDAT entry. In current implementation this
> > has caused offsets with sizeof(__le32) to the actual buffers. This led
> > to hardly readable code and even bugs. E.g., see fix of devm_kfree()
> > in read_cdat_data():
> > 
> >  c65efe3685f5 cxl/cdat: Free correct buffer on checksum error
> > 
> > Rework code to avoid calculations with sizeof(__le32). Introduce
> > struct cdat_doe_rsp for this which contains the Table Access Response
> > Header and a variable payload size for various data structures
> > afterwards to access the CDAT table and its CDAT Data Structures
> > without recalculating buffer offsets.
> > 
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Fan Ni <nifan.cxl@gmail.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> 
> Hi Robert,
> 
> I like this in general.  A few comments inline though.
> 
> > ---
> >  drivers/cxl/core/pci.c | 75 ++++++++++++++++++++++--------------------
> >  drivers/cxl/cxlpci.h   | 20 +++++++++++
> >  2 files changed, 59 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 39366ce94985..569354a5536f 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -544,55 +544,55 @@ static int cxl_cdat_get_length(struct device *dev,
> >  
> >  static int cxl_cdat_read_table(struct device *dev,
> >  			       struct pci_doe_mb *doe_mb,
> > -			       void *cdat_table, size_t *cdat_length)
> > +			       struct cdat_doe_rsp *rsp, size_t *length)
> 
> Nitpick, but rsp isn't a response, it's the whole table.
> Maybe it's worth a 
> #define cdat_doe_table cdat_doe_rsp
> or a typedef so the two are different in name at least whilst sharing
> same structure definition?

There is a comment near the kzalloc of buf. I think introducing
another type here for single use will just add confusion.

I will also update the description of cdat_doe_rsp.

> 
> >  {
> > -	size_t length = *cdat_length + sizeof(__le32);
> > -	__le32 *data = cdat_table;
> > -	int entry_handle = 0;
> > +	size_t received, remaining = *length;
> > +	unsigned int entry_handle = 0;
> > +	union cdat_data *data;
> >  	__le32 saved_dw = 0;
> >  
> >  	do {
> >  		__le32 request = CDAT_DOE_REQ(entry_handle);
> > -		struct cdat_entry_header *entry;
> > -		size_t entry_dw;
> >  		int rc;
> >  
> >  		rc = pci_doe(doe_mb, PCI_DVSEC_VENDOR_ID_CXL,
> >  			     CXL_DOE_PROTOCOL_TABLE_ACCESS,
> >  			     &request, sizeof(request),
> > -			     data, length);
> > +			     rsp, sizeof(*rsp) + remaining);
> 
> I guess it's not really worth using struct_size here.
> It's main advantage is making it clear we are dealing with a
> trailing [] 

Yes, will keep it as is. Since it's a u8 array, count is equal the
size for the remaining data and we do not need struct_size() here.

> 
> >  		if (rc < 0) {
> >  			dev_err(dev, "DOE failed: %d", rc);
> >  			return rc;
> >  		}
> >  
> > -		/* 1 DW Table Access Response Header + CDAT entry */
> > -		entry = (struct cdat_entry_header *)(data + 1);
> > -		if ((entry_handle == 0 &&
> > -		     rc != sizeof(__le32) + sizeof(struct cdat_header)) ||
> > -		    (entry_handle > 0 &&
> > -		     (rc < sizeof(__le32) + sizeof(*entry) ||
> > -		      rc != sizeof(__le32) + le16_to_cpu(entry->length))))
> > +		if (rc < sizeof(*rsp))
> > +			return -EIO;
> > +
> > +		data = (void *)rsp->data;
> 
> Nicer to cast to (union cdat_data *) than rely on bounce via a void *

Will change.

> 
> > +		received = rc - sizeof(*rsp);
> > +
> > +		if ((!entry_handle &&
> 
> Prefer == 0 for this because 0 is a magic value here.
> 
> > +		     received != sizeof(data->header)) ||
> > +		    (entry_handle &&
> > +		     (received < sizeof(data->entry) ||
> > +		      received != le16_to_cpu(data->entry.length))))
> >  			return -EIO;
> 
> Given it's two rather involved conditions maybe better to do.
> 
> 		if (entry_handle == 0) {
> 			if (received != sizeof(data->header)
> 				return -EIO;
> 		} else {
> 			if (received < sizeof(data->entry) ||
> 			    received != le16_to_cpu(data->entry.length))
> 				return -EIO;
> 		}
> 
> More code but easier to see the header vs entry checks.
> Could even define a little utility function / macro.
> 
> 		cdat_is_head_handle(val) entry_handle == 0
> so you get somewhat more self documenting code.
> 
> 		if (cdat_is_head_handle(entry_handle)) {
> 		} else {
> 		}

I will take this but without the macro.

> 
> >  
> >  		/* Get the CXL table access header entry handle */
> >  		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE,
> > -					 le32_to_cpu(data[0]));
> > -		entry_dw = rc / sizeof(__le32);
> > -		/* Skip Header */
> > -		entry_dw -= 1;
> > +					 le32_to_cpu(rsp->doe_header));
> > +
> >  		/*
> >  		 * Table Access Response Header overwrote the last DW of
> >  		 * previous entry, so restore that DW
> >  		 */
> > -		*data = saved_dw;
> > -		length -= entry_dw * sizeof(__le32);
> > -		data += entry_dw;
> > -		saved_dw = *data;
> > +		rsp->doe_header = saved_dw;
> 
> I'm not keen on this looking like we are writing the doe header
> as we are writing the tail of the last response.
> 
> Maybe the comment is enough.  I don't have a better idea on how
> to make this more obvious.

I think the comment is good enough here.

> 
> > +		remaining -= received;
> > +		rsp = (void *)rsp + received;
> 
> Was a potential problem with previous code, but this could
> in theory become unaligned and we should be using unaligned accessors
> for it as a result, or maybe adding a check that it doesn't ever become so.
> The check is probably the easier path given CDAT entries are thankfully
> (I think) all dword multiples as are the two headers.

Yes, buffers are dwords. In any case, pci_doe_recv_resp() is safe to
be used unaligned anyway.

Thanks for your review, will prepare a v4.

-Robert

> 
> > +		saved_dw = rsp->doe_header;
> >  	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
> >  
> >  	/* Length in CDAT header may exceed concatenation of CDAT entries */
> > -	*cdat_length -= length - sizeof(__le32);
> > +	*length -= remaining;
> >  
> >  	return 0;
> >  }
> 
>