On Sun, 2016-10-23 at 16:57 +0200, Lukas Wunner wrote:
> On Sun, Oct 23, 2016 at 01:37:55PM +0100, Bryan O'Donoghue wrote:
> >
> The usage of a mutex in mid_pwr_set_power_state() actually seems
> questionable since this is called with interrupts disabled:
> pci_pm_resume_noirq
> pci_pm_default_resume_early
> pci_power_up
> platform_pci_set_power_state
> mid_pci_set_power_state
> intel_mid_pci_set_power_state
> mid_pwr_set_power_state
That was my other question then - though I assume the mutex is put in
place to future-proof the code.
I'm just wondering out loud - considering we have the case where we update a
register and then spin waiting for a command completion - is it in fact
logically valid to have a concurrent reader read out the power state - when
another writer is executing mid_pwr_wait() - for example.
/* Wait 500ms that the latest PWRMU command finished */
static int mid_pwr_wait(struct mid_pwr *pwr)
{
unsigned int count = 500000;
bool busy;
do {
busy = mid_pwr_is_busy(pwr);
if (!busy)
return 0;
udelay(1);
} while (--count);
return -EBUSY;
}
static int mid_pwr_wait_for_cmd(struct mid_pwr *pwr, u8 cmd)
{
writel(PM_CMD_CMD(cmd) | PM_CMD_CM_IMMEDIATE, pwr->regs +
PM_CMD);
return mid_pwr_wait(pwr);
}
static int __update_power_state(struct mid_pwr *pwr, int reg, int bit,
int new)
{
<snip>
/* Update the power state */
mid_pwr_set_state(pwr, reg, (power & ~(3 << bit)) | (new <<
bit));
/* Send command to SCU */
ret = mid_pwr_wait_for_cmd(pwr, CMD_SET_CFG);
if (ret)
return ret;
<snip>
}
anyway...
I've tested your patch and it looks good. We can otherwise defer to
andy on the usage of the mutex.
Tested-by: Bryan O'Donoghue <[email protected]>
---
bod
