Re: [PATCH v3 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_state hook

[Bryan O'Donoghue]

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 <pure.logic@xxxxxxxxxxxxxxxxx>

---
bod