linux-kernel - Re: [PATCH 3/4] mmc: sdhci: defer probing on regulator_get

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]

Message-ID: <CAL1qeaEGpz-9rOtp4fAHFz8KG2fCscFsqu1tbi2d9mxTM8yHQQ@mail.gmail.com>
Date:	Tue, 15 Apr 2014 12:59:37 -0700
From:	Andrew Bresticker <abrestic@...omium.org>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Thierry Reding <thierry.reding@...il.com>,
	Chris Ball <chris@...ntf.net>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	linux-tegra@...r.kernel.org,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] mmc: sdhci: defer probing on regulator_get_optional() failures

On Tue, Apr 15, 2014 at 11:28 AM, Stephen Warren <swarren@...dotorg.org> wrote:
> On 04/14/2014 07:42 PM, Andrew Bresticker wrote:
>> If regulator_get_optional() returns EPROBE_DEFER, it indicates
>> that the regulator may show up later (e.g. the DT property is
>> present but the corresponding regulator may not have probed).
>> Instead of continuing without the regulator, return EPROBE_DEFER
>> from sdhci_add_host().  Also, fix regulator leaks in the error
>> paths in sdhci_add_host().
>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>
>>       if (IS_ERR_OR_NULL(host->vqmmc)) {
>
> This is a pre-existing condition, but ick: Why doesn't this test either
> IS_ERR() /or/ == NULL, but not both. On error, the regulator API should
> either return and error-point or return NULL, so that client code
> shouldn't need to check for both.

Unfortunately, this is necessary.  regulator_get() returns NULL if
!CONFIG_REGULATOR and we do not want to call
regulator_is_supported_voltage() in this case since it will always
return false, causing sdhci_add_host() to disable UHS modes.
Alternatively, we could put the regulator_is_supported_voltage() check
wihin an #ifdef CONFIG_REGULATOR, as is done when checking supported
vmmc voltages below.

>
>> +put_vmmc:
>> +     if (host->vmmc)
>> +             regulator_put(host->vmmc);
>> +put_vqmmc:
>> +     if (host->vqmmc)
>> +             regulator_put(host->vqmmc);
>
> If IS_ERR_OR_NULL() really is required above, it should be used here
> too. More likely, I hope you need to replace if (host->vmmc) with if
> (!IS_ERR(host->vmmc)).

host->vmmc and host->vqmmc are set to NULL in the case of IS_ERR(), so
the IS_ERR() check here isn't necessary.

>
> I wonder if fixing this would help solve the crashes that Alex saw?

I believe the crash Alex is seeing is due to a race between tearing
down the sdhci driver on probe deferral and the card-detect IRQ.
Looking at it now.
--
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/