linux-kernel - Re: [PATCH v2 2/6] ARM: EXYNOS: Move SYS_I2C_CFG register save/restore to i2c driver

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] [day] [month] [year] [list]

Message-id: <53698171.9070501@samsung.com>
Date:	Wed, 07 May 2014 09:42:25 +0900
From:	Pankaj Dubey <pankaj.dubey@...sung.com>
To:	Tomasz Figa <tomasz.figa@...il.com>
Cc:	linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, kgene.kim@...sung.com,
	Russell King <linux@....linux.org.uk>, arnd@...db.de,
	Wolfram Sang <wsa@...-dreams.de>, t.figa@...sung.com,
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v2 2/6] ARM: EXYNOS: Move SYS_I2C_CFG register save/restore
 to i2c driver

On 05/07/2014 04:04 AM, Tomasz Figa wrote:
> Hi Pankaj,
>
> On 06.05.2014 10:51, Pankaj Dubey wrote:
>> Let's move SYS_I2C_CFG register save/restore during s2r into i2c driver.
>> This will help in removing static iodesc based mapping from exynos.c.
>> Also will help in removing SoC specific checks in pm.c making it
>> more independent of such macros.
>>
>> CC: Wolfram Sang <wsa@...-dreams.de>
>> CC: Russell King <linux@....linux.org.uk>
>> CC: linux-i2c@...r.kernel.org
>> Signed-off-by: Pankaj Dubey <pankaj.dubey@...sung.com>
>> ---
>>   arch/arm/mach-exynos/exynos.c           |   12 +-----------
>>   arch/arm/mach-exynos/include/mach/map.h |    3 ---
>>   arch/arm/mach-exynos/pm.c               |   10 ----------
>>   arch/arm/mach-exynos/regs-sys.h         |   22 ----------------------
>>   drivers/i2c/busses/i2c-s3c2410.c        |    8 ++++++++
>>   5 files changed, 9 insertions(+), 46 deletions(-)
>>   delete mode 100644 arch/arm/mach-exynos/regs-sys.h
>
> [snip]
>
>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c 
>> b/drivers/i2c/busses/i2c-s3c2410.c
>> index 0420150..2095a01 100644
>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
>> @@ -133,6 +133,7 @@ struct s3c24xx_i2c {
>>       struct notifier_block    freq_transition;
>>   #endif
>>       struct regmap        *sysreg;
>> +    unsigned int        syc_cfg;
>
> I suspect this is a typo, as the name syc_cfg looks a bit strange. 
> Shouldn't it be sys_i2c_cfg?
Oops. Will correct it in next version.
>
>>   };
>>
>>   static struct platform_device_id s3c24xx_driver_ids[] = {
>> @@ -1293,6 +1294,9 @@ static int s3c24xx_i2c_suspend_noirq(struct device 
>> *dev)
>>       struct platform_device *pdev = to_platform_device(dev);
>>       struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
>>
>> +    if (i2c->sysreg)
>
> IS_ERR() should be used.
>
>> +        regmap_read(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, &i2c->syc_cfg);
>
> Aha, so this is where the reference to the regmap gets used outside the 
> probe. I'd say that changes to the i2c driver from this patch should be 
> squashed with previous patch and this patch should contain only arch 
> changes to remove old code.

OK, in next version I will update accordingly.

>
> However, I wonder if this is really the right approach to this, as now 
> you have save and restore duplicated for every instance of s3c24xx-i2c IP 
> block. If there are no bits other than interrupt mux selectors in this 
> registers then I guess this is fine (I can't look it up in the 
> documentation at the moment), but otherwise you can end-up with multiple 
> paths doing read-modify-write to this register in parallel possibly with 
> different values.

SYS_I2C_CFG for exynos5250 has only bits for mux selectors for i2c.

>
> Needless to say, this isn't very elegant, but I'm not opposed too much, 
> as I can't really think of anything better right now.
>
>> +
>>       i2c->suspended = 1;
>>
>>       return 0;
>> @@ -1304,6 +1308,10 @@ static int s3c24xx_i2c_resume(struct device *dev)
>>       struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
>>
>>       i2c->suspended = 0;
>> +
>> +    if (i2c->sysreg)
>> +        regmap_write(i2c->sysreg, i2c->syc_cfg, EXYNOS5_SYS_I2C_CFG);
>> +
>
> I'd say this should be happening before setting i2c->suspended to 0 to 
> account for possible i2c transfers being requested in parallel. Also see 
> patch [1].
>
> [1] https://lkml.org/lkml/2014/4/11/632

OK, will move this before i2c->suspended = 0.

If possible please review other patches also in this series so that I can 
update
whole series with addressing all review comments.

>
> Best regards,
> Tomasz
>


-- 
Best Regards,
Pankaj Dubey

--
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/