On Fri, 19 Apr 2013 17:14:12 +0200 Alexander Holler <hol...@ahsoftware.de>
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
