Re: [PATCH 2/4] devcoredump: Add dev_coredump_timeout_set()

[Mukesh Ojha]

On 2/28/2024 2:30 AM, José Roberto de Souza wrote:
> Add function to set a custom coredump timeout.
>
> Current 5-minute timeout may be too short for users to search and
> understand what needs to be done to capture coredump to report bugs.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Cc: Mukesh Ojha <quic_mojha@xxxxxxxxxxx>
> Cc: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> Cc: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
> Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> ---
>   drivers/base/devcoredump.c  | 23 +++++++++++++++++++++++
>   include/linux/devcoredump.h |  6 ++++++
>   2 files changed, 29 insertions(+)
>
> diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> index e96427411b87c..2857ceb3eb3aa 100644
> --- a/drivers/base/devcoredump.c
> +++ b/drivers/base/devcoredump.c
> @@ -326,6 +326,29 @@ void dev_coredump_put(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(dev_coredump_put);
>   
> +/**
> + * dev_coredump_timeout_set - set coredump timeout
> + * @dev: the struct device for the crashed device
> + * @secs_timeout: new timeout in seconds
> + *
> + * If giving device has a coredump this sets a new timeout for coredump.
> + */
> +void dev_coredump_timeout_set(struct device *dev, unsigned long secs_timeout)
> +{
> +	struct device *existing;
> +	struct devcd_entry *devcd;
> +
> +	existing = class_find_device(&devcd_class, NULL, dev,
> +				     devcd_match_failing);
> +	if (!existing)
> +		return;
> +
> +	devcd = dev_to_devcd(existing);
> +	mod_delayed_work(system_wq, &devcd->del_wk, HZ * secs_timeout);
> +	put_device(existing);
> +}
> +EXPORT_SYMBOL_GPL(dev_coredump_timeout_set);

Why don't we provide a new exported function like [1] as calling
dev_coredumpm() and just another function to configure the timeout
does not look good.

[1]

dev_coredump_timeout(..., timeout);
 __dev_coredump_timeout(...);

dev_coredumpm(..)
 __dev_coredump_timeout(..., DEVCD_TIMEOUT);

dev_coredumpv()
 __dev_coredump_timeout(..., DEVCD_TIMEOUT);

dev_coredumpsg()
  __dev_coredump_timeout(..., DEVCD_TIMEOUT);

-Mukesh

> +
>   /**
>    * dev_coredumpm - create device coredump with read/free methods
>    * @dev: the struct device for the crashed device
> diff --git a/include/linux/devcoredump.h b/include/linux/devcoredump.h
> index c8f7eb6cc1915..70fe72a5c6d36 100644
> --- a/include/linux/devcoredump.h
> +++ b/include/linux/devcoredump.h
> @@ -65,6 +65,7 @@ void dev_coredumpsg(struct device *dev, struct scatterlist *table,
>   		    size_t datalen, gfp_t gfp);
>   
>   void dev_coredump_put(struct device *dev);
> +void dev_coredump_timeout_set(struct device *dev, unsigned long secs_timeout);
>   #else
>   static inline void dev_coredumpv(struct device *dev, void *data,
>   				 size_t datalen, gfp_t gfp)
> @@ -90,6 +91,11 @@ static inline void dev_coredumpsg(struct device *dev, struct scatterlist *table,
>   static inline void dev_coredump_put(struct device *dev)
>   {
>   }
> +
> +static inline void dev_coredump_timeout_set(struct device *dev,
> +					    unsigned long secs_timeout)
> +{
> +}
>   #endif /* CONFIG_DEV_COREDUMP */
>   
>   #endif /* __DEVCOREDUMP_H */