[v2,1/4] firmware: xilinx: Add sysfs interface

Commit Message

Jolly Shah Jan. 8, 2020, 11:54 p.m. UTC

From: Rajan Vaja <[e-mail]>

Add Firmware-ggs sysfs interface which provides read/write
interface to global storage registers.

Signed-off-by: Rajan Vaja <[e-mail]>
Signed-off-by: Michal Simek <[e-mail]>
Signed-off-by: Jolly Shah <[e-mail]>
---
Changes in v2:
 - Updated Linux kernel version in documentation.
 - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
 - Free Kobject structure in case of error.
 - Resolved smatch errors.
 - Updated Signed-off-by sequence.
---
 Documentation/ABI/stable/sysfs-firmware-zynqmp |  50 +++++
 drivers/firmware/xilinx/Makefile               |   2 +-
 drivers/firmware/xilinx/zynqmp-ggs.c           | 284 +++++++++++++++++++++++++
 drivers/firmware/xilinx/zynqmp.c               |  32 +++
 include/linux/firmware/xlnx-zynqmp.h           |  10 +
 5 files changed, 377 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
 create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c

Comments

Greg KH Jan. 14, 2020, 2:52 p.m. UTC | #1

On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> From: Rajan Vaja <[e-mail]>
> 
> Add Firmware-ggs sysfs interface which provides read/write
> interface to global storage registers.
> 
> Signed-off-by: Rajan Vaja <[e-mail]>
> Signed-off-by: Michal Simek <[e-mail]>
> Signed-off-by: Jolly Shah <[e-mail]>
> ---
> Changes in v2:
>  - Updated Linux kernel version in documentation.
>  - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
>  - Free Kobject structure in case of error.
>  - Resolved smatch errors.
>  - Updated Signed-off-by sequence.
> ---
>  Documentation/ABI/stable/sysfs-firmware-zynqmp |  50 +++++
>  drivers/firmware/xilinx/Makefile               |   2 +-
>  drivers/firmware/xilinx/zynqmp-ggs.c           | 284 +++++++++++++++++++++++++
>  drivers/firmware/xilinx/zynqmp.c               |  32 +++
>  include/linux/firmware/xlnx-zynqmp.h           |  10 +
>  5 files changed, 377 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
>  create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
> 
> diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> new file mode 100644
> index 0000000..cffa2fc
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> @@ -0,0 +1,50 @@
> +What:		/sys/firmware/zynqmp/ggs*

Why are these attributes just not hanging off of the platform device for
the firmware controller?  Why do you need a new subdir under "firmware"?

> +Date:		January 2018
> +KernelVersion:	5.5

5.6?  :)

> +Contact:	"Jolly Shah" <[e-mail]>
> +Description:
> +		Read/Write PMU global general storage register value,
> +		GLOBAL_GEN_STORAGE{0:3}.
> +		Global general storage register that can be used
> +		by system to pass information between masters.
> +
> +		The register is reset during system or power-on
> +		resets. Three registers are used by the FSBL and
> +		other Xilinx software products: GLOBAL_GEN_STORAGE{4:6}.
> +
> +		Usage:
> +		# cat /sys/firmware/zynqmp/ggs0
> +		# echo <mask> <value> > /sys/firmware/zynqmp/ggs0
> +
> +		Example:
> +		# cat /sys/firmware/zynqmp/ggs0
> +		# echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
> +
> +Users:		Xilinx
> +
> +What:		/sys/firmware/zynqmp/pggs*
> +Date:		January 2018
> +KernelVersion:	5.5
> +Contact:	"Jolly Shah" <[e-mail]>
> +Description:
> +		Read/Write PMU persistent global general storage register
> +		value, PERS_GLOB_GEN_STORAGE{0:3}.
> +		Persistent global general storage register that
> +		can be used by system to pass information between
> +		masters.
> +
> +		This register is only reset by the power-on reset
> +		and maintains its value through a system reset.
> +		Four registers are used by the FSBL and other Xilinx
> +		software products: PERS_GLOB_GEN_STORAGE{4:7}.
> +		Register is reset only by a POR reset.
> +
> +		Usage:
> +		# cat /sys/firmware/zynqmp/pggs0
> +		# echo <mask> <value> > /sys/firmware/zynqmp/pggs0
> +
> +		Example:
> +		# cat /sys/firmware/zynqmp/pggs0
> +		# echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/pggs0
> +
> +Users:		Xilinx
> diff --git a/drivers/firmware/xilinx/Makefile b/drivers/firmware/xilinx/Makefile
> index 875a537..1e8643c 100644
> --- a/drivers/firmware/xilinx/Makefile
> +++ b/drivers/firmware/xilinx/Makefile
> @@ -1,5 +1,5 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Makefile for Xilinx firmwares
>  
> -obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o
> +obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o zynqmp-ggs.o
>  obj-$(CONFIG_ZYNQMP_FIRMWARE_DEBUG) += zynqmp-debug.o
> diff --git a/drivers/firmware/xilinx/zynqmp-ggs.c b/drivers/firmware/xilinx/zynqmp-ggs.c
> new file mode 100644
> index 0000000..e2a6700
> --- /dev/null
> +++ b/drivers/firmware/xilinx/zynqmp-ggs.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Xilinx Zynq MPSoC Firmware layer
> + *
> + *  Copyright (C) 2014-2018 Xilinx, Inc.
> + *
> + *  Jolly Shah <[e-mail]>
> + *  Rajan Vaja <[e-mail]>
> + */
> +
> +#include <linux/compiler.h>
> +#include <linux/of.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
> +
> +static ssize_t read_register(char *buf, u32 ioctl_id, u32 reg)
> +{
> +	int ret;
> +	u32 ret_payload[PAYLOAD_ARG_CNT];
> +	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
> +
> +	if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> +		return -EFAULT;
> +
> +	ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "0x%x\n", ret_payload[1]);
> +}
> +
> +static ssize_t write_register(const char *buf, size_t count, u32 read_ioctl,
> +			      u32 write_ioctl, u32 reg)
> +{
> +	char *kern_buff, *inbuf, *tok;
> +	long mask, value;
> +	int ret;
> +	u32 ret_payload[PAYLOAD_ARG_CNT];
> +	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
> +
> +	if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> +		return -EFAULT;
> +
> +	kern_buff = kzalloc(count, GFP_KERNEL);
> +	if (!kern_buff)
> +		return -ENOMEM;
> +
> +	ret = strlcpy(kern_buff, buf, count);
> +	if (ret < 0) {
> +		ret = -EFAULT;
> +		goto err;
> +	}
> +
> +	inbuf = kern_buff;
> +
> +	/* Read the write mask */
> +	tok = strsep(&inbuf, " ");
> +	if (!tok) {
> +		ret = -EFAULT;

If you just set count to the error value, no need to test the value of
ret when you exit.  Not a big deal...

> +		goto err;
> +	}
> +
> +	ret = kstrtol(tok, 16, &mask);
> +	if (ret) {
> +		ret = -EFAULT;
> +		goto err;
> +	}
> +
> +	/* Read the write value */
> +	tok = strsep(&inbuf, " ");
> +	if (!tok) {
> +		ret = -EFAULT;
> +		goto err;
> +	}
> +
> +	ret = kstrtol(tok, 16, &value);
> +	if (ret) {
> +		ret = -EFAULT;
> +		goto err;
> +	}
> +
> +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);

This feels "tricky", if you tie this to the device you have your driver
bound to, will this make it easier instead of having to go through the
ioctl callback?


> +	if (ret) {
> +		ret = -EFAULT;
> +		goto err;
> +	}
> +	ret_payload[1] &= ~mask;
> +	value &= mask;
> +	value |= ret_payload[1];
> +
> +	ret = eemi_ops->ioctl(0, write_ioctl, reg, value, NULL);
> +	if (ret)
> +		ret = -EFAULT;
> +
> +err:
> +	kfree(kern_buff);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +/**
> + * ggs_show - Show global general storage (ggs) sysfs attribute
> + * @device: Device structure
> + * @attr: Device attribute structure
> + * @buf: Requested available shutdown_scope attributes string
> + * @reg: Register number
> + *
> + * Return:Number of bytes printed into the buffer.
> + *
> + * Helper function for viewing a ggs register value.
> + *
> + * User-space interface for viewing the content of the ggs0 register.
> + * cat /sys/firmware/zynqmp/ggs0
> + */
> +static ssize_t ggs_show(struct device *device,
> +			struct device_attribute *attr,
> +			char *buf,
> +			u32 reg)
> +{
> +	return read_register(buf, IOCTL_READ_GGS, reg);
> +}
> +
> +/**
> + * ggs_store - Store global general storage (ggs) sysfs attribute
> + * @device: Device structure
> + * @attr: Device attribute structure
> + * @buf: User entered shutdown_scope attribute string
> + * @count: Size of buf
> + * @reg: Register number
> + *
> + * Return: count argument if request succeeds, the corresponding
> + * error code otherwise
> + *
> + * Helper function for storing a ggs register value.
> + *
> + * For example, the user-space interface for storing a value to the
> + * ggs0 register:
> + * echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
> + */
> +static ssize_t ggs_store(struct device *device,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count,
> +			 u32 reg)
> +{
> +	if (!device || !attr || !buf || !count || reg >= GSS_NUM_REGS)
> +		return -EINVAL;
> +
> +	return write_register(buf, count, IOCTL_READ_GGS, IOCTL_WRITE_GGS, reg);
> +}
> +
> +/* GGS register show functions */
> +#define GGS0_SHOW(N)						\
> +	ssize_t ggs##N##_show(struct device *device,		\
> +			      struct device_attribute *attr,	\
> +			      char *buf)			\
> +	{							\
> +		return ggs_show(device, attr, buf, N);		\
> +	}
> +
> +static GGS0_SHOW(0);
> +static GGS0_SHOW(1);
> +static GGS0_SHOW(2);
> +static GGS0_SHOW(3);
> +
> +/* GGS register store function */
> +#define GGS0_STORE(N)						\
> +	ssize_t ggs##N##_store(struct device *device,		\
> +			       struct device_attribute *attr,	\
> +			       const char *buf,			\
> +			       size_t count)			\
> +	{							\
> +		return ggs_store(device, attr, buf, count, N);	\
> +	}
> +
> +static GGS0_STORE(0);
> +static GGS0_STORE(1);
> +static GGS0_STORE(2);
> +static GGS0_STORE(3);
> +
> +/**
> + * pggs_show - Show persistent global general storage (pggs) sysfs attribute
> + * @device: Device structure
> + * @attr: Device attribute structure
> + * @buf: Requested available shutdown_scope attributes string
> + * @reg: Register number
> + *
> + * Return:Number of bytes printed into the buffer.
> + *
> + * Helper function for viewing a pggs register value.
> + */
> +static ssize_t pggs_show(struct device *device,
> +			 struct device_attribute *attr,
> +			 char *buf,
> +			 u32 reg)
> +{
> +	return read_register(buf, IOCTL_READ_PGGS, reg);
> +}
> +
> +/**
> + * pggs_store - Store persistent global general storage (pggs) sysfs attribute
> + * @device: Device structure
> + * @attr: Device attribute structure
> + * @buf: User entered shutdown_scope attribute string
> + * @count: Size of buf
> + * @reg: Register number
> + *
> + * Return: count argument if request succeeds, the corresponding
> + * error code otherwise
> + *
> + * Helper function for storing a pggs register value.
> + */
> +static ssize_t pggs_store(struct device *device,
> +			  struct device_attribute *attr,
> +			  const char *buf, size_t count,
> +			  u32 reg)
> +{
> +	return write_register(buf, count, IOCTL_READ_PGGS,
> +			      IOCTL_WRITE_PGGS, reg);
> +}
> +
> +#define PGGS0_SHOW(N)						\
> +	ssize_t pggs##N##_show(struct device *device,		\
> +			       struct device_attribute *attr,	\
> +			       char *buf)			\
> +	{							\
> +		return pggs_show(device, attr, buf, N);		\
> +	}
> +
> +#define PGGS0_STORE(N)						\
> +	ssize_t pggs##N##_store(struct device *device,		\
> +				struct device_attribute *attr,	\
> +				const char *buf,		\
> +				size_t count)			\
> +	{							\
> +		return pggs_store(device, attr, buf, count, N);	\
> +	}
> +
> +/* PGGS register show functions */
> +static PGGS0_SHOW(0);
> +static PGGS0_SHOW(1);
> +static PGGS0_SHOW(2);
> +static PGGS0_SHOW(3);
> +
> +/* PGGS register store functions */
> +static PGGS0_STORE(0);
> +static PGGS0_STORE(1);
> +static PGGS0_STORE(2);
> +static PGGS0_STORE(3);
> +
> +/* GGS register attributes */
> +static DEVICE_ATTR_RW(ggs0);
> +static DEVICE_ATTR_RW(ggs1);
> +static DEVICE_ATTR_RW(ggs2);
> +static DEVICE_ATTR_RW(ggs3);
> +
> +/* PGGS register attributes */
> +static DEVICE_ATTR_RW(pggs0);
> +static DEVICE_ATTR_RW(pggs1);
> +static DEVICE_ATTR_RW(pggs2);
> +static DEVICE_ATTR_RW(pggs3);
> +
> +static struct attribute *zynqmp_ggs_attrs[] = {
> +	&dev_attr_ggs0.attr,
> +	&dev_attr_ggs1.attr,
> +	&dev_attr_ggs2.attr,
> +	&dev_attr_ggs3.attr,
> +	&dev_attr_pggs0.attr,
> +	&dev_attr_pggs1.attr,
> +	&dev_attr_pggs2.attr,
> +	&dev_attr_pggs3.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(zynqmp_ggs);
> +
> +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> +{
> +	return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);

You might be racing userspace here and loosing :(

> +}
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index 75bdfaa..4c1117d 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
>  	case IOCTL_GET_PLL_FRAC_MODE:
>  	case IOCTL_SET_PLL_FRAC_DATA:
>  	case IOCTL_GET_PLL_FRAC_DATA:
> +	case IOCTL_WRITE_GGS:
> +	case IOCTL_READ_GGS:
> +	case IOCTL_WRITE_PGGS:
> +	case IOCTL_READ_PGGS:

Huh???

>  		return 1;
>  	default:
>  		return 0;
> @@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
>  }
>  EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
>  
> +static int zynqmp_pm_sysfs_init(void)
> +{
> +	struct kobject *zynqmp_kobj;
> +	int ret;
> +
> +	zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
> +	if (!zynqmp_kobj) {
> +		pr_err("zynqmp: Firmware kobj add failed.\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = zynqmp_pm_ggs_init(zynqmp_kobj);
> +	if (ret) {
> +		kobject_put(zynqmp_kobj);
> +		pr_err("%s() GGS init fail with error %d\n",
> +		       __func__, ret);
> +		goto err;
> +	}
> +err:
> +	return ret;
> +}
> +
>  static int zynqmp_firmware_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
>  	/* Assign eemi_ops_table */
>  	eemi_ops_tbl = &eemi_ops;
>  
> +	ret = zynqmp_pm_sysfs_init();

See, you have a platform device, hang the attributes off of that instead
of making a kobject and detatching yourself from the global device tree!

Please redo this, I think it will make it a lot simpler and more
obvious.

thanks,

greg k-h

Jolly Shah Jan. 23, 2020, 11:47 p.m. UTC | #2

Hi Greg,

Thanks for the review.

> -----Original Message-----
> From: Greg KH <[e-mail]>
> Sent: Tuesday, January 14, 2020 6:53 AM
> To: Jolly Shah <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Michal Simek <[e-mail]>; Rajan Vaja
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; Rajan Vaja <[e-mail]>; Jolly Shah
> <[e-mail]>
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> 
> On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> > From: Rajan Vaja <[e-mail]>
> >
> > Add Firmware-ggs sysfs interface which provides read/write
> > interface to global storage registers.
> >
> > Signed-off-by: Rajan Vaja <[e-mail]>
> > Signed-off-by: Michal Simek <[e-mail]>
> > Signed-off-by: Jolly Shah <[e-mail]>
> > ---
> > Changes in v2:
> >  - Updated Linux kernel version in documentation.
> >  - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
> >  - Free Kobject structure in case of error.
> >  - Resolved smatch errors.
> >  - Updated Signed-off-by sequence.
> > ---
> >  Documentation/ABI/stable/sysfs-firmware-zynqmp |  50 +++++
> >  drivers/firmware/xilinx/Makefile               |   2 +-
> >  drivers/firmware/xilinx/zynqmp-ggs.c           | 284
> +++++++++++++++++++++++++
> >  drivers/firmware/xilinx/zynqmp.c               |  32 +++
> >  include/linux/firmware/xlnx-zynqmp.h           |  10 +
> >  5 files changed, 377 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
> >  create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
> >
> > diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp
> b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > new file mode 100644
> > index 0000000..cffa2fc
> > --- /dev/null
> > +++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > @@ -0,0 +1,50 @@
> > +What:		/sys/firmware/zynqmp/ggs*
> 
> Why are these attributes just not hanging off of the platform device for
> the firmware controller?  Why do you need a new subdir under "firmware"?

Firmware driver was changed later to be platform driver but these interfaces were defined 
earlier and are in use.

> 
> > +Date:		January 2018
> > +KernelVersion:	5.5
> 
> 5.6?  :)

Yes. Will fix it in next version.

> 
> > +Contact:	"Jolly Shah" <[e-mail]>
> > +Description:
> > +		Read/Write PMU global general storage register value,
> > +		GLOBAL_GEN_STORAGE{0:3}.
> > +		Global general storage register that can be used
> > +		by system to pass information between masters.
> > +
> > +		The register is reset during system or power-on
> > +		resets. Three registers are used by the FSBL and
> > +		other Xilinx software products: GLOBAL_GEN_STORAGE{4:6}.
> > +
> > +		Usage:
> > +		# cat /sys/firmware/zynqmp/ggs0
> > +		# echo <mask> <value> > /sys/firmware/zynqmp/ggs0
> > +
> > +		Example:
> > +		# cat /sys/firmware/zynqmp/ggs0
> > +		# echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
> > +
> > +Users:		Xilinx
> > +
> > +What:		/sys/firmware/zynqmp/pggs*
> > +Date:		January 2018
> > +KernelVersion:	5.5
> > +Contact:	"Jolly Shah" <[e-mail]>
> > +Description:
> > +		Read/Write PMU persistent global general storage register
> > +		value, PERS_GLOB_GEN_STORAGE{0:3}.
> > +		Persistent global general storage register that
> > +		can be used by system to pass information between
> > +		masters.
> > +
> > +		This register is only reset by the power-on reset
> > +		and maintains its value through a system reset.
> > +		Four registers are used by the FSBL and other Xilinx
> > +		software products: PERS_GLOB_GEN_STORAGE{4:7}.
> > +		Register is reset only by a POR reset.
> > +
> > +		Usage:
> > +		# cat /sys/firmware/zynqmp/pggs0
> > +		# echo <mask> <value> > /sys/firmware/zynqmp/pggs0
> > +
> > +		Example:
> > +		# cat /sys/firmware/zynqmp/pggs0
> > +		# echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/pggs0
> > +
> > +Users:		Xilinx
> > diff --git a/drivers/firmware/xilinx/Makefile
> b/drivers/firmware/xilinx/Makefile
> > index 875a537..1e8643c 100644
> > --- a/drivers/firmware/xilinx/Makefile
> > +++ b/drivers/firmware/xilinx/Makefile
> > @@ -1,5 +1,5 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  # Makefile for Xilinx firmwares
> >
> > -obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o
> > +obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o zynqmp-ggs.o
> >  obj-$(CONFIG_ZYNQMP_FIRMWARE_DEBUG) += zynqmp-debug.o
> > diff --git a/drivers/firmware/xilinx/zynqmp-ggs.c
> b/drivers/firmware/xilinx/zynqmp-ggs.c
> > new file mode 100644
> > index 0000000..e2a6700
> > --- /dev/null
> > +++ b/drivers/firmware/xilinx/zynqmp-ggs.c
> > @@ -0,0 +1,284 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Xilinx Zynq MPSoC Firmware layer
> > + *
> > + *  Copyright (C) 2014-2018 Xilinx, Inc.
> > + *
> > + *  Jolly Shah <[e-mail]>
> > + *  Rajan Vaja <[e-mail]>
> > + */
> > +
> > +#include <linux/compiler.h>
> > +#include <linux/of.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/slab.h>
> > +#include <linux/firmware/xlnx-zynqmp.h>
> > +
> > +static ssize_t read_register(char *buf, u32 ioctl_id, u32 reg)
> > +{
> > +	int ret;
> > +	u32 ret_payload[PAYLOAD_ARG_CNT];
> > +	const struct zynqmp_eemi_ops *eemi_ops =
> zynqmp_pm_get_eemi_ops();
> > +
> > +	if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> > +		return -EFAULT;
> > +
> > +	ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sprintf(buf, "0x%x\n", ret_payload[1]);
> > +}
> > +
> > +static ssize_t write_register(const char *buf, size_t count, u32 read_ioctl,
> > +			      u32 write_ioctl, u32 reg)
> > +{
> > +	char *kern_buff, *inbuf, *tok;
> > +	long mask, value;
> > +	int ret;
> > +	u32 ret_payload[PAYLOAD_ARG_CNT];
> > +	const struct zynqmp_eemi_ops *eemi_ops =
> zynqmp_pm_get_eemi_ops();
> > +
> > +	if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> > +		return -EFAULT;
> > +
> > +	kern_buff = kzalloc(count, GFP_KERNEL);
> > +	if (!kern_buff)
> > +		return -ENOMEM;
> > +
> > +	ret = strlcpy(kern_buff, buf, count);
> > +	if (ret < 0) {
> > +		ret = -EFAULT;
> > +		goto err;
> > +	}
> > +
> > +	inbuf = kern_buff;
> > +
> > +	/* Read the write mask */
> > +	tok = strsep(&inbuf, " ");
> > +	if (!tok) {
> > +		ret = -EFAULT;
> 
> If you just set count to the error value, no need to test the value of
> ret when you exit.  Not a big deal...

Ok. Will fix it in next version

> 
> > +		goto err;
> > +	}
> > +
> > +	ret = kstrtol(tok, 16, &mask);
> > +	if (ret) {
> > +		ret = -EFAULT;
> > +		goto err;
> > +	}
> > +
> > +	/* Read the write value */
> > +	tok = strsep(&inbuf, " ");
> > +	if (!tok) {
> > +		ret = -EFAULT;
> > +		goto err;
> > +	}
> > +
> > +	ret = kstrtol(tok, 16, &value);
> > +	if (ret) {
> > +		ret = -EFAULT;
> > +		goto err;
> > +	}
> > +
> > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> 
> This feels "tricky", if you tie this to the device you have your driver
> bound to, will this make it easier instead of having to go through the
> ioctl callback?
> 

GGS(general global storage) registers are in PMU space and linux doesn't have access to it 
Hence ioctl is used.

> 
> > +	if (ret) {
> > +		ret = -EFAULT;
> > +		goto err;
> > +	}
> > +	ret_payload[1] &= ~mask;
> > +	value &= mask;
> > +	value |= ret_payload[1];
> > +
> > +	ret = eemi_ops->ioctl(0, write_ioctl, reg, value, NULL);
> > +	if (ret)
> > +		ret = -EFAULT;
> > +
> > +err:
> > +	kfree(kern_buff);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return count;
> > +}
> > +
> > +/**
> > + * ggs_show - Show global general storage (ggs) sysfs attribute
> > + * @device: Device structure
> > + * @attr: Device attribute structure
> > + * @buf: Requested available shutdown_scope attributes string
> > + * @reg: Register number
> > + *
> > + * Return:Number of bytes printed into the buffer.
> > + *
> > + * Helper function for viewing a ggs register value.
> > + *
> > + * User-space interface for viewing the content of the ggs0 register.
> > + * cat /sys/firmware/zynqmp/ggs0
> > + */
> > +static ssize_t ggs_show(struct device *device,
> > +			struct device_attribute *attr,
> > +			char *buf,
> > +			u32 reg)
> > +{
> > +	return read_register(buf, IOCTL_READ_GGS, reg);
> > +}
> > +
> > +/**
> > + * ggs_store - Store global general storage (ggs) sysfs attribute
> > + * @device: Device structure
> > + * @attr: Device attribute structure
> > + * @buf: User entered shutdown_scope attribute string
> > + * @count: Size of buf
> > + * @reg: Register number
> > + *
> > + * Return: count argument if request succeeds, the corresponding
> > + * error code otherwise
> > + *
> > + * Helper function for storing a ggs register value.
> > + *
> > + * For example, the user-space interface for storing a value to the
> > + * ggs0 register:
> > + * echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
> > + */
> > +static ssize_t ggs_store(struct device *device,
> > +			 struct device_attribute *attr,
> > +			 const char *buf, size_t count,
> > +			 u32 reg)
> > +{
> > +	if (!device || !attr || !buf || !count || reg >= GSS_NUM_REGS)
> > +		return -EINVAL;
> > +
> > +	return write_register(buf, count, IOCTL_READ_GGS, IOCTL_WRITE_GGS,
> reg);
> > +}
> > +
> > +/* GGS register show functions */
> > +#define GGS0_SHOW(N)						\
> > +	ssize_t ggs##N##_show(struct device *device,		\
> > +			      struct device_attribute *attr,	\
> > +			      char *buf)			\
> > +	{							\
> > +		return ggs_show(device, attr, buf, N);		\
> > +	}
> > +
> > +static GGS0_SHOW(0);
> > +static GGS0_SHOW(1);
> > +static GGS0_SHOW(2);
> > +static GGS0_SHOW(3);
> > +
> > +/* GGS register store function */
> > +#define GGS0_STORE(N)						\
> > +	ssize_t ggs##N##_store(struct device *device,		\
> > +			       struct device_attribute *attr,	\
> > +			       const char *buf,			\
> > +			       size_t count)			\
> > +	{							\
> > +		return ggs_store(device, attr, buf, count, N);	\
> > +	}
> > +
> > +static GGS0_STORE(0);
> > +static GGS0_STORE(1);
> > +static GGS0_STORE(2);
> > +static GGS0_STORE(3);
> > +
> > +/**
> > + * pggs_show - Show persistent global general storage (pggs) sysfs attribute
> > + * @device: Device structure
> > + * @attr: Device attribute structure
> > + * @buf: Requested available shutdown_scope attributes string
> > + * @reg: Register number
> > + *
> > + * Return:Number of bytes printed into the buffer.
> > + *
> > + * Helper function for viewing a pggs register value.
> > + */
> > +static ssize_t pggs_show(struct device *device,
> > +			 struct device_attribute *attr,
> > +			 char *buf,
> > +			 u32 reg)
> > +{
> > +	return read_register(buf, IOCTL_READ_PGGS, reg);
> > +}
> > +
> > +/**
> > + * pggs_store - Store persistent global general storage (pggs) sysfs attribute
> > + * @device: Device structure
> > + * @attr: Device attribute structure
> > + * @buf: User entered shutdown_scope attribute string
> > + * @count: Size of buf
> > + * @reg: Register number
> > + *
> > + * Return: count argument if request succeeds, the corresponding
> > + * error code otherwise
> > + *
> > + * Helper function for storing a pggs register value.
> > + */
> > +static ssize_t pggs_store(struct device *device,
> > +			  struct device_attribute *attr,
> > +			  const char *buf, size_t count,
> > +			  u32 reg)
> > +{
> > +	return write_register(buf, count, IOCTL_READ_PGGS,
> > +			      IOCTL_WRITE_PGGS, reg);
> > +}
> > +
> > +#define PGGS0_SHOW(N)						\
> > +	ssize_t pggs##N##_show(struct device *device,		\
> > +			       struct device_attribute *attr,	\
> > +			       char *buf)			\
> > +	{							\
> > +		return pggs_show(device, attr, buf, N);		\
> > +	}
> > +
> > +#define PGGS0_STORE(N)						\
> > +	ssize_t pggs##N##_store(struct device *device,		\
> > +				struct device_attribute *attr,	\
> > +				const char *buf,		\
> > +				size_t count)			\
> > +	{							\
> > +		return pggs_store(device, attr, buf, count, N);	\
> > +	}
> > +
> > +/* PGGS register show functions */
> > +static PGGS0_SHOW(0);
> > +static PGGS0_SHOW(1);
> > +static PGGS0_SHOW(2);
> > +static PGGS0_SHOW(3);
> > +
> > +/* PGGS register store functions */
> > +static PGGS0_STORE(0);
> > +static PGGS0_STORE(1);
> > +static PGGS0_STORE(2);
> > +static PGGS0_STORE(3);
> > +
> > +/* GGS register attributes */
> > +static DEVICE_ATTR_RW(ggs0);
> > +static DEVICE_ATTR_RW(ggs1);
> > +static DEVICE_ATTR_RW(ggs2);
> > +static DEVICE_ATTR_RW(ggs3);
> > +
> > +/* PGGS register attributes */
> > +static DEVICE_ATTR_RW(pggs0);
> > +static DEVICE_ATTR_RW(pggs1);
> > +static DEVICE_ATTR_RW(pggs2);
> > +static DEVICE_ATTR_RW(pggs3);
> > +
> > +static struct attribute *zynqmp_ggs_attrs[] = {
> > +	&dev_attr_ggs0.attr,
> > +	&dev_attr_ggs1.attr,
> > +	&dev_attr_ggs2.attr,
> > +	&dev_attr_ggs3.attr,
> > +	&dev_attr_pggs0.attr,
> > +	&dev_attr_pggs1.attr,
> > +	&dev_attr_pggs2.attr,
> > +	&dev_attr_pggs3.attr,
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(zynqmp_ggs);
> > +
> > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> > +{
> > +	return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
> 
> You might be racing userspace here and loosing :(

Prob is called before user space is notified about sysfs so racing shouldn't happen.
Or you are referring to some other race condition?

> 
> > +}
> > diff --git a/drivers/firmware/xilinx/zynqmp.c
> b/drivers/firmware/xilinx/zynqmp.c
> > index 75bdfaa..4c1117d 100644
> > --- a/drivers/firmware/xilinx/zynqmp.c
> > +++ b/drivers/firmware/xilinx/zynqmp.c
> > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
> >  	case IOCTL_GET_PLL_FRAC_MODE:
> >  	case IOCTL_SET_PLL_FRAC_DATA:
> >  	case IOCTL_GET_PLL_FRAC_DATA:
> > +	case IOCTL_WRITE_GGS:
> > +	case IOCTL_READ_GGS:
> > +	case IOCTL_WRITE_PGGS:
> > +	case IOCTL_READ_PGGS:
> 
> Huh???

Sorry not sure about your concern here. These registers are in PMU space and hence
Ioctl is needed to let linux access them.

> 
> >  		return 1;
> >  	default:
> >  		return 0;
> > @@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops
> *zynqmp_pm_get_eemi_ops(void)
> >  }
> >  EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
> >
> > +static int zynqmp_pm_sysfs_init(void)
> > +{
> > +	struct kobject *zynqmp_kobj;
> > +	int ret;
> > +
> > +	zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
> > +	if (!zynqmp_kobj) {
> > +		pr_err("zynqmp: Firmware kobj add failed.\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ret = zynqmp_pm_ggs_init(zynqmp_kobj);
> > +	if (ret) {
> > +		kobject_put(zynqmp_kobj);
> > +		pr_err("%s() GGS init fail with error %d\n",
> > +		       __func__, ret);
> > +		goto err;
> > +	}
> > +err:
> > +	return ret;
> > +}
> > +
> >  static int zynqmp_firmware_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct
> platform_device *pdev)
> >  	/* Assign eemi_ops_table */
> >  	eemi_ops_tbl = &eemi_ops;
> >
> > +	ret = zynqmp_pm_sysfs_init();
> 
> See, you have a platform device, hang the attributes off of that instead
> of making a kobject and detatching yourself from the global device tree!
> 
> Please redo this, I think it will make it a lot simpler and more
> obvious.

Agree it will be simpler but to as firmware driver was changed to be platform driver,
to keep paths same, we used sysfs.

Thanks,
Jolly Shah


> 
> thanks,
> 
> greg k-h

Greg KH Jan. 24, 2020, 6:03 a.m. UTC | #3

On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
> Hi Greg,
> 
> Thanks for the review.
> 
> > -----Original Message-----
> > From: Greg KH <[e-mail]>
> > Sent: Tuesday, January 14, 2020 6:53 AM
> > To: Jolly Shah <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Michal Simek <[e-mail]>; Rajan Vaja
> > <[e-mail]>; [e-mail]; linux-
> > [e-mail]; Rajan Vaja <[e-mail]>; Jolly Shah
> > <[e-mail]>
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> > 
> > On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> > > From: Rajan Vaja <[e-mail]>
> > >
> > > Add Firmware-ggs sysfs interface which provides read/write
> > > interface to global storage registers.
> > >
> > > Signed-off-by: Rajan Vaja <[e-mail]>
> > > Signed-off-by: Michal Simek <[e-mail]>
> > > Signed-off-by: Jolly Shah <[e-mail]>
> > > ---
> > > Changes in v2:
> > >  - Updated Linux kernel version in documentation.
> > >  - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
> > >  - Free Kobject structure in case of error.
> > >  - Resolved smatch errors.
> > >  - Updated Signed-off-by sequence.
> > > ---
> > >  Documentation/ABI/stable/sysfs-firmware-zynqmp |  50 +++++
> > >  drivers/firmware/xilinx/Makefile               |   2 +-
> > >  drivers/firmware/xilinx/zynqmp-ggs.c           | 284
> > +++++++++++++++++++++++++
> > >  drivers/firmware/xilinx/zynqmp.c               |  32 +++
> > >  include/linux/firmware/xlnx-zynqmp.h           |  10 +
> > >  5 files changed, 377 insertions(+), 1 deletion(-)
> > >  create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
> > >  create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
> > >
> > > diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > > new file mode 100644
> > > index 0000000..cffa2fc
> > > --- /dev/null
> > > +++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > > @@ -0,0 +1,50 @@
> > > +What:		/sys/firmware/zynqmp/ggs*
> > 
> > Why are these attributes just not hanging off of the platform device for
> > the firmware controller?  Why do you need a new subdir under "firmware"?
> 
> Firmware driver was changed later to be platform driver but these interfaces were defined 
> earlier and are in use.

Defined and in use where?  Not in the upstream kernel tree, right?  Or
am I missing them somewhere else?

> > > +	ret = kstrtol(tok, 16, &value);
> > > +	if (ret) {
> > > +		ret = -EFAULT;
> > > +		goto err;
> > > +	}
> > > +
> > > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > 
> > This feels "tricky", if you tie this to the device you have your driver
> > bound to, will this make it easier instead of having to go through the
> > ioctl callback?
> > 
> 
> GGS(general global storage) registers are in PMU space and linux doesn't have access to it 
> Hence ioctl is used.

Why not just a "real" call to the driver to make this type of reading?
You don't have ioctls within the kernel for other drivers to call,
that's not needed at all.

> > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> > > +{
> > > +	return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
> > 
> > You might be racing userspace here and loosing :(
> 
> Prob is called before user space is notified about sysfs so racing shouldn't happen.

"shouldn't"?  How do you know this?

> Or you are referring to some other race condition?

Your kobject was created, and notified userspace that it was present and
then sometime after that you add more attributes which userspace has no
idea are there.

If you use the proper device model interfaces, none of these problems
would be there.

> 
> > 
> > > +}
> > > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > b/drivers/firmware/xilinx/zynqmp.c
> > > index 75bdfaa..4c1117d 100644
> > > --- a/drivers/firmware/xilinx/zynqmp.c
> > > +++ b/drivers/firmware/xilinx/zynqmp.c
> > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
> > >  	case IOCTL_GET_PLL_FRAC_MODE:
> > >  	case IOCTL_SET_PLL_FRAC_DATA:
> > >  	case IOCTL_GET_PLL_FRAC_DATA:
> > > +	case IOCTL_WRITE_GGS:
> > > +	case IOCTL_READ_GGS:
> > > +	case IOCTL_WRITE_PGGS:
> > > +	case IOCTL_READ_PGGS:
> > 
> > Huh???
> 
> Sorry not sure about your concern here. These registers are in PMU space and hence
> Ioctl is needed to let linux access them.

userspace or kernelspace?

You seem to be mixing them both here.

> 
> > 
> > >  		return 1;
> > >  	default:
> > >  		return 0;
> > > @@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops
> > *zynqmp_pm_get_eemi_ops(void)
> > >  }
> > >  EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
> > >
> > > +static int zynqmp_pm_sysfs_init(void)
> > > +{
> > > +	struct kobject *zynqmp_kobj;
> > > +	int ret;
> > > +
> > > +	zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
> > > +	if (!zynqmp_kobj) {
> > > +		pr_err("zynqmp: Firmware kobj add failed.\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	ret = zynqmp_pm_ggs_init(zynqmp_kobj);
> > > +	if (ret) {
> > > +		kobject_put(zynqmp_kobj);
> > > +		pr_err("%s() GGS init fail with error %d\n",
> > > +		       __func__, ret);
> > > +		goto err;
> > > +	}
> > > +err:
> > > +	return ret;
> > > +}
> > > +
> > >  static int zynqmp_firmware_probe(struct platform_device *pdev)
> > >  {
> > >  	struct device *dev = &pdev->dev;
> > > @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct
> > platform_device *pdev)
> > >  	/* Assign eemi_ops_table */
> > >  	eemi_ops_tbl = &eemi_ops;
> > >
> > > +	ret = zynqmp_pm_sysfs_init();
> > 
> > See, you have a platform device, hang the attributes off of that instead
> > of making a kobject and detatching yourself from the global device tree!
> > 
> > Please redo this, I think it will make it a lot simpler and more
> > obvious.
> 
> Agree it will be simpler but to as firmware driver was changed to be platform driver,
> to keep paths same, we used sysfs.

Keep them same from what?  Use the platform device as that is what it is
there for, do not go create new apis when they are not needed at all.

thanks,

greg k-h

Sudeep Holla Jan. 24, 2020, 11:32 a.m. UTC | #4

Hi Greg,

Thanks for raising various issues that I have repeatedly asked and
constantly ignored.

On Fri, Jan 24, 2020 at 07:03:39AM +0100, Greg KH wrote:
> On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
> > Hi Greg,
> >

[...]

> > Firmware driver was changed later to be platform driver but these
> > interfaces were defined earlier and are in use.
>
> Defined and in use where?  Not in the upstream kernel tree, right?  Or
> am I missing them somewhere else?
>

Exactly and they keep saying there partners are using these for 3-4 years
and they want to retain that. I have told them to change several times.

> > > > +	ret = kstrtol(tok, 16, &value);
> > > > +	if (ret) {
> > > > +		ret = -EFAULT;
> > > > +		goto err;
> > > > +	}
> > > > +
> > > > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > >
> > > This feels "tricky", if you tie this to the device you have your driver
> > > bound to, will this make it easier instead of having to go through the
> > > ioctl callback?
> > >
> >
> > GGS(general global storage) registers are in PMU space and linux doesn't have access to it
> > Hence ioctl is used.
>
> Why not just a "real" call to the driver to make this type of reading?
> You don't have ioctls within the kernel for other drivers to call,
> that's not needed at all.
>

Oh yes, this is so broken in design. This firmware is designed to abstract
the power and configuration management on their platform, but they decided
to add direct register access to some registers anyway. Weird use case,
don't even ask. But I strongly objected such interface in sysfs even if
they moved under platform device. If they need this at any cost, I have
suggested debugfs.


[...]

> >
> > Agree it will be simpler but to as firmware driver was changed to be
> > platform driver, to keep paths same, we used sysfs.
>
> Keep them same from what?  Use the platform device as that is what it is
> there for, do not go create new apis when they are not needed at all.
>

+1, and please don't add any sysfs that can read/write those GGS registers
directly from userspace. Move them to debugfs if you are desperate to have
something.

--
Regards,
Sudeep

Jolly Shah Jan. 27, 2020, 10:46 p.m. UTC | #5

Hi Sudeep,

On 1/24/20, 3:32 AM, "Sudeep Holla" <[e-mail]> wrote:

    Hi Greg,
    
    Thanks for raising various issues that I have repeatedly asked and
    constantly ignored.

Sorry, never intended to ignore you. We agreed to your comments for reg access and that patch is already removed. GGS and PGGS registers are for general purpose registers(set of 4) for users. Since this registers belongs to PMU register space, interface is provided to read or write the way user wants. 
    
    On Fri, Jan 24, 2020 at 07:03:39AM +0100, Greg KH wrote:
    > On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
    > > Hi Greg,
    > >
    
    [...]
    
    > > Firmware driver was changed later to be platform driver but these
    > > interfaces were defined earlier and are in use.
    >
    > Defined and in use where?  Not in the upstream kernel tree, right?  Or
    > am I missing them somewhere else?
    >
    
    Exactly and they keep saying there partners are using these for 3-4 years
    and they want to retain that. I have told them to change several times.

Sorry we might have missed your comments for this change. We agree to your and greg's comment and will update sysfs path.


    > > > > +	ret = kstrtol(tok, 16, &value);
    > > > > +	if (ret) {
    > > > > +		ret = -EFAULT;
    > > > > +		goto err;
    > > > > +	}
    > > > > +
    > > > > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
    > > >
    > > > This feels "tricky", if you tie this to the device you have your driver
    > > > bound to, will this make it easier instead of having to go through the
    > > > ioctl callback?
    > > >
    > >
    > > GGS(general global storage) registers are in PMU space and linux doesn't have access to it
    > > Hence ioctl is used.
    >
    > Why not just a "real" call to the driver to make this type of reading?
    > You don't have ioctls within the kernel for other drivers to call,
    > that's not needed at all.
    >
    
    Oh yes, this is so broken in design. This firmware is designed to abstract
    the power and configuration management on their platform, but they decided
    to add direct register access to some registers anyway. Weird use case,
    don't even ask. But I strongly objected such interface in sysfs even if
    they moved under platform device. If they need this at any cost, I have
    suggested debugfs.
    
As mentioned, these registers are for users  and for special needs where users wants to retain values over resets. but as they belong to PMU address space,     these interface APIs are provided. They don’t allow access to any other registers.


    [...]
    
    > >
    > > Agree it will be simpler but to as firmware driver was changed to be
    > > platform driver, to keep paths same, we used sysfs.
    >
    > Keep them same from what?  Use the platform device as that is what it is
    > there for, do not go create new apis when they are not needed at all.
    >
    
    +1, and please don't add any sysfs that can read/write those GGS registers
    directly from userspace. Move them to debugfs if you are desperate to have
    something.


Thanks,
Jolly Shah

    
    --
    Regards,
    Sudeep

Jolly Shah Jan. 27, 2020, 11:01 p.m. UTC | #6

Hi Greg,

On 1/23/20, 10:03 PM, "Greg KH" <[e-mail]> wrote:

    On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
    > Hi Greg,
    > 
    > Thanks for the review.
    > 
    > > -----Original Message-----
    > > From: Greg KH <[e-mail]>
    > > Sent: Tuesday, January 14, 2020 6:53 AM
    > > To: Jolly Shah <[e-mail]>
    > > Cc: [e-mail]; [e-mail]; [e-mail];
    > > [e-mail]; [e-mail]; [e-mail];
    > > [e-mail]; Michal Simek <[e-mail]>; Rajan Vaja
    > > <[e-mail]>; [e-mail]; linux-
    > > [e-mail]; Rajan Vaja <[e-mail]>; Jolly Shah
    > > <[e-mail]>
    > > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
    > > 
    > > On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
    > > > From: Rajan Vaja <[e-mail]>
    > > >
    > > > Add Firmware-ggs sysfs interface which provides read/write
    > > > interface to global storage registers.
    > > >
    > > > Signed-off-by: Rajan Vaja <[e-mail]>
    > > > Signed-off-by: Michal Simek <[e-mail]>
    > > > Signed-off-by: Jolly Shah <[e-mail]>
    > > > ---
    > > > Changes in v2:
    > > >  - Updated Linux kernel version in documentation.
    > > >  - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
    > > >  - Free Kobject structure in case of error.
    > > >  - Resolved smatch errors.
    > > >  - Updated Signed-off-by sequence.
    > > > ---
    > > >  Documentation/ABI/stable/sysfs-firmware-zynqmp |  50 +++++
    > > >  drivers/firmware/xilinx/Makefile               |   2 +-
    > > >  drivers/firmware/xilinx/zynqmp-ggs.c           | 284
    > > +++++++++++++++++++++++++
    > > >  drivers/firmware/xilinx/zynqmp.c               |  32 +++
    > > >  include/linux/firmware/xlnx-zynqmp.h           |  10 +
    > > >  5 files changed, 377 insertions(+), 1 deletion(-)
    > > >  create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
    > > >  create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
    > > >
    > > > diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp
    > > b/Documentation/ABI/stable/sysfs-firmware-zynqmp
    > > > new file mode 100644
    > > > index 0000000..cffa2fc
    > > > --- /dev/null
    > > > +++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
    > > > @@ -0,0 +1,50 @@
    > > > +What:		/sys/firmware/zynqmp/ggs*
    > > 
    > > Why are these attributes just not hanging off of the platform device for
    > > the firmware controller?  Why do you need a new subdir under "firmware"?
    > 
    > Firmware driver was changed later to be platform driver but these interfaces were defined 
    > earlier and are in use.
    
    Defined and in use where?  Not in the upstream kernel tree, right?  Or
    am I missing them somewhere else?

Sorry I meant Xilinx kernel users. We will update it as per your suggestion.
    
    > > > +	ret = kstrtol(tok, 16, &value);
    > > > +	if (ret) {
    > > > +		ret = -EFAULT;
    > > > +		goto err;
    > > > +	}
    > > > +
    > > > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
    > > 
    > > This feels "tricky", if you tie this to the device you have your driver
    > > bound to, will this make it easier instead of having to go through the
    > > ioctl callback?
    > > 
    > 
    > GGS(general global storage) registers are in PMU space and linux doesn't have access to it 
    > Hence ioctl is used.
    
    Why not just a "real" call to the driver to make this type of reading?
    You don't have ioctls within the kernel for other drivers to call,
    that's not needed at all.

these registers are for users  and for special needs where users wants to retain values over resets. but as they belong to PMU address space,     these interface APIs are provided. They don’t allow access to any other registers.
    
    > > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
    > > > +{
    > > > +	return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
    > > 
    > > You might be racing userspace here and loosing :(
    > 
    > Prob is called before user space is notified about sysfs so racing shouldn't happen.
    
    "shouldn't"?  How do you know this?

Since firmware driver is always built-in (we don't provide support to use as module), user space won't be available before prob is complete. Correct if I am wrong.
    
    > Or you are referring to some other race condition?
    
    Your kobject was created, and notified userspace that it was present and
    then sometime after that you add more attributes which userspace has no
    idea are there.
    
    If you use the proper device model interfaces, none of these problems
    would be there.

Ok we will update it.

    
    > 
    > > 
    > > > +}
    > > > diff --git a/drivers/firmware/xilinx/zynqmp.c
    > > b/drivers/firmware/xilinx/zynqmp.c
    > > > index 75bdfaa..4c1117d 100644
    > > > --- a/drivers/firmware/xilinx/zynqmp.c
    > > > +++ b/drivers/firmware/xilinx/zynqmp.c
    > > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
    > > >  	case IOCTL_GET_PLL_FRAC_MODE:
    > > >  	case IOCTL_SET_PLL_FRAC_DATA:
    > > >  	case IOCTL_GET_PLL_FRAC_DATA:
    > > > +	case IOCTL_WRITE_GGS:
    > > > +	case IOCTL_READ_GGS:
    > > > +	case IOCTL_WRITE_PGGS:
    > > > +	case IOCTL_READ_PGGS:
    > > 
    > > Huh???
    > 
    > Sorry not sure about your concern here. These registers are in PMU space and hence
    > Ioctl is needed to let linux access them.
    
    userspace or kernelspace?
    
    You seem to be mixing them both here.

They are in Platform Management Unit register address space so it allows only secure access. Hence for linux to access it, interface APIs are provided. 
    
    > 
    > > 
    > > >  		return 1;
    > > >  	default:
    > > >  		return 0;
    > > > @@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops
    > > *zynqmp_pm_get_eemi_ops(void)
    > > >  }
    > > >  EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
    > > >
    > > > +static int zynqmp_pm_sysfs_init(void)
    > > > +{
    > > > +	struct kobject *zynqmp_kobj;
    > > > +	int ret;
    > > > +
    > > > +	zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
    > > > +	if (!zynqmp_kobj) {
    > > > +		pr_err("zynqmp: Firmware kobj add failed.\n");
    > > > +		return -ENOMEM;
    > > > +	}
    > > > +
    > > > +	ret = zynqmp_pm_ggs_init(zynqmp_kobj);
    > > > +	if (ret) {
    > > > +		kobject_put(zynqmp_kobj);
    > > > +		pr_err("%s() GGS init fail with error %d\n",
    > > > +		       __func__, ret);
    > > > +		goto err;
    > > > +	}
    > > > +err:
    > > > +	return ret;
    > > > +}
    > > > +
    > > >  static int zynqmp_firmware_probe(struct platform_device *pdev)
    > > >  {
    > > >  	struct device *dev = &pdev->dev;
    > > > @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct
    > > platform_device *pdev)
    > > >  	/* Assign eemi_ops_table */
    > > >  	eemi_ops_tbl = &eemi_ops;
    > > >
    > > > +	ret = zynqmp_pm_sysfs_init();
    > > 
    > > See, you have a platform device, hang the attributes off of that instead
    > > of making a kobject and detatching yourself from the global device tree!
    > > 
    > > Please redo this, I think it will make it a lot simpler and more
    > > obvious.
    > 
    > Agree it will be simpler but to as firmware driver was changed to be platform driver,
    > to keep paths same, we used sysfs.
    
    Keep them same from what?  Use the platform device as that is what it is
    there for, do not go create new apis when they are not needed at all.

Ok we will update it in next version.

Thanks,
Jolly Shah
    

    thanks,
    
    greg k-h

Greg KH Jan. 28, 2020, 6:28 a.m. UTC | #7

On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
>     > > > +	ret = kstrtol(tok, 16, &value);
>     > > > +	if (ret) {
>     > > > +		ret = -EFAULT;
>     > > > +		goto err;
>     > > > +	}
>     > > > +
>     > > > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
>     > > 
>     > > This feels "tricky", if you tie this to the device you have your driver
>     > > bound to, will this make it easier instead of having to go through the
>     > > ioctl callback?
>     > > 
>     > 
>     > GGS(general global storage) registers are in PMU space and linux doesn't have access to it 
>     > Hence ioctl is used.
>     
>     Why not just a "real" call to the driver to make this type of reading?
>     You don't have ioctls within the kernel for other drivers to call,
>     that's not needed at all.
> 
> these registers are for users  and for special needs where users wants
> to retain values over resets. but as they belong to PMU address space,
> these interface APIs are provided. They don’t allow access to any
> other registers.

That's not the issue here.  The issue is you are using an "internal"
ioctl, instead just make a "real" call.

>     > > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
>     > > > +{
>     > > > +	return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
>     > > 
>     > > You might be racing userspace here and loosing :(
>     > 
>     > Prob is called before user space is notified about sysfs so racing shouldn't happen.
>     
>     "shouldn't"?  How do you know this?
> 
> Since firmware driver is always built-in (we don't provide support to
> use as module), user space won't be available before prob is complete.
> Correct if I am wrong.

Userspace starts earlier than you think, and also, use the correct
interfaces for this type of thing, that is why it is there.  Don't
create purposfully-incorrect code :)

>     > > > diff --git a/drivers/firmware/xilinx/zynqmp.c
>     > > b/drivers/firmware/xilinx/zynqmp.c
>     > > > index 75bdfaa..4c1117d 100644
>     > > > --- a/drivers/firmware/xilinx/zynqmp.c
>     > > > +++ b/drivers/firmware/xilinx/zynqmp.c
>     > > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
>     > > >  	case IOCTL_GET_PLL_FRAC_MODE:
>     > > >  	case IOCTL_SET_PLL_FRAC_DATA:
>     > > >  	case IOCTL_GET_PLL_FRAC_DATA:
>     > > > +	case IOCTL_WRITE_GGS:
>     > > > +	case IOCTL_READ_GGS:
>     > > > +	case IOCTL_WRITE_PGGS:
>     > > > +	case IOCTL_READ_PGGS:
>     > > 
>     > > Huh???
>     > 
>     > Sorry not sure about your concern here. These registers are in PMU space and hence
>     > Ioctl is needed to let linux access them.
>     
>     userspace or kernelspace?
>     
>     You seem to be mixing them both here.
> 
> They are in Platform Management Unit register address space so it
> allows only secure access. Hence for linux to access it, interface
> APIs are provided. 

Again, that's fine, but why are you creating an "internal ioctl"?  Just
make a real function call.

thanks,

greg k-h

Jolly Shah Jan. 30, 2020, 11:59 p.m. UTC | #8

Hi Greg,

On 1/27/20, 10:28 PM, "[e-mail] on behalf of Greg KH" <[e-mail] on behalf of [e-mail]> wrote:

    On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
    >     > > > +	ret = kstrtol(tok, 16, &value);
    >     > > > +	if (ret) {
    >     > > > +		ret = -EFAULT;
    >     > > > +		goto err;
    >     > > > +	}
    >     > > > +
    >     > > > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
    >     > > 
    >     > > This feels "tricky", if you tie this to the device you have your driver
    >     > > bound to, will this make it easier instead of having to go through the
    >     > > ioctl callback?
    >     > > 
    >     > 
    >     > GGS(general global storage) registers are in PMU space and linux doesn't have access to it 
    >     > Hence ioctl is used.
    >     
    >     Why not just a "real" call to the driver to make this type of reading?
    >     You don't have ioctls within the kernel for other drivers to call,
    >     that's not needed at all.
    > 
    > these registers are for users  and for special needs where users wants
    > to retain values over resets. but as they belong to PMU address space,
    > these interface APIs are provided. They don’t allow access to any
    > other registers.
    
    That's not the issue here.  The issue is you are using an "internal"
    ioctl, instead just make a "real" call.

Sorry I am not clear. Do you mean that we should use linux standard ioctl interface instead of internal ioctl by mentioning "real" ?
    
    >     > > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
    >     > > > +{
    >     > > > +	return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
    >     > > 
    >     > > You might be racing userspace here and loosing :(
    >     > 
    >     > Prob is called before user space is notified about sysfs so racing shouldn't happen.
    >     
    >     "shouldn't"?  How do you know this?
    > 
    > Since firmware driver is always built-in (we don't provide support to
    > use as module), user space won't be available before prob is complete.
    > Correct if I am wrong.
    
    Userspace starts earlier than you think, and also, use the correct
    interfaces for this type of thing, that is why it is there.  Don't
    create purposfully-incorrect code :)

Sure. We will change it.

Thanks,
Jolly Shah

    
    >     > > > diff --git a/drivers/firmware/xilinx/zynqmp.c
    >     > > b/drivers/firmware/xilinx/zynqmp.c
    >     > > > index 75bdfaa..4c1117d 100644
    >     > > > --- a/drivers/firmware/xilinx/zynqmp.c
    >     > > > +++ b/drivers/firmware/xilinx/zynqmp.c
    >     > > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
    >     > > >  	case IOCTL_GET_PLL_FRAC_MODE:
    >     > > >  	case IOCTL_SET_PLL_FRAC_DATA:
    >     > > >  	case IOCTL_GET_PLL_FRAC_DATA:
    >     > > > +	case IOCTL_WRITE_GGS:
    >     > > > +	case IOCTL_READ_GGS:
    >     > > > +	case IOCTL_WRITE_PGGS:
    >     > > > +	case IOCTL_READ_PGGS:
    >     > > 
    >     > > Huh???
    >     > 
    >     > Sorry not sure about your concern here. These registers are in PMU space and hence
    >     > Ioctl is needed to let linux access them.
    >     
    >     userspace or kernelspace?
    >     
    >     You seem to be mixing them both here.
    > 
    > They are in Platform Management Unit register address space so it
    > allows only secure access. Hence for linux to access it, interface
    > APIs are provided. 
    
    Again, that's fine, but why are you creating an "internal ioctl"?  Just
    make a real function call.
    
    thanks,
    
    greg k-h

Greg KH Jan. 31, 2020, 6:10 a.m. UTC | #9

On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> Hi Greg,
> 
> On 1/27/20, 10:28 PM, "[e-mail] on behalf of Greg KH" <[e-mail] on behalf of [e-mail]> wrote:
> 
>     On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
>     >     > > > +	ret = kstrtol(tok, 16, &value);
>     >     > > > +	if (ret) {
>     >     > > > +		ret = -EFAULT;
>     >     > > > +		goto err;
>     >     > > > +	}
>     >     > > > +
>     >     > > > +	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
>     >     > > 
>     >     > > This feels "tricky", if you tie this to the device you have your driver
>     >     > > bound to, will this make it easier instead of having to go through the
>     >     > > ioctl callback?
>     >     > > 
>     >     > 
>     >     > GGS(general global storage) registers are in PMU space and linux doesn't have access to it 
>     >     > Hence ioctl is used.
>     >     
>     >     Why not just a "real" call to the driver to make this type of reading?
>     >     You don't have ioctls within the kernel for other drivers to call,
>     >     that's not needed at all.
>     > 
>     > these registers are for users  and for special needs where users wants
>     > to retain values over resets. but as they belong to PMU address space,
>     > these interface APIs are provided. They don’t allow access to any
>     > other registers.
>     
>     That's not the issue here.  The issue is you are using an "internal"
>     ioctl, instead just make a "real" call.
> 
> Sorry I am not clear. Do you mean that we should use linux standard
> ioctl interface instead of internal ioctl by mentioning "real" ?

No, you should just make a "real" function call to the exact thing you
want to do.  Not have an internal multi-plexor ioctl() call that others
then call.  This isn't a microkernel :)

thanks,

greg k-h

Rajan Vaja Jan. 31, 2020, 9:05 a.m. UTC | #10

Hi Greg,

> -----Original Message-----
> From: Greg KH <[e-mail]>
> Sent: 31 January 2020 11:41 AM
> To: Jolly Shah <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Michal Simek <[e-mail]>; Rajan Vaja
> <[e-mail]>; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> 
> EXTERNAL EMAIL
> 
> On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> > Hi Greg,
> >
> > On 1/27/20, 10:28 PM, "[e-mail] on behalf of Greg
> KH" <[e-mail] on behalf of
> [e-mail]> wrote:
> >
> >     On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> >     >     > > > +     ret = kstrtol(tok, 16, &value);
> >     >     > > > +     if (ret) {
> >     >     > > > +             ret = -EFAULT;
> >     >     > > > +             goto err;
> >     >     > > > +     }
> >     >     > > > +
> >     >     > > > +     ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> >     >     > >
> >     >     > > This feels "tricky", if you tie this to the device you have your driver
> >     >     > > bound to, will this make it easier instead of having to go through the
> >     >     > > ioctl callback?
> >     >     > >
> >     >     >
> >     >     > GGS(general global storage) registers are in PMU space and linux
> doesn't have access to it
> >     >     > Hence ioctl is used.
> >     >
> >     >     Why not just a "real" call to the driver to make this type of reading?
> >     >     You don't have ioctls within the kernel for other drivers to call,
> >     >     that's not needed at all.
> >     >
> >     > these registers are for users  and for special needs where users wants
> >     > to retain values over resets. but as they belong to PMU address space,
> >     > these interface APIs are provided. They don’t allow access to any
> >     > other registers.
> >
> >     That's not the issue here.  The issue is you are using an "internal"
> >     ioctl, instead just make a "real" call.
> >
> > Sorry I am not clear. Do you mean that we should use linux standard
> > ioctl interface instead of internal ioctl by mentioning "real" ?
> 
> No, you should just make a "real" function call to the exact thing you
> want to do.  Not have an internal multi-plexor ioctl() call that others
> then call.  This isn't a microkernel :)
[Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?

Thanks,
Rajan

> 
> thanks,
> 
> greg k-h

Greg KH Jan. 31, 2020, 9:36 a.m. UTC | #11

On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg KH <[e-mail]>
> > Sent: 31 January 2020 11:41 AM
> > To: Jolly Shah <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Michal Simek <[e-mail]>; Rajan Vaja
> > <[e-mail]>; [e-mail]; linux-
> > [e-mail]
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> > 
> > EXTERNAL EMAIL
> > 
> > On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> > > Hi Greg,
> > >
> > > On 1/27/20, 10:28 PM, "[e-mail] on behalf of Greg
> > KH" <[e-mail] on behalf of
> > [e-mail]> wrote:
> > >
> > >     On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > >     >     > > > +     ret = kstrtol(tok, 16, &value);
> > >     >     > > > +     if (ret) {
> > >     >     > > > +             ret = -EFAULT;
> > >     >     > > > +             goto err;
> > >     >     > > > +     }
> > >     >     > > > +
> > >     >     > > > +     ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > >     >     > >
> > >     >     > > This feels "tricky", if you tie this to the device you have your driver
> > >     >     > > bound to, will this make it easier instead of having to go through the
> > >     >     > > ioctl callback?
> > >     >     > >
> > >     >     >
> > >     >     > GGS(general global storage) registers are in PMU space and linux
> > doesn't have access to it
> > >     >     > Hence ioctl is used.
> > >     >
> > >     >     Why not just a "real" call to the driver to make this type of reading?
> > >     >     You don't have ioctls within the kernel for other drivers to call,
> > >     >     that's not needed at all.
> > >     >
> > >     > these registers are for users  and for special needs where users wants
> > >     > to retain values over resets. but as they belong to PMU address space,
> > >     > these interface APIs are provided. They don’t allow access to any
> > >     > other registers.
> > >
> > >     That's not the issue here.  The issue is you are using an "internal"
> > >     ioctl, instead just make a "real" call.
> > >
> > > Sorry I am not clear. Do you mean that we should use linux standard
> > > ioctl interface instead of internal ioctl by mentioning "real" ?
> > 
> > No, you should just make a "real" function call to the exact thing you
> > want to do.  Not have an internal multi-plexor ioctl() call that others
> > then call.  This isn't a microkernel :)
> [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
> Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?

That is correct.

Jolly Shah Feb. 11, 2020, 12:57 a.m. UTC | #12

Hi Greg,

 > ------Original Message------
 > From: 'Greg Kh' <[e-mail]>
 > Sent:  Friday, January 31, 2020 1:36AM
 > To: Rajan Vaja <[e-mail]>
 > Cc: Jolly Shah <[e-mail]>, Ard Biesheuvel 
<[e-mail]>, Mingo <[e-mail]>, Matt 
<[e-mail]>, Sudeep Holla <[e-mail]>, 
Hkallweit1 <[e-mail]>, Keescook <[e-mail]>, 
Dmitry Torokhov <[e-mail]>, Michal Simek 
<[e-mail]>, Linux-arm-kernel 
<[e-mail]>, Linux-kernel 
<[e-mail]>
 > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
 >
> On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
>> Hi Greg,
>>
>>> -----Original Message-----
>>> From: Greg KH <[e-mail]>
>>> Sent: 31 January 2020 11:41 AM
>>> To: Jolly Shah <[e-mail]>
>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; [e-mail]; [e-mail];
>>> [e-mail]; Michal Simek <[e-mail]>; Rajan Vaja
>>> <[e-mail]>; [e-mail]; linux-
>>> [e-mail]
>>> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>>>
>>> EXTERNAL EMAIL
>>>
>>> On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
>>>> Hi Greg,
>>>>
>>>> On 1/27/20, 10:28 PM, "[e-mail] on behalf of Greg
>>> KH" <[e-mail] on behalf of
>>> [e-mail]> wrote:
>>>>
>>>>      On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
>>>>      >     > > > +     ret = kstrtol(tok, 16, &value);
>>>>      >     > > > +     if (ret) {
>>>>      >     > > > +             ret = -EFAULT;
>>>>      >     > > > +             goto err;
>>>>      >     > > > +     }
>>>>      >     > > > +
>>>>      >     > > > +     ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
>>>>      >     > >
>>>>      >     > > This feels "tricky", if you tie this to the device you have your driver
>>>>      >     > > bound to, will this make it easier instead of having to go through the
>>>>      >     > > ioctl callback?
>>>>      >     > >
>>>>      >     >
>>>>      >     > GGS(general global storage) registers are in PMU space and linux
>>> doesn't have access to it
>>>>      >     > Hence ioctl is used.
>>>>      >
>>>>      >     Why not just a "real" call to the driver to make this type of reading?
>>>>      >     You don't have ioctls within the kernel for other drivers to call,
>>>>      >     that's not needed at all.
>>>>      >
>>>>      > these registers are for users  and for special needs where users wants
>>>>      > to retain values over resets. but as they belong to PMU address space,
>>>>      > these interface APIs are provided. They don’t allow access to any
>>>>      > other registers.
>>>>
>>>>      That's not the issue here.  The issue is you are using an "internal"
>>>>      ioctl, instead just make a "real" call.
>>>>
>>>> Sorry I am not clear. Do you mean that we should use linux standard
>>>> ioctl interface instead of internal ioctl by mentioning "real" ?
>>>
>>> No, you should just make a "real" function call to the exact thing you
>>> want to do.  Not have an internal multi-plexor ioctl() call that others
>>> then call.  This isn't a microkernel :)
>> [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
>> Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
> 
> That is correct.



Would like to clarify purpose of having ioctl API to avoid any confusion.
eemi interface apis are defined to be platform independent and allows 
clock, reset, power etc management through firmware but apart from these 
generic operations, there are some operations which needs secure access 
through firmware. Examples are accessing some storage registers(ggs and 
pggs) for inter agent communication, configuring another agent(RPU) 
mode, boot device configuration etc. Those operations are covered as 
ioctls as they are very platform specific. Also only whitelisted 
operations are allowed through ioctl and is not exposed to user for any 
random read/write operation.

Olof earlier had same concerns. We had clarified the purpose and with 
his agreement, initial set of ioctls were accepted. 
(https://www.lkml.org/lkml/2018/9/24/1570)

Please suggest the best approach to handle this for current and future 
patches.

Thanks,
Jolly Shah


>

Greg KH Feb. 14, 2020, 5:10 p.m. UTC | #13

On Mon, Feb 10, 2020 at 04:57:01PM -0800, Jolly Shah wrote:
> Hi Greg,
> 
> > ------Original Message------
> > From: 'Greg Kh' <[e-mail]>
> > Sent:  Friday, January 31, 2020 1:36AM
> > To: Rajan Vaja <[e-mail]>
> > Cc: Jolly Shah <[e-mail]>, Ard Biesheuvel
> <[e-mail]>, Mingo <[e-mail]>, Matt
> <[e-mail]>, Sudeep Holla <[e-mail]>, Hkallweit1
> <[e-mail]>, Keescook <[e-mail]>, Dmitry Torokhov
> <[e-mail]>, Michal Simek <[e-mail]>,
> Linux-arm-kernel <[e-mail]>, Linux-kernel
> <[e-mail]>
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> >
> > On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
> > > Hi Greg,
> > > 
> > > > -----Original Message-----
> > > > From: Greg KH <[e-mail]>
> > > > Sent: 31 January 2020 11:41 AM
> > > > To: Jolly Shah <[e-mail]>
> > > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; Michal Simek <[e-mail]>; Rajan Vaja
> > > > <[e-mail]>; [e-mail]; linux-
> > > > [e-mail]
> > > > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> > > > 
> > > > EXTERNAL EMAIL
> > > > 
> > > > On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> > > > > Hi Greg,
> > > > > 
> > > > > On 1/27/20, 10:28 PM, "[e-mail] on behalf of Greg
> > > > KH" <[e-mail] on behalf of
> > > > [e-mail]> wrote:
> > > > > 
> > > > >      On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > > > >      >     > > > +     ret = kstrtol(tok, 16, &value);
> > > > >      >     > > > +     if (ret) {
> > > > >      >     > > > +             ret = -EFAULT;
> > > > >      >     > > > +             goto err;
> > > > >      >     > > > +     }
> > > > >      >     > > > +
> > > > >      >     > > > +     ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > > > >      >     > >
> > > > >      >     > > This feels "tricky", if you tie this to the device you have your driver
> > > > >      >     > > bound to, will this make it easier instead of having to go through the
> > > > >      >     > > ioctl callback?
> > > > >      >     > >
> > > > >      >     >
> > > > >      >     > GGS(general global storage) registers are in PMU space and linux
> > > > doesn't have access to it
> > > > >      >     > Hence ioctl is used.
> > > > >      >
> > > > >      >     Why not just a "real" call to the driver to make this type of reading?
> > > > >      >     You don't have ioctls within the kernel for other drivers to call,
> > > > >      >     that's not needed at all.
> > > > >      >
> > > > >      > these registers are for users  and for special needs where users wants
> > > > >      > to retain values over resets. but as they belong to PMU address space,
> > > > >      > these interface APIs are provided. They don’t allow access to any
> > > > >      > other registers.
> > > > > 
> > > > >      That's not the issue here.  The issue is you are using an "internal"
> > > > >      ioctl, instead just make a "real" call.
> > > > > 
> > > > > Sorry I am not clear. Do you mean that we should use linux standard
> > > > > ioctl interface instead of internal ioctl by mentioning "real" ?
> > > > 
> > > > No, you should just make a "real" function call to the exact thing you
> > > > want to do.  Not have an internal multi-plexor ioctl() call that others
> > > > then call.  This isn't a microkernel :)
> > > [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
> > > Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
> > 
> > That is correct.
> 
> 
> 
> Would like to clarify purpose of having ioctl API to avoid any confusion.
> eemi interface apis are defined to be platform independent and allows clock,
> reset, power etc management through firmware but apart from these generic
> operations, there are some operations which needs secure access through
> firmware. Examples are accessing some storage registers(ggs and pggs) for
> inter agent communication, configuring another agent(RPU) mode, boot device
> configuration etc. Those operations are covered as ioctls as they are very
> platform specific. Also only whitelisted operations are allowed through
> ioctl and is not exposed to user for any random read/write operation.
> 
> Olof earlier had same concerns. We had clarified the purpose and with his
> agreement, initial set of ioctls were accepted.
> (https://www.lkml.org/lkml/2018/9/24/1570)
> 
> Please suggest the best approach to handle this for current and future
> patches.

Ok, in looking further at this, it's both better than I thought, and
totally worse.

This interface you all are using where you ask the firmware driver for a
pointer to a set of operation functions and then make calls through that
is indicitive of an api that is NOT what we normally use in Linux at
all.

Just make the direct call to the firmware driver, no need to muck around
with tables of function pointers.  In fact, with the spectre changes,
you just made things slower than needed, and you can get back a bunch of
throughput by removing that whole middle layer.

So go do that first please, before adding any new stuff.

Now for the ioctl, yeah, that's not a "normal" pattern either.  But
right now you only have 2 "different" ioctls that you call.  So why not
just turn those 2 into real function calls as well that then makes the
"ioctl" call to the hardware?  That makes things a lot more obvious on
the kernel driver side exactly what is going on.

If you need to add more "ioctl" like calls, just add them as more
functions, no big deal.  How many more of these are you going to need
over time?

But that's not all that big of a deal right now, get rid of that whole
middle-layer first, that's more important to clean up.  You will get rid
of a lot of unneeded code and indirection that way, making it simpler
and easier to understand what exactly is happening.

thanks,

greg k-h

Jolly Shah Feb. 15, 2020, 12:37 a.m. UTC | #14

Hi Greg,

Thanks for the response.

 > ------Original Message------
 > From: 'Greg Kh' <[e-mail]>
 > Sent:  Friday, February 14, 2020 9:10AM
 > To: Jolly Shah <[e-mail]>
 > Cc: Rajan Vaja <[e-mail]>, [e-mail] 
<[e-mail]>, [e-mail] <[e-mail]>, 
[e-mail] <[e-mail]>, 
[e-mail] <[e-mail]>, [e-mail] 
<[e-mail]>, [e-mail] <[e-mail]>, 
[e-mail] <[e-mail]>, Michal Simek 
<[e-mail]>, [e-mail] 
<[e-mail]>, [e-mail] 
<[e-mail]>
 > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
 >
> On Mon, Feb 10, 2020 at 04:57:01PM -0800, Jolly Shah wrote:
>> Hi Greg,
>>
>>> ------Original Message------
>>> From: 'Greg Kh' <[e-mail]>
>>> Sent:  Friday, January 31, 2020 1:36AM
>>> To: Rajan Vaja <[e-mail]>
>>> Cc: Jolly Shah <[e-mail]>, Ard Biesheuvel
>> <[e-mail]>, Mingo <[e-mail]>, Matt
>> <[e-mail]>, Sudeep Holla <[e-mail]>, Hkallweit1
>> <[e-mail]>, Keescook <[e-mail]>, Dmitry Torokhov
>> <[e-mail]>, Michal Simek <[e-mail]>,
>> Linux-arm-kernel <[e-mail]>, Linux-kernel
>> <[e-mail]>
>>> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>>>
>>> On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
>>>> Hi Greg,
>>>>
>>>>> -----Original Message-----
>>>>> From: Greg KH <[e-mail]>
>>>>> Sent: 31 January 2020 11:41 AM
>>>>> To: Jolly Shah <[e-mail]>
>>>>> Cc: [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>> [e-mail]; Michal Simek <[e-mail]>; Rajan Vaja
>>>>> <[e-mail]>; [e-mail]; linux-
>>>>> [e-mail]
>>>>> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>>>>>
>>>>> EXTERNAL EMAIL
>>>>>
>>>>> On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On 1/27/20, 10:28 PM, "[e-mail] on behalf of Greg
>>>>> KH" <[e-mail] on behalf of
>>>>> [e-mail]> wrote:
>>>>>>
>>>>>>       On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
>>>>>>       >     > > > +     ret = kstrtol(tok, 16, &value);
>>>>>>       >     > > > +     if (ret) {
>>>>>>       >     > > > +             ret = -EFAULT;
>>>>>>       >     > > > +             goto err;
>>>>>>       >     > > > +     }
>>>>>>       >     > > > +
>>>>>>       >     > > > +     ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
>>>>>>       >     > >
>>>>>>       >     > > This feels "tricky", if you tie this to the device you have your driver
>>>>>>       >     > > bound to, will this make it easier instead of having to go through the
>>>>>>       >     > > ioctl callback?
>>>>>>       >     > >
>>>>>>       >     >
>>>>>>       >     > GGS(general global storage) registers are in PMU space and linux
>>>>> doesn't have access to it
>>>>>>       >     > Hence ioctl is used.
>>>>>>       >
>>>>>>       >     Why not just a "real" call to the driver to make this type of reading?
>>>>>>       >     You don't have ioctls within the kernel for other drivers to call,
>>>>>>       >     that's not needed at all.
>>>>>>       >
>>>>>>       > these registers are for users  and for special needs where users wants
>>>>>>       > to retain values over resets. but as they belong to PMU address space,
>>>>>>       > these interface APIs are provided. They don’t allow access to any
>>>>>>       > other registers.
>>>>>>
>>>>>>       That's not the issue here.  The issue is you are using an "internal"
>>>>>>       ioctl, instead just make a "real" call.
>>>>>>
>>>>>> Sorry I am not clear. Do you mean that we should use linux standard
>>>>>> ioctl interface instead of internal ioctl by mentioning "real" ?
>>>>>
>>>>> No, you should just make a "real" function call to the exact thing you
>>>>> want to do.  Not have an internal multi-plexor ioctl() call that others
>>>>> then call.  This isn't a microkernel :)
>>>> [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
>>>> Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
>>>
>>> That is correct.
>>
>>
>>
>> Would like to clarify purpose of having ioctl API to avoid any confusion.
>> eemi interface apis are defined to be platform independent and allows clock,
>> reset, power etc management through firmware but apart from these generic
>> operations, there are some operations which needs secure access through
>> firmware. Examples are accessing some storage registers(ggs and pggs) for
>> inter agent communication, configuring another agent(RPU) mode, boot device
>> configuration etc. Those operations are covered as ioctls as they are very
>> platform specific. Also only whitelisted operations are allowed through
>> ioctl and is not exposed to user for any random read/write operation.
>>
>> Olof earlier had same concerns. We had clarified the purpose and with his
>> agreement, initial set of ioctls were accepted.
>> (https://www.lkml.org/lkml/2018/9/24/1570)
>>
>> Please suggest the best approach to handle this for current and future
>> patches.
> 
> Ok, in looking further at this, it's both better than I thought, and
> totally worse.
> 
> This interface you all are using where you ask the firmware driver for a
> pointer to a set of operation functions and then make calls through that
> is indicitive of an api that is NOT what we normally use in Linux at
> all.
> 
> Just make the direct call to the firmware driver, no need to muck around
> with tables of function pointers.  In fact, with the spectre changes,
> you just made things slower than needed, and you can get back a bunch of
> throughput by removing that whole middle layer.
> 

arm,scpi is doing the same way and we thought this approach will be more 
acceptable than direct function calls but happy to change as suggested.

> So go do that first please, before adding any new stuff.
> 
> Now for the ioctl, yeah, that's not a "normal" pattern either.  But
> right now you only have 2 "different" ioctls that you call.  So why not
> just turn those 2 into real function calls as well that then makes the
> "ioctl" call to the hardware?  That makes things a lot more obvious on
> the kernel driver side exactly what is going on.
> 

Sure as i understand firmware driver will provide real function calls to 
be used by user drivers and underneath it will call ioctl for desired 
operation. Please correct if I misunderstood.

Thanks,
Jolly Shah


> If you need to add more "ioctl" like calls, just add them as more
> functions, no big deal.  How many more of these are you going to need
> over time?
> 
> But that's not all that big of a deal right now, get rid of that whole
> middle-layer first, that's more important to clean up.  You will get rid
> of a lot of unneeded code and indirection that way, making it simpler
> and easier to understand what exactly is happening.
> 
> thanks,
> 
> greg k-h
>

Greg KH Feb. 15, 2020, 12:52 a.m. UTC | #15

On Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
> > Just make the direct call to the firmware driver, no need to muck around
> > with tables of function pointers.  In fact, with the spectre changes,
> > you just made things slower than needed, and you can get back a bunch of
> > throughput by removing that whole middle layer.
> > 
> 
> arm,scpi is doing the same way and we thought this approach will be more
> acceptable than direct function calls but happy to change as suggested.

Just because one random tiny thing does it the wrong way does not mean
to focus on that design pattern and ignore the thousands of other
apis/interfaces in the kernel that do not do it that way :)

> > So go do that first please, before adding any new stuff.
> > 
> > Now for the ioctl, yeah, that's not a "normal" pattern either.  But
> > right now you only have 2 "different" ioctls that you call.  So why not
> > just turn those 2 into real function calls as well that then makes the
> > "ioctl" call to the hardware?  That makes things a lot more obvious on
> > the kernel driver side exactly what is going on.
> > 
> 
> Sure as i understand firmware driver will provide real function calls to be
> used by user drivers and underneath it will call ioctl for desired
> operation. Please correct if I misunderstood.

You do not misunderstand.

thanks,

greg k-h

Jolly Shah Feb. 19, 2020, 10:50 p.m. UTC | #16

Hi Greg,

On 2/14/20, 4:58 PM, "[e-mail] on behalf of 'Greg KH'" <[e-mail] on behalf of [e-mail]> wrote:

    On Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
    > > Just make the direct call to the firmware driver, no need to muck around
    > > with tables of function pointers.  In fact, with the spectre changes,
    > > you just made things slower than needed, and you can get back a bunch of
    > > throughput by removing that whole middle layer.
    > > 
    > 
    > arm,scpi is doing the same way and we thought this approach will be more
    > acceptable than direct function calls but happy to change as suggested.
    
    Just because one random tiny thing does it the wrong way does not mean
    to focus on that design pattern and ignore the thousands of other
    apis/interfaces in the kernel that do not do it that way :)
    
Sure. Will change in next version.

    > > So go do that first please, before adding any new stuff.
    > > 
    > > Now for the ioctl, yeah, that's not a "normal" pattern either.  But
    > > right now you only have 2 "different" ioctls that you call.  So why not
    > > just turn those 2 into real function calls as well that then makes the
    > > "ioctl" call to the hardware?  That makes things a lot more obvious on
    > > the kernel driver side exactly what is going on.
    > > 
    > 
    > Sure as i understand firmware driver will provide real function calls to be
    > used by user drivers and underneath it will call ioctl for desired
    > operation. Please correct if I misunderstood.
    
    You do not misunderstand.

Thanks for confirmation.

Thanks,
Jolly Shah
    
    thanks,
    
    greg k-h

Jolly Shah March 6, 2020, 11:55 p.m. UTC | #17

Hi Greg,

 > ------Original Message------
 > From: 'Greg Kh' <[e-mail]>
 > Sent:  Friday, February 14, 2020 4:52PM
 > To: Jolly Shah <[e-mail]>
 > Cc: Rajan Vaja <[e-mail]>, [e-mail] 
<[e-mail]>, [e-mail] <[e-mail]>, 
[e-mail] <[e-mail]>, 
[e-mail] <[e-mail]>, [e-mail] 
<[e-mail]>, [e-mail] <[e-mail]>, 
[e-mail] <[e-mail]>, Michal Simek 
<[e-mail]>, [e-mail] 
<[e-mail]>, [e-mail] 
<[e-mail]>
 > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
 >
> On Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
>>> Just make the direct call to the firmware driver, no need to muck around
>>> with tables of function pointers.  In fact, with the spectre changes,
>>> you just made things slower than needed, and you can get back a bunch of
>>> throughput by removing that whole middle layer.
>>>
>>
>> arm,scpi is doing the same way and we thought this approach will be more
>> acceptable than direct function calls but happy to change as suggested.
> 
> Just because one random tiny thing does it the wrong way does not mean
> to focus on that design pattern and ignore the thousands of other
> apis/interfaces in the kernel that do not do it that way :)
> 
>>> So go do that first please, before adding any new stuff.
>>>
>>> Now for the ioctl, yeah, that's not a "normal" pattern either.  But
>>> right now you only have 2 "different" ioctls that you call.  So why not
>>> just turn those 2 into real function calls as well that then makes the
>>> "ioctl" call to the hardware?  That makes things a lot more obvious on
>>> the kernel driver side exactly what is going on.
>>>
>>
>> Sure as i understand firmware driver will provide real function calls to be
>> used by user drivers and underneath it will call ioctl for desired
>> operation. Please correct if I misunderstood.
> 
> You do not misunderstand.

Submitted v3 with required changes. Please review.

Thanks,
Jolly Shah

> 
> thanks,
> 
> greg k-h
>

Greg KH March 7, 2020, 8:47 a.m. UTC | #18

On Fri, Mar 06, 2020 at 03:55:46PM -0800, Jolly Shah wrote:
> Hi Greg,
> 
> > ------Original Message------
> > From: 'Greg Kh' <[e-mail]>
> > Sent:  Friday, February 14, 2020 4:52PM
> > To: Jolly Shah <[e-mail]>
> > Cc: Rajan Vaja <[e-mail]>, [e-mail]
> <[e-mail]>, [e-mail] <[e-mail]>,
> [e-mail] <[e-mail]>, [e-mail]
> <[e-mail]>, [e-mail] <[e-mail]>,
> [e-mail] <[e-mail]>, [e-mail]
> <[e-mail]>, Michal Simek <[e-mail]>,
> [e-mail] <[e-mail]>,
> [e-mail] <[e-mail]>
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> >
> > On Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
> > > > Just make the direct call to the firmware driver, no need to muck around
> > > > with tables of function pointers.  In fact, with the spectre changes,
> > > > you just made things slower than needed, and you can get back a bunch of
> > > > throughput by removing that whole middle layer.
> > > > 
> > > 
> > > arm,scpi is doing the same way and we thought this approach will be more
> > > acceptable than direct function calls but happy to change as suggested.
> > 
> > Just because one random tiny thing does it the wrong way does not mean
> > to focus on that design pattern and ignore the thousands of other
> > apis/interfaces in the kernel that do not do it that way :)
> > 
> > > > So go do that first please, before adding any new stuff.
> > > > 
> > > > Now for the ioctl, yeah, that's not a "normal" pattern either.  But
> > > > right now you only have 2 "different" ioctls that you call.  So why not
> > > > just turn those 2 into real function calls as well that then makes the
> > > > "ioctl" call to the hardware?  That makes things a lot more obvious on
> > > > the kernel driver side exactly what is going on.
> > > > 
> > > 
> > > Sure as i understand firmware driver will provide real function calls to be
> > > used by user drivers and underneath it will call ioctl for desired
> > > operation. Please correct if I misunderstood.
> > 
> > You do not misunderstand.
> 
> Submitted v3 with required changes. Please review.

Will do, when I get to it, relax :)

Patch

diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp b/Documentation/ABI/stable/sysfs-firmware-zynqmp
new file mode 100644
index 0000000..cffa2fc
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
@@ -0,0 +1,50 @@ 
+What:		/sys/firmware/zynqmp/ggs*
+Date:		January 2018
+KernelVersion:	5.5
+Contact:	"Jolly Shah" <[e-mail]>
+Description:
+		Read/Write PMU global general storage register value,
+		GLOBAL_GEN_STORAGE{0:3}.
+		Global general storage register that can be used
+		by system to pass information between masters.
+
+		The register is reset during system or power-on
+		resets. Three registers are used by the FSBL and
+		other Xilinx software products: GLOBAL_GEN_STORAGE{4:6}.
+
+		Usage:
+		# cat /sys/firmware/zynqmp/ggs0
+		# echo <mask> <value> > /sys/firmware/zynqmp/ggs0
+
+		Example:
+		# cat /sys/firmware/zynqmp/ggs0
+		# echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
+
+Users:		Xilinx
+
+What:		/sys/firmware/zynqmp/pggs*
+Date:		January 2018
+KernelVersion:	5.5
+Contact:	"Jolly Shah" <[e-mail]>
+Description:
+		Read/Write PMU persistent global general storage register
+		value, PERS_GLOB_GEN_STORAGE{0:3}.
+		Persistent global general storage register that
+		can be used by system to pass information between
+		masters.
+
+		This register is only reset by the power-on reset
+		and maintains its value through a system reset.
+		Four registers are used by the FSBL and other Xilinx
+		software products: PERS_GLOB_GEN_STORAGE{4:7}.
+		Register is reset only by a POR reset.
+
+		Usage:
+		# cat /sys/firmware/zynqmp/pggs0
+		# echo <mask> <value> > /sys/firmware/zynqmp/pggs0
+
+		Example:
+		# cat /sys/firmware/zynqmp/pggs0
+		# echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/pggs0
+
+Users:		Xilinx
diff --git a/drivers/firmware/xilinx/Makefile b/drivers/firmware/xilinx/Makefile
index 875a537..1e8643c 100644
--- a/drivers/firmware/xilinx/Makefile
+++ b/drivers/firmware/xilinx/Makefile
@@ -1,5 +1,5 @@ 
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Xilinx firmwares
 
-obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o
+obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o zynqmp-ggs.o
 obj-$(CONFIG_ZYNQMP_FIRMWARE_DEBUG) += zynqmp-debug.o
diff --git a/drivers/firmware/xilinx/zynqmp-ggs.c b/drivers/firmware/xilinx/zynqmp-ggs.c
new file mode 100644
index 0000000..e2a6700
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp-ggs.c
@@ -0,0 +1,284 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Xilinx Zynq MPSoC Firmware layer
+ *
+ *  Copyright (C) 2014-2018 Xilinx, Inc.
+ *
+ *  Jolly Shah <[e-mail]>
+ *  Rajan Vaja <[e-mail]>
+ */
+
+#include <linux/compiler.h>
+#include <linux/of.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+
+static ssize_t read_register(char *buf, u32 ioctl_id, u32 reg)
+{
+	int ret;
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+	if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
+		return -EFAULT;
+
+	ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", ret_payload[1]);
+}
+
+static ssize_t write_register(const char *buf, size_t count, u32 read_ioctl,
+			      u32 write_ioctl, u32 reg)
+{
+	char *kern_buff, *inbuf, *tok;
+	long mask, value;
+	int ret;
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+	const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+	if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
+		return -EFAULT;
+
+	kern_buff = kzalloc(count, GFP_KERNEL);
+	if (!kern_buff)
+		return -ENOMEM;
+
+	ret = strlcpy(kern_buff, buf, count);
+	if (ret < 0) {
+		ret = -EFAULT;
+		goto err;
+	}
+
+	inbuf = kern_buff;
+
+	/* Read the write mask */
+	tok = strsep(&inbuf, " ");
+	if (!tok) {
+		ret = -EFAULT;
+		goto err;
+	}
+
+	ret = kstrtol(tok, 16, &mask);
+	if (ret) {
+		ret = -EFAULT;
+		goto err;
+	}
+
+	/* Read the write value */
+	tok = strsep(&inbuf, " ");
+	if (!tok) {
+		ret = -EFAULT;
+		goto err;
+	}
+
+	ret = kstrtol(tok, 16, &value);
+	if (ret) {
+		ret = -EFAULT;
+		goto err;
+	}
+
+	ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
+	if (ret) {
+		ret = -EFAULT;
+		goto err;
+	}
+	ret_payload[1] &= ~mask;
+	value &= mask;
+	value |= ret_payload[1];
+
+	ret = eemi_ops->ioctl(0, write_ioctl, reg, value, NULL);
+	if (ret)
+		ret = -EFAULT;
+
+err:
+	kfree(kern_buff);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+/**
+ * ggs_show - Show global general storage (ggs) sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: Requested available shutdown_scope attributes string
+ * @reg: Register number
+ *
+ * Return:Number of bytes printed into the buffer.
+ *
+ * Helper function for viewing a ggs register value.
+ *
+ * User-space interface for viewing the content of the ggs0 register.
+ * cat /sys/firmware/zynqmp/ggs0
+ */
+static ssize_t ggs_show(struct device *device,
+			struct device_attribute *attr,
+			char *buf,
+			u32 reg)
+{
+	return read_register(buf, IOCTL_READ_GGS, reg);
+}
+
+/**
+ * ggs_store - Store global general storage (ggs) sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: User entered shutdown_scope attribute string
+ * @count: Size of buf
+ * @reg: Register number
+ *
+ * Return: count argument if request succeeds, the corresponding
+ * error code otherwise
+ *
+ * Helper function for storing a ggs register value.
+ *
+ * For example, the user-space interface for storing a value to the
+ * ggs0 register:
+ * echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
+ */
+static ssize_t ggs_store(struct device *device,
+			 struct device_attribute *attr,
+			 const char *buf, size_t count,
+			 u32 reg)
+{
+	if (!device || !attr || !buf || !count || reg >= GSS_NUM_REGS)
+		return -EINVAL;
+
+	return write_register(buf, count, IOCTL_READ_GGS, IOCTL_WRITE_GGS, reg);
+}
+
+/* GGS register show functions */
+#define GGS0_SHOW(N)						\
+	ssize_t ggs##N##_show(struct device *device,		\
+			      struct device_attribute *attr,	\
+			      char *buf)			\
+	{							\
+		return ggs_show(device, attr, buf, N);		\
+	}
+
+static GGS0_SHOW(0);
+static GGS0_SHOW(1);
+static GGS0_SHOW(2);
+static GGS0_SHOW(3);
+
+/* GGS register store function */
+#define GGS0_STORE(N)						\
+	ssize_t ggs##N##_store(struct device *device,		\
+			       struct device_attribute *attr,	\
+			       const char *buf,			\
+			       size_t count)			\
+	{							\
+		return ggs_store(device, attr, buf, count, N);	\
+	}
+
+static GGS0_STORE(0);
+static GGS0_STORE(1);
+static GGS0_STORE(2);
+static GGS0_STORE(3);
+
+/**
+ * pggs_show - Show persistent global general storage (pggs) sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: Requested available shutdown_scope attributes string
+ * @reg: Register number
+ *
+ * Return:Number of bytes printed into the buffer.
+ *
+ * Helper function for viewing a pggs register value.
+ */
+static ssize_t pggs_show(struct device *device,
+			 struct device_attribute *attr,
+			 char *buf,
+			 u32 reg)
+{
+	return read_register(buf, IOCTL_READ_PGGS, reg);
+}
+
+/**
+ * pggs_store - Store persistent global general storage (pggs) sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: User entered shutdown_scope attribute string
+ * @count: Size of buf
+ * @reg: Register number
+ *
+ * Return: count argument if request succeeds, the corresponding
+ * error code otherwise
+ *
+ * Helper function for storing a pggs register value.
+ */
+static ssize_t pggs_store(struct device *device,
+			  struct device_attribute *attr,
+			  const char *buf, size_t count,
+			  u32 reg)
+{
+	return write_register(buf, count, IOCTL_READ_PGGS,
+			      IOCTL_WRITE_PGGS, reg);
+}
+
+#define PGGS0_SHOW(N)						\
+	ssize_t pggs##N##_show(struct device *device,		\
+			       struct device_attribute *attr,	\
+			       char *buf)			\
+	{							\
+		return pggs_show(device, attr, buf, N);		\
+	}
+
+#define PGGS0_STORE(N)						\
+	ssize_t pggs##N##_store(struct device *device,		\
+				struct device_attribute *attr,	\
+				const char *buf,		\
+				size_t count)			\
+	{							\
+		return pggs_store(device, attr, buf, count, N);	\
+	}
+
+/* PGGS register show functions */
+static PGGS0_SHOW(0);
+static PGGS0_SHOW(1);
+static PGGS0_SHOW(2);
+static PGGS0_SHOW(3);
+
+/* PGGS register store functions */
+static PGGS0_STORE(0);
+static PGGS0_STORE(1);
+static PGGS0_STORE(2);
+static PGGS0_STORE(3);
+
+/* GGS register attributes */
+static DEVICE_ATTR_RW(ggs0);
+static DEVICE_ATTR_RW(ggs1);
+static DEVICE_ATTR_RW(ggs2);
+static DEVICE_ATTR_RW(ggs3);
+
+/* PGGS register attributes */
+static DEVICE_ATTR_RW(pggs0);
+static DEVICE_ATTR_RW(pggs1);
+static DEVICE_ATTR_RW(pggs2);
+static DEVICE_ATTR_RW(pggs3);
+
+static struct attribute *zynqmp_ggs_attrs[] = {
+	&dev_attr_ggs0.attr,
+	&dev_attr_ggs1.attr,
+	&dev_attr_ggs2.attr,
+	&dev_attr_ggs3.attr,
+	&dev_attr_pggs0.attr,
+	&dev_attr_pggs1.attr,
+	&dev_attr_pggs2.attr,
+	&dev_attr_pggs3.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(zynqmp_ggs);
+
+int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
+{
+	return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
+}
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 75bdfaa..4c1117d 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -473,6 +473,10 @@  static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
 	case IOCTL_GET_PLL_FRAC_MODE:
 	case IOCTL_SET_PLL_FRAC_DATA:
 	case IOCTL_GET_PLL_FRAC_DATA:
+	case IOCTL_WRITE_GGS:
+	case IOCTL_READ_GGS:
+	case IOCTL_WRITE_PGGS:
+	case IOCTL_READ_PGGS:
 		return 1;
 	default:
 		return 0;
@@ -704,6 +708,28 @@  const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
 }
 EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
 
+static int zynqmp_pm_sysfs_init(void)
+{
+	struct kobject *zynqmp_kobj;
+	int ret;
+
+	zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
+	if (!zynqmp_kobj) {
+		pr_err("zynqmp: Firmware kobj add failed.\n");
+		return -ENOMEM;
+	}
+
+	ret = zynqmp_pm_ggs_init(zynqmp_kobj);
+	if (ret) {
+		kobject_put(zynqmp_kobj);
+		pr_err("%s() GGS init fail with error %d\n",
+		       __func__, ret);
+		goto err;
+	}
+err:
+	return ret;
+}
+
 static int zynqmp_firmware_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -751,6 +777,12 @@  static int zynqmp_firmware_probe(struct platform_device *pdev)
 	/* Assign eemi_ops_table */
 	eemi_ops_tbl = &eemi_ops;
 
+	ret = zynqmp_pm_sysfs_init();
+	if (ret) {
+		pr_err("%s() sysfs init fail with error %d\n", __func__, ret);
+		return ret;
+	}
+
 	zynqmp_pm_api_debugfs_init();
 
 	ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, firmware_devs,
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index e41ad9e..534814e 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -13,6 +13,8 @@ 
 #ifndef __FIRMWARE_ZYNQMP_H__
 #define __FIRMWARE_ZYNQMP_H__
 
+#include <linux/device.h>
+
 #define ZYNQMP_PM_VERSION_MAJOR	1
 #define ZYNQMP_PM_VERSION_MINOR	0
 
@@ -42,6 +44,8 @@ 
 
 #define ZYNQMP_PM_MAX_QOS		100U
 
+#define GSS_NUM_REGS	(4)
+
 /* Node capabilities */
 #define	ZYNQMP_PM_CAPABILITY_ACCESS	0x1U
 #define	ZYNQMP_PM_CAPABILITY_CONTEXT	0x2U
@@ -97,6 +101,10 @@  enum pm_ioctl_id {
 	IOCTL_GET_PLL_FRAC_MODE,
 	IOCTL_SET_PLL_FRAC_DATA,
 	IOCTL_GET_PLL_FRAC_DATA,
+	IOCTL_WRITE_GGS,
+	IOCTL_READ_GGS,
+	IOCTL_WRITE_PGGS,
+	IOCTL_READ_PGGS,
 };
 
 enum pm_query_id {
@@ -311,6 +319,8 @@  struct zynqmp_eemi_ops {
 int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
 			u32 arg2, u32 arg3, u32 *ret_payload);
 
+int zynqmp_pm_ggs_init(struct kobject *parent_kobj);
+
 #if IS_REACHABLE(CONFIG_ARCH_ZYNQMP)
 const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void);
 #else