From: Mirela Simonovic <mirela.simonovic@aggios.com>

Introduce a separate struct for watchdog timers. It is needed to properly
implement the suspend/resume actions for the watchdog timers. To be able
to restart watchdog timer after suspend we need to remember their
frequency somewhere. To not bloat the struct timer a new struct
watchdog_timer is introduced, containing the original timer and the last
set timeout.

Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
This commit was introduced in patch series V2.
---
 xen/common/keyhandler.c    |  2 +-
 xen/common/sched/core.c    | 11 ++++++-----
 xen/include/xen/sched.h    |  3 ++-
 xen/include/xen/watchdog.h |  6 ++++++
 4 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 0bb842ec00..caf614c0c2 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -305,7 +305,7 @@ static void cf_check dump_domains(unsigned char key)
         for ( i = 0 ; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
             if ( test_bit(i, &d->watchdog_inuse_map) )
                 printk("    watchdog %d expires in %d seconds\n",
-                       i, (u32)((d->watchdog_timer[i].expires - NOW()) >> 30));
+                       i, (u32)((d->watchdog_timer[i].timer.expires - NOW()) >> 30));
 
         arch_dump_domain_info(d);
 
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index d6296d99fd..b1c6b6b9fa 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -1556,7 +1556,8 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
         {
             if ( test_and_set_bit(id, &d->watchdog_inuse_map) )
                 continue;
-            set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
+            d->watchdog_timer[id].timeout = timeout;
+            set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout));
             break;
         }
         spin_unlock(&d->watchdog_lock);
@@ -1572,12 +1573,12 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
 
     if ( timeout == 0 )
     {
-        stop_timer(&d->watchdog_timer[id]);
+        stop_timer(&d->watchdog_timer[id].timer);
         clear_bit(id, &d->watchdog_inuse_map);
     }
     else
     {
-        set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
+        set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout));
     }
 
     spin_unlock(&d->watchdog_lock);
@@ -1593,7 +1594,7 @@ void watchdog_domain_init(struct domain *d)
     d->watchdog_inuse_map = 0;
 
     for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
-        init_timer(&d->watchdog_timer[i], domain_watchdog_timeout, d, 0);
+        init_timer(&d->watchdog_timer[i].timer, domain_watchdog_timeout, d, 0);
 }
 
 void watchdog_domain_destroy(struct domain *d)
@@ -1601,7 +1602,7 @@ void watchdog_domain_destroy(struct domain *d)
     unsigned int i;
 
     for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
-        kill_timer(&d->watchdog_timer[i]);
+        kill_timer(&d->watchdog_timer[i].timer);
 }
 
 /*
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 177784e6da..d0d10612ce 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -24,6 +24,7 @@
 #include <asm/current.h>
 #include <xen/vpci.h>
 #include <xen/wait.h>
+#include <xen/watchdog.h>
 #include <public/xen.h>
 #include <public/domctl.h>
 #include <public/sysctl.h>
@@ -569,7 +570,7 @@ struct domain
 #define NR_DOMAIN_WATCHDOG_TIMERS 2
     spinlock_t watchdog_lock;
     uint32_t watchdog_inuse_map;
-    struct timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
+    struct watchdog_timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
 
     struct rcu_head rcu;
 
diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h
index 4c2840bd91..2b7169632d 100644
--- a/xen/include/xen/watchdog.h
+++ b/xen/include/xen/watchdog.h
@@ -8,6 +8,12 @@
 #define __XEN_WATCHDOG_H__
 
 #include <xen/types.h>
+#include <xen/timer.h>
+
+struct watchdog_timer {
+    struct timer timer;
+    uint32_t timeout;
+};
 
 #ifdef CONFIG_WATCHDOG
 
-- 
2.43.0

On 05.03.25 11:11, Mykola Kvach wrote:
> From: Mirela Simonovic <mirela.simonovic@aggios.com>
> 
> Introduce a separate struct for watchdog timers. It is needed to properly
> implement the suspend/resume actions for the watchdog timers. To be able
> to restart watchdog timer after suspend we need to remember their
> frequency somewhere. To not bloat the struct timer a new struct
> watchdog_timer is introduced, containing the original timer and the last
> set timeout.
> 
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> ---
> This commit was introduced in patch series V2.
> ---
>   xen/common/keyhandler.c    |  2 +-
>   xen/common/sched/core.c    | 11 ++++++-----
>   xen/include/xen/sched.h    |  3 ++-
>   xen/include/xen/watchdog.h |  6 ++++++
>   4 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index 0bb842ec00..caf614c0c2 100644
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -305,7 +305,7 @@ static void cf_check dump_domains(unsigned char key)
>           for ( i = 0 ; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
>               if ( test_bit(i, &d->watchdog_inuse_map) )
>                   printk("    watchdog %d expires in %d seconds\n",
> -                       i, (u32)((d->watchdog_timer[i].expires - NOW()) >> 30));
> +                       i, (u32)((d->watchdog_timer[i].timer.expires - NOW()) >> 30));

I'd like to propose to add watchdog API wrapper here, like

watchdog_domain_expires_sec(d,id)

or

watchdog_domain_dump(d)

and so hide implementation internals.

>   
>           arch_dump_domain_info(d);
>   
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index d6296d99fd..b1c6b6b9fa 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1556,7 +1556,8 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
>           {
>               if ( test_and_set_bit(id, &d->watchdog_inuse_map) )
>                   continue;
> -            set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
> +            d->watchdog_timer[id].timeout = timeout;
> +            set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout));
>               break;
>           }
>           spin_unlock(&d->watchdog_lock);
> @@ -1572,12 +1573,12 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
>   
>       if ( timeout == 0 )
>       {
> -        stop_timer(&d->watchdog_timer[id]);
> +        stop_timer(&d->watchdog_timer[id].timer);
>           clear_bit(id, &d->watchdog_inuse_map);
>       }
>       else
>       {
> -        set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
> +        set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout));
>       }
>   
>       spin_unlock(&d->watchdog_lock);
> @@ -1593,7 +1594,7 @@ void watchdog_domain_init(struct domain *d)
>       d->watchdog_inuse_map = 0;
>   
>       for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> -        init_timer(&d->watchdog_timer[i], domain_watchdog_timeout, d, 0);
> +        init_timer(&d->watchdog_timer[i].timer, domain_watchdog_timeout, d, 0);
>   }
>   
>   void watchdog_domain_destroy(struct domain *d)
> @@ -1601,7 +1602,7 @@ void watchdog_domain_destroy(struct domain *d)
>       unsigned int i;
>   
>       for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> -        kill_timer(&d->watchdog_timer[i]);
> +        kill_timer(&d->watchdog_timer[i].timer);
>   }
>   
>   /*
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 177784e6da..d0d10612ce 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -24,6 +24,7 @@
>   #include <asm/current.h>
>   #include <xen/vpci.h>
>   #include <xen/wait.h>
> +#include <xen/watchdog.h>
>   #include <public/xen.h>
>   #include <public/domctl.h>
>   #include <public/sysctl.h>

I think struct watchdog_timer (or whatever you going to add) need to be moved in sched.h
because...

> @@ -569,7 +570,7 @@ struct domain
>   #define NR_DOMAIN_WATCHDOG_TIMERS 2
>       spinlock_t watchdog_lock;
>       uint32_t watchdog_inuse_map;
> -    struct timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
> +    struct watchdog_timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
>   
>       struct rcu_head rcu;
>   
> diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h
> index 4c2840bd91..2b7169632d 100644
> --- a/xen/include/xen/watchdog.h
> +++ b/xen/include/xen/watchdog.h
> @@ -8,6 +8,12 @@
>   #define __XEN_WATCHDOG_H__
>   
>   #include <xen/types.h>
> +#include <xen/timer.h>

...this interface is not related to domain's watchdogs.
 From x86 code, it seems like some sort of HW watchdog used to check pCPUs state
and not domains/vcpu. And it's Not enabled for Arm now.

> +
> +struct watchdog_timer {
> +    struct timer timer;
> +    uint32_t timeout;
> +};
>   
>   #ifdef CONFIG_WATCHDOG
>

Hi,

On Wed, Mar 19, 2025 at 6:14 PM Grygorii Strashko
<grygorii_strashko@epam.com> wrote:
>
>
>
> On 05.03.25 11:11, Mykola Kvach wrote:
> > From: Mirela Simonovic <mirela.simonovic@aggios.com>
> >
> > Introduce a separate struct for watchdog timers. It is needed to properly
> > implement the suspend/resume actions for the watchdog timers. To be able
> > to restart watchdog timer after suspend we need to remember their
> > frequency somewhere. To not bloat the struct timer a new struct
> > watchdog_timer is introduced, containing the original timer and the last
> > set timeout.
> >
> > Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
> > ---
> > This commit was introduced in patch series V2.
> > ---
> >   xen/common/keyhandler.c    |  2 +-
> >   xen/common/sched/core.c    | 11 ++++++-----
> >   xen/include/xen/sched.h    |  3 ++-
> >   xen/include/xen/watchdog.h |  6 ++++++
> >   4 files changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> > index 0bb842ec00..caf614c0c2 100644
> > --- a/xen/common/keyhandler.c
> > +++ b/xen/common/keyhandler.c
> > @@ -305,7 +305,7 @@ static void cf_check dump_domains(unsigned char key)
> >           for ( i = 0 ; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> >               if ( test_bit(i, &d->watchdog_inuse_map) )
> >                   printk("    watchdog %d expires in %d seconds\n",
> > -                       i, (u32)((d->watchdog_timer[i].expires - NOW()) >> 30));
> > +                       i, (u32)((d->watchdog_timer[i].timer.expires - NOW()) >> 30));
>
> I'd like to propose to add watchdog API wrapper here, like
>
> watchdog_domain_expires_sec(d,id)
>
> or
>
> watchdog_domain_dump(d)
>
> and so hide implementation internals.

It was already proposed by Jan Beulich. I'll do it.

>
> >
> >           arch_dump_domain_info(d);
> >
> > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> > index d6296d99fd..b1c6b6b9fa 100644
> > --- a/xen/common/sched/core.c
> > +++ b/xen/common/sched/core.c
> > @@ -1556,7 +1556,8 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
> >           {
> >               if ( test_and_set_bit(id, &d->watchdog_inuse_map) )
> >                   continue;
> > -            set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
> > +            d->watchdog_timer[id].timeout = timeout;
> > +            set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout));
> >               break;
> >           }
> >           spin_unlock(&d->watchdog_lock);
> > @@ -1572,12 +1573,12 @@ static long domain_watchdog(struct domain *d, uint32_t id, uint32_t timeout)
> >
> >       if ( timeout == 0 )
> >       {
> > -        stop_timer(&d->watchdog_timer[id]);
> > +        stop_timer(&d->watchdog_timer[id].timer);
> >           clear_bit(id, &d->watchdog_inuse_map);
> >       }
> >       else
> >       {
> > -        set_timer(&d->watchdog_timer[id], NOW() + SECONDS(timeout));
> > +        set_timer(&d->watchdog_timer[id].timer, NOW() + SECONDS(timeout));
> >       }
> >
> >       spin_unlock(&d->watchdog_lock);
> > @@ -1593,7 +1594,7 @@ void watchdog_domain_init(struct domain *d)
> >       d->watchdog_inuse_map = 0;
> >
> >       for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> > -        init_timer(&d->watchdog_timer[i], domain_watchdog_timeout, d, 0);
> > +        init_timer(&d->watchdog_timer[i].timer, domain_watchdog_timeout, d, 0);
> >   }
> >
> >   void watchdog_domain_destroy(struct domain *d)
> > @@ -1601,7 +1602,7 @@ void watchdog_domain_destroy(struct domain *d)
> >       unsigned int i;
> >
> >       for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> > -        kill_timer(&d->watchdog_timer[i]);
> > +        kill_timer(&d->watchdog_timer[i].timer);
> >   }
> >
> >   /*
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index 177784e6da..d0d10612ce 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -24,6 +24,7 @@
> >   #include <asm/current.h>
> >   #include <xen/vpci.h>
> >   #include <xen/wait.h>
> > +#include <xen/watchdog.h>
> >   #include <public/xen.h>
> >   #include <public/domctl.h>
> >   #include <public/sysctl.h>
>
> I think struct watchdog_timer (or whatever you going to add) need to be moved in sched.h
> because...
>
> > @@ -569,7 +570,7 @@ struct domain
> >   #define NR_DOMAIN_WATCHDOG_TIMERS 2
> >       spinlock_t watchdog_lock;
> >       uint32_t watchdog_inuse_map;
> > -    struct timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
> > +    struct watchdog_timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
> >
> >       struct rcu_head rcu;
> >
> > diff --git a/xen/include/xen/watchdog.h b/xen/include/xen/watchdog.h
> > index 4c2840bd91..2b7169632d 100644
> > --- a/xen/include/xen/watchdog.h
> > +++ b/xen/include/xen/watchdog.h
> > @@ -8,6 +8,12 @@
> >   #define __XEN_WATCHDOG_H__
> >
> >   #include <xen/types.h>
> > +#include <xen/timer.h>
>
> ...this interface is not related to domain's watchdogs.
>  From x86 code, it seems like some sort of HW watchdog used to check pCPUs state
> and not domains/vcpu. And it's Not enabled for Arm now.

Sorry, but maybe I missed something. However, this struct and the
previous watchdog timer
are used as fields of the domain struct and correspond to a particular
domain. Also, take a look
at some functions where the watchdog timer field is used: domain_watchdog,
watchdog_domain_init, and watchdog_domain_destroy.
I see a direct connection with a domain..

>
> > +
> > +struct watchdog_timer {
> > +    struct timer timer;
> > +    uint32_t timeout;
> > +};
> >
> >   #ifdef CONFIG_WATCHDOG
> >

Best regards,
Mykola

On 05.03.2025 10:11, Mykola Kvach wrote:
> From: Mirela Simonovic <mirela.simonovic@aggios.com>
> 
> Introduce a separate struct for watchdog timers. It is needed to properly
> implement the suspend/resume actions for the watchdog timers. To be able
> to restart watchdog timer after suspend we need to remember their
> frequency somewhere. To not bloat the struct timer a new struct
> watchdog_timer is introduced, containing the original timer and the last
> set timeout.
> 
> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>

A From: with no corresponding S-o-b: is potentially problematic. You also
can't simply add one with her agreement, though.

> ---
> This commit was introduced in patch series V2.

Yet, btw, the whole series isn't tagged with a version.

> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -305,7 +305,7 @@ static void cf_check dump_domains(unsigned char key)
>          for ( i = 0 ; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
>              if ( test_bit(i, &d->watchdog_inuse_map) )
>                  printk("    watchdog %d expires in %d seconds\n",
> -                       i, (u32)((d->watchdog_timer[i].expires - NOW()) >> 30));
> +                       i, (u32)((d->watchdog_timer[i].timer.expires - NOW()) >> 30));

I realize you mean to just do a mechanical replacement here, yet the use of
u32 is not only against our style (should be uint32_t then), but it's also
not clear to me that this subtraction can't ever yield a negative result.
Hence the use of %d looks more correct to me than the cast to an unsigned
type.

In any event the already long line now grows too long and hence needs
wrapping.

> @@ -569,7 +570,7 @@ struct domain
>  #define NR_DOMAIN_WATCHDOG_TIMERS 2
>      spinlock_t watchdog_lock;
>      uint32_t watchdog_inuse_map;
> -    struct timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
> +    struct watchdog_timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];

An alternative would be to have a separate array for the timeout values.
This would also save some space, seeing that on 64-bit arches you
introduce 32 bits of tail padding in the struct.

If we go the struct watchdog_timer route, may I at least suggest to rename
the field to just "watchdog", so things like &d->watchdog_timer[i].timer
don't say "timer" twice?

> --- a/xen/include/xen/watchdog.h
> +++ b/xen/include/xen/watchdog.h
> @@ -8,6 +8,12 @@
>  #define __XEN_WATCHDOG_H__
>  
>  #include <xen/types.h>
> +#include <xen/timer.h>
> +
> +struct watchdog_timer {
> +    struct timer timer;
> +    uint32_t timeout;

This wants a brief comment mentioning the granularity.

Jan

Hi,

On Thu, Mar 13, 2025 at 5:27 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.03.2025 10:11, Mykola Kvach wrote:
> > From: Mirela Simonovic <mirela.simonovic@aggios.com>
> >
> > Introduce a separate struct for watchdog timers. It is needed to properly
> > implement the suspend/resume actions for the watchdog timers. To be able
> > to restart watchdog timer after suspend we need to remember their
> > frequency somewhere. To not bloat the struct timer a new struct
> > watchdog_timer is introduced, containing the original timer and the last
> > set timeout.
> >
> > Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
> > Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>
> A From: with no corresponding S-o-b: is potentially problematic. You also
> can't simply add one with her agreement, though.

Thank you for pointing that out! I'll revisit all commits and add the missing
Signed-off-by tags in the next version of patch series.

>
> > ---
> > This commit was introduced in patch series V2.
>
> Yet, btw, the whole series isn't tagged with a version.

Yes, I added a description of the versions in the cover letter and
followed the style
used in version 2 meaning I avoided using tags. Since years have passed between
the patch series, I thought including tags might confuse reviewers.
If you want I'll add a correct tag in the next version of this patch series,
i.e. V4 instead of V2.

>
> > --- a/xen/common/keyhandler.c
> > +++ b/xen/common/keyhandler.c
> > @@ -305,7 +305,7 @@ static void cf_check dump_domains(unsigned char key)
> >          for ( i = 0 ; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
> >              if ( test_bit(i, &d->watchdog_inuse_map) )
> >                  printk("    watchdog %d expires in %d seconds\n",
> > -                       i, (u32)((d->watchdog_timer[i].expires - NOW()) >> 30));
> > +                       i, (u32)((d->watchdog_timer[i].timer.expires - NOW()) >> 30));
>
> I realize you mean to just do a mechanical replacement here, yet the use of
> u32 is not only against our style (should be uint32_t then), but it's also
> not clear to me that this subtraction can't ever yield a negative result.
> Hence the use of %d looks more correct to me than the cast to an unsigned
> type.
>
> In any event the already long line now grows too long and hence needs
> wrapping.

Maybe it would be better to send a separate patch for this. I'm not sure if such
changes are needed within the scope of this patch series

>
> > @@ -569,7 +570,7 @@ struct domain
> >  #define NR_DOMAIN_WATCHDOG_TIMERS 2
> >      spinlock_t watchdog_lock;
> >      uint32_t watchdog_inuse_map;
> > -    struct timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
> > +    struct watchdog_timer watchdog_timer[NR_DOMAIN_WATCHDOG_TIMERS];
>
> An alternative would be to have a separate array for the timeout values.
> This would also save some space, seeing that on 64-bit arches you
> introduce 32 bits of tail padding in the struct.

Maybe it will be enough to leave it as is and only change the order of
the timeout
value and the timer. This way, we will avoid potential padding issues
and still get
the benefits of using a single struct.

>
> If we go the struct watchdog_timer route, may I at least suggest to rename
> the field to just "watchdog", so things like &d->watchdog_timer[i].timer
> don't say "timer" twice?

I agree, I'll change the name of the fields to avoid duplication.

>
> > --- a/xen/include/xen/watchdog.h
> > +++ b/xen/include/xen/watchdog.h
> > @@ -8,6 +8,12 @@
> >  #define __XEN_WATCHDOG_H__
> >
> >  #include <xen/types.h>
> > +#include <xen/timer.h>
> > +
> > +struct watchdog_timer {
> > +    struct timer timer;
> > +    uint32_t timeout;
>
> This wants a brief comment mentioning the granularity.

Thanks for pointing that out, I'll add a comment.

>
> Jan

On 20.03.2025 11:25, Mykola Kvach wrote:
> On Thu, Mar 13, 2025 at 5:27 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 05.03.2025 10:11, Mykola Kvach wrote:
>>> From: Mirela Simonovic <mirela.simonovic@aggios.com>
>>>
>>> Introduce a separate struct for watchdog timers. It is needed to properly
>>> implement the suspend/resume actions for the watchdog timers. To be able
>>> to restart watchdog timer after suspend we need to remember their
>>> frequency somewhere. To not bloat the struct timer a new struct
>>> watchdog_timer is introduced, containing the original timer and the last
>>> set timeout.
>>>
>>> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
>>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>>
>> A From: with no corresponding S-o-b: is potentially problematic. You also
>> can't simply add one with her agreement, though.
> 
> Thank you for pointing that out! I'll revisit all commits and add the missing
> Signed-off-by tags in the next version of patch series.

Ftaod - you may not add anyone's S-o-b without their agreement.

>>> ---
>>> This commit was introduced in patch series V2.
>>
>> Yet, btw, the whole series isn't tagged with a version.
> 
> Yes, I added a description of the versions in the cover letter and
> followed the style
> used in version 2 meaning I avoided using tags. Since years have passed between
> the patch series, I thought including tags might confuse reviewers.
> If you want I'll add a correct tag in the next version of this patch series,
> i.e. V4 instead of V2.

Yes, no matter how much time has passed, versioning is helpful and
meaningful.

>>> --- a/xen/common/keyhandler.c
>>> +++ b/xen/common/keyhandler.c
>>> @@ -305,7 +305,7 @@ static void cf_check dump_domains(unsigned char key)
>>>          for ( i = 0 ; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
>>>              if ( test_bit(i, &d->watchdog_inuse_map) )
>>>                  printk("    watchdog %d expires in %d seconds\n",
>>> -                       i, (u32)((d->watchdog_timer[i].expires - NOW()) >> 30));
>>> +                       i, (u32)((d->watchdog_timer[i].timer.expires - NOW()) >> 30));
>>
>> I realize you mean to just do a mechanical replacement here, yet the use of
>> u32 is not only against our style (should be uint32_t then), but it's also
>> not clear to me that this subtraction can't ever yield a negative result.
>> Hence the use of %d looks more correct to me than the cast to an unsigned
>> type.
>>
>> In any event the already long line now grows too long and hence needs
>> wrapping.
> 
> Maybe it would be better to send a separate patch for this. I'm not sure if such
> changes are needed within the scope of this patch series

Simple style adjustments on lines touched anyway are generally fine. That's
better than having individual (huge) patches adjusting only style, at the
very least from a "git blame" perspective. And when avoiding such, moving
towards more modern style can only be achieved if code being touched anyway
is getting modernized at such occasions.

Jan

On 20.03.2025 11:31, Jan Beulich wrote:
> On 20.03.2025 11:25, Mykola Kvach wrote:
>> On Thu, Mar 13, 2025 at 5:27 PM Jan Beulich <jbeulich@suse.com> wrote:
>>> On 05.03.2025 10:11, Mykola Kvach wrote:
>>>> From: Mirela Simonovic <mirela.simonovic@aggios.com>
>>>>
>>>> Introduce a separate struct for watchdog timers. It is needed to properly
>>>> implement the suspend/resume actions for the watchdog timers. To be able
>>>> to restart watchdog timer after suspend we need to remember their
>>>> frequency somewhere. To not bloat the struct timer a new struct
>>>> watchdog_timer is introduced, containing the original timer and the last
>>>> set timeout.
>>>>
>>>> Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
>>>> Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
>>>
>>> A From: with no corresponding S-o-b: is potentially problematic. You also
>>> can't simply add one with her agreement, though.
>>
>> Thank you for pointing that out! I'll revisit all commits and add the missing
>> Signed-off-by tags in the next version of patch series.
> 
> Ftaod - you may not add anyone's S-o-b without their agreement.

Oh, and it would help if you could avoid submitting patches with invalid
email addresses in Cc:. Everyone replying will then experience delivery
failures.

Jan