From patchwork Mon Sep 10 15:04:21 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Sylwester Nawrocki/Kernel \\\(PLT\\\) /SRPOL/Staff Engineer/Samsung Electronics" X-Patchwork-Id: 11287 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id C81E323EFF for ; Mon, 10 Sep 2012 15:04:28 +0000 (UTC) Received: from mail-ie0-f180.google.com (mail-ie0-f180.google.com [209.85.223.180]) by fiordland.canonical.com (Postfix) with ESMTP id 43EC6A18374 for ; Mon, 10 Sep 2012 15:04:28 +0000 (UTC) Received: by ieak11 with SMTP id k11so3264957iea.11 for ; Mon, 10 Sep 2012 08:04:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf:message-id :date:from:user-agent:mime-version:to:cc:subject:references :in-reply-to:content-type:content-transfer-encoding:x-tm-as-mml :x-gm-message-state; bh=0MQ4CE/PP7SuBigx5W7pdLllV27o/NIyK8m5cnnBirk=; b=PmRMBjVfDC/FbDcUNsHk2B5uFd3KrSL4RdohtWaJxFP/ihUlhNvB5ztipMs9qX9Mo8 Y3hngvltWVoBgITCjdO2ngoR6CDIxtUMRlm/6TI3Ix4Il5x2dZUyqXs76s1C39BbKVxB mfwyeHIjHlC0POIL0DqKrS50eAc64stEpwivjpmmjVvclGtJZ8DnjQOwCq6snwoNREej KHS93jS1YkMucjhRo9lsEyU14PsG+rtePMjWVwRAIB/cFEqkmVNMyHEhrSTKhIM/xrAK LZyhgEWSPxR3ce7Moo72B+BLsWDRccFUWR4ra2J0ldstL7CP1IFzt14rTtT99Mp0vCqO lPbw== Received: by 10.50.191.227 with SMTP id hb3mr3424036igc.43.1347289467657; Mon, 10 Sep 2012 08:04:27 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.50.184.232 with SMTP id ex8csp98392igc; Mon, 10 Sep 2012 08:04:27 -0700 (PDT) Received: by 10.66.79.38 with SMTP id g6mr22187338pax.40.1347289465219; Mon, 10 Sep 2012 08:04:25 -0700 (PDT) Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com. [210.118.77.11]) by mx.google.com with ESMTP id pp2si20795617pbc.46.2012.09.10.08.04.23; Mon, 10 Sep 2012 08:04:25 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of s.nawrocki@samsung.com designates 210.118.77.11 as permitted sender) client-ip=210.118.77.11; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of s.nawrocki@samsung.com designates 210.118.77.11 as permitted sender) smtp.mail=s.nawrocki@samsung.com Received: from eusync3.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by mailout1.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MA500DI2380T1B0@mailout1.w1.samsung.com>; Mon, 10 Sep 2012 16:04:48 +0100 (BST) Received: from [106.116.147.32] by eusync3.samsung.com (Oracle Communications Messaging Server 7u4-23.01(7.0.4.23.0) 64bit (built Aug 10 2011)) with ESMTPA id <0MA5001M437AX060@eusync3.samsung.com>; Mon, 10 Sep 2012 16:04:22 +0100 (BST) Message-id: <504E0175.80504@samsung.com> Date: Mon, 10 Sep 2012 17:04:21 +0200 From: Sylwester Nawrocki User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 MIME-version: 1.0 To: Francesco Lavra , Sangwook Lee Cc: linux-media@vger.kernel.org, linaro-dev@lists.linaro.org, patches@linaro.org, mchehab@infradead.org, kyungmin.park@samsung.com, hans.verkuil@cisco.com, laurent.pinchart@ideasonboard.com, 'Arnd Bergmann' Subject: Re: [RFC PATCH v6] media: add v4l2 subdev driver for S5K4ECGX sensor References: <1346944114-17527-1-git-send-email-sangwook.lee@linaro.org> <504CBD47.5050802@gmail.com> In-reply-to: <504CBD47.5050802@gmail.com> Content-type: text/plain; charset=ISO-8859-1 Content-transfer-encoding: 7bit X-TM-AS-MML: No X-Gm-Message-State: ALoCoQl1MU0ZPU5uJM7sruOPeOWgitXYXRTMr3Ns4QVr2+HnxEx/jACtpUfYs7mAe9n55nK6N5GQ Hi, 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. 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. 8<--------------------------------------------------------------------- >From a970480b99bdb74e2bf48e1a321724231e6516a0 Mon Sep 17 00:00:00 2001 From: Sylwester Nawrocki Date: Sun, 9 Sep 2012 19:56:31 +0200 Subject: [PATCH] s5k4ecgx: Fix unaligned access issues Signed-off-by: Sylwester Nawrocki --- 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 #include #include +#include #include #include @@ -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 = 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; + if (addr - addr_inc != 2) err = s5k4ecgx_write(client, addr, val); else