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] [thread-next>] [day] [month] [year] [list]
Message-ID: <mfvuoslj27mbayypzr3wuagrq3p5wzelklgveedhwrxiaavwxy@7ipv2tup6oou>
Date: Wed, 3 Sep 2025 15:13:38 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: hrishabh.rajput@....qualcomm.com
Cc: Konrad Dybcio <konradybcio@...nel.org>, 
	Wim Van Sebroeck <wim@...ux-watchdog.org>, Guenter Roeck <linux@...ck-us.net>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, linux-arm-msm@...r.kernel.org, linux-watchdog@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] watchdog: Add driver for Gunyah Watchdog

On Wed, Sep 03, 2025 at 07:34:00PM +0000, Hrishabh Rajput via B4 Relay wrote:
> From: Hrishabh Rajput <hrishabh.rajput@....qualcomm.com>
> 
> Add driver to support the SMC-based watchdog timer provided by the
> Gunyah Hypervisor.
> 

Start the commit message with a problem description, end with a
technical description of the solution. I.e. move this paragraph down.

> On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
> through MMIO is not available. Depending on the hypervisor
> configuration, the watchdog is either fully emulated or exposed via
> ARM's SMC Calling Conventions (SMCCC) through the Vendor Specific
> Hypervisor Service Calls space.
> 
> When the SMC-based interface is enabled, a device tree overlay is used
> to provide the pretimeout interrupt configuration.
> 
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@....qualcomm.com>
[..]
> diff --git a/drivers/watchdog/gunyah_wdt.c b/drivers/watchdog/gunyah_wdt.c
[..]
> +#define GUNYAH_WDT_SMCCC_CALL_VAL(func_id) \
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32,\
> +			   ARM_SMCCC_OWNER_VENDOR_HYP, func_id)
> +
> +/* SMCCC function IDs for watchdog operations */
> +#define GUNYAH_WDT_CONTROL   GUNYAH_WDT_SMCCC_CALL_VAL(0x0005)
> +#define GUNYAH_WDT_STATUS    GUNYAH_WDT_SMCCC_CALL_VAL(0x0006)
> +#define GUNYAH_WDT_PING       GUNYAH_WDT_SMCCC_CALL_VAL(0x0007)

Uneven indentation.

> +#define GUNYAH_WDT_SET_TIME  GUNYAH_WDT_SMCCC_CALL_VAL(0x0008)
> +
> +/*
> + * Control values for GUNYAH_WDT_CONTROL.
> + * Bit 0 is used to enable or disable the watchdog. If this bit is set,
> + * then the watchdog is enabled and vice versa.
> + * Bit 1 should always be set to 1 as this bit is reserved in Gunyah and
> + * it's expected to be 1.
> + */
> +#define WDT_CTRL_ENABLE  (BIT(1) | BIT(0))
> +#define WDT_CTRL_DISABLE BIT(1)
> +
> +struct gunyah_wdt {
> +	unsigned int pretimeout_irq;

This is only used momentarily in gunyah_wdt_probe(), make it a local
variable.

> +	struct watchdog_device wdd;

Which means that gunyah_wdt is just watchdog_device, so you can drop
gunyah_wdt completely, and put wdd directly in drvdata.

> +};
> +
[..]
> +static int __init gunyah_wdt_init(void)
> +{
> +	return platform_driver_register(&gunyah_wdt_driver);
> +}
> +
> +module_init(gunyah_wdt_init);

module_platform_driver(gunyah_wdt_driver);

> +
> +MODULE_DESCRIPTION("Gunyah Watchdog Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/gunyah_errno.h b/include/linux/gunyah_errno.h
> new file mode 100644
> index 000000000000..518228e333bd
> --- /dev/null
> +++ b/include/linux/gunyah_errno.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */

Isn't this content solely used from within gunyah_wdt.c? Why is it a
separate header file? Just move it into the c-file.

Regards,
Bjorn

> +
> +#ifndef _LINUX_GUNYAH_ERRNO_H
> +#define _LINUX_GUNYAH_ERRNO_H
> +
> +#include <linux/errno.h>
> +
> +enum gunyah_error {
> +	GUNYAH_ERROR_OK				= 0,
> +	GUNYAH_ERROR_UNIMPLEMENTED		= -1,
> +	GUNYAH_ERROR_RETRY			= -2,
> +
> +	GUNYAH_ERROR_ARG_INVAL			= 1,
> +	GUNYAH_ERROR_ARG_SIZE			= 2,
> +	GUNYAH_ERROR_ARG_ALIGN			= 3,
> +
> +	GUNYAH_ERROR_NOMEM			= 10,
> +
> +	GUNYAH_ERROR_ADDR_OVFL			= 20,
> +	GUNYAH_ERROR_ADDR_UNFL			= 21,
> +	GUNYAH_ERROR_ADDR_INVAL			= 22,
> +
> +	GUNYAH_ERROR_DENIED			= 30,
> +	GUNYAH_ERROR_BUSY			= 31,
> +	GUNYAH_ERROR_IDLE			= 32,
> +
> +	GUNYAH_ERROR_IRQ_BOUND			= 40,
> +	GUNYAH_ERROR_IRQ_UNBOUND		= 41,
> +
> +	GUNYAH_ERROR_CSPACE_CAP_NULL		= 50,
> +	GUNYAH_ERROR_CSPACE_CAP_REVOKED		= 51,
> +	GUNYAH_ERROR_CSPACE_WRONG_OBJ_TYPE	= 52,
> +	GUNYAH_ERROR_CSPACE_INSUF_RIGHTS	= 53,
> +	GUNYAH_ERROR_CSPACE_FULL		= 54,
> +
> +	GUNYAH_ERROR_MSGQUEUE_EMPTY		= 60,
> +	GUNYAH_ERROR_MSGQUEUE_FULL		= 61,
> +};
> +
> +/**
> + * gunyah_error_remap() - Remap Gunyah hypervisor errors into a Linux error code
> + * @gunyah_error: Gunyah hypercall return value
> + */
> +static inline int gunyah_error_remap(enum gunyah_error gunyah_error)
> +{
> +	switch (gunyah_error) {
> +	case GUNYAH_ERROR_OK:
> +		return 0;
> +	case GUNYAH_ERROR_NOMEM:
> +		return -ENOMEM;
> +	case GUNYAH_ERROR_DENIED:
> +	case GUNYAH_ERROR_CSPACE_CAP_NULL:
> +	case GUNYAH_ERROR_CSPACE_CAP_REVOKED:
> +	case GUNYAH_ERROR_CSPACE_WRONG_OBJ_TYPE:
> +	case GUNYAH_ERROR_CSPACE_INSUF_RIGHTS:
> +	case GUNYAH_ERROR_CSPACE_FULL:
> +		return -EACCES;
> +	case GUNYAH_ERROR_BUSY:
> +	case GUNYAH_ERROR_IDLE:
> +		return -EBUSY;
> +	case GUNYAH_ERROR_IRQ_BOUND:
> +	case GUNYAH_ERROR_IRQ_UNBOUND:
> +	case GUNYAH_ERROR_MSGQUEUE_FULL:
> +	case GUNYAH_ERROR_MSGQUEUE_EMPTY:
> +		return -EIO;
> +	case GUNYAH_ERROR_UNIMPLEMENTED:
> +	case GUNYAH_ERROR_RETRY:
> +		return -EOPNOTSUPP;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +#endif
> 
> -- 
> 2.43.0
> 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ