Re: AW: Bug#854854: qcontrol: reboot/poweroff

Re: AW: Bug#854854: qcontrol: reboot/poweroff

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 695 bytes --]

Hello Peter,

On 03/16/2017 10:52 AM, Peter Buechner wrote:
> I cleared the bug according the link below and the system is working now .

\o/

> But what I wonder: my kernel version is 3.16.0-4-kirkwood. How I got this?

This is expected for a jessie system.

> I followed the procedure on page http://www.cyrius.com/debian/kirkwood/qnap/ts-119/deinstall/
> And got the kernel and initrd files of the installer via links there.  You are writing actual version would be  4.6.4-1.
> How to get the actual kernel?

4.6.4-1 is the version where the bug is fixed. So you had the problem
because you use an older kernel than 4.6.4-1.

Is this more clear now?

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 0/4] rtc: s35390a: add fixes from v4.8-rc1 to stable

[Uwe Kleine-König]

Hello

I suggest to add the four patches in this series to the stable releases
for stable kernels before 4.8. Three of them can be directly
cherry-picked from the commits from 4.8-rc1 as indicated in the
respective commits[1]. One patch however corresponds to a more invasive
patch (d68778b80dd7 ("rtc: initialize output parameter for read alarm to
"uninitialized"") that when I discussed that with Ben Hutchings some
time ago we considered safer to only apply it to the s35390a driver. So
this one doesn't match a mainline commit.

Best regards
Uwe

[1] f87e904ddd8f0ef120e46045b0addeb1cc88354e
    8e6583f1b5d1f5f129b873f1428b7e414263d847
    3bd32722c827d00eafe8e6d5b83e9f3148ea7c7e

Uwe Kleine-König (4):
  rtc: s35390a: fix reading out alarm
  rtc: s35390a: make sure all members in the output are set
  rtc: s35390a: implement reset routine as suggested by the reference
  rtc: s35390a: improve irq handling

 drivers/rtc/rtc-s35390a.c | 167 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 131 insertions(+), 36 deletions(-)

-- 
2.11.0

[PATCH 1/4] rtc: s35390a: fix reading out alarm

[Uwe Kleine-König]

commit f87e904ddd8f0ef120e46045b0addeb1cc88354e upstream.

There are several issues fixed in this patch:

 - When alarm isn't enabled, set .enabled to zero instead of returning
   -EINVAL.
 - Ignore how IRQ1 is configured when determining if IRQ2 is on.
 - The three alarm registers have an enable flag which must be
   evaluated.
 - The chip always triggers when the seconds register gets 0.

Note that the rtc framework however doesn't handle the result correctly
because it doesn't check wday being initialized and so interprets an
alarm being set for 10:00 AM in three days as 10:00 AM tomorrow (or
today if that's not over yet).

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/rtc/rtc-s35390a.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index f40afdd0e5f5..6507a01cf9ad 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -242,6 +242,8 @@ static int s35390a_set_alarm(struct i2c_client *client, struct rtc_wkalrm *alm)
 
 	if (alm->time.tm_wday != -1)
 		buf[S35390A_ALRM_BYTE_WDAY] = bin2bcd(alm->time.tm_wday) | 0x80;
+	else
+		buf[S35390A_ALRM_BYTE_WDAY] = 0;
 
 	buf[S35390A_ALRM_BYTE_HOURS] = s35390a_hr2reg(s35390a,
 			alm->time.tm_hour) | 0x80;
@@ -269,23 +271,43 @@ static int s35390a_read_alarm(struct i2c_client *client, struct rtc_wkalrm *alm)
 	if (err < 0)
 		return err;
 
-	if (bitrev8(sts) != S35390A_INT2_MODE_ALARM)
-		return -EINVAL;
+	if ((bitrev8(sts) & S35390A_INT2_MODE_MASK) != S35390A_INT2_MODE_ALARM) {
+		/*
+		 * When the alarm isn't enabled, the register to configure
+		 * the alarm time isn't accessible.
+		 */
+		alm->enabled = 0;
+		return 0;
+	} else {
+		alm->enabled = 1;
+	}
 
 	err = s35390a_get_reg(s35390a, S35390A_CMD_INT2_REG1, buf, sizeof(buf));
 	if (err < 0)
 		return err;
 
 	/* This chip returns the bits of each byte in reverse order */
-	for (i = 0; i < 3; ++i) {
+	for (i = 0; i < 3; ++i)
 		buf[i] = bitrev8(buf[i]);
-		buf[i] &= ~0x80;
-	}
 
-	alm->time.tm_wday = bcd2bin(buf[S35390A_ALRM_BYTE_WDAY]);
-	alm->time.tm_hour = s35390a_reg2hr(s35390a,
-						buf[S35390A_ALRM_BYTE_HOURS]);
-	alm->time.tm_min = bcd2bin(buf[S35390A_ALRM_BYTE_MINS]);
+	/*
+	 * B0 of the three matching registers is an enable flag. Iff it is set
+	 * the configured value is used for matching.
+	 */
+	if (buf[S35390A_ALRM_BYTE_WDAY] & 0x80)
+		alm->time.tm_wday =
+			bcd2bin(buf[S35390A_ALRM_BYTE_WDAY] & ~0x80);
+
+	if (buf[S35390A_ALRM_BYTE_HOURS] & 0x80)
+		alm->time.tm_hour =
+			s35390a_reg2hr(s35390a,
+				       buf[S35390A_ALRM_BYTE_HOURS] & ~0x80);
+
+	if (buf[S35390A_ALRM_BYTE_MINS] & 0x80)
+		alm->time.tm_min = bcd2bin(buf[S35390A_ALRM_BYTE_MINS] & ~0x80);
+
+	/* alarm triggers always at s=0 */
+	alm->time.tm_sec = 0;
 
 	dev_dbg(&client->dev, "%s: alm is mins=%d, hours=%d, wday=%d\n",
 			__func__, alm->time.tm_min, alm->time.tm_hour,
-- 
2.11.0

[PATCH 2/4] rtc: s35390a: make sure all members in the output are set

[Uwe Kleine-König]

The rtc core calls the .read_alarm with all fields initialized to 0. As
the s35390a driver doesn't touch some fields the returned date is
interpreted as a date in January 1900. So make sure all fields are set
to -1; some of them are then overwritten with the right data depending
on the hardware state.

In mainline this is done by commit d68778b80dd7 ("rtc: initialize output
parameter for read alarm to "uninitialized"") in the core. This is
considered to dangerous for stable as it might have side effects for
other rtc drivers that might for example rely on alarm->time.tm_sec
being initialized to 0.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/rtc/rtc-s35390a.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 6507a01cf9ad..47b88bbe4ce7 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -267,6 +267,20 @@ static int s35390a_read_alarm(struct i2c_client *client, struct rtc_wkalrm *alm)
 	char buf[3], sts;
 	int i, err;
 
+	/*
+	 * initialize all members to -1 to signal the core that they are not
+	 * defined by the hardware.
+	 */
+	alm->time.tm_sec = -1;
+	alm->time.tm_min = -1;
+	alm->time.tm_hour = -1;
+	alm->time.tm_mday = -1;
+	alm->time.tm_mon = -1;
+	alm->time.tm_year = -1;
+	alm->time.tm_wday = -1;
+	alm->time.tm_yday = -1;
+	alm->time.tm_isdst = -1;
+
 	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
 	if (err < 0)
 		return err;
-- 
2.11.0

Patch "rtc: s35390a: make sure all members in the output are set" has been added to the 4.4-stable tree

[gregkh]

This is a note to let you know that I've just added the patch titled

    rtc: s35390a: make sure all members in the output are set

to the 4.4-stable tree which can be found at:
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
     rtc-s35390a-make-sure-all-members-in-the-output-are-set.patch
and it can be found in the queue-4.4 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.


>From uwe@kleine-koenig.org  Thu Apr  6 09:35:55 2017
From: Uwe Kleine-K�nig <uwe@kleine-koenig.org>
Date: Mon,  3 Apr 2017 23:32:38 +0200
Subject: rtc: s35390a: make sure all members in the output are set
To: stable@vger.kernel.org
Cc: Martin Michlmayr <tbm@cyrius.com>, peter <ptrbucr3075@gmail.com>, Alexandre Belloni <alexandre.belloni@free-electrons.com>, debian-kernel@lists.debian.org
Message-ID: <20170403213240.3856-3-uwe@kleine-koenig.org>

From: Uwe Kleine-K�nig <uwe@kleine-koenig.org>

The rtc core calls the .read_alarm with all fields initialized to 0. As
the s35390a driver doesn't touch some fields the returned date is
interpreted as a date in January 1900. So make sure all fields are set
to -1; some of them are then overwritten with the right data depending
on the hardware state.

In mainline this is done by commit d68778b80dd7 ("rtc: initialize output
parameter for read alarm to "uninitialized"") in the core. This is
considered to dangerous for stable as it might have side effects for
other rtc drivers that might for example rely on alarm->time.tm_sec
being initialized to 0.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/rtc/rtc-s35390a.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -267,6 +267,20 @@ static int s35390a_read_alarm(struct i2c
 	char buf[3], sts;
 	int i, err;
 
+	/*
+	 * initialize all members to -1 to signal the core that they are not
+	 * defined by the hardware.
+	 */
+	alm->time.tm_sec = -1;
+	alm->time.tm_min = -1;
+	alm->time.tm_hour = -1;
+	alm->time.tm_mday = -1;
+	alm->time.tm_mon = -1;
+	alm->time.tm_year = -1;
+	alm->time.tm_wday = -1;
+	alm->time.tm_yday = -1;
+	alm->time.tm_isdst = -1;
+
 	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS2, &sts, sizeof(sts));
 	if (err < 0)
 		return err;


Patches currently in stable-queue which might be from uwe@kleine-koenig.org are

queue-4.4/rtc-s35390a-improve-irq-handling.patch
queue-4.4/rtc-s35390a-fix-reading-out-alarm.patch
queue-4.4/rtc-s35390a-make-sure-all-members-in-the-output-are-set.patch
queue-4.4/rtc-s35390a-implement-reset-routine-as-suggested-by-the-reference.patch

[PATCH 3/4] rtc: s35390a: implement reset routine as suggested by the reference

[Uwe Kleine-König]

commit 8e6583f1b5d1f5f129b873f1428b7e414263d847 upstream.

There were two deviations from the reference manual: you have to wait
half a second when POC is active and you might have to repeat
initialization when POC or BLD are still set after the sequence.

Note however that as POC and BLD are cleared by read the driver might
not be able to detect that a reset is necessary. I don't have a good
idea how to fix this.

Additionally report the value read from STATUS1 to the caller. This
prepares the next patch.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/rtc/rtc-s35390a.c | 65 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index 47b88bbe4ce7..c7c1fce69635 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -15,6 +15,7 @@
 #include <linux/bitrev.h>
 #include <linux/bcd.h>
 #include <linux/slab.h>
+#include <linux/delay.h>
 
 #define S35390A_CMD_STATUS1	0
 #define S35390A_CMD_STATUS2	1
@@ -94,19 +95,63 @@ static int s35390a_get_reg(struct s35390a *s35390a, int reg, char *buf, int len)
 	return 0;
 }
 
-static int s35390a_reset(struct s35390a *s35390a)
+/*
+ * Returns <0 on error, 0 if rtc is setup fine and 1 if the chip was reset.
+ * To keep the information if an irq is pending, pass the value read from
+ * STATUS1 to the caller.
+ */
+static int s35390a_reset(struct s35390a *s35390a, char *status1)
 {
-	char buf[1];
+	char buf;
+	int ret;
+	unsigned initcount = 0;
 
-	if (s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf)) < 0)
-		return -EIO;
+	ret = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, status1, 1);
+	if (ret < 0)
+		return ret;
 
-	if (!(buf[0] & (S35390A_FLAG_POC | S35390A_FLAG_BLD)))
+	if (*status1 & S35390A_FLAG_POC)
+		/*
+		 * Do not communicate for 0.5 seconds since the power-on
+		 * detection circuit is in operation.
+		 */
+		msleep(500);
+	else if (!(*status1 & S35390A_FLAG_BLD))
+		/*
+		 * If both POC and BLD are unset everything is fine.
+		 */
 		return 0;
 
-	buf[0] |= (S35390A_FLAG_RESET | S35390A_FLAG_24H);
-	buf[0] &= 0xf0;
-	return s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf));
+	/*
+	 * At least one of POC and BLD are set, so reinitialise chip. Keeping
+	 * this information in the hardware to know later that the time isn't
+	 * valid is unfortunately not possible because POC and BLD are cleared
+	 * on read. So the reset is best done now.
+	 *
+	 * The 24H bit is kept over reset, so set it already here.
+	 */
+initialize:
+	*status1 = S35390A_FLAG_24H;
+	buf = S35390A_FLAG_RESET | S35390A_FLAG_24H;
+	ret = s35390a_set_reg(s35390a, S35390A_CMD_STATUS1, &buf, 1);
+
+	if (ret < 0)
+		return ret;
+
+	ret = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, &buf, 1);
+	if (ret < 0)
+		return ret;
+
+	if (buf & (S35390A_FLAG_POC | S35390A_FLAG_BLD)) {
+		/* Try up to five times to reset the chip */
+		if (initcount < 5) {
+			++initcount;
+			goto initialize;
+		} else
+			return -EIO;
+	}
+
+	return 1;
 }
 
 static int s35390a_disable_test_mode(struct s35390a *s35390a)
@@ -367,7 +412,7 @@ static int s35390a_probe(struct i2c_client *client,
 	unsigned int i;
 	struct s35390a *s35390a;
 	struct rtc_time tm;
-	char buf[1];
+	char buf[1], status1;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		err = -ENODEV;
@@ -396,7 +441,7 @@ static int s35390a_probe(struct i2c_client *client,
 		}
 	}
 
-	err = s35390a_reset(s35390a);
+	err = s35390a_reset(s35390a, &status1);
 	if (err < 0) {
 		dev_err(&client->dev, "error resetting chip\n");
 		goto exit_dummy;
-- 
2.11.0

[PATCH 4/4] rtc: s35390a: improve irq handling

[Uwe Kleine-König]

commit 3bd32722c827d00eafe8e6d5b83e9f3148ea7c7e upstream.

On some QNAP NAS devices the rtc can wake the machine. Several people
noticed that once the machine was woken this way it fails to shut down.
That's because the driver fails to acknowledge the interrupt and so it
keeps active and restarts the machine immediatly after shutdown. See
https://bugs.debian.org/794266 for a bug report.

Doing this correctly requires to interpret the INT2 flag of the first read
of the STATUS1 register because this bit is cleared by read.

Note this is not maximally robust though because a pending irq isn't
detected when the STATUS1 register was already read (and so INT2 is not
set) but the irq was not disabled. But that is a hardware imposed problem
that cannot easily be fixed by software.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/rtc/rtc-s35390a.c | 48 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index c7c1fce69635..00662dd28d66 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -35,10 +35,14 @@
 #define S35390A_ALRM_BYTE_HOURS	1
 #define S35390A_ALRM_BYTE_MINS	2
 
+/* flags for STATUS1 */
 #define S35390A_FLAG_POC	0x01
 #define S35390A_FLAG_BLD	0x02
+#define S35390A_FLAG_INT2	0x04
 #define S35390A_FLAG_24H	0x40
 #define S35390A_FLAG_RESET	0x80
+
+/* flag for STATUS2 */
 #define S35390A_FLAG_TEST	0x01
 
 #define S35390A_INT2_MODE_MASK		0xF0
@@ -408,11 +412,11 @@ static struct i2c_driver s35390a_driver;
 static int s35390a_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
-	int err;
+	int err, err_reset;
 	unsigned int i;
 	struct s35390a *s35390a;
 	struct rtc_time tm;
-	char buf[1], status1;
+	char buf, status1;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		err = -ENODEV;
@@ -441,29 +445,35 @@ static int s35390a_probe(struct i2c_client *client,
 		}
 	}
 
-	err = s35390a_reset(s35390a, &status1);
-	if (err < 0) {
+	err_reset = s35390a_reset(s35390a, &status1);
+	if (err_reset < 0) {
+		err = err_reset;
 		dev_err(&client->dev, "error resetting chip\n");
 		goto exit_dummy;
 	}
 
-	err = s35390a_disable_test_mode(s35390a);
-	if (err < 0) {
-		dev_err(&client->dev, "error disabling test mode\n");
-		goto exit_dummy;
-	}
-
-	err = s35390a_get_reg(s35390a, S35390A_CMD_STATUS1, buf, sizeof(buf));
-	if (err < 0) {
-		dev_err(&client->dev, "error checking 12/24 hour mode\n");
-		goto exit_dummy;
-	}
-	if (buf[0] & S35390A_FLAG_24H)
+	if (status1 & S35390A_FLAG_24H)
 		s35390a->twentyfourhour = 1;
 	else
 		s35390a->twentyfourhour = 0;
 
-	if (s35390a_get_datetime(client, &tm) < 0)
+	if (status1 & S35390A_FLAG_INT2) {
+		/* disable alarm (and maybe test mode) */
+		buf = 0;
+		err = s35390a_set_reg(s35390a, S35390A_CMD_STATUS2, &buf, 1);
+		if (err < 0) {
+			dev_err(&client->dev, "error disabling alarm");
+			goto exit_dummy;
+		}
+	} else {
+		err = s35390a_disable_test_mode(s35390a);
+		if (err < 0) {
+			dev_err(&client->dev, "error disabling test mode\n");
+			goto exit_dummy;
+		}
+	}
+
+	if (err_reset > 0 || s35390a_get_datetime(client, &tm) < 0)
 		dev_warn(&client->dev, "clock needs to be set\n");
 
 	device_set_wakeup_capable(&client->dev, 1);
@@ -476,6 +486,10 @@ static int s35390a_probe(struct i2c_client *client,
 		err = PTR_ERR(s35390a->rtc);
 		goto exit_dummy;
 	}
+
+	if (status1 & S35390A_FLAG_INT2)
+		rtc_update_irq(s35390a->rtc, 1, RTC_AF);
+
 	return 0;
 
 exit_dummy:
-- 
2.11.0