| Message ID | 20260812074533.3545058-2-lakshay.piplani@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND,v4,1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config | expand |
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: New issues: - [Medium] The IRQ handler silently clears the Battery Switch-over Flag (BSF) from the hardware register without saving its state, causing battery switch-over events to be lost if an interrupt triggers before userspace calls RTC_VL_READ. Pre-existing issues: - [High] `pcf85363_probe()` unconditionally wipes the `CTRL_FLAGS` register, destroying boot-time battery switch-over events before they can be read via `RTC_VL_READ`. -- --- Patch [2]: [RESEND v4 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL --- commit 641bd03440f1f0094b5ca119b02a17fb2dd848b5 Author: Lakshay Piplani <lakshay.piplani@nxp.com> rtc: pcf85363: support reporting battery switch-over via RTC_VL Add battery switch-over reporting for PCF85263/PCF85363 using the standard RTC_VL_* ioctl interface. When the backup supply takes over, the BSF flag is exposed to userspace through RTC_VL_READ and can be cleared using RTC_VL_CLR. > diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c > index 540042b9eec8f..c03d5a65c5f79 100644 > --- a/drivers/rtc/rtc-pcf85363.c > +++ b/drivers/rtc/rtc-pcf85363.c [ ... ] > @@ -295,23 +296,67 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id) > { > struct pcf85363 *pcf85363 = i2c_get_clientdata(dev_id); > + bool handled = false; > unsigned int flags; > int err; > > err = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags); > + > if (err) > return IRQ_NONE; > > + if (flags) { > + dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s\n", > + flags, (flags & FLAGS_A1F) ? " [A1F]" : "", > + (flags & FLAGS_BSF) ? " [BSF]" : ""); > + } > + > if (flags & FLAGS_A1F) { > rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF); > regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A1F, 0); > - return IRQ_HANDLED; > + handled = true; > } > > - return IRQ_NONE; > + if (flags & FLAGS_BSF) { > + regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0); > + handled = true; > + } [Severity: Medium] Does this unconditionally clear the hardware Battery Switch-over Flag (BSF) without saving its state? If an interrupt triggers before userspace calls RTC_VL_READ, won't the battery switch-over event be lost since the hardware flag is cleared but not cached in software? > + > + return handled ? IRQ_HANDLED : IRQ_NONE; > +} > + > +static int pcf85363_rtc_ioctl(struct device *dev, > + unsigned int cmd, unsigned long arg) > +{ > + struct pcf85363 *pcf85363 = dev_get_drvdata(dev); > + unsigned int val; > + int ret; > + > + switch (cmd) { > + case RTC_VL_READ: { > + u32 status = 0; > + > + ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &val); [Severity: Medium] Because RTC_VL_READ reads directly from the hardware register, it will return 0 if the IRQ handler has already cleared the flag. > + > + if (ret) > + return ret; > + > + if (val & FLAGS_BSF) > + status |= RTC_VL_BACKUP_SWITCH; > + > + return put_user(status, (u32 __user *)arg); > + } > + > + case RTC_VL_CLR: > + return regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0); > + > + default: > + return -ENOIOCTLCMD; > + } > } [Severity: High] This isn't a bug introduced by this patch, but the pre-existing code in pcf85363_probe() appears to break the boot-time power loss detection intended by this new feature. During system boot, pcf85363_probe() unconditionally clears all hardware flags if an IRQ or wakeup source is configured: drivers/rtc/rtc-pcf85363.c:pcf85363_probe() { ... if (client->irq > 0 || wakeup_source) { regmap_write(pcf85363->regmap, CTRL_FLAGS, 0); regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO, PIN_IO_INTAPM, PIN_IO_INTA_OUT); } ... } Does this destroy the battery switch-over event before userspace can query it with RTC_VL_READ?
diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c index 540042b9eec8..c03d5a65c5f7 100644 --- a/drivers/rtc/rtc-pcf85363.c +++ b/drivers/rtc/rtc-pcf85363.c @@ -14,6 +14,7 @@ #include <linux/err.h> #include <linux/errno.h> #include <linux/bcd.h> +#include <linux/device.h> #include <linux/of.h> #include <linux/regmap.h> @@ -295,23 +296,67 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id) { struct pcf85363 *pcf85363 = i2c_get_clientdata(dev_id); + bool handled = false; unsigned int flags; int err; err = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags); + if (err) return IRQ_NONE; + if (flags) { + dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s\n", + flags, (flags & FLAGS_A1F) ? " [A1F]" : "", + (flags & FLAGS_BSF) ? " [BSF]" : ""); + } + if (flags & FLAGS_A1F) { rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF); regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A1F, 0); - return IRQ_HANDLED; + handled = true; } - return IRQ_NONE; + if (flags & FLAGS_BSF) { + regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0); + handled = true; + } + + return handled ? IRQ_HANDLED : IRQ_NONE; +} + +static int pcf85363_rtc_ioctl(struct device *dev, + unsigned int cmd, unsigned long arg) +{ + struct pcf85363 *pcf85363 = dev_get_drvdata(dev); + unsigned int val; + int ret; + + switch (cmd) { + case RTC_VL_READ: { + u32 status = 0; + + ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &val); + + if (ret) + return ret; + + if (val & FLAGS_BSF) + status |= RTC_VL_BACKUP_SWITCH; + + return put_user(status, (u32 __user *)arg); + } + + case RTC_VL_CLR: + return regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_BSF, 0); + + default: + return -ENOIOCTLCMD; + } } static const struct rtc_class_ops rtc_ops = { + .ioctl = pcf85363_rtc_ioctl, .read_time = pcf85363_rtc_read_time, .set_time = pcf85363_rtc_set_time, .read_alarm = pcf85363_rtc_read_alarm,
Add battery switch-over reporting for PCF85263/PCF85363 using the standard RTC_VL_* ioctl interface. When the backup supply takes over, the BSF flag is exposed to userspace through RTC_VL_READ and can be cleared using RTC_VL_CLR. This allows applications to detect loss of main power without relying on non-standard interfaces. Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com> --- V3 -> V4: - No changes in v4. V2 -> V3: - Split into separate patches as suggested: - Battery switch-over detection. - Timestamp recording for TS pin and battery switch-over events. - Offset calibration. - Watchdog timer (to be reviewed by watchdog maintainers). - Dropped Alarm2 support - Switched to rtc_add_group() for sysfs attributes V1 -> V2: - Watchdog related changes due to removal of vendor specific properties from device tree * remove vendor DT knobs (enable/timeout/stepsize/repeat) * use watchdog_init_timeout (with 10s default) * derive clock_sel from final timeout * default, repeat=true (repeat mode) - Fixed uninitalised warning on 'ret' (reported by kernel test robot) - Use dev_dbg instead of dev_info for debug related print messages - Minor cleanup and comments. drivers/rtc/rtc-pcf85363.c | 49 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-)