Re: [PATCH v1 1/1] watchdog: iTCO_wdt: Move update_no_reboot_bit() out of atomic context

[sathyanarayanan kuppuswamy]

Hi,


On 08/14/2017 10:49 AM, Guenter Roeck wrote:
> On Thu, Jul 27, 2017 at 11:08:44AM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> >
> > In iTCO_wdt_start() and iTCO_wdt_stop() functions, update_no_reboot_bit()
> > call has been made within io_lock spin lock context. But if the
> > update_no_reboot_bit() function is implemented by chipset/PMC driver
> > then we can't be sure whether their implementation does not include any
> > sleeping calls. Also, iTCO_wdt io_lock is mainly intended for protecting the
> > watchdog IO operations and has nothing to with no_reboot_bit update.
> I disagree. The spinlock also protects update_no_reboot_bit_{pci,mem}().
> Those functions do a read-modify-write, read back the original value,
> and bail out if the reported value is unexpected. If protecting those
> functions is unnecessary, we may as well claim that the spinlock itself
> is unnecessary to start with.
I thought io_lock is only intended for protecting in/out operations. 
Thanks for
clarifying it.
> This also changes the order of calls, now calling iTCO_vendor_pre_start()
> _after_ updating no_reboot_bit. While that should not matter, it adds another
> level of risk and uncertainty.
>
> > Currently, update_no_reboot_bit() function implemented by intel_pmc_ipc
> > driver uses mutex_lock() which in turn causes "sleeping into atomic
> > context" issue in iTCO_wdt_start()/iTCO_wdt_stop() functions.
> A better fix might be to use a spinlock for gcr reads and updates
> in the intel driver. Note that it isn't entirely clear to me why gcr
> register accesses are protected by the mutex used to protect ipc
> command execution, or why gcr_data_readq() is unprotected but
> intel_pmc_gcr_read() is, or why is_gcr_valid() would need to be protected.
Agreed.  Will submit a fix for it in intel_pmc_ipc driver.

> Thanks,
> Guenter
>
> > So moving the update_no_reboot_bit() function out of atomic context.
> >
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/watchdog/iTCO_wdt.c | 13 ++++++-------
> >   1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> > index c4f6587..5a018b2 100644
> > --- a/drivers/watchdog/iTCO_wdt.c
> > +++ b/drivers/watchdog/iTCO_wdt.c
> > @@ -243,17 +243,16 @@ static int iTCO_wdt_start(struct watchdog_device *wd_dev)
> >   	struct iTCO_wdt_private *p = watchdog_get_drvdata(wd_dev);
> >   	unsigned int val;
> >   
> > -	spin_lock(&p->io_lock);
> > -
> > -	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
> > -
> >   	/* disable chipset's NO_REBOOT bit */
> >   	if (p->update_no_reboot_bit(p->no_reboot_priv, false)) {
> > -		spin_unlock(&p->io_lock);
> >   		pr_err("failed to reset NO_REBOOT flag, reboot disabled by hardware/BIOS\n");
> >   		return -EIO;
> >   	}
> >   
> > +	spin_lock(&p->io_lock);
> > +
> > +	iTCO_vendor_pre_start(p->smi_res, wd_dev->timeout);
> > +
> >   	/* Force the timer to its reload value by writing to the TCO_RLD
> >   	   register */
> >   	if (p->iTCO_version >= 2)
> > @@ -288,11 +287,11 @@ static int iTCO_wdt_stop(struct watchdog_device *wd_dev)
> >   	outw(val, TCO1_CNT(p));
> >   	val = inw(TCO1_CNT(p));
> >   
> > +	spin_unlock(&p->io_lock);
> > +
> >   	/* Set the NO_REBOOT bit to prevent later reboots, just for sure */
> >   	p->update_no_reboot_bit(p->no_reboot_priv, true);
> >   
> > -	spin_unlock(&p->io_lock);
> > -
> >   	if ((val & 0x0800) == 0)
> >   		return -1;
> >   	return 0;
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Sathyanarayanan Kuppuswamy
Linux kernel developer