* Re: [iio:fixes-togreg 19/19] drivers/iio/adc/mcp3422.c:147:3-9: preceding lock on line 137 (fwd)
[not found] ` <CA+TH9VmQq3=Kf=f72CSn2ZziKP3YP6qjsXQL1nXzS-O8FscBWw@mail.gmail.com>
@ 2020-09-01 8:50 ` Jonathan Cameron
2020-09-01 9:08 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2020-09-01 8:50 UTC (permalink / raw)
To: Angelo Compagnucci; +Cc: Julia Lawall, gregkh, linux-iio
On Mon, 31 Aug 2020 11:09:53 +0200
Angelo Compagnucci <angelo.compagnucci@gmail.com> wrote:
> Il giorno lun 31 ago 2020 alle ore 09:48 Julia Lawall
> <julia.lawall@inria.fr> ha scritto:
> >
> >
> >
> > On Mon, 31 Aug 2020, Angelo Compagnucci wrote:
> >
> > > Hi Julia,
> > >
> > > Il giorno sab 29 ago 2020 alle ore 22:29 Julia Lawall
> > > <julia.lawall@inria.fr> ha scritto:
> > > >
> > > > Please check whether there should be a mutex_unlock before line 147.
> > >
> > > Having a mutex_unlock before line 147 is wrong here, cause the lock
> > > should be held for the entire reading operation. Adding an unlock
> > > before the lock means that a concurrent call can unlock the lock
> > > previously held by another call and the result ends up mixing the
> > > reading for the first call to the reading of the second call.
> >
> > OK, I don't know the calling context. But you have a function where the
> > lock is held on the failure path and is released on the success path,
> > which seems at least a little strange.
>
> I see.
>
> I have to respin!
>
> Thanks for your support!
Hi Julia, Angelo,
Please can we cc linux-iio@vger.kernel.org for such reports.
The fix has headed upstream. So we need to chase it with a fix asap.
Greg, would you prefer a following fix (please cc Greg directly) or
to revert the patch?
3f1093d83d71 ("iio: adc: mcp3422: fix locking scope")
Sorry I missed this one. Was working on wrong computer to access
the account this went to.
Jonathan
>
> > But if the calling context deals
> > with that in a reasonable way, then ok. Maybe a comment would be useful.
> >
> > julia
> >
> > >
> > > Sincerely, Angelo
> > >
> > > >
> > > > julia
> > > >
> > > >
> > > > ---------- Forwarded message ----------
> > > > Date: Sun, 30 Aug 2020 04:08:59 +0800
> > > > From: kernel test robot <lkp@intel.com>
> > > > To: kbuild@lists.01.org
> > > > Cc: lkp@intel.com, Julia Lawall <julia.lawall@lip6.fr>
> > > > Subject: [iio:fixes-togreg 19/19] drivers/iio/adc/mcp3422.c:147:3-9: preceding
> > > > lock on line 137
> > > >
> > > > CC: kbuild-all@lists.01.org
> > > > TO: Angelo Compagnucci <angelo.compagnucci@gmail.com>
> > > > CC: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > tree: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git fixes-togreg
> > > > head: ba255800f7fbb8da411c92c33b25d52970558509
> > > > commit: ba255800f7fbb8da411c92c33b25d52970558509 [19/19] iio: adc: mcp3422: fix locking scope
> > > > :::::: branch date: 3 hours ago
> > > > :::::: commit date: 3 hours ago
> > > > config: i386-randconfig-c001-20200830 (attached as .config)
> > > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > >
> > > > If you fix the issue, kindly add following tag as appropriate
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> > > >
> > > >
> > > > coccinelle warnings: (new ones prefixed by >>)
> > > >
> > > > >> drivers/iio/adc/mcp3422.c:147:3-9: preceding lock on line 137
> > > >
> > > > # https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?id=ba255800f7fbb8da411c92c33b25d52970558509
> > > > git remote add iio https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> > > > git fetch --no-tags iio fixes-togreg
> > > > git checkout ba255800f7fbb8da411c92c33b25d52970558509
> > > > vim +147 drivers/iio/adc/mcp3422.c
> > > >
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 129
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 130 static int mcp3422_read_channel(struct mcp3422 *adc,
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 131 struct iio_chan_spec const *channel, int *value)
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 132 {
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 133 int ret;
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 134 u8 config;
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 135 u8 req_channel = channel->channel;
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 136
> > > > ba255800f7fbb8d Angelo Compagnucci 2020-08-19 @137 mutex_lock(&adc->lock);
> > > > ba255800f7fbb8d Angelo Compagnucci 2020-08-19 138
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 139 if (req_channel != MCP3422_CHANNEL(adc->config)) {
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 140 config = adc->config;
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 141 config &= ~MCP3422_CHANNEL_MASK;
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 142 config |= MCP3422_CHANNEL_VALUE(req_channel);
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 143 config &= ~MCP3422_PGA_MASK;
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 144 config |= MCP3422_PGA_VALUE(adc->pga[req_channel]);
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 145 ret = mcp3422_update_config(adc, config);
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 146 if (ret < 0)
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 @147 return ret;
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 148 msleep(mcp3422_read_times[MCP3422_SAMPLE_RATE(adc->config)]);
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 149 }
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 150
> > > > ba255800f7fbb8d Angelo Compagnucci 2020-08-19 151 ret = mcp3422_read(adc, value, &config);
> > > > ba255800f7fbb8d Angelo Compagnucci 2020-08-19 152
> > > > ba255800f7fbb8d Angelo Compagnucci 2020-08-19 153 mutex_unlock(&adc->lock);
> > > > ba255800f7fbb8d Angelo Compagnucci 2020-08-19 154
> > > > ba255800f7fbb8d Angelo Compagnucci 2020-08-19 155 return ret;
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 156 }
> > > > 07914c84ba30e31 Angelo Compagnucci 2013-09-02 157
> > > >
> > > > :::::: The code at line 147 was first introduced by commit
> > > > :::::: 07914c84ba30e311f6bfb65b811b33a1dc094311 iio: adc: Add driver for Microchip MCP3422/3/4 high resolution ADC
> > > >
> > > > :::::: TO: Angelo Compagnucci <angelo.compagnucci@gmail.com>
> > > > :::::: CC: Jonathan Cameron <jic23@kernel.org>
> > > >
> > > > ---
> > > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> > >
> > >
> > >
> > > --
> > > Profile: http://it.linkedin.com/in/compagnucciangelo
> > >
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread