Hi Mark,
On 17 September 2014 18:17, Ashwin Chaugule <ashwin.chaugule@xxxxxxxxxx> wrote:
> 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.
Can I add the above snippet to V8 with your Reviewed-by tag?
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
