On Tue, May 10, 2011 at 07:57:12AM -0700, K. Y. Srinivasan wrote:
> Get rid of the function count_hv_channel()
> by inlining the code.
Close, but not quite:
> +static atomic_t num_channels;
Why is this atomic?
> struct vmbus_channel_message_table_entry {
> enum vmbus_channel_message_type message_type;
> void (*message_handler)(struct vmbus_channel_message_header *msg);
> @@ -316,21 +318,6 @@ void free_channel(struct vmbus_channel *channel)
> DECLARE_COMPLETION(hv_channel_ready);
>
> /*
> - * Count initialized channels, and ensure all channels are ready when hv_vmbus
> - * module loading completes.
> - */
> -static void count_hv_channel(void)
> -{
> - static int counter;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
> - if (++counter == MAX_MSG_TYPES)
> - complete(&hv_channel_ready);
> - spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
> -}
> -
> -/*
> * vmbus_process_rescind_offer -
> * Rescind the offer by initiating a device removal
> */
> @@ -433,7 +420,16 @@ static void vmbus_process_offer(struct work_struct *work)
>
> pr_info("%s\n", hv_cb_utils[cnt].log_msg);
>
> - count_hv_channel();
> + /*
> + * Count initialized channels, and ensure
> + * all channels are ready when hv_vmbus
> + * module loading completes.
> + */
> +
> + atomic_inc(&num_channels);
> + if (atomic_read(&num_channels)
> + == MAX_MSG_TYPES)
> + complete(&hv_channel_ready);
I don't see why this needs to be atomic, if you properly lock things it
can just be a variable, right?
And what's copying this from ever terminating if the count never
increments? This logic is very strange and should be reworked to not do
this kind of "loop until something else counts high enough" code.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
