Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Jolly Shah <JOLLYS@xilinx.com>
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     "ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "matt@codeblueprint.co.uk" <matt@codeblueprint.co.uk>,
        "sudeep.holla@arm.com" <sudeep.holla@arm.com>,
        "hkallweit1@gmail.com" <hkallweit1@gmail.com>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "dmitry.torokhov@gmail.com" <dmitry.torokhov@gmail.com>,
        Michal Simek <michals@xilinx.com>,
        Rajan Vaja <RAJANV@xilinx.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Rajan Vaja <RAJANV@xilinx.com>
Subject: RE: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
Thread-Topic: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
Thread-Index: AQHVxn8DFx2mcD2XT0SouhH9uTzcjqfqR5+AgA6zcwA=
Date:   Thu, 23 Jan 2020 23:47:57 +0000
Message-ID: <BYAPR02MB5992FC37E0D2AD9946414417B80F0@BYAPR02MB5992.namprd02.prod.outlook.com>
References: <1578527663-10243-1-git-send-email-jolly.shah@xilinx.com>
 <1578527663-10243-2-git-send-email-jolly.shah@xilinx.com>
 <20200114145257.GA1910108@kroah.com>
In-Reply-To: <20200114145257.GA1910108@kroah.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 9903c75f-42e9-41a4-1130-08d7a05eac29
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Jan 2020 23:47:57.0467
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Y/0JjJmEYCEnjiKofgeIjHJjAFsSMNRi8AI0bKPDpdq4SelcsSCSIhnUM1eRqfJWTh0hohVprXt3eqnvxbfnlA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR02MB4582
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Greg,

Thanks for the review.

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Tuesday, January 14, 2020 6:53 AM
> To: Jolly Shah <JOLLYS@xilinx.com>
> Cc: ard.biesheuvel@linaro.org; mingo@kernel.org; matt@codeblueprint.co.uk=
;
> sudeep.holla@arm.com; hkallweit1@gmail.com; keescook@chromium.org;
> dmitry.torokhov@gmail.com; Michal Simek <michals@xilinx.com>; Rajan Vaja
> <RAJANV@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Rajan Vaja <RAJANV@xilinx.com>; Jolly Shah
> <JOLLYS@xilinx.com>
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>=20
> On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> > From: Rajan Vaja <rajan.vaja@xilinx.com>
> >
> > Add Firmware-ggs sysfs interface which provides read/write
> > interface to global storage registers.
> >
> > Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > Signed-off-by: Jolly Shah <jollys@xilinx.com>
> > ---
> > 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*
>=20
> 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 interface=
s were defined=20
earlier and are in use.

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

Yes. Will fix it in next version.

>=20
> > +Contact:	"Jolly Shah" <jollys@xilinx.com>
> > +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" <jollys@xilinx.com>
> > +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) +=3D zynqmp.o
> > +obj-$(CONFIG_ZYNQMP_FIRMWARE) +=3D zynqmp.o zynqmp-ggs.o
> >  obj-$(CONFIG_ZYNQMP_FIRMWARE_DEBUG) +=3D 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 <jollys@xilinx.com>
> > + *  Rajan Vaja <rajanv@xilinx.com>
> > + */
> > +
> > +#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 =3D
> zynqmp_pm_get_eemi_ops();
> > +
> > +	if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> > +		return -EFAULT;
> > +
> > +	ret =3D 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 =3D
> zynqmp_pm_get_eemi_ops();
> > +
> > +	if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> > +		return -EFAULT;
> > +
> > +	kern_buff =3D kzalloc(count, GFP_KERNEL);
> > +	if (!kern_buff)
> > +		return -ENOMEM;
> > +
> > +	ret =3D strlcpy(kern_buff, buf, count);
> > +	if (ret < 0) {
> > +		ret =3D -EFAULT;
> > +		goto err;
> > +	}
> > +
> > +	inbuf =3D kern_buff;
> > +
> > +	/* Read the write mask */
> > +	tok =3D strsep(&inbuf, " ");
> > +	if (!tok) {
> > +		ret =3D -EFAULT;
>=20
> 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

>=20
> > +		goto err;
> > +	}
> > +
> > +	ret =3D kstrtol(tok, 16, &mask);
> > +	if (ret) {
> > +		ret =3D -EFAULT;
> > +		goto err;
> > +	}
> > +
> > +	/* Read the write value */
> > +	tok =3D strsep(&inbuf, " ");
> > +	if (!tok) {
> > +		ret =3D -EFAULT;
> > +		goto err;
> > +	}
> > +
> > +	ret =3D kstrtol(tok, 16, &value);
> > +	if (ret) {
> > +		ret =3D -EFAULT;
> > +		goto err;
> > +	}
> > +
> > +	ret =3D eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
>=20
> 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?
>=20

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

>=20
> > +	if (ret) {
> > +		ret =3D -EFAULT;
> > +		goto err;
> > +	}
> > +	ret_payload[1] &=3D ~mask;
> > +	value &=3D mask;
> > +	value |=3D ret_payload[1];
> > +
> > +	ret =3D eemi_ops->ioctl(0, write_ioctl, reg, value, NULL);
> > +	if (ret)
> > +		ret =3D -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 >=3D 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 att=
ribute
> > + * @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 a=
ttribute
> > + * @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[] =3D {
> > +	&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]);
>=20
> 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?

>=20
> > +}
> > 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:
>=20
> Huh???

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

>=20
> >  		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 =3D kobject_create_and_add("zynqmp", firmware_kobj);
> > +	if (!zynqmp_kobj) {
> > +		pr_err("zynqmp: Firmware kobj add failed.\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ret =3D 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 =3D &pdev->dev;
> > @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct
> platform_device *pdev)
> >  	/* Assign eemi_ops_table */
> >  	eemi_ops_tbl =3D &eemi_ops;
> >
> > +	ret =3D zynqmp_pm_sysfs_init();
>=20
> 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!
>=20
> 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 platfo=
rm driver,
to keep paths same, we used sysfs.

Thanks,
Jolly Shah


>=20
> thanks,
>=20
> greg k-h