On 9/5/24 07:12, Avri Altman wrote:
> Thanks for having a look.
>
>>>
>>> + if (mrq->cmd && mrq->cmd->has_ext_addr)
>>> + mmc_send_ext_addr(host, mrq->cmd->ext_addr);
>>
>> We should check that this was actually successful, right?
> Actually no, as errors in CMD22 are being carried in the subsequent command.
I see, sorry for the noise.
>
>>
>>> +
>>> init_completion(&mrq->cmd_completion);
>>>
>>> mmc_retune_hold(host);
>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h index
>>> f0ac2e469b32..41c21c216584 100644
>>> --- a/include/linux/mmc/core.h
>>> +++ b/include/linux/mmc/core.h
>>> @@ -76,6 +76,11 @@ struct mmc_command {
>>> */
>>> #define mmc_cmd_type(cmd) ((cmd)->flags & MMC_CMD_MASK)
>>>
>>> + /* for SDUC */
>>> + u8 has_ext_addr;
>>> + u8 ext_addr;
>>> + u16 reserved;
>>
>> Is there a reason for has_ext_addr being u8?
> Theoretically a single bit suffices, and since ext_addr uses only 6 bits, I had that bit to spare in ext_addr,
> but I see no reason to be cheap here - see the reserved u16.
>
>> What's the reserved for?
> Not to break the packed 4bytes alignment of mmc_command.
FWIW, that's what it looks like so I was a bit surprised:
pahole -C mmc_command vmlinux
struct mmc_command {
u32 opcode; /* 0 4 */
u32 arg; /* 4 4 */
u32 resp[4]; /* 8 16 */
unsigned int flags; /* 24 4 */
bool has_ext_addr; /* 28 1 */
u8 ext_addr; /* 29 1 */
u16 reserved; /* 30 2 */
unsigned int retries; /* 32 4 */
int error; /* 36 4 */
unsigned int busy_timeout; /* 40 4 */
/* XXX 4 bytes hole, try to pack */
struct mmc_data * data; /* 48 8 */
struct mmc_request * mrq; /* 56 8 */
/* size: 64, cachelines: 1, members: 12 */
/* sum members: 60, holes: 1, sum holes: 4 */
};
has_ext_addr also should be equivalent to:
mmc_card_ult_capacity(card) && opcode in [17,18,24,25,32,33]
but the flag is also fine.
I'm a bit hesitant to put my R-b but it all looks plausible
to me FWIW.
