Re: [PATCH v5 1/2] devcoredump: Add dev_coredumpm_timeout()

Rodrigo Vivi
2024-06-10T20:16:14Z

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?

> 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
>