Re: [RFC PATCH] mmc: support background operation

[Jaehoon Chung <jh80.chung@samsung.com>]

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@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> CC: Hanumath Prasad<hanumath.prasad@stericsson.com>
>>
>> ---
>>   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