linux-kernel - Re: [PATCH 01/14] memory: ti-emif-sram: Add resume function to recopy sram code

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, 16 Apr 2018 15:59:14 +0530
From:   Keerthy <j-keerthy@...com>
To:     "santosh.shilimkar@...cle.com" <santosh.shilimkar@...cle.com>,
        <linus.walleij@...aro.org>, <grygorii.strashko@...com>,
        <tony@...mide.com>
CC:     <t-kristo@...com>, <Russ.Dill@...com>,
        <linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <ssantosh@...nel.org>, <haojian.zhuang@...aro.org>,
        <linux-arm-kernel@...ts.infradead.org>, <d-gerlach@...com>
Subject: Re: [PATCH 01/14] memory: ti-emif-sram: Add resume function to recopy
 sram code



On Thursday 12 April 2018 10:14 PM, santosh.shilimkar@...cle.com wrote:
> On 4/11/18 9:53 PM, Keerthy wrote:
>> From: Dave Gerlach <d-gerlach@...com>
>>
>> After an RTC+DDR cycle we lose sram context so emif pm functions present
>> in sram are lost. We can check if the first byte of the original
>> code in DDR contains the same first byte as the code in sram, and if
>> they do not match we know we have lost context and must recopy the
>> functions to the previous address to maintain PM functionality.
>>
>> Signed-off-by: Dave Gerlach <d-gerlach@...com>
>> Signed-off-by: Keerthy <j-keerthy@...com>
>> ---
>>   drivers/memory/ti-emif-pm.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/memory/ti-emif-pm.c b/drivers/memory/ti-emif-pm.c
>> index 632651f..ec4a62c 100644
>> --- a/drivers/memory/ti-emif-pm.c
>> +++ b/drivers/memory/ti-emif-pm.c
>> @@ -249,6 +249,25 @@ int ti_emif_get_mem_type(void)
>>   };
>>   MODULE_DEVICE_TABLE(of, ti_emif_of_match);
>>   +#ifdef CONFIG_PM_SLEEP
>> +static int ti_emif_resume(struct device *dev)
>> +{
>> +    unsigned long tmp =
>> +            __raw_readl((void *)emif_instance->ti_emif_sram_virt);
>> +
>> +    /*
>> +     * Check to see if what we are copying is already present in the
>> +     * first byte at the destination, only copy if it is not which
>> +     * indicates we have lost context and sram no longer contains
>> +     * the PM code
>> +     */
> 
>> +    if (tmp != ti_emif_sram)
>> +        ti_emif_push_sram(dev, emif_instance);
>> +
>> +    return 0;
>> +}
>> +#endif /* CONFIG_PM_SLEEP */
> Instead of this indirect method , why can't just check the previous
> deep sleep mode and based on that do copy or not. EMIF power status
> register should have something like that ?

I will check if we have a register that tells the previous state of sram.

> 
> Another minor point is even though there is nothing to do in suspend,
> might be good to have a callback with comment that nothing to do with
> some explanation why not. Don't have strong preference but may for
> better readability.

Okay. Thanks a lot for the quick feedback!

> 
> Regards,
> Santosh
> 
>