Re: [RFC] staging/vSMP: new driver

[Dan Carpenter <dan.carpenter@oracle.com>]

"new driver" is too vague.  What is the driver for?

You need a README or a TODO which explains how to move the driver out of
staging and into the main kernel.

Run the patch through scripts/checkpatch.pl --strict.  You don't have
to fix everything because this is staging but a bunch of the style
issues are easy to fix so we may as well just do it.

There are a number of bugs and style issues.
1) Find and replace every -1 with a define or a proper error code
2) Never do assignments inside of a condition
3) Do success handling instead of error handling.  Try to return errors
   as directly as possible.

Bad:
	ret = frob();
	if (!ret)
		printk("success!");
	return ret;

Good:
	ret = frob();
	if (ret)
		return ret;
	printk("success!");
	return 0;

Keep scrolling down for the details.

On Wed, Mar 16, 2022 at 06:13:04PM +0000, Czerwacki, Eial wrote:
> Introducing the vSMP guest driver which allows interaction with the vSMP control device when
> running a Linux OS atop of the vSMP hypervisor.
> vSMP is a resource aggregation hypervisor from SAP.
> 
> the driver comprises of 3 modules, vsmp which includes all the api needed to interact with the
> control driver, vsmp_logs which allows reading logs from the hypervisor and vsmp_common_info which
> allows reading generic information the hypervisor exposes, currently only the version is exposed.
> 
> Signed-off-by: Eial Czerwacki <eial.czerwacki@sap.com>
> ---
>  MAINTAINERS                             |   6 +
>  drivers/staging/Kconfig                 |   2 +
>  drivers/staging/Makefile                |   1 +
>  drivers/staging/vsmp/Kconfig            |  14 +
>  drivers/staging/vsmp/Makefile           |   7 +
>  drivers/staging/vsmp/api.c              | 402 ++++++++++++++++++++++++
>  drivers/staging/vsmp/api.h              |  61 ++++
>  drivers/staging/vsmp/common/Kconfig     |  11 +
>  drivers/staging/vsmp/common/Makefile    |   7 +
>  drivers/staging/vsmp/common/common.c    |  64 ++++
>  drivers/staging/vsmp/common/common.h    |  27 ++
>  drivers/staging/vsmp/common/version.c   |  85 +++++
>  drivers/staging/vsmp/logs/Kconfig       |  10 +
>  drivers/staging/vsmp/logs/Makefile      |   7 +
>  drivers/staging/vsmp/logs/active_logs.c | 112 +++++++
>  drivers/staging/vsmp/registers.h        |  16 +
>  16 files changed, 832 insertions(+)
>  create mode 100644 drivers/staging/vsmp/Kconfig
>  create mode 100644 drivers/staging/vsmp/Makefile
>  create mode 100644 drivers/staging/vsmp/api.c
>  create mode 100644 drivers/staging/vsmp/api.h
>  create mode 100644 drivers/staging/vsmp/common/Kconfig
>  create mode 100644 drivers/staging/vsmp/common/Makefile
>  create mode 100644 drivers/staging/vsmp/common/common.c
>  create mode 100644 drivers/staging/vsmp/common/common.h
>  create mode 100644 drivers/staging/vsmp/common/version.c
>  create mode 100644 drivers/staging/vsmp/logs/Kconfig
>  create mode 100644 drivers/staging/vsmp/logs/Makefile
>  create mode 100644 drivers/staging/vsmp/logs/active_logs.c
>  create mode 100644 drivers/staging/vsmp/registers.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 68f21d46614c..2fc61b4d13e5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18381,6 +18381,12 @@ F:	Documentation/core-api/printk-formats.rst
>  F:	lib/test_printf.c
>  F:	lib/vsprintf.c
>  
> +VSMP GUEST DRIVER
> +M:     Eial Czerwacki <eial.czerwacki@sap.com>
> +M:     linux-vsmp@sap.com
> +S:     Maintained
> +F:     drivers/staging/vsmp-guest 
> +
>  VT1211 HARDWARE MONITOR DRIVER
>  M:	Juerg Haefliger <juergh@gmail.com>
>  L:	linux-hwmon@vger.kernel.org
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index 4ec5528f89fa..f61778bccb0b 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -114,6 +114,8 @@ source "drivers/staging/axis-fifo/Kconfig"
>  
>  source "drivers/staging/fieldbus/Kconfig"
>  
> +source "drivers/staging/vsmp/Kconfig"
> +
>  source "drivers/staging/kpc2000/Kconfig"
>  
>  source "drivers/staging/qlge/Kconfig"
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 4d34198151b3..726e704ba8bc 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_SOC_MT7621)	+= mt7621-dts/
>  obj-$(CONFIG_STAGING_GASKET_FRAMEWORK)	+= gasket/
>  obj-$(CONFIG_XIL_AXIS_FIFO)	+= axis-fifo/
>  obj-$(CONFIG_FIELDBUS_DEV)     += fieldbus/
> +obj-$(CONFIG_VSMP)       += vsmp/
>  obj-$(CONFIG_KPC2000)		+= kpc2000/
>  obj-$(CONFIG_QLGE)		+= qlge/
>  obj-$(CONFIG_WFX)		+= wfx/
> diff --git a/drivers/staging/vsmp/Kconfig b/drivers/staging/vsmp/Kconfig
> new file mode 100644
> index 000000000000..c57cfcb55033
> --- /dev/null
> +++ b/drivers/staging/vsmp/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +menuconfig VSMP
> +	tristate "vSMP Guest Support"
> +	depends on SYS_HYPERVISOR && X86_64 && PCI
> +	help
> +	  Support for vSMP Guest Driver.
> +
> +	  this drivers allows information gathering of data from the vSMP hypervisor when
> +	  running ontop of a vSMP based VM.
> +
> +	  If unsure, say no.
> +
> +source "drivers/staging/vsmp/common/Kconfig"
> +source "drivers/staging/vsmp/logs/Kconfig"
> diff --git a/drivers/staging/vsmp/Makefile b/drivers/staging/vsmp/Makefile
> new file mode 100644
> index 000000000000..88625cd3707d
> --- /dev/null
> +++ b/drivers/staging/vsmp/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for vSMP Guest drivers.
> +#
> +
> +obj-$(CONFIG_VSMP)	+= vsmp.o common/ logs/
> +vsmp-y			:= api.o
> diff --git a/drivers/staging/vsmp/api.c b/drivers/staging/vsmp/api.c
> new file mode 100644
> index 000000000000..b0d4b5a990d4
> --- /dev/null
> +++ b/drivers/staging/vsmp/api.c
> @@ -0,0 +1,402 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP api
> + * Copyright (C) 2022 SAP.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/pci_regs.h>
> +#include <linux/pci.h>
> +#include <linux/kobject.h>
> +#include <linux/kernel.h>
> +
> +#include <asm/io.h>
> +
> +#include "api.h"
> +
> +#define DRIVER_LICENSE "GPL"
> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> +#define DRIVER_DESC "vSMP api driver"
> +#define DRIVER_VERSION "0.1"
> +
> +#define PCI_DEVICE_ID_SAP_FLX_VSMP_CTL 0x1011
> +#define VSMP_CTL_MAX_PCI_BARS 2
> +
> +/* modules info */
> +MODULE_LICENSE(DRIVER_LICENSE);
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_VERSION(DRIVER_VERSION);
> +
> +static unsigned int vsmp_bus = 0;
> +static unsigned short vsmp_dev = 0x1f;
> +static unsigned char vsmp_func = 0;
> +static bool vsmp_bdf_static = false;
> +static char *vsmp_ctrl_pci_addr = "";
> +module_param(vsmp_ctrl_pci_addr, charp, 0660);
> +
> +/* defines */
> +// copied from arch/x86/pci/direct.c
> +#define PCI_CONF1_ADDRESS(bus, devfn, reg) \
> +	   (0x80000000 | ((reg & 0xF00) << 16) | (bus << 16) \
> +	   | (devfn << 8) | (reg & 0xFC))

Put the operator on the first line.

> +
> +/* global vars */
> +void __iomem *cfg_addr = NULL;
> +static struct kobject *vsmp_sysfs_kobj;
> +static struct pci_dev *vsmp_dev_obj = NULL;
> +
> +static const struct pci_device_id pci_tbl[] = {
> +	{ PCI_VDEVICE(SCALEMP, PCI_DEVICE_ID_SAP_FLX_VSMP_CTL), 0, },
> +	{ 0, },			/* terminate list */
> +};
> +
> +static struct {
> +	uint64_t start;
> +	uint64_t len;
> +} mapped_bars[VSMP_CTL_MAX_PCI_BARS] = { 0 };
> +
> +/* internal functions */
> +
> +/**
> + * parses the pci address of a given address where the vSMP
> + * pci device is found
> + */
> +static void parse_vsmp_ctrl_pci_addr(void)
> +{
> +	if (strlen(vsmp_ctrl_pci_addr)) {

Reverse this condition.

	if (strlen(vsmp_ctrl_pci_addr) != 0)
		return;

> +		unsigned int bus;
> +		unsigned short dev;
> +		unsigned char func;
> +
> +		if (sscanf(vsmp_ctrl_pci_addr, "%04x:%02hx.%1hhx", &bus, &dev,
> +			   &func) == 3) {

Same:

	if (sscanf(vsmp_ctrl_pci_addr, "%04x:%02hx.%1hhx", &bus, &dev,
		   &func) != 3)
		return;


> +			vsmp_bus = bus;
> +			vsmp_dev = dev;
> +			vsmp_func = func;
> +			vsmp_bdf_static = true;
> +		}
> +	}
> +}
> +
> +/**
> + * queries as specfic device in order to determine if it is a vSMP pci device
> + */
> +static void query_pci_bus_for_vsmp_directly(unsigned int default_bus,
> +					    unsigned int devfn)
> +{
> +	printk(KERN_INFO
> +	       "vSMP pci controller not found in devices tree, trying directly at %x:%x.%x...\n",
> +	       vsmp_bus, vsmp_dev, vsmp_func);
> +
> +	outl(PCI_CONF1_ADDRESS(vsmp_bus, devfn, PCI_BASE_ADDRESS_0), 0xcf8);
> +	mapped_bars[0].start = inl(0xcfc);

Doesn't this need to set mapped_bars[0].len as well?

> +}
> +
> +/**
> + * scan all the devices on the system in order to locate the
> + * vSMP pic device for a given dev end func is provided
> + */
> +static void scan_pci_devs_list_for_vsmp(unsigned int devfn)

This function should return negatives if we do not find a device.
The current error hanlding is convoluted and relies on globals.

> +{
> +	int found, i;

"found" is never set to false.  Make it a bool.

> +	struct pci_dev *pdev = NULL;

Do not initialize "pdev" to a bogus value.

> +	const struct pci_device_id *ent;
> +
> +	for_each_pci_dev(pdev) {
> +		if (devfn != -1) {

Make this -1 a define instead of a magic number.

> +			if ((pdev->bus->number == vsmp_bus) &&
> +			    (devfn == pdev->devfn)) {
> +				found = 1;
> +				break;
> +			}
> +		} else {
> +			ent = pci_match_id(pci_tbl, pdev);
> +			if (ent) {
> +				found = 1;
> +				vsmp_bus = pdev->bus->number;
> +				vsmp_dev = PCI_SLOT(pdev->devfn);
> +				vsmp_func = PCI_FUNC(pdev->devfn);
> +				break;
> +			}
> +		}
> +	}

	if (!found)
		return -ENODEV;

> +
> +	for (i = 0; (i < ARRAY_SIZE(mapped_bars)) && found; i++) {
> +		mapped_bars[i].start = pci_resource_start(pdev, i);
> +		mapped_bars[i].len = pci_resource_len(pdev, i);
> +	}
> +	vsmp_dev_obj = pdev;
> +}
> +
> +/**
> + * open the cfg address space of the vSDP device
> + */
> +static int open_cfg_addr(void)
> +{
> +	unsigned int devfn = (vsmp_bdf_static) ? PCI_DEVFN(vsmp_dev, vsmp_func)
> +						: -1;
> +
> +	scan_pci_devs_list_for_vsmp(devfn);
> +	if (!mapped_bars[0].start)
> +		query_pci_bus_for_vsmp_directly(0, devfn);
> +
> +	if (!mapped_bars[0].len) {
> +		printk(KERN_INFO "vSMP pci controller not found, exiting.\n");
> +		return -1;

Return proper kernel error codes.  "return -EINVAL;"

> +	}
> +
> +	printk(KERN_INFO "mapping bar 0: [0x%llx,0x%llx]\n",
> +	       mapped_bars[0].start, mapped_bars[0].start + mapped_bars[0].len);
> +	cfg_addr = ioremap(mapped_bars[0].start, mapped_bars[0].len);
> +
> +	if (!cfg_addr) {

Delete the blank link between the call and the error checking.  They are
part of the same thing.

> +		printk(KERN_ERR
> +		       "failed to map vSMP pci controller at %x:%x.%x, exiting.\n",
> +		       vsmp_bus, vsmp_dev, vsmp_func);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/* exported functions */
> +
> +/**
> + * read a value from a specfic register in the vSMP's device config space
> + */
> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type)


Use u64 when 64 bit types are intended.

> +{
> +	unsigned long ret_val;


u64 ret_val;

> +
> +	switch (type) {
> +	case VSMP_CTL_REG_SIZE_8BIT:
> +		ret_val = readb(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_16BIT:
> +		ret_val = readw(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_32BIT:
> +		ret_val = readl(cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_64BIT:
> +		ret_val = readq(cfg_addr + reg);
> +		break;
> +
> +	default:
> +		printk(KERN_ERR "unsupported reg size type %d.\n", type);
> +		ret_val = (unsigned long)(-1);


64 bit types.  "ret_val = -1ULL;"


> +	}
> +
> +	dev_dbg(&vsmp_dev_obj->dev, "%s: read 0x%lx from reg 0x%x of %d bits\n",
> +		__func__, ret_val, reg, (type + 1) * 8);
> +	return ret_val;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_read_reg_from_cfg);
> +
> +/**
> + * write a value to a specfic register in the vSMP's device config space
> + */
> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
> +			   enum reg_size_type type)
> +{
> +	switch (type) {
> +	case VSMP_CTL_REG_SIZE_8BIT:
> +		writeb((unsigned char)value, cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_16BIT:
> +		writew((unsigned short)value, cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_32BIT:
> +		writel((unsigned int)value, cfg_addr + reg);
> +		break;
> +
> +	case VSMP_CTL_REG_SIZE_64BIT:
> +		writeq(value, cfg_addr + reg);
> +		break;
> +
> +	default:
> +		printk(KERN_ERR "unsupported reg size type %d.\n", type);
> +	}
> +
> +	dev_dbg(&vsmp_dev_obj->dev, "%s: wrote 0x%x to reg 0x%x of %u bits\n",
> +		__func__, reg, reg, (type + 1) * 8);
> +}
> +EXPORT_SYMBOL_GPL(vsmp_write_reg_to_cfg);
> +
> +/**
> + * reag a buffer from a specific offset in a specific bar, maxed to a predefined len size-wise from the vSMP device
> + */
> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,

This is unsigned int but it returns zero or negative error codes.

Create a separate function for "halt_on_null".  It will be cleaner that
way.

> +				     char *out, unsigned int len,
> +				     bool halt_on_null)
> +{
> +	unsigned char __iomem *buff;
> +
> +	BUG_ON(!mapped_bars[bar].start);
> +	BUG_ON(ARRAY_SIZE(mapped_bars) <= bar);
> +	BUG_ON((offset + len) > mapped_bars[bar].len);
> +
> +	buff = ioremap(mapped_bars[bar].start + offset, len);
> +	if (!buff)
> +		return -ENOMEM;
> +
> +	if (halt_on_null) {
> +		int i;
> +		unsigned char c;
> +
> +		for (i = 0; i < len; i++) {
> +			c = ioread8(&buff[i]);
> +			if (!c)
> +				break;
> +			out[i] = c;

Copy the NUL terminator to out[i] before the break.

> +		}
> +	} else
> +		memcpy_fromio(out, buff, len);
> +
> +	iounmap(buff);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_read_buff_from_bar);
> +
> +/**
> + * register the vSMP sysfs object for user space interaction
> + */
> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	int error = -1;
> +
> +	if (vsmp_sysfs_kobj && bin_attr) {
> +		if ((error = sysfs_create_bin_file(vsmp_sysfs_kobj, bin_attr)))
> +			printk(KERN_CRIT "%s returned error %d\n", __func__,
> +			       error);
> +	}
> +
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_register_sysfs_group);
> +
> +/**
> + * deergister the vSMP sysfs object for user space interaction
> + */
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr)
> +{
> +	if (vsmp_sysfs_kobj && bin_attr)
> +		sysfs_remove_bin_file(vsmp_sysfs_kobj, bin_attr);
> +}
> +EXPORT_SYMBOL_GPL(vsmp_deregister_sysfs_group);
> +
> +/**
> + * generic function to read from the bar
> + */
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, unsigned bar, unsigned reg,
> +			       char *buf, loff_t off, size_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (op->buff_offset >= op->hwi_block_size) {	// preform H/W op
> +		vsmp_reset_op(op);
> +
> +		if ((ret_val = vsmp_read_buff_from_bar(bar, reg, op->buff,
> +						       op->hwi_block_size, false))) {
> +			printk(KERN_ERR "%s operation failed\n",
> +			       (op->action == FW_READ) ? "read" : "write");
> +		}
> +		op->buff_offset = 0;
> +	}
> +
> +	if (!ret_val) {

Flip this around.  Do error handling.  Don't do success handling.

> +		ret_val = memory_read_from_buffer(buf, count, &op->buff_offset,
> +						  op->buff, op->hwi_block_size);
> +	}
> +
> +	return ret_val;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_generic_buff_read);
> +
> +void vsmp_reset_op(struct fw_ops *op)
> +{
> +	memset(op->buff, 0, op->hwi_block_size);
> +	op->buff_offset = op->hwi_block_size;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_reset_op);
> +
> +int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
> +		  enum vsmp_fw_action action)
> +{
> +	op->hwi_block_size = max_size;
> +	op->action = action;
> +	op->buff_offset = op->hwi_block_size;
> +
> +	if (!(op->buff = kzalloc(op->hwi_block_size, GFP_KERNEL)))
> +		return -ENOMEM;
> +
> +	vsmp_reset_op(op);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_init_op);
> +
> +void vsmp_release_op(struct fw_ops *op)
> +{
> +	BUG_ON(!op->buff);
> +	kfree(op->buff);
> +	memset(op, 0, sizeof(*op));
> +}
> +EXPORT_SYMBOL_GPL(vsmp_release_op);
> +
> +bool vsmp_op_buffer_depleted(struct fw_ops *op)
> +{
> +	return op->buff_offset >= op->hwi_block_size;
> +}
> +EXPORT_SYMBOL_GPL(vsmp_op_buffer_depleted);
> +
> +/* module functions */
> +
> +/**
> + * init the module
> + */
> +static int __init vsmp_api_init(void)
> +{
> +	int ret_val;
> +
> +	parse_vsmp_ctrl_pci_addr();
> +
> +	if (!(ret_val = open_cfg_addr())) {

Error handling not success handling.

> +		printk(KERN_INFO "%s [%02x:%02x.%x] up and running\n",
> +		       DRIVER_DESC, vsmp_bus, vsmp_dev, vsmp_func);
> +
> +		if (!(vsmp_sysfs_kobj = kobject_create_and_add("vsmp",
> +								hypervisor_kobj))) {

Reverse.

> +			printk(KERN_ERR
> +			       "failed to create sysfs entry for vSMP pci controller at %x:%x.%x, exiting.\n",
> +			       vsmp_bus, vsmp_dev, vsmp_func);
> +			ret_val = -1;
> +		}
> +	}
> +
> +	return ret_val;
> +}
> +
> +/**
> + * cleanup after the module upon exit
> + */
> +static void __exit vsmp_api_exit(void)
> +{
> +	if (cfg_addr)
> +		iounmap(cfg_addr);
> +
> +	if (vsmp_sysfs_kobj)
> +		kobject_put(vsmp_sysfs_kobj);
> +}
> +
> +module_init(vsmp_api_init);
> +module_exit(vsmp_api_exit);
> diff --git a/drivers/staging/vsmp/api.h b/drivers/staging/vsmp/api.h
> new file mode 100644
> index 000000000000..88d81b80bd81
> --- /dev/null
> +++ b/drivers/staging/vsmp/api.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP api header
> + * Copyright (C) 2022 SAP.
> + */
> +
> +#ifndef VSMP_API_H
> +#define VSMP_API_H
> +
> +#include <linux/sysfs.h>
> +
> +enum reg_size_type {
> +	VSMP_CTL_REG_SIZE_8BIT = 0,
> +	VSMP_CTL_REG_SIZE_16BIT,
> +	VSMP_CTL_REG_SIZE_32BIT,
> +	VSMP_CTL_REG_SIZE_64BIT
> +};
> +
> +enum vsmp_fw_action {
> +	FW_READ = 0,
> +	FW_WRITE = 1
> +};
> +
> +struct fw_ops {
> +	enum vsmp_fw_action action;
> +	ssize_t hwi_block_size;
> +	unsigned char *buff;
> +	loff_t buff_offset;
> +	bool in_progress;
> +};
> +
> +#define FILE_PREM (S_IRUSR | S_IRGRP | S_IROTH)
> +
> +#define VSMP_ATTR_RO(_name) static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
> +#define vsmp_read_reg8_from_cfg(_reg_) ((unsigned char) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_8BIT))
> +#define vsmp_read_reg16_from_cfg(_reg_) ((unsigned short) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_16BIT))
> +#define vsmp_read_reg32_from_cfg(_reg_) ((unsigned int) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_32BIT))
> +#define vsmp_read_reg64_from_cfg(_reg_) ((unsigned long) vsmp_read_reg_from_cfg((_reg_), VSMP_CTL_REG_SIZE_64BIT))
> +#define vsmp_write_reg8_to_cfg(_reg_, _val_) vsmp_write_reg_to_cfg((_reg_),(_val_), VSMP_CTL_REG_SIZE_8BIT)
> +#define vsmp_write_reg16_to_cfg(_reg_, _val_) vsmp_write_reg_to_cfg((_reg_), (_val_), VSMP_CTL_REG_SIZE_16BIT)
> +#define vsmp_write_reg32_to_cfg(_reg_, _val_) vsmp_write_reg_to_cfg((_reg_), (_val_), VSMP_CTL_REG_SIZE_32BIT)
> +#define vsmp_write_reg64_to_cfg(_reg_, _val_) vsmp_write_reg_to_cfg((_reg_), (_val_), VSMP_CTL_REG_SIZE_64BIT)
> +
> +unsigned long vsmp_read_reg_from_cfg(unsigned int reg, enum reg_size_type type);
> +void vsmp_write_reg_to_cfg(unsigned int reg, unsigned long value,
> +			   enum reg_size_type type);
> +unsigned int vsmp_read_buff_from_bar(unsigned int bar, unsigned int offset,
> +				     char *out, unsigned int len,
> +				     bool halt_on_null);
> +int vsmp_register_sysfs_group(const struct bin_attribute *bin_attr);
> +void vsmp_deregister_sysfs_group(const struct bin_attribute *bin_attr);
> +
> +ssize_t vsmp_generic_buff_read(struct fw_ops *op, unsigned bar, unsigned reg,
> +			       char *buf, loff_t off, size_t count);
> +void vsmp_reset_op(struct fw_ops *op);
> +int vsmp_init_op(struct fw_ops *op, ssize_t max_size,
> +		 enum vsmp_fw_action action);
> +void vsmp_release_op(struct fw_ops *op);
> +bool vsmp_op_buffer_depleted(struct fw_ops *op);
> +
> +#endif // VSMP_API_H
> diff --git a/drivers/staging/vsmp/common/Kconfig b/drivers/staging/vsmp/common/Kconfig
> new file mode 100644
> index 000000000000..fe0cfeaca1bb
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config VSMP_COMMON
> +	tristate "vSMP Guest driver common Support"
> +	depends on VSMP
> +	help
> +	  select this feature in order to be able to retrive generic data from the vSMP
> +	  VM.
> +	  currently old version is supported.
> +	  the data can be found under /sys/hypervisor/vsmp
> +
> +	  If unsure, say N.
> diff --git a/drivers/staging/vsmp/common/Makefile b/drivers/staging/vsmp/common/Makefile
> new file mode 100644
> index 000000000000..e6f6aa1d73df
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for vSMP Guest common feature.
> +#
> +
> +obj-$(CONFIG_VSMP_COMMON)	+= common_info.o 
> +common_info-y		+= common.o version.o
> diff --git a/drivers/staging/vsmp/common/common.c b/drivers/staging/vsmp/common/common.c
> new file mode 100644
> index 000000000000..31e6551f2b7f
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/common.c
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP common
> + * Copyright (C) 2022 SAP.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/pci_regs.h>
> +#include <linux/pci.h>
> +#include <linux/kobject.h>
> +#include <linux/kernel.h>
> +
> +#include <asm/io.h>
> +
> +#include "../api.h"
> +#include "common.h"
> +
> +#define DRIVER_LICENSE "GPL"
> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> +#define DRIVER_DESC "vSMP common info driver"
> +#define DRIVER_VERSION "0.1"
> +
> +/* modules info */
> +MODULE_LICENSE(DRIVER_LICENSE);
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_VERSION(DRIVER_VERSION);
> +
> +static struct sysfs_entry_cbs cbs_arr[] = {
> +	create_entry(version),
> +};
> +
> +/* module functions */
> +
> +/**
> + * init the common module
> + */
> +static int __init vsmp_common_info_init(void)
> +{
> +	int ret_val = 0, i;
> +
> +	for (i = 0; (i < ARRAY_SIZE(cbs_arr) && !ret_val); i++) {
> +		if ((ret_val = cbs_arr[i].reg_cb()))
> +			printk(KERN_ERR "failed to init sysfs (%d), exiting.\n",
> +			       ret_val);
> +	}
> +
> +	return -1 * ret_val;

sysfs_register_version_cb() already returns negatives.  Why the "-1 *"?

> +}
> +
> +/**
> + * cleanup after the module
> + */
> +static void __exit vsmp_common_info_exit(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cbs_arr); i++)
> +		cbs_arr[i].dereg_cb();
> +}
> +
> +module_init(vsmp_common_info_init);
> +module_exit(vsmp_common_info_exit);
> diff --git a/drivers/staging/vsmp/common/common.h b/drivers/staging/vsmp/common/common.h
> new file mode 100644
> index 000000000000..08a096628c95
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/common.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP common header
> + * Copyright (C) 2022 SAP.
> + */
> +
> +#ifndef VSM_COMMON_H
> +#define VSM_COMMON_H
> +
> +#define create_entry(_label_) \
> +	{ \
> +		.reg_cb = sysfs_register_ ## _label_ ## _cb, \
> +		.dereg_cb = sysfs_deregister_ ## _label_ ## _cb, \
> +	}
> +
> +typedef int (*sysfs_register_cb)(void);
> +typedef void (*sysfs_deregister_cb)(void);
> +
> +struct sysfs_entry_cbs {
> +	sysfs_register_cb reg_cb;
> +	sysfs_deregister_cb dereg_cb;
> +};
> +
> +int sysfs_register_version_cb(void);
> +void sysfs_deregister_version_cb(void);
> +
> +#endif // !VSM_COMMON_H
> diff --git a/drivers/staging/vsmp/common/version.c b/drivers/staging/vsmp/common/version.c
> new file mode 100644
> index 000000000000..35b5a7785e9a
> --- /dev/null
> +++ b/drivers/staging/vsmp/common/version.c
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP version
> + * Copyright (C) 2022 SAP.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kobject.h>
> +
> +#include "../api.h"
> +#include "../registers.h"
> +
> +#define VERSION_MAX_LEN (1 << 19)
> +
> +static struct fw_ops op;
> +
> +static ssize_t version_read(struct file *filp, struct kobject *kobj,
> +			    struct bin_attribute *bin_attr,
> +			    char *buf, loff_t off, size_t count)
> +{
> +	ssize_t ret_val = vsmp_generic_buff_read(&op, 0,
> +						 vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
> +						 buf, off, count);

Don't put functions which can fail in the declaration block.

	ret_val = vsmp_generic_buff_read(&op, 0,
					 vsmp_read_reg32_from_cfg(VSMP_VERSION_REG),
					 buf, off, count);

> +
> +	if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))

Don't test for specific error codes.

	if (ret_val < 0)
		return ret_val;


> +	    buf[ret_val++] = '\n';
> +
> +	return ret_val;
> +}
> +
> +struct bin_attribute version_raw_attr = __BIN_ATTR(version, FILE_PREM, version_read, NULL, VERSION_MAX_LEN);
> +
> +/**
> + * retrive str in order to determine the proper length.
> + * this is the best way to maintain backwards compatibility with all
> + * vSMP versions.
> + */
> +static int get_version_len(void)
> +{
> +	unsigned reg;
> +	char *version_str = kzalloc(VERSION_MAX_LEN, GFP_ATOMIC | __GFP_NOWARN);

VERSION_MAX_LEN is 524288 bytes.  That's too long.  Create a small
buffer and loop over it until you find the NUL terminator.  Why does
this need to be ATOMIC, are we holding a lock?  Hopefully if we can fix
the length the __GFP_NOWARN will be unnecessary.

> +
> +	if (!version_str)
> +		return -1;
> +
> +	reg = vsmp_read_reg32_from_cfg(VSMP_VERSION_REG);
> +	memset(version_str, 0, VERSION_MAX_LEN);
> +
> +	if (vsmp_read_buff_from_bar(0, reg, version_str, VERSION_MAX_LEN, true)) {
> +		printk(KERN_ERR "failed to read buffer from bar\n");
> +		return -1;
> +	}
> +
> +	version_raw_attr.size = strlen(version_str);

get_version_len() should return the length instead of setting a global.

> +	kfree(version_str);
> +
> +	return 0;
> +}
> +
> +/**
> + * register the version sysfs entry
> + */
> +int sysfs_register_version_cb(void)
> +{
> +	if (get_version_len()) {
> +		printk(KERN_ERR "failed to init vSMP version module\n");
> +		return -1;
> +	}
> +
> +	if (!vsmp_init_op(&op, version_raw_attr.size, FW_READ)) {

This if statement is reversed so this code can never work.

> +		printk(KERN_ERR "failed to init vSMP version op\n");
> +		return -1;
> +	}
> +
> +	return vsmp_register_sysfs_group(&version_raw_attr);
> +}
> +
> +/**
> + * deregister the version sysfs entry
> + */
> +void sysfs_deregister_version_cb(void)
> +{
> +	vsmp_deregister_sysfs_group(&version_raw_attr);
> +	vsmp_release_op(&op);
> +}
> diff --git a/drivers/staging/vsmp/logs/Kconfig b/drivers/staging/vsmp/logs/Kconfig
> new file mode 100644
> index 000000000000..d41491472bab
> --- /dev/null
> +++ b/drivers/staging/vsmp/logs/Kconfig
> @@ -0,0 +1,10 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config VSMP_LOGS
> +	tristate "vSMP Guest driver logs Support"
> +	depends on VSMP
> +	help
> +	  select this feature in order to be able to retrive logs from the vSMP
> +	  VM.
> +	  the data can be found under /sys/hypervisor/logs
> +
> +	  If unsure, say N.
> diff --git a/drivers/staging/vsmp/logs/Makefile b/drivers/staging/vsmp/logs/Makefile
> new file mode 100644
> index 000000000000..edc679803fe6
> --- /dev/null
> +++ b/drivers/staging/vsmp/logs/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for vSMP Guest common feature.
> +#
> +
> +obj-$(CONFIG_VSMP_LOGS)	+= logs.o 
> +logs-y		+= active_logs.o
> diff --git a/drivers/staging/vsmp/logs/active_logs.c b/drivers/staging/vsmp/logs/active_logs.c
> new file mode 100644
> index 000000000000..91492c72cc6f
> --- /dev/null
> +++ b/drivers/staging/vsmp/logs/active_logs.c
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * vSMP logs
> + * Copyright (C) 2022 SAP.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/pci_regs.h>
> +#include <linux/pci.h>
> +#include <linux/kobject.h>
> +#include <linux/kernel.h>
> +#include <linux/mutex.h>
> +
> +#include <asm/io.h>
> +
> +#include "../api.h"
> +#include "../registers.h"
> +
> +#define DRIVER_LICENSE "GPL"
> +#define DRIVER_AUTHOR "Eial Czerwacki <eial.czerwacki@sap.com>"
> +#define DRIVER_DESC "vSMP logs driver"
> +#define DRIVER_VERSION "0.1"
> +
> +#define LOGS_MASK (1 << 0)
> +
> +/* modules info */
> +MODULE_LICENSE(DRIVER_LICENSE);
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_VERSION(DRIVER_VERSION);

Just put:

MODULE_LICENSE("GPL v2");

Adding unnecessary indirection is against kernel style.  It says v2 at
the top.

> +
> +static struct fw_ops op;
> +static unsigned log_start;
> +DEFINE_MUTEX(log_mutex);
> +
> +/* module functions */
> +static ssize_t log_read(struct file *filp, struct kobject *kobj,
> +			struct bin_attribute *bin_attr,
> +			char *buf, loff_t off, size_t count)
> +{
> +	ssize_t ret_val = 0;
> +
> +	if (!op.in_progress) {
> +		if ((ret_val = mutex_lock_interruptible(&log_mutex))) {

Don't do assignments inside of an if condition.

> +			printk(KERN_ERR
> +			       "error in atemmpt to lock mutex (%lx)\n",
> +			       ret_val);
> +			return ret_val;
> +		}
> +
> +		vsmp_write_reg16_to_cfg(VSMP_LOGS_CTRL_REG,
> +					vsmp_read_reg16_from_cfg
> +					(VSMP_LOGS_CTRL_REG) | LOGS_MASK);
> +		op.in_progress = true;
> +		vsmp_write_reg64_to_cfg(VSMP_LOGS_STATUS_REG, 0);
> +	}
> +
> +	if (vsmp_op_buffer_depleted(&op)) {
> +		if (!(vsmp_read_reg16_from_cfg(VSMP_LOGS_CTRL_REG) & LOGS_MASK)) {
> +			vsmp_reset_op(&op);
> +			op.in_progress = 0;
> +			mutex_unlock(&log_mutex);
> +			return ret_val;

Return a literal when possible. "return 0;" is more clear than
"return ret_val;".

> +		}
> +	}
> +
> +	ret_val = vsmp_generic_buff_read(&op, 1, log_start, buf, off, count);
> +	if (!ret_val || (ret_val == -ENOMEM) || (ret_val == -EINVAL)) {
> +		printk(KERN_ERR "error reading from buffer\n");
> +		mutex_unlock(&log_mutex);
> +		return 0;
> +	}
> +
> +	if (vsmp_op_buffer_depleted(&op)) {
> +		vsmp_write_reg64_to_cfg(VSMP_LOGS_STATUS_REG,
> +					~0LL & ~(1LL << 0));
> +	}
> +
> +	return count;
> +}
> +
> +static struct bin_attribute log_raw_attr = __BIN_ATTR(log, FILE_PREM, log_read, NULL, 0);
> +
> +/**
> + * init the log module
> + */
> +static int __init vsmp_logs_init(void)
> +{
> +	if (!vsmp_init_op(&op, vsmp_read_reg32_from_cfg(VSMP_LOGS_LEN_REG),
> +							 FW_READ)) {
> +		printk(KERN_ERR "failed to init vSMP log op\n");
> +		return -1;
> +	}
> +
> +	log_start = vsmp_read_reg32_from_cfg(VSMP_LOGS_START_REG);
> +	mutex_init(&log_mutex);
> +
> +	return vsmp_register_sysfs_group(&log_raw_attr);

If vsmp_register_sysfs_group() fails then it needs to vsmp_release_op().

I have written a lot about how to write error handling code:
https://lore.kernel.org/all/20210831084735.GL12231@kadam/

regards,
dan carpenter