Message-ID: <a4136e60-2f1d-0e82-bbdc-dbffb4b9e914@amd.com>
Date:   Mon, 4 Jul 2022 04:07:07 +0000
From:   "Rao, Appana Durga Kedareswara" 
        <appana.durga.kedareswara.rao@....com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Appana Durga Kedareswara rao <appana.durga.rao@...inx.com>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "michal.simek@...inx.com" <michal.simek@...inx.com>,
        "derek.kiernan@...inx.com" <derek.kiernan@...inx.com>,
        "dragan.cvetic@...inx.com" <dragan.cvetic@...inx.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "git (AMD-Xilinx)" <git@....com>, "git@...inx.com" <git@...inx.com>
Subject: Re: [PATCH 2/4] drivers: misc: Add Support for TMR Manager

Hi Greg,

Thanks for the review.

On 01/07/22 1:49 pm, Greg KH wrote:
> On Tue, Jun 28, 2022 at 11:13:36AM +0530, Appana Durga Kedareswara rao wrote:
>> The Triple Modular Redundancy(TMR) Manager is responsible for handling
>> the TMR subsystem state, including fault detection and error recovery
>> provides soft error detection, correction and recovery features.
>>
>> This commit adds support for fault detection feature via
>> sysfs interface.
>>
>> Usage:
>> To know the hardware status:
>> cat /sys/devices/platform/amba_pl/44a10000.tmr_manager/status
>> To know the break handler count(Error count):
>> cat /sys/devices/platform/amba_pl/44a10000.tmr_manager/errcnt
>>
>> Signed-off-by: Appana Durga Kedareswara rao <appana.durga.rao@...inx.com>
>> ---
>>   .../testing/sysfs-driver-xilinx-tmr-manager   |  18 ++
>>   MAINTAINERS                                   |   7 +
>>   drivers/misc/Kconfig                          |  10 +
>>   drivers/misc/Makefile                         |   1 +
>>   drivers/misc/xilinx_tmr_manager.c             | 288 ++++++++++++++++++
>>   5 files changed, 324 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-xilinx-tmr-manager
>>   create mode 100644 drivers/misc/xilinx_tmr_manager.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-xilinx-tmr-manager b/Documentation/ABI/testing/sysfs-driver-xilinx-tmr-manager
>> new file mode 100644
>> index 000000000000..c2f615126201
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-xilinx-tmr-manager
>> @@ -0,0 +1,18 @@
>> +What:		/sys/devices/platform/amba_pl/<dev>/status
>> +Date:		June 2022
>> +Contact:	appana.durga.rao@...inx.com
>> +Description:	This control file provides the status of the tmr manager
>> +		useful for getting the status of fault.
>> +		This file cannot be written.
> 
> Examples of what the data in this file will be?
> 

Okay will fix in v2.
>> +
>> +What:		/sys/devices/platform/amba_pl/<dev>/errcnt
>> +Date:		June 2022
>> +Contact:	appana.durga.rao@...inx.com
>> +Description:	This control file provides the fault detection count.
>> +		This file cannot be written.
> 
> Again, example output?
> 
> 

sure will update in v2.
> 
>> +
>> +What:		/sys/devices/platform/amba_pl/<dev>/dis_block_break
>> +Date:		June 2022
>> +Contact:	appana.durga.rao@...inx.com
>> +Description:	This control file enables the break signal.
>> +		This file is write only.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6cc825857722..3d29109788ed 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -12962,6 +12962,13 @@ W:	http://www.monstr.eu/fdt/
>>   T:	git git://git.monstr.eu/linux-2.6-microblaze.git
>>   F:	arch/microblaze/
>>   
>> +MICROBLAZE TMR MANAGER
>> +M:	Appana Durga Kedareswara rao <appana.durga.rao@...inx.com>
>> +S:	Supported
>> +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
>> +
>>   MICROCHIP AT91 DMA DRIVERS
>>   M:	Ludovic Desroches <ludovic.desroches@...rochip.com>
>>   M:	Tudor Ambarus <tudor.ambarus@...rochip.com>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 41d2bb0ae23a..555ae2e33b91 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -483,6 +483,16 @@ config OPEN_DICE
>>   
>>   	  If unsure, say N.
>>   
>> +config TMR_MANAGER
>> +	bool "Select TMR Manager"
>> +	depends on MICROBLAZE && MB_MANAGER
>> +	help
>> +	  This option enables the driver developed for TMR Manager. The Triple
>> +	  Modular Redundancy(TMR) manager provides support for fault detection
>> +	  via sysfs interface.
>> +
>> +	  Say N here unless you know what you are doing.
>> +
>>   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 70e800e9127f..28b9803f909b 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -60,3 +60,4 @@ obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
>>   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
>> diff --git a/drivers/misc/xilinx_tmr_manager.c b/drivers/misc/xilinx_tmr_manager.c
>> new file mode 100644
>> index 000000000000..5a4562e4a9d4
>> --- /dev/null
>> +++ b/drivers/misc/xilinx_tmr_manager.c
>> @@ -0,0 +1,288 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Xilinx TMR Subsystem.
>> + *
>> + * Copyright (C) 2022 Xilinx, Inc.
>> + *
>> + * Description:
>> + * This driver is developed for TMR Manager,The Triple Modular Redundancy(TMR)
>> + * Manager is responsible for handling the TMR subsystem state, including
>> + * fault detection and error recovery. The core is triplicated in each of
>> + * the sub-blocks in the TMR subsystem, and provides majority voting of
>> + * its internal state provides soft error detection, correction and
>> + * recovery. Error detection feature is provided through sysfs
>> + * entries which allow the user to observer the TMR microblaze
>> + * status.
>> + */
>> +
>> +#include <asm/xilinx_mb_manager.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +
>> +/* TMR Manager Register offsets */
>> +#define XTMR_MANAGER_CR_OFFSET		0x0
>> +#define XTMR_MANAGER_FFR_OFFSET		0x4
>> +#define XTMR_MANAGER_CMR0_OFFSET	0x8
>> +#define XTMR_MANAGER_CMR1_OFFSET	0xC
>> +#define XTMR_MANAGER_BDIR_OFFSET	0x10
>> +#define XTMR_MANAGER_SEMIMR_OFFSET	0x1C
>> +
>> +/* Register Bitmasks/shifts */
>> +#define XTMR_MANAGER_CR_MAGIC1_MASK	GENMASK(7, 0)
>> +#define XTMR_MANAGER_CR_MAGIC2_MASK	GENMASK(15, 8)
>> +#define XTMR_MANAGER_CR_RIR_MASK	BIT(16)
>> +#define XTMR_MANAGER_FFR_LM12_MASK	BIT(0)
>> +#define XTMR_MANAGER_FFR_LM13_MASK	BIT(1)
>> +#define XTMR_MANAGER_FFR_LM23_MASK	BIT(2)
>> +
>> +#define XTMR_MANAGER_CR_MAGIC2_SHIFT	4
>> +#define XTMR_MANAGER_CR_RIR_SHIFT	16
>> +#define XTMR_MANAGER_CR_BB_SHIFT	18
>> +
>> +#define XTMR_MANAGER_MAGIC1_MAX_VAL	255
>> +
>> +/**
>> + * struct xtmr_manager_dev - Driver data for TMR Manager
>> + * @regs: device physical base address
>> + * @dev: pointer to device struct
>> + * @cr_val: control register value
>> + * @magic1: Magic 1 hardware configuration value
>> + * @err_cnt: error statistics count
>> + * @phys_baseaddr: Physical base address
>> + */
>> +struct xtmr_manager_dev {
>> +	void __iomem *regs;
>> +	struct device *dev;
> 
> Is this the parent of this device?  Why not make a new device?  The
> interaction here isn't well defined, what controls the lifespan of this
> structure?
> 

It is parent of this device,
Agree with your comments will fix in v2.
>> +	u32 cr_val;
>> +	u32 magic1;
>> +	u32 err_cnt;
>> +	uintptr_t phys_baseaddr;
> 
> This should be resource_size_t, not uintptr_t, right?
> 

Okay will fix in v2.

> 
>> +};
>> +
>> +/* IO accessors */
>> +static inline void xtmr_manager_write(struct xtmr_manager_dev *xtmr_manager,
>> +				      u32 addr, u32 value)
>> +{
>> +	iowrite32(value, xtmr_manager->regs + addr);
>> +}
>> +
>> +static inline u32 xtmr_manager_read(struct xtmr_manager_dev *xtmr_manager,
>> +				    u32 addr)
>> +{
>> +	return ioread32(xtmr_manager->regs + addr);
>> +}
>> +
>> +/**
>> + * xtmr_manager_unblock_break - unblocks the break signal
>> + * @xtmr_manager: Pointer to xtmr_manager_dev structure
>> + */
>> +static void xtmr_manager_unblock_break(struct xtmr_manager_dev *xtmr_manager)
>> +{
>> +	xtmr_manager->cr_val &= ~(1 << XTMR_MANAGER_CR_BB_SHIFT);
>> +	xtmr_manager_write(xtmr_manager, XTMR_MANAGER_CR_OFFSET,
>> +			   xtmr_manager->cr_val);
>> +}
>> +
>> +/**
>> + * xmb_manager_reset_handler - clears the ffr register contents
>> + * @priv: Private pointer
>> + */
>> +static void xmb_manager_reset_handler(void *priv)
>> +{
>> +	struct xtmr_manager_dev *xtmr_manager = (struct xtmr_manager_dev *)priv;
>> +	/* Clear the FFR Register contents as a part of recovery process. */
>> +	xtmr_manager_write(xtmr_manager, XTMR_MANAGER_FFR_OFFSET, 0);
> 
> Did you run checkpatch on this code?  Please do so.
> 

Yes i didn't get any warnings from the script.

>> +}
>> +
>> +/**
> 
> No need for kernel doc formatting for static functions, right?
> 
will remove in v2.

>> + * xmb_manager_update_errcnt - update the error inject count
>> + * @priv: Private pointer
>> + */
>> +static void xmb_manager_update_errcnt(void *priv)
> 
> Use a real pointer, not a void one please.  We want type safe functions.
> 

will fix in v2.
>> +{
>> +	struct xtmr_manager_dev *xtmr_manager = (struct xtmr_manager_dev *)priv;
> 
> And even if you did have a real pointer, you never need to cast from a
> void *.
>

will fix in v2.

>> +
>> +	xtmr_manager->err_cnt++;
>> +}
>> +
>> +static ssize_t errcnt_show(struct device *dev, struct device_attribute *attr,
>> +			   char *buf)
>> +{
>> +	struct xtmr_manager_dev *xtmr_manager = dev_get_drvdata(dev);
>> +
>> +	return sprintf(buf, "%x\n", xtmr_manager->err_cnt);
> 
> sysfs_emit() here and everywhere please.
> 

sure will fix in v2.
>> +}
>> +static DEVICE_ATTR_RO(errcnt);
>> +
>> +static ssize_t status_show(struct device *dev, struct device_attribute *attr,
>> +			   char *buf)
>> +{
>> +	struct xtmr_manager_dev *xtmr_manager = dev_get_drvdata(dev);
>> +	size_t ffr;
>> +	int len = 0;
>> +
>> +	ffr = xtmr_manager_read(xtmr_manager, XTMR_MANAGER_FFR_OFFSET);
>> +	if ((ffr & XTMR_MANAGER_FFR_LM12_MASK) == XTMR_MANAGER_FFR_LM12_MASK) {
>> +		len += sprintf(buf + len,
>> +			       "Lockstep mismatch between processor 1 and 2\n");
>> +	}
>> +
>> +	if ((ffr & XTMR_MANAGER_FFR_LM13_MASK) == XTMR_MANAGER_FFR_LM13_MASK) {
>> +		len += sprintf(buf + len,
>> +			       "Lockstep mismatch between processor 1 and 3\n");
>> +	}
>> +
>> +	if ((ffr & XTMR_MANAGER_FFR_LM23_MASK) == XTMR_MANAGER_FFR_LM23_MASK) {
>> +		len += sprintf(buf + len,
>> +			       "Lockstep mismatch between processor 2 and 3\n");
> 
> That feels like a lot more than "one bit of information", who is going
> to parse this and do something with this information?
> 
> 

It is just to convey the TMR Manager status to the user.

>> +	}
>> +
>> +	return len;
>> +}
>> +static DEVICE_ATTR_RO(status);
>> +
>> +static ssize_t dis_block_break_store(struct device *dev,
>> +				     struct device_attribute *attr,
>> +				     const char *buf, size_t size)
>> +{
>> +	struct xtmr_manager_dev *xtmr_manager = dev_get_drvdata(dev);
>> +	int ret;
>> +	long value;
>> +
>> +	ret = kstrtoul(buf, 16, &value);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (value > 1)
>> +		return -EINVAL;
>> +
>> +	xtmr_manager_unblock_break(xtmr_manager);
> 
> No error checking?
> 

will fix in v2.

>> +	return size;
>> +}
>> +static DEVICE_ATTR_WO(dis_block_break);
>> +
>> +static struct attribute *xtmr_manager_attrs[] = {
>> +	&dev_attr_dis_block_break.attr,
>> +	&dev_attr_status.attr,
>> +	&dev_attr_errcnt.attr,
>> +	NULL,
>> +};
>> +ATTRIBUTE_GROUPS(xtmr_manager);
>> +
>> +static void xtmr_manager_init(struct xtmr_manager_dev *xtmr_manager)
>> +{
>> +	/* Clear the SEM interrupt mask register to disable the interrupt */
>> +	xtmr_manager_write(xtmr_manager, XTMR_MANAGER_SEMIMR_OFFSET, 0);
>> +
>> +	/* Allow recovery reset by default */
>> +	xtmr_manager->cr_val = (1 << XTMR_MANAGER_CR_RIR_SHIFT) |
>> +				xtmr_manager->magic1;
>> +	xtmr_manager_write(xtmr_manager, XTMR_MANAGER_CR_OFFSET,
>> +			   xtmr_manager->cr_val);
>> +	/*
>> +	 * Configure Break Delay Initialization Register to zero so that
>> +	 * break occurs immediately
>> +	 */
>> +	xtmr_manager_write(xtmr_manager, XTMR_MANAGER_BDIR_OFFSET, 0);
>> +
>> +	/*
>> +	 * To come out of break handler need to block the break signal
>> +	 * in the tmr manager, update the xtmr_manager cr_val for the same
>> +	 */
>> +	xtmr_manager->cr_val |= (1 << XTMR_MANAGER_CR_BB_SHIFT);
>> +
>> +	/*
>> +	 * When the break vector gets asserted because of error injection,
>> +	 * the break signal must be blocked before exiting from the
>> +	 * break handler, Below api updates the TMR manager address and
>> +	 * control register and error counter callback arguments,
>> +	 * which will be used by the break handler to block the
>> +	 * break and call the callback function.
>> +	 */
>> +	xmb_manager_register(xtmr_manager->phys_baseaddr, xtmr_manager->cr_val,
>> +			     xmb_manager_update_errcnt,
>> +			     xtmr_manager, xmb_manager_reset_handler);
> 
> No error handling?
>

will fix in v2.

>> +}
>> +
>> +/**
>> + * xtmr_manager_probe - Driver probe function
>> + * @pdev: Pointer to the platform_device structure
>> + *
>> + * This is the driver probe routine. It does all the memory
>> + * allocation and creates sysfs entries for the device.
>> + *
>> + * Return: 0 on success and failure value on error
>> + */
>> +static int xtmr_manager_probe(struct platform_device *pdev)
>> +{
>> +	struct xtmr_manager_dev *xtmr_manager;
>> +	struct resource *res;
>> +	int err;
>> +
>> +	xtmr_manager = devm_kzalloc(&pdev->dev, sizeof(*xtmr_manager),
>> +				    GFP_KERNEL);
>> +	if (!xtmr_manager)
>> +		return -ENOMEM;
>> +
>> +	xtmr_manager->dev = &pdev->dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	xtmr_manager->regs =  devm_ioremap_resource(xtmr_manager->dev, res);
>> +	if (IS_ERR(xtmr_manager->regs))
>> +		return PTR_ERR(xtmr_manager->regs);
>> +
>> +	xtmr_manager->phys_baseaddr = res->start;
>> +
>> +	err = of_property_read_u32(pdev->dev.of_node, "xlnx,magic1",
>> +				   &xtmr_manager->magic1);
>> +	if (err < 0) {
>> +		dev_err(&pdev->dev, "unable to read xlnx,magic1 property");
>> +		return err;
>> +	} else if (xtmr_manager->magic1 > XTMR_MANAGER_MAGIC1_MAX_VAL) {
> 
> No need for else.

ok will fix in v2
> 
>> +		dev_err(&pdev->dev, "invalid xlnx,magic1 property value");
>> +		return err;
> 
> but err is not an error value here, right?
> 

Yes will return proper error code in v2
>> +	}
>> +
>> +	/* Initialize TMR Manager */
>> +	xtmr_manager_init(xtmr_manager);
>> +
>> +	err = sysfs_create_groups(&xtmr_manager->dev->kobj,
>> +				  xtmr_manager_groups);
> 
> You just raced with userspace and lost.  NEVER deal with raw kobjects,
> that's a huge sign that you are doing something wrong in a driver.
> 
> Please use the proper default groups pointer for your driver to do this
> correctly so that the driver core will handle this for you.
> 

Okay will fix in v2.

>> +	if (err < 0) {
>> +		dev_err(&pdev->dev, "unable to create sysfs entries\n");
>> +		return err;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, xtmr_manager);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xtmr_manager_remove(struct platform_device *pdev)
>> +{
>> +	sysfs_remove_groups(&pdev->dev.kobj, xtmr_manager_groups);
> 
> Again, never touch a kobject in a driver.
> 

Sure will fix in v2.

Regards,
Kedar.
> thanks,
> 
> greg k-h

Powered by blists - more mailing lists