On 26/10/2021 10:15:06-0500, Limonciello, Mario wrote:
> >
> > > + if (!rtc_device)
> > > + return 0;
> > > + rc = rtc_read_alarm(rtc_device, &alarm);
> > > + if (rc)
> > > + return rc;
> > > + if (!alarm.enabled) {
> > > + dev_dbg(pdev->dev, "alarm not enabled\n");
> > > + return 0;
> > > + }
> > > + rc = rtc_valid_tm(&alarm.time);
> > > + if (rc)
> > > + return rc;
> >
> > Why do you think the RTC core give you an invalid alarm time?
>
> It doesn't matter "why" to me. It's defensive programming.
> The function has a return code to check for problems. If an invalid alarm
> time was somehow programmed, we shouldn't be using it.
>
The question was not why it would happen, but why you think it could.
The RTC core will not return an invalid alarm, else, you'd already
catch that checking the return value of rtc_read_alarm. This is not
defensive programming, this is useless.
> >
> > > + rc = rtc_read_time(rtc_device, &tm);
> > > + if (rc)
> > > + return rc;
> > > + then = rtc_tm_to_time64(&alarm.time);
> > > + now = rtc_tm_to_time64(&tm);
> > > + duration = then-now;
> > > +
> > > + /* in the past */
> > > + if (then < now)
> > > + return 0;
> > > +
> > > + /* will be stored in upper 16 bits of s0i3 hint argument,
> > > + * so timer wakeup from s0i3 is limited to ~18 hours or less
> > > + */
> > > + if (duration <= 4 || duration > U16_MAX)
> > > + return -EINVAL;
> > > +
> > > + *arg |= (duration << 16);
> > > + rc = rtc_alarm_irq_enable(rtc_device, 0);
> >
> > What if userspace is waiting for the alarm to happen?
>
> They won't receive it. Note; this is no different than before the patch on
> these affected systems.
>
But then shouldn't that be fixed? Why does the alarm need to be
disabled?
> >
> > > + dev_info(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
> > > +
> >
> > Do you actually need this message, looks like leftover debug to me?
>
> Yes it was definitely used in debugging to produce this patch in the first
> place.
>
> I left it for two reasons:
> 1) It makes it clear if someone is using this functionality when they report
> an error.
>
> 2) It shows if they were operating really close to the boundary conditions
> (and thus it may be required to adjust).
>
> I am amenable to downgrading it to dev_dbg and on reported issues around
> this asking people to +p dynamic debugging for amd_pmc.
>
> >
> > > + return rc;
> > > +}
> > > +
> > > static int __maybe_unused amd_pmc_suspend(struct device *dev)
> > > {
> > > struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
> > > int rc;
> > > u8 msg;
> > > + u32 arg = 1;
> > > /* Reset and Start SMU logging - to monitor the s0i3 stats */
> > > amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
> > > amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
> > > + /* Activate CZN specific RTC functionality */
> > > + if (pdev->cpu_id == AMD_CPU_ID_CZN) {
> > > + rc = amd_pmc_verify_czn_rtc(pdev, &arg);
> > > + if (rc < 0)
> > > + return rc;
> > > + }
> > > +
> > > /* Dump the IdleMask before we send hint to SMU */
> > > amd_pmc_idlemask_read(pdev, dev, NULL);
> > > msg = amd_pmc_get_os_hint(pdev);
> > > - rc = amd_pmc_send_cmd(pdev, 1, NULL, msg, 0);
> > > + rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
> > > if (rc)
> > > dev_err(pdev->dev, "suspend failed\n");
> > > --
> > > 2.25.1
> > >
> >
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
