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, 16 Nov 2022 17:07:28 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC:     Dan Williams <dan.j.williams@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Ben Widawsky" <bwidawsk@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH 03/11] cxl/mem: Implement Clear Event Records command
Message-ID: <Y3WJUN4oNs6t5qOS@iweiny-desk3>
References: <20221110185758.879472-1-ira.weiny@intel.com>
 <20221110185758.879472-4-ira.weiny@intel.com>
 <20221116152426.0000626b@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221116152426.0000626b@Huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Dj/vt/uKqe6aan9f0mGHciMtx4vGzumd8lX1KvYlH69DeZAh9Z8MyYUc44Qr?=
 =?us-ascii?Q?iWfMwIpuqGB4NIo9m+o348YazN0NIK+X+6zYT1BhABdstxt78yZplNvow33y?=
 =?us-ascii?Q?72hKKgw4vTn83RvZ2kCAWfkjlkCnKfzBU7UmHitipwNQFMD6TWMxds6fRQyU?=
 =?us-ascii?Q?qG8zxiA+ZZQjr1w9c0dNrTvh4P82V7sYG6HIVYbg1dLnQY3OEcsIEy2Zs0Mu?=
 =?us-ascii?Q?AEUXE863Q4k1vBhTGzIzctooL+L4adLTFPRvEaA2avEpKJ25xdrBWRrIb5n0?=
 =?us-ascii?Q?vLcW4ulpsZPnKr9/ReozMLVLA9as8xC+iidtAj6tHT5V6spzlNBUg0YiX6iC?=
 =?us-ascii?Q?vclZ+OUy/4b1NlTU8XAi/TdkgUEbAGYHKEh19sF71wdcQ9C65CtbIawk/gKm?=
 =?us-ascii?Q?C3megnmW05UAMjuxtwxpUrc/U+5bTn5Tux0OGRsXksmoRpfkjkC/kq96uFHa?=
 =?us-ascii?Q?2TGNETPzVIxRht8CIgW9f3s6I9HBozGsphuLmBsgYSjj0b2MSzlfcVNom6IM?=
 =?us-ascii?Q?VX2s38GTRFNUBlIbsl+Mc+7AuUwRRXI0r60OVWX2LZ5T0gwAYZlVU16W6dco?=
 =?us-ascii?Q?m6iDqBkB+iCyXnHRfUmEMMFzEe1le9beueO2SxYHKoPx8a2gnN3Z1Xs29Zw6?=
 =?us-ascii?Q?3J/adpWSk1bL1qCQ7y9acVnJudDawlGE7rrHcFGZap8/PrucWbykqUFiyWl6?=
 =?us-ascii?Q?5DL3GVTXYQTbLkgtvTxAbbGJw6uAAnl3XRpoMcuibktv6VKtrMDbrBg9RN4Z?=
 =?us-ascii?Q?X9wz9rXW4pPtB5x0uZksIEj38R1M61tKB4RbsijMcOY9+Viy4mz4nmEinJtG?=
 =?us-ascii?Q?BjcxwT1hyepM8S/7B2R95/L7e1U1URoYw3ygpSaSI8ViiZFlwhMHNOAkD+i8?=
 =?us-ascii?Q?RdJmMl24gLZGfiq4gad4pquRVHxaRbkwKE1iGM7wEp4qxbL6kubSnWzkaOM0?=
 =?us-ascii?Q?ro0/vx2jO9eliaruerZl1tSJvfwEQJVNZREofKABV4C5ecpZLFrjqOBFAEyE?=
 =?us-ascii?Q?iGlBz8zUk1Mn74uyZeq9S293kBm2MHGKX657Bfmm7ehLxFQKSfUJKRXYvBb7?=
 =?us-ascii?Q?ECqpy1qQWBREJ1dxsb0T50AkQisGVwxb1qydTlTTBnRStyqjUvG8Ff+bVXCY?=
 =?us-ascii?Q?0X1y3qyOLpNW/KA2sYN2g87l4jwhINKnu9DMFJz5mJaEblraVc/pDNh+5zku?=
 =?us-ascii?Q?W2DZGrpyhqK86uecC3IuqsuLpv6NQB0ULMH07BH9iZkzRR2dMgA2zuwEzMd5?=
 =?us-ascii?Q?EwWN/ELwgldEdBi1U2wG2iBw/b04dUhCTMJEIpT5zv5ywzbZykBMbSZQIzXH?=
 =?us-ascii?Q?cscpnfZmvoBFB5KZz4BPL//995xrLnHIUxTYWK9atKKzCnQA6X/I6T5JwThz?=
 =?us-ascii?Q?Y90b/EMatLyhU6U0nKC1UkBlkwMdw7LZIQCnheWkfcEy6+3GGI/DtoUJSaBR?=
 =?us-ascii?Q?nHViEDMixIWRHHR0Zc2a3hz4bzuefUA137xDP3Y3spmAonwBj5qO0wFhtLk1?=
 =?us-ascii?Q?xJqadSZk/OOBuZ0mL3aDAoXW9cxx3U8xARcvOEl0adNGsagtUrsQ1cgPqosQ?=
 =?us-ascii?Q?7LWeGXivDN+bOQSB0lAGeeHeeyL6UelF6DFZ3W1U?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a6e947d4-c0bd-409a-55ad-08dac8381b35
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Nov 2022 01:07:32.7643
 (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: j0CNQj2QhxEJq6fouUT1zueC2zkbiFRy0a3rRzEX+0qGJgv2tZHd8OawmNjYYxofUMPo+7W6dezWxV0MqdZpAg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB7096
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,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,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 Wed, Nov 16, 2022 at 03:24:26PM +0000, Jonathan Cameron wrote:
> On Thu, 10 Nov 2022 10:57:50 -0800
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > CXL rev 3.0 section 8.2.9.2.3 defines the Clear Event Records mailbox
> > command.  After an event record is read it needs to be cleared from the
> > event log.
> > 
> > Implement cxl_clear_event_record() and call it for each record retrieved
> > from the device.
> > 
> > Each record is cleared individually.  A clear all bit is specified but
> > events could arrive between a get and the final clear all operation.
> > Therefore each event is cleared specifically.
> > 
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> Some follow through comment updates needed from changes in earlier patches +
> one comment you can ignore if you prefer to keep it as is.
> 
> >  static void cxl_mem_get_records_log(struct cxl_dev_state *cxlds,
> >  				    enum cxl_event_log_type type)
> >  {
> > @@ -728,14 +750,23 @@ static void cxl_mem_get_records_log(struct cxl_dev_state *cxlds,
> >  		}
> >  
> >  		pl_nr = le16_to_cpu(payload.record_count);
> > -		if (trace_cxl_generic_event_enabled()) {
> 
> To simplify this patch, maybe push this check down in the previous patch so this
> one doesn't move code around?  It'll look a tiny bit odd there of course..

That is the issue I think the oddness is easier to defend here vs having it in
the previous patch.

> 
> > +		if (pl_nr > 0) {
> >  			u16 nr_rec = min_t(u16, pl_nr, CXL_GET_EVENT_NR_RECORDS);
> >  			int i;
> >  
> > -			for (i = 0; i < nr_rec; i++)
> > -				trace_cxl_generic_event(dev_name(cxlds->dev),
> > -							type,
> > -							&payload.record[i]);
> > +			if (trace_cxl_generic_event_enabled()) {
> > +				for (i = 0; i < nr_rec; i++)
> > +					trace_cxl_generic_event(dev_name(cxlds->dev),
> > +								type,
> > +								&payload.record[i]);
> > +			}
> > +
> > +			rc = cxl_clear_event_record(cxlds, type, &payload, nr_rec);
> > +			if (rc) {
> > +				dev_err(cxlds->dev, "Event log '%s': Failed to clear events : %d",
> > +					cxl_event_log_type_str(type), rc);
> > +				return;
> > +			}
> >  		}
> >  
> 
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index da64ba0f156b..28a114c7cf69 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> 
> >  
> > +/*
> > + * Clear Event Records input payload
> > + * CXL rev 3.0 section 8.2.9.2.3; Table 8-51
> > + *
> > + * Space given for 1 record
> 
> Nope...

<sigh> yep...  ;-)

> 
> 
> > + */
> > +struct cxl_mbox_clear_event_payload {
> > +	u8 event_log;		/* enum cxl_event_log_type */
> > +	u8 clear_flags;
> > +	u8 nr_recs;		/* 1 for this struct */
> Nope :)  Delete the comments so they can't be wrong if this changes in future!

Yep.  :-/

Ira