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

[Jolly Shah]

Hi Greg,

ïOn 1/23/20, 10:03 PM, "Greg KH" <gregkh@xxxxxxxxxxxxxxxxxxx> 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 <gregkh@xxxxxxxxxxxxxxxxxxx>
    > > Sent: Tuesday, January 14, 2020 6:53 AM
    > > To: Jolly Shah <JOLLYS@xxxxxxxxxx>
    > > Cc: ard.biesheuvel@xxxxxxxxxx; mingo@xxxxxxxxxx; matt@xxxxxxxxxxxxxxxxxxx;
    > > sudeep.holla@xxxxxxx; hkallweit1@xxxxxxxxx; keescook@xxxxxxxxxxxx;
    > > dmitry.torokhov@xxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>; Rajan Vaja
    > > <RAJANV@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
    > > kernel@xxxxxxxxxxxxxxx; Rajan Vaja <RAJANV@xxxxxxxxxx>; Jolly Shah
    > > <JOLLYS@xxxxxxxxxx>
    > > 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 <rajan.vaja@xxxxxxxxxx>
    > > >
    > > > Add Firmware-ggs sysfs interface which provides read/write
    > > > interface to global storage registers.
    > > >
    > > > Signed-off-by: Rajan Vaja <rajan.vaja@xxxxxxxxxx>
    > > > Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
    > > > Signed-off-by: Jolly Shah <jollys@xxxxxxxxxx>
    > > > ---
    > > > 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