Re: [PATCH v2 4/4] drivers: misc: Add Support for TMR Inject IP

Rao, Appana Durga Kedareswara
2022-07-29T08:14:04Z

Hi Greg,

Thanks for the review.

On 28/07/22 7:54 pm, Greg KH wrote:
> On Wed, Jul 20, 2022 at 11:30:16AM +0530, Appana Durga Kedareswara rao wrote:
>> From: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
>>
>> The Triple Modular Redundancy(TMR) provides functional fault injection by
>> changing selected MicroBlaze instructions, which provides the possibility
>> to verify that the TMR subsystem error detection and fault recovery logic
>> is working properly, provided sysfs entries which allow the user to inject
>> a fault.
>
> We already have a fault-injection api, why are you not using that?
>

Inorder to inject the error using TMR inject IP, The API
which injects the error should be executed from Processor LMB,
below sysfs entry calls microblaze core API xmb_inject_err()
which switches the processor to real mode and injects the error,
Please find the code corresponds to xmb_inject_err() API here:
https://www.spinics.net/lists/arm-kernel/msg991888.html

>>
>> Usage:
>> echo 1 > /sys/devices/platform/amba_pl/44a30000.tmr_inject/inject_err
>>
>> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.kedareswara.rao@amd.com>
>> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@xilinx.com>
>
> Odd, just one is needed.

Sure will fix in next version.
>
>> ---
>> Changes for v2:
>> --> Added Examples for sysfs entries
>> --> Removed uneeded struct dev from the driver private structure
>> --> Updated driver to use sysfs_emit() API instead of sprintf() API
>> --> Added error checks wherever applicable.
>> --> Fixed sysfs registration.
>> .../testing/sysfs-driver-xilinx-tmr-inject | 16 ++
>> MAINTAINERS | 7 +
>> drivers/misc/Kconfig | 10 +
>> drivers/misc/Makefile | 1 +
>> drivers/misc/xilinx_tmr_inject.c | 186 ++++++++++++++++++
>> 5 files changed, 220 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-driver-xilinx-tmr-inject
>> create mode 100644 drivers/misc/xilinx_tmr_inject.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-xilinx-tmr-inject b/Documentation/ABI/testing/sysfs-driver-xilinx-tmr-inject
>> new file mode 100644
>> index 000000000000..d274b30ee24c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-xilinx-tmr-inject
>> @@ -0,0 +1,16 @@
>> +What: /sys/devices/platform/amba_pl/<dev>/inject_err
>> +Date: June 2022
>
> It's not June anymore, even when you sent this patch :(

Will fix in next version
>
>> +Contact: appana.durga.rao@xilinx.com
>> +Description: This control file allows to inject fault using tmr inject.
>> + This file is write only.
>> + Example:
>> + # echo 1 > /sys/devices/platform/amba_pl/44a30000.tmr_inject/inject_err
>> +
>> +What: /sys/devices/platform/amba_pl/<dev>/inject_cpuid
>> +Date: June 2022
>> +Contact: appana.durga.rao@xilinx.com
>> +Description: This control file allows to configure the CPU identifier
>> + to enable fault injection.
>> + This file is write only.
>> + Example:
>> + # echo 1 > /sys/devices/platform/amba_pl/44a30000.tmr_inject/inject_cpuid
>
> What errors and faults happen? Where is that documented? What happens
> when you write to these sysfs files? Does the system crash? Why would
> you want to use them ever?
>
>

TMR subsystem has 3 Microblaze processor cores, by default driver is
configured to inject the error at processor core-0, This sysfs entry
provides a mechanism to inject the fault at the user-specified core.

>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 732fd9ae7d9f..c903b45c204a 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -13087,6 +13087,13 @@ F: Documentation/ABI/testing/sysfs-driver-xilinx-tmr-manager
>> F: Documentation/devicetree/bindings/misc/xlnx,tmr-manager.yaml
>> F: drivers/misc/xilinx_tmr_manager.c
>>
>> +MICROBLAZE TMR INJECT
>> +M: Appana Durga Kedareswara rao <appana.durga.kedareswara.rao@amd.com>
>> +S: Supported
>> +F: Documentation/ABI/testing/sysfs-driver-xilinx-tmr-inject
>> +F: Documentation/devicetree/bindings/misc/xlnx,tmr-inject.yaml
>> +F: drivers/misc/xilinx_tmr_inject.c
>> +
>> MICROCHIP AT91 DMA DRIVERS
>> M: Ludovic Desroches <ludovic.desroches@microchip.com>
>> M: Tudor Ambarus <tudor.ambarus@microchip.com>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 555ae2e33b91..0989c36f3051 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -493,6 +493,16 @@ config TMR_MANAGER
>>
>> Say N here unless you know what you are doing.
>>
>> +config TMR_INJECT
>> + bool "Select TMR Inject"
>> + depends on TMR_MANAGER
>> + help
>> + This option enables the driver developed for TMR Inject.
>> + The Triple Modular Redundancy(TMR) Inject provides
>> + fault injection.
>> +
>> + Say N here unless you know what you are doing.
>
> Why can't this be a module?
>
>

We can use this driver as a module will fix in next version.

>
>> +
>> 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 28b9803f909b..e9d0a709e207 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -61,3 +61,4 @@ obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o
>> obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.o
>> obj-$(CONFIG_OPEN_DICE) += open-dice.o
>> obj-$(CONFIG_TMR_MANAGER) += xilinx_tmr_manager.o
>> +obj-$(CONFIG_TMR_INJECT) += xilinx_tmr_inject.o
>> diff --git a/drivers/misc/xilinx_tmr_inject.c b/drivers/misc/xilinx_tmr_inject.c
>> new file mode 100644
>> index 000000000000..930d89e90b61
>> --- /dev/null
>> +++ b/drivers/misc/xilinx_tmr_inject.c
>> @@ -0,0 +1,186 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for Xilinx TMR Inject IP.
>> + *
>> + * Copyright (C) 2022 Xilinx, Inc.
>> + *
>> + * Description:
>> + * This driver is developed for TMR Inject IP,The Triple Modular Redundancy(TMR)
>> + * Inject provides fault injection.
>> + * Fault injection and detection features are provided through sysfs entries
>> + * which allow the user to generate a fault.
>> + */
>> +
>> +#include <asm/xilinx_mb_manager.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +
>> +/* TMR Inject Register offsets */
>> +#define XTMR_INJECT_CR_OFFSET 0x0
>> +#define XTMR_INJECT_AIR_OFFSET 0x4
>> +#define XTMR_INJECT_IIR_OFFSET 0xC
>> +#define XTMR_INJECT_EAIR_OFFSET 0x10
>> +#define XTMR_INJECT_ERR_OFFSET 0x204
>> +
>> +/* Register Bitmasks/shifts */
>> +#define XTMR_INJECT_CR_CPUID_SHIFT 8
>> +#define XTMR_INJECT_CR_IE_SHIFT 10
>> +#define XTMR_INJECT_IIR_ADDR_MASK GENMASK(31, 16)
>> +
>> +#define XTMR_INJECT_MAGIC_MAX_VAL 255
>> +
>> +/**
>> + * struct xtmr_inject_dev - Driver data for TMR Inject
>> + * @regs: device physical base address
>> + * @cr_val: control register value
>> + * @magic: Magic hardware configuration value
>> + * @err_cnt: error statistics count
>> + */
>> +struct xtmr_inject_dev {
>> + void __iomem *regs;
>> + u32 cr_val;
>> + u32 magic;
>> + u32 err_cnt;
>> +};
>> +
>> +/* IO accessors */
>> +static inline void xtmr_inject_write(struct xtmr_inject_dev *xtmr_inject,
>> + u32 addr, u32 value)
>> +{
>> + iowrite32(value, xtmr_inject->regs + addr);
>> +}
>> +
>> +static inline u32 xtmr_inject_read(struct xtmr_inject_dev *xtmr_inject,
>> + u32 addr)
>> +{
>> + return ioread32(xtmr_inject->regs + addr);
>> +}
>> +
>> +static ssize_t inject_err_store(struct device *dev,
>> + struct device_attribute *attr, const char *buf,
>> + size_t size)
>> +{
>> + int ret;
>> + long value;
>> +
>> + ret = kstrtoul(buf, 16, &value);
>> + if (ret)
>> + return ret;
>> +
>> + if (value > 1)
>> + return -EINVAL;
>
> That does not match your documentation :(
>

Will fix the documentation in next version

>
>> +
>> + xmb_inject_err();
>> +
>> + return size;
>> +}
>> +static DEVICE_ATTR_WO(inject_err);
>> +
>> +static ssize_t inject_cpuid_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t size)
>> +{
>> + struct xtmr_inject_dev *xtmr_inject = dev_get_drvdata(dev);
>> + int ret;
>> + long value;
>> +
>> + ret = kstrtoul(buf, 0, &value);
>> + if (ret)
>> + return ret;
>> +
>> + if (value > 3)
>> + return -EINVAL;
>
> Again, does not match the documentation at all.
>

Will fix the documentation in next version

> confused,

sorry for the improper sysfs documentation will fix in next version.

Regards,
Kedar.
>
> greg k-h