| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200318115105.GA2472686@kroah.com>
Date: Wed, 18 Mar 2020 12:51:05 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Jolly Shah <jolly.shah@...inx.com>
Cc: ard.biesheuvel@...aro.org, mingo@...nel.org,
matt@...eblueprint.co.uk, sudeep.holla@....com,
hkallweit1@...il.com, keescook@...omium.org,
dmitry.torokhov@...il.com, michal.simek@...inx.com,
rajanv@...inx.com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, Rajan Vaja <rajan.vaja@...inx.com>,
Jolly Shah <jollys@...inx.com>,
Tejas Patel <tejas.patel@...inx.com>
Subject: Re: [PATCH v3 21/24] firmware: xilinx: Add sysfs interface
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
Powered by blists - more mailing lists