Re: [PATCH] Testing: counter: 104-quad-8.c: Added lock protection

From: William Breathitt Gray
Date: Sun Mar 08 2020 - 10:20:31 EST

Next message: Tobias Schramm: "[RFC PATCH 0/1] Add generic inverted led triggers"
Previous message: Russell King - ARM Linux admin: "Re: [PATCH] vfs: keep inodes with page cache off the inode shrinker LRU"
In reply to: Syed Waris: "[PATCH] Testing: counter: 104-quad-8.c: Added lock protection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>

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
>

Attachment: signature.asc
Description: PGP signature

Next message: Tobias Schramm: "[RFC PATCH 0/1] Add generic inverted led triggers"
Previous message: Russell King - ARM Linux admin: "Re: [PATCH] vfs: keep inodes with page cache off the inode shrinker LRU"
In reply to: Syed Waris: "[PATCH] Testing: counter: 104-quad-8.c: Added lock protection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]