Re: [PATCH 09/10] cfg80211: move regulatory hints to workqueue

[Johannes Berg <johannes@xxxxxxxxxxxxxxxx>]

On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:

> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -396,6 +396,7 @@ enum environment_cap {
>   * 	country IE
>   * @country_ie_env: lets us know if the AP is telling us we are outdoor,
>   * 	indoor, or if it doesn't matter
> + * @list: used to insert into a linked list

I wouldn't mind it explaining which list ;)

> +static LIST_HEAD(reg_requests_list);
> +static DEFINE_MUTEX(reg_mutex);
> +
> +static void reg_process_pending_hints(void);

can you reorder the code in a way that doesn't require forward
declarations?

> +/* This currently only processes user and driver regulatory hints */
> +static int reg_process_hint(struct regulatory_request *reg_request)
> +{

why bother offloading _user_ hints to a workqueue? That seems a little
pointless since we can sleep there and do whatever. OTOH, maybe _all_
should be offloaded so the order is correct?

> +static void reg_process_pending_hints(void)
> +	{
> +	struct regulatory_request *reg_request, *tmp;
> +	int r;
> +
> +	mutex_lock(&reg_mutex);
> +	list_for_each_entry_safe(reg_request, tmp, &reg_requests_list, list) {
> +		r = reg_process_hint(reg_request);
> +		if (r)
> +			printk(KERN_ERR "cfg80211: wiphy_idx %d sent a "
> +				"regulatory hint but now has gone fishing, "
> +				"ignoring request\n", reg_request->wiphy_idx);

pointless warning, it'll happen if somebody just inserts/unplugs very
quickly on a loaded system.

> +		list_del(&reg_request->list);
> +		kfree(reg_request);
> +	}
> +	mutex_unlock(&reg_mutex);
> +}

Are you sure the mutex order reg_mutex --> cfg80211_mutex is a good
idea? Is there a need for two locks at all?

johannes

Attachment: signature.asc
Description: This is a digitally signed message part