On 13/05/15 15:04, Lars-Peter Clausen wrote:
> Currently __iio_update_buffers() verifies whether the new configuration
> will work in the middle of the update sequence. This means if the new
> configuration is invalid we need to rollback the changes already made. This
> patch moves the validation of the new configuration at the beginning of
> __iio_update_buffers() and will not start to make any changes if the new
> configuration is invalid.
It's funny how code evolves sometimes. It never occurred to me that
we could do this before actually unwinding the buffers but of course
we can. :)
>
> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
This one looks good, will wait on responses to the previous before taking
it however.
> ---
> drivers/iio/industrialio-buffer.c | 164 +++++++++++++++++++++++---------------
> 1 file changed, 100 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 8ecba2f..68e2669 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -603,20 +603,104 @@ static void iio_free_scan_mask(struct iio_dev *indio_dev,
> kfree(mask);
> }
>
> +struct iio_device_config {
> + unsigned int mode;
> + const unsigned long *scan_mask;
> + unsigned int scan_bytes;
> + bool scan_timestamp;
> +};
> +
> +static int iio_verify_update(struct iio_dev *indio_dev,
> + struct iio_buffer *insert_buffer, struct iio_buffer *remove_buffer,
> + struct iio_device_config *config)
> +{
> + unsigned long *compound_mask;
> + const unsigned long *scan_mask;
> + struct iio_buffer *buffer;
> + bool scan_timestamp;
> +
> + memset(config, 0, sizeof(*config));
> +
> + /*
> + * If there is just one buffer and we are removing it there is nothing
> + * to verify.
> + */
> + if (remove_buffer && !insert_buffer &&
> + list_is_singular(&indio_dev->buffer_list))
> + return 0;
> +
> + /* Definitely possible for devices to support both of these. */
> + if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
> + config->mode = INDIO_BUFFER_TRIGGERED;
> + } else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
> + config->mode = INDIO_BUFFER_HARDWARE;
> + } else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
> + config->mode = INDIO_BUFFER_SOFTWARE;
> + } else {
> + /* Can only occur on first buffer */
> + if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
> + dev_dbg(&indio_dev->dev, "Buffer not started: no trigger\n");
> + return -EINVAL;
> + }
> +
> + /* What scan mask do we actually have? */
> + compound_mask = kcalloc(BITS_TO_LONGS(indio_dev->masklength),
> + sizeof(long), GFP_KERNEL);
> + if (compound_mask == NULL)
> + return -ENOMEM;
> +
> + scan_timestamp = false;
> +
> + list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> + if (buffer == remove_buffer)
> + continue;
> + bitmap_or(compound_mask, compound_mask, buffer->scan_mask,
> + indio_dev->masklength);
> + scan_timestamp |= buffer->scan_timestamp;
> + }
> +
> + if (insert_buffer) {
> + bitmap_or(compound_mask, compound_mask,
> + insert_buffer->scan_mask, indio_dev->masklength);
> + scan_timestamp |= insert_buffer->scan_timestamp;
> + }
> +
> + if (indio_dev->available_scan_masks) {
> + scan_mask = iio_scan_mask_match(indio_dev->available_scan_masks,
> + indio_dev->masklength,
> + compound_mask);
> + kfree(compound_mask);
> + if (scan_mask == NULL)
> + return -EINVAL;
> + } else {
> + scan_mask = compound_mask;
> + }
> +
> + config->scan_bytes = iio_compute_scan_bytes(indio_dev,
> + scan_mask, scan_timestamp);
> + config->scan_mask = scan_mask;
> + config->scan_timestamp = scan_timestamp;
> +
> + return 0;
> +}
> +
> static int __iio_update_buffers(struct iio_dev *indio_dev,
> struct iio_buffer *insert_buffer,
> struct iio_buffer *remove_buffer)
> {
> int ret;
> - int success = 0;
> - struct iio_buffer *buffer;
> - unsigned long *compound_mask;
> const unsigned long *old_mask;
> + struct iio_device_config new_config;
> +
> + ret = iio_verify_update(indio_dev, insert_buffer, remove_buffer,
> + &new_config);
> + if (ret)
> + return ret;
>
> if (insert_buffer) {
> ret = iio_buffer_request_update(indio_dev, insert_buffer);
> if (ret)
> - return ret;
> + goto err_free_config;
> }
>
> /* Wind down existing buffers - iff there are any */
> @@ -624,13 +708,13 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
> if (indio_dev->setup_ops->predisable) {
> ret = indio_dev->setup_ops->predisable(indio_dev);
> if (ret)
> - return ret;
> + goto err_free_config;
> }
>
> if (indio_dev->setup_ops->postdisable) {
> ret = indio_dev->setup_ops->postdisable(indio_dev);
> if (ret)
> - return ret;
> + goto err_free_config;
> }
>
> indio_dev->currentmode = INDIO_DIRECT_MODE;
> @@ -652,59 +736,10 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
> return 0;
> }
>
> - /* What scan mask do we actually have? */
> - compound_mask = kcalloc(BITS_TO_LONGS(indio_dev->masklength),
> - sizeof(long), GFP_KERNEL);
> - if (compound_mask == NULL) {
> - iio_free_scan_mask(indio_dev, old_mask);
> - return -ENOMEM;
> - }
> - indio_dev->scan_timestamp = 0;
> -
> - list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
> - bitmap_or(compound_mask, compound_mask, buffer->scan_mask,
> - indio_dev->masklength);
> - indio_dev->scan_timestamp |= buffer->scan_timestamp;
> - }
> - if (indio_dev->available_scan_masks) {
> - indio_dev->active_scan_mask =
> - iio_scan_mask_match(indio_dev->available_scan_masks,
> - indio_dev->masklength,
> - compound_mask);
> - kfree(compound_mask);
> - if (indio_dev->active_scan_mask == NULL) {
> - /*
> - * Roll back.
> - * Note can only occur when adding a buffer.
> - */
> - iio_buffer_deactivate(insert_buffer);
> - if (old_mask) {
> - indio_dev->active_scan_mask = old_mask;
> - success = -EINVAL;
> - }
> - else {
> - ret = -EINVAL;
> - return ret;
> - }
> - }
> - } else {
> - indio_dev->active_scan_mask = compound_mask;
> - }
> -
> - /* Definitely possible for devices to support both of these. */
> - if ((indio_dev->modes & INDIO_BUFFER_TRIGGERED) && indio_dev->trig) {
> - indio_dev->currentmode = INDIO_BUFFER_TRIGGERED;
> - } else if (indio_dev->modes & INDIO_BUFFER_HARDWARE) {
> - indio_dev->currentmode = INDIO_BUFFER_HARDWARE;
> - } else if (indio_dev->modes & INDIO_BUFFER_SOFTWARE) {
> - indio_dev->currentmode = INDIO_BUFFER_SOFTWARE;
> - } else { /* Should never be reached */
> - /* Can only occur on first buffer */
> - if (indio_dev->modes & INDIO_BUFFER_TRIGGERED)
> - dev_dbg(&indio_dev->dev, "Buffer not started: no trigger\n");
> - ret = -EINVAL;
> - goto error_remove_inserted;
> - }
> + indio_dev->currentmode = new_config.mode;
> + indio_dev->active_scan_mask = new_config.scan_mask;
> + indio_dev->scan_timestamp = new_config.scan_timestamp;
> + indio_dev->scan_bytes = new_config.scan_bytes;
>
> iio_update_demux(indio_dev);
>
> @@ -717,10 +752,7 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
> goto error_remove_inserted;
> }
> }
> - indio_dev->scan_bytes =
> - iio_compute_scan_bytes(indio_dev,
> - indio_dev->active_scan_mask,
> - indio_dev->scan_timestamp);
> +
> if (indio_dev->info->update_scan_mode) {
> ret = indio_dev->info
> ->update_scan_mode(indio_dev,
> @@ -744,7 +776,7 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
>
> iio_free_scan_mask(indio_dev, old_mask);
>
> - return success;
> + return 0;
>
> error_run_postdisable:
> if (indio_dev->setup_ops->postdisable)
> @@ -756,6 +788,10 @@ error_remove_inserted:
> iio_free_scan_mask(indio_dev, indio_dev->active_scan_mask);
> indio_dev->active_scan_mask = old_mask;
> return ret;
> +
> +err_free_config:
> + iio_free_scan_mask(indio_dev, new_config.scan_mask);
> + return ret;
> }
>
> int iio_update_buffers(struct iio_dev *indio_dev,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
