Re: [PATCH v6 3/9] mmc: core: Add open-ended Ext memory addressing

[Christian Loehle <christian.loehle@arm.com>]

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.