Re: [RESEND PATCH 5.15 v3 5/5] counter: 104-quad-8: Fix race condition between FLAG and CNTR reads

[William Breathitt Gray <william.gray@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 3849 bytes --]

On Tue, Apr 18, 2023 at 11:41:29AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 17, 2023 at 09:40:52AM -0400, William Breathitt Gray wrote:
> > On Tue, Apr 11, 2023 at 11:52:20AM -0400, William Breathitt Gray wrote:
> > > commit 4aa3b75c74603c3374877d5fd18ad9cc3a9a62ed upstream.
> > > 
> > > The Counter (CNTR) register is 24 bits wide, but we can have an
> > > effective 25-bit count value by setting bit 24 to the XOR of the Borrow
> > > flag and Carry flag. The flags can be read from the FLAG register, but a
> > > race condition exists: the Borrow flag and Carry flag are instantaneous
> > > and could change by the time the count value is read from the CNTR
> > > register.
> > > 
> > > Since the race condition could result in an incorrect 25-bit count
> > > value, remove support for 25-bit count values from this driver.
> > > 
> > > Fixes: 28e5d3bb0325 ("iio: 104-quad-8: Add IIO support for the ACCES 104-QUAD-8")
> > > Cc: <stable@vger.kernel.org> # 5.15.x
> > > Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
> > > ---
> > >  drivers/counter/104-quad-8.c | 18 +++---------------
> > >  1 file changed, 3 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> > > index 0caa60537b..643aae0c9f 100644
> > > --- a/drivers/counter/104-quad-8.c
> > > +++ b/drivers/counter/104-quad-8.c
> > > @@ -61,10 +61,6 @@ struct quad8 {
> > >  #define QUAD8_REG_CHAN_OP 0x11
> > >  #define QUAD8_REG_INDEX_INPUT_LEVELS 0x16
> > >  #define QUAD8_DIFF_ENCODER_CABLE_STATUS 0x17
> > > -/* Borrow Toggle flip-flop */
> > > -#define QUAD8_FLAG_BT BIT(0)
> > > -/* Carry Toggle flip-flop */
> > > -#define QUAD8_FLAG_CT BIT(1)
> > >  /* Error flag */
> > >  #define QUAD8_FLAG_E BIT(4)
> > >  /* Up/Down flag */
> > > @@ -121,17 +117,9 @@ static int quad8_count_read(struct counter_device *counter,
> > >  {
> > >  	struct quad8 *const priv = counter->priv;
> > >  	const int base_offset = priv->base + 2 * count->id;
> > > -	unsigned int flags;
> > > -	unsigned int borrow;
> > > -	unsigned int carry;
> > >  	int i;
> > >  
> > > -	flags = inb(base_offset + 1);
> > > -	borrow = flags & QUAD8_FLAG_BT;
> > > -	carry = !!(flags & QUAD8_FLAG_CT);
> > > -
> > > -	/* Borrow XOR Carry effectively doubles count range */
> > > -	*val = (unsigned long)(borrow ^ carry) << 24;
> > > +	*val = 0;
> > >  
> > >  	mutex_lock(&priv->lock);
> > >  
> > > @@ -699,8 +687,8 @@ static ssize_t quad8_count_ceiling_read(struct counter_device *counter,
> > >  
> > >  	mutex_unlock(&priv->lock);
> > >  
> > > -	/* By default 0x1FFFFFF (25 bits unsigned) is maximum count */
> > > -	return sprintf(buf, "33554431\n");
> > > +	/* By default 0xFFFFFF (24 bits unsigned) is maximum count */
> > > +	return sprintf(buf, "16777215\n");
> > >  }
> > >  
> > >  static ssize_t quad8_count_ceiling_write(struct counter_device *counter,
> > > 
> > > base-commit: d86dfc4d95cd218246b10ca7adf22c8626547599
> > > -- 
> > > 2.39.2
> > 
> > Greg,
> > 
> > This patch will no longer apply to 5.15.x when the "counter: Internalize
> > sysfs interface code" patch in the stable-queue tree is merged [0].
> > However, I believe the 6.1 backport [1] will apply instead at that
> > point. What is the best way to handle this situation? Should I resend
> > the 6.1 backport with the stable list Cc tag adjusted for 5.15.x, or are
> > you able to apply the 6.1 backport patch directly to the 5.15.x tree?
> 
> The 6.1.y backport didn't apply either :(
> 
> Can you resend all of these rebased against the next round of stable
> releases when they are released later this week?
> 
> thanks,
> 
> greg k-h

Sure, I'll rebase and resend these after the next round of stable
releases is available.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]