[alsa-devel] [PATCH 1/8] firmware: Sigma: Prevent out of bounds memory access
The SigmaDSP firmware loader currently does not perform enough boundary size checks when processing the firmware. As a result it is possible that a malformed firmware can cause an out of bounds memory access.
This patch adds checks which ensure that both the action header and the payload are completely inside the firmware data boundaries before processing them.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Cc: stable@kernel.org --- drivers/firmware/sigma.c | 70 ++++++++++++++++++++++++++++++++-------------- include/linux/sigma.h | 5 --- 2 files changed, 49 insertions(+), 26 deletions(-)
diff --git a/drivers/firmware/sigma.c b/drivers/firmware/sigma.c index f10fc52..5b17966 100644 --- a/drivers/firmware/sigma.c +++ b/drivers/firmware/sigma.c @@ -14,13 +14,30 @@ #include <linux/module.h> #include <linux/sigma.h>
-/* Return: 0==OK, <0==error, =1 ==no more actions */ +static size_t sigma_action_size(struct sigma_action *sa) +{ + size_t payload = 0; + + switch (sa->instr) { + case SIGMA_ACTION_WRITEXBYTES: + case SIGMA_ACTION_WRITESINGLE: + case SIGMA_ACTION_WRITESAFELOAD: + payload = sigma_action_len(sa); + break; + default: + break; + } + + payload = ALIGN(payload, 2); + + return payload + sizeof(struct sigma_action); +} + static int -process_sigma_action(struct i2c_client *client, struct sigma_firmware *ssfw) +process_sigma_action(struct i2c_client *client, struct sigma_action *sa) { - struct sigma_action *sa = (void *)(ssfw->fw->data + ssfw->pos); size_t len = sigma_action_len(sa); - int ret = 0; + int ret;
pr_debug("%s: instr:%i addr:%#x len:%zu\n", __func__, sa->instr, sa->addr, len); @@ -29,44 +46,50 @@ process_sigma_action(struct i2c_client *client, struct sigma_firmware *ssfw) 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; - + return 0; default: return -EINVAL; }
- /* when arrive here ret=0 or sent data */ - ssfw->pos += sigma_action_size(sa, len); - return ssfw->pos == ssfw->fw->size; + return 1; }
static int process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw) { - pr_debug("%s: processing %p\n", __func__, ssfw); + struct sigma_action *sa; + size_t size; + int ret; + + while (ssfw->pos + sizeof(*sa) <= ssfw->fw->size) { + sa = (struct sigma_action *)(ssfw->fw->data + ssfw->pos); + + size = sigma_action_size(sa); + ssfw->pos += size; + if (ssfw->pos > ssfw->fw->size || size == 0) + break; + + ret = process_sigma_action(client, sa);
- 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) + + if (ret <= 0) return ret; } + + if (ssfw->pos != ssfw->fw->size) + return -EINVAL; + + return 0; }
int process_sigma_firmware(struct i2c_client *client, const char *name) @@ -89,7 +112,12 @@ int process_sigma_firmware(struct i2c_client *client, const char *name)
/* then verify the header */ ret = -EINVAL; - if (fw->size < sizeof(*ssfw_head)) + + /* Reject too small or unreasonable large files. The upper limit is + * chosen a bit arbitrarily but it should be enough for all practical + * purposes and having the limit makes it easier to avoid integer + * overflows later in the loading process. */ + if (fw->size < sizeof(*ssfw_head) || fw->size >= 0x4000000) goto done;
ssfw_head = (void *)fw->data; diff --git a/include/linux/sigma.h b/include/linux/sigma.h index e2accb3..9a138c2 100644 --- a/include/linux/sigma.h +++ b/include/linux/sigma.h @@ -50,11 +50,6 @@ 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); -} - extern int process_sigma_firmware(struct i2c_client *client, const char *name);
#endif
The firmware header is not part of the CRC, so skip it. Otherwise the firmware will be rejected due to non-matching CRCs.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Cc: stable@kernel.org --- drivers/firmware/sigma.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/firmware/sigma.c b/drivers/firmware/sigma.c index 5b17966..b1ab32a6 100644 --- a/drivers/firmware/sigma.c +++ b/drivers/firmware/sigma.c @@ -124,7 +124,8 @@ int process_sigma_firmware(struct i2c_client *client, const char *name) if (memcmp(ssfw_head->magic, SIGMA_MAGIC, ARRAY_SIZE(ssfw_head->magic))) goto done;
- crc = crc32(0, fw->data, fw->size); + crc = crc32(0, fw->data + sizeof(*ssfw_head), + fw->size - sizeof(*ssfw_head)); pr_debug("%s: crc=%x\n", __func__, crc); if (crc != ssfw_head->crc) goto done;
On Thursday 24 November 2011 07:48:21 Lars-Peter Clausen wrote:
The firmware header is not part of the CRC, so skip it. Otherwise the firmware will be rejected due to non-matching CRCs.
that's because you didn't compare to the right value ;). include the CRC -> compare to 0. omit the CRC -> compare to the CRC value.
including the CRC and doing a compare to 0 would probably make for slightly smaller code ... -mike
On 11/24/2011 06:21 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:21 Lars-Peter Clausen wrote:
The firmware header is not part of the CRC, so skip it. Otherwise the firmware will be rejected due to non-matching CRCs.
that's because you didn't compare to the right value ;). include the CRC -> compare to 0. omit the CRC -> compare to the CRC value.
Does this really work if the CRC is inserted somewhere in the middle of the bytestream?
On Friday 25 November 2011 03:55:42 Lars-Peter Clausen wrote:
On 11/24/2011 06:21 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:21 Lars-Peter Clausen wrote:
The firmware header is not part of the CRC, so skip it. Otherwise the firmware will be rejected due to non-matching CRCs.
that's because you didn't compare to the right value ;). include the CRC -> compare to 0. omit the CRC -> compare to the CRC value.
Does this really work if the CRC is inserted somewhere in the middle of the bytestream?
i don't think the position matters to the CRC algorithm used by sigmadsp. math principle: a ^ b ^ c is the same thing as b ^ a ^ c and c ^ b ^ a.
i could be wrong as to the CRC algo used though. simple enough for you to check -- i implemented this firmware code based on a spec i wrote up for the sigmadsp peeps; i never actually had real firmware to test with. -mike
On 11/25/2011 09:00 PM, Mike Frysinger wrote:
On Friday 25 November 2011 03:55:42 Lars-Peter Clausen wrote:
On 11/24/2011 06:21 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:21 Lars-Peter Clausen wrote:
The firmware header is not part of the CRC, so skip it. Otherwise the firmware will be rejected due to non-matching CRCs.
that's because you didn't compare to the right value ;). include the CRC -> compare to 0. omit the CRC -> compare to the CRC value.
Does this really work if the CRC is inserted somewhere in the middle of the bytestream?
i don't think the position matters to the CRC algorithm used by sigmadsp. math principle: a ^ b ^ c is the same thing as b ^ a ^ c and c ^ b ^ a.
If CRC algorithms were commutative they would be pretty weak, I guess ;)
i could be wrong as to the CRC algo used though. simple enough for you to check -- i implemented this firmware code based on a spec i wrote up for the sigmadsp peeps; i never actually had real firmware to test with. -mike
It does not work.
- Lars
On Monday 28 November 2011 02:56:33 Lars-Peter Clausen wrote:
On 11/25/2011 09:00 PM, Mike Frysinger wrote:
On Friday 25 November 2011 03:55:42 Lars-Peter Clausen wrote:
On 11/24/2011 06:21 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:21 Lars-Peter Clausen wrote:
The firmware header is not part of the CRC, so skip it. Otherwise the firmware will be rejected due to non-matching CRCs.
that's because you didn't compare to the right value ;). include the CRC -> compare to 0. omit the CRC -> compare to the CRC value.
Does this really work if the CRC is inserted somewhere in the middle of the bytestream?
i don't think the position matters to the CRC algorithm used by sigmadsp. math principle: a ^ b ^ c is the same thing as b ^ a ^ c and c ^ b ^ a.
If CRC algorithms were commutative they would be pretty weak, I guess ;)
that doesn't stop people from implementing them this way ;). the last CRC code i did actually implement and test related to the Blackfin arch did have this property (the bootrom). so i just included the computed CRC value and made sure the end result is 0.
i could be wrong as to the CRC algo used though. simple enough for you to check -- i implemented this firmware code based on a spec i wrote up for the sigmadsp peeps; i never actually had real firmware to test with.
It does not work.
ok, i withdraw my complaints here then :) -mike
Currently the SigmaDSP firmware loader only works correctly on little-endian systems. Fix this by using the proper endianess conversion functions.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Cc: stable@kernel.org --- drivers/firmware/sigma.c | 2 +- include/linux/sigma.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/firmware/sigma.c b/drivers/firmware/sigma.c index b1ab32a6..749932c 100644 --- a/drivers/firmware/sigma.c +++ b/drivers/firmware/sigma.c @@ -127,7 +127,7 @@ int process_sigma_firmware(struct i2c_client *client, const char *name) crc = crc32(0, fw->data + sizeof(*ssfw_head), fw->size - sizeof(*ssfw_head)); pr_debug("%s: crc=%x\n", __func__, crc); - if (crc != ssfw_head->crc) + if (crc != le32_to_cpu(ssfw_head->crc)) goto done;
ssfw.pos = sizeof(*ssfw_head); diff --git a/include/linux/sigma.h b/include/linux/sigma.h index 9a138c2..d0de882 100644 --- a/include/linux/sigma.h +++ b/include/linux/sigma.h @@ -24,7 +24,7 @@ struct sigma_firmware { struct sigma_firmware_header { unsigned char magic[7]; u8 version; - u32 crc; + __le32 crc; };
enum { @@ -40,14 +40,14 @@ enum { struct sigma_action { u8 instr; u8 len_hi; - u16 len; - u16 addr; + __le16 len; + __be16 addr; unsigned char payload[]; };
static inline u32 sigma_action_len(struct sigma_action *sa) { - return (sa->len_hi << 16) | sa->len; + return (sa->len_hi << 16) | le16_to_cpu(sa->len); }
extern int process_sigma_firmware(struct i2c_client *client, const char *name);
On Thursday 24 November 2011 07:48:22 Lars-Peter Clausen wrote:
Currently the SigmaDSP firmware loader only works correctly on little-endian systems. Fix this by using the proper endianess conversion functions.
Acked-by: Mike Frysinger vapier@gentoo.org -mike
Mark structs which are embedded into the firmware as packed to avoid alignment issues.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de Cc: stable@kernel.org --- include/linux/sigma.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/sigma.h b/include/linux/sigma.h index d0de882..745f1bd 100644 --- a/include/linux/sigma.h +++ b/include/linux/sigma.h @@ -25,7 +25,7 @@ struct sigma_firmware_header { unsigned char magic[7]; u8 version; __le32 crc; -}; +} __packed;
enum { SIGMA_ACTION_WRITEXBYTES = 0, @@ -43,7 +43,7 @@ struct sigma_action { __le16 len; __be16 addr; unsigned char payload[]; -}; +} __packed;
static inline u32 sigma_action_len(struct sigma_action *sa) {
On Thursday 24 November 2011 07:48:23 Lars-Peter Clausen wrote:
Mark structs which are embedded into the firmware as packed to avoid alignment issues.
while in general this makes sense, i designed the struct layout specifically to work on any sane system. that means 8bits align on 8bits, 16bits align on 16bits, and 32bits align on 32bits.
do you see any place where this is not the case ? otherwise, using __packed by itself doesn't make much sense unless you also change all the loads from the struct to the get_unaligned variety which would add useless overhead to many embedded parts.
all in all, i'd omit the __packed markings since they're unnecessary. -mike
On 11/24/2011 06:19 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:23 Lars-Peter Clausen wrote:
Mark structs which are embedded into the firmware as packed to avoid alignment issues.
while in general this makes sense, i designed the struct layout specifically to work on any sane system. that means 8bits align on 8bits, 16bits align on 16bits, and 32bits align on 32bits.
do you see any place where this is not the case ? otherwise, using __packed by itself doesn't make much sense unless you also change all the loads from the struct to the get_unaligned variety which would add useless overhead to many embedded parts.
all in all, i'd omit the __packed markings since they're unnecessary. -mike
While you are right I think it's generally a good idea to mark anything as packed, where we don't have direct influence on the alignment, to avoid unpleasant suppresses. The compiler will usually take of performing proper unaligned reads. I've though about adding a __aligned__(2) to the struct, but since the majority of the data is 8bit data the overhead should be negligible.
- Lars
On Friday 25 November 2011 05:48:20 Lars-Peter Clausen wrote:
On 11/24/2011 06:19 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:23 Lars-Peter Clausen wrote:
Mark structs which are embedded into the firmware as packed to avoid alignment issues.
while in general this makes sense, i designed the struct layout specifically to work on any sane system. that means 8bits align on 8bits, 16bits align on 16bits, and 32bits align on 32bits.
do you see any place where this is not the case ? otherwise, using __packed by itself doesn't make much sense unless you also change all the loads from the struct to the get_unaligned variety which would add useless overhead to many embedded parts.
all in all, i'd omit the __packed markings since they're unnecessary.
While you are right I think it's generally a good idea to mark anything as packed, where we don't have direct influence on the alignment, to avoid unpleasant suppresses.
don't we though ? the ADI peeps writing the driver code work directly with the ADI sigmadsp peeps for the firmware needs. so as long as we make sure the layout stays sane, then it stays sane ... otherwise, we need to be vigilant to make sure people don't go slipping in useless get_unaligned calls to the code. if they see "__packed", they think it's correct. -mike
It has been pointed out previously, that the firmware subsystem is not the right place for the SigmaDSP firmware loader. Furthermore the SigmaDSP is currently only used in audio products and we are aiming for better integration into the ASoC framework in the future, with support for ALSA controls for firmware parameters and support dynamic power management as well. So the natural choice for the SigmaDSP firmware loader is the ASoC subsystem.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de --- MAINTAINERS | 1 + drivers/firmware/Kconfig | 12 ---- drivers/firmware/Makefile | 1 - drivers/firmware/sigma.c | 147 ------------------------------------------ include/linux/sigma.h | 55 ---------------- sound/soc/codecs/Kconfig | 6 ++- sound/soc/codecs/Makefile | 2 + sound/soc/codecs/adau1701.c | 2 +- sound/soc/codecs/sigmadsp.c | 148 +++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/sigmadsp.h | 55 ++++++++++++++++ 10 files changed, 212 insertions(+), 217 deletions(-) delete mode 100644 drivers/firmware/sigma.c delete mode 100644 include/linux/sigma.h create mode 100644 sound/soc/codecs/sigmadsp.c create mode 100644 sound/soc/codecs/sigmadsp.h
diff --git a/MAINTAINERS b/MAINTAINERS index fd7e441..d14e47a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -542,6 +542,7 @@ F: sound/soc/codecs/adau* F: sound/soc/codecs/adav* F: sound/soc/codecs/ad1* F: sound/soc/codecs/ssm* +F: sound/soc/codecs/sigma.*
ANALOG DEVICES INC ASOC DRIVERS L: uclinux-dist-devel@blackfin.uclinux.org diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index efba163..9b00072 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -145,18 +145,6 @@ 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 - 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. - source "drivers/firmware/google/Kconfig"
endmenu diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index 47338c9..5a7e273 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -12,6 +12,5 @@ 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
obj-$(CONFIG_GOOGLE_FIRMWARE) += google/ diff --git a/drivers/firmware/sigma.c b/drivers/firmware/sigma.c deleted file mode 100644 index 749932c..0000000 --- a/drivers/firmware/sigma.c +++ /dev/null @@ -1,147 +0,0 @@ -/* - * Load Analog Devices SigmaStudio firmware files - * - * Copyright 2009-2011 Analog Devices Inc. - * - * Licensed under the GPL-2 or later. - */ - -#include <linux/crc32.h> -#include <linux/delay.h> -#include <linux/firmware.h> -#include <linux/kernel.h> -#include <linux/i2c.h> -#include <linux/module.h> -#include <linux/sigma.h> - -static size_t sigma_action_size(struct sigma_action *sa) -{ - size_t payload = 0; - - switch (sa->instr) { - case SIGMA_ACTION_WRITEXBYTES: - case SIGMA_ACTION_WRITESINGLE: - case SIGMA_ACTION_WRITESAFELOAD: - payload = sigma_action_len(sa); - break; - default: - break; - } - - payload = ALIGN(payload, 2); - - return payload + sizeof(struct sigma_action); -} - -static int -process_sigma_action(struct i2c_client *client, struct sigma_action *sa) -{ - size_t len = sigma_action_len(sa); - int ret; - - 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: - ret = i2c_master_send(client, (void *)&sa->addr, len); - if (ret < 0) - return -EINVAL; - break; - case SIGMA_ACTION_DELAY: - udelay(len); - len = 0; - break; - case SIGMA_ACTION_END: - return 0; - default: - return -EINVAL; - } - - return 1; -} - -static int -process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw) -{ - struct sigma_action *sa; - size_t size; - int ret; - - while (ssfw->pos + sizeof(*sa) <= ssfw->fw->size) { - sa = (struct sigma_action *)(ssfw->fw->data + ssfw->pos); - - size = sigma_action_size(sa); - ssfw->pos += size; - if (ssfw->pos > ssfw->fw->size || size == 0) - break; - - ret = process_sigma_action(client, sa); - - pr_debug("%s: action returned %i\n", __func__, ret); - - if (ret <= 0) - return ret; - } - - if (ssfw->pos != ssfw->fw->size) - return -EINVAL; - - return 0; -} - -int process_sigma_firmware(struct i2c_client *client, const char *name) -{ - int ret; - struct sigma_firmware_header *ssfw_head; - struct sigma_firmware ssfw; - const struct firmware *fw; - u32 crc; - - 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 */ - ret = -EINVAL; - - /* Reject too small or unreasonable large files. The upper limit is - * chosen a bit arbitrarily but it should be enough for all practical - * purposes and having the limit makes it easier to avoid integer - * overflows later in the loading process. */ - if (fw->size < sizeof(*ssfw_head) || fw->size >= 0x4000000) - goto done; - - ssfw_head = (void *)fw->data; - if (memcmp(ssfw_head->magic, SIGMA_MAGIC, ARRAY_SIZE(ssfw_head->magic))) - goto done; - - crc = crc32(0, fw->data + sizeof(*ssfw_head), - fw->size - sizeof(*ssfw_head)); - pr_debug("%s: crc=%x\n", __func__, crc); - if (crc != le32_to_cpu(ssfw_head->crc)) - goto done; - - 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); - -MODULE_LICENSE("GPL"); diff --git a/include/linux/sigma.h b/include/linux/sigma.h deleted file mode 100644 index 745f1bd..0000000 --- a/include/linux/sigma.h +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Load firmware files from Analog Devices SigmaStudio - * - * Copyright 2009-2011 Analog Devices Inc. - * - * Licensed under the GPL-2 or later. - */ - -#ifndef __SIGMA_FIRMWARE_H__ -#define __SIGMA_FIRMWARE_H__ - -#include <linux/firmware.h> -#include <linux/types.h> - -struct i2c_client; - -#define SIGMA_MAGIC "ADISIGM" - -struct sigma_firmware { - const struct firmware *fw; - size_t pos; -}; - -struct sigma_firmware_header { - unsigned char magic[7]; - u8 version; - __le32 crc; -} __packed; - -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; - __le16 len; - __be16 addr; - unsigned char payload[]; -} __packed; - -static inline u32 sigma_action_len(struct sigma_action *sa) -{ - return (sa->len_hi << 16) | le16_to_cpu(sa->len); -} - -extern int process_sigma_firmware(struct i2c_client *client, const char *name); - -#endif diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 686f45a..593174c 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -141,7 +141,7 @@ config SND_SOC_AD73311 tristate
config SND_SOC_ADAU1701 - select SIGMA + select SND_SOC_SIGMADSP tristate
config SND_SOC_ADAU1373 @@ -234,6 +234,10 @@ config SND_SOC_RT5631 config SND_SOC_SGTL5000 tristate
+config SND_SOC_SIGMADSP + tristate + select CRC32 + config SND_SOC_SN95031 tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 62b01e4..4aea5b1 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -33,6 +33,7 @@ snd-soc-rt5631-objs := rt5631.o snd-soc-sgtl5000-objs := sgtl5000.o snd-soc-alc5623-objs := alc5623.o snd-soc-alc5632-objs := alc5632.o +snd-soc-sigma-objs := sigma.o snd-soc-sn95031-objs := sn95031.o snd-soc-spdif-objs := spdif_transciever.o snd-soc-ssm2602-objs := ssm2602.o @@ -134,6 +135,7 @@ obj-$(CONFIG_SND_SOC_MAX9850) += snd-soc-max9850.o obj-$(CONFIG_SND_SOC_PCM3008) += snd-soc-pcm3008.o obj-$(CONFIG_SND_SOC_RT5631) += snd-soc-rt5631.o obj-$(CONFIG_SND_SOC_SGTL5000) += snd-soc-sgtl5000.o +obj-$(CONFIG_SND_SOC_SIGMA) += snd-soc-sigma.o obj-$(CONFIG_SND_SOC_SN95031) +=snd-soc-sn95031.o obj-$(CONFIG_SND_SOC_SPDIF) += snd-soc-spdif.o obj-$(CONFIG_SND_SOC_SSM2602) += snd-soc-ssm2602.o diff --git a/sound/soc/codecs/adau1701.c b/sound/soc/codecs/adau1701.c index 8b7e1c5..6a6af56 100644 --- a/sound/soc/codecs/adau1701.c +++ b/sound/soc/codecs/adau1701.c @@ -12,13 +12,13 @@ #include <linux/init.h> #include <linux/i2c.h> #include <linux/delay.h> -#include <linux/sigma.h> #include <linux/slab.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h>
+#include "sigmadsp.h" #include "adau1701.h"
#define ADAU1701_DSPCTRL 0x1c diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c new file mode 100644 index 0000000..01b69f4 --- /dev/null +++ b/sound/soc/codecs/sigmadsp.c @@ -0,0 +1,148 @@ +/* + * Load Analog Devices SigmaStudio firmware files + * + * Copyright 2009-2011 Analog Devices Inc. + * + * Licensed under the GPL-2 or later. + */ + +#include <linux/crc32.h> +#include <linux/delay.h> +#include <linux/firmware.h> +#include <linux/kernel.h> +#include <linux/i2c.h> +#include <linux/module.h> + +#include "sigmadsp.h" + +static size_t sigma_action_size(struct sigma_action *sa) +{ + size_t payload = 0; + + switch (sa->instr) { + case SIGMA_ACTION_WRITEXBYTES: + case SIGMA_ACTION_WRITESINGLE: + case SIGMA_ACTION_WRITESAFELOAD: + payload = sigma_action_len(sa); + break; + default: + break; + } + + payload = ALIGN(payload, 2); + + return payload + sizeof(struct sigma_action); +} + +static int +process_sigma_action(struct i2c_client *client, struct sigma_action *sa) +{ + size_t len = sigma_action_len(sa); + int ret; + + 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: + ret = i2c_master_send(client, (void *)&sa->addr, len); + if (ret < 0) + return -EINVAL; + break; + case SIGMA_ACTION_DELAY: + udelay(len); + len = 0; + break; + case SIGMA_ACTION_END: + return 0; + default: + return -EINVAL; + } + + return 1; +} + +static int +process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw) +{ + struct sigma_action *sa; + size_t size; + int ret; + + while (ssfw->pos + sizeof(*sa) <= ssfw->fw->size) { + sa = (struct sigma_action *)(ssfw->fw->data + ssfw->pos); + + size = sigma_action_size(sa); + ssfw->pos += size; + if (ssfw->pos > ssfw->fw->size || size == 0) + break; + + ret = process_sigma_action(client, sa); + + pr_debug("%s: action returned %i\n", __func__, ret); + + if (ret <= 0) + return ret; + } + + if (ssfw->pos != ssfw->fw->size) + return -EINVAL; + + return 0; +} + +int process_sigma_firmware(struct i2c_client *client, const char *name) +{ + int ret; + struct sigma_firmware_header *ssfw_head; + struct sigma_firmware ssfw; + const struct firmware *fw; + u32 crc; + + 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 */ + ret = -EINVAL; + + /* Reject too small or unreasonable large files. The upper limit is + * chosen a bit arbitrarily but it should be enough for all practical + * purposes and having the limit makes it easier to avoid integer + * overflows later in the loading process. */ + if (fw->size < sizeof(*ssfw_head) || fw->size >= 0x4000000) + goto done; + + ssfw_head = (void *)fw->data; + if (memcmp(ssfw_head->magic, SIGMA_MAGIC, ARRAY_SIZE(ssfw_head->magic))) + goto done; + + crc = crc32(0, fw->data + sizeof(*ssfw_head), + fw->size - sizeof(*ssfw_head)); + pr_debug("%s: crc=%x\n", __func__, crc); + if (crc != le32_to_cpu(ssfw_head->crc)) + goto done; + + 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); + +MODULE_LICENSE("GPL"); diff --git a/sound/soc/codecs/sigmadsp.h b/sound/soc/codecs/sigmadsp.h new file mode 100644 index 0000000..745f1bd --- /dev/null +++ b/sound/soc/codecs/sigmadsp.h @@ -0,0 +1,55 @@ +/* + * Load firmware files from Analog Devices SigmaStudio + * + * Copyright 2009-2011 Analog Devices Inc. + * + * Licensed under the GPL-2 or later. + */ + +#ifndef __SIGMA_FIRMWARE_H__ +#define __SIGMA_FIRMWARE_H__ + +#include <linux/firmware.h> +#include <linux/types.h> + +struct i2c_client; + +#define SIGMA_MAGIC "ADISIGM" + +struct sigma_firmware { + const struct firmware *fw; + size_t pos; +}; + +struct sigma_firmware_header { + unsigned char magic[7]; + u8 version; + __le32 crc; +} __packed; + +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; + __le16 len; + __be16 addr; + unsigned char payload[]; +} __packed; + +static inline u32 sigma_action_len(struct sigma_action *sa) +{ + return (sa->len_hi << 16) | le16_to_cpu(sa->len); +} + +extern int process_sigma_firmware(struct i2c_client *client, const char *name); + +#endif
It has been pointed out previously, that the firmware subsystem is not the right place for the SigmaDSP firmware loader. Furthermore the SigmaDSP is currently only used in audio products and we are aiming for better integration into the ASoC framework in the future, with support for ALSA controls for firmware parameters and support dynamic power management as well. So the natural choice for the SigmaDSP firmware loader is the ASoC subsystem.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de
--- Changes since v1: * Fix last-minute renames * Generate patch with `git format-patch -M ...` --- MAINTAINERS | 1 + drivers/firmware/Kconfig | 12 ------------ drivers/firmware/Makefile | 1 - sound/soc/codecs/Kconfig | 6 +++++- sound/soc/codecs/Makefile | 2 ++ sound/soc/codecs/adau1701.c | 2 +- .../sigma.c => sound/soc/codecs/sigmadsp.c | 3 ++- .../linux/sigma.h => sound/soc/codecs/sigmadsp.h | 0 8 files changed, 11 insertions(+), 16 deletions(-) rename drivers/firmware/sigma.c => sound/soc/codecs/sigmadsp.c (99%) rename include/linux/sigma.h => sound/soc/codecs/sigmadsp.h (100%)
diff --git a/MAINTAINERS b/MAINTAINERS index fd7e441..6a93a93 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -542,6 +542,7 @@ F: sound/soc/codecs/adau* F: sound/soc/codecs/adav* F: sound/soc/codecs/ad1* F: sound/soc/codecs/ssm* +F: sound/soc/codecs/sigmadsp.*
ANALOG DEVICES INC ASOC DRIVERS L: uclinux-dist-devel@blackfin.uclinux.org diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index efba163..9b00072 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -145,18 +145,6 @@ 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 - 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. - source "drivers/firmware/google/Kconfig"
endmenu diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index 47338c9..5a7e273 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -12,6 +12,5 @@ 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
obj-$(CONFIG_GOOGLE_FIRMWARE) += google/ diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 686f45a..593174c 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -141,7 +141,7 @@ config SND_SOC_AD73311 tristate
config SND_SOC_ADAU1701 - select SIGMA + select SND_SOC_SIGMADSP tristate
config SND_SOC_ADAU1373 @@ -234,6 +234,10 @@ config SND_SOC_RT5631 config SND_SOC_SGTL5000 tristate
+config SND_SOC_SIGMADSP + tristate + select CRC32 + config SND_SOC_SN95031 tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index 62b01e4..fa15006 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -33,6 +33,7 @@ snd-soc-rt5631-objs := rt5631.o snd-soc-sgtl5000-objs := sgtl5000.o snd-soc-alc5623-objs := alc5623.o snd-soc-alc5632-objs := alc5632.o +snd-soc-sigmadsp-objs := sigmadsp.o snd-soc-sn95031-objs := sn95031.o snd-soc-spdif-objs := spdif_transciever.o snd-soc-ssm2602-objs := ssm2602.o @@ -134,6 +135,7 @@ obj-$(CONFIG_SND_SOC_MAX9850) += snd-soc-max9850.o obj-$(CONFIG_SND_SOC_PCM3008) += snd-soc-pcm3008.o obj-$(CONFIG_SND_SOC_RT5631) += snd-soc-rt5631.o obj-$(CONFIG_SND_SOC_SGTL5000) += snd-soc-sgtl5000.o +obj-$(CONFIG_SND_SOC_SIGMADSP) += snd-soc-sigmadsp.o obj-$(CONFIG_SND_SOC_SN95031) +=snd-soc-sn95031.o obj-$(CONFIG_SND_SOC_SPDIF) += snd-soc-spdif.o obj-$(CONFIG_SND_SOC_SSM2602) += snd-soc-ssm2602.o diff --git a/sound/soc/codecs/adau1701.c b/sound/soc/codecs/adau1701.c index 8b7e1c5..6a6af56 100644 --- a/sound/soc/codecs/adau1701.c +++ b/sound/soc/codecs/adau1701.c @@ -12,13 +12,13 @@ #include <linux/init.h> #include <linux/i2c.h> #include <linux/delay.h> -#include <linux/sigma.h> #include <linux/slab.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> #include <sound/soc.h>
+#include "sigmadsp.h" #include "adau1701.h"
#define ADAU1701_DSPCTRL 0x1c diff --git a/drivers/firmware/sigma.c b/sound/soc/codecs/sigmadsp.c similarity index 99% rename from drivers/firmware/sigma.c rename to sound/soc/codecs/sigmadsp.c index 749932c..01b69f4 100644 --- a/drivers/firmware/sigma.c +++ b/sound/soc/codecs/sigmadsp.c @@ -12,7 +12,8 @@ #include <linux/kernel.h> #include <linux/i2c.h> #include <linux/module.h> -#include <linux/sigma.h> + +#include "sigmadsp.h"
static size_t sigma_action_size(struct sigma_action *sa) { diff --git a/include/linux/sigma.h b/sound/soc/codecs/sigmadsp.h similarity index 100% rename from include/linux/sigma.h rename to sound/soc/codecs/sigmadsp.h
On Thursday 24 November 2011 08:15:16 Lars-Peter Clausen wrote:
It has been pointed out previously, that the firmware subsystem is not the right place for the SigmaDSP firmware loader. Furthermore the SigmaDSP is currently only used in audio products and we are aiming for better integration into the ASoC framework in the future, with support for ALSA controls for firmware parameters and support dynamic power management as well. So the natural choice for the SigmaDSP firmware loader is the ASoC subsystem.
Acked-by: Mike Frysinger vapier@gentoo.org -mike
Provide some error messages when loading the firmware fails, so it is possible to diagnose the reason for the failure.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de --- sound/soc/codecs/sigmadsp.c | 14 +++++++++++--- 1 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c index 01b69f4..e13501a 100644 --- a/sound/soc/codecs/sigmadsp.c +++ b/sound/soc/codecs/sigmadsp.c @@ -118,18 +118,26 @@ int process_sigma_firmware(struct i2c_client *client, const char *name) * chosen a bit arbitrarily but it should be enough for all practical * purposes and having the limit makes it easier to avoid integer * overflows later in the loading process. */ - if (fw->size < sizeof(*ssfw_head) || fw->size >= 0x4000000) + if (fw->size < sizeof(*ssfw_head) || fw->size >= 0x4000000) { + dev_err(&client->dev, "Failed to load firmware: Invalid size\n"); goto done; + }
ssfw_head = (void *)fw->data; - if (memcmp(ssfw_head->magic, SIGMA_MAGIC, ARRAY_SIZE(ssfw_head->magic))) + if (memcmp(ssfw_head->magic, SIGMA_MAGIC, ARRAY_SIZE(ssfw_head->magic))) { + dev_err(&client->dev, "Failed to load firmware: Invalid magic\n"); goto done; + }
crc = crc32(0, fw->data + sizeof(*ssfw_head), fw->size - sizeof(*ssfw_head)); pr_debug("%s: crc=%x\n", __func__, crc); - if (crc != le32_to_cpu(ssfw_head->crc)) + if (crc != le32_to_cpu(ssfw_head->crc)) { + dev_err(&client->dev, "Failed to load firmware: Wrong crc checksum:" \ + " expected %x got %x.\n", + le32_to_cpu(ssfw_head->crc), crc); goto done; + }
ssfw.pos = sizeof(*ssfw_head);
On Thursday 24 November 2011 07:48:25 Lars-Peter Clausen wrote:
- if (crc != le32_to_cpu(ssfw_head->crc)) {
dev_err(&client->dev, "Failed to load firmware: Wrong crc checksum:" \" expected %x got %x.\n",
it's my understanding that string constants should not be split -mike
On 11/24/2011 06:32 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:25 Lars-Peter Clausen wrote:
- if (crc != le32_to_cpu(ssfw_head->crc)) {
dev_err(&client->dev, "Failed to load firmware: Wrong crc checksum:" \" expected %x got %x.\n",it's my understanding that string constants should not be split -mike
I think it is fine here. You probably wouldn't grep for %x anyway.
On Friday 25 November 2011 03:59:21 Lars-Peter Clausen wrote:
On 11/24/2011 06:32 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:25 Lars-Peter Clausen wrote:
- if (crc != le32_to_cpu(ssfw_head->crc)) {
dev_err(&client->dev, "Failed to load firmware: Wrong crc
checksum:"
\ + " expected %x got %x.\n",
it's my understanding that string constants should not be split
I think it is fine here. You probably wouldn't grep for %x anyway.
no, but you would copy and paste a string like: "Failed to load firmware: Wrong crc checksum: expected" and then not find a match
i think it's best to follow the style anyways ...
also, no need for the period at the end of the msg. that's useless noise and can be annoying to copying & pasting numbers. -mike
Move the structs and functions only used by SigmaDSP firmware loader itself from the header to the c file.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de --- sound/soc/codecs/sigmadsp.c | 35 +++++++++++++++++++++++++++++++++++ sound/soc/codecs/sigmadsp.h | 39 --------------------------------------- 2 files changed, 35 insertions(+), 39 deletions(-)
diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c index e13501a..c6964f4 100644 --- a/sound/soc/codecs/sigmadsp.c +++ b/sound/soc/codecs/sigmadsp.c @@ -15,6 +15,41 @@
#include "sigmadsp.h"
+#define SIGMA_MAGIC "ADISIGM" + +struct sigma_firmware_header { + unsigned char magic[7]; + u8 version; + __le32 crc; +} __packed; + +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; + __le16 len; + __be16 addr; + unsigned char payload[]; +} __packed; + +struct sigma_firmware { + const struct firmware *fw; + size_t pos; +}; + +static inline u32 sigma_action_len(struct sigma_action *sa) +{ + return (sa->len_hi << 16) | le16_to_cpu(sa->len); +} static size_t sigma_action_size(struct sigma_action *sa) { size_t payload = 0; diff --git a/sound/soc/codecs/sigmadsp.h b/sound/soc/codecs/sigmadsp.h index 745f1bd..99a6091 100644 --- a/sound/soc/codecs/sigmadsp.h +++ b/sound/soc/codecs/sigmadsp.h @@ -9,47 +9,8 @@ #ifndef __SIGMA_FIRMWARE_H__ #define __SIGMA_FIRMWARE_H__
-#include <linux/firmware.h> -#include <linux/types.h> - struct i2c_client;
-#define SIGMA_MAGIC "ADISIGM" - -struct sigma_firmware { - const struct firmware *fw; - size_t pos; -}; - -struct sigma_firmware_header { - unsigned char magic[7]; - u8 version; - __le32 crc; -} __packed; - -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; - __le16 len; - __be16 addr; - unsigned char payload[]; -} __packed; - -static inline u32 sigma_action_len(struct sigma_action *sa) -{ - return (sa->len_hi << 16) | le16_to_cpu(sa->len); -} - extern int process_sigma_firmware(struct i2c_client *client, const char *name);
#endif
On Thursday 24 November 2011 07:48:26 Lars-Peter Clausen wrote:
Move the structs and functions only used by SigmaDSP firmware loader itself from the header to the c file.
Acked-by: Mike Frysinger vapier@gentoo.org -mike
Add support for loading the SigmaDSP firmware using regmap. This allows us to transparently use SPI or I2C as the transport protocol on devices which support them.
For now we keep the old I2C support since we have one user of this which is not straight forward to convert to regmap, due to variable length registers.
Signed-off-by: Lars-Peter Clausen lars@metafoo.de --- sound/soc/codecs/sigmadsp.c | 68 ++++++++++++++++++++++++++++++++++-------- sound/soc/codecs/sigmadsp.h | 5 +++ 2 files changed, 60 insertions(+), 13 deletions(-)
diff --git a/sound/soc/codecs/sigmadsp.c b/sound/soc/codecs/sigmadsp.c index c6964f4..6fed878d 100644 --- a/sound/soc/codecs/sigmadsp.c +++ b/sound/soc/codecs/sigmadsp.c @@ -11,6 +11,7 @@ #include <linux/firmware.h> #include <linux/kernel.h> #include <linux/i2c.h> +#include <linux/regmap.h> #include <linux/module.h>
#include "sigmadsp.h" @@ -44,12 +45,17 @@ struct sigma_action { struct sigma_firmware { const struct firmware *fw; size_t pos; + + void *control_data; + int (*write)(void *control_data, const struct sigma_action *sa, + size_t len); };
static inline u32 sigma_action_len(struct sigma_action *sa) { return (sa->len_hi << 16) | le16_to_cpu(sa->len); } + static size_t sigma_action_size(struct sigma_action *sa) { size_t payload = 0; @@ -69,8 +75,22 @@ static size_t sigma_action_size(struct sigma_action *sa) return payload + sizeof(struct sigma_action); }
+static int sigma_action_write_regmap(void *control_data, + const struct sigma_action *sa, size_t len) +{ + return regmap_raw_write(control_data, le16_to_cpu(sa->addr), + sa->payload, len - 2); +} + +static int sigma_action_write_i2c(void *control_data, + const struct sigma_action *sa, size_t len) +{ + return i2c_master_send(control_data, (const unsigned char *)&sa->addr, + len); +} + static int -process_sigma_action(struct i2c_client *client, struct sigma_action *sa) +process_sigma_action(struct sigma_firmware *ssfw, struct sigma_action *sa) { size_t len = sigma_action_len(sa); int ret; @@ -82,7 +102,7 @@ process_sigma_action(struct i2c_client *client, struct sigma_action *sa) case SIGMA_ACTION_WRITEXBYTES: case SIGMA_ACTION_WRITESINGLE: case SIGMA_ACTION_WRITESAFELOAD: - ret = i2c_master_send(client, (void *)&sa->addr, len); + ret = ssfw->write(ssfw->control_data, sa, len); if (ret < 0) return -EINVAL; break; @@ -100,7 +120,7 @@ process_sigma_action(struct i2c_client *client, struct sigma_action *sa) }
static int -process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw) +process_sigma_actions(struct sigma_firmware *ssfw) { struct sigma_action *sa; size_t size; @@ -114,7 +134,7 @@ process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw) if (ssfw->pos > ssfw->fw->size || size == 0) break;
- ret = process_sigma_action(client, sa); + ret = process_sigma_action(ssfw, sa);
pr_debug("%s: action returned %i\n", __func__, ret);
@@ -128,23 +148,23 @@ process_sigma_actions(struct i2c_client *client, struct sigma_firmware *ssfw) return 0; }
-int process_sigma_firmware(struct i2c_client *client, const char *name) +static int _process_sigma_firmware(struct device *dev, + struct sigma_firmware *ssfw, const char *name) { int ret; struct sigma_firmware_header *ssfw_head; - struct sigma_firmware ssfw; const struct firmware *fw; u32 crc;
pr_debug("%s: loading firmware %s\n", __func__, name);
/* first load the blob */ - ret = request_firmware(&fw, name, &client->dev); + ret = request_firmware(&fw, name, dev); if (ret) { pr_debug("%s: request_firmware() failed with %i\n", __func__, ret); return ret; } - ssfw.fw = fw; + ssfw->fw = fw;
/* then verify the header */ ret = -EINVAL; @@ -154,13 +174,13 @@ int process_sigma_firmware(struct i2c_client *client, const char *name) * purposes and having the limit makes it easier to avoid integer * overflows later in the loading process. */ if (fw->size < sizeof(*ssfw_head) || fw->size >= 0x4000000) { - dev_err(&client->dev, "Failed to load firmware: Invalid size\n"); + dev_err(dev, "Failed to load firmware: Invalid size\n"); goto done; }
ssfw_head = (void *)fw->data; if (memcmp(ssfw_head->magic, SIGMA_MAGIC, ARRAY_SIZE(ssfw_head->magic))) { - dev_err(&client->dev, "Failed to load firmware: Invalid magic\n"); + dev_err(dev, "Failed to load firmware: Invalid magic\n"); goto done; }
@@ -168,16 +188,16 @@ int process_sigma_firmware(struct i2c_client *client, const char *name) fw->size - sizeof(*ssfw_head)); pr_debug("%s: crc=%x\n", __func__, crc); if (crc != le32_to_cpu(ssfw_head->crc)) { - dev_err(&client->dev, "Failed to load firmware: Wrong crc checksum:" \ + dev_err(dev, "Failed to load firmware: Wrong crc checksum:" \ " expected %x got %x.\n", le32_to_cpu(ssfw_head->crc), crc); goto done; }
- ssfw.pos = sizeof(*ssfw_head); + ssfw->pos = sizeof(*ssfw_head);
/* finally process all of the actions */ - ret = process_sigma_actions(client, &ssfw); + ret = process_sigma_actions(ssfw);
done: release_firmware(fw); @@ -186,6 +206,28 @@ int process_sigma_firmware(struct i2c_client *client, const char *name)
return ret; } + +int process_sigma_firmware(struct i2c_client *client, const char *name) +{ + struct sigma_firmware ssfw; + + ssfw.control_data = client; + ssfw.write = sigma_action_write_i2c; + + return _process_sigma_firmware(&client->dev, &ssfw, name); +} EXPORT_SYMBOL(process_sigma_firmware);
+int process_sigma_firmware_regmap(struct device *dev, struct regmap *regmap, + const char *name) +{ + struct sigma_firmware ssfw; + + ssfw.control_data = regmap; + ssfw.write = sigma_action_write_regmap; + + return _process_sigma_firmware(dev, &ssfw, name); +} +EXPORT_SYMBOL(process_sigma_firmware_regmap); + MODULE_LICENSE("GPL"); diff --git a/sound/soc/codecs/sigmadsp.h b/sound/soc/codecs/sigmadsp.h index 99a6091..e439cbd 100644 --- a/sound/soc/codecs/sigmadsp.h +++ b/sound/soc/codecs/sigmadsp.h @@ -9,8 +9,13 @@ #ifndef __SIGMA_FIRMWARE_H__ #define __SIGMA_FIRMWARE_H__
+#include <linux/device.h> +#include <linux/regmap.h> + struct i2c_client;
extern int process_sigma_firmware(struct i2c_client *client, const char *name); +extern int process_sigma_firmware_regmap(struct device *dev, + struct regmap *regmap, const char *name);
#endif
On Thursday 24 November 2011 07:48:27 Lars-Peter Clausen wrote:
Add support for loading the SigmaDSP firmware using regmap. This allows us to transparently use SPI or I2C as the transport protocol on devices which support them.
Acked-by: Mike Frysinger vapier@gentoo.org
For now we keep the old I2C support since we have one user of this which is not straight forward to convert to regmap, due to variable length registers.
add a Kconfig knob so it's disabled by default except for the old codec ? -mike
On 11/24/2011 06:30 PM, Mike Frysinger wrote:
On Thursday 24 November 2011 07:48:27 Lars-Peter Clausen wrote:
Add support for loading the SigmaDSP firmware using regmap. This allows us to transparently use SPI or I2C as the transport protocol on devices which support them.
Acked-by: Mike Frysinger vapier@gentoo.org
For now we keep the old I2C support since we have one user of this which is not straight forward to convert to regmap, due to variable length registers.
add a Kconfig knob so it's disabled by default except for the old codec ? -mike
I don't think it's worth it. The I2C specific code is maybe 15 lines.
On Thursday 24 November 2011 07:48:20 Lars-Peter Clausen wrote:
The SigmaDSP firmware loader currently does not perform enough boundary size checks when processing the firmware. As a result it is possible that a malformed firmware can cause an out of bounds memory access.
This patch adds checks which ensure that both the action header and the payload are completely inside the firmware data boundaries before processing them.
in general this looks fine ...
--- a/drivers/firmware/sigma.c +++ b/drivers/firmware/sigma.c
-/* Return: 0==OK, <0==error, =1 ==no more actions */ static int +process_sigma_action(struct i2c_client *client, struct sigma_action *sa)
looks like you're inverting the semantics of this func. i'd add an updated comment above the func to document the new return values.
- /* Reject too small or unreasonable large files. The upper limit is
* chosen a bit arbitrarily but it should be enough for all practical* purposes and having the limit makes it easier to avoid integer* overflows later in the loading process. */
multi-line comment style: /* * line one * line two */ -mike
participants (2)
-
Lars-Peter Clausen -
Mike Frysinger