From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1cmeOD-0001Ca-Cs for ath10k@lists.infradead.org; Sat, 11 Mar 2017 10:36:47 +0000 Received: by mail-lf0-x241.google.com with SMTP id v2so8477041lfi.2 for ; Sat, 11 Mar 2017 02:36:24 -0800 (PST) Subject: Re: [RFC v4 06/21] ath10k: sdio support References: <1487693741-10042-1-git-send-email-erik.stromdahl@gmail.com> <1487693741-10042-7-git-send-email-erik.stromdahl@gmail.com> <8760jh2uy8.fsf@kamboji.qca.qualcomm.com> <871su52th2.fsf@kamboji.qca.qualcomm.com> <2528f323-6ec7-915c-d98b-615db04510e9@gmail.com> From: Erik Stromdahl Message-ID: <0d331e5f-7868-ed9a-7506-eecd1e8b9df6@gmail.com> Date: Sat, 11 Mar 2017 11:36:21 +0100 MIME-Version: 1.0 In-Reply-To: <2528f323-6ec7-915c-d98b-615db04510e9@gmail.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "ath10k" Errors-To: ath10k-bounces+kvalo=adurom.com@lists.infradead.org To: "Valo, Kalle" Cc: "linux-wireless@vger.kernel.org" , "ath10k@lists.infradead.org" On 2017-03-10 17:29, Erik Stromdahl wrote: > > > On 2017-03-10 13:43, Valo, Kalle wrote: >> "Valo, Kalle" writes: >> >>> Erik Stromdahl writes: >>> >>>> sdio/mailbox HIF implementation. >>>> >>>> Signed-off-by: Erik Stromdahl >>> >>> I'm looking at this more carefully now and noticed this: >>> >>>> +static int ath10k_sdio_bmi_credits(struct ath10k *ar) >>>> +{ >>>> + int ret; >>>> + u32 addr, *cmd_credits; >>>> + unsigned long timeout; >>>> + >>>> + cmd_credits = kzalloc(sizeof(*cmd_credits), GFP_KERNEL); >>>> + if (!cmd_credits) { >>>> + ret = -ENOMEM; >>>> + goto err; >>>> + } >>>> + >>>> + /* Read the counter register to get the command credits */ >>>> + addr = MBOX_COUNT_DEC_ADDRESS + ATH10K_HIF_MBOX_NUM_MAX * 4; >>>> + >>>> + timeout = jiffies + BMI_COMMUNICATION_TIMEOUT_HZ; >>>> + while (time_before(jiffies, timeout) && !*cmd_credits) { >>>> + /* Hit the credit counter with a 4-byte access, the first byte >>>> + * read will hit the counter and cause a decrement, while the >>>> + * remaining 3 bytes has no effect. The rationale behind this >>>> + * is to make all HIF accesses 4-byte aligned. >>>> + */ >>>> + ret = ath10k_sdio_read_write_sync(ar, addr, >>>> + (u8 *)cmd_credits, >>>> + sizeof(*cmd_credits), >>>> + HIF_RD_SYNC_BYTE_INC); >>>> + if (ret) { >>>> + ath10k_warn(ar, >>>> + "Unable to decrement the command credit count register: %d\n", >>>> + ret); >>>> + goto err_free; >>>> + } >>>> + >>>> + /* The counter is only 8 bits. >>>> + * Ignore anything in the upper 3 bytes >>>> + */ >>>> + *cmd_credits &= 0xFF; >>>> + } >>>> + >>>> + if (!*cmd_credits) { >>>> + ath10k_warn(ar, "bmi communication timeout\n"); >>>> + ret = -ETIMEDOUT; >>>> + goto err_free; >>>> + } >>>> + >>>> + return 0; >>>> +err_free: >>>> + kfree(cmd_credits); >>>> +err: >>>> + return ret; >>>> +} >>> >>> AFAICS we are leaking cmd_credits if there's no error. Or is the buffer >>> freed somewhere within the mmc stack or something? The reason why I ask >>> is that I saw the same pattern in multiple functions so I'm curious. >> >> Also I'm worried about endianness. We are reading from the device >> directly to an u32 variable and not converting the bytes. Is the MMC >> subsystem doing the conversion, I guess not? >> > You are right, there is definitely a memory leak (and there are similar problems > in a couple of other functions as well as you have pointed out). > > This was introduced in version 3 of the > RFC when I removed the bounce buffer from ath6kl. Instead I introduced a bunch of > local "bounce" buffers in order to make sure that the buffers passed to the sdio > subsystem is dma-able (malloc'ed buffer instead of stack allocated). > > Regarding endianess: That particular code construct is an artifact from ath6kl. > I am not sure it makes any sense to use a u32 in that particular case. > A u8 array is most likely more convenient. > > It is really nice you have found some time to review the patches! > > -- > Erik After doing some reviewing myself I have found some more endianess related issues. I will fix those (and the memory leaks) and submit v5 some time next week. _______________________________________________ ath10k mailing list ath10k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath10k