Re: [PATCH 1/2] watchdog: introduce watchdog.open_timeout commandline parameter

2017-11-28 Thread Guenter Roeck 
On Tue, Nov 28, 2017 at 11:35:49AM +0100, Rasmus Villemoes wrote:
> The watchdog framework takes care of feeding a hardware watchdog until
> userspace opens /dev/watchdogN. If that never happens for some reason
> (buggy init script, corrupt root filesystem or whatnot) but the kernel
> itself is fine, the machine stays up indefinitely. This patch allows
> setting an upper limit for how long the kernel will take care of the
> watchdog, thus ensuring that the watchdog will eventually reset the
> machine.
> 
> This is particularly useful for embedded devices where some fallback
> logic is implemented in the bootloader (e.g., use a different root
> partition, boot from network, ...).
> 
> The existing handle_boot_enabled parameter has the same purpose, but
> that is only usable when the hardware watchdog has a sufficiently long
> timeout (possibly configured by the bootloader). Many hardware watchdogs
> cannot be configured, or can only be configured up to a certain value,
> making the timeout short enough that it is completely impossible to have
> userspace ready soon enough. Hence it is necessary for the kernel to
> handle those watchdogs for a while.
> 
> The open timeout is also used as a maximum time for an application to
> re-open /dev/watchdogN after closing it.
> 
> A value of 0 (the default) means infinite timeout, preserving the
> current behaviour.
> 
> The unit is milliseconds rather than seconds because that covers more
> use cases. For example, one can effectively disable the kernel handling
> by setting the open_timeout to 1 ms. There are also customers with very
> strict requirements that may want to set the open_timeout to something
> like 4500 ms, which combined with a hardware watchdog that must be
> pinged every 250 ms ensures userspace is up no more than 5 seconds after
> the bootloader hands control to the kernel (250 ms until the driver gets
> registered and kernel handling starts, 4500 ms of kernel handling, and
> then up to 250 ms from the last ping until userspace takes over).

This is quite vague, especially since it doesn't count the time from
boot to starting the watchdog driver, which can vary even across boots.
Why not make it specific, for example by adjusting the open timeout with
ktime_get_boot_ns() ?

I would actually make it even more specific and calculate the open
timeout such that the system would reboot after open_timeout, not
after . Any reason for not doing that ?
The upside would be more accuracy, and I don't really see a downside.

Thanks,
Guenter

> 
> Signed-off-by: Rasmus Villemoes 
> Reviewed-by: Esben Haabendal 
> ---
>  Documentation/watchdog/watchdog-parameters.txt |  8 
>  drivers/watchdog/watchdog_dev.c| 27 
> +-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/watchdog/watchdog-parameters.txt 
> b/Documentation/watchdog/watchdog-parameters.txt
> index 6f9d7b4..5363bf3 100644
> --- a/Documentation/watchdog/watchdog-parameters.txt
> +++ b/Documentation/watchdog/watchdog-parameters.txt
> @@ -8,6 +8,14 @@ See Documentation/admin-guide/kernel-parameters.rst for 
> information on
>  providing kernel parameters for builtin drivers versus loadable
>  modules.
>  
> +The watchdog core parameter watchdog.open_timeout is the maximum time,
> +in milliseconds, for which the watchdog framework will take care of
> +pinging a hardware watchdog until userspace opens the corresponding
> +/dev/watchdogN device. A value of 0 (the default) means an infinite
> +timeout. Setting this to a non-zero value can be useful to ensure that
> +either userspace comes up properly, or the board gets reset and allows
> +fallback logic in the bootloader to try something else.
> +
>  
>  -
>  acquirewdt:
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 1e971a5..b4985db 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -67,6 +67,7 @@ struct watchdog_core_data {
>   struct mutex lock;
>   unsigned long last_keepalive;
>   unsigned long last_hw_keepalive;
> + unsigned long open_deadline;
>   struct delayed_work work;
>   unsigned long status;   /* Internal status bits */
>  #define _WDOG_DEV_OPEN   0   /* Opened ? */
> @@ -83,6 +84,19 @@ static struct workqueue_struct *watchdog_wq;
>  
>  static bool handle_boot_enabled =
>   IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
> +static unsigned open_timeout;
> +
> +static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
> +{
> + if (!open_timeout)
> + return false;
> + return time_is_before_jiffies(data->open_deadline);
> +}
> +
> +static void watchdog_set_open_deadline(struct watchdog_core_data *data)
> +{
> + data->open_deadline = jiffies + msecs_to_jiffies(open_timeout);


> +}
>  
>  static inline

Re: [PATCH v5 11/11] intel_sgx: driver documentation

2017-11-28 Thread Jarkko Sakkinen 
On Tue, Nov 28, 2017 at 10:37:48PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 27, 2017 at 09:03:39AM -0800, Sean Christopherson wrote:
> > I have a branch based on Jarkko's patches (I believe it's up-to-date with 
> > v5)
> > that implements what I described.  I'd be happy to send RFC patches if that
> > would help.
> 
> That would only slow things down. The code is easy to move around and
> I'm doing infrastructure changes as part of the review process. The
> latest version (v6) that I sent on Sat has struct sgx_epc_page removed
> just to name an example.
> 
> Rather than sending any deprecated patches it would be more useful to
> get input (in English) on directory layout.
> 
> I guess you missed v6 as it I had to drop the 01.org list temporarily.
> It will be back in v7 as I was able to retrieve the admin password and
> configure it in suitable way.

Once my series has been landed, you could drop a series for review. Then
it does make sense. I won't of course add any exports needed by KVM etc.
I'm only doing "lowest common denominator" groundwork.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 11/11] intel_sgx: driver documentation

2017-11-28 Thread Jarkko Sakkinen 
On Mon, Nov 27, 2017 at 09:03:39AM -0800, Sean Christopherson wrote:
> I have a branch based on Jarkko's patches (I believe it's up-to-date with v5)
> that implements what I described.  I'd be happy to send RFC patches if that
> would help.

That would only slow things down. The code is easy to move around and
I'm doing infrastructure changes as part of the review process. The
latest version (v6) that I sent on Sat has struct sgx_epc_page removed
just to name an example.

Rather than sending any deprecated patches it would be more useful to
get input (in English) on directory layout.

I guess you missed v6 as it I had to drop the 01.org list temporarily.
It will be back in v7 as I was able to retrieve the admin password and
configure it in suitable way.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Are .txt -> .rst patches welcome

2017-11-28 Thread Tobin C. Harding 
On Tue, Nov 28, 2017 at 11:03:56AM -0700, Jonathan Corbet wrote:
> On Fri, 24 Nov 2017 10:30:56 +1100
> "Tobin C. Harding"  wrote:
> 
> > What's the status of the move from .txt to .rst files within the
> > kernel?
> > 
> > The point of the question; are patches converting .txt files in
> > Documentation/ to .rst files welcome?
> 
> As a general rule, yes, they are.  That said, I'm much happier when these
> patches also involve a look at the documentation to ensure that it's still
> current and worth keeping and also include some thought as to where the
> converted documentation should go in a more rational hierarchy.
> 
> A different way of putting it would be: Documentation/ now is a mess.
> Simply converting .txt files to RST does not, on its own, reduce the mess
> factor much.  But it can be a part of the bigger effort to turn the
> kernel's documentation into some sort of coherent whole.

Cool, thanks for the reply. I was hoping to do the odd conversion for
the purely selfish reason that updating the doc makes me read (and
understand) them thoroughly. I'll keep your points in mind.

thanks,
Tobin.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] MAINTAINERS: regulator: Add Documentation/power/regulator/

2017-11-28 Thread Jonathan Neuschäfer 
On Tue, Nov 28, 2017 at 03:15:37PM +, Mark Brown wrote:
> On Tue, Nov 28, 2017 at 05:22:02AM +0100, Jonathan Neuschäfer wrote:
> > On Sun, Nov 19, 2017 at 06:09:06AM +0100, Jonathan Neuschäfer wrote:
> > > Signed-off-by: Jonathan Neuschäfer 
> > > ---
> > >  MAINTAINERS | 1 +
> > >  1 file changed, 1 insertion(+)
> > 
> > Ping.
> > 
> > Should I resend this series with Cc: linux-doc@vger.kernel.org?
> 
> Please don't send content free pings and please allow a reasonable time
> for review.  People get busy, go on holiday, attend conferences and so 
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review.  If there have been
> review comments then people may be waiting for those to be addressed.

Okay. Sorry.

> Sending content free pings adds to the mail volume (if they are seen at
> all) which is often the problem and since they can't be reviewed
> directly if something has gone wrong you'll have to resend the patches
> anyway, though there are some other maintainers who like them - if in
> doubt look at how patches for the subsystem are normally handled.

Right, this makes sense.


Thanks,
Jonathan Neuschäfer


signature.asc
Description: PGP signature

Re: Are .txt -> .rst patches welcome

2017-11-28 Thread Jonathan Corbet 
On Fri, 24 Nov 2017 10:30:56 +1100
"Tobin C. Harding"  wrote:

> What's the status of the move from .txt to .rst files within the
> kernel?
> 
> The point of the question; are patches converting .txt files in
> Documentation/ to .rst files welcome?

As a general rule, yes, they are.  That said, I'm much happier when these
patches also involve a look at the documentation to ensure that it's still
current and worth keeping and also include some thought as to where the
converted documentation should go in a more rational hierarchy.

A different way of putting it would be: Documentation/ now is a mess.
Simply converting .txt files to RST does not, on its own, reduce the mess
factor much.  But it can be a part of the bigger effort to turn the
kernel's documentation into some sort of coherent whole.

Thanks,

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] MAINTAINERS: regulator: Add Documentation/power/regulator/

2017-11-28 Thread Mark Brown 
On Tue, Nov 28, 2017 at 05:22:02AM +0100, Jonathan Neuschäfer wrote:
> On Sun, Nov 19, 2017 at 06:09:06AM +0100, Jonathan Neuschäfer wrote:
> > Signed-off-by: Jonathan Neuschäfer 
> > ---
> >  MAINTAINERS | 1 +
> >  1 file changed, 1 insertion(+)
> 
> Ping.
> 
> Should I resend this series with Cc: linux-doc@vger.kernel.org?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, though there are some other maintainers who like them - if in
doubt look at how patches for the subsystem are normally handled.


signature.asc
Description: PGP signature

Re: [PATCH V2 7/7] PCI: make reset poll time adjustable

2017-11-28 Thread Sinan Kaya 
On 11/28/2017 9:20 AM, Bjorn Helgaas wrote:
>> Introduce pci=resetpolltime= argument to override 60 seconds poll time in
>> units of milliseconds.
> I resist adding kernel parameters because they really complicate the
> user experience.  Obviously you added this for a reason, but I don't
> know what that is.  If we really need it, is there any way we could
> automatically figure out in the kernel when we need different poll
> times?
> 

No, we can drop this patch if we want to . This was mostly a
scalability concern. 

I worried adding 60 seconds would be too much for some use case. We can
hold onto this change until that use case happens if you want.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 7/7] PCI: make reset poll time adjustable

2017-11-28 Thread Bjorn Helgaas 
On Mon, Nov 27, 2017 at 01:20:28AM -0500, Sinan Kaya wrote:
> Introduce pci=resetpolltime= argument to override 60 seconds poll time in
> units of milliseconds.

I resist adding kernel parameters because they really complicate the
user experience.  Obviously you added this for a reason, but I don't
know what that is.  If we really need it, is there any way we could
automatically figure out in the kernel when we need different poll
times?

> Signed-off-by: Sinan Kaya 
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  2 ++
>  drivers/pci/pci.c   | 13 -
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> b/Documentation/admin-guide/kernel-parameters.txt
> index 0549662..a07d4f5 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3071,6 +3071,8 @@
>   pcie_scan_all   Scan all possible PCIe devices.  Otherwise we
>   only look for one device below a PCIe downstream
>   port.
> + resetpolltime=  Adjusts the default poll time following hot 
> reset
> + and D3->D0 transition.
>  
>   pcie_aspm=  [PCIE] Forcibly enable or disable PCIe Active State 
> Power
>   Management.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8472c24..a6c3e25 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -127,7 +127,7 @@ static int __init pcie_port_pm_setup(char *str)
>  
>  /* time to wait after a reset for device to become responsive */
>  #define PCIE_RESET_READY_POLL_MS 6
> -
> +static unsigned long pci_reset_polltime = PCIE_RESET_READY_POLL_MS;
>  /**
>   * pci_bus_max_busnr - returns maximum PCI bus number of given bus' children
>   * @bus: pointer to PCI bus structure to search
> @@ -3904,7 +3904,7 @@ int pcie_flr(struct pci_dev *dev)
>*/
>   msleep(100);
>  
> - return pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);
> + return pci_dev_wait(dev, "FLR", pci_reset_polltime);
>  }
>  EXPORT_SYMBOL_GPL(pcie_flr);
>  
> @@ -3945,7 +3945,7 @@ static int pci_af_flr(struct pci_dev *dev, int probe)
>*/
>   msleep(100);
>  
> - return pci_dev_wait(dev, "AF_FLR", PCIE_RESET_READY_POLL_MS);
> + return pci_dev_wait(dev, "AF_FLR", pci_reset_polltime);
>  }
>  
>  /**
> @@ -3990,7 +3990,7 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
>   pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
>   pci_dev_d3_sleep(dev);
>  
> - return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS);
> + return pci_dev_wait(dev, "PM D3->D0", pci_reset_polltime);
>  }
>  
>  void pci_reset_secondary_bus(struct pci_dev *dev)
> @@ -4035,7 +4035,7 @@ int pci_reset_bridge_secondary_bus(struct pci_dev *dev)
>  {
>   pcibios_reset_secondary_bus(dev);
>  
> - return pci_dev_wait(dev, "bus reset", PCIE_RESET_READY_POLL_MS);
> + return pci_dev_wait(dev, "bus reset", pci_reset_polltime);
>  }
>  EXPORT_SYMBOL_GPL(pci_reset_bridge_secondary_bus);
>  
> @@ -5528,6 +5528,9 @@ static int __init pci_setup(char *str)
>   pcie_bus_config = PCIE_BUS_PEER2PEER;
>   } else if (!strncmp(str, "pcie_scan_all", 13)) {
>   pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
> + } else if (!strncmp(str, "resetpolltime=", 14)) {
> + pci_reset_polltime =
> + simple_strtoul(str + 14, , 0);
>   } else {
>   printk(KERN_ERR "PCI: Unknown option `%s'\n",
>   str);
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/i915/guc: Fix doc reference to intel_guc_fw.c

2017-11-28 Thread Jonathan Neuschäfer 
On Tue, Nov 28, 2017 at 09:51:13AM +0100, Michal Wajdeczko wrote:
> On Tue, 28 Nov 2017 07:50:52 +0100, Jonathan Neuschäfer
>  wrote:
> 
> > Sphinx complains that it can't find intel_guc_loader.c, and rightly so:
> > The file has been renamed.
> > 
> > Fixes: e8668bbcb0f9 ("drm/i915/guc: Rename intel_guc_loader.c to
> > intel_guc_fw.c")
> > Cc: Michal Wajdeczko 
> > Signed-off-by: Jonathan Neuschäfer 
> > ---
> >  Documentation/gpu/i915.rst | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> > index 2e7ee0313c1c..e21698e16534 100644
> > --- a/Documentation/gpu/i915.rst
> > +++ b/Documentation/gpu/i915.rst
> > @@ -341,10 +341,10 @@ GuC
> >  GuC-specific firmware loader
> >  
> > -.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_loader.c
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_fw.c
> > :doc: GuC-specific firmware loader
> > -.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_loader.c
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_fw.c
> > :internal:
> > GuC-based command submission
> 
> + Ville
> 
> Well, this will fix sphinx error, but in my opinion it will not make
> i915 documentation any better. See my earlier patch/comments in [1].

Thanks for the pointer.

Hmm, right, given that there's no "DOC:" line in intel_guc_fw.c anymore,
the ":doc:" directive above is not useful.

As a tiny step towards more complete documentation (and to keep people
from patching this spot again ;), IMHO it makes sense to do this (it's
of course up to the maintainers whether they agree):

---
diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
index 2e7ee0313c1c..e94d3ac2bdd0 100644
--- a/Documentation/gpu/i915.rst
+++ b/Documentation/gpu/i915.rst
@@ -341,10 +341,7 @@ GuC
 GuC-specific firmware loader
 
 
-.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_loader.c
-   :doc: GuC-specific firmware loader
-
-.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_loader.c
+.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_fw.c
:internal:
 
 GuC-based command submission
---

> So maybe better to wait for other comments which way to go.

Makes sense.


Thanks,
Jonathan Neuschäfer

> 
> Thanks for the patch,
> Michal
> 
> [1] https://patchwork.freedesktop.org/patch/188424/


signature.asc
Description: PGP signature

[PATCH 2/2] watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

2017-11-28 Thread Rasmus Villemoes 
This allows setting a default value for the watchdog.open_timeout
commandline parameter via Kconfig.

Some BSPs allow remote updating of the kernel image and root file
system, but updating the bootloader requires physical access. Hence, if
one has a firmware update that requires relaxing the
watchdog.open_timeout a little, the value used must be baked into the
kernel image itself and cannot come from the u-boot environment via the
kernel command line.

Being able to set the initial value in .config doesn't change the fact
that the value on the command line, if present, takes precedence, and is
of course immensely useful for development purposes while one has
console acccess, as well as usable in the cases where one can make a
permanent update of the kernel command line.

Signed-off-by: Rasmus Villemoes 
Reviewed-by: Esben Haabendal 
---
 Documentation/watchdog/watchdog-parameters.txt | 3 ++-
 drivers/watchdog/Kconfig   | 9 +
 drivers/watchdog/watchdog_dev.c| 2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/watchdog/watchdog-parameters.txt 
b/Documentation/watchdog/watchdog-parameters.txt
index 5363bf3..60dd2be 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -11,7 +11,8 @@ modules.
 The watchdog core parameter watchdog.open_timeout is the maximum time,
 in milliseconds, for which the watchdog framework will take care of
 pinging a hardware watchdog until userspace opens the corresponding
-/dev/watchdogN device. A value of 0 (the default) means an infinite
+/dev/watchdogN device. The defalt value is
+CONFIG_WATCHDOG_OPEN_TIMEOUT. A value of 0 means an infinite
 timeout. Setting this to a non-zero value can be useful to ensure that
 either userspace comes up properly, or the board gets reset and allows
 fallback logic in the bootloader to try something else.
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index ca200d1..a142e1e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -63,6 +63,15 @@ config WATCHDOG_SYSFS
  Say Y here if you want to enable watchdog device status read through
  sysfs attributes.
 
+config WATCHDOG_OPEN_TIMEOUT
+   int "Timeout value for opening watchdog device"
+   default 0
+   help
+ The maximum time, in milliseconds, for which the watchdog
+ framework takes care of pinging a hardware watchdog. A value
+ of 0 means infinite. The value set here can be overridden by
+ the commandline parameter "watchdog.open_timeout".
+
 #
 # General Watchdog drivers
 #
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index b4985db..9f18952 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -84,7 +84,7 @@ static struct workqueue_struct *watchdog_wq;
 
 static bool handle_boot_enabled =
IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
-static unsigned open_timeout;
+static unsigned open_timeout = CONFIG_WATCHDOG_OPEN_TIMEOUT;
 
 static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
 {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 0/2] watchdog: allow setting deadline for opening /dev/watchdogN

2017-11-28 Thread Rasmus Villemoes 
If a watchdog driver tells the framework that the device is running,
the framework takes care of feeding the watchdog until userspace opens
the device. If the userspace application which is supposed to do that
never comes up properly, the watchdog is fed indefinitely by the
kernel. This can be especially problematic for embedded devices.

The existing handle_boot_enabled cmdline parameter/config option
partially solves that, but that is only usable for the subset of
hardware watchdogs that have (or can be configured by the bootloader
to have) a timeout that is sufficient to make it realistic for
userspace to come up. Many devices have timeouts of a second, or even
less, making handle_boot_enabled insufficient.

These patches allow one to set a maximum time for which the kernel
will feed the watchdog, thus ensuring that either userspace has come
up, or the board gets reset. This allows fallback logic in the
bootloader to attempt some recovery (for example, if an automatic
update is in progress, it could roll back to the previous version).

The patches have been tested on a Raspberry Pi 2 and a Wandboard.

A preparatory patch of this series has already been merged
(c013b65ad8a1e "watchdog: introduce watchdog_worker_should_ping
helper"). On 2017-07-08, Guenter wrote [1]

  It is sufficiently different to handle_boot_enabled to keep it
  separate. I am mostly ok with the patch.  One comment below.

That one comment (regarding the placement of the module_param) has
been addressed in this version.

There has been some opposition to making the default value of
watchdog.open_timeout configurable in Kconfig, but in [2] Guenter said

  I used to be opposed to it, but it does seem to make some sense to
  me now after thinking about it.

I do hope that these patches can now find their way into the kernel,
but if 2/2 is somehow still controversial, please consider just taking
1/2. (I can't help but noting that handle_boot_enabled does get its
default value from Kconfig, and nobody complained about that when that
option was added).

[1] https://patchwork.kernel.org/patch/9754095/
[2] https://patchwork.kernel.org/patch/9754093/

Rasmus Villemoes (2):
  watchdog: introduce watchdog.open_timeout commandline parameter
  watchdog: introduce CONFIG_WATCHDOG_OPEN_TIMEOUT

 Documentation/watchdog/watchdog-parameters.txt |  9 +
 drivers/watchdog/Kconfig   |  9 +
 drivers/watchdog/watchdog_dev.c| 27 +-
 3 files changed, 44 insertions(+), 1 deletion(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] watchdog: introduce watchdog.open_timeout commandline parameter

2017-11-28 Thread Rasmus Villemoes 
The watchdog framework takes care of feeding a hardware watchdog until
userspace opens /dev/watchdogN. If that never happens for some reason
(buggy init script, corrupt root filesystem or whatnot) but the kernel
itself is fine, the machine stays up indefinitely. This patch allows
setting an upper limit for how long the kernel will take care of the
watchdog, thus ensuring that the watchdog will eventually reset the
machine.

This is particularly useful for embedded devices where some fallback
logic is implemented in the bootloader (e.g., use a different root
partition, boot from network, ...).

The existing handle_boot_enabled parameter has the same purpose, but
that is only usable when the hardware watchdog has a sufficiently long
timeout (possibly configured by the bootloader). Many hardware watchdogs
cannot be configured, or can only be configured up to a certain value,
making the timeout short enough that it is completely impossible to have
userspace ready soon enough. Hence it is necessary for the kernel to
handle those watchdogs for a while.

The open timeout is also used as a maximum time for an application to
re-open /dev/watchdogN after closing it.

A value of 0 (the default) means infinite timeout, preserving the
current behaviour.

The unit is milliseconds rather than seconds because that covers more
use cases. For example, one can effectively disable the kernel handling
by setting the open_timeout to 1 ms. There are also customers with very
strict requirements that may want to set the open_timeout to something
like 4500 ms, which combined with a hardware watchdog that must be
pinged every 250 ms ensures userspace is up no more than 5 seconds after
the bootloader hands control to the kernel (250 ms until the driver gets
registered and kernel handling starts, 4500 ms of kernel handling, and
then up to 250 ms from the last ping until userspace takes over).

Signed-off-by: Rasmus Villemoes 
Reviewed-by: Esben Haabendal 
---
 Documentation/watchdog/watchdog-parameters.txt |  8 
 drivers/watchdog/watchdog_dev.c| 27 +-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/Documentation/watchdog/watchdog-parameters.txt 
b/Documentation/watchdog/watchdog-parameters.txt
index 6f9d7b4..5363bf3 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt
@@ -8,6 +8,14 @@ See Documentation/admin-guide/kernel-parameters.rst for 
information on
 providing kernel parameters for builtin drivers versus loadable
 modules.
 
+The watchdog core parameter watchdog.open_timeout is the maximum time,
+in milliseconds, for which the watchdog framework will take care of
+pinging a hardware watchdog until userspace opens the corresponding
+/dev/watchdogN device. A value of 0 (the default) means an infinite
+timeout. Setting this to a non-zero value can be useful to ensure that
+either userspace comes up properly, or the board gets reset and allows
+fallback logic in the bootloader to try something else.
+
 
 -
 acquirewdt:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 1e971a5..b4985db 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -67,6 +67,7 @@ struct watchdog_core_data {
struct mutex lock;
unsigned long last_keepalive;
unsigned long last_hw_keepalive;
+   unsigned long open_deadline;
struct delayed_work work;
unsigned long status;   /* Internal status bits */
 #define _WDOG_DEV_OPEN 0   /* Opened ? */
@@ -83,6 +84,19 @@ static struct workqueue_struct *watchdog_wq;
 
 static bool handle_boot_enabled =
IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
+static unsigned open_timeout;
+
+static bool watchdog_past_open_deadline(struct watchdog_core_data *data)
+{
+   if (!open_timeout)
+   return false;
+   return time_is_before_jiffies(data->open_deadline);
+}
+
+static void watchdog_set_open_deadline(struct watchdog_core_data *data)
+{
+   data->open_deadline = jiffies + msecs_to_jiffies(open_timeout);
+}
 
 static inline bool watchdog_need_worker(struct watchdog_device *wdd)
 {
@@ -200,7 +214,13 @@ static bool watchdog_worker_should_ping(struct 
watchdog_core_data *wd_data)
 {
struct watchdog_device *wdd = wd_data->wdd;
 
-   return wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd));
+   if (!wdd)
+   return false;
+
+   if (watchdog_active(wdd))
+   return true;
+
+   return watchdog_hw_running(wdd) && 
!watchdog_past_open_deadline(wd_data);
 }
 
 static void watchdog_ping_work(struct work_struct *work)
@@ -861,6 +881,7 @@ static int watchdog_release(struct inode *inode, struct 
file *file)
watchdog_ping(wdd);
}
 
+   watchdog_set_open_deadline(wd_data);

Re: [PATCH] drm/i915/guc: Fix doc reference to intel_guc_fw.c

2017-11-28 Thread Michal Wajdeczko 
On Tue, 28 Nov 2017 07:50:52 +0100, Jonathan Neuschäfer  
 wrote:



Sphinx complains that it can't find intel_guc_loader.c, and rightly so:
The file has been renamed.

Fixes: e8668bbcb0f9 ("drm/i915/guc: Rename intel_guc_loader.c to  
intel_guc_fw.c")

Cc: Michal Wajdeczko 
Signed-off-by: Jonathan Neuschäfer 
---
 Documentation/gpu/i915.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
index 2e7ee0313c1c..e21698e16534 100644
--- a/Documentation/gpu/i915.rst
+++ b/Documentation/gpu/i915.rst
@@ -341,10 +341,10 @@ GuC
 GuC-specific firmware loader
 
-.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_loader.c
+.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_fw.c
:doc: GuC-specific firmware loader
-.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_loader.c
+.. kernel-doc:: drivers/gpu/drm/i915/intel_guc_fw.c
:internal:
GuC-based command submission


+ Ville

Well, this will fix sphinx error, but in my opinion it will not make
i915 documentation any better. See my earlier patch/comments in [1].
So maybe better to wait for other comments which way to go.

Thanks for the patch,
Michal

[1] https://patchwork.freedesktop.org/patch/188424/
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html