[PATCH v2 1/2] mmc: block: remove the check of packed for end_packed_req and hdr_wrq_prep

[PATCH v2 1/2] mmc: block: remove the check of packed for end_packed_req and hdr_wrq_prep

[Shawn Lin]

packed should always exist without calling its cleanup function
explicitly. Moreover, we have use it when preparing packed list.
So I don't believe we should ever fall into this check again when
doing mmc_blk_packed_hdr_wrq_prep or mmc_blk_end_packed_req. And
the code of mmc_blk_end_packed_req is trying to use packed before
checking it which makes it quite weird. This patch is trying to
remove these two checks and move it to the mmc_blk_prep_packed_list.
If we find packed is null, then we should never use MMC_PACKED_NONE
and MMC_BLK_PACKED_CMD. By doing this, we could fall back to
non-packed request if finding null packed, though it's impossible
theoretically.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- remove BUG_ON and add fallback for packed instead of
  panic it.

 drivers/mmc/card/block.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 48a5dd7..c7a9b28 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1673,6 +1673,18 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
 	u8 max_packed_rw = 0;
 	u8 reqs = 0;
 
+	/*
+	 * We don't need to check packed for any further
+	 * operation of packed stuff as we set MMC_PACKED_NONE
+	 * and return zero for reqs if geting null packed. Also
+	 * we clean the flag of MMC_BLK_PACKED_CMD to avoid doing
+	 * it again when removing blk req.
+	 */
+	if (!mqrq->packed) {
+		md->flags &= (~MMC_BLK_PACKED_CMD);
+		goto no_packed;
+	}
+
 	if (!(md->flags & MMC_BLK_PACKED_CMD))
 		goto no_packed;
 
@@ -1781,8 +1793,6 @@ static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
 	u8 hdr_blocks;
 	u8 i = 1;
 
-	BUG_ON(!packed);
-
 	mqrq->cmd_type = MMC_PACKED_WRITE;
 	packed->blocks = 0;
 	packed->idx_failure = MMC_PACKED_NR_IDX;
@@ -1886,8 +1896,6 @@ static int mmc_blk_end_packed_req(struct mmc_queue_req *mq_rq)
 	int idx = packed->idx_failure, i = 0;
 	int ret = 0;
 
-	BUG_ON(!packed);
-
 	while (!list_empty(&packed->list)) {
 		prq = list_entry_rq(packed->list.next);
 		if (idx == i) {
-- 
2.3.7

[PATCH v2 2/2] mmc: sdio: deploy error handling instead of triggering BUG_ON

[Shawn Lin]

When using mmc_io_rw_extended, it's intent to avoid null
pointer of card and invalid func number. But actually it
didn't prevent that as the seg_size already use the card.
Currently the wrapper function sdio_io_rw_ext_helper already
use card before calling mmc_io_rw_extended, so we should move
this check to there. As to the func number, it was token from
'(ocr & 0x70000000) >> 28' which should be enough to guarantee
that it won't be larger than 7. But we should prevent the
caller like wifi drivers modify this value. So let's move this
check into sdio_io_rw_ext_helper either.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2:
- remove the BUG_ON and move these into sdio_io_rw_ext_helper

 drivers/mmc/core/sdio_io.c  | 3 +++
 drivers/mmc/core/sdio_ops.c | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 78cb4d5..18f2938 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -307,6 +307,9 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
 	unsigned max_blocks;
 	int ret;
 
+	if ((!func->card) || (func->num > 7))
+		return -EINVAL;
+
 	/* Do the bulk of the transfer using block mode (if supported). */
 	if (func->card->cccr.multi_block && (size > sdio_max_byte_size(func))) {
 		/* Blocks per command is limited by host count, host transfer
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 34f6e80..45397e8 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -129,8 +129,6 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 	unsigned int nents, left_size, i;
 	unsigned int seg_size = card->host->max_seg_size;
 
-	BUG_ON(!card);
-	BUG_ON(fn > 7);
 	WARN_ON(blksz == 0);
 
 	/* sanity check */
-- 
2.3.7

[PATCH v2 2/2] mmc: sdio: deploy error handling instead of triggering BUG_ON

[Shawn Lin]

When using mmc_io_rw_extended, it's intent to avoid null
pointer of card and invalid func number. But actually it
didn't prevent that as the seg_size already use the card.
Currently the wrapper function sdio_io_rw_ext_helper already
use card before calling mmc_io_rw_extended, so we should move
this check to there. As to the func number, it was token from
'(ocr & 0x70000000) >> 28' which should be enough to guarantee
that it won't be larger than 7. But we should prevent the
caller like wifi drivers modify this value. So let's move this
check into sdio_io_rw_ext_helper either.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v2:
- remove the BUG_ON and move these into sdio_io_rw_ext_helper

 drivers/mmc/core/sdio_io.c  | 3 +++
 drivers/mmc/core/sdio_ops.c | 2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 78cb4d5..18f2938 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -307,6 +307,9 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
 	unsigned max_blocks;
 	int ret;
 
+	if ((!func->card) || (func->num > 7))
+		return -EINVAL;
+
 	/* Do the bulk of the transfer using block mode (if supported). */
 	if (func->card->cccr.multi_block && (size > sdio_max_byte_size(func))) {
 		/* Blocks per command is limited by host count, host transfer
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 34f6e80..45397e8 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -129,8 +129,6 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 	unsigned int nents, left_size, i;
 	unsigned int seg_size = card->host->max_seg_size;
 
-	BUG_ON(!card);
-	BUG_ON(fn > 7);
 	WARN_ON(blksz == 0);
 
 	/* sanity check */
-- 
2.3.7

Re: [PATCH v2 2/2] mmc: sdio: deploy error handling instead of triggering BUG_ON

[Ulf Hansson]

On 24 August 2016 at 09:58, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> When using mmc_io_rw_extended, it's intent to avoid null
> pointer of card and invalid func number. But actually it
> didn't prevent that as the seg_size already use the card.
> Currently the wrapper function sdio_io_rw_ext_helper already
> use card before calling mmc_io_rw_extended, so we should move
> this check to there. As to the func number, it was token from
> '(ocr & 0x70000000) >> 28' which should be enough to guarantee
> that it won't be larger than 7. But we should prevent the
> caller like wifi drivers modify this value. So let's move this
> check into sdio_io_rw_ext_helper either.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v2:
> - remove the BUG_ON and move these into sdio_io_rw_ext_helper
>
>  drivers/mmc/core/sdio_io.c  | 3 +++
>  drivers/mmc/core/sdio_ops.c | 2 --
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index 78cb4d5..18f2938 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -307,6 +307,9 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
>         unsigned max_blocks;
>         int ret;
>
> +       if ((!func->card) || (func->num > 7))

You also need to validate func and before func->card.

> +               return -EINVAL;
> +
>         /* Do the bulk of the transfer using block mode (if supported). */
>         if (func->card->cccr.multi_block && (size > sdio_max_byte_size(func))) {
>                 /* Blocks per command is limited by host count, host transfer
> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> index 34f6e80..45397e8 100644
> --- a/drivers/mmc/core/sdio_ops.c
> +++ b/drivers/mmc/core/sdio_ops.c
> @@ -129,8 +129,6 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
>         unsigned int nents, left_size, i;
>         unsigned int seg_size = card->host->max_seg_size;
>
> -       BUG_ON(!card);
> -       BUG_ON(fn > 7);
>         WARN_ON(blksz == 0);
>
>         /* sanity check */
> --
> 2.3.7
>
>

There are a couple of more BUG_ONs() that is being invoked when SDIO
func drivers abuses the SDIO func API.

In cases when the API allows us to propagate error codes we should do
that and just when the sdio_io_rw_ext_helper() is involved. In case
when the API don't allows to propagate error codes, we should convert
the BUG_ON() to  "if (WARN_ON()) return;".

Kind regards
Uffe

Re: [PATCH v2 1/2] mmc: block: remove the check of packed for end_packed_req and hdr_wrq_prep

[Ulf Hansson]

On 24 August 2016 at 09:58, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> packed should always exist without calling its cleanup function
> explicitly. Moreover, we have use it when preparing packed list.
> So I don't believe we should ever fall into this check again when
> doing mmc_blk_packed_hdr_wrq_prep or mmc_blk_end_packed_req. And
> the code of mmc_blk_end_packed_req is trying to use packed before
> checking it which makes it quite weird. This patch is trying to
> remove these two checks and move it to the mmc_blk_prep_packed_list.
> If we find packed is null, then we should never use MMC_PACKED_NONE
> and MMC_BLK_PACKED_CMD. By doing this, we could fall back to
> non-packed request if finding null packed, though it's impossible
> theoretically.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>
> ---
>
> Changes in v2:
> - remove BUG_ON and add fallback for packed instead of
>   panic it.
>
>  drivers/mmc/card/block.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 48a5dd7..c7a9b28 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1673,6 +1673,18 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
>         u8 max_packed_rw = 0;
>         u8 reqs = 0;
>
> +       /*
> +        * We don't need to check packed for any further
> +        * operation of packed stuff as we set MMC_PACKED_NONE
> +        * and return zero for reqs if geting null packed. Also
> +        * we clean the flag of MMC_BLK_PACKED_CMD to avoid doing
> +        * it again when removing blk req.
> +        */
> +       if (!mqrq->packed) {
> +               md->flags &= (~MMC_BLK_PACKED_CMD);
> +               goto no_packed;
> +       }
> +
>         if (!(md->flags & MMC_BLK_PACKED_CMD))
>                 goto no_packed;
>
> @@ -1781,8 +1793,6 @@ static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
>         u8 hdr_blocks;
>         u8 i = 1;
>
> -       BUG_ON(!packed);
> -
>         mqrq->cmd_type = MMC_PACKED_WRITE;
>         packed->blocks = 0;
>         packed->idx_failure = MMC_PACKED_NR_IDX;
> @@ -1886,8 +1896,6 @@ static int mmc_blk_end_packed_req(struct mmc_queue_req *mq_rq)
>         int idx = packed->idx_failure, i = 0;
>         int ret = 0;
>
> -       BUG_ON(!packed);
> -
>         while (!list_empty(&packed->list)) {
>                 prq = list_entry_rq(packed->list.next);
>                 if (idx == i) {
> --
> 2.3.7
>
>

This is step in the right direction, although there are still some
BUG_ONs() for the packed command path. Could you please have look and
remove all of them?

Kind regards
Uffe

Re: [PATCH v2 1/2] mmc: block: remove the check of packed for end_packed_req and hdr_wrq_prep

[Shawn Lin]

在 2016/8/24 17:30, Ulf Hansson 写道:
> On 24 August 2016 at 09:58, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> packed should always exist without calling its cleanup function
>> explicitly. Moreover, we have use it when preparing packed list.
>> So I don't believe we should ever fall into this check again when
>> doing mmc_blk_packed_hdr_wrq_prep or mmc_blk_end_packed_req. And
>> the code of mmc_blk_end_packed_req is trying to use packed before
>> checking it which makes it quite weird. This patch is trying to
>> remove these two checks and move it to the mmc_blk_prep_packed_list.
>> If we find packed is null, then we should never use MMC_PACKED_NONE
>> and MMC_BLK_PACKED_CMD. By doing this, we could fall back to
>> non-packed request if finding null packed, though it's impossible
>> theoretically.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
>>
>> Changes in v2:
>> - remove BUG_ON and add fallback for packed instead of
>>   panic it.
>>
>>  drivers/mmc/card/block.c | 16 ++++++++++++----
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index 48a5dd7..c7a9b28 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -1673,6 +1673,18 @@ static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
>>         u8 max_packed_rw = 0;
>>         u8 reqs = 0;
>>
>> +       /*
>> +        * We don't need to check packed for any further
>> +        * operation of packed stuff as we set MMC_PACKED_NONE
>> +        * and return zero for reqs if geting null packed. Also
>> +        * we clean the flag of MMC_BLK_PACKED_CMD to avoid doing
>> +        * it again when removing blk req.
>> +        */
>> +       if (!mqrq->packed) {
>> +               md->flags &= (~MMC_BLK_PACKED_CMD);
>> +               goto no_packed;
>> +       }
>> +
>>         if (!(md->flags & MMC_BLK_PACKED_CMD))
>>                 goto no_packed;
>>
>> @@ -1781,8 +1793,6 @@ static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
>>         u8 hdr_blocks;
>>         u8 i = 1;
>>
>> -       BUG_ON(!packed);
>> -
>>         mqrq->cmd_type = MMC_PACKED_WRITE;
>>         packed->blocks = 0;
>>         packed->idx_failure = MMC_PACKED_NR_IDX;
>> @@ -1886,8 +1896,6 @@ static int mmc_blk_end_packed_req(struct mmc_queue_req *mq_rq)
>>         int idx = packed->idx_failure, i = 0;
>>         int ret = 0;
>>
>> -       BUG_ON(!packed);
>> -
>>         while (!list_empty(&packed->list)) {
>>                 prq = list_entry_rq(packed->list.next);
>>                 if (idx == i) {
>> --
>> 2.3.7
>>
>>
>
> This is step in the right direction, although there are still some
> BUG_ONs() for the packed command path. Could you please have look and
> remove all of them?

yup, I will try to remove all of them when I respin v3.

Thansk.

>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin

Re: [PATCH v2 2/2] mmc: sdio: deploy error handling instead of triggering BUG_ON

[Shawn Lin]

在 2016/8/24 17:30, Ulf Hansson 写道:
> On 24 August 2016 at 09:58, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> When using mmc_io_rw_extended, it's intent to avoid null
>> pointer of card and invalid func number. But actually it
>> didn't prevent that as the seg_size already use the card.
>> Currently the wrapper function sdio_io_rw_ext_helper already
>> use card before calling mmc_io_rw_extended, so we should move
>> this check to there. As to the func number, it was token from
>> '(ocr & 0x70000000) >> 28' which should be enough to guarantee
>> that it won't be larger than 7. But we should prevent the
>> caller like wifi drivers modify this value. So let's move this
>> check into sdio_io_rw_ext_helper either.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - remove the BUG_ON and move these into sdio_io_rw_ext_helper
>>
>>  drivers/mmc/core/sdio_io.c  | 3 +++
>>  drivers/mmc/core/sdio_ops.c | 2 --
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
>> index 78cb4d5..18f2938 100644
>> --- a/drivers/mmc/core/sdio_io.c
>> +++ b/drivers/mmc/core/sdio_io.c
>> @@ -307,6 +307,9 @@ static int sdio_io_rw_ext_helper(struct sdio_func *func, int write,
>>         unsigned max_blocks;
>>         int ret;
>>
>> +       if ((!func->card) || (func->num > 7))
>
> You also need to validate func and before func->card.

Good catch, thanks.

>
>> +               return -EINVAL;
>> +
>>         /* Do the bulk of the transfer using block mode (if supported). */
>>         if (func->card->cccr.multi_block && (size > sdio_max_byte_size(func))) {
>>                 /* Blocks per command is limited by host count, host transfer
>> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
>> index 34f6e80..45397e8 100644
>> --- a/drivers/mmc/core/sdio_ops.c
>> +++ b/drivers/mmc/core/sdio_ops.c
>> @@ -129,8 +129,6 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
>>         unsigned int nents, left_size, i;
>>         unsigned int seg_size = card->host->max_seg_size;
>>
>> -       BUG_ON(!card);
>> -       BUG_ON(fn > 7);
>>         WARN_ON(blksz == 0);
>>
>>         /* sanity check */
>> --
>> 2.3.7
>>
>>
>
> There are a couple of more BUG_ONs() that is being invoked when SDIO
> func drivers abuses the SDIO func API.
>
> In cases when the API allows us to propagate error codes we should do
> that and just when the sdio_io_rw_ext_helper() is involved. In case
> when the API don't allows to propagate error codes, we should convert
> the BUG_ON() to  "if (WARN_ON()) return;".

Right, I was wondering that why there are so many BUG_ON for SDIO routine.

I will look at all of these API routines and check them
one by one.

>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin