| 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: <20240712094836.2395132-1-wangshuaijie@awinic.com>
Date: Fri, 12 Jul 2024 09:48:36 +0000
From: wangshuaijie@...nic.com
To: krzk@...nel.org
Cc: conor+dt@...nel.org,
devicetree@...r.kernel.org,
dmitry.torokhov@...il.com,
jeff@...undy.com,
kangjiajun@...nic.com,
krzk+dt@...nel.org,
linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org,
liweilei@...nic.com,
robh@...nel.org,
wangshuaijie@...nic.com
Subject: Re: [PATCH V2 4/5] Add aw963xx series related interfaces to the aw_sar driver
Hi Krzysztof,
On Wed, 5 Jun 2024 12:29:06, krzk@...nel.org wrote:
>On 05/06/2024 11:11, wangshuaijie@...nic.com wrote:
>> From: shuaijie wang <wangshuaijie@...nic.com>
>>
>> Signed-off-by: shuaijie wang <wangshuaijie@...nic.com>
>> | Reported-by: kernel test robot <lkp@...el.com>
>> | Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
>> | Reported-by: Dan Carpenter <error27@...il.com>
>> ---
>> drivers/input/misc/aw_sar/aw963xx/aw963xx.c | 974 ++++++++++++++++++++
>> drivers/input/misc/aw_sar/aw963xx/aw963xx.h | 753 +++++++++++++++
>> 2 files changed, 1727 insertions(+)
>> create mode 100644 drivers/input/misc/aw_sar/aw963xx/aw963xx.c
>> create mode 100644 drivers/input/misc/aw_sar/aw963xx/aw963xx.h
>>
>> diff --git a/drivers/input/misc/aw_sar/aw963xx/aw963xx.c b/drivers/input/misc/aw_sar/aw963xx/aw963xx.c
>> new file mode 100644
>> index 000000000000..7ce40174a089
>> --- /dev/null
>> +++ b/drivers/input/misc/aw_sar/aw963xx/aw963xx.c
>> @@ -0,0 +1,974 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AWINIC sar sensor driver (aw963xx)
>> + *
>> + * Author: Shuaijie Wang<wangshuaijie@...nic.com>
>> + *
>> + * Copyright (c) 2024 awinic Technology CO., LTD
>> + */
>> +#include "aw963xx.h"
>> +#include "../aw_sar.h"
>> +
>> +#define AW963XX_I2C_NAME "aw963xx_sar"
>> +
>> +static void aw963xx_set_cs_as_irq(struct aw_sar *p_sar, int flag);
>> +static void aw963xx_get_ref_ch_enable(struct aw_sar *p_sar);
>> +
>> +static int32_t aw963xx_read_init_over_irq(void *load_bin_para)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
>> + uint32_t cnt = 1000;
>> + uint32_t reg;
>> + int32_t ret;
>> +
>> + while (cnt--) {
>> + ret = aw_sar_i2c_read(p_sar->i2c, REG_IRQSRC, ®);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "i2c error %d", ret);
>> + return ret;
>> + }
>> + if ((reg & 0x01) == 0x01) {
>> + aw_sar_i2c_read(p_sar->i2c, REG_FWVER, ®);
>> + return 0;
>> + }
>> + mdelay(1);
>> + }
>> +
>> + aw_sar_i2c_read(p_sar->i2c, REG_FWVER, ®);
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static void aw963xx_convert_little_endian_2_big_endian(struct aw_bin *aw_bin)
>> +{
>> + uint32_t start_index = aw_bin->header_info[0].valid_data_addr;
>> + uint32_t fw_len = aw_bin->header_info[0].reg_num;
>> + uint32_t uints = fw_len / AW963XX_SRAM_UPDATE_ONE_UINT_SIZE;
>> + uint8_t tmp1;
>> + uint8_t tmp2;
>> + uint8_t tmp3;
>> + uint8_t tmp4;
>> + int i;
>> +
>> + for (i = 0; i < uints; i++) {
>> + tmp1 = aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 3];
>> + tmp2 = aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 2];
>> + tmp3 = aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 1];
>> + tmp4 = aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE];
>> + aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE] = tmp1;
>> + aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 1] = tmp2;
>> + aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 2] = tmp3;
>> + aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 3] = tmp4;
>> + }
>> +}
>> +
>> +/**
>> + * @aw963xx_sram_fill_not_wrote_area()
>> + * |----------------code ram-----------------|
>> + * 0x2000 0x4fff
>> + * |--- app wrote here ---|--fill with 0xff--|
>> + *
>> + * if the size of app is less than the size of code ram, the rest of the
>> + * ram is filled with 0xff.
>> + * @load_bin_para
>> + * @offset the rear addr of app
>> + * @return int32_t
>> + */
>> +static int32_t aw963xx_sram_fill_not_wrote_area(void *load_bin_para, uint32_t offset)
>> +{
>> + uint32_t last_pack_len = (AW963XX_SRAM_END_ADDR - offset) %
>> + AW963XX_SRAM_UPDATE_ONE_PACK_SIZE;
>> + uint32_t pack_cnt = last_pack_len == 0 ?
>> + ((AW963XX_SRAM_END_ADDR - offset) / AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) :
>> + ((AW963XX_SRAM_END_ADDR - offset) / AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) + 1;
>> + uint8_t buf[AW963XX_SRAM_UPDATE_ONE_PACK_SIZE + 2] = { 0 };
>> + struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
>> + uint32_t download_addr_with_ofst;
>> + uint8_t *r_buf;
>> + int32_t ret;
>> + uint32_t i;
>> +
>> + r_buf = devm_kzalloc(p_sar->dev, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE, GFP_KERNEL);
>> + if (!r_buf)
>> + return -ENOMEM;
>> +
>> + memset(buf, 0xff, sizeof(buf));
>> + for (i = 0; i < pack_cnt; i++) {
>> + memset(r_buf, 0, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
>> + download_addr_with_ofst = offset + i * AW963XX_SRAM_UPDATE_ONE_PACK_SIZE;
>> + buf[0] = (uint8_t)(download_addr_with_ofst >> OFFSET_BIT_8);
>> + buf[1] = (uint8_t)(download_addr_with_ofst);
>> + if (i != (pack_cnt - 1)) {
>> + ret = aw_sar_i2c_write_seq(p_sar->i2c, buf,
>> + AW963XX_SRAM_UPDATE_ONE_PACK_SIZE + 2);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, write_seq error!", i);
>> + devm_kfree(p_sar->dev, r_buf);
>> + return ret;
>> + }
>> + ret = aw_sar_i2c_read_seq(p_sar->i2c, buf, 2, r_buf,
>> + AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, read_seq error!", i);
>> + devm_kfree(p_sar->dev, r_buf);
>> + return ret;
>> + }
>> + if (memcmp(&buf[2], r_buf, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) != 0) {
>> + dev_err(p_sar->dev, "read is not equal to write ");
>> + devm_kfree(p_sar->dev, r_buf);
>> + return -EINVAL;
>> + }
>> + } else {
>> + ret = aw_sar_i2c_write_seq(p_sar->i2c, buf, last_pack_len + 2);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, write_seq error!", i);
>> + devm_kfree(p_sar->dev, r_buf);
>> + return ret;
>> + }
>> + ret = aw_sar_i2c_read_seq(p_sar->i2c, buf, 2, r_buf, last_pack_len);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, read_seq error!", i);
>> + devm_kfree(p_sar->dev, r_buf);
>> + return ret;
>> + }
>> + if (memcmp(&buf[2], r_buf, last_pack_len) != 0) {
>> + dev_err(p_sar->dev, "read is not equal to write ");
>> + devm_kfree(p_sar->dev, r_buf);
>> + return -EINVAL;
>> + }
>> + }
>> + }
>> +
>> + devm_kfree(p_sar->dev, r_buf);
>> +
>> + return 0;
>> +}
>> +
>> +static int32_t aw963xx_sram_data_write(struct aw_bin *aw_bin, void *load_bin_para)
>> +{
>> + uint8_t buf[AW963XX_SRAM_UPDATE_ONE_PACK_SIZE + 2] = { 0 };
>> + uint32_t start_index = aw_bin->header_info[0].valid_data_addr;
>> + uint32_t fw_bin_version = aw_bin->header_info[0].app_version;
>> + uint32_t download_addr = AW963XX_RAM_START_ADDR;
>> + uint32_t fw_len = aw_bin->header_info[0].reg_num;
>> + uint32_t last_pack_len = fw_len % AW963XX_SRAM_UPDATE_ONE_PACK_SIZE;
>> + struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
>> + uint32_t download_addr_with_ofst = 0;
>> + uint32_t pack_cnt;
>> + uint8_t *r_buf;
>> + int32_t ret = -EINVAL;
>> + uint32_t i;
>> +
>> + r_buf = devm_kzalloc(p_sar->dev, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE, GFP_KERNEL);
>> + if (!r_buf)
>> + return -ENOMEM;
>> +
>> + pack_cnt = ((fw_len % AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) == 0) ?
>> + (fw_len / AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) :
>> + (fw_len / AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) + 1;
>> +
>> + dev_info(p_sar->dev, "fw_bin_version = 0x%x", fw_bin_version);
>> + for (i = 0; i < pack_cnt; i++) {
>> + memset(r_buf, 0, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
>> + download_addr_with_ofst = download_addr + i * AW963XX_SRAM_UPDATE_ONE_PACK_SIZE;
>> + buf[0] = (uint8_t)(download_addr_with_ofst >> OFFSET_BIT_8);
>> + buf[1] = (uint8_t)(download_addr_with_ofst);
>> + if (i != (pack_cnt - 1)) {
>> + memcpy(&buf[2], &aw_bin->info.data[start_index +
>> + i * AW963XX_SRAM_UPDATE_ONE_PACK_SIZE],
>> + AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
>> + ret = aw_sar_i2c_write_seq(p_sar->i2c, buf,
>> + AW963XX_SRAM_UPDATE_ONE_PACK_SIZE + 2);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, write_seq error!", i);
>> + goto err_out;
>> + }
>> + ret = aw_sar_i2c_read_seq(p_sar->i2c, buf, 2, r_buf,
>> + AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, read_seq error!", i);
>> + goto err_out;
>> + }
>> + if (memcmp(&buf[2], r_buf, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) != 0) {
>> + dev_err(p_sar->dev, "read is not equal to write ");
>> + ret = -EIO;
>> + goto err_out;
>> + }
>> + } else { // last pack process
>> + memcpy(&buf[2], &aw_bin->info.data[start_index +
>> + i * AW963XX_SRAM_UPDATE_ONE_PACK_SIZE], last_pack_len);
>> + ret = aw_sar_i2c_write_seq(p_sar->i2c, buf, last_pack_len + 2);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, write_seq error!", i);
>> + goto err_out;
>> + }
>> + ret = aw_sar_i2c_read_seq(p_sar->i2c, buf, 2, r_buf, last_pack_len);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, read_seq error!", i);
>> + goto err_out;
>> + }
>> + if (memcmp(&buf[2], r_buf, last_pack_len) != 0) {
>> + dev_err(p_sar->dev, "read is not equal to write ");
>> + ret = -EIO;
>> + goto err_out;
>> + }
>> + /* fill 0xff in the area that not worte. */
>> + ret = aw963xx_sram_fill_not_wrote_area(load_bin_para,
>> + download_addr_with_ofst + last_pack_len);
>> + if (ret != 0) {
>> + dev_err(p_sar->dev, "cnt%d, sram_fill_not_wrote_area error!", i);
>> + goto err_out;
>> + }
>> + }
>> + }
>> +
>> +err_out:
>> + devm_kfree(p_sar->dev, r_buf);
>
>Why do you use managed interface?
>
The patch for v3 will fix these issues.
>> +
>> + return ret;
>> +}
>> +
>> +static int32_t aw963xx_update_firmware(struct aw_bin *aw_bin, void *load_bin_para)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
>> + struct aw963xx *aw963xx = (struct aw963xx *)p_sar->priv_data;
>> + struct i2c_client *i2c = p_sar->i2c;
>> + int32_t ret;
>> +
>> + if (aw963xx->start_mode == AW963XX_ROM_MODE) {
>> + dev_info(p_sar->dev, "no need to update fw.");
>> + return 0;
>> + }
>> +
>> + //step1: close coderam shutdown mode
>
>Plaese fix your style to be consistent. There is a space after //.
>Always, so fix all your patches.
>
The patch for v3 will fix these issues.
>
>
>...
>
>> +
>> +int32_t aw963xx_check_chipid(void *data)
>> +{
>> + struct aw_sar *p_sar = (struct aw_sar *)data;
>> + uint32_t reg_val;
>> + int32_t ret;
>> +
>> + if (!p_sar)
>> + return -EINVAL;
>> +
>> + ret = aw_sar_i2c_read(p_sar->i2c, REG_CHIP_ID0, ®_val);
>> + if (ret < 0) {
>> + dev_err(p_sar->dev, "read CHIP ID failed: %d", ret);
>> + return ret;
>> + }
>> +
>> + switch (reg_val) {
>> + case AW96303_CHIP_ID:
>> + dev_info(p_sar->dev, "aw96303 detected, 0x%04x", reg_val);
>
>Your driver is quite noisy. Reduce the severity of informational
>messages, because driver should be quiet on success.
>
>I don't understand why even having dev_info in 5 places instead of one
>place.
>
The patch for v3 will fix these issues.
>> + memcpy(p_sar->chip_name, AW96303, 8);
>> + ret = 0;
>> + break;
>> + case AW96305_CHIP_ID:
>> + dev_info(p_sar->dev, "aw96305 detected, 0x%04x", reg_val);
>> + memcpy(p_sar->chip_name, AW96305, 8);
>> + ret = 0;
>> + break;
>> + case AW96305BFOR_CHIP_ID:
>> + dev_info(p_sar->dev, "aw96305bfor detected, 0x%04x", reg_val);
>> + memcpy(p_sar->chip_name, AW96305BFOR, 8);
>> + ret = 0;
>> + break;
>> + case AW96308_CHIP_ID:
>> + dev_info(p_sar->dev, "aw96308 detected, 0x%04x", reg_val);
>> + memcpy(p_sar->chip_name, AW96308, 8);
>> + ret = 0;
>> + break;
>> + case AW96310_CHIP_ID:
>> + dev_info(p_sar->dev, "aw96310 detected, 0x%04x", reg_val);
>> + memcpy(p_sar->chip_name, AW96310, 8);
>
>No, all these memcpy are just silly. You later compare strings instead
>of comparing the detected chip id (integer).
>
The register configuration file contains a chip name string, which needs
to be compared with chip_name during the validation of the register
configuration file.
>> + ret = 0;
>> + break;
>> + default:
>> + dev_info(p_sar->dev, "chip id error, 0x%04x", reg_val);
>> + ret = -EIO;
>
>Fix your style, just one space after =. This applies in multiple places.
>
The patch for v3 will fix these issues.
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>
>There are so many trivial issues in this driver that I think you should
>start from huge cleanup from all these trivialities before sending to
>review. You try to upstream a downstream, poor quality code. This is
>always a pain. Instead you should take moderately recent driver, which
>passed review, as a template and work on top of it with Linux coding
>uniformed style.
>
>Best regards,
>Krzysztof
Kind regards,
Wang Shuaijie
Powered by blists - more mailing lists