Hi Andrew.
Andrew Morton wrote:
> On Tue, 30 Nov 2010 17:42:13 +0900
> Jaehoon Chung <jh80.chung@xxxxxxxxxxx> wrote:
>
>> This patch is mmcoops.
>>
>> When Kernel panic or oops, write panic log to circular buffer in eMMC.
>> then after reset, we can see the log in MMC.
>>
>> So we need change mmc_wait_for_req().
>> because if we used schedule(), we can find atomic schedule warning.
>> That is not our needs log. so i used delay.
>> i want to know how about __mmc_wait_for_req()
>>
>> If any question, let me know.
>>
>> ...
>>
>> /**
>> - * mmc_wait_for_req - start a request and wait for completion
>> + * __mmc_wait_for_req - start a request and wait for completion
>> * @host: MMC host to start command
>> * @mrq: MMC request to start
>> + * @panic: kernel panic/oops or not
>> *
>> * Start a new MMC custom command request for a host, and wait
>> * for the command to complete. Does not attempt to parse the
>> * response.
>> */
>> -void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
>> +void __mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq,
>> + int panic)
>> {
>> - DECLARE_COMPLETION_ONSTACK(complete);
>> + if (panic) {
>> + DECLARE_WAITQUEUE(wait, current);
>> +
>> + mmc_start_request(host, mrq);
>> +
>> + spin_lock_irq(&host->wq.lock);
>> + init_waitqueue_head(&host->wq);
>> +
>> + add_wait_queue_exclusive(&host->wq, &wait);
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>>
>> - mrq->done_data = &complete;
>> - mrq->done = mmc_wait_done;
>> + mdelay(10);
>> + spin_unlock_irq(&host->wq.lock);
>> + } else {
>> + DECLARE_COMPLETION_ONSTACK(complete);
>> +
>> + mrq->done_data = &complete;
>> + mrq->done = mmc_wait_done;
>> +
>> + mmc_start_request(host, mrq);
>> +
>> + wait_for_completion(&complete);
>> + }
>> +}
>
> This approach seems somewhat OK - it will only do the busy-wait in the
> "panic=1" case, so existing callers shouldn't be affected. (`panic'
> could have had type `bool', btw).
>
> That being said, the code is odd/wrong.
>
> void __mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq,
> int panic)
> {
> if (panic) {
> DECLARE_WAITQUEUE(wait, current);
>
> mmc_start_request(host, mrq);
>
> spin_lock_irq(&host->wq.lock);
> init_waitqueue_head(&host->wq);
>
> add_wait_queue_exclusive(&host->wq, &wait);
> set_current_state(TASK_UNINTERRUPTIBLE);
>
> mdelay(10);
> spin_unlock_irq(&host->wq.lock);
> } else {
>
> Surely the waitqueue is unneeded and the missing remove_wait_queue() is a
> bug and the set_current_state() is a bug and the spin_lock_irq()
> appears to be unneeded. afaict you want this:
>
Sorry, Missing remove_wait_queue()...
Yes, actually i want below code. but that is not working well..
You said we need to know request completed.
But it is difficult to know correct delay time..
And when we try to write log, if happen other panic/oops log..
maybe that try to write that log..then happen write crash..
So i used waitqueue.
i changed spin_lock_irqsave() instead of spin_lock_irq()
> void __mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq,
> int panic)
> {
> if (panic) {
> mmc_start_request(host, mrq);
> mdelay(10);
> } else {
>
>
> However it is pretty bad to just delay 10 milliseconds and then assume
> that the request has completed. Would it not be better to poll for the
> IRQ completion, or to poll some hardware flag?
right, 10 milliseconds is assumed..i'm also testing how much delay need..
If need, i will change that time.
>
> (We can't poll for IRQ completion if the caller has disabled IRQs, but
> if the caller has disabled IRQs then the use of spin_lock_irq() instead
> of spin_lock_irqsave() is yet another bug!)
> --
> 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
>
Thank you...
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
