Re: [PATCH 3/9 v2] rtc: rtc-hid-sensor-time: delay registering as rtc into a work

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 20 Jun 2013 12:39:36 +0200 Alexander Holler <holler@ahsoftware.de> wrote:

> rtc_device_register() might want to read the clock which doesn't work
> before the hid device is registered. Therefor we delay the registration of
> the rtc driver by moving it to a work.
> 


> --- a/drivers/rtc/rtc-hid-sensor-time.c
> +++ b/drivers/rtc/rtc-hid-sensor-time.c
> @@ -37,6 +37,11 @@ enum hid_time_channel {
>  	TIME_RTC_CHANNEL_MAX,
>  };
>  
> +struct hid_time_workts {
	
Strange name.  I can't work out what the "ts" means.
	
> +	struct work_struct work;
> +	struct hid_time_state *time_state;
> +};
> +
>  struct hid_time_state {
>  	struct hid_sensor_hub_callbacks callbacks;
>  	struct hid_sensor_common common_attributes;
>
> ...
>
> @@ -237,6 +243,36 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
>  	.read_time = hid_rtc_read_time,
>  };
>  
> +static void hid_time_register_rtc_work(struct work_struct *work)
> +{
> +	struct hid_time_state *time_state =
> +		container_of(work, struct hid_time_workts, work)
> +			->time_state;
> +	struct platform_device *pdev = time_state->callbacks.pdev;

Ick.  When the initialisers overflow 80 cols, the fix is easy: don't
use initalisers:

	struct hid_time_state *time_state;
	struct platform_device *pdev;

	time_state = container_of(work, struct hid_time_workts, work)->time_state;
	pdev = time_state->callbacks.pdev;

> +	time_state->rtc = devm_rtc_device_register(&pdev->dev,
> +					"hid-sensor-time", &hid_time_rtc_ops,
> +					THIS_MODULE);
> +	if (IS_ERR_OR_NULL(time_state->rtc)) {
> +		struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;

Newline after end-of-definitions and before start-of-code, please.

> +		sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
> +		time_state->rtc = NULL;
> +		dev_err(&pdev->dev, "rtc device register failed!\n");
> +		/*
> +		 *  I haven't a found a way to remove only this device from
> +		 *  hid-sensor-hub. Removing the device a level above (the
> +		 *  complete HID device) doesn't work, because a sensor-hub
> +		 *  might provide more than just a time-sensor and thus we
> +		 *  would remove all sensors not just this one.
> +		 *  So we just leave this driver idling around until I or
> +		 *  someone else has figured out how to remove this device
> +		 *  from hid-sensor-hub.
> +		 */
> +	}
> +	time_state->workts = NULL;
> +	kfree(work);
> +}
> +
>  static int hid_time_probe(struct platform_device *pdev)
>  {
>  	int ret = 0;
> @@ -279,22 +315,34 @@ static int hid_time_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	time_state->rtc = devm_rtc_device_register(&pdev->dev,
> -					"hid-sensor-time", &hid_time_rtc_ops,
> -					THIS_MODULE);
> -
> -	if (IS_ERR(time_state->rtc)) {
> -		dev_err(&pdev->dev, "rtc device register failed!\n");
> -		return PTR_ERR(time_state->rtc);
> +	/*
> +	 * The HID device has to be registered to read the clock.
> +	 * Because rtc_device_register() might read the time, we have to delay
> +	 * rtc_device_register() to a work in order to finish the probe before.
> +	 */
> +	time_state->workts = kmalloc(sizeof(struct hid_time_workts),
> +		GFP_KERNEL);
> +	if (time_state->workts == NULL) {
> +		sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
> +		return -ENOMEM;
>  	}
> +	time_state->workts->time_state = time_state;
> +	INIT_WORK(&time_state->workts->work,
> +			hid_time_register_rtc_work);
> +	schedule_work(&time_state->workts->work);

This seems unreliable.  The scheduled work can run one nanosecond
later, on this or a different CPU.  What guarantees that the HID device
will then be fully registered?