Hi,
J Freyensee wrote:
> On 08/12/2011 04:14 AM, Jaehoon Chung wrote:
>> Hi mailing.
>>
>> This RFC patch is supported background operation(BKOPS).
>> And if you want to test this patch, must apply "[PATCH v3] mmc:
>> support HPI send command"
>>
>> This patch is based on Hanumath Prasad's patch "mmc: enable background
>> operations for emmc4.41 with HPI support"
>> Hanumath's patch is implemented before applied per forlin's patch "use
>> nonblock mmc request...".
>> This patch is based on 3.1.0-rc1 in mmc-next.
>
> I'm a little confused by this statement. Was this patch done before Per
> Forlin's work, or is this patch the implementation of the infrastructure
> Per Forlin worked on to do non-blocking requests to the host controller?
>
This feature(BKOPS) is defined in eMMC4.41 spec.(differ with Per Forlins's non-blocking patch)
Hanumath Prasad's patch is sent before Per Forlin's patch.
so need to rebase for applied patch.
>>
>> Background operations is run when set the URGENT_BKOPS in response.
>>
>> if set the URGENT_BKOPS in response, we can notify that card need the
>> BKOPS.
>> (URGENT_BKOPS is used in eMMC4.41 spec, but in eMMC4.5 changed to
>> EXCEPTION_EVENT bit.
>> maybe, we need to change this point).
>>
>> And all request is done, then run background operation.
>> if request read/write operation when running BKOPS, issue HPI interrupt
>>
>> This patch is just RFC patch (not to merge), because i want to use
>> BKOPS in userspace.
>> (using ioctl).
>>
>> I want to get mailing's review for this patch.
>>
>>
>> Signed-off-by: Jaehoon Chung<jh80.chung@xxxxxxxxxxx>
>> Signed-off-by: Kyungmin Park<kyungmin.park@xxxxxxxxxxx>
>> CC: Hanumath Prasad<hanumath.prasad@xxxxxxxxxxxxxx>
>>
>> ---
>> drivers/mmc/card/block.c | 4 ++++
>> drivers/mmc/card/queue.c | 10 ++++++++++
>> drivers/mmc/core/core.c | 35 +++++++++++++++++++++++++++++++++++
>> drivers/mmc/core/mmc.c | 28 ++++++++++++++++++++++++++++
>> drivers/mmc/core/mmc_ops.c | 3 +++
>> include/linux/mmc/card.h | 11 +++++++++++
>> include/linux/mmc/core.h | 1 +
>> include/linux/mmc/mmc.h | 4 ++++
>> 8 files changed, 96 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index 1ff5486..ff72c4a 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -1078,6 +1078,10 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
>> *mq, struct request *rqc)
>> switch (status) {
>> case MMC_BLK_SUCCESS:
>> case MMC_BLK_PARTIAL:
>> + if (mmc_card_mmc(card)&&
>> + (brq->cmd.resp[0]& R1_URGENT_BKOPS)) {
>> + mmc_card_set_need_bkops(card);
>> + }
>> /*
>> * A block was successfully transferred.
>> */
>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
>> index 45fb362..52b1293 100644
>> --- a/drivers/mmc/card/queue.c
>> +++ b/drivers/mmc/card/queue.c
>> @@ -46,6 +46,8 @@ static int mmc_queue_thread(void *d)
>> {
>> struct mmc_queue *mq = d;
>> struct request_queue *q = mq->queue;
>> + struct mmc_card *card = mq->card;
>> + unsigned long flags;
>>
>> current->flags |= PF_MEMALLOC;
>>
>> @@ -61,6 +63,13 @@ static int mmc_queue_thread(void *d)
>> spin_unlock_irq(q->queue_lock);
>>
>> if (req || mq->mqrq_prev->req) {
>> + if (mmc_card_doing_bkops(card)) {
>> + mmc_interrupt_hpi(card);
>> + spin_lock_irqsave(&card->host->lock, flags);
>> + mmc_card_clr_doing_bkops(card);
>> + spin_unlock_irqrestore(&card->host->lock,
>> + flags);
>> + }
>> set_current_state(TASK_RUNNING);
>> mq->issue_fn(mq, req);
>> } else {
>> @@ -68,6 +77,7 @@ static int mmc_queue_thread(void *d)
>> set_current_state(TASK_RUNNING);
>> break;
>> }
>> + mmc_start_bkops(mq->card);
>> up(&mq->thread_sem);
>> schedule();
>> down(&mq->thread_sem);
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 7c1ab06..b6de0e5 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -347,6 +347,41 @@ int mmc_wait_for_cmd(struct mmc_host *host,
>> struct mmc_command *cmd, int retries
>>
>> EXPORT_SYMBOL(mmc_wait_for_cmd);
>>
>> +/* Start background operation */
>> +void mmc_start_bkops(struct mmc_card *card)
>
> Is it possible to follow the kernel documentation standard for comment
> function headers (I believe Randy Dunlap has given links to this in the
> past)? You can see in this patch that after this function the next
> function is using a function comment header per kernel guidelines.
You're right.
But this patch is the RFC patch (i mentioned this patch is not to merge).
So i didn't add the comment for this function.
If this patch should be merge, i will add the comment for function.
(at next time, if i send the other RFC patch, i will also add the comment)
>
>> +{
>> + int err;
>> + unsigned long flags;
>> +
>> + BUG_ON(!card);
>> +
>> + if (!card->ext_csd.bkops_en) {
>> + printk(KERN_INFO "Didn't set BKOPS enable bit!\n");
>
> I know that if new drivers are added to the kernel, maintainers will
> reject the work if it's using printk()'s. If this code is getting new
> functions, is it a good idea to start using the more modern, accepted
> coding functions like pr_info()?
No problem..using pr_info or pr_debug..
>
>> + return;
>> + }
>> +
>> + if (mmc_card_doing_bkops(card) ||
>> + !mmc_card_need_bkops(card)) {
>
> This code wouldn't pass the checkpatch.pl tool; I've been burned by the
> Linux community of having brackets around a single line of code.
I used the checkpatch.pl tool..but i didn't find message about this line.
If you point out the brackets {}, i will removed that and make a single line.
>
>> + return;
>> + }
>> +
>> + mmc_claim_host(card->host);
>> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> + EXT_CSD_BKOPS_START, 1, 0);
>> + if (err) {
>> + mmc_card_clr_need_bkops(card);
>> + goto out;
>> + }
>> +
>> + spin_lock_irqsave(&card->host->lock, flags);
>> + mmc_card_clr_need_bkops(card);
>> + mmc_card_set_doing_bkops(card);
>> + spin_unlock_irqrestore(&card->host->lock, flags);
>> +out:
>> + mmc_release_host(card->host);
>> +}
>> +EXPORT_SYMBOL(mmc_start_bkops);
>> +
>> /**
>> * mmc_interrupt_hpi - Issue for High priority Interrupt
>> * @card: the MMC card associated with the HPI transfer
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index ef10bfd..0372414 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -421,6 +421,17 @@ static int mmc_read_ext_csd(struct mmc_card
>> *card, u8 *ext_csd)
>> ext_csd[EXT_CSD_OUT_OF_INTERRUPT_TIME] * 10;
>> }
>>
>> + /*
>> + * check whether the eMMC card support BKOPS.
>> + * if set BKOPS_SUPPORT bit,
>> + * BKOPS_STATUS, BKOPS_EN,,BKOPS_START and
>> + * URGENT_BKOPS are supported.(default)
>> + */
>> + if (ext_csd[EXT_CSD_BKOPS_SUPPORT]& 0x1) {
>
> That is kind of an ugly if() statement; a bit further down I explain my
> reasons for making if() statements like this more readable.
I didn't understand this comment...how do you change this statement?
>
>> + card->ext_csd.bkops = 1;
>> + card->ext_csd.bkops_en = ext_csd[EXT_CSD_BKOPS_EN];
>> + }
>> +
>> card->ext_csd.rel_param = ext_csd[EXT_CSD_WR_REL_PARAM];
>> }
>>
>> @@ -762,6 +773,23 @@ static int mmc_init_card(struct mmc_host *host,
>> u32 ocr,
>> }
>>
>> /*
>> + * Enable HPI feature (if supported)
>> + */
>> + if (card->ext_csd.hpi) {
>
> I know some people prefer doing things like
>
> A.'if (x)'
> instead of
> B.'if (x != NULL)
>
> because A. is supposed to be some type of 'expert way' of doing things.
> However, B. is a whole lot more readable and easier for people to
> decipher precisely what is going on, especially newer people that may
> not be as familiar with this part of the Linux kernel as others. Just
> looking at this patch, I can't tell if 'card->ext_csd.hpi' is supposed
> to be a number value or a pointer. And if you use Linus's tool 'sparse'
> to check your kernel code before submitting, there is a difference
> between statements like 'if (x == 0)' and 'if (x == NULL)', even though
> they could evaluate to the same result in this if() statement.
>
> So I suggest adding the equality or inequality sign to this if() as well
> as any other if() to make the code a bit easier to understand.
Right..i will modify this point..
Thanks for your comments..
Any other opinion about this feature (BKOPS)..??
Regards,
Jaehoon Chung
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
