On Wed, 2018-10-03 at 19:20 +0800, yhchuang@xxxxxxxxxxx wrote:
>
> + do {
> + cnt--;
> + value = rtw_read8(rtwdev, offset);
> + value &= cmd->mask;
> + if (value == (cmd->value & cmd->mask))
> + return 0;
> + if (cnt == 0) {
> + if (rtw_hci_type(rtwdev) == RTW_HCI_TYPE_PCIE &&
> + flag == 0) {
> + value = rtw_read8(rtwdev, REG_SYS_PW_CTRL);
> + value |= BIT(3);
> + rtw_write8(rtwdev, REG_SYS_PW_CTRL, value);
> + value &= ~BIT(3);
> + rtw_write8(rtwdev, REG_SYS_PW_CTRL, value);
It stands to reason this might need some sort of udelay() inbetween
togging the bit?
> + value = rtw_read8(rtwdev, offset);
> + value &= ~cur_cmd->mask;
> + value |= (cur_cmd->value & cur_cmd->mask);
> + rtw_write8(rtwdev, offset, value);
You might want to have a helper function/inline for this type of
sequence? Hmm, maybe I'm confusing it - now I can't find where I thought
it was also used elsewhere.
> +static bool check_firmware_size(const u8 *data, u32 size)
> +{
> + u32 dmem_size;
> + u32 imem_size;
> + u32 emem_size;
> + u32 real_size;
> +
> + dmem_size = le32_to_cpu(*((__le32 *)(data + FW_HDR_DMEM_SIZE)));
> + imem_size = le32_to_cpu(*((__le32 *)(data + FW_HDR_IMEM_SIZE)));
> + emem_size = ((*(data + FW_HDR_MEM_USAGE)) & BIT(4)) ?
> + le32_to_cpu(*((__le32 *)(data + FW_HDR_EMEM_SIZE))) : 0;
This dereferencing data as __le32 seems very problematic due to
alignment concerns?
> +static bool ltecoex_read_reg(struct rtw_dev *rtwdev, u16 offset, u32 *val)
> +{
> + u32 cnt = 10000;
> +
> + while ((rtw_read8(rtwdev, LTECOEX_ACCESS_CTRL + 3) & BIT(5)) == 0) {
> + if (cnt-- == 0)
> + return false;
> + udelay(50);
> + }
You have this sort of loop a lot it seems - perhaps make a macro out of
it?
> + buf = kmalloc(size, GFP_KERNEL);
> + memcpy(buf, data, size);
kmemdup, but you need an error check too
> + while (rtw_read32(rtwdev, REG_DDMA_CH0CTRL) & BIT_DDMACH0_OWN) {
> + cnt--;
> + if (cnt == 0)
> + return -EBUSY;
> + }
Here's another one of the loops, but it probably needs a udelay()?
> +static int iddma_download_firmware(struct rtw_dev *rtwdev, u32 src, u32 dst,
> + u32 len, u8 first)
> +{
> + u32 cnt = DDMA_POLLING_COUNT;
> + u32 ch0_ctrl = BIT_DDMACH0_CHKSUM_EN | BIT_DDMACH0_OWN;
> +
> + while (rtw_read32(rtwdev, REG_DDMA_CH0CTRL) & BIT_DDMACH0_OWN) {
> + cnt--;
> + if (cnt == 0)
> + return -EBUSY;
> + }
and here
> +static void update_firmware_info(struct rtw_dev *rtwdev, const u8 *data)
> +{
> + struct rtw_fw_state *fw = &rtwdev->fw;
> +
> + fw->h2c_version =
> + le16_to_cpu(*((__le16 *)(data + FW_HDR_H2C_FMT_VER)));
> + fw->version =
> + le16_to_cpu(*((__le16 *)(data + FW_HDR_VERSION)));
more potential alignment issues
> +start_download_firmware(struct rtw_dev *rtwdev, const u8 *data, u32 size)
> +{
> + const u8 *cur_fw;
> + u16 val;
> + u16 fw_ctrl;
> + u32 imem_size;
> + u32 dmem_size;
> + u32 emem_size;
> + u32 addr;
> + int ret;
> +
> + dmem_size = le32_to_cpu(*((__le32 *)(data + FW_HDR_DMEM_SIZE)));
> + imem_size = le32_to_cpu(*((__le32 *)(data + FW_HDR_IMEM_SIZE)));
> + emem_size = ((*(data + FW_HDR_MEM_USAGE)) & BIT(4)) ?
> + le32_to_cpu(*((__le32 *)(data + FW_HDR_EMEM_SIZE))) : 0;
same here
> + cnt = 1000;
> + while (rtw_read8(rtwdev, REG_AUTO_LLT_V1) & BIT_AUTO_INIT_LLT_V1)
> + if (cnt-- == 0)
> + return -EBUSY;
missing udelay again?
johannes
