On 2017-03-10 13:43, Valo, Kalle wrote:
"Valo, Kalle" <kvalo@xxxxxxxxxxxxxxxx> writes:
Erik Stromdahl <erik.stromdahl@xxxxxxxxx> writes:
sdio/mailbox HIF implementation.
Signed-off-by: Erik Stromdahl <erik.stromdahl@xxxxxxxxx>
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
