Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 29 Jun 2022 21:42:42 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lukas Wunner <lukas@wunner.de>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Dave Jiang" <dave.jiang@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
        <linux-pci@vger.kernel.org>
Subject: Re: [PATCH V12 7/9] cxl/port: Introduce cxl_cdat_valid()
Message-ID: <Yr0pwgbJJT7HmtDA@iweiny-desk3>
References: <20220628041527.742333-1-ira.weiny@intel.com>
 <20220628041527.742333-8-ira.weiny@intel.com>
 <20220628154942.00002064@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220628154942.00002064@Huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?wLgj8KtBlOSUG0+38kezNbqGJMpQpmEcp8AatZb/odxza7YRsoap4bJkYHaU?=
 =?us-ascii?Q?Ry6VHqImdJWW9CQzfolfg3cSL4pxPbo9xS5WDIJaN0MRJKAEz22FPUOslGy+?=
 =?us-ascii?Q?oAhjctv1oioKToRFpCvYAj+8sfPGF+r+1vhpVfgdtX2cPRUj92EESARolKVz?=
 =?us-ascii?Q?biQRGlkoRBbuuY9vh6aN3+EsXgFAi4w7YfpuyWmeFRhzRzk3sxCCSIB+uPeE?=
 =?us-ascii?Q?dpXwhs9QiSDVxOgUHtg60oSrrUgzUJCNbyFTt7so6MZ1PZANtIiquBNy7Kne?=
 =?us-ascii?Q?iSl1zZeNpIc0dab6fpFDYYkDgXckVD2Xm9JOZ7pw78vXcD31Jz7I55ggEkTK?=
 =?us-ascii?Q?x4XlNSN3PCtlx6RARjfY4K+HxyulMl7zVQ/DTPJ9gAodR7zqKh0hnu6J2ehP?=
 =?us-ascii?Q?zm3EJvSHzQXiW7/poSI1wwszoRm9Lxhdu1iiV7IfcB85x2XGH77c25L2P0w7?=
 =?us-ascii?Q?cz7XIVlBPdw+H88HB+48ibj89dlroOd9NeuQ6Q5afneFWpREAOhYDMdwxB5N?=
 =?us-ascii?Q?43SqqTNXsymCd3UNAemSpGVAv++ytXQc8P/qkV9nT7jmx6p6nU2ODQF91mC0?=
 =?us-ascii?Q?Rx2Pgs3XJ8tpqJBcsgJn7U54LGV3m+3JCyOg/jtYm7VepvvYfE+s3uiXrQFH?=
 =?us-ascii?Q?o5Y34btyTeNtA2+azBcoqmrXswH81UpP0bkKV9jX7hCIus79LhGcaYOCdafU?=
 =?us-ascii?Q?CmxivW0Sd3uq9tTgSThRy+Cjvyv7AVlSpYQC7HgQwVPVt8bH9wkn2lOoMeKc?=
 =?us-ascii?Q?qp9XLhTOMhj5V2p7xd57P1hmm2lRzzcZNDgpZosCx373LBw2SHa2iSQH7whs?=
 =?us-ascii?Q?b+LzuMJZqwNG9weTg1GEgWWFhf2bL7N9MtgP4XHN437JD1mGS03lBrGobhMp?=
 =?us-ascii?Q?TbkypBXTVcKs41IXqOnA9FJztWq03pb8PF2kAlI+BYaLiHFEdFhz7l84SlB/?=
 =?us-ascii?Q?piciTcGH69sHCJzHBQ/Sy6wtof29oNLss08KE9jG9eZmujCPZ44te25brjKF?=
 =?us-ascii?Q?zpQPGnAFTMvSAoej+IKVFtgwA9PUOBAftoa9j1PGmrJkfqwuuOsoqkP/xwP/?=
 =?us-ascii?Q?KCDlXdhf6ZIs9EjVfj2rSMBUh/eiVwVC8YrplFuF/mB9D/uE+PIwoAmnp/Tl?=
 =?us-ascii?Q?8doccE72Xgokb1HajM8HSpYdguvgZ1Ec+Jjdh4wyyg6qSbeK1Mbn7BLde/Hj?=
 =?us-ascii?Q?yQ+Yz1vCScXzpFV/P4DxnGCbnq9w1QbSk0XpFjZOtWj3la1r5WDWXERUxAwB?=
 =?us-ascii?Q?SIA5kItwSTIiTYimBQ24vU2dexdgUTAgAwZV/KzbhPSeUog6wgtrvaDCFZy/?=
 =?us-ascii?Q?cJGVpcdbk96nXRPIPDr0jHNfVW+DGPQGorgu3eRpw1gK93co6wRUo4YAzuxM?=
 =?us-ascii?Q?ikCcP55ADDEdZYAPkv1r3iAOmndYsMuD3JOAWvnaC1G0HnyinrPIJ2m6ExuJ?=
 =?us-ascii?Q?7PbffXECqBpX9IH3EMWkdTRxQYD/ycKrh+nPtPMTYPAJ/5SULSB5qPV5KBYn?=
 =?us-ascii?Q?E02+9r055DDLo90SecxKJhgFYvL/z01YuXiM1YWo9OjE6WYsQAW5V/Mm7U7x?=
 =?us-ascii?Q?rpAPT42YJ8sieSE9Aa6X0OmCBZcfISBbQR1//gLKfdUpxIJ3ww/4pkQkgi/u?=
 =?us-ascii?Q?fh5Bzkwwa7XidSKjSlDK5AlzLJALhVfIDqWhCOb3ylo2?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e646763e-b4ea-4bda-ff2d-08da5a52fd15
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jun 2022 04:42:50.7491
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: aFgvBTcoDOterjuArdbVATIEcr/V17D3PxJJiVpHPNOkYFbjCMsnddiL6FhX9sKJmBy9BGu/LUlRPVRQhwblUg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5770
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE 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

On Tue, Jun 28, 2022 at 03:49:42PM +0100, Jonathan Cameron wrote:
> On Mon, 27 Jun 2022 21:15:25 -0700
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The CDAT data is protected by a checksum and should be the proper
> > length.
> > 
> > Introduce cxl_cdat_valid() to validate the data.  While at it check and
> > store the sequence number.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> Minor ordering comment.  With that tidied up
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 4bd479ec0253..6d775cc3dca1 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -532,6 +532,40 @@ static int cxl_cdat_get_length(struct device *dev,
> >  	return rc;
> >  }
> >  
> > +static bool cxl_cdat_valid(struct device *dev, struct cxl_cdat *cdat)
> > +{
> > +	u32 *table = cdat->table;
> > +	u8 *data8 = cdat->table;
> > +	u32 length, seq;
> > +	u8 check;
> > +	int i;
> > +
> > +	length = FIELD_GET(CDAT_HEADER_DW0_LENGTH, table[0]);
> > +	if ((length < CDAT_HEADER_LENGTH_BYTES) || (length > cdat->length)) {
> > +		dev_err(dev, "CDAT Invalid length %u (%zu-%zu)\n", length,
> > +			CDAT_HEADER_LENGTH_BYTES, cdat->length);
> > +		return false;
> > +	}
> > +
> > +	for (check = 0, i = 0; i < length; i++)
> > +		check += data8[i];
> > +
> > +	dev_dbg(dev, "CDAT length %u CS %u\n", length, check);
> > +	if (check != 0) {
> > +		dev_err(dev, "CDAT Invalid checksum %u\n", check);
> > +		return false;
> > +	}
> > +
> > +	seq = FIELD_GET(CDAT_HEADER_DW3_SEQUENCE, table[3]);
> > +	/* Store the sequence for now. */
> > +	if (cdat->seq != seq) {
> > +		dev_info(dev, "CDAT seq change %x -> %x\n", cdat->seq, seq);
> > +		cdat->seq = seq;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  static int cxl_cdat_read_table(struct device *dev,
> >  			       struct pci_doe_mb *cdat_mb,
> >  			       struct cxl_cdat *cdat)
> > @@ -579,6 +613,8 @@ static int cxl_cdat_read_table(struct device *dev,
> >  
> >  	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
> >  
> > +	if (!rc && !cxl_cdat_valid(dev, cdat))
> > +		return -EIO;
> 
> I'd prefer those handled separately as flow is more readable if error
> handling always out of line.
> 
> 	if (rc)
> 		return rc;

Actually rc is never not 0 here.  :-/

> 
> 	if (!cxl_cdata_valid)
> 		return -EIO;
> 
> 	return 0;

So this is all that is needed.  But the previous patch needs some clean up
to make sense in the series.

Ira