Re: [PATCH 3/3] rtc: rtc-hid-sensor-time; add option hctosys to settime at boot

[Andrew Morton]

On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler <holler@xxxxxxxxxxxxx> wrote:

> drivers/rtc/hctosys (CONFIG_RTC_HCTOSYS) doesn't work for
> rtc-hid-sensor-time because it will be called in late_init, and thus before
> rtc-hid-sensor-time gets loaded.

Isn't that true of all RTC drivers which are built as modules?  There's
nothing special about hid-sensor-time here?

I assume the standard answer here is "your RTC driver should be built
into vmlinux".  If we wish to make things work for modular RTC drivers
then we should find a solution which addresses *all* RTC drivers?

> To set the time through rtc-hid-sensor-time
> at startup, the module now checks by default if the system time is before
> 1970-01-02 and sets the system time (once) if this is the case.
> 
> To disable this behaviour, set the module option hctosys to zero, e.g. by
> using rtc-hid-sensor-time.hctosys=0 at the kernel command line if the
> driver is statically linked into the kernel.

Is a bit hacky, no?

> @@ -237,6 +279,22 @@ static const struct rtc_class_ops hid_time_rtc_ops = {
>  	.read_time = hid_rtc_read_time,
>  };
>  
> +struct hid_time_work_time_state {
> +	struct work_struct work;
> +	struct hid_time_state *time_state;
> +};
> +
> +static void hid_time_request_report_work(struct work_struct *work)
> +{
> +	struct hid_time_state *time_state =
> +		((struct hid_time_work_time_state *)work)->time_state;

Yikes.  Use container_of() here.

Also, you don't *have* to initialise things at their definition site.  So

	struct hid_time_state *time_state =
		some-ginormous-expression-which-overflows-80-columns;

becomes

	struct hid_time_state *time_state;
	time_state = some-ginormous-expression-which-no-longer-overflows-80-columns;

Simple, no?

> +	/* get a report with all values through requesting one value */
> +	sensor_hub_input_attr_get_raw_value(
> +		time_state->common_attributes.hsdev, HID_USAGE_SENSOR_TIME,
> +		hid_time_addresses[0], time_state->info[0].report_id);
> +	kfree(work);
> +}
> +
>  static int hid_time_probe(struct platform_device *pdev)
>  {
>  	int ret = 0;
> @@ -287,6 +345,20 @@ static int hid_time_probe(struct platform_device *pdev)
>  		return PTR_ERR(time_state->rtc);
>  	}
>  
> +	if (!hid_time_time_set_once && hid_time_hctosys_enabled) {
> +		/*
> +		 * Request a HID report to set the time.
> +		 * Calling sensor_hub_..._get_raw_value() here directly
> +		 * doesn't work, therefor we have to use a work.
> +		 */
> +		struct hid_time_work_time_state *hdwork =
> +			kmalloc(sizeof(struct hid_time_work_time_state),
> +				GFP_KERNEL);

looky:

		struct hid_time_work_time_state *hdwork;

		hdwork = kmalloc(sizeof(*hdwork), GFP_KERNEL);

> +		hdwork->time_state = time_state;

Forgot to check for kmalloc() failure.

> +		INIT_WORK(&hdwork->work, hid_time_request_report_work);
> +		schedule_work(&hdwork->work);

The patch adds a schedule_work() but no flush_scheduled_work(), etc. 
So if the driver is shut down or rmmodded while the work is still
pending, the kernel will go kapow.

> +	}
> +
>  	return ret;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/