linux-kernel - Re: [PATCH 4/5] mmc: sdhci-msm: Add ops to do sdhc register write

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:   Mon, 28 Aug 2017 18:05:12 +0530
From:   Vijay Viswanath <vviswana@...eaurora.org>
To:     Adrian Hunter <adrian.hunter@...el.com>, ulf.hansson@...aro.org,
        will.deacon@....com
Cc:     linux-arm-kernel@...ts.infradead.org, linux-mmc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        asutoshd@...eaurora.org, stummala@...eaurora.org,
        riteshh@...eaurora.org, subhashj@...eaurora.org
Subject: Re: [PATCH 4/5] mmc: sdhci-msm: Add ops to do sdhc register write



On 8/24/2017 3:41 PM, Adrian Hunter wrote:
> On 18/08/17 08:19, Vijay Viswanath wrote:
>> Register writes which change voltage of IO lines or turn the IO bus
>> on/off require controller to be ready before progressing further. When
>> the controller is ready, it will generate a power irq which needs to be
>> handled. The thread which initiated the register write should wait for
>> power irq to complete. This will be done through the new sdhc msm write
>> APIs which will check whether the particular write can trigger a power
>> irq and wait for it with a timeout if it is expected.
>> The SDHC core power control IRQ gets triggered when -
>> * There is a state change in power control bit (bit 0)
>>    of SDHCI_POWER_CONTROL register.
>> * There is a state change in 1.8V enable bit (bit 3) of
>>    SDHCI_HOST_CONTROL2 register.
>> * Bit 1 of SDHCI_SOFTWARE_RESET is set.
>>
>> Signed-off-by: Vijay Viswanath <vviswana@...eaurora.org>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 6d3b1fd..6571880 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -1250,6 +1250,41 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>   	__sdhci_msm_set_clock(host, clock);
>>   }
>>   
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +static void __sdhci_msm_check_write(struct sdhci_host *host, u16 val, int reg)
>> +{
>> +	u32 req_type = 0;
>> +
>> +	switch (reg) {
>> +	case SDHCI_HOST_CONTROL2:
>> +		req_type = (val & SDHCI_CTRL_VDD_180) ? REQ_IO_LOW :
>> +			REQ_IO_HIGH;
>> +		break;
>> +	case SDHCI_SOFTWARE_RESET:
>> +		if (host->pwr && (val & SDHCI_RESET_ALL))
>> +			req_type = REQ_BUS_OFF;
>> +		break;
>> +	case SDHCI_POWER_CONTROL:
>> +		req_type = !val ? REQ_BUS_OFF : REQ_BUS_ON;
>> +		break;
>> +	}
>> +
>> +	if (req_type)
> 
> So you are really relying on these register writes not being done in an
> atomic context.  Since the spin lock was removed from sdhci_set_ios() that
> seems to be true, but it would be good to add a comment here acknowledging
> that you are depending on that.
>

Will add the comments mentioning that this function can sleep and that 
it should not be called from atomic contexts.

>> +		sdhci_msm_check_power_status(host, req_type);
>> +}
>> +
>> +static void sdhci_msm_writew(struct sdhci_host *host, u16 val, int reg)
>> +{
>> +		writew_relaxed(val, host->ioaddr + reg);
>> +		__sdhci_msm_check_write(host, val, reg);
>> +}
>> +
>> +static void sdhci_msm_writeb(struct sdhci_host *host, u8 val, int reg)
>> +{
>> +		writeb_relaxed(val, host->ioaddr + reg);
>> +		__sdhci_msm_check_write(host, val, reg);
>> +}
>> +#endif
>>   static const struct of_device_id sdhci_msm_dt_match[] = {
>>   	{ .compatible = "qcom,sdhci-msm-v4" },
>>   	{},
>> @@ -1264,6 +1299,10 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>   	.get_max_clock = sdhci_msm_get_max_clock,
>>   	.set_bus_width = sdhci_set_bus_width,
>>   	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
>> +#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
>> +	.write_w = sdhci_msm_writew,
>> +	.write_b = sdhci_msm_writeb,
>> +#endif
>>   };
>>   
>>   static const struct sdhci_pltfm_data sdhci_msm_pdata = {
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>