lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <41a40700-ae78-42d9-8eab-72e0f1197792@roeck-us.net>
Date: Thu, 13 Jun 2024 12:53:59 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Thomas Richard <thomas.richard@...tlin.com>, wim@...ux-watchdog.org
Cc: linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org,
 michael.brunner@...tron.com, thomas.petazzoni@...tlin.com
Subject: Re: [PATCH] watchdog: kempld: use pretimeout member of
 watchdog_device struct

On 6/13/24 09:41, Thomas Richard wrote:
> Th driver was written whereas pretimeout wasn't supported by the watchdog
> core. So the pretimeout was stored in the driver data.
> 
> Now the core has pretimeout support, so use it.
> 
> Signed-off-by: Thomas Richard <thomas.richard@...tlin.com>
> ---
>   drivers/watchdog/kempld_wdt.c | 17 ++++++++---------
>   1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/watchdog/kempld_wdt.c b/drivers/watchdog/kempld_wdt.c
> index e6c7a2906680..ebb873502203 100644
> --- a/drivers/watchdog/kempld_wdt.c
> +++ b/drivers/watchdog/kempld_wdt.c
> @@ -73,7 +73,6 @@ struct kempld_wdt_stage {
>   struct kempld_wdt_data {
>   	struct kempld_device_data	*pld;
>   	struct watchdog_device		wdd;
> -	unsigned int			pretimeout;
>   	struct kempld_wdt_stage		stage[KEMPLD_WDT_MAX_STAGES];
>   	u8				pm_status_store;
>   };
> @@ -205,8 +204,8 @@ static int kempld_wdt_set_timeout(struct watchdog_device *wdd,
>   	timeout_stage = &wdt_data->stage[STAGE_TIMEOUT];
>   	pretimeout_stage = &wdt_data->stage[STAGE_PRETIMEOUT];
>   
> -	if (pretimeout_stage->mask && wdt_data->pretimeout > 0)
> -		timeout = wdt_data->pretimeout;
> +	if (pretimeout_stage->mask && wdd->pretimeout > 0)
> +		timeout = wdd->pretimeout;
>   
>   	ret = kempld_wdt_set_stage_action(wdt_data, timeout_stage,
>   						ACTION_RESET);
> @@ -249,13 +248,14 @@ static int kempld_wdt_set_pretimeout(struct watchdog_device *wdd,
>   	if (ret)
>   		return ret;
>   
> -	wdt_data->pretimeout = pretimeout;
> +	wdd->pretimeout = pretimeout;
>   	return 0;
>   }
>   
>   static void kempld_wdt_update_timeouts(struct kempld_wdt_data *wdt_data)
>   {
>   	struct kempld_device_data *pld = wdt_data->pld;
> +	struct watchdog_device *wdd = &wdt_data->wdd;
>   	struct kempld_wdt_stage *pretimeout_stage;
>   	struct kempld_wdt_stage *timeout_stage;
>   	unsigned int pretimeout, timeout;
> @@ -269,11 +269,11 @@ static void kempld_wdt_update_timeouts(struct kempld_wdt_data *wdt_data)
>   	kempld_release_mutex(pld);
>   
>   	if (pretimeout)
> -		wdt_data->pretimeout = timeout;
> +		wdd->pretimeout = timeout;
>   	else
> -		wdt_data->pretimeout = 0;
> +		wdd->pretimeout = 0;
>   
> -	wdt_data->wdd.timeout = pretimeout + timeout;
> +	wdd->timeout = pretimeout + timeout;
>   }
>   
>   static int kempld_wdt_start(struct watchdog_device *wdd)
> @@ -336,7 +336,6 @@ static int kempld_wdt_keepalive(struct watchdog_device *wdd)
>   static long kempld_wdt_ioctl(struct watchdog_device *wdd, unsigned int cmd,
>   				unsigned long arg)
>   {
> -	struct kempld_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>   	void __user *argp = (void __user *)arg;
>   	int ret = -ENOIOCTLCMD;
>   	int __user *p = argp;
> @@ -352,7 +351,7 @@ static long kempld_wdt_ioctl(struct watchdog_device *wdd, unsigned int cmd,
>   		ret = kempld_wdt_keepalive(wdd);
>   		break;
>   	case WDIOC_GETPRETIMEOUT:
> -		ret = put_user(wdt_data->pretimeout, (int __user *)arg);
> +		ret = put_user(wdd->pretimeout, (int __user *)arg);
>   		break;
>   	}
>   

Sorry, but this patch doesn't make sense to me unless the conversion
is complete. If the private ioctl functions are still necessary,
using the pretimeout variable from the watchdog core does not really
add any value. It just makes the code even more difficult to understand,
and sets a bad precedent.

Guenter


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ