Re: [PATCH v3 21/24] firmware: xilinx: Add sysfs interface

[Greg KH]

On Fri, Mar 06, 2020 at 03:47:29PM -0800, Jolly Shah wrote:
> +/**
> + * 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/devices/platform/firmware\:zynqmp-firmware/ggs0
> + */

Do you really need a whole kernel_doc format for a static function?
Anyway...

> +static ssize_t ggs_store(struct device *device,
> +			 struct device_attribute *attr,
> +			 const char *buf, size_t count,
> +			 u32 reg)
> +{
> +	char *kern_buff, *inbuf, *tok;
> +	long mask, value;
> +	int ret;
> +	u32 ret_payload[PAYLOAD_ARG_CNT];
> +
> +	if (!device || !attr || !buf || !count || reg >= GSS_NUM_REGS)

How can !device, !attr, !buf, or !count ever happen?

Do not check for things that are impossible please.

> +		return -EINVAL;
> +
> +	kern_buff = kzalloc(count, GFP_KERNEL);
> +	if (!kern_buff)
> +		return -ENOMEM;
> +
> +	ret = strlcpy(kern_buff, buf, count);
> +	if (ret < 0) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	inbuf = kern_buff;
> +
> +	/* Read the write mask */
> +	tok = strsep(&inbuf, " ");
> +	if (!tok) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	ret = kstrtol(tok, 16, &mask);
> +	if (ret) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	/* Read the write value */
> +	tok = strsep(&inbuf, " ");
> +	if (!tok) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	ret = kstrtol(tok, 16, &value);
> +	if (ret) {
> +		count = -EFAULT;
> +		goto err;
> +	}

sysfs files are "one value per file" which prevents this string parsing
mess.  Please do not do that.

Again, one value per file, not X values.


> +
> +	ret = zynqmp_pm_read_ggs(reg, ret_payload);
> +	if (ret) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +	ret_payload[1] &= ~mask;
> +	value &= mask;
> +	value |= ret_payload[1];
> +
> +	ret = zynqmp_pm_write_ggs(reg, value);
> +	if (ret)
> +		count = -EFAULT;
> +
> +err:
> +	kfree(kern_buff);
> +
> +	return count;
> +}

> +/**
> + * 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)
> +{
> +	char *kern_buff, *inbuf, *tok;
> +	long mask, value;
> +	int ret;
> +	u32 ret_payload[PAYLOAD_ARG_CNT];
> +
> +	if (!device || !attr || !buf || !count || reg >= GSS_NUM_REGS)
> +		return -EINVAL;

Again, clean this up.

> +
> +	kern_buff = kzalloc(count, GFP_KERNEL);
> +	if (!kern_buff)
> +		return -ENOMEM;
> +
> +	ret = strlcpy(kern_buff, buf, count);
> +	if (ret < 0) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	inbuf = kern_buff;
> +
> +	/* Read the write mask */
> +	tok = strsep(&inbuf, " ");

No need to parse when there is only one value.

> +	if (!tok) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	ret = kstrtol(tok, 16, &mask);
> +	if (ret) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	/* Read the write value */
> +	tok = strsep(&inbuf, " ");
> +	if (!tok) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	ret = kstrtol(tok, 16, &value);
> +	if (ret) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +
> +	ret = zynqmp_pm_read_pggs(reg, ret_payload);
> +	if (ret) {
> +		count = -EFAULT;
> +		goto err;
> +	}
> +	ret_payload[1] &= ~mask;
> +	value &= mask;
> +	value |= ret_payload[1];
> +
> +	ret = zynqmp_pm_write_pggs(reg, value);
> +	if (ret)
> +		count = -EFAULT;
> +
> +err:
> +	kfree(kern_buff);
> +
> +	return count;
> +}

> +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,
> +};
> +
> +static const struct attribute_group ggs_attribute_group = {
> +	.attrs = zynqmp_ggs_attrs,
> +};
> +
> +const struct attribute_group *firmware_attribute_groups[] = {
> +	&ggs_attribute_group,
> +	NULL,
> +};

ATTRIBUTE_GROUPS()?



> +
>  static int zynqmp_firmware_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -910,6 +1226,7 @@ static struct platform_driver zynqmp_firmware_driver = {
>  	.driver = {
>  		.name = "zynqmp_firmware",
>  		.of_match_table = zynqmp_firmware_of_match,
> +		.dev_groups = firmware_attribute_groups,
>  	},
>  	.probe = zynqmp_firmware_probe,
>  	.remove = zynqmp_firmware_remove,
> diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
> index 725dccf..8ccaa39 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>

Why is this needed here?

> +
>  #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


You are not adding anything that depends on device.h in this file, so
just include the needed .h file in the .c file that needs it please.

Helps unwind .h include messes.

thanks,

greg k-h