On 09/10/2012 08:52 PM, Francesco Lavra wrote: > On 09/10/2012 05:04 PM, Sylwester Nawrocki wrote: >> On 09/09/2012 06:01 PM, Francesco Lavra wrote: >>>> +static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd) >>>> +{ >>>> + const struct firmware *fw; >>>> + int err, i, regs_num; >>>> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>>> + u16 val; >>>> + u32 addr, crc, crc_file, addr_inc = 0; >>>> + >>>> + err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev); >>>> + if (err) { >>>> + v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE); >>>> + return err; >>>> + } >>>> + regs_num = *(u32 *)(fw->data); >>>> + v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n", >>>> + S5K4ECGX_FIRMWARE, fw->size, regs_num); >>>> + regs_num++; /* Add header */ >>>> + if (fw->size != regs_num * FW_RECORD_SIZE + FW_CRC_SIZE) { >>>> + err = -EINVAL; >>>> + goto fw_out; >>>> + } >>>> + crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE); >>> >>> Depending on the value of regs_num, this may result in unaligned access >> >> Thanks for the catch. I think it is not the only place where unaligned >> issues are possible. Since the data records are 4-byte address + 2-byte >> value there is also an issue with reading the address entries. Assuming >> fw->data is aligned to at least 2-bytes (not quite sure if we can assume >> that) there should be no problem with reading 2-byte register values. > > I'm not sure 2-byte alignment can be safely assumed, either. > >> We could change the data types of the register values from u16 to u32, >> wasting some memory (there is approximately 3 000 records), so there is >> no other data types in the file structure than u32. Or use a patch as >> below. Not sure what's better. > > I prefer the approach of your patch below, but I would use get_unaligned > to get the 2-byte values, too. Also there are another couple of > glitches, see below. OK, thanks for the feedback. It was also my preference. The performance impact seems insignificant, given a write of each record takes time of 1 ms order. >> 8<--------------------------------------------------------------------- >> From a970480b99bdb74e2bf48e1a321724231e6516a0 Mon Sep 17 00:00:00 2001 >> From: Sylwester Nawrocki<sylvester.nawrocki@xxxxxxxxx> >> Date: Sun, 9 Sep 2012 19:56:31 +0200 >> Subject: [PATCH] s5k4ecgx: Fix unaligned access issues >> >> Signed-off-by: Sylwester Nawrocki<sylvester.nawrocki@xxxxxxxxx> >> --- >> drivers/media/i2c/s5k4ecgx.c | 16 ++++++++++++---- >> 1 files changed, 12 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c >> index 0ef0b7d..4c6439a 100644 >> --- a/drivers/media/i2c/s5k4ecgx.c >> +++ b/drivers/media/i2c/s5k4ecgx.c >> @@ -24,6 +24,7 @@ >> #include<linux/module.h> >> #include<linux/regulator/consumer.h> >> #include<linux/slab.h> >> +#include<asm/unaligned.h> >> >> #include<media/media-entity.h> >> #include<media/s5k4ecgx.h> >> @@ -331,6 +332,7 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd) >> const struct firmware *fw; >> int err, i, regs_num; >> u32 addr, crc, crc_file, addr_inc = 0; >> + const u8 *ptr; >> u16 val; >> >> err = request_firmware(&fw, S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev); >> @@ -338,7 +340,7 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd) >> v4l2_err(sd, "Failed to read firmware %s\n", S5K4ECGX_FIRMWARE); >> return err; >> } >> - regs_num = le32_to_cpu(*(u32 *)fw->data); >> + regs_num = le32_to_cpu(get_unaligned((__le32 *)fw->data)); >> >> v4l2_dbg(3, debug, sd, "FW: %s size %d register sets %d\n", >> S5K4ECGX_FIRMWARE, fw->size, regs_num); >> @@ -349,7 +351,8 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd) >> goto fw_out; >> } >> >> - crc_file = *(u32 *)(fw->data + regs_num * FW_RECORD_SIZE); >> + memcpy(&crc_file, fw->data + regs_num * FW_RECORD_SIZE, sizeof(u32)); > > crc_file should be converted from little endian to native endian. Right, I should have verified that crc32_le() return value is in native endianness. >> + >> crc = crc32_le(~0, fw->data, regs_num * FW_RECORD_SIZE); >> if (crc != crc_file) { >> v4l2_err(sd, "FW: invalid crc (%#x:%#x)\n", crc, crc_file); >> @@ -357,9 +360,14 @@ static int s5k4ecgx_load_firmware(struct v4l2_subdev *sd) >> goto fw_out; >> } >> >> + ptr = fw->data + FW_RECORD_SIZE; >> + >> for (i = 1; i< regs_num; i++) { >> - addr = le32_to_cpu(*(u32 *)(fw->data + i * FW_RECORD_SIZE)); >> - val = le16_to_cpu(*(u16 *)(fw->data + i * FW_RECORD_SIZE + 4)); >> + addr = le32_to_cpu(get_unaligned((__le32 *)ptr)); >> + ptr += 4; >> + val = le16_to_cpu(*(__le16 *)ptr); >> + ptr += FW_RECORD_SIZE; > > ptr is being incremented by (4 + FW_RECORD_SIZE) bytes at each iteration. Oops, I was to quick in sending that patch out. Indeed, that's wrong. Sangwook, FWIW, I just pushed the corrected patch to my tree (http://git.linuxtv.org/snawrocki/media.git/commitdiff/4a0ecad6f08ccbba3) -- Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html