Re: [RFC/RFT v2] rfkill: rewrite

[Larry Finger <Larry.Finger@lwfinger.net>]

Johannes Berg wrote:
> This patch completely rewrites the rfkill core to address
> the following deficiencies:

-- snip --

> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ wireless-testing/net/rfkill/core.c	2009-03-31 00:04:25.000000000 +0200
> @@ -0,0 +1,752 @@
> +/*

-- snip --

> +int __must_check rfkill_register(struct rfkill *rfkill)
> +{
> +	static unsigned long rfkill_no = 0;
> +	struct device *dev = &rfkill->dev;
> +	int error;
> +
> +	BUG_ON(!rfkill);
> +
> +	mutex_lock(&rfkill_global_mutex);
> +
> +	if (rfkill->registered) {
> +		error = -EALREADY;
> +		goto unlock;
> +	}
> +
> +	dev_set_name(dev, "rfkill%lu", rfkill_no);
> +	rfkill_no++;
> +
> +	if (!(rfkill_states_default_locked & BIT(rfkill->type))) {
> +		/* first of its kind */
> +		BUILD_BUG_ON(NUM_RFKILL_TYPES >
> +			sizeof(rfkill_states_default_locked) * 8);
> +		rfkill_states_default_locked |= BIT(rfkill->type);
> +		rfkill_global_states[rfkill->type].cur =
> +			rfkill_global_states[rfkill->type].def;
> +	}
> +
> +	/* XXX: schedule work to set default state */
> +
> +	list_add_tail(&rfkill->node, &rfkill_list);
> +
> +	error = device_add(dev);
> +	if (error)
> +		goto remove;
> +
> +	error = rfkill_led_trigger_register(rfkill);
> +	if (error)
> +		goto devdel;
> +
> +	rfkill->registered = true;
> +
> +	if (rfkill->ops->poll_hw_block) {
> +		INIT_DELAYED_WORK(&rfkill->poll_work, rfkill_poll);
> +		schedule_delayed_work(&rfkill->poll_work,
> +			round_jiffies_relative(POLL_INTERVAL));
> +	}
> +
> +	INIT_WORK(&rfkill->uevent_work, rfkill_uevent_work);
> +
> +	return 0;

The locking looks funny here. The normal return leaves the rfkill_global_mutex
locked. Did you mean "goto unlock" instead of "return 0"?

> +
> + devdel:
> +	device_del(&rfkill->dev);
> + remove:
> +	list_del_init(&rfkill->node);
> + unlock:
> + 	mutex_unlock(&rfkill_global_mutex);
> +	return error;
> +}

I was alerted to this problem when I was unable to shut down or reboot normally.
Those problems are fixed with the patch

Index: wireless-testing/net/rfkill/core.c
===================================================================
--- wireless-testing.orig/net/rfkill/core.c
+++ wireless-testing/net/rfkill/core.c
@@ -676,7 +676,7 @@ int __must_check rfkill_register(struct

 	INIT_WORK(&rfkill->uevent_work, rfkill_uevent_work);

-	return 0;
+	goto unlock;

  devdel:
        device_del(&rfkill->dev);


Larry