Message ID | 1487295898-90177-4-git-send-email-preid@electromag.com.au |
---|---|
State | Superseded |
Headers | show |
On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote: > 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). > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- > drivers/rtc/rtc-ds3232.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c > index 60de3a0..ce943d0 100644 > --- a/drivers/rtc/rtc-ds3232.c > +++ b/drivers/rtc/rtc-ds3232.c > @@ -363,11 +363,6 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, > if (ret) > return ret; > > - ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops, > - THIS_MODULE); > - if (IS_ERR(ds3232->rtc)) > - return PTR_ERR(ds3232->rtc); > - > if (ds3232->irq > 0) { > ret = devm_request_threaded_irq(dev, ds3232->irq, NULL, > ds3232_irq, > @@ -380,6 +375,11 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, > device_init_wakeup(dev, 1); > } > > + ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops, > + THIS_MODULE); > + if (IS_ERR(ds3232->rtc)) > + return PTR_ERR(ds3232->rtc); > + > return 0; > } > > -- > 1.8.3.1 >
On 22/02/2017 04:33, Alexandre Belloni wrote: > On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote: >> 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? > >> Signed-off-by: Phil Reid <preid@electromag.com.au> >> --- >> drivers/rtc/rtc-ds3232.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c >> index 60de3a0..ce943d0 100644 >> --- a/drivers/rtc/rtc-ds3232.c >> +++ b/drivers/rtc/rtc-ds3232.c >> @@ -363,11 +363,6 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, >> if (ret) >> return ret; >> >> - ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops, >> - THIS_MODULE); >> - if (IS_ERR(ds3232->rtc)) >> - return PTR_ERR(ds3232->rtc); >> - >> if (ds3232->irq > 0) { >> ret = devm_request_threaded_irq(dev, ds3232->irq, NULL, >> ds3232_irq, >> @@ -380,6 +375,11 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, >> device_init_wakeup(dev, 1); >> } >> >> + ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops, >> + THIS_MODULE); >> + if (IS_ERR(ds3232->rtc)) >> + return PTR_ERR(ds3232->rtc); >> + >> return 0; >> } >> >> -- >> 1.8.3.1 >> >
On 22/02/2017 at 09:33:14 +0800, Phil Reid wrote: > On 22/02/2017 04:33, Alexandre Belloni wrote: > > On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote: > > > 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).
On 24/02/2017 09:46, Alexandre Belloni wrote: > On 22/02/2017 at 09:33:14 +0800, Phil Reid wrote: >> On 22/02/2017 04:33, Alexandre Belloni wrote: >>> On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote: >>>> 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?
On 24/02/2017 at 10:32:40 +0800, Phil Reid wrote: > On 24/02/2017 09:46, Alexandre Belloni wrote: > > On 22/02/2017 at 09:33:14 +0800, Phil Reid wrote: > > > On 22/02/2017 04:33, Alexandre Belloni wrote: > > > > On 17/02/2017 at 09:44:58 +0800, Phil Reid wrote: > > > > > 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 >
diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c index 60de3a0..ce943d0 100644 --- a/drivers/rtc/rtc-ds3232.c +++ b/drivers/rtc/rtc-ds3232.c @@ -363,11 +363,6 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, if (ret) return ret; - ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops, - THIS_MODULE); - if (IS_ERR(ds3232->rtc)) - return PTR_ERR(ds3232->rtc); - if (ds3232->irq > 0) { ret = devm_request_threaded_irq(dev, ds3232->irq, NULL, ds3232_irq, @@ -380,6 +375,11 @@ static int ds3232_probe(struct device *dev, struct regmap *regmap, int irq, device_init_wakeup(dev, 1); } + ds3232->rtc = devm_rtc_device_register(dev, name, &ds3232_rtc_ops, + THIS_MODULE); + if (IS_ERR(ds3232->rtc)) + return PTR_ERR(ds3232->rtc); + return 0; }
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. Signed-off-by: Phil Reid <preid@electromag.com.au> --- drivers/rtc/rtc-ds3232.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)