"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?
--
Kalle Valo
