Re: [PATCH v5 3/3] misc/pvpanic: add PCI driver

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Mar 16, 2021 at 02:20:29PM +0200, Mihai Carabas wrote:
> Add support for pvpanic PCI device added in qemu [1]. At probe time, obtain the
> address where to read/write pvpanic events and pass it to the generic handling
> code. Will follow the same logic as pvpanic MMIO device driver. At remove time,
> unmap base address and disable PCI device.
> 
> [1] https://github.com/qemu/qemu/commit/9df52f58e76e904fb141b10318362d718f470db2
> 
> Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
> ---
>  drivers/misc/pvpanic/Kconfig       |   6 +++
>  drivers/misc/pvpanic/Makefile      |   1 +
>  drivers/misc/pvpanic/pvpanic-pci.c | 102 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 109 insertions(+)
>  create mode 100644 drivers/misc/pvpanic/pvpanic-pci.c
> 
> diff --git a/drivers/misc/pvpanic/Kconfig b/drivers/misc/pvpanic/Kconfig
> index 454f1ee..9a081ed 100644
> --- a/drivers/misc/pvpanic/Kconfig
> +++ b/drivers/misc/pvpanic/Kconfig
> @@ -17,3 +17,9 @@ config PVPANIC_MMIO
>  	depends on HAS_IOMEM && (ACPI || OF) && PVPANIC
>  	help
>  	  This driver provides support for the MMIO pvpanic device.
> +
> +config PVPANIC_PCI
> +	tristate "pvpanic PCI device support"
> +	depends on PCI && PVPANIC
> +	help
> +	  This driver provides support for the PCI pvpanic device.

Please provide a bit more information here.

> diff --git a/drivers/misc/pvpanic/Makefile b/drivers/misc/pvpanic/Makefile
> index e12a2dc..9471df7 100644
> --- a/drivers/misc/pvpanic/Makefile
> +++ b/drivers/misc/pvpanic/Makefile
> @@ -5,3 +5,4 @@
>  # Copyright (C) 2021 Oracle.
>  #
>  obj-$(CONFIG_PVPANIC_MMIO)	+= pvpanic.o pvpanic-mmio.o
> +obj-$(CONFIG_PVPANIC_PCI)	+= pvpanic.o pvpanic-pci.o
> diff --git a/drivers/misc/pvpanic/pvpanic-pci.c b/drivers/misc/pvpanic/pvpanic-pci.c
> new file mode 100644
> index 00000000..27526d3
> --- /dev/null
> +++ b/drivers/misc/pvpanic/pvpanic-pci.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  Pvpanic PCI Device Support
> + *
> + *  Copyright (C) 2021 Oracle.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +#include "pvpanic.h"
> +
> +#define PCI_VENDOR_ID_REDHAT             0x1b36
> +#define PCI_DEVICE_ID_REDHAT_PVPANIC     0x0011
> +
> +static void __iomem *base;
> +static const struct pci_device_id pvpanic_pci_id_tbl[]  = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_REDHAT, PCI_DEVICE_ID_REDHAT_PVPANIC),},

Why the ")}"?

Shouldn't it just be "}"?

> +	{}
> +};
> +static unsigned int capability = PVPANIC_PANICKED | PVPANIC_CRASH_LOADED;
> +static unsigned int events;
> +
> +static ssize_t capability_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%x\n", capability);

A global capability for all devices?  No, this needs to be a per-device
attttribute as you are showing it to userspace as such.

> +}
> +static DEVICE_ATTR_RO(capability);
> +
> +static ssize_t events_show(struct device *dev,  struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%x\n", events);

Same here, this is not global for the whole module.

> +}
> +
> +static ssize_t events_store(struct device *dev,  struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	unsigned int tmp;
> +	int err;
> +
> +	err = kstrtouint(buf, 16, &tmp);
> +	if (err)
> +		return err;
> +
> +	if ((tmp & capability) != tmp)
> +		return -EINVAL;
> +
> +	events = tmp;
> +
> +	pvpanic_set_events(base, events);
> +
> +	return count;
> +
> +}
> +static DEVICE_ATTR_RW(events);
> +
> +static struct attribute *pvpanic_pci_dev_attrs[] = {
> +	&dev_attr_capability.attr,
> +	&dev_attr_events.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(pvpanici_pci_dev);

You did not document these new sysfs files in Documentation/ABI/ so it's
hard to judge what they do.  Please do so next version.

thanks,

greg k-h