[rtc-linux] Re: [PATCH 3/3] rtc: ds3232: Call device_init_wakeup before device_register

On 24/02/2017 at 10:32:40 +0800, Phil Reid wrote:

On 24/02/2017 09:46, Alexandre Belloni wrote:

quoted

On 22/02/2017 at 09:33:14 +0800, Phil Reid wrote:

quoted

On 22/02/2017 04:33, Alexandre Belloni wrote:

quoted

On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote:

quoted

The wakealarm attribute is currently not exposed in the sysfs interface
as the device has not been set as doing wakealarm when device_register
is called. Changing the order of the calls fixes that problem. Interrupts
are cleared in check_rtc_status prior to requesting the interrupt.

This basically revert b4b77f3c280e38cec178f81d7a4d7e65f4045913 So I'm
not sure this is sufficient to ensure the IRQ will never fire.

(I don't know whether this was a real bug or something reported by a
static analysis tool).

G'day Alexandre,
Without this change the wakealarm sysfs is never created.
This is done in rtc_device_register, but a filter to wakealarm attribute effectively on
dev->power.can_wakeup flag. Which is set via the init wakeup call.

But looking at that commit and thinking about it some more I can see the problem
b4b77f3c280e38cec178f81d7a4d7e65f4045913 was try to solve.

Looking at other drivers some of them have similar order but use their own mutex.
eg rtc-rc8803 and don't use the ops_lock

Or others set device_init_wakeup and then fail if the irq fails to register.

Currently if the ds3232 has an irq set but fails to register the irq it falls back
to being a non wakeup source. To me it makes more sense to just fall if the config
has an irq defined.

Then we could set device_init_wakeup based on there being an irq prior to device_register
and then request the irq after device register. But fail the probe if the irq request fails.

Thoughts?

I think you can use device_set_wakeup_capable() afterwards which would
fit this use case. Can you try that?

Anyway, I have a pending rework of the rtc registration that will solve
this issue (and the sysfs attribute creation race).

Nope, doesn't help.
device_init_wakeup already calls device_set_wakeup_capable.

rtc_attr_groups is fetched via rtc_get_dev_attribute_groups
and assigned to the rtc->dev.groups in rtc_device_register
The is_visible callback checks device_can_wakeup.
And this is not set until after device creation.

Is there a way to retrigger the sysfs groups creation after device
registration? Thou this would result in sysfs attrib. races I guess.

Failing to probe when irq requested seems the easiest.
Would anyone want the irq registration to fail but device creation succeed if requested?

Yes because th RTC is still functional and can give the time (and that
may break userspace).

What you can do is call device_init_wakeup before registering the rtc
and then device_set_wakeup_capable(&client->dev, false); if the irq
request fails.

-- 
Regards
Phil Reid

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.