Re: [PATCH V5 2/8] misc/pvpanic: Remove one extra semicolon

Re: [PATCH V5 2/8] misc/pvpanic: Remove one extra semicolon

[Mark Rutland]

On Thu, Nov 01, 2018 at 11:09:56PM +0800, Peng Hao wrote:
> There is an extra semicolon in pvpanic_device_ids, remove it.

This is a comma, not a semicolon.

How about:

  Since there should be nothing after the sentinel entry in an
  acpi_device_id list, it doesn't make sense for it to have a trailing
  comma.

  This patch removes the redundant comma.

... with that commit message (and the title fixed to say 'comma' rather
than 'semicolon':

Acked-by: Mark Rutland <mark.rutland@arm.com>

Either that, or simply drop this patch entirely.

Thanks,
Mark.

> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  drivers/misc/pvpanic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index fd86dab..059005c 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -35,7 +35,7 @@
>  
>  static const struct acpi_device_id pvpanic_device_ids[] = {
>  	{ "QEMU0001", 0 },
> -	{ "", 0 },
> +	{ "", 0 }
>  };
>  MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH V5 3/8] misc/pvpanic: revmove unnecessary header file

[Mark Rutland]

On Thu, Nov 01, 2018 at 11:09:57PM +0800, Peng Hao wrote:
> remove unnecessary header file init.h.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/misc/pvpanic.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index 059005c..66534d4 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -22,7 +22,6 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> -#include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/acpi.h>
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH V5 3/8] misc/pvpanic: revmove unnecessary header file

[Mark Rutland]

On Thu, Nov 01, 2018 at 11:34:29AM +0000, Mark Rutland wrote:
> On Thu, Nov 01, 2018 at 11:09:57PM +0800, Peng Hao wrote:
> > remove unnecessary header file init.h.
> > 
> > Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

... though please fix the title:

s/revmove/remove/

Mark.

Re: [PATCH V5 4/8] misc/pvpanic :convert to SPDX license tags

[Mark Rutland]

Hi,

Please fix the spacing for the colon in the commit title. It should be
formatted as:

	misc/pvpanic: convert to SPDX license tags

On Thu, Nov 01, 2018 at 11:09:58PM +0800, Peng Hao wrote:
> This patch updates license to use SPDX-License-Identifier
> instead of verbose license text.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>

Other than the commit title, this looks fine to me:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/misc/pvpanic.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index 66534d4..0bcf1cd 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -1,21 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
> - *  pvpanic.c - pvpanic Device Support
> + *  Pvpanic Device Support
>   *
>   *  Copyright (C) 2013 Fujitsu.
>   *
> - *  This program is free software; you can redistribute it and/or modify
> - *  it under the terms of the GNU General Public License as published by
> - *  the Free Software Foundation; either version 2 of the License, or
> - *  (at your option) any later version.
> - *
> - *  This program is distributed in the hope that it will be useful,
> - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - *  GNU General Public License for more details.
> - *
> - *  You should have received a copy of the GNU General Public License
> - *  along with this program; if not, write to the Free Software
> - *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -- 
> 1.8.3.1
> 

Re: [PATCH V5 6/8] misc/pvpanic: add MMIO support

[Mark Rutland]

On Thu, Nov 01, 2018 at 11:10:00PM +0800, Peng Hao wrote:
> On some architectures (e.g. arm64), it's preferable to use MMIO, since
> this can be used standalone. Add MMIO support to the pvpanic driver.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/misc/pvpanic.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index c20fdff..c0b787e 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -28,7 +28,7 @@
>  
>  #define PVPANIC_PANICKED	(1 << 0)
>  
> -static u16 port;
> +static void __iomem *base;
>  
>  static struct acpi_driver pvpanic_driver = {
>  	.name =		"pvpanic",
> @@ -44,7 +44,7 @@
>  static void
>  pvpanic_send_event(unsigned int event)
>  {
> -	outb(event, port);
> +	iowrite8(event, base);
>  }
>  
>  static int
> @@ -66,8 +66,14 @@
>  {
>  	struct resource r;
>  
> -	if (acpi_dev_resource_io(res, &r) {
> -		port = r.start;
> +	if (acpi_dev_resource_io(res, &r) ||
> +	    acpi_dev_resource_memory(res, &r)) {
> +		if (r.flags & IORESOURCE_IO)
> +			base = (void __iomem *) ioport_map(r.start,
> +						r.end - r.start + 1);
> +		else
> +			base = ioremap(r.start, r.end - r.start + 1);
> +
>  		return AE_OK;
>  	}
>  
> @@ -89,7 +95,7 @@ static int pvpanic_add(struct acpi_device *device)
>  	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
>  			    pvpanic_walk_resources, NULL);
>  
> -	if (!port)
> +	if (!base)
>  		return -ENODEV;
>  
>  	atomic_notifier_chain_register(&panic_notifier_list,
> @@ -103,6 +109,8 @@ static int pvpanic_remove(struct acpi_device *device)
>  
>  	atomic_notifier_chain_unregister(&panic_notifier_list,
>  					 &pvpanic_panic_nb);
> +
> +	iounmap(base);
>  	return 0;
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [PATCH V5 8/8] dt-bindings/misc/pvpanic :add document for pvpanic-mmio

[Mark Rutland]

Please fix the spacing in the commit title. It should be formatted:

  dt-bindings: misc/pvpanic: add document for pvpanic-mmio

On Thu, Nov 01, 2018 at 11:10:02PM +0800, Peng Hao wrote:
> Add dt-bindings document for "qemu:pvpanic-mmio".
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>

With the title fixed:

Acked-by: Mark Rutland <mark.rutland@arm.com>

When sending DT bindings in future, please follow the process in:

  Documentation/devicetree/bindings/submitting-patches.txt

... e.g., send the binding before the code using the binding.

Thanks,
Mark.

> ---
>  .../devicetree/bindings/misc/pvpanic-mmio.txt      | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/pvpanic-mmio.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/pvpanic-mmio.txt b/Documentation/devicetree/bindings/misc/pvpanic-mmio.txt
> new file mode 100644
> index 0000000..985e907
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/pvpanic-mmio.txt
> @@ -0,0 +1,29 @@
> +* QEMU PVPANIC MMIO Configuration bindings
> +
> +QEMU's emulation / virtualization targets provide the following PVPANIC
> +MMIO Configuration interface on the "virt" machine.
> +type:
> +
> +- a read-write, 16-bit wide data register.
> +
> +QEMU exposes the data register to guests as memory mapped registers.
> +
> +Required properties:
> +
> +- compatible: "qemu,pvpanic-mmio".
> +- reg: the MMIO region used by the device.
> +  * Bytes 0x0  Write panic event to the reg when guest OS panics.
> +  * Bytes 0x1  Reserved.
> +
> +Example:
> +
> +/ {
> +        #size-cells = <0x2>;
> +        #address-cells = <0x2>;
> +
> +        pvpanic-mmio@9060000 {
> +                compatible = "qemu,pvpanic-mmio";
> +                reg = <0x0 0x9060000 0x0 0x2>;
> +        };
> +};
> +
> -- 
> 1.8.3.1
> 

Re: [PATCH V5 7/8] misc/pvpanic: add support to get pvpanic device info by FDT

[Mark Rutland]

On Thu, Nov 01, 2018 at 11:10:01PM +0800, Peng Hao wrote:
> By default, when ACPI tables and FDT coexist for ARM64,
> current kernel takes precedence over FDT to get device information.
> Virt machine in qemu provides both FDT and ACPI table. This patch
> increases the way to get information through FDT.
> 
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  drivers/misc/pvpanic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index c0b787e..9d9f0d7 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -3,15 +3,18 @@
>   *  Pvpanic Device Support
>   *
>   *  Copyright (C) 2013 Fujitsu.
> - *
> + *  Copyright (C) 2018 ZTE.
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
> +#include <linux/acpi.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>
> -#include <linux/acpi.h>
>  
>  MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
>  MODULE_DESCRIPTION("pvpanic device driver");
> @@ -60,6 +63,32 @@
>  	.priority = 1, /* let this called before broken drm_fb_helper */
>  };
>  
> +static int pvpanic_mmio_probe(struct platform_device *pdev)
> +{
> +	struct resource *mem;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem)
> +		return -EINVAL;
> +
> +	base = devm_ioremap_resource(&pdev->dev, mem);
> +	if (base == NULL)
> +		return -EFAULT;
> +
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &pvpanic_panic_nb);
> +
> +	return 0;
> +}
> +
> +static int pvpanic_mmio_remove(struct platform_device *pdev)
> +{
> +
> +	atomic_notifier_chain_unregister(&panic_notifier_list,
> +					 &pvpanic_panic_nb);
> +
> +	return 0;
> +}
>  
>  static acpi_status
>  pvpanic_walk_resources(struct acpi_resource *res, void *context)
> @@ -114,4 +143,35 @@ static int pvpanic_remove(struct acpi_device *device)
>  	return 0;
>  }
>  
> -module_acpi_driver(pvpanic_driver);
> +static const struct of_device_id pvpanic_mmio_match[] = {
> +	{ .compatible = "qemu,pvpanic-mmio", },
> +	{}
> +};
> +
> +static struct platform_driver pvpanic_mmio_driver = {
> +	.driver = {
> +		.name = "pvpanic-mmio",
> +		.of_match_table = pvpanic_mmio_match,
> +	},
> +	.probe = pvpanic_mmio_probe,
> +	.remove = pvpanic_mmio_remove,
> +};
> +
> +static int __init pvpanic_mmio_init(void)
> +{
> +	if (acpi_disabled)
> +		return platform_driver_register(&pvpanic_mmio_driver);
> +	else
> +		return acpi_bus_register_driver(&pvpanic_driver);
> +}
> +
> +static void __exit pvpanic_mmio_exit(void)
> +{
> +	if (acpi_disabled)
> +		platform_driver_unregister(&pvpanic_mmio_driver);
> +	else
> +		acpi_bus_unregister_driver(&pvpanic_driver);
> +}
> +
> +module_init(pvpanic_mmio_init);
> +module_exit(pvpanic_mmio_exit);
> -- 
> 1.8.3.1
> 

Re: [PATCH V5 1/8] pvpanic: move pvpanic to misc as common driver

[Andy Shevchenko]

On Thu, Nov 1, 2018 at 8:59 AM Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> move pvpanic.c from drivers/platform/x86 to drivers/misc.
> following patches will use pvpanic device in arm64.

There are few items you need to address in the next revision:
- use proper English grammar, i.e. Capitalize beginning of sentenses
- don't forget the tags people gave you in case you didn't (much) change a patch

Other than that, this one looks good to me

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

P.S. WRT tags this one in v6 should have something like:
...

Reviewed-by: me
Acked-by: Mark
Signed-off-by: you

at the end of commit message

>
> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  drivers/misc/Kconfig                     | 8 +++++++
>  drivers/misc/Makefile                    | 1 +
>  drivers/{platform/x86 => misc}/pvpanic.c | 0
>  drivers/platform/x86/Kconfig             | 8 --------
>  drivers/platform/x86/Makefile            | 1 -
>  5 files changed, 9 insertions(+), 9 deletions(-)
>  rename drivers/{platform/x86 => misc}/pvpanic.c (100%)
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 3726eac..1ba0f62 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -513,6 +513,14 @@ config MISC_RTSX
>         tristate
>         default MISC_RTSX_PCI || MISC_RTSX_USB
>
> +config PVPANIC
> +       tristate "pvpanic device support"
> +       depends on ACPI || OF
> +       help
> +         This driver provides support for the pvpanic device.  pvpanic is
> +         a paravirtualized device provided by QEMU; it lets a virtual machine
> +         (guest) communicate panic events to the host.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index af22bbc..df951f4 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -58,3 +58,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)        += aspeed-lpc-snoop.o
>  obj-$(CONFIG_PCI_ENDPOINT_TEST)        += pci_endpoint_test.o
>  obj-$(CONFIG_OCXL)             += ocxl/
>  obj-$(CONFIG_MISC_RTSX)                += cardreader/
> +obj-$(CONFIG_PVPANIC)          += pvpanic.o
> diff --git a/drivers/platform/x86/pvpanic.c b/drivers/misc/pvpanic.c
> similarity index 100%
> rename from drivers/platform/x86/pvpanic.c
> rename to drivers/misc/pvpanic.c
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 0c1aa6c..4ac276c 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1121,14 +1121,6 @@ config INTEL_SMARTCONNECT
>           This driver checks to determine whether the device has Intel Smart
>           Connect enabled, and if so disables it.
>
> -config PVPANIC
> -       tristate "pvpanic device support"
> -       depends on ACPI
> -       ---help---
> -         This driver provides support for the pvpanic device.  pvpanic is
> -         a paravirtualized device provided by QEMU; it lets a virtual machine
> -         (guest) communicate panic events to the host.
> -
>  config INTEL_PMC_IPC
>         tristate "Intel PMC IPC Driver"
>         depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index e6d1bec..e88f44e 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -75,7 +75,6 @@ obj-$(CONFIG_APPLE_GMUX)      += apple-gmux.o
>  obj-$(CONFIG_INTEL_RST)                += intel-rst.o
>  obj-$(CONFIG_INTEL_SMARTCONNECT)       += intel-smartconnect.o
>
> -obj-$(CONFIG_PVPANIC)           += pvpanic.o
>  obj-$(CONFIG_ALIENWARE_WMI)    += alienware-wmi.o
>  obj-$(CONFIG_INTEL_PMC_IPC)    += intel_pmc_ipc.o
>  obj-$(CONFIG_TOUCHSCREEN_DMI)  += touchscreen_dmi.o
> --
> 1.8.3.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V5 2/8] misc/pvpanic: Remove one extra semicolon

[Andy Shevchenko]

On Thu, Nov 1, 2018 at 1:34 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Thu, Nov 01, 2018 at 11:09:56PM +0800, Peng Hao wrote:
> > There is an extra semicolon in pvpanic_device_ids, remove it.
>
> This is a comma, not a semicolon.
>
> How about:
>
>   Since there should be nothing after the sentinel entry in an
>   acpi_device_id list, it doesn't make sense for it to have a trailing
>   comma.
>
>   This patch removes the redundant comma.
>
> ... with that commit message (and the title fixed to say 'comma' rather
> than 'semicolon':

Agree on this, but...

>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> Either that, or simply drop this patch entirely.

...point was to gather style changes here, like moving linux/acpi.h.

And definitely the style changes should go last in the series.
If you decide to leave this patch, it would be 8/8.

>
> Thanks,
> Mark.
>
> >
> > Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> > ---
> >  drivers/misc/pvpanic.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> > index fd86dab..059005c 100644
> > --- a/drivers/misc/pvpanic.c
> > +++ b/drivers/misc/pvpanic.c
> > @@ -35,7 +35,7 @@
> >
> >  static const struct acpi_device_id pvpanic_device_ids[] = {
> >       { "QEMU0001", 0 },
> > -     { "", 0 },
> > +     { "", 0 }
> >  };
> >  MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
> >
> > --
> > 1.8.3.1
> >



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V5 3/8] misc/pvpanic: revmove unnecessary header file

[Andy Shevchenko]

On Thu, Nov 1, 2018 at 9:00 AM Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> remove unnecessary header file init.h.
>

Same comments as per patch 1, on top of this is ordering issue. This
patch should be 6/8.
Also, address Mark's comments, please.

Otherwise,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  drivers/misc/pvpanic.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index 059005c..66534d4 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -22,7 +22,6 @@
>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> -#include <linux/init.h>
>  #include <linux/types.h>
>  #include <linux/acpi.h>
>
> --
> 1.8.3.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V5 4/8] misc/pvpanic :convert to SPDX license tags

[Andy Shevchenko]

On Thu, Nov 1, 2018 at 9:00 AM Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> This patch updates license to use SPDX-License-Identifier
> instead of verbose license text.
>

After addressing Mark's comment consider to move this to be patch 7/8
in the series.
Otherwise,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  drivers/misc/pvpanic.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index 66534d4..0bcf1cd 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -1,21 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
> - *  pvpanic.c - pvpanic Device Support
> + *  Pvpanic Device Support
>   *
>   *  Copyright (C) 2013 Fujitsu.
>   *
> - *  This program is free software; you can redistribute it and/or modify
> - *  it under the terms of the GNU General Public License as published by
> - *  the Free Software Foundation; either version 2 of the License, or
> - *  (at your option) any later version.
> - *
> - *  This program is distributed in the hope that it will be useful,
> - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - *  GNU General Public License for more details.
> - *
> - *  You should have received a copy of the GNU General Public License
> - *  along with this program; if not, write to the Free Software
> - *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
>   */
>
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> --
> 1.8.3.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V5 5/8] misc/pvpanic: simplify the code using acpi_dev_resource_io

[Andy Shevchenko]

On Thu, Nov 1, 2018 at 9:00 AM Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> use acpi_dev_resource_io API.
>

Same comments as per patch 1.
This should be patch 2/8 in the series.

Otherwise,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Also, you might consider to use Suggested-by tag here.

> Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
> ---
>  drivers/misc/pvpanic.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
> index 0bcf1cd..c20fdff 100644
> --- a/drivers/misc/pvpanic.c
> +++ b/drivers/misc/pvpanic.c
> @@ -64,17 +64,15 @@
>  static acpi_status
>  pvpanic_walk_resources(struct acpi_resource *res, void *context)
>  {
> -       switch (res->type) {
> -       case ACPI_RESOURCE_TYPE_END_TAG:
> -               return AE_OK;
> +       struct resource r;
>
> -       case ACPI_RESOURCE_TYPE_IO:
> -               port = res->data.io.minimum;
> +       if (acpi_dev_resource_io(res, &r)) {
> +               port = r.start;
>                 return AE_OK;
> -
> -       default:
> -               return AE_ERROR;
>         }
> +
> +       return AE_ERROR;
> +
>  }
>
>  static int pvpanic_add(struct acpi_device *device)
> --
> 1.8.3.1
>


-- 
With Best Regards,
Andy Shevchenko

[PATCH V5 1/8] pvpanic: move pvpanic to misc as common driver

[Peng Hao]

move pvpanic.c from drivers/platform/x86 to drivers/misc.
following patches will use pvpanic device in arm64.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 drivers/misc/Kconfig                     | 8 +++++++
 drivers/misc/Makefile                    | 1 +
 drivers/{platform/x86 => misc}/pvpanic.c | 0
 drivers/platform/x86/Kconfig             | 8 --------
 drivers/platform/x86/Makefile            | 1 -
 5 files changed, 9 insertions(+), 9 deletions(-)
 rename drivers/{platform/x86 => misc}/pvpanic.c (100%)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 3726eac..1ba0f62 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -513,6 +513,14 @@ config MISC_RTSX
 	tristate
 	default MISC_RTSX_PCI || MISC_RTSX_USB
 
+config PVPANIC
+	tristate "pvpanic device support"
+	depends on ACPI || OF
+	help
+	  This driver provides support for the pvpanic device.  pvpanic is
+	  a paravirtualized device provided by QEMU; it lets a virtual machine
+	  (guest) communicate panic events to the host.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index af22bbc..df951f4 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -58,3 +58,4 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)	+= aspeed-lpc-snoop.o
 obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
 obj-$(CONFIG_OCXL)		+= ocxl/
 obj-$(CONFIG_MISC_RTSX)		+= cardreader/
+obj-$(CONFIG_PVPANIC)   	+= pvpanic.o
diff --git a/drivers/platform/x86/pvpanic.c b/drivers/misc/pvpanic.c
similarity index 100%
rename from drivers/platform/x86/pvpanic.c
rename to drivers/misc/pvpanic.c
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 0c1aa6c..4ac276c 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1121,14 +1121,6 @@ config INTEL_SMARTCONNECT
 	  This driver checks to determine whether the device has Intel Smart
 	  Connect enabled, and if so disables it.
 
-config PVPANIC
-	tristate "pvpanic device support"
-	depends on ACPI
-	---help---
-	  This driver provides support for the pvpanic device.  pvpanic is
-	  a paravirtualized device provided by QEMU; it lets a virtual machine
-	  (guest) communicate panic events to the host.
-
 config INTEL_PMC_IPC
 	tristate "Intel PMC IPC Driver"
 	depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e6d1bec..e88f44e 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -75,7 +75,6 @@ obj-$(CONFIG_APPLE_GMUX)	+= apple-gmux.o
 obj-$(CONFIG_INTEL_RST)		+= intel-rst.o
 obj-$(CONFIG_INTEL_SMARTCONNECT)	+= intel-smartconnect.o
 
-obj-$(CONFIG_PVPANIC)           += pvpanic.o
 obj-$(CONFIG_ALIENWARE_WMI)	+= alienware-wmi.o
 obj-$(CONFIG_INTEL_PMC_IPC)	+= intel_pmc_ipc.o
 obj-$(CONFIG_TOUCHSCREEN_DMI)	+= touchscreen_dmi.o
-- 
1.8.3.1

[PATCH V5 2/8] misc/pvpanic: Remove one extra semicolon

[Peng Hao]

There is an extra semicolon in pvpanic_device_ids, remove it.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 drivers/misc/pvpanic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
index fd86dab..059005c 100644
--- a/drivers/misc/pvpanic.c
+++ b/drivers/misc/pvpanic.c
@@ -35,7 +35,7 @@
 
 static const struct acpi_device_id pvpanic_device_ids[] = {
 	{ "QEMU0001", 0 },
-	{ "", 0 },
+	{ "", 0 }
 };
 MODULE_DEVICE_TABLE(acpi, pvpanic_device_ids);
 
-- 
1.8.3.1

[PATCH V5 3/8] misc/pvpanic: revmove unnecessary header file

[Peng Hao]

remove unnecessary header file init.h.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 drivers/misc/pvpanic.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
index 059005c..66534d4 100644
--- a/drivers/misc/pvpanic.c
+++ b/drivers/misc/pvpanic.c
@@ -22,7 +22,6 @@
 
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/init.h>
 #include <linux/types.h>
 #include <linux/acpi.h>
 
-- 
1.8.3.1

[PATCH V5 4/8] misc/pvpanic :convert to SPDX license tags

[Peng Hao]

This patch updates license to use SPDX-License-Identifier
instead of verbose license text.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 drivers/misc/pvpanic.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
index 66534d4..0bcf1cd 100644
--- a/drivers/misc/pvpanic.c
+++ b/drivers/misc/pvpanic.c
@@ -1,21 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
- *  pvpanic.c - pvpanic Device Support
+ *  Pvpanic Device Support
  *
  *  Copyright (C) 2013 Fujitsu.
  *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-- 
1.8.3.1

[PATCH V5 5/8] misc/pvpanic: simplify the code using acpi_dev_resource_io

[Peng Hao]

use acpi_dev_resource_io API.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 drivers/misc/pvpanic.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
index 0bcf1cd..c20fdff 100644
--- a/drivers/misc/pvpanic.c
+++ b/drivers/misc/pvpanic.c
@@ -64,17 +64,15 @@
 static acpi_status
 pvpanic_walk_resources(struct acpi_resource *res, void *context)
 {
-	switch (res->type) {
-	case ACPI_RESOURCE_TYPE_END_TAG:
-		return AE_OK;
+	struct resource r;
 
-	case ACPI_RESOURCE_TYPE_IO:
-		port = res->data.io.minimum;
+	if (acpi_dev_resource_io(res, &r)) {
+		port = r.start;
 		return AE_OK;
-
-	default:
-		return AE_ERROR;
 	}
+
+	return AE_ERROR;
+
 }
 
 static int pvpanic_add(struct acpi_device *device)
-- 
1.8.3.1

[PATCH V5 6/8] misc/pvpanic: add MMIO support

[Peng Hao]

On some architectures (e.g. arm64), it's preferable to use MMIO, since
this can be used standalone. Add MMIO support to the pvpanic driver.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 drivers/misc/pvpanic.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
index c20fdff..c0b787e 100644
--- a/drivers/misc/pvpanic.c
+++ b/drivers/misc/pvpanic.c
@@ -28,7 +28,7 @@
 
 #define PVPANIC_PANICKED	(1 << 0)
 
-static u16 port;
+static void __iomem *base;
 
 static struct acpi_driver pvpanic_driver = {
 	.name =		"pvpanic",
@@ -44,7 +44,7 @@
 static void
 pvpanic_send_event(unsigned int event)
 {
-	outb(event, port);
+	iowrite8(event, base);
 }
 
 static int
@@ -66,8 +66,14 @@
 {
 	struct resource r;
 
-	if (acpi_dev_resource_io(res, &r) {
-		port = r.start;
+	if (acpi_dev_resource_io(res, &r) ||
+	    acpi_dev_resource_memory(res, &r)) {
+		if (r.flags & IORESOURCE_IO)
+			base = (void __iomem *) ioport_map(r.start,
+						r.end - r.start + 1);
+		else
+			base = ioremap(r.start, r.end - r.start + 1);
+
 		return AE_OK;
 	}
 
@@ -89,7 +95,7 @@ static int pvpanic_add(struct acpi_device *device)
 	acpi_walk_resources(device->handle, METHOD_NAME__CRS,
 			    pvpanic_walk_resources, NULL);
 
-	if (!port)
+	if (!base)
 		return -ENODEV;
 
 	atomic_notifier_chain_register(&panic_notifier_list,
@@ -103,6 +109,8 @@ static int pvpanic_remove(struct acpi_device *device)
 
 	atomic_notifier_chain_unregister(&panic_notifier_list,
 					 &pvpanic_panic_nb);
+
+	iounmap(base);
 	return 0;
 }
 
-- 
1.8.3.1

[PATCH V5 7/8] misc/pvpanic: add support to get pvpanic device info by FDT

[Peng Hao]

By default, when ACPI tables and FDT coexist for ARM64,
current kernel takes precedence over FDT to get device information.
Virt machine in qemu provides both FDT and ACPI table. This patch
increases the way to get information through FDT.

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 drivers/misc/pvpanic.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/pvpanic.c b/drivers/misc/pvpanic.c
index c0b787e..9d9f0d7 100644
--- a/drivers/misc/pvpanic.c
+++ b/drivers/misc/pvpanic.c
@@ -3,15 +3,18 @@
  *  Pvpanic Device Support
  *
  *  Copyright (C) 2013 Fujitsu.
- *
+ *  Copyright (C) 2018 ZTE.
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/acpi.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>
-#include <linux/acpi.h>
 
 MODULE_AUTHOR("Hu Tao <hutao@cn.fujitsu.com>");
 MODULE_DESCRIPTION("pvpanic device driver");
@@ -60,6 +63,32 @@
 	.priority = 1, /* let this called before broken drm_fb_helper */
 };
 
+static int pvpanic_mmio_probe(struct platform_device *pdev)
+{
+	struct resource *mem;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem)
+		return -EINVAL;
+
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (base == NULL)
+		return -EFAULT;
+
+	atomic_notifier_chain_register(&panic_notifier_list,
+				       &pvpanic_panic_nb);
+
+	return 0;
+}
+
+static int pvpanic_mmio_remove(struct platform_device *pdev)
+{
+
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &pvpanic_panic_nb);
+
+	return 0;
+}
 
 static acpi_status
 pvpanic_walk_resources(struct acpi_resource *res, void *context)
@@ -114,4 +143,35 @@ static int pvpanic_remove(struct acpi_device *device)
 	return 0;
 }
 
-module_acpi_driver(pvpanic_driver);
+static const struct of_device_id pvpanic_mmio_match[] = {
+	{ .compatible = "qemu,pvpanic-mmio", },
+	{}
+};
+
+static struct platform_driver pvpanic_mmio_driver = {
+	.driver = {
+		.name = "pvpanic-mmio",
+		.of_match_table = pvpanic_mmio_match,
+	},
+	.probe = pvpanic_mmio_probe,
+	.remove = pvpanic_mmio_remove,
+};
+
+static int __init pvpanic_mmio_init(void)
+{
+	if (acpi_disabled)
+		return platform_driver_register(&pvpanic_mmio_driver);
+	else
+		return acpi_bus_register_driver(&pvpanic_driver);
+}
+
+static void __exit pvpanic_mmio_exit(void)
+{
+	if (acpi_disabled)
+		platform_driver_unregister(&pvpanic_mmio_driver);
+	else
+		acpi_bus_unregister_driver(&pvpanic_driver);
+}
+
+module_init(pvpanic_mmio_init);
+module_exit(pvpanic_mmio_exit);
-- 
1.8.3.1

[PATCH V5 8/8] dt-bindings/misc/pvpanic :add document for pvpanic-mmio

[Peng Hao]

Add dt-bindings document for "qemu:pvpanic-mmio".

Signed-off-by: Peng Hao <peng.hao2@zte.com.cn>
---
 .../devicetree/bindings/misc/pvpanic-mmio.txt      | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/pvpanic-mmio.txt

diff --git a/Documentation/devicetree/bindings/misc/pvpanic-mmio.txt b/Documentation/devicetree/bindings/misc/pvpanic-mmio.txt
new file mode 100644
index 0000000..985e907
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/pvpanic-mmio.txt
@@ -0,0 +1,29 @@
+* QEMU PVPANIC MMIO Configuration bindings
+
+QEMU's emulation / virtualization targets provide the following PVPANIC
+MMIO Configuration interface on the "virt" machine.
+type:
+
+- a read-write, 16-bit wide data register.
+
+QEMU exposes the data register to guests as memory mapped registers.
+
+Required properties:
+
+- compatible: "qemu,pvpanic-mmio".
+- reg: the MMIO region used by the device.
+  * Bytes 0x0  Write panic event to the reg when guest OS panics.
+  * Bytes 0x1  Reserved.
+
+Example:
+
+/ {
+        #size-cells = <0x2>;
+        #address-cells = <0x2>;
+
+        pvpanic-mmio@9060000 {
+                compatible = "qemu,pvpanic-mmio";
+                reg = <0x0 0x9060000 0x0 0x2>;
+        };
+};
+
-- 
1.8.3.1

Re: [PATCH V5 6/8] misc/pvpanic: add MMIO support

[Andy Shevchenko]

On Thu, Nov 1, 2018 at 9:00 AM Peng Hao <peng.hao2@zte.com.cn> wrote:
>
> On some architectures (e.g. arm64), it's preferable to use MMIO, since
> this can be used standalone. Add MMIO support to the pvpanic driver.

> -       if (acpi_dev_resource_io(res, &r) {
> -               port = r.start;
> +       if (acpi_dev_resource_io(res, &r) ||
> +           acpi_dev_resource_memory(res, &r)) {


> +               if (r.flags & IORESOURCE_IO)

You need spend some time to understand better how acpi_dev_resource_*() works.
Hint: this condition is redundant.

> +                       base = (void __iomem *) ioport_map(r.start,
> +                                               r.end - r.start + 1);
> +               else
> +                       base = ioremap(r.start, r.end - r.start + 1);
> +


-- 
With Best Regards,
Andy Shevchenko