Re: [PATCH RESEND v4] mmc: core: Remove bounce buffer in mmc_send_cxd_data()

["kyungsik.lee" <kyungsik.lee@lge.com>]

Hello Chanho Min,


On 2012-08-02 오전 11:50, Chanho Min wrote:
>> -----Original Message-----
>> From: Kyungsik Lee [mailto:kyungsik.lee@lge.com]
>> Sent: Wednesday, August 01, 2012 10:06 AM
>> To: Chris Ball
>> Cc: Venkatraman S; Jaehoon Chung; raphael.andy.lee@gmail.com; linux-
>> mmc@vger.kernel.org; linux-kernel@vger.kernel.org; Kyungsik Lee
>> Subject: [PATCH RESEND v4] mmc: core: Remove bounce buffer in
>> mmc_send_cxd_data()
>>
>> It is expected that Extended CSD register(the size of this register
>> is larger than CID/CSD) will be referenced more frequently as more
>> fields have been added to Extended CSD and it seems that it is not
>> a good option to double the memory used.
>>
>> This patch is intended to avoid the use of bounce buffer for reading
>> Extended CSD register in mmc_send_cxd_data().
>>
>> Signed-off-by: Kyungsik Lee <kyungsik.lee@lge.com>
>> Signed-off-by: S, Venkatraman <svenkatr@ti.com>
>> ---
>> Changes in v2:
>> - Handling on-stack buffer if it's used in caller.
>>
>> Changes in v3:
>> - Remove unnecesary code.
>>
>> Changes in v4:
>> - Modify codes based-on S, Venkatraman's comments.
>> ---
>> drivers/mmc/core/mmc_ops.c |   54
> +++++++++++++++++++++++++++++++------------
>> -
>> 1 files changed, 38 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index 0ed2cc5..920a017 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -239,13 +239,19 @@ mmc_send_cxd_data(struct mmc_card *card, struct
> mmc_host
>> *host,
>> 	struct mmc_data data = {0};
>> 	struct scatterlist sg;
>> 	void *data_buf;
>> +	int is_on_stack;
>>
>> -	/* dma onto stack is unsafe/nonportable, but callers to this
>> -	 * routine normally provide temporary on-stack buffers ...
>> -	 */
>> -	data_buf = kmalloc(len, GFP_KERNEL);
>> -	if (data_buf == NULL)
>> -		return -ENOMEM;
>> +	is_on_stack = object_is_on_stack(buf);
>> +	if (is_on_stack) {
>> +
>> +		/* dma onto stack is unsafe/nonportable, but callers to this
>> +		 * routine normally provide temporary on-stack buffers ...
>> +		 */
>> +		data_buf = kmalloc(len, GFP_KERNEL);
>> +		if (data_buf == NULL)
>> +			return -ENOMEM;
>> +	} else
>> +		data_buf = buf;
>>
>> 	mrq.cmd = &cmd;
>> 	mrq.data = &data;
>> @@ -280,8 +286,10 @@ mmc_send_cxd_data(struct mmc_card *card, struct
> mmc_host
>> *host,
>>
>> 	mmc_wait_for_req(host, &mrq);
>>
>> -	memcpy(buf, data_buf, len);
>> -	kfree(data_buf);
>> +	if (is_on_stack) {
>> +		memcpy(buf, data_buf, len);
>> +		kfree(data_buf);
>> +	}
>>
>> 	if (cmd.error)
>> 		return cmd.error;
>> @@ -294,24 +302,32 @@ mmc_send_cxd_data(struct mmc_card *card, struct
> mmc_host
>> *host,
>> int mmc_send_csd(struct mmc_card *card, u32 *csd)
>> {
>> 	int ret, i;
>> +	u32 *csd_tmp;
>>
>> 	if (!mmc_host_is_spi(card->host))
>> 		return mmc_send_cxd_native(card->host, card->rca << 16,
>> 				csd, MMC_SEND_CSD);
>>
>> -	ret = mmc_send_cxd_data(card, card->host, MMC_SEND_CSD, csd, 16);
>> +	csd_tmp = kmalloc(16, GFP_KERNEL);
>> +	if (!csd_tmp)
>> +		return -ENOMEM;
>> +
>> +	ret = mmc_send_cxd_data(card, card->host, MMC_SEND_CSD, csd_tmp,
> 16);
>> 	if (ret)
>> -		return ret;
>> +		goto err;
>>
>> 	for (i = 0;i < 4;i++)
>> -		csd[i] = be32_to_cpu(csd[i]);
>> +		csd[i] = be32_to_cpu(csd_tmp[i]);
>>
>> -	return 0;
>> +err:
>> +	kfree(csd_tmp);
>> +	return ret;
>> }
> If we can handle for the on-stack buffer in mmc_send_cxd_data, why do we
> need callers's modification as bellows?
As you comment above, on-stack buffer can be handled with no better 
performance gain.
In case of both mmc_send_cid() and mmc_send_csd(), on-stack buffers have 
not been allocated
in the upper callers to the two functions(you may check it in the upper 
callers).
And you will find out the reason why such a modification below is needed 
in the mail thread
(Reply to S, Venkatraman's comment).

Thanks
Kyungsik Lee
>> int mmc_send_cid(struct mmc_host *host, u32 *cid)
>> {
>> 	int ret, i;
>> +	u32 *cid_tmp;
>>
>> 	if (!mmc_host_is_spi(host)) {
>> 		if (!host->card)
>> @@ -320,14 +336,20 @@ int mmc_send_cid(struct mmc_host *host, u32 *cid)
>> 				cid, MMC_SEND_CID);
>> 	}
>>
>> -	ret = mmc_send_cxd_data(NULL, host, MMC_SEND_CID, cid, 16);
>> +	cid_tmp = kmalloc(16, GFP_KERNEL);
>> +	if (!cid_tmp)
>> +		return -ENOMEM;
>> +
>> +	ret = mmc_send_cxd_data(NULL, host, MMC_SEND_CID, cid_tmp, 16);
>> 	if (ret)
>> -		return ret;
>> +		goto err;
>>
>> 	for (i = 0;i < 4;i++)
>> -		cid[i] = be32_to_cpu(cid[i]);
>> +		cid[i] = be32_to_cpu(cid_tmp[i]);
>>
>> -	return 0;
>> +err:
>> +	kfree(cid_tmp);
>> +	return ret;
>> }
>>
>> int mmc_send_ext_csd(struct mmc_card *card, u8 *ext_csd)
>> --
>> 1.7.0.4
> Thanks
> Chanho Min
>
>