linux-kernel - Re: [PATCH 2/6] misc/pvpanic: Add pvpanic driver framework

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: <20190122093732.GA29657@kroah.com>
Date:   Tue, 22 Jan 2019 10:37:32 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Peng Hao <peng.hao2@....com.cn>
Cc:     arnd@...db.de, andy.shevchenko@...il.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/6]  misc/pvpanic: Add pvpanic driver framework

On Tue, Jan 22, 2019 at 03:25:07AM +0800, Peng Hao wrote:
> Add pvpanic driver framework.
> 

You need a lot more description of what you did here than this, as I can
not understand from this text, what the patch does, or more importantly,
why it is doing this, at all.



> Signed-off-by: Peng Hao <peng.hao2@....com.cn>
> ---
>  drivers/misc/pvpanic/pvpanic.c | 171 ++++++++++-------------------------------
>  1 file changed, 39 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic/pvpanic.c b/drivers/misc/pvpanic/pvpanic.c
> index 595ac06..6380540 100644
> --- a/drivers/misc/pvpanic/pvpanic.c
> +++ b/drivers/misc/pvpanic/pvpanic.c
> @@ -8,15 +8,20 @@
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> -#include <linux/acpi.h>
> +#include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
>  #include <linux/platform_device.h>
>  #include <linux/types.h>
>  
> -static void __iomem *base;
> +static struct {
> +	struct platform_device *pdev;
> +	void __iomem *base;
> +	bool is_ioport;
> +} pvpanic_data = {
> +	.pdev = NULL,
> +	.is_ioport = false,

You do not need to initialize variables to 0 specifically like this.

> +};
>  
>  #define PVPANIC_PANICKED        (1 << 0)
>  
> @@ -27,7 +32,7 @@
>  static void
>  pvpanic_send_event(unsigned int event)
>  {
> -	iowrite8(event, base);
> +	iowrite8(event, pvpanic_data.base);

Why did you convert a single global variable into a single global
structure?  Why not, if you really need to pass this value around, do
that at the same time as you will end up touching these same functions
again, right?

thanks,

greg k-h