mmcoops with panic/oops

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.
*/

drivers/mmc/core/core.c +15 -4

-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 {

Quoted 46 lines · On Tue, 30 Nov 2010 17:42:13 +0900 Jaehoon Chung <[email protected]> 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.
*/

drivers/mmc/core/core.c +15 -4

-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 {

drivers/mmc/core/core.c static void mmc_wait_done(struct mmc_request *mrq) · line 237

Quoted 10 lines

drivers/mmc/core/core.c +10 -0

+		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:

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?

(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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html