Greetings Dan,
>"new driver" is too vague. What is the driver for?
the first line limitation makes it harder to describe it properly, I'll try to find a more
informative line
>
>You need a README or a TODO which explains how to move the driver out of
>staging and into the main kernel.
already discussed this with Greg, he pointed out that after I fix all the
rejects, I should post it in the lkml as proper driver
>
>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.
I'll run it and fix resulting issues
>
>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.
sure thing, I'll go over the issues you've mentioned above and fix.
>
>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@xxxxxxx>
>> ---
>> 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@xxxxxxx>
>> +M: linux-vsmp@xxxxxxx
>> +S: Maintained
>> +F: drivers/staging/vsmp-guest
>> +
>> VT1211 HARDWARE MONITOR DRIVER
>> M: Juerg Haefliger <juergh@xxxxxxxxx>
>> L: linux-hwmon@xxxxxxxxxxxxxxx
>> 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@xxxxxxx>"
>> +#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;
>
will do.
>> + 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;
>
according to Greg, I should use the probe callback in the ops struct. that
will provide me with the proper dev struct without the need to scan anything
>> +
>> + 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;"
probably missed it when I was fixing such issues. will fix.
>
>> + }
>> +
>> + 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.
did you meant line?
>
>> + 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;
sure
>
>> +
>> + 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.
I'll refractor this code properly.
>
>> + 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.
>
should I memset out to zero instead?
>> + }
>> + } 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@xxxxxxx>"
>> +#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 *"?
leftovers from prior iteration of the driver
>
>> +}
>> +
>> +/**
>> + * 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);
>
can you explain what is the reason behind this recommendation?
>> +
>> + if ((ret_val != -ENOMEM) && ((ret_val != -EINVAL)))
>
>Don't test for specific error codes.
>
> if (ret_val < 0)
> return ret_val;
>
>
now I understand, there was a patchset which fixes for all the -1 and the specific ret_val testing, I see it
was misplaced at some point of the work.
thanks for pointing it out
>> + 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.
I'm not sure it is possible to loopover, I'll take that into consideration.
in this flow, only one access is possible, so I assume GFP_ATOMIC is not needed
>
>> +
>> + 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.
do you mean version_raw_attr.size = get_version_len()?
>
>> + 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.
not sure I follow, can you explain?
>
>> + 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@xxxxxxx>"
>> +#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().
thanks, will fix
>
>I have written a lot about how to write error handling code:
>https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20210831084735.GL12231%40kadam%2F&data=04%7C01%7Ceial.czerwacki%40sap.com%7Cf1e83fe49d074f7552da08da07ffb8e1%7C42f7676cf455423c82f6dc2d99791af7%7C0%7C0%7C637831092157869913%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=C5q%2BpdsckU%2FAo%2BKu2t63n6sMRv%2FGx5HID0oVYS3ZxEI%3D&reserved=0
>
I'll read the link above,.
Thanks for your remarks.
Eial
