| Message ID | 20200308104101.GA18548@syed.domain.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Testing: counter: 104-quad-8.c: Added lock protection | expand |
On Sun, Mar 08, 2020 at 04:11:01PM +0530, Syed Waris wrote: > Added protection for quad8_iio configurations from race conditions in > the 104-quad-8 counter driver. There are no IRQs, used spin-locks for > protection. > > Signed-off-by: Syed Waris <syednwaris@gmail.com> Hi Syed, Thank you for your submission. I have some changes that should be made to this patch before we accept it. When you write a commit message and title, keep it in the present tense; you can also remove the "Testing: " portion of the title since it's not needed: counter: 104-quad-8: Add lock protection Add protection for quad8_iio configurations from race conditions in the 104-quad-8 counter driver. There is no IRQ handling so spin_lock calls are used for protection. As for the code changes in this patch, it will be good to protect all the port I/O calls (outb/inb) in this driver in addition to the quad8_iio configurations. The reason is that ACCES 104-QUAD-8 devices use a byte pointer register to coordinate access to the data registers, so these register states should be kept locked so that they are not corrupted while accessing and modifying data on the device. William Breathitt Gray > --- > drivers/counter/104-quad-8.c | 61 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c > index 0cfc813..cd8e09f 100644 > --- a/drivers/counter/104-quad-8.c > +++ b/drivers/counter/104-quad-8.c > @@ -43,6 +43,7 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses"); > */ > struct quad8_iio { > struct counter_device counter; > + spinlock_t lock; > unsigned int fck_prescaler[QUAD8_NUM_COUNTERS]; > unsigned int preset[QUAD8_NUM_COUNTERS]; > unsigned int count_mode[QUAD8_NUM_COUNTERS]; > @@ -185,6 +186,8 @@ static int quad8_write_raw(struct iio_dev *indio_dev, > if (val < 0 || val > 1) > return -EINVAL; > > + spin_lock(&priv->lock); > + > priv->ab_enable[chan->channel] = val; > > ior_cfg = val | priv->preset_enable[chan->channel] << 1; > @@ -192,6 +195,8 @@ static int quad8_write_raw(struct iio_dev *indio_dev, > /* Load I/O control configuration */ > outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1); > > + spin_unlock(&priv->lock); > + > return 0; > case IIO_CHAN_INFO_SCALE: > /* Quadrature scaling only available in quadrature mode */ > @@ -251,6 +256,8 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private, > if (preset > 0xFFFFFF) > return -EINVAL; > > + spin_lock(&priv->lock); > + > priv->preset[chan->channel] = preset; > > /* Reset Byte Pointer */ > @@ -260,6 +267,8 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private, > for (i = 0; i < 3; i++) > outb(preset >> (8 * i), base_offset); > > + spin_unlock(&priv->lock); > + > return len; > } > > @@ -289,6 +298,8 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev, > /* Preset enable is active low in Input/Output Control register */ > preset_enable = !preset_enable; > > + spin_lock(&priv->lock); > + > priv->preset_enable[chan->channel] = preset_enable; > > ior_cfg = priv->ab_enable[chan->channel] | > @@ -297,6 +308,8 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev, > /* Load I/O control configuration to Input / Output Control Register */ > outb(QUAD8_CTR_IOR | ior_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return len; > } > > @@ -354,6 +367,8 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev, > unsigned int mode_cfg = cnt_mode << 1; > const int base_offset = priv->base + 2 * chan->channel + 1; > > + spin_lock(&priv->lock); > + > priv->count_mode[chan->channel] = cnt_mode; > > /* Add quadrature mode configuration */ > @@ -363,6 +378,8 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev, > /* Load mode configuration to Counter Mode Register */ > outb(QUAD8_CTR_CMR | mode_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return 0; > } > > @@ -398,11 +415,15 @@ static int quad8_set_synchronous_mode(struct iio_dev *indio_dev, > if (synchronous_mode && !priv->quadrature_mode[chan->channel]) > return -EINVAL; > > + spin_lock(&priv->lock); > + > priv->synchronous_mode[chan->channel] = synchronous_mode; > > /* Load Index Control configuration to Index Control Register */ > outb(QUAD8_CTR_IDR | idr_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return 0; > } > > @@ -444,11 +465,15 @@ static int quad8_set_quadrature_mode(struct iio_dev *indio_dev, > quad8_set_synchronous_mode(indio_dev, chan, 0); > } > > + spin_lock(&priv->lock); > + > priv->quadrature_mode[chan->channel] = quadrature_mode; > > /* Load mode configuration to Counter Mode Register */ > outb(QUAD8_CTR_CMR | mode_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return 0; > } > > @@ -480,11 +505,15 @@ static int quad8_set_index_polarity(struct iio_dev *indio_dev, > index_polarity << 1; > const int base_offset = priv->base + 2 * chan->channel + 1; > > + spin_lock(&priv->lock); > + > priv->index_polarity[chan->channel] = index_polarity; > > /* Load Index Control configuration to Index Control Register */ > outb(QUAD8_CTR_IDR | idr_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return 0; > } > > @@ -852,11 +881,15 @@ static int quad8_index_polarity_set(struct counter_device *counter, > index_polarity << 1; > const int base_offset = priv->base + 2 * channel_id + 1; > > + spin_lock(&priv->lock); > + > priv->index_polarity[channel_id] = index_polarity; > > /* Load Index Control configuration to Index Control Register */ > outb(QUAD8_CTR_IDR | idr_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return 0; > } > > @@ -891,11 +924,15 @@ static int quad8_synchronous_mode_set(struct counter_device *counter, > if (synchronous_mode && !priv->quadrature_mode[channel_id]) > return -EINVAL; > > + spin_lock(&priv->lock); > + > priv->synchronous_mode[channel_id] = synchronous_mode; > > /* Load Index Control configuration to Index Control Register */ > outb(QUAD8_CTR_IDR | idr_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return 0; > } > > @@ -960,6 +997,8 @@ static int quad8_count_mode_set(struct counter_device *counter, > break; > } > > + spin_lock(&priv->lock); > + > priv->count_mode[count->id] = cnt_mode; > > /* Set count mode configuration value */ > @@ -972,6 +1011,8 @@ static int quad8_count_mode_set(struct counter_device *counter, > /* Load mode configuration to Counter Mode Register */ > outb(QUAD8_CTR_CMR | mode_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return 0; > } > > @@ -1013,6 +1054,8 @@ static ssize_t quad8_count_enable_write(struct counter_device *counter, > if (err) > return err; > > + spin_lock(&priv->lock); > + > priv->ab_enable[count->id] = ab_enable; > > ior_cfg = ab_enable | priv->preset_enable[count->id] << 1; > @@ -1020,6 +1063,8 @@ static ssize_t quad8_count_enable_write(struct counter_device *counter, > /* Load I/O control configuration */ > outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1); > > + spin_unlock(&priv->lock); > + > return len; > } > > @@ -1065,6 +1110,8 @@ static ssize_t quad8_count_preset_write(struct counter_device *counter, > if (preset > 0xFFFFFF) > return -EINVAL; > > + spin_lock(&priv->lock); > + > priv->preset[count->id] = preset; > > /* Reset Byte Pointer */ > @@ -1074,6 +1121,8 @@ static ssize_t quad8_count_preset_write(struct counter_device *counter, > for (i = 0; i < 3; i++) > outb(preset >> (8 * i), base_offset); > > + spin_unlock(&priv->lock); > + > return len; > } > > @@ -1133,6 +1182,8 @@ static ssize_t quad8_count_preset_enable_write(struct counter_device *counter, > /* Preset enable is active low in Input/Output Control register */ > preset_enable = !preset_enable; > > + spin_lock(&priv->lock); > + > priv->preset_enable[count->id] = preset_enable; > > ior_cfg = priv->ab_enable[count->id] | (unsigned int)preset_enable << 1; > @@ -1140,6 +1191,8 @@ static ssize_t quad8_count_preset_enable_write(struct counter_device *counter, > /* Load I/O control configuration to Input / Output Control Register */ > outb(QUAD8_CTR_IOR | ior_cfg, base_offset); > > + spin_unlock(&priv->lock); > + > return len; > } > > @@ -1166,6 +1219,8 @@ static ssize_t quad8_signal_fck_prescaler_write(struct counter_device *counter, > if (ret) > return ret; > > + spin_lock(&priv->lock); > + > priv->fck_prescaler[channel_id] = prescaler; > > /* Reset Byte Pointer */ > @@ -1176,6 +1231,8 @@ static ssize_t quad8_signal_fck_prescaler_write(struct counter_device *counter, > outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_PRESET_PSC, > base_offset + 1); > > + spin_unlock(&priv->lock); > + > return len; > } > > @@ -1383,6 +1440,10 @@ static int quad8_probe(struct device *dev, unsigned int id) > /* Disable index function; negative index polarity */ > outb(QUAD8_CTR_IDR, base_offset + 1); > } > + > + /* Initialize the spin lock */ > + spin_lock_init(&quad8iio->lock); > + > /* Enable all counters */ > outb(QUAD8_CHAN_OP_ENABLE_COUNTERS, base[id] + QUAD8_REG_CHAN_OP); > > -- > 2.7.4 >
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c index 0cfc813..cd8e09f 100644 --- a/drivers/counter/104-quad-8.c +++ b/drivers/counter/104-quad-8.c @@ -43,6 +43,7 @@ MODULE_PARM_DESC(base, "ACCES 104-QUAD-8 base addresses"); */ struct quad8_iio { struct counter_device counter; + spinlock_t lock; unsigned int fck_prescaler[QUAD8_NUM_COUNTERS]; unsigned int preset[QUAD8_NUM_COUNTERS]; unsigned int count_mode[QUAD8_NUM_COUNTERS]; @@ -185,6 +186,8 @@ static int quad8_write_raw(struct iio_dev *indio_dev, if (val < 0 || val > 1) return -EINVAL; + spin_lock(&priv->lock); + priv->ab_enable[chan->channel] = val; ior_cfg = val | priv->preset_enable[chan->channel] << 1; @@ -192,6 +195,8 @@ static int quad8_write_raw(struct iio_dev *indio_dev, /* Load I/O control configuration */ outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1); + spin_unlock(&priv->lock); + return 0; case IIO_CHAN_INFO_SCALE: /* Quadrature scaling only available in quadrature mode */ @@ -251,6 +256,8 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private, if (preset > 0xFFFFFF) return -EINVAL; + spin_lock(&priv->lock); + priv->preset[chan->channel] = preset; /* Reset Byte Pointer */ @@ -260,6 +267,8 @@ static ssize_t quad8_write_preset(struct iio_dev *indio_dev, uintptr_t private, for (i = 0; i < 3; i++) outb(preset >> (8 * i), base_offset); + spin_unlock(&priv->lock); + return len; } @@ -289,6 +298,8 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev, /* Preset enable is active low in Input/Output Control register */ preset_enable = !preset_enable; + spin_lock(&priv->lock); + priv->preset_enable[chan->channel] = preset_enable; ior_cfg = priv->ab_enable[chan->channel] | @@ -297,6 +308,8 @@ static ssize_t quad8_write_set_to_preset_on_index(struct iio_dev *indio_dev, /* Load I/O control configuration to Input / Output Control Register */ outb(QUAD8_CTR_IOR | ior_cfg, base_offset); + spin_unlock(&priv->lock); + return len; } @@ -354,6 +367,8 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev, unsigned int mode_cfg = cnt_mode << 1; const int base_offset = priv->base + 2 * chan->channel + 1; + spin_lock(&priv->lock); + priv->count_mode[chan->channel] = cnt_mode; /* Add quadrature mode configuration */ @@ -363,6 +378,8 @@ static int quad8_set_count_mode(struct iio_dev *indio_dev, /* Load mode configuration to Counter Mode Register */ outb(QUAD8_CTR_CMR | mode_cfg, base_offset); + spin_unlock(&priv->lock); + return 0; } @@ -398,11 +415,15 @@ static int quad8_set_synchronous_mode(struct iio_dev *indio_dev, if (synchronous_mode && !priv->quadrature_mode[chan->channel]) return -EINVAL; + spin_lock(&priv->lock); + priv->synchronous_mode[chan->channel] = synchronous_mode; /* Load Index Control configuration to Index Control Register */ outb(QUAD8_CTR_IDR | idr_cfg, base_offset); + spin_unlock(&priv->lock); + return 0; } @@ -444,11 +465,15 @@ static int quad8_set_quadrature_mode(struct iio_dev *indio_dev, quad8_set_synchronous_mode(indio_dev, chan, 0); } + spin_lock(&priv->lock); + priv->quadrature_mode[chan->channel] = quadrature_mode; /* Load mode configuration to Counter Mode Register */ outb(QUAD8_CTR_CMR | mode_cfg, base_offset); + spin_unlock(&priv->lock); + return 0; } @@ -480,11 +505,15 @@ static int quad8_set_index_polarity(struct iio_dev *indio_dev, index_polarity << 1; const int base_offset = priv->base + 2 * chan->channel + 1; + spin_lock(&priv->lock); + priv->index_polarity[chan->channel] = index_polarity; /* Load Index Control configuration to Index Control Register */ outb(QUAD8_CTR_IDR | idr_cfg, base_offset); + spin_unlock(&priv->lock); + return 0; } @@ -852,11 +881,15 @@ static int quad8_index_polarity_set(struct counter_device *counter, index_polarity << 1; const int base_offset = priv->base + 2 * channel_id + 1; + spin_lock(&priv->lock); + priv->index_polarity[channel_id] = index_polarity; /* Load Index Control configuration to Index Control Register */ outb(QUAD8_CTR_IDR | idr_cfg, base_offset); + spin_unlock(&priv->lock); + return 0; } @@ -891,11 +924,15 @@ static int quad8_synchronous_mode_set(struct counter_device *counter, if (synchronous_mode && !priv->quadrature_mode[channel_id]) return -EINVAL; + spin_lock(&priv->lock); + priv->synchronous_mode[channel_id] = synchronous_mode; /* Load Index Control configuration to Index Control Register */ outb(QUAD8_CTR_IDR | idr_cfg, base_offset); + spin_unlock(&priv->lock); + return 0; } @@ -960,6 +997,8 @@ static int quad8_count_mode_set(struct counter_device *counter, break; } + spin_lock(&priv->lock); + priv->count_mode[count->id] = cnt_mode; /* Set count mode configuration value */ @@ -972,6 +1011,8 @@ static int quad8_count_mode_set(struct counter_device *counter, /* Load mode configuration to Counter Mode Register */ outb(QUAD8_CTR_CMR | mode_cfg, base_offset); + spin_unlock(&priv->lock); + return 0; } @@ -1013,6 +1054,8 @@ static ssize_t quad8_count_enable_write(struct counter_device *counter, if (err) return err; + spin_lock(&priv->lock); + priv->ab_enable[count->id] = ab_enable; ior_cfg = ab_enable | priv->preset_enable[count->id] << 1; @@ -1020,6 +1063,8 @@ static ssize_t quad8_count_enable_write(struct counter_device *counter, /* Load I/O control configuration */ outb(QUAD8_CTR_IOR | ior_cfg, base_offset + 1); + spin_unlock(&priv->lock); + return len; } @@ -1065,6 +1110,8 @@ static ssize_t quad8_count_preset_write(struct counter_device *counter, if (preset > 0xFFFFFF) return -EINVAL; + spin_lock(&priv->lock); + priv->preset[count->id] = preset; /* Reset Byte Pointer */ @@ -1074,6 +1121,8 @@ static ssize_t quad8_count_preset_write(struct counter_device *counter, for (i = 0; i < 3; i++) outb(preset >> (8 * i), base_offset); + spin_unlock(&priv->lock); + return len; } @@ -1133,6 +1182,8 @@ static ssize_t quad8_count_preset_enable_write(struct counter_device *counter, /* Preset enable is active low in Input/Output Control register */ preset_enable = !preset_enable; + spin_lock(&priv->lock); + priv->preset_enable[count->id] = preset_enable; ior_cfg = priv->ab_enable[count->id] | (unsigned int)preset_enable << 1; @@ -1140,6 +1191,8 @@ static ssize_t quad8_count_preset_enable_write(struct counter_device *counter, /* Load I/O control configuration to Input / Output Control Register */ outb(QUAD8_CTR_IOR | ior_cfg, base_offset); + spin_unlock(&priv->lock); + return len; } @@ -1166,6 +1219,8 @@ static ssize_t quad8_signal_fck_prescaler_write(struct counter_device *counter, if (ret) return ret; + spin_lock(&priv->lock); + priv->fck_prescaler[channel_id] = prescaler; /* Reset Byte Pointer */ @@ -1176,6 +1231,8 @@ static ssize_t quad8_signal_fck_prescaler_write(struct counter_device *counter, outb(QUAD8_CTR_RLD | QUAD8_RLD_RESET_BP | QUAD8_RLD_PRESET_PSC, base_offset + 1); + spin_unlock(&priv->lock); + return len; } @@ -1383,6 +1440,10 @@ static int quad8_probe(struct device *dev, unsigned int id) /* Disable index function; negative index polarity */ outb(QUAD8_CTR_IDR, base_offset + 1); } + + /* Initialize the spin lock */ + spin_lock_init(&quad8iio->lock); + /* Enable all counters */ outb(QUAD8_CHAN_OP_ENABLE_COUNTERS, base[id] + QUAD8_REG_CHAN_OP);
Added protection for quad8_iio configurations from race conditions in the 104-quad-8 counter driver. There are no IRQs, used spin-locks for protection. Signed-off-by: Syed Waris <syednwaris@gmail.com> --- drivers/counter/104-quad-8.c | 61 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)