All of lore.kernel.org
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Alexey Kardashevskiy <aik@ozlabs.ru>,
	Tzung-Bi Shih <tzungbi@kernel.org>,
	Scott Cheloha <cheloha@linux.ibm.com>
Cc: linux-watchdog@vger.kernel.org, brking@linux.ibm.com,
	nathanl@linux.ibm.com, npiggin@gmail.com,
	vaishnavi@linux.ibm.com, wvoigt@us.ibm.com
Subject: Re: [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers
Date: Tue, 10 May 2022 22:27:50 -0700	[thread overview]
Message-ID: <3ccf7a2f-4690-5b85-21f1-21df69eb0a8d@roeck-us.net> (raw)
In-Reply-To: <4f829857-5255-58d9-c3f4-7bc33980e4f6@ozlabs.ru>

On 5/10/22 22:08, Alexey Kardashevskiy wrote:
> 
> 
> On 5/10/22 13:34, Guenter Roeck wrote:
>> On 5/9/22 19:35, Tzung-Bi Shih wrote:
>>> On Mon, May 09, 2022 at 12:43:57PM -0500, Scott Cheloha wrote:
>>>> +#define SETFIELD(_v, _b, _e)    \
>>>> +    (((unsigned long)(_v) << PPC_BITLSHIFT(_e)) & PPC_BITMASK((_b), (_e)))
>>>> +#define GETFIELD(_v, _b, _e)    \
>>>> +    (((unsigned long)(_v) & PPC_BITMASK((_b), (_e))) >> PPC_BITLSHIFT(_e))
>>>
>>>> From `./scripts/checkpatch.pl --strict`:
>>> WARNING: please, no spaces at the start of a line
>>>
>>>> +#define PSERIES_WDTQL_MUST_STOP           1
>>>
>>>> From `./scripts/checkpatch.pl --strict`:
>>> WARNING: please, no space before tabs
>>>
>>>> +static const struct kernel_param_ops action_ops = { .set = action_set };
>>>> +module_param_cb(action, &action_ops, NULL, S_IRUGO);
>>>
>>>> From `./scripts/checkpatch.pl --strict`:
>>> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
>>> octal permissions '0444'.
>>>
>>>> +MODULE_PARM_DESC(action, "Action taken when watchdog expires: \"hard-poweroff\", \"hard-restart\", or \"dump-restart\" (default=\"hard-restart\")");
>>>
>>> The line exceeds 100 columns.
>>>
>>>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>>>> +module_param(nowayout, bool, S_IRUGO);
>>>
>>>> From `./scripts/checkpatch.pl --strict`:
>>> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
>>> octal permissions '0444'.
>>>
>>>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>>>
>>>> From `./scripts/checkpatch.pl --strict`, the line exceeds 100 columns.
>>>
>>>> +#define WATCHDOG_TIMEOUT 60
>>>> +static unsigned int timeout = WATCHDOG_TIMEOUT;
>>>> +module_param(timeout, uint, S_IRUGO);
>>>
>>>> From `./scripts/checkpatch.pl --strict`:
>>> WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using
>>> octal permissions '0444'.
>>>
>>>> +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds (default=" __MODULE_STRING(WATCHDOG_TIMEOUT) ")");
>>>
>>>> From `./scripts/checkpatch.pl --strict`, the line exceeds 100 columns.
>>>
>>>> +struct pseries_wdt {
>>>> +    struct watchdog_device wd;
>>>> +    unsigned long num;        /* NB: Watchdog numbers are 1-based */
>>>
>>> What does NB stand for?  Could it be removed from the comment?
>>>
>>
>> Latin "Nota Bene", for "This is important".
>> All comments should be important, so I agree, this has little
>> if any value.
>>
>>> Does `timer_id` or some other equivalent names make more sense for the
>>> variable?
>>>
>>>> +static int pseries_wdt_start(struct watchdog_device *wdd)
>>>> +{
>>> [...]
>>>> +    rc = plpar_hcall_norets(H_WATCHDOG, flags, pw->num, msecs);
>>>> +    if (rc != H_SUCCESS) {
>>>> +        dev_crit(dev, "H_WATCHDOG: %ld: failed to start timer %lu",
>>>> +             rc, pw->num);
>>>> +               return -EIO;
>>>
>>>> From `./scripts/checkpatch.pl --strict`:
>>> ERROR: code indent should use tabs where possible
>>> WARNING: please, no space before tabs
>>>
>>>> +static struct watchdog_info pseries_wdt_info = {
>>>> +    .identity = DRV_NAME,
>>>> +    .options = WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT \
>>>> +        | WDIOF_PRETIMEOUT,
>>>
>>>> From `./scripts/checkpatch.pl --strict`:
>>> WARNING: Avoid unnecessary line continuations
>>>
>>>> +static const struct watchdog_ops pseries_wdt_ops = {
>>>> +    .owner = THIS_MODULE,
>>>> +    .ping = pseries_wdt_start,
>>>
>>> Does this mean: it needs hard restart for every ping?
>>>
>>
>> If there is no separate ping function, there is no need to point
>> the ping function to the start function. The watchdog core uses
>> the start function automatically in this case.
>>
>>>> +static int pseries_wdt_probe(struct platform_device *pdev)
>>>> +{
>>> [...]
>>>> +    rc = plpar_hcall(H_WATCHDOG, ret, PSERIES_WDTF_OP_QUERY);
>>>> +    if (rc != H_SUCCESS)
>>>> +        return (rc == H_FUNCTION) ? -ENODEV : -EIO;
>>>
>>> The parentheses can be dropped.
>>>
>>>> +    pw = devm_kzalloc(&pdev->dev, sizeof *pw, GFP_KERNEL);
>>>> +    if (pw == NULL)
>>>
>>>> From `./scripts/checkpatch.pl --strict`:
>>> CHECK: Comparison to NULL could be written "!pw"
>>>
>>>> +    pw->num = pdev->id + 1;        /* 0-based -> 1-based */
>>>
>>> Didn't see where the platform device was registered but using the pdev->id as
>>> the timer id could be unreliable (e.g. from auto increment).
>>
>> That is at the beginning of the patch, in arch/powerpc/platforms/pseries/setup.c,
>> which calls platform_device_register_simple() with an explicit device ID.
>>
>> I agree, though, that it is fragile: The code in the probe function
>> explicitly checks for a negative ID, which would only be provided if
>> platform_device_register() explicitly declares that (ie if it provides
>> PLATFORM_DEVID_NONE as device ID). But if that is a concern, there
>> might as well be code registering a platform device with
>> PLATFORM_DEVID_AUTO as device ID, and then th device id would be automatic
>> and more or less random. I think the instantiation code should be more
>> explicit: Either assume that PLATFORM_DEVID_NONE or PLATFORM_DEVID_AUTO
>> will never be used to register the watchdog devices, or provide other
>> information such as platform data to make it explicit.
>>
>> Also, the changes in arch code need to be made in separate patches.
> 
> 
> Often this is true but in case like this - the driver itself is dead code until the platform enables it and if there is a problem with the driver - bisect will point to the device enablement commit. Backporting becomes a problem as it is going to be 3 apart patches vs. one:
> 1) define hypercall
> 2) implement the driver
> 3) enable the driver + device FW feature flag.
> 
> Instead, either maintainer (powerpc or watchdog) says "ack" and the other one puts it in the tree, I saw this in practice. Thanks,
> 

I won't accept this. I don't mind giving an Ack to a patch applied elsewhere,
and I don't mind applying patches from another subsystem if Acked by affected
maintainers, but I strongly believe that there should be no cross-subsystem
patches. Also, I don't consider 2) and 3) to be a single logical change,
and the rule for patches is "one logical change per patch".

Other maintainers may handle this differently, but that is their call and
responsibility.

Guenter

  reply	other threads:[~2022-05-11  5:27 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-09 17:43 [RFC v2 0/2] Add driver for PAPR watchdog timers Scott Cheloha
2022-05-09 17:43 ` [RFC v2 1/2] powerpc/pseries: hvcall.h: add H_WATCHDOG opcode, H_NOOP return code Scott Cheloha
2022-05-09 17:43 ` [RFC v2 2/2] watchdog: pseries-wdt: initial support for PAPR virtual watchdog timers Scott Cheloha
2022-05-10  2:35   ` Tzung-Bi Shih
2022-05-10  3:34     ` Guenter Roeck
2022-05-11  5:08       ` Alexey Kardashevskiy
2022-05-11  5:27         ` Guenter Roeck [this message]
2022-05-10 15:48     ` Scott Cheloha
2022-05-10 16:04       ` Guenter Roeck
2022-05-10 16:15         ` Scott Cheloha
2022-05-10 19:00           ` Guenter Roeck
2022-05-11  5:38 ` [RFC v2 0/2] Add driver for PAPR " Alexey Kardashevskiy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3ccf7a2f-4690-5b85-21f1-21df69eb0a8d@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=aik@ozlabs.ru \
    --cc=brking@linux.ibm.com \
    --cc=cheloha@linux.ibm.com \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=nathanl@linux.ibm.com \
    --cc=npiggin@gmail.com \
    --cc=tzungbi@kernel.org \
    --cc=vaishnavi@linux.ibm.com \
    --cc=wvoigt@us.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.