linux-kernel - Re: Re: [PATCH] MFD : add MAX77686 mfd 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>] [day] [month] [year] [list]

Message-id: <24380132.112141335951419265.JavaMail.weblogic@epml09>
Date:	Wed, 02 May 2012 09:37:00 +0000 (GMT)
From:	함명주 <myungjoo.ham@...sung.com>
To:	Andi Shyti <andi.shyti@...il.com>,
	이종화 <jonghwa3.lee@...sung.com>
Cc:	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	최찬우 <cw00.choi@...sung.com>,
	변치웅 <woong.byun@...sung.com>,
	박경민 <kyungmin.park@...sung.com>
Subject: Re: Re: [PATCH] MFD : add MAX77686 mfd driver

> Hi,
> 
> On Wed, May 02, 2012 at 02:02:55PM +0900, jonghwa3.lee@...sung.com wrote:
> > On 2012-04-30 18:17, Andi Shyti wrote:
> > > Hi,
> > > 
> > >> + mutex_lock(&max77686->iolock);
> > >> + ret = i2c_smbus_read_i2c_block_data(i2c, reg, count, buf);
> > >> + mutex_unlock(&max77686->iolock);
> > > 
> > > Is it relly necessay to lock whenever you read/write from/to the
> > > i2c bus? Considering also that these are exported function,
> > > someone else may lock here before, so we can have a double
> > > locking on the same mutex.
> > 
> > These exported functions will be used in 77686 area only, so there is no
> > overlap locking.
> 
> OK... I think this could be a reason more to not over-use mutexes :)
> 
> When you call i2c_smbus_* functions you are not accessing to any
> private data, all the new data is allocated in a new area. The
> smbus_xfer function should take care by himself that the global
> data are locked correctly. If not, is not up to your driver to do
> it.
> If, instead, you are taking care about the concurrency in the
> bus, this should be somehow managed by the chip itself.
> In my opinion you are abusing a bit of mutex_lock/unlock.
> 
> Andi
> 
> P.S. copied and paste your reply at the bottom of my previous
> comment.

I expect MAX77686-PMIC(Regulator) driver will be using update_reg() heavily. That function requires mutexing such contexts to work correctly. You won't get correct update without a mutex as it will read a register and write to a register not atomically.

Without this mutex, updating a register (i.e., update the third bit to 1) can be disasterous with regulators.


Cheers!
MyungJoo.

> 
> 
> 


--

MyungJoo Ham (함명주), PHD

System S/W Lab, S/W Platform Team, Software Center
Samsung Electronics
Cell: +82-10-6714-2858