linux-kernel - Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by itself

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 31 Oct 2012 23:20:37 +0800
From:	Shawn Guo <shawn.guo@...aro.org>
To:	yongd <yongd@...vell.com>
CC:	Chris Ball <cjb@...top.org>,
	Anton Vorontsov <anton.vorontsov@...aro.org>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Daniel Drake <dsd@...top.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Wilson Callan <wilson.callan@...antsystems.com>,
	Ben Dooks <ben-linux@...ff.org>, <zhangfei.gao@...vell.com>,
	<kevin.liu@...vell.com>, <linux-mmc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 1/3] mmc: esdhc: enable polling to detect card by
 itself

On Tue, Oct 30, 2012 at 05:30:01PM +0800, yongd wrote:
> In the current code logic, sdhci_add_host() will enable the polling
> method (set MMC_CAP_NEEDS_POLL) for a removable card (MMC_CAP_
> NONREMOVABLE is not set) whose host's internal card detection method
> is disabled for some reason (SDHCI_QUIRK_BROKEN_CARD_DETECTION is set).
> 
> However, this is improper since we can have some other card detection
> methods besides host internal card detection and polling method. For
> example, if the card detection type is ESDHC_CD_GPIO (external gpio pin
> for CD), we should keep SDHCI_QUIRK_BROKEN_CARD_DETECTION set, or host
> internal card detection interrupt will still be enabled. So, here comes
> the 1st change in this patch to keep SDHCI_QUIRK_BROKEN_CARD_DETECTION
> set for ESDHC_CD_GPIO type. But, after the 1st change, just as above
> said, sdhci_add_host() will still enable polling for such a card.This
> is redundant.
> 
This actually results in an unpleasant rather than redundant behavior.
Right after this patch gets applied and before patch #3 gets applied,
driver sdhci-esdhc-imx will use polling even for ESDHC_CD_GPIO case.

> On the other hand, for the card with ESDHC_CD_NONE detection type(no CD,
> neither controller nor gpio, so use polling), we currently rely on
> sdhci_add_host() to enable polling for us.
> 
> Here propose the 2nd change in this patch for such an embarrassing case.

Besides above, this is another sign that the patch (and series) should
be better partitioned.

> 1st, this patch will de-couple polling enabling with sdhci_add_host() by
> doing this in host driver itself, just as some other vendors. 2nd, one
> more patch will remove such improper MMC_CAP_NEEDS_POLL enabling in
> sdhci_add_host().
> 
> Change-Id: Ia7525009d8fd188e3f0169f225e4a76ff9e94b47

Remove this, please.

> Signed-off-by: yongd <yongd@...vell.com>
> ---
>  drivers/mmc/host/sdhci-esdhc-imx.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index effc2ac..ff201a5 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -557,7 +557,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  			dev_err(mmc_dev(host->mmc), "request irq error\n");
>  			goto no_card_detect_irq;
>  		}
> -		/* fall through */
> +		break;

The current sdhci-esdhc-imx implementation clears flag
SDHCI_QUIRK_BROKEN_CARD_DETECTION even for ESDHC_CD_GPIO case
and then emulate flag SDHCI_CARD_PRESENT by reading CD gpio state in
esdhc_readl_le().  Obviously, simply setting the flag for gpio case
does not provide an equal behavior as before.

Shawn

>  
>  	case ESDHC_CD_CONTROLLER:
>  		/* we have a working card_detect back */
> @@ -569,6 +569,7 @@ static int __devinit sdhci_esdhc_imx_probe(struct platform_device *pdev)
>  		break;
>  
>  	case ESDHC_CD_NONE:
> +		host->mmc->caps = MMC_CAP_NEEDS_POLL;
>  		break;
>  	}
>  
> -- 
> 1.7.9.5
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/