On 2017-03-15 13:09, Kalle Valo wrote:
Erik Stromdahl <erik.stromd...@gmail.com> writes:
On 2017-03-10 13:43, Valo, Kalle wrote:
"Valo, Kalle" <kv...@qca.qualcomm.com> writes:
Erik Stromdahl <erik.stromd...@gmail.com> writes:
sdio/mailbox HIF implementation.
Signed-off-by: Erik Stromdahl <erik.stromd...@gmail.com>
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.
There seems to be same pattern for reading four bytes, what if we should
add a helper for that? Something like ath10k_sdio_read32()? It could
handle the kmalloc and switch endianess also.
But please don't make any chances to sdio.c for the moment, let me
submit v5 first.
I notice that you have submitted v5 (and v6) of the sdio patches. Great!
I assume you will add them to master-pending soon so I will try out
everything (usb too) as soon as they are on master-pending.
