diff mbox

4.8-rc1 REGRESSION mmc-blk triggers WARN_ON(!host->claimed), related to: "drivers: use req op accessor" ?

Message ID 6e68ae5a-4b79-a636-2c46-5726cf2cc44b@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hans de Goede Aug. 21, 2016, 3:15 p.m. UTC 
Hi All,

With 4.8-rc1 I'm seeing WARN_ON(!host->claimed) triggering in both
mmc_start_request() as well as in mmc_release_host(). The first
indicating that we're executing mmc commands without doing
mmc_claim_host() and the second one indicating that we're
releasing the host without having claimed it first.

The backtraces all point to mmc_blk_issue_rq(). I've done
a very naive hack / workaround:


Which fixes this, further pointing to the somewhat magical claim / release
code in mmc_blk_issue_rq() being the culprit.

Looking at recent commits these 2 stand out as possible causes of this:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c2df40dfb8c015211ec55f4b1dd0587f875c7b34
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3a5e02ced11e22ecd9da3d6710afe15bcfee1d10

I've the feeling that one of these is making mmc_blk_issue_rq() not claiming
the host while it should do so ...

Maybe instead of the magic at the top we need a simple bool to check
if the host is already claimed by the mmc_queue code ?

I can reproduce the WARN_ON's pretty much at will (running 4.8-rc1 on
Allwinner ARM devices with rootfs on a sdcard), let me know if you've
a fix you want me to test.

Regards,

Hans


p.s.

Besides the WARN_ON triggering another side-effect of this is that the
machines will hang at poweroff.

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Mike Christie Aug. 22, 2016, 4:09 p.m. UTC | #1 
On 08/21/2016 10:15 AM, Hans de Goede wrote:
> Hi All,
> 
> With 4.8-rc1 I'm seeing WARN_ON(!host->claimed) triggering in both
> mmc_start_request() as well as in mmc_release_host(). The first
> indicating that we're executing mmc commands without doing
> mmc_claim_host() and the second one indicating that we're
> releasing the host without having claimed it first.
> 
> The backtraces all point to mmc_blk_issue_rq(). I've done
> a very naive hack / workaround:
> 
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -2151,9 +2151,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
> struct request *req)
>      struct mmc_host *host = card->host;
>      unsigned long flags;
> 
> -    if (req && !mq->mqrq_prev->req)
> -        /* claim host only for the first request */
> -        mmc_get_card(card);
> +    mmc_get_card(card);
> 
>      ret = mmc_blk_part_switch(card, md);
>      if (ret) {
> @@ -2190,15 +2188,8 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
> struct request *req)
>      }
> 
>  out:
> -    if ((!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) ||
> -        mmc_req_is_special(req))
> -        /*
> -         * Release host when there are no more requests
> -         * and after special request(discard, flush) is done.
> -         * In case sepecial request, there is no reentry to
> -         * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
> -         */
> -        mmc_put_card(card);
> +    mmc_put_card(card);
> +
>      return ret;
>  }
> 
> 
> Which fixes this, further pointing to the somewhat magical claim / release
> code in mmc_blk_issue_rq() being the culprit.
> 
> Looking at recent commits these 2 stand out as possible causes of this:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c2df40dfb8c015211ec55f4b1dd0587f875c7b34
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3a5e02ced11e22ecd9da3d6710afe15bcfee1d10
> 
> 
> I've the feeling that one of these is making mmc_blk_issue_rq() not
> claiming
> the host while it should do so ...
> 

There is a bug with those patches and the secure discard ones where when
REQ_OP_SECURE_ERASE is sent, mmc_put_card above will not be called, and
they will be treated as normal requests instead of special one so the
drivers lists are not executed properly.

The patch in Jens's tree
https://git.kernel.org/cgit/linux/kernel/git/axboe/linux-block.git/commit/?h=for-linus&id=7afafc8a44bf0ab841b17d450b02aedb3a138985
fixes the issue.

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede Aug. 24, 2016, 8:47 a.m. UTC | #2 
Hi,

On 22-08-16 18:09, Mike Christie wrote:
> On 08/21/2016 10:15 AM, Hans de Goede wrote:
>> Hi All,
>>
>> With 4.8-rc1 I'm seeing WARN_ON(!host->claimed) triggering in both
>> mmc_start_request() as well as in mmc_release_host(). The first
>> indicating that we're executing mmc commands without doing
>> mmc_claim_host() and the second one indicating that we're
>> releasing the host without having claimed it first.
>>
>> The backtraces all point to mmc_blk_issue_rq(). I've done
>> a very naive hack / workaround:
>>
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -2151,9 +2151,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>> struct request *req)
>>      struct mmc_host *host = card->host;
>>      unsigned long flags;
>>
>> -    if (req && !mq->mqrq_prev->req)
>> -        /* claim host only for the first request */
>> -        mmc_get_card(card);
>> +    mmc_get_card(card);
>>
>>      ret = mmc_blk_part_switch(card, md);
>>      if (ret) {
>> @@ -2190,15 +2188,8 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
>> struct request *req)
>>      }
>>
>>  out:
>> -    if ((!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) ||
>> -        mmc_req_is_special(req))
>> -        /*
>> -         * Release host when there are no more requests
>> -         * and after special request(discard, flush) is done.
>> -         * In case sepecial request, there is no reentry to
>> -         * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
>> -         */
>> -        mmc_put_card(card);
>> +    mmc_put_card(card);
>> +
>>      return ret;
>>  }
>>
>>
>> Which fixes this, further pointing to the somewhat magical claim / release
>> code in mmc_blk_issue_rq() being the culprit.
>>
>> Looking at recent commits these 2 stand out as possible causes of this:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c2df40dfb8c015211ec55f4b1dd0587f875c7b34
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3a5e02ced11e22ecd9da3d6710afe15bcfee1d10
>>
>>
>> I've the feeling that one of these is making mmc_blk_issue_rq() not
>> claiming
>> the host while it should do so ...
>>
>
> There is a bug with those patches and the secure discard ones where when
> REQ_OP_SECURE_ERASE is sent, mmc_put_card above will not be called, and
> they will be treated as normal requests instead of special one so the
> drivers lists are not executed properly.

Actually the problem seems to be mmc_get_card not getting called while it
should at the top of mmc_blk_issue_rq, I first get a WARN_ON(!host->claimed)
triggering in mmc_start_request (so missing mmc_card_get) and then
in mmc_release_host (mmc_card_put called without mc_card_get being called
first).

> The patch in Jens's tree
> https://git.kernel.org/cgit/linux/kernel/git/axboe/linux-block.git/commit/?h=for-linus&id=7afafc8a44bf0ab841b17d450b02aedb3a138985
> fixes the issue.

I've tried 4.8-rc3 with my hack drop and this commit cherry-picked,
but the problem is still there, so this patch does not fix it.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -2151,9 +2151,7 @@  static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
  	struct mmc_host *host = card->host;
  	unsigned long flags;

-	if (req && !mq->mqrq_prev->req)
-		/* claim host only for the first request */
-		mmc_get_card(card);
+	mmc_get_card(card);

  	ret = mmc_blk_part_switch(card, md);
  	if (ret) {
@@ -2190,15 +2188,8 @@  static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
  	}

  out:
-	if ((!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) ||
-	    mmc_req_is_special(req))
-		/*
-		 * Release host when there are no more requests
-		 * and after special request(discard, flush) is done.
-		 * In case sepecial request, there is no reentry to
-		 * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
-		 */
-		mmc_put_card(card);
+	mmc_put_card(card);
+
  	return ret;
  }