Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Oct 2022 18:10:53 +0100
From:   Alexandre Belloni <alexandre.belloni@bootlin.com>
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Alessandro Zummo <a.zummo@towertech.it>,
        Benson Leung <bleung@chromium.org>, linux-rtc@vger.kernel.org,
        chrome-platform@lists.linux.dev, linux-kernel@vger.kernel.org,
        Brian Norris <briannorris@chromium.org>
Subject: Re: [PATCH] rtc: cros-ec: Limit RTC alarm range if needed
Message-ID: <Y2ABnbBGSJGM3gSS@mail.local>
References: <20221029005400.2712577-1-linux@roeck-us.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221029005400.2712577-1-linux@roeck-us.net>
Precedence: bulk

Hello,

On 28/10/2022 17:54:00-0700, Guenter Roeck wrote:
> RTC chips on some older Chromebooks can only handle alarms less than 24
> hours in the future. Attempts to set an alarm beyond that range fails.
> The most severe impact of this limitation is that suspend requests fail
> if alarmtimer_suspend() tries to set an alarm for more than 24 hours
> in the future.
> 
> Try to set the real-time alarm to just below 24 hours if setting it to
> a larger value fails to work around the problem. While not perfect, it
> is better than just failing the call. A similar workaround is already
> implemented in the rtc-tps6586x driver.

I'm not super convinced this is actually better than failing the call
because your are implementing policy in the driver which is bad from a
user point of view. It would be way better to return -ERANGE and let
userspace select a better alarm time.
Do you have to know in advance which are the "older" chromebooks that
are affected?

> 
> Drop error messages in cros_ec_rtc_get() and cros_ec_rtc_set() since the
> calling code also logs an error and to avoid spurious error messages if
> setting the alarm ultimately succeeds.
> 
> Cc: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/rtc/rtc-cros-ec.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-cros-ec.c b/drivers/rtc/rtc-cros-ec.c
> index 887f5193e253..a3ec066d8066 100644
> --- a/drivers/rtc/rtc-cros-ec.c
> +++ b/drivers/rtc/rtc-cros-ec.c
> @@ -14,6 +14,8 @@
>  
>  #define DRV_NAME	"cros-ec-rtc"
>  
> +#define SECS_PER_DAY	(24 * 60 * 60)
> +
>  /**
>   * struct cros_ec_rtc - Driver data for EC RTC
>   *
> @@ -43,13 +45,8 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
>  	msg.msg.insize = sizeof(msg.data);
>  
>  	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> -	if (ret < 0) {
> -		dev_err(cros_ec->dev,
> -			"error getting %s from EC: %d\n",
> -			command == EC_CMD_RTC_GET_VALUE ? "time" : "alarm",
> -			ret);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	*response = msg.data.time;
>  
> @@ -59,7 +56,7 @@ static int cros_ec_rtc_get(struct cros_ec_device *cros_ec, u32 command,
>  static int cros_ec_rtc_set(struct cros_ec_device *cros_ec, u32 command,
>  			   u32 param)
>  {
> -	int ret = 0;
> +	int ret;
>  	struct {
>  		struct cros_ec_command msg;
>  		struct ec_response_rtc data;
> @@ -71,13 +68,8 @@ static int cros_ec_rtc_set(struct cros_ec_device *cros_ec, u32 command,
>  	msg.data.time = param;
>  
>  	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> -	if (ret < 0) {
> -		dev_err(cros_ec->dev, "error setting %s on EC: %d\n",
> -			command == EC_CMD_RTC_SET_VALUE ? "time" : "alarm",
> -			ret);
> +	if (ret < 0)
>  		return ret;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -190,8 +182,21 @@ static int cros_ec_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  
>  	ret = cros_ec_rtc_set(cros_ec, EC_CMD_RTC_SET_ALARM, alarm_offset);
>  	if (ret < 0) {
> -		dev_err(dev, "error setting alarm: %d\n", ret);
> -		return ret;
> +		if (ret == -EINVAL && alarm_offset >= SECS_PER_DAY) {
> +			/*
> +			 * RTC chips on some older Chromebooks can only handle
> +			 * alarms up to 24h in the future. Try to set an alarm
> +			 * below that limit to avoid suspend failures.
> +			 */
> +			ret = cros_ec_rtc_set(cros_ec, EC_CMD_RTC_SET_ALARM,
> +					      SECS_PER_DAY - 1);
> +		}
> +
> +		if (ret < 0) {
> +			dev_err(dev, "error setting alarm in %u seconds: %d\n",
> +				alarm_offset, ret);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.36.2
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com