linux-kernel - Re: [PATCH 3/3 v2] iio: add rtc-driver for HID sensors of type time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <50C66406.9090606@metafoo.de>
Date:	Mon, 10 Dec 2012 23:36:54 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Alexander Holler <holler@...oftware.de>
CC:	linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
	Jonathan Cameron <jic23@....ac.uk>, rtc-linux@...glegroups.com,
	Alessandro Zummo <a.zummo@...ertech.it>,
	srinivas pandruvada <srinivas.pandruvada@...el.com>
Subject: Re: [PATCH 3/3 v2] iio: add rtc-driver for HID sensors of type time

On 12/10/2012 11:20 PM, Alexander Holler wrote:
> [...]
>>> +    init_completion(&time_state->comp_last_time);
>>
>> This needs to be INIT_COMPLETION. init_completion must be called exactly
>> once on a completion, which should be from inside probe() in this case.
> 
> Ah, so I've misread http://lwn.net/Articles/23993/ . I've read it as
> init_completion() is usable multiple times (I had the impression
> INIT_COMPLETION got replaced by init_completion().
> 

Well, I've been exaggerating a bit, you can call it multiple times, but you
need to make sure that the completion is not in use at the same time, since
init_completion will completely reinitialize the struct and if it is in use at
the same time you'll get undefined behavior. So it is a good rule of thumb to
just use it once in probe(). INIT_COMPLETION on the other hand will just clear
the done flag, so you know that between your call to INIT_COMPLETION and when
wait_for_completion returns successfully your event has occurred at least once.

>>> +    /* wait for all values (event) */
>>> +   
>>> wait_for_completion_interruptible_timeout(&time_state->comp_last_time,
>>> +                            HZ*6);
>>
>> You should check the return value in case either a timeout happens or the
>> sleep is interrupted.
> 
> Yes, I already wanted to do it, but it seems I've forgotten it.
> 
>>> +    struct hid_time_state *time_state =
>>> +        kzalloc(sizeof(struct hid_time_state), GFP_KERNEL);
>>
>> You could use devm_kzalloc here. By doing so you don't have to take
>> care of
>> freeing it again since it will be auto-freed once the device is removed.
> 
> Thanks. I already searched such and wondered why such didn't exist. Just
> to clarify, if I use devm_kzalloc, I don't have to free time_state here
> 
>>> +error_free_drvdata:
>>> +    platform_set_drvdata(pdev, NULL);
>>
>> Setting the platform data to NULL should not be necessary. Some
>> drivers do
>> this but it's kind of the result of cargo-cult-coding.
>>
>>> +    kfree(time_state);
>>> +error_ret:
>>> +    return ret;
>>> +}
>>> +
>>> +static int __devinit hid_time_remove(struct platform_device *pdev)
>>> +{
>>> +    struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>>> +    struct hid_time_state *time_state = platform_get_drvdata(pdev);
>>> +
>>> +    if (!IS_ERR(time_state->rtc))
>>> +        rtc_device_unregister(time_state->rtc);
>>> +    sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TIME);
>>> +    platform_set_drvdata(pdev, NULL);
>>
> 
> and here?
> 

yes.

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