| 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
| ||
|
Date: Wed, 19 Jan 2011 16:37:58 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: Mike Frysinger <vapier@...too.org>
Cc: linux-kernel@...r.kernel.org,
device-drivers-devel@...ckfin.uclinux.org
Subject: Re: [PATCH] sigma-firmware: loader for Analog Devices' SigmaStudio
On Wed, 12 Jan 2011 00:41:31 -0500
Mike Frysinger <vapier@...too.org> wrote:
> Analog Devices' SigmaStudio can produce firmware blobs for devices with
> these DSPs embedded (like some audio codecs). Allow these device drivers
> to easily parse and load them.
>
> Signed-off-by: Mike Frysinger <vapier@...too.org>
> ---
> drivers/firmware/Kconfig | 12 +++++
> drivers/firmware/Makefile | 1 +
> drivers/firmware/sigma.c | 110 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/sigma.h | 60 ++++++++++++++++++++++++
The newly-added code has no callsites. What are we to make of this?
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index e8b6a13..8f4f7b2 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -134,4 +134,16 @@ config ISCSI_IBFT
> detect iSCSI boot parameters dynamically during system boot, say Y.
> Otherwise, say N.
>
> +config SIGMA
> + tristate "SigmaStudio firmware loader"
> + depends on I2C
The dependence on just I2C looks odd. What's the rationale here?
> + select CRC32
> + default n
> + help
> + Enable helper functions for working with Analog Devices SigmaDSP
> + parts and binary firmwares produced by Analog Devices SigmaStudio.
> +
> + If unsure, say N here. Drivers that need these helpers will select
> + this option automatically.
> +
> endmenu
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 1c3c173..ede9722 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_DMIID) += dmi-id.o
> obj-$(CONFIG_ISCSI_IBFT_FIND) += iscsi_ibft_find.o
> obj-$(CONFIG_ISCSI_IBFT) += iscsi_ibft.o
> obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o
> +obj-$(CONFIG_SIGMA) += sigma.o
> diff --git a/drivers/firmware/sigma.c b/drivers/firmware/sigma.c
> new file mode 100644
> index 0000000..e2945be
> --- /dev/null
> +++ b/drivers/firmware/sigma.c
> @@ -0,0 +1,110 @@
> +/*
> + * Load Analog Devices SigmaStudio firmware files
> + *
> +__* Copyright 2009 Analog Devices Inc.
> +__*
> +__* Licensed under the GPL-2 or later.
> + */
Code had weird non-ascii chars.
> +
> +#include <linux/crc32.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/kernel.h>
> +#include <linux/i2c.h>
> +#include <linux/sigma.h>
> +
> +/* Return: 0==OK, <0==error, =1 ==no more actions */
> +int process_sigma_action(struct i2c_client *client, struct sigma_firmware *ssfw)
> +{
> + struct sigma_action *sa = (void *)(ssfw->fw->data + ssfw->pos);
> + size_t len = sigma_action_len(sa);
> + int ret = 0;
> +
> + pr_debug("%s: instr:%i addr:%#x len:%zu\n", __func__,
> + sa->instr, sa->addr, len);
> +
> + switch (sa->instr) {
> + case SIGMA_ACTION_WRITEXBYTES:
> + case SIGMA_ACTION_WRITESINGLE:
> + case SIGMA_ACTION_WRITESAFELOAD:
> + if (ssfw->fw->size < ssfw->pos + len)
> + return -EINVAL;
> + ret = i2c_master_send(client, (void *)&sa->addr, len);
> + if (ret < 0)
> + return -EINVAL;
> + break;
> +
> + case SIGMA_ACTION_DELAY:
> + ret = 0;
> + udelay(len);
> + len = 0;
> + break;
> +
> + case SIGMA_ACTION_END:
> + return 1;
> +
> + default:
> + return -EINVAL;
> + }
Indent in the switch was wrong. Please use checkpatch.
> + /* when arrive here ret=0 or sent data */
> + ssfw->pos += sigma_action_size(sa, len);
> + return ssfw->pos == ssfw->fw->size;
> +}
> +
> +int process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw)
Global scope appears to be unnecessary.
> +{
> + pr_debug("%s: processing %p\n", __func__, ssfw);
> +
> + while (1) {
> + int ret = process_sigma_action(client, ssfw);
> + pr_debug("%s: action returned %i\n", __func__, ret);
> + if (ret == 1)
> + return 0;
> + else if (ret)
> + return ret;
> + }
> +}
> +
> +int process_sigma_firmware(struct i2c_client *client, const char *name)
Global scope appears to be unnecessary.
> +{
> + int ret;
> + struct sigma_firmware_header *ssfw_head;
> + struct sigma_firmware ssfw;
> + const struct firmware *fw;
> +
> + pr_debug("%s: loading firmware %s\n", __func__, name);
> +
> + /* first load the blob */
> + ret = request_firmware(&fw, name, &client->dev);
> + if (ret) {
> + pr_debug("%s: request_firmware() failed with %i\n", __func__, ret);
> + return ret;
> + }
> + ssfw.fw = fw;
> +
> + /* then verify the header */
> + if (fw->size < sizeof(*ssfw_head)) {
> + ret = -ENODATA;
> + goto done;
> + }
> + ssfw_head = (void *)fw->data;
> + if (memcmp(ssfw_head->magic, SIGMA_MAGIC, ARRAY_SIZE(ssfw_head->magic))) {
> + ret = -EINVAL;
> + goto done;
> + }
> + pr_debug("%s: crc=%x\n", __func__, crc32(0, fw->data, fw->size));
The code calculates and prints the crc but doesn't actually check it
against anything. What's up with that?
> + ssfw.pos = sizeof(*ssfw_head);
> +
> + /* finally process all of the actions */
> + ret = process_sigma_actions(client, &ssfw);
> +
> + done:
> + release_firmware(fw);
> +
> + pr_debug("%s: loaded %s\n", __func__, name);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(process_sigma_firmware);
> diff --git a/include/linux/sigma.h b/include/linux/sigma.h
> new file mode 100644
> index 0000000..510765f
> --- /dev/null
> +++ b/include/linux/sigma.h
> @@ -0,0 +1,60 @@
> +/*
> + * Load firmware files from Analog Devices SigmaStudio
> + *
> +__* Copyright 2009 Analog Devices Inc.
> +__*
> +__* Licensed under the GPL-2 or later.
> + */
More weird non-ascii chars.
> +#ifndef __SIGMA_FIRMWARE_H__
> +#define __SIGMA_FIRMWARE_H__
> +
> +#include <linux/firmware.h>
> +#include <linux/types.h>
> +
> +#define SIGMA_MAGIC "ADISIGM"
> +
> +struct sigma_firmware {
> + const struct firmware *fw;
> + size_t pos;
> +};
> +
> +struct sigma_firmware_header {
> + unsigned char magic[7];
> + u8 version;
> + u32 crc;
> +};
> +
> +enum {
> + SIGMA_ACTION_WRITEXBYTES = 0,
> + SIGMA_ACTION_WRITESINGLE,
> + SIGMA_ACTION_WRITESAFELOAD,
> + SIGMA_ACTION_DELAY,
> + SIGMA_ACTION_PLLWAIT,
> + SIGMA_ACTION_NOOP,
> + SIGMA_ACTION_END,
> +};
> +
> +struct sigma_action {
> + u8 instr;
> + u8 len_hi;
> + u16 len;
> + u16 addr;
> + unsigned char payload[];
> +};
> +
> +static inline u32 sigma_action_len(struct sigma_action *sa)
> +{
> + return (sa->len_hi << 16) | sa->len;
> +}
> +
> +static inline size_t sigma_action_size(struct sigma_action *sa, u32 payload_len)
> +{
> + return sizeof(*sa) + payload_len + (payload_len % 2);
> +}
Did these two functions need to be in a header? If they're only ever
used in sigma.c then they should be located in that file.
> +struct i2c_client;
It's best to put forward declarations of this form right at the top of
the header file. Otherwise, we end up with duplicated
forward-declarations as people add references to `struct i2c_client' at
earlier sites in this file. This has happened before.
> +extern int process_sigma_firmware(struct i2c_client *client, const char *name);
> +
> +#endif
A patch to address some of the above:
--- a/drivers/firmware/sigma.c~sigma-firmware-loader-for-analog-devices-sigmastudio-fix
+++ a/drivers/firmware/sigma.c
@@ -1,9 +1,9 @@
/*
* Load Analog Devices SigmaStudio firmware files
*
- * Copyright 2009 Analog Devices Inc.
- *
- * Licensed under the GPL-2 or later.
+_* Copyright 2009 Analog Devices Inc.
+_*
+_* Licensed under the GPL-2 or later.
*/
#include <linux/crc32.h>
@@ -14,7 +14,8 @@
#include <linux/sigma.h>
/* Return: 0==OK, <0==error, =1 ==no more actions */
-int process_sigma_action(struct i2c_client *client, struct sigma_firmware *ssfw)
+static int process_sigma_action(struct i2c_client *client,
+ struct sigma_firmware *ssfw)
{
struct sigma_action *sa = (void *)(ssfw->fw->data + ssfw->pos);
size_t len = sigma_action_len(sa);
@@ -24,27 +25,27 @@ int process_sigma_action(struct i2c_clie
sa->instr, sa->addr, len);
switch (sa->instr) {
- case SIGMA_ACTION_WRITEXBYTES:
- case SIGMA_ACTION_WRITESINGLE:
- case SIGMA_ACTION_WRITESAFELOAD:
- if (ssfw->fw->size < ssfw->pos + len)
- return -EINVAL;
- ret = i2c_master_send(client, (void *)&sa->addr, len);
- if (ret < 0)
- return -EINVAL;
- break;
-
- case SIGMA_ACTION_DELAY:
- ret = 0;
- udelay(len);
- len = 0;
- break;
+ case SIGMA_ACTION_WRITEXBYTES:
+ case SIGMA_ACTION_WRITESINGLE:
+ case SIGMA_ACTION_WRITESAFELOAD:
+ if (ssfw->fw->size < ssfw->pos + len)
+ return -EINVAL;
+ ret = i2c_master_send(client, (void *)&sa->addr, len);
+ if (ret < 0)
+ return -EINVAL;
+ break;
- case SIGMA_ACTION_END:
- return 1;
+ case SIGMA_ACTION_DELAY:
+ ret = 0;
+ udelay(len);
+ len = 0;
+ break;
- default:
- return -EINVAL;
+ case SIGMA_ACTION_END:
+ return 1;
+
+ default:
+ return -EINVAL;
}
/* when arrive here ret=0 or sent data */
@@ -52,7 +53,8 @@ int process_sigma_action(struct i2c_clie
return ssfw->pos == ssfw->fw->size;
}
-int process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw)
+static int process_sigma_actions(struct i2c_client *client,
+ struct sigma_firmware *ssfw)
{
pr_debug("%s: processing %p\n", __func__, ssfw);
@@ -78,7 +80,8 @@ int process_sigma_firmware(struct i2c_cl
/* first load the blob */
ret = request_firmware(&fw, name, &client->dev);
if (ret) {
- pr_debug("%s: request_firmware() failed with %i\n", __func__, ret);
+ pr_debug("%s: request_firmware() failed with %i\n",
+ __func__, ret);
return ret;
}
ssfw.fw = fw;
diff -puN include/linux/sigma.h~sigma-firmware-loader-for-analog-devices-sigmastudio-fix include/linux/sigma.h
--- a/include/linux/sigma.h~sigma-firmware-loader-for-analog-devices-sigmastudio-fix
+++ a/include/linux/sigma.h
@@ -1,9 +1,9 @@
/*
* Load firmware files from Analog Devices SigmaStudio
*
- * Copyright 2009 Analog Devices Inc.
- *
- * Licensed under the GPL-2 or later.
+_* Copyright 2009 Analog Devices Inc.
+_*
+_* Licensed under the GPL-2 or later.
*/
#ifndef __SIGMA_FIRMWARE_H__
@@ -12,6 +12,8 @@
#include <linux/firmware.h>
#include <linux/types.h>
+struct i2c_client;
+
#define SIGMA_MAGIC "ADISIGM"
struct sigma_firmware {
@@ -53,8 +55,6 @@ static inline size_t sigma_action_size(s
return sizeof(*sa) + payload_len + (payload_len % 2);
}
-struct i2c_client;
-
extern int process_sigma_firmware(struct i2c_client *client, const char *name);
#endif
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists