Re: [PATCH 03/16] xen/arm: introduce a separate struct for watchdog timers

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2025 11:25, Mykola Kvach wrote:
> On Thu, Mar 13, 2025 at 5:27 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 05.03.2025 10:11, Mykola Kvach wrote:
>>> From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxx>
>>> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
>>
>> 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