On 16 September 2014 14:28, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote:
> Hi Mark,
>
> On 16 September 2014 13:40, Mark Brown <broonie@xxxxxxxxxx> wrote:
>> On Wed, Sep 10, 2014 at 01:20:18PM -0400, Ashwin Chaugule wrote:
>>
>>> ACPI 5.0+ spec defines a generic mode of communication
>>> between the OS and a platform such as the BMC. This medium
>>> (PCC) is typically used by CPPC (ACPI CPU Performance management),
>>> RAS (ACPI reliability protocol) and MPST (ACPI Memory power
>>> states).
>>
>> This mostly looks good to me at a fairly high level, one small nit below
>> which I don't think should be a blocker to an initial merge but would be
>> good to fix.
>>
>> Reviewed-by: Mark Brown <broonie@xxxxxxxxxx>
>
> Much appreciated!
>
>>
>>> +static bool pcc_tx_done(struct mbox_chan *chan)
>>> +{
>>> + struct acpi_pcct_subspace *pcct_ss = chan->con_priv;
>>> + struct acpi_pcct_shared_memory *generic_comm_base =
>>> + (struct acpi_pcct_shared_memory *) pcct_ss->base_address;
>>> + u16 cmd_delay = pcct_ss->min_turnaround_time;
>>> +
>>> + /* Wait for Platform to consume. */
>>> + while (!(readw_relaxed(&generic_comm_base->status) & PCC_CMD_COMPLETE))
>>> + udelay(cmd_delay);
>>
>> This will busy wait for ever if the controller dies. We're in big
>> trouble if that happens but it would be good to have some sort of
>> timeout and scream here.
>
> Agreed. I'll add this along with Lv's feedback.
How about something like this?
--------8<-------------
-
- /* Wait for Platform to consume. */
- while (!(readw_relaxed(&generic_comm_base->status) & PCC_CMD_COMPLETE))
- udelay(cmd_delay);
-
+ unsigned int retries = 0;
+
+ /* Try a few times while waiting for platform to consume */
+ while (!(readw_relaxed(&generic_comm_base->status) &
PCC_CMD_COMPLETE)) {
+ if (retries++ < 5)
+ udelay(cmd_delay);
+ else {
+ pr_err("PCC platform did not respond.\n");
+ return false;
+ }
+ }
--------8<-------------
And the Mbox client additionally sets .tx_block = true and .tx_out =
10(msecs). That way, if the remote hangs or crashes, the
wait_for_completion in the controller code timesout and sends an -EIO
to the client .tx_done method so it knows something went wrong.
Thanks,
Ashwin
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
