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:   Thu, 1 Dec 2022 09:49:49 -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>,
        Dave Jiang <dave.jiang@intel.com>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH V2 09/11] cxl/test: Add generic mock events
Message-ID: <Y4jpPVSX9/89x3PH@iweiny-desk3>
References: <20221201002719.2596558-1-ira.weiny@intel.com>
 <20221201002719.2596558-10-ira.weiny@intel.com>
 <20221201143727.0000255c@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221201143727.0000255c@Huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?d48XdMK/9RVJSSlbsElqBHQdBYVP9dZjkf1H6r/jdCrhmO+4bXzNOwhcDZsg?=
 =?us-ascii?Q?TjBpC10Tq3YMhhKIMG8o89dNRgKwJSbgxG3ViW7Jueb2yRBUi35uyiUqPJv1?=
 =?us-ascii?Q?PSpay+g19ajSqp4wAYecxTau1gTE/QJ/TxYfxkNALeJJyO0BHSr8g5to0Vqo?=
 =?us-ascii?Q?dX2B1UbEN0RmS7xzFj5lKngzlUzt/Ua7nnRamCA77v3b7GmpfsmxejwzdTqm?=
 =?us-ascii?Q?488CblNbIAXTk+6ADlaLiqZ8/Zs2Asn56lJRtQWT8SDQUMtQEZtBnVPQ3kPm?=
 =?us-ascii?Q?oLzqGv8HYEwRncUHkMr/ks1hUs/K5c66S3vIrmD+n1ijTXXnYMG0rJTAG06G?=
 =?us-ascii?Q?JsN/26glffvAK6XArV1euQfmx3DjWd9JifXcHMAmLH1MbJiN/k4yxHICV69Z?=
 =?us-ascii?Q?N28prFkOACZuvVnfuhXEbhY9r1p//0HDu3hNO401fwRmAkzvghtRGJ/6kFf0?=
 =?us-ascii?Q?2ukj/wmUf/54MO5G7VpbuHQAfGfmvGI9COF23ZJomxD8W4mSJyA8Kk6m7/jP?=
 =?us-ascii?Q?kK7HomqUU8LwbdgamBQPJ6hT0js2qty+b8rlfLFCgZmHLolPzYMRhZ3946dj?=
 =?us-ascii?Q?bxBNS1brPRt0ULa6SrDGbRp3G4Wj1KRscG3j5f8D7wvTbDOx/zKorc4xA0so?=
 =?us-ascii?Q?38hDgdJusOGWGB0qNRZJldWK2gd22AH6OzIWuiYunvYA9mBBx84d4bhVKzeE?=
 =?us-ascii?Q?A16LcDSbTGFHF1ZcK+mYvmzgSD07KoKwh9CnDpd6gw6JQKfC7somIO/GzKIc?=
 =?us-ascii?Q?8ULtJCmKajiZmDm1r+uVaKhwcyaYPohqHw+WGDigHsjksTJDahtVawjBN2Pd?=
 =?us-ascii?Q?lZWjy4vSa7ulFUUKzcfCvoXuf8+jo09m02wt5QD07roNLrom8JwsslJyspyk?=
 =?us-ascii?Q?OThW7bm05hvktkhHj5CaPq+mdaB7KyDlOdGGTNOoC7HngoytcLRvKxqGeP2W?=
 =?us-ascii?Q?itxz3FordzZv8w9HCYS32km27hlOo6q0z+AK6kPpdPm4Cahuf7B39rRxiPeC?=
 =?us-ascii?Q?WcIxfERBN1Fv9KjgLMpgVTKOTSWHXKoLlKJMJqbyxtkoeTLhFGyHkEUCLz+N?=
 =?us-ascii?Q?Eg6GwYyTp8/zvUPWTsVWWmJQk+B8sfPi822XhUKJvyvuD8afoGmFcq5/VWlT?=
 =?us-ascii?Q?6RxIu+ZHhvVIY1te4T3jJFKyyPU5reG9Xdswy+Z6o4TRVlJ60Uvuyk9l8SWp?=
 =?us-ascii?Q?59bzR8IfUZV9M14mx6JlCBI4Sa+O5MpOAeoIFY89rbgS1Whlwvz4CBK3Rhtd?=
 =?us-ascii?Q?+SNDSaWqxa2ZCTafNP1K+yOGRZ4UuBjbO1eocIiFnrDDY4gISNbeQpY4g1aV?=
 =?us-ascii?Q?qmxIamzxsM4T9u4ydcMOXf24JYMe+o+DJIX4C+INyfaamcPZd3nrP2Db4oP+?=
 =?us-ascii?Q?KUJJqQdiTkCV5TNu8aMeqWp9rLunwrNmDUpKYnMnDPPgpf+KUCdeYFoK09f4?=
 =?us-ascii?Q?UA/gNQFBy7xQiiXyHmxEjZqNO6qEYNvhdvF4KXqWw34YPysBLctZkpsZYgfR?=
 =?us-ascii?Q?3u1mXRvxIOrdsucDjqxG2ygecNsRsvZ5io4RADRp/1tlvUIbnmawPTTXCwGu?=
 =?us-ascii?Q?S2wtnomm5+3/nxKv7lPzcvD57Ly94R2h+Ta3kKFv?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 53c4edf2-14f1-4eef-d794-08dad3c47411
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2022 17:49:54.2129
 (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: TE6CDcbkiYOFiMWh1TL2F3uvpam/+BvX4MhtZj+If14sAWRQUZPkMJD+0PBDvqtZeuFTVEyvgBBrhaAAcl3KKw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6740
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 Thu, Dec 01, 2022 at 02:37:27PM +0000, Jonathan Cameron wrote:
> On Wed, 30 Nov 2022 16:27:17 -0800
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Facilitate testing basic Get/Clear Event functionality by creating
> > multiple logs and generic events with made up UUID's.
> > 
> > Data is completely made up with data patterns which should be easy to
> > spot in trace output.
> > 
> > A single sysfs entry resets the event data and triggers collecting the
> > events for testing.
> > 
> > Events are returned one at a time which is within the specification even
> > though it does not exercise the full capabilities of what a device may
> > do.
> > 
> > Test traces are easy to obtain with a small script such as this:
> > 
> > 	#!/bin/bash -x
> > 
> > 	devices=`find /sys/devices/platform -name cxl_mem*`
> > 
> > 	# Turn on tracing
> > 	echo "" > /sys/kernel/tracing/trace
> > 	echo 1 > /sys/kernel/tracing/events/cxl/enable
> > 	echo 1 > /sys/kernel/tracing/tracing_on
> > 
> > 	# Generate fake interrupt
> > 	for device in $devices; do
> > 	        echo 1 > $device/event_trigger
> > 	done
> > 
> > 	# Turn off tracing and report events
> > 	echo 0 > /sys/kernel/tracing/tracing_on
> > 	cat /sys/kernel/tracing/trace
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> 
> A minor comment on xarray cleanup inline
> 
> Jonathan
> 
> 
> > +void cxl_mock_remove_event_logs(struct device *dev)
> > +{
> > +	struct mock_event_store *mes;
> > +
> > +	mes = xa_erase(&mock_dev_event_store, (unsigned long)dev);
> 
> As below, I'd move this into a devm_add_action_or_reset() so
> that we don't need to deal with doing it manually.

yea

> 
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_mock_remove_event_logs);
> 
> ...
> 
> >  static int cxl_mock_mem_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct cxl_memdev *cxlmd;
> >  	struct cxl_dev_state *cxlds;
> > +	u32 ev_status;
> >  	void *lsa;
> >  	int rc;
> >  
> > @@ -281,11 +304,13 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
> >  	if (rc)
> >  		return rc;
> >  
> > +	ev_status = cxl_mock_add_event_logs(cxlds);
> 
> On error later in this function these leak.  Just use devm_add_action_or_reset()
> inside cxl_mock_add_event_logs() so we don't have to care about that.

I see it now.  There is an entry in the xarray which is leaked.

> 
> > +
> >  	cxlmd = devm_cxl_add_memdev(cxlds);
> >  	if (IS_ERR(cxlmd))
> >  		return PTR_ERR(cxlmd);
> >  
> > -	cxl_mem_get_event_records(cxlds);
> > +	__cxl_mem_get_event_records(cxlds, ev_status);
> >  
> >  	if (resource_size(&cxlds->pmem_res) && IS_ENABLED(CONFIG_CXL_PMEM))
> >  		rc = devm_cxl_add_nvdimm(dev, cxlmd);
> > @@ -293,6 +318,12 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +static int cxl_mock_mem_remove(struct platform_device *pdev)
> > +{
> > +	cxl_mock_remove_event_logs(&pdev->dev);
> Why not use devm_add_action_or_reset()?

Yea that is a better pattern.

Ira