diff mbox

[v2,2/4] mmc: mmci: move block size validation under relevant code

Message ID 1408446866-1649-1-git-send-email-srinivas.kandagatla@linaro.org
State New
Headers show

Commit Message

Srinivas Kandagatla Aug. 19, 2014, 11:14 a.m. UTC
This code moves a BUG_ON condition to relevant if block, this check is
not necessary for IPs which can set any arbitrary block size in a given
range.
This patch is necessary for SDIO which sets block sizes that are exactly
not power of 2.

Original issue detected while testing WLAN ath6kl on Qualcomm APQ8064 SOC.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/mmc/host/mmci.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Ulf Hansson Aug. 19, 2014, 11:55 a.m. UTC | #1
On 19 August 2014 13:14, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> This code moves a BUG_ON condition to relevant if block, this check is
> not necessary for IPs which can set any arbitrary block size in a given
> range.
> This patch is necessary for SDIO which sets block sizes that are exactly
> not power of 2.
>
> Original issue detected while testing WLAN ath6kl on Qualcomm APQ8064 SOC.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  drivers/mmc/host/mmci.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 3089fba..1c99195 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -800,15 +800,16 @@ static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
>         writel(timeout, base + MMCIDATATIMER);
>         writel(host->size, base + MMCIDATALENGTH);
>
> -       blksz_bits = ffs(data->blksz) - 1;
> -       BUG_ON(1 << blksz_bits != data->blksz);

I don't like this BUG_ON at all, I would prefer if we remove it. The
original patch "mmc: mmci: Support any block sizes for ux500v2", did
so as well.

Now, if we still think removing it is fragile, let additional tests in
mmci_validate_data() and return and error code from there.

Kind regards
Uffe

>
> -       if (variant->blksz_datactrl16)
> +       if (variant->blksz_datactrl16) {
>                 datactrl = MCI_DPSM_ENABLE | (data->blksz << 16);
> -       else if (variant->blksz_datactrl4)
> +       } else if (variant->blksz_datactrl4) {
>                 datactrl = MCI_DPSM_ENABLE | (data->blksz << 4);
> -       else
> +       } else {
> +               blksz_bits = ffs(data->blksz) - 1;
> +               BUG_ON(1 << blksz_bits != data->blksz);
>                 datactrl = MCI_DPSM_ENABLE | blksz_bits << 4;
> +       }
>
>         if (data->flags & MMC_DATA_READ)
>                 datactrl |= MCI_DPSM_DIRECTION;
> --
> 1.9.1
>
--
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
Srinivas Kandagatla Aug. 19, 2014, 12:08 p.m. UTC | #2
On 19/08/14 12:55, Ulf Hansson wrote:
>>          writel(host->size, base + MMCIDATALENGTH);
>> >
>> >-       blksz_bits = ffs(data->blksz) - 1;
>> >-       BUG_ON(1 << blksz_bits != data->blksz);
> I don't like this BUG_ON at all, I would prefer if we remove it. The
> original patch "mmc: mmci: Support any block sizes for ux500v2", did
> so as well.
>
Yes, you are right this is redundant check, mmci_validate_data in 
mmci_request should catch this error anyway. we can remove this check 
totally and use your original patch.


--srini
> Now, if we still think removing it is fragile, let additional tests in
> mmci_validate_data() and return and error code from there.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 3089fba..1c99195 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -800,15 +800,16 @@  static void mmci_start_data(struct mmci_host *host, struct mmc_data *data)
 	writel(timeout, base + MMCIDATATIMER);
 	writel(host->size, base + MMCIDATALENGTH);
 
-	blksz_bits = ffs(data->blksz) - 1;
-	BUG_ON(1 << blksz_bits != data->blksz);
 
-	if (variant->blksz_datactrl16)
+	if (variant->blksz_datactrl16) {
 		datactrl = MCI_DPSM_ENABLE | (data->blksz << 16);
-	else if (variant->blksz_datactrl4)
+	} else if (variant->blksz_datactrl4) {
 		datactrl = MCI_DPSM_ENABLE | (data->blksz << 4);
-	else
+	} else {
+		blksz_bits = ffs(data->blksz) - 1;
+		BUG_ON(1 << blksz_bits != data->blksz);
 		datactrl = MCI_DPSM_ENABLE | blksz_bits << 4;
+	}
 
 	if (data->flags & MMC_DATA_READ)
 		datactrl |= MCI_DPSM_DIRECTION;