Re: [RFC v4 06/21] ath10k: sdio support

From: Erik Stromdahl <erik.stromdahl@gmail.com>
To: "Valo, Kalle" <kvalo@qca.qualcomm.com>
Cc: "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
	"ath10k@lists.infradead.org" <ath10k@lists.infradead.org>
Subject: Re: [RFC v4 06/21] ath10k: sdio support
Date: Sat, 11 Mar 2017 11:36:21 +0100	[thread overview]
Message-ID: <0d331e5f-7868-ed9a-7506-eecd1e8b9df6@gmail.com> (raw)
In-Reply-To: <2528f323-6ec7-915c-d98b-615db04510e9@gmail.com>

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