Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Sat, 4 Nov 2023 14:05:02 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Thomas =?iso-8859-1?Q?Wei=DFschuh?= <linux@weissschuh.net>
Cc:     Arnd Bergmann <arnd@arndb.de>, linux-kernel@vger.kernel.org,
        Zhangjin Wu <falcon@tinylab.org>, Willy Tarreau <w@1wt.eu>,
        Yuan Tan <tanyuan@tinylab.org>
Subject: Re: [PATCH RFC] misc/pvpanic: add support for normal shutdowns
Message-ID: <2023110407-unselect-uptight-b96d@gregkh>
References: <20231104-pvpanic-shutdown-v1-1-5ee7c9b3e301@weissschuh.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20231104-pvpanic-shutdown-v1-1-5ee7c9b3e301@weissschuh.net>
Precedence: bulk

On Sat, Nov 04, 2023 at 12:29:30PM +0100, Thomas Wei?schuh wrote:
> Shutdown requests are normally hardware dependent.
> By extending pvpanic to also handle shutdown requests, guests can
> submit such requests with an easily implementable and cross-platform
> mechanism.
> 
> Signed-off-by: Thomas Wei?schuh <linux@weissschuh.net>
> ---
> The corresponding patch to qemu has also been submitted[0].
> General discussions about the feature should happen on the other thread.
> 
> [0] https://lore.kernel.org/qemu-devel/20231104-pvpanic-shutdown-v1-0-02353157891b@t-8ch.de/
> ---
>  drivers/misc/pvpanic/pvpanic.c | 19 +++++++++++++++++--
>  include/uapi/misc/pvpanic.h    |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c
> index 305b367e0ce3..d7d807f5e47a 100644
> --- a/drivers/misc/pvpanic/pvpanic.c
> +++ b/drivers/misc/pvpanic/pvpanic.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/panic_notifier.h>
> +#include <linux/reboot.h>
>  #include <linux/types.h>
>  #include <linux/cdev.h>
>  #include <linux/list.h>
> @@ -74,6 +75,13 @@ static struct notifier_block pvpanic_panic_nb = {
>  	.priority = INT_MAX,
>  };
>  
> +static int pvpanic_sys_off(struct sys_off_data *data)
> +{
> +	pvpanic_send_event(PVPANIC_SHUTDOWN);
> +
> +	return NOTIFY_DONE;
> +}
> +
>  static void pvpanic_remove(void *param)
>  {
>  	struct pvpanic_instance *pi_cur, *pi_next;
> @@ -152,7 +160,7 @@ int devm_pvpanic_probe(struct device *dev, void __iomem *base)
>  		return -ENOMEM;
>  
>  	pi->base = base;
> -	pi->capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
> +	pi->capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED | PVPANIC_SHUTDOWN;
>  
>  	/* initlize capability by RDPT */
>  	pi->capability &= ioread8(base);
> @@ -168,12 +176,18 @@ int devm_pvpanic_probe(struct device *dev, void __iomem *base)
>  }
>  EXPORT_SYMBOL_GPL(devm_pvpanic_probe);
>  
> +static struct sys_off_handler *sys_off_handler;
> +
>  static int pvpanic_init(void)
>  {
>  	INIT_LIST_HEAD(&pvpanic_list);
>  	spin_lock_init(&pvpanic_lock);
>  
>  	atomic_notifier_chain_register(&panic_notifier_list, &pvpanic_panic_nb);
> +	sys_off_handler = register_sys_off_handler(SYS_OFF_MODE_POWER_OFF, SYS_OFF_PRIO_DEFAULT,
> +						   pvpanic_sys_off, NULL);
> +	if (IS_ERR(sys_off_handler))
> +		sys_off_handler = NULL;

Why are you allowing this to succeed?  Shouldn't you be returning the
error instead?

>  	return 0;
>  }
> @@ -182,6 +196,7 @@ module_init(pvpanic_init);
>  static void pvpanic_exit(void)
>  {
>  	atomic_notifier_chain_unregister(&panic_notifier_list, &pvpanic_panic_nb);
> -
> +	if (sys_off_handler)
> +		unregister_sys_off_handler(sys_off_handler);
>  }
>  module_exit(pvpanic_exit);
> diff --git a/include/uapi/misc/pvpanic.h b/include/uapi/misc/pvpanic.h
> index 54b7485390d3..82fc618bfbcf 100644
> --- a/include/uapi/misc/pvpanic.h
> +++ b/include/uapi/misc/pvpanic.h
> @@ -5,5 +5,6 @@
>  
>  #define PVPANIC_PANICKED	(1 << 0)
>  #define PVPANIC_CRASH_LOADED	(1 << 1)
> +#define PVPANIC_SHUTDOWN	(1 << 2)

Why are these in a uapi file?

And if they need to be here, why not use the proper BIT() macro for it?

thanks,

greg k-h