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, 4 Jan 2023 15:53:13 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     johnny <johnny.li@montage-tech.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        "Alison Schofield" <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Dave Jiang <dave.jiang@intel.com>,
        <linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>,
        <linux-acpi@vger.kernel.org>, <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH V4 2/9] cxl/mem: Read, trace, and clear events on driver
 load
Message-ID: <Y7YRaYk1Qx3d2Bgt@iweiny-desk3>
References: <20221212070627.1372402-1-ira.weiny@intel.com>
 <20221212070627.1372402-3-ira.weiny@intel.com>
 <20221216153939.00007c41@Huawei.com>
 <Y5zo+UqOmGCE4ObC@iweiny-desk3>
 <Y55d/l1nTfpTX/1A@SR651>
 <20221218155553.000043e5@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221218155553.000043e5@Huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Vk295WJMpxSSJex7hcpLYANAwCF4PLWsGMydKeTFWeKqw9opkFgiLIwar7gF?=
 =?us-ascii?Q?FepyOVd84/e+09hjq2swvPgYRz/ow8O5qKNXKNCymR7Pcufj+c/g9bacxLws?=
 =?us-ascii?Q?G0RgtNf0T1d7YQXS6eug3wKjuMGnzsxBA5Yupxsn7LgXR45T9sGk2/NVJ8m2?=
 =?us-ascii?Q?UFYAnmYbbla+WxAmqPY3BubQBRt7E7U3uPwR8/+B4nzFOPrQ1Z2yu42T5hl6?=
 =?us-ascii?Q?MbGyiwsag5AAZ0XTfZKKwrYOfluul1ThrLfvZe5pjhh/6hQimQ8f96NWVsOS?=
 =?us-ascii?Q?PyZLlchwMvN+Rh4+Typ8Wnk2/R6ulnMjPrMsOJELe9f1BYNmMuIyj5Xkh79c?=
 =?us-ascii?Q?94qnlDgTTikH15gOoNmIbnoPAnv/WrEMOomREeaDZ08GUbY0rsrjoY77TSEv?=
 =?us-ascii?Q?oGn3jl6Ehmh/L0uXzVWMRYu4o5Z0zZQY+mcpNi2+QWBhzO+MKKTOsnNbW7+A?=
 =?us-ascii?Q?svuJvQDIRsdDn1cLs+IIDl1zYc0WSuqEfZtrtzN7GL/hfyPZG4SZdTLwXirs?=
 =?us-ascii?Q?7ccVNHz0bAd0/DQFeb52MVqLlvU4YRaw6VuesIfhCZoocEGw+7wMVfIpApI8?=
 =?us-ascii?Q?kk4sGHFzSjmdmgK9+gb4/bwTfp+EVK/UqmxMuy1P+po7ehVG2moXwzJi8gZT?=
 =?us-ascii?Q?WxTs9n6BEiylkxegM4lgYkLeGWNLw028G/MUMHtxmNwYU22rgVXoknnOYNgQ?=
 =?us-ascii?Q?d57w1ObdCluPbT60sHxdWCHsWMMzvMS6nMaqSC15EY4IWH/WV13R/9jFbdPG?=
 =?us-ascii?Q?JofQaJ4wKYU9Hhpg7YfV5S6R4zPfh6yXjfUveKkRFAHGYC8Ctraiotj0ETdy?=
 =?us-ascii?Q?/RugEOuJMcIe2lB3gdui1Hh4tF+graPlpGoa+LIjzAuLJf4Rj3vGLPz3E251?=
 =?us-ascii?Q?djK1+aenh8qgcHC7zN1Gb9GipHsW0RVOn1FAWQDXP/ku5a/1PfPNDCddJPKx?=
 =?us-ascii?Q?lmRVGMDAg0pYkW/izALCCQcH4Gr26GaizLso9WVIXA4N3INiVTePezax7E5t?=
 =?us-ascii?Q?vNK9jVlQRjtTnGNS7YAi7Koa7NX2dbUDpFw5BTjb9d8BMs0x6w5sjD4lHiCe?=
 =?us-ascii?Q?MaUGynRcHO4ufL4mtz9Uo6ssUYebj1HesQ9kB4yL37ThVjC6SoCAunSzebJ9?=
 =?us-ascii?Q?QGkYl40eoYoAk6MWp5BkMEenTzeUIIDkpxK/s78SJMi0KBwkG85An44hGbRh?=
 =?us-ascii?Q?psg6uc2YiVMF3uPBkGwcYCBA/75a1Cj8JtzqBlK1TMnz3Gk/9PNSQUvd5mFr?=
 =?us-ascii?Q?cK92J0uJVLWUHXrZHbpShHpzj9a1a9Nbz0RtYJrc9h41M2dp2KCyFOocZy8J?=
 =?us-ascii?Q?WukYhjjZv/ZfBdeO1MRJrHfiftEcP9/fbmglX6oh5TSpvS3l6I1sOlPb17fK?=
 =?us-ascii?Q?Y72leOQp/upidzT9f5y67iBdnKih+8Jo/ezXmFlGYpQLt/BlmAzaDedSXueT?=
 =?us-ascii?Q?aR4fMZjiw5liGw4W11M9ryHsMheSfYyBDi7yen2hTxqTi82uXS2YSAYx0fWC?=
 =?us-ascii?Q?9Qd3K4psR71/87tJQW/43/nTuQi1M9MVQ0Nw1EWd0Uol+lFNFS0A+bquhTUi?=
 =?us-ascii?Q?IzFbQzq8zbEMgjn5PMbGr7stdab8F7kNxgqm/DZA?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6025035c-97eb-445a-9c97-08daeeaedb25
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jan 2023 23:53:19.6147
 (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: f+UzXEpCwDBXe7hZT1TfBcw9E6vnoFEgGKHiXfQRC0/Px1ocYoBKTNkL59YjxbU1zSbyUbet3HIr9vRoIANwVA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4564
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE 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 Sun, Dec 18, 2022 at 03:55:53PM +0000, Jonathan Cameron wrote:
> On Sun, 18 Dec 2022 08:25:34 +0800
> johnny <johnny.li@montage-tech.com> wrote:
> 

[snip]

> > >   
> > > > > +	}
> > > > > +
> > > > > +	mbox_cmd = (struct cxl_mbox_cmd) {
> > > > > +		.opcode = CXL_MBOX_OP_CLEAR_EVENT_RECORD,
> > > > > +		.payload_in = &payload,
> > > > > +		.size_in = pl_size,  
> > > > 
> > > > This payload size should be whatever we need to store the records,
> > > > not the max size possible.  Particularly as that size is currently
> > > > bigger than the mailbox might be.  
> > > 
> > > But the above check and set ensures that does not happen.
> > >   
> > > > 
> > > > It shouldn't fail (I think) simply because a later version of the spec might
> > > > add more to this message and things should still work, but definitely not
> > > > good practice to tell the hardware this is much longer than it actually is.  
> > > 
> > > I don't follow.
> > > 
> > > The full payload is going to be sent even if we are just clearing 1 record
> > > which is inefficient but it should never overflow the hardware because it is
> > > limited by the check above.
> > > 
> > > So why would this be a problem?
> > >   
> > 
> > per spec3.0, Event Record Handles field is "A list of Event Record Handles the 
> > host has consumed and the device shall now remove from its internal Event Log 
> > store.". Extra unused handle list does not folow above description. And also 
> > spec mentions "All event record handles shall be nonzero value. A value of 0 
> > shall be treated by the device as an invalid handle.". So if there is value 0 
> > in extra unused handles, device shall return invalid handle error code
> 
> I don't think we call into that particular corner as the number of event
> record handles is set correctly.  Otherwise I agree this isn't following the
> spec - though I think key here is that it won't be broken against CXL 3.0 devices
> (with that rather roundabout argument that a CXL 3.0 devices should handle later
> spec messages as those should be backwards compatible) but it might be broken
> against CXL 3.0+ ones that interpret the 0s at the end as having meaning.

I'm respining this to add the pci_set_master() anyway.  So I'm going to change
this as well.  I really don't see how hardware would go off anything but the
number of records to process the handles I could see some overly strict
firmware wanting to validate the size being exactly equal to the number
specified rather than just less than (which is what I would anticipate an issue
with).

Dan has agreed to land the movement of the trace point definition to
drivers/cxl patch I need to cxl/next.  After that I will rebase and send out.

Ira

> 
> Thanks,
> 
> Jonathan
>