On 09/28/2017 11:12 AM, Jeffrey Hugo wrote:
> On 9/27/2017 5:33 PM, Al Stone wrote:
>> On 09/27/2017 11:41 AM, Jeffrey Hugo wrote:
>>> On 9/26/2017 5:33 PM, Colin Ian King wrote:
>>>> On 27/09/17 00:27, Al Stone wrote:
>>>>> So, some platforms don't really have an ACPI wake alarm, but the test is
>>>>> set to
>>>>> essentially always be run:
>>>>>
>>>>> FWTS_REGISTER("wakealarm", &wakealarm_ops, FWTS_TEST_ANYTIME,
>>>>> FWTS_FLAG_BATCH |
>>>>> FWTS_FLAG_ROOT_PRIV)
>>>>>
>>>>> Is there a recommended way to make a test more hardware specific? Granted, if
>>>>> I run FWTS on the platform, I can double check and make sure
>>>>> /sys/class/rtc/rtc0
>>>>> exists (or something like that); if I run FWTS elsewhere, though, that's
>>>>> clearly
>>>>> not going to work. I know I've added options in the past for
>>>>> architectures, but
>>>>> this is specific to an individual machine and it would seem like overkill to
>>>>> put in options for specific machines.
>>>>>
>>>>> Suggestions?
>>>>
>>>> Perhaps the simplest approach is to skip all the subsequent tests if
>>>> wakealarm_test1() fails to find a working interface.
>>>
>>> Its been a while since I've looked at this, but if I recall correctly,
>>> wakealarm_tes1() always finds a "working" interface due to how the Linux kernel
>>> abstracts RTC.
>>>
>>> What I've seen on our platform is that UEFI does not implement the wake alarm
>>> functionality, however the kernel is happy to "fake" it, until you try setting
>>> an actual alarm, then you start getting failures.
>>>
>>> I'd be interested in determining a way to address this, since it would alleviate
>>> the burden of blacklisting the test and explaining why.
>>
>> @Jeffrey: would you mind testing the patch below (it should apply to top-of-tree
>> for FWTS)? Two things change with this patch: (1) assume fwts_wakealarm_get()
>> does not work, and the ioctl() has to to succeed, and (2) fail if the wakealarm
>> is not currently enabled. That *may* be sufficient for your platform, but I
>> think the RTC ioctl() has to be fixed ultimately -- it's kind of strange the way
>> it works, as you noted, and it makes it really hard for the device to tell you
>> if the RTC wakealarm is working, or even exists.
>>
>>
>> diff --git a/src/lib/src/fwts_wakealarm.c b/src/lib/src/fwts_wakealarm.c
>> index c0550303..d725caaa 100644
>> --- a/src/lib/src/fwts_wakealarm.c
>> +++ b/src/lib/src/fwts_wakealarm.c
>> @@ -39,17 +39,21 @@ static const char *fwts_rtc = "/dev/rtc0";
>> int fwts_wakealarm_get(fwts_framework *fw, struct rtc_time *rtc_tm)
>> {
>> int fd;
>> - int ret = FWTS_OK;
>> + int ret = FWTS_ERROR;
>> + struct rtc_wkalrm rtc_wakealarm;
>>
>> if ((fd = open(fwts_rtc, O_RDWR)) < 0) {
>> fwts_log_error(fw, "Cannot access Real Time Clock device %s.",
>> fwts_rtc);
>> return FWTS_ERROR;
>> }
>>
>> - if (ioctl(fd, RTC_ALM_READ, rtc_tm) < 0) {
>> - fwts_log_error(fw, "Cannot read Real Time Clock Alarm with ioctl
>> RTC_ALM_READ
>> %s.", fwts_rtc);
>> - ret = FWTS_ERROR;
>> + if (ioctl(fd, RTC_WKALM_RD, &rtc_wakealarm) < 0) {
>> + if (!rtc_wakealarm.enabled) {
>> + fwts_log_error(fw, "Cannot read Real Time Clock Alarm with ioctl
>> RTC_ALM_READ %s.", fwts_rtc);
>> + ret = FWTS_ERROR;
>> + }
>> }
>> + ret = FWTS_OK;
>> (void)close(fd);
>>
>> return ret;
>>
>>
>
> Doesn't look like your suggested change made a difference in my case. Attaching
> results before and after the change.
>
Rats. I was afraid of that :(. Thanks for trying. The ioctl() is
structured in a way that may make it impossible to fix this from user
space.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------