Received-SPF: pass (google.com: domain of linux-kernel+bounces-208973-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 10 Jun 2024 18:00:21 -0400
From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: "Souza, Jose" <jose.souza@intel.com>
CC: "Cavitt, Jonathan" <jonathan.cavitt@intel.com>,
	"intel-xe@lists.freedesktop.org" <intel-xe@lists.freedesktop.org>,
	"quic_mojha@quicinc.com" <quic_mojha@quicinc.com>,
	"johannes@sipsolutions.net" <johannes@sipsolutions.net>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 1/2] devcoredump: Add dev_coredumpm_timeout()
Message-ID: <Zmd3SdVjfjBo-KnL@intel.com>
References: <20240610161133.156566-1-jose.souza@intel.com>
 <ZmdfCDCQSZIsAWOQ@intel.com>
 <CH0PR11MB5444A6196C5AC7D3AE960FD3E5C62@CH0PR11MB5444.namprd11.prod.outlook.com>
 <f40ea3ddb5434715335b4634f38011ccd79e870c.camel@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <f40ea3ddb5434715335b4634f38011ccd79e870c.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?+T1+txu7fGtD8VcaqiQ4nUk0RMl/qFQJEl/cmB8Hnmzhl4gnFN17wPlK/5?=
 =?iso-8859-1?Q?Z8G9NMvgzy8tHo609fqX4IetntmHfIYm7Hko4NuKjzVvrvpzRnNexeYPIJ?=
 =?iso-8859-1?Q?VdmfQYsM2FWOyRl8MGUCZlU/Tjiyti2Hzr1j5DqLn4GuANc+sr6CcsAHkU?=
 =?iso-8859-1?Q?y2GfaybvKR7Bd0s/tpoZ5APiCsuYiVnvTAW0hVEaVSa8djoOdiKofbWDQO?=
 =?iso-8859-1?Q?DFbgUhu59d/puGD+WnzU/zmEnFHEKDOiV3Nc1GJlZJpoBIXmIb5nTf0MVD?=
 =?iso-8859-1?Q?7SVnVUVDwnHOw6FGzaRtrPgqa9VCyCsWtQaOlMsnUcTXcvlLHXiKy3pbLH?=
 =?iso-8859-1?Q?NBg6s/aTPdlpx0gLhHCyEVigyh5pyNPWWQ5yJw1Xn2K+V4sV2BjjryZAoN?=
 =?iso-8859-1?Q?S7keAZngFNRxjnv8P9rdKZs+96zs5sk/YW2a7BUQKc2vZiJFcDnsQETBOD?=
 =?iso-8859-1?Q?geF7arSpQEc1xF/4WrIkk2KCwWLkjlilPK46VOSP1hvThHrpqPeCDe1S9R?=
 =?iso-8859-1?Q?ycVIoM6nvxPD2iPs9Ik8Z8zydKnOruIHmh1PXjPssfK1gKCqKbPrrBIGaI?=
 =?iso-8859-1?Q?oJ0lnfhEQNyvMWt3tlIYQHtP6idIaZLe7iIMkIWfwuIew42d38kU7opfUQ?=
 =?iso-8859-1?Q?K5QyJH7BQ6hGRAsyzz9Ql114BPXb4PyD2YK6uKcU9qJBW++WcXIBWC2BCK?=
 =?iso-8859-1?Q?XVwMmewBTCgThi5B5IQOsK7DrhFBaIOVFN3oPHnzcfkRT6EJGphUOJgvYI?=
 =?iso-8859-1?Q?sWlUapbfmqmwHRsZpMzOiHX8kXRsEXGu+uKt5wtiA6qYzbjCB/xNufCrK8?=
 =?iso-8859-1?Q?PHOj7P+ZqR7zD/gTBgIPOghMF3gsBkRBM67IF7ZTWYe5ynhyCiFtfcXtqK?=
 =?iso-8859-1?Q?2eZc9Qi84DKgYQm6PrQ6A2HDL0197AVLxH91jD/7/zszYx2bZuiPJ/GpW3?=
 =?iso-8859-1?Q?jxsEKPpSrpAwngPD4dkc6dZOvB4lZJfdFEPiHSX64vqn7tG/FYZe6a8ULd?=
 =?iso-8859-1?Q?xUWCKTbliGijwqPQVHUSkdBOV8WJbeKthytxQDYmKo4AoM4mhajfuA0gRd?=
 =?iso-8859-1?Q?oGoZFJph17dFIEiQV4X8cAFufr/dXHXA2kOLo6sUkv6pcpXzYybp/hM6Rl?=
 =?iso-8859-1?Q?CBhTQtt6QsVqDBP22YwMlOO+xder9Oiv+QsjvHabepMH63S+/ea5BHcH/5?=
 =?iso-8859-1?Q?KBcR4RobHpQAN236vWmpmSrZQBh8+LBQzqTQ2bfCJujz22/yPYn9DXdF5f?=
 =?iso-8859-1?Q?Jd2udKFZDaDBkbtawo5C6ZBqTktQ8ooNsEgpPDcGjeaRMtfCKeD1/TjysD?=
 =?iso-8859-1?Q?3DEtCZDBcBWFCqnaQN4DrXC52tQcm9P8P2BoSskyG1Xb3h1sx637qHkncG?=
 =?iso-8859-1?Q?uJw6gRA4YpDBuHjAwZ+T9fxdMq/nkCRUp57cJHcMsXhHywZRgYiF/5qpbN?=
 =?iso-8859-1?Q?VHx/1eZ4UD74gW5DQn04vIdX8Z6i9RCdP0K3/bP+OHrgzKcvrOj3xo/5QU?=
 =?iso-8859-1?Q?Tp+QTcsel6m+2JJL9T9ugG5bQPRhAzDPgthlEK6WiMd1hhQyJJV4gJ5ZMq?=
 =?iso-8859-1?Q?/yFe/JpTgv7GxXYQfGCJzROxg1SouCiP4sOOlJLF09VUL3UD/djGXCXGB4?=
 =?iso-8859-1?Q?nMrA4rH16Vi5Gy5YpeMAZRUrFLaryx2fJznnIUMWQJeYXTBGfENFR3iA?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 92087ba0-9cb5-4367-97f9-08dc8998bac9
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB2854.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jun 2024 22:00:24.3769
 (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: saGOhMnP7GB3z5CG23ulVhAbgvZNgdGe6G1GhAGBR7rJTkQ/YqYgOPbpznQScmmek/I4SSUsB9BTEwlYnkBvJQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7250
X-OriginatorOrg: intel.com

On Mon, Jun 10, 2024 at 04:52:11PM -0400, Souza, Jose wrote:
> On Mon, 2024-06-10 at 20:42 +0000, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com> 
> > Sent: Monday, June 10, 2024 1:16 PM
> > To: Souza, Jose <jose.souza@intel.com>
> > Cc: linux-kernel@vger.kernel.org; intel-xe@lists.freedesktop.org; Mukesh Ojha <quic_mojha@quicinc.com>; Johannes Berg <johannes@sipsolutions.net>; Cavitt, Jonathan <jonathan.cavitt@intel.com>
> > Subject: Re: [PATCH v5 1/2] devcoredump: Add dev_coredumpm_timeout()
> > > 
> > > On Mon, Jun 10, 2024 at 09:11:32AM -0700, Jos? Roberto de Souza wrote:
> > > > Add function to set a custom coredump timeout.
> > > > 
> > > > For Xe driver usage, current 5 minutes timeout may be too short for
> > > > users to search and understand what needs to be done to capture
> > > > coredump to report bugs.
> > > > 
> > > > We have plans to automate(distribute a udev script) it but at the end
> > > > will be up to distros and users to pack it so having a option to
> > > > increase the timeout is a safer option.
> > > > 
> > > > v2:
> > > > - replace dev_coredump_timeout_set() by dev_coredumpm_timeout() (Mukesh)
> > > > 
> > > > v3:
> > > > - make dev_coredumpm() static inline (Johannes)
> > > > 
> > > > v5:
> > > > - rename DEVCOREDUMP_TIMEOUT -> DEVCD_TIMEOUT to avoid redefinition
> > > > in include/net/bluetooth/coredump.h
> > > > 
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > > Cc: Mukesh Ojha <quic_mojha@quicinc.com>
> > > > Cc: Johannes Berg <johannes@sipsolutions.net>
> > > > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > > Acked-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > > 
> > > Jonathan, also ack to merge this through drm-next flow?
> > 
> > Ack clear for drm-next flow.  Actually, you can upgrade that to a
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > I provided the Ack back before I knew I was allowed to provide
> > Reviewed-bys, or that they were different from Acks even in my
> > case.  I trust that my past-self who originally reviewed this meant
> > for the Ack to be a stand-in as a non-committal RB.
> 
> 
> Thanks you both but I think we need wait for an ack from Johannes or Mukesh to merge this devcoredump patch.
> On my other devcoredump patch already merged we also ask from a Greg's ack to merge it through drm-next.

My bad. I'm sorry.
Of course I meant to ask ack from Johannes there. I though he had acked this
approach already.

> 
> 
> > 
> > -Jonathan Cavitt
> > 
> > > 
> > > > Signed-off-by: Jos? Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/base/devcoredump.c  | 23 ++++++++--------
> > > >  include/linux/devcoredump.h | 54 ++++++++++++++++++++++++++++---------
> > > >  2 files changed, 54 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> > > > index 82aeb09b3d1b5..c795edad1b969 100644
> > > > --- a/drivers/base/devcoredump.c
> > > > +++ b/drivers/base/devcoredump.c
> > > > @@ -18,9 +18,6 @@ static struct class devcd_class;
> > > >  /* global disable flag, for security purposes */
> > > >  static bool devcd_disabled;
> > > >  
> > > > -/* if data isn't read by userspace after 5 minutes then delete it */
> > > > -#define DEVCD_TIMEOUT	(HZ * 60 * 5)
> > > > -
> > > >  struct devcd_entry {
> > > >  	struct device devcd_dev;
> > > >  	void *data;
> > > > @@ -328,7 +325,8 @@ void dev_coredump_put(struct device *dev)
> > > >  EXPORT_SYMBOL_GPL(dev_coredump_put);
> > > >  
> > > >  /**
> > > > - * dev_coredumpm - create device coredump with read/free methods
> > > > + * dev_coredumpm_timeout - create device coredump with read/free methods with a
> > > > + * custom timeout.
> > > >   * @dev: the struct device for the crashed device
> > > >   * @owner: the module that contains the read/free functions, use %THIS_MODULE
> > > >   * @data: data cookie for the @read/@free functions
> > > > @@ -336,17 +334,20 @@ EXPORT_SYMBOL_GPL(dev_coredump_put);
> > > >   * @gfp: allocation flags
> > > >   * @read: function to read from the given buffer
> > > >   * @free: function to free the given buffer
> > > > + * @timeout: time in jiffies to remove coredump
> > > >   *
> > > >   * Creates a new device coredump for the given device. If a previous one hasn't
> > > >   * been read yet, the new coredump is discarded. The data lifetime is determined
> > > >   * by the device coredump framework and when it is no longer needed the @free
> > > >   * function will be called to free the data.
> > > >   */
> > > > -void dev_coredumpm(struct device *dev, struct module *owner,
> > > > -		   void *data, size_t datalen, gfp_t gfp,
> > > > -		   ssize_t (*read)(char *buffer, loff_t offset, size_t count,
> > > > -				   void *data, size_t datalen),
> > > > -		   void (*free)(void *data))
> > > > +void dev_coredumpm_timeout(struct device *dev, struct module *owner,
> > > > +			   void *data, size_t datalen, gfp_t gfp,
> > > > +			   ssize_t (*read)(char *buffer, loff_t offset,
> > > > +					   size_t count, void *data,
> > > > +					   size_t datalen),
> > > > +			   void (*free)(void *data),
> > > > +			   unsigned long timeout)
> > > >  {
> > > >  	static atomic_t devcd_count = ATOMIC_INIT(0);
> > > >  	struct devcd_entry *devcd;
> > > > @@ -403,7 +404,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
> > > >  	dev_set_uevent_suppress(&devcd->devcd_dev, false);
> > > >  	kobject_uevent(&devcd->devcd_dev.kobj, KOBJ_ADD);
> > > >  	INIT_DELAYED_WORK(&devcd->del_wk, devcd_del);
> > > > -	schedule_delayed_work(&devcd->del_wk, DEVCD_TIMEOUT);
> > > > +	schedule_delayed_work(&devcd->del_wk, timeout);
> > > >  	mutex_unlock(&devcd->mutex);
> > > >  	return;
> > > >   put_device:
> > > > @@ -414,7 +415,7 @@ void dev_coredumpm(struct device *dev, struct module *owner,
> > > >   free:
> > > >  	free(data);
> > > >  }
> > > > -EXPORT_SYMBOL_GPL(dev_coredumpm);
> > > > +EXPORT_SYMBOL_GPL(dev_coredumpm_timeout);
> > > >  
> > > >  /**
> > > >   * dev_coredumpsg - create device coredump that uses scatterlist as data
> > > > diff --git a/include/linux/devcoredump.h b/include/linux/devcoredump.h
> > > > index c8f7eb6cc1915..e3de1e545a4a5 100644
> > > > --- a/include/linux/devcoredump.h
> > > > +++ b/include/linux/devcoredump.h
> > > > @@ -12,6 +12,9 @@
> > > >  #include <linux/scatterlist.h>
> > > >  #include <linux/slab.h>
> > > >  
> > > > +/* if data isn't read by userspace after 5 minutes then delete it */
> > > > +#define DEVCD_TIMEOUT	(HZ * 60 * 5)
> > > > +
> > > >  /*
> > > >   * _devcd_free_sgtable - free all the memory of the given scatterlist table
> > > >   * (i.e. both pages and scatterlist instances)
> > > > @@ -50,16 +53,17 @@ static inline void _devcd_free_sgtable(struct scatterlist *table)
> > > >  	kfree(delete_iter);
> > > >  }
> > > >  
> > > > -
> > > >  #ifdef CONFIG_DEV_COREDUMP
> > > >  void dev_coredumpv(struct device *dev, void *data, size_t datalen,
> > > >  		   gfp_t gfp);
> > > >  
> > > > -void dev_coredumpm(struct device *dev, struct module *owner,
> > > > -		   void *data, size_t datalen, gfp_t gfp,
> > > > -		   ssize_t (*read)(char *buffer, loff_t offset, size_t count,
> > > > -				   void *data, size_t datalen),
> > > > -		   void (*free)(void *data));
> > > > +void dev_coredumpm_timeout(struct device *dev, struct module *owner,
> > > > +			   void *data, size_t datalen, gfp_t gfp,
> > > > +			   ssize_t (*read)(char *buffer, loff_t offset,
> > > > +					   size_t count, void *data,
> > > > +					   size_t datalen),
> > > > +			   void (*free)(void *data),
> > > > +			   unsigned long timeout);
> > > >  
> > > >  void dev_coredumpsg(struct device *dev, struct scatterlist *table,
> > > >  		    size_t datalen, gfp_t gfp);
> > > > @@ -72,12 +76,13 @@ static inline void dev_coredumpv(struct device *dev, void *data,
> > > >  	vfree(data);
> > > >  }
> > > >  
> > > > -static inline void
> > > > -dev_coredumpm(struct device *dev, struct module *owner,
> > > > -	      void *data, size_t datalen, gfp_t gfp,
> > > > -	      ssize_t (*read)(char *buffer, loff_t offset, size_t count,
> > > > -			      void *data, size_t datalen),
> > > > -	      void (*free)(void *data))
> > > > +void dev_coredumpm_timeout(struct device *dev, struct module *owner,
> > > > +			   void *data, size_t datalen, gfp_t gfp,
> > > > +			   ssize_t (*read)(char *buffer, loff_t offset,
> > > > +					   size_t count, void *data,
> > > > +					   size_t datalen),
> > > > +			   void (*free)(void *data),
> > > > +			   unsigned long timeout)
> > > >  {
> > > >  	free(data);
> > > >  }
> > > > @@ -92,4 +97,29 @@ static inline void dev_coredump_put(struct device *dev)
> > > >  }
> > > >  #endif /* CONFIG_DEV_COREDUMP */
> > > >  
> > > > +/**
> > > > + * dev_coredumpm - create device coredump with read/free methods
> > > > + * @dev: the struct device for the crashed device
> > > > + * @owner: the module that contains the read/free functions, use %THIS_MODULE
> > > > + * @data: data cookie for the @read/@free functions
> > > > + * @datalen: length of the data
> > > > + * @gfp: allocation flags
> > > > + * @read: function to read from the given buffer
> > > > + * @free: function to free the given buffer
> > > > + *
> > > > + * Creates a new device coredump for the given device. If a previous one hasn't
> > > > + * been read yet, the new coredump is discarded. The data lifetime is determined
> > > > + * by the device coredump framework and when it is no longer needed the @free
> > > > + * function will be called to free the data.
> > > > + */
> > > > +static inline void dev_coredumpm(struct device *dev, struct module *owner,
> > > > +				 void *data, size_t datalen, gfp_t gfp,
> > > > +				 ssize_t (*read)(char *buffer, loff_t offset, size_t count,
> > > > +						 void *data, size_t datalen),
> > > > +				void (*free)(void *data))
> > > > +{
> > > > +	dev_coredumpm_timeout(dev, owner, data, datalen, gfp, read, free,
> > > > +			      DEVCD_TIMEOUT);
> > > > +}
> > > > +
> > > >  #endif /* __DEVCOREDUMP_H */
> > > > -- 
> > > > 2.45.2
> > > > 
> > > 
>