Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756254AbbEUN2P (ORCPT ); Thu, 21 May 2015 09:28:15 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:49367 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754536AbbEUN2I (ORCPT ); Thu, 21 May 2015 09:28:08 -0400 Message-ID: <555DDD63.9070705@roeck-us.net> Date: Thu, 21 May 2015 06:28:03 -0700 From: Guenter Roeck User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Fu Wei CC: Suravee Suthikulpanit , Linaro ACPI Mailman List , linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, Wei Fu , G Gregory , Al Stone , Hanjun Guo , Timur Tabi , Ashwin Chaugule , Arnd Bergmann , vgandhi@codeaurora.org, wim@iguana.be, Jon Masters , Leo Duran , Jon Corbet , "mark.rutland" Subject: Re: [PATCH v2 5/7] Watchdog: introduce "pretimeout" into framework References: <=fu.wei@linaro.org> <1432197156-16947-1-git-send-email-fu.wei@linaro.org> <1432197156-16947-6-git-send-email-fu.wei@linaro.org> <555D9FA8.9060106@roeck-us.net> <555DB0B8.9000503@roeck-us.net> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2562 Lines: 64 On 05/21/2015 03:50 AM, Fu Wei wrote: > Hi Guenter, > > Great thanks, I have got your review feedback, I will fix the problems :-) > For update_limits, I also don't want to have that in the watchdog > driver, and you can't find it in my last version. > And this version, I integrate the watchdog_init_timeout and > watchdog_init_pretimeout. so I can not add a driver specific > "update_limits" between them. > I think maybe we can not make this "update_limits" after calling > init_timeouts, because we need to test and verify the timeout setting > right after init_pretimeout by max_timeout and min_timeout. that is Don't you have to do that in set_pretimeout as well, and even adjust timeout if necessary ? > why I call update_limits right after init_pretimeout and before > init_timeout. > > any suggestion ? Great thanks ! :-) > Any sequence of set_timeout() set_pretimeout() can result in an invalid timeout, since the practical timeout limits changed. Therefore, you'll have to validate (and possibly update) the timeout after or during each call to set_pretimeout(). Changing min_timeout and max_timeout won't help there, that validation still has to happen. Given this, some other driver developer may choose to not change the timeout limits at all, validate (and if necessary silently adjust) the timeout value in the set_timeout function, and call the set_timeout function from set_pretimeout to ensure that the timeout is still valid after the call to set_pretimeout. At least I would most likely implement it that way. This means your choice of continuously updating the timeout limits is really a driver developer choice, not something that is needed in the driver core. It might even be problematic, since driver developers might forget to re-validate the current timeout after changing the pretimeout in a similar situation. Having said that, and looking through the SBSA driver code, it seems to me that it does not re-validate the current timeout after setting a new pretimeout. Unless I am missing something, set_timeout(10); set_pretimeout(20); might therefore result in an invalid / unexpected timeout value. Setting set_timeout(10); set_pretimeout(10); might have even more interesting results. Can you try what happens with your driver if you set those values ? Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/