Message-ID: <13c328c9-eb44-4a40-9f0e-734a84594bef@bootlin.com>
Date: Mon, 2 Sep 2024 15:38:23 +0200
From: Thomas Richard <thomas.richard@...tlin.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Lee Jones <lee@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>, Andi Shyti <andi.shyti@...nel.org>,
 Wim Van Sebroeck <wim@...ux-watchdog.org>, linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
 linux-watchdog@...r.kernel.org, thomas.petazzoni@...tlin.com,
 blake.vermeer@...sight.com
Subject: Re: [PATCH 4/5] watchdog: Congatec Board Controller watchdog timer
 driver

Hi Guenter,

>> +
>> +struct cgbc_wdt_cmd_cfg {
>> +	u8 cmd;
>> +	u8 mode;
>> +	u8 action;
>> +	u8 timeout1[3];
>> +	u8 timeout2[3];
>> +	u8 reserved[3];
>> +	u8 delay[3];
>> +} __packed;
>> +
>> +static_assert(sizeof(struct cgbc_wdt_cmd_cfg) == 15);
> 
> static_assert() is declared in linux/build_bug.h. Please include all
> necessary include files explicitly and do not depend on indirect includes.

Fixed in next iteration.

> 
>> +
>> +static int cgbc_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
> 
> Unusual way to get wdt_data instead of using container_of().
> Any special reason ?

No special reason, I saw that watchdog_get_drvdata() was often used in
watchdog drivers (more than container_of()) to get wdt_data.
But I can use container_of() if you think I should.

> 
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	unsigned int timeout1 = (wdd->timeout - wdd->pretimeout) * 1000;
>> +	unsigned int timeout2 = wdd->pretimeout * 1000;
>> +	u8 action;
>> +
>> +	struct cgbc_wdt_cmd_cfg cmd_start = {
>> +		.cmd = CGBC_WDT_CMD_INIT,
>> +		.mode = CGBC_WDT_MODE_SINGLE_EVENT,
>> +		.timeout1[0] = (u8)timeout1,
>> +		.timeout1[1] = (u8)(timeout1 >> 8),
>> +		.timeout1[2] = (u8)(timeout1 >> 16),
>> +		.timeout2[0] = (u8)timeout2,
>> +		.timeout2[1] = (u8)(timeout2 >> 8),
>> +		.timeout2[2] = (u8)(timeout2 >> 16),
>> +	};
>> +
>> +	if (wdd->pretimeout) {
>> +		action = 2;
>> +		action |= wdt_data->pretimeout_action << 2;
>> +		action |= wdt_data->timeout_action << 4;
>> +	} else {
>> +		action = 1;
>> +		action |= wdt_data->timeout_action << 2;
>> +	}
>> +
>> +	cmd_start.action = action;
>> +
>> +	return cgbc_command(cgbc, &cmd_start, sizeof(cmd_start), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_stop(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	struct cgbc_wdt_cmd_cfg cmd_stop = {
>> +		.cmd = CGBC_WDT_CMD_INIT,
>> +		.mode = CGBC_WDT_DISABLE,
>> +	};
>> +
>> +	return cgbc_command(cgbc, &cmd_stop, sizeof(cmd_stop), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_keepalive(struct watchdog_device *wdd)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +	struct cgbc_device_data *cgbc = wdt_data->cgbc;
>> +	u8 cmd_ping = CGBC_WDT_CMD_TRIGGER;
>> +
>> +	return cgbc_command(cgbc, &cmd_ping, sizeof(cmd_ping), NULL, 0, NULL);
>> +}
>> +
>> +static int cgbc_wdt_set_pretimeout(struct watchdog_device *wdd,
>> +				   unsigned int pretimeout)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +
>> +	wdd->pretimeout = pretimeout;
>> +	wdt_data->pretimeout_action = ACTION_SMI;
>> +
>> +	if (watchdog_active(wdd))
>> +		return cgbc_wdt_start(wdd);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cgbc_wdt_set_timeout(struct watchdog_device *wdd,
>> +				unsigned int timeout)
>> +{
>> +	struct cgbc_wdt_data *wdt_data = watchdog_get_drvdata(wdd);
>> +
>> +	if (timeout < wdd->pretimeout) {
>> +		dev_warn(wdd->parent, "timeout <= pretimeout. Setting pretimeout to zero\n");
> 
> That is a normal condition which does not warrant a log message.
> Also see drivers/watchdog/watchdog_dev.c around line 385.

Fixed in next iteration.

> 
>> +		wdd->pretimeout = 0;
>> +	}
>> +
>> +	wdd->timeout = timeout;
>> +	wdt_data->timeout_action = ACTION_RESET;
> 
> Both timeout_action and pretimeout_action are set statically.
> What is the point of doing that instead of just using
> ACTION_RESET and ACTION_SMI as needed irectly ?

Yes indeed, using ACTION_RESET and ACTION_SMI directly in
cgbc_wdt_start() makes the code smaller.

> 
>> +
>> +	if (watchdog_active(wdd))
>> +		return cgbc_wdt_start(wdd);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct watchdog_info cgbc_wdt_info = {
>> +	.identity	= "CGBC Watchdog",
>> +	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
>> +		WDIOF_MAGICCLOSE | WDIOF_PRETIMEOUT
>> +};
>> +
>> +static const struct watchdog_ops cgbc_wdt_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.start		= cgbc_wdt_start,
>> +	.stop		= cgbc_wdt_stop,
>> +	.ping		= cgbc_wdt_keepalive,
>> +	.set_timeout	= cgbc_wdt_set_timeout,
>> +	.set_pretimeout = cgbc_wdt_set_pretimeout,
>> +};
>> +
>> +static int cgbc_wdt_probe(struct platform_device *pdev)
>> +{
>> +	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
>> +	struct device *dev = &pdev->dev;
>> +	struct cgbc_wdt_data *wdt_data;
>> +	struct watchdog_device *wdd;
>> +	int ret;
>> +
>> +	wdt_data = devm_kzalloc(dev, sizeof(*wdt_data), GFP_KERNEL);
> 
> devm_kzalloc() is declared in linux/device.h. Again, please include all
> necessary include files explicitly.

Fixed in next iteration.

> 
>> +	if (!wdt_data)
>> +		return -ENOMEM;
>> +
>> +	wdt_data->cgbc = cgbc;
>> +	wdd = &wdt_data->wdd;
>> +	wdd->parent = dev;
>> +
> 
> No limits ? That is unusual. Are you sure the driver accepts all
> timeouts from 0 to UINT_MAX ?

Yes limits are needed.
For next iteration I added 1s as min_timeout (which seems to be the
usual value, and it is accepted by the hardware), and a max_timeout.

> 
>> +	wdd->info = &cgbc_wdt_info;
>> +	wdd->ops = &cgbc_wdt_ops;
>> +
>> +	watchdog_set_drvdata(wdd, wdt_data);
>> +	watchdog_set_nowayout(wdd, nowayout);
>> +
>> +	cgbc_wdt_set_timeout(wdd, timeout);
>> +	cgbc_wdt_set_pretimeout(wdd, pretimeout);
> 
> The more common approach would be to set default limits in wdd->{timout,pretimeout}
> and only override the values if needed, ie if provided using module parameters.
> That implies initializing the module parameters with 0. YOur call, though.

Ok.
For next iteration I added limits (min_timeout, max_timeout), the
timeout module parameter is set to 0 by default, and
watchdog_init_timeout() is called in the probe.

> 
>> +
>> +	platform_set_drvdata(pdev, wdt_data);
>> +	watchdog_stop_on_reboot(wdd);
>> +	watchdog_stop_on_unregister(wdd);
>> +
>> +	ret = devm_watchdog_register_device(dev, wdd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> Why not just
> 	return devm_watchdog_register_device(dev, wdd);
> ?

I don't know :)
Fixed in the next iteration.

Thanks for the review !!

Thomas.

Powered by blists - more mailing lists