Re: [PATCH v2 0/7] updates to amd-pmc driver

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/24/21 7:42 AM, Shyam Sundar S K wrote:
> Hi Hans,
> 
> On 6/24/2021 2:14 AM, Hans de Goede wrote:
>> Hi Shyam,
>>
>>
>> On 6/23/21 10:01 PM, Shyam Sundar S K wrote:
>>> Couple of existing issues on the completion codes to SMU
>>> and a newer way to get the s0ix statistics are introduced.
>>>
>>> Also, add acpi ids for current and future revisions of the
>>> pmc controller.
>>>
>>> Shyam Sundar S K (7):
>>>   platform/x86: amd-pmc: Fix command completion code
>>>   platform/x86: amd-pmc: Fix SMU firmware reporting mechanism
>>>   platform/x86: amd-pmc: call dump registers only once
>>>   platform/x86: amd-pmc: Add support for logging SMU metrics
>>>   platform/x86: amd-pmc: Add support for logging s0ix counters
>>>   platform/x86: amd-pmc: Add support for ACPI ID AMDI0006
>>>   platform/x86: amd-pmc: Add new acpi id for future PMC controllers
>>
>> Thank you for the new version.
>>
>> Can you please respond to Raul's reply to patch 1/6 of v1 of
>> the series?  Raul's remark seems very relevant to me.
> 
> Unfortunately, I could not find Raul's email in my inbox and hence I
> missed to reply.
> 
> Hi Raul,
> 
> The break condition for readx_poll_timeout() should be 'val ==
> AMD_PMC_RESULT_OK'. Reason being:
> 
> The smu firmware spec says, we have to wait until the response register
> says 1, meaning it is ready to take the job requests. If it is anything
> apart from 1, it means it's not in a state to take any requests.

Hmm, if I understand things correctly 0 means "not ready", waiting
for that to go away is fine, but then we have:

43:#define AMD_PMC_RESULT_OK                    0x01
44:#define AMD_PMC_RESULT_CMD_REJECT_BUSY       0xFC
45:#define AMD_PMC_RESULT_CMD_REJECT_PREREQ     0xFD
46:#define AMD_PMC_RESULT_CMD_UNKNOWN           0xFE
47:#define AMD_PMC_RESULT_FAILED                0xFF

What if the PMC e.g. responds with AMD_PMC_RESULT_CMD_UNKNOWN ? Then we
should definitely stop polling.

After patch 1/7 there are 2 waits:

1. To wait for any pending previous command to complete
2. A new one introduced by patch 1/7 which waits for the just
send command to complete.

1. seems redundant since we are the only one talking to the PMC,
and we (now) wait for the command to complete before returning
from amd_pmc_send_cmd().

2. The command could be unknown, or fail for some reason,
so it seems that the old wait for any response != 0
(after we clear the register) is the right thing to do
and then we should do a switch-case on the response
to see if the response is ok, or some error.

Also I see no protection against 2 amd_pmc_send_cmd()
calls running in parallel. ATM that seems to be fine since
this cannot happen, but it might be good to still add a
mutex and take that so that this does not break in the
future when some new user may show up which can run in
parallel.

So IMHO the right sequence here would be:

1. Take mutex
2. Clear response register (clearing response from previous command)
3. Write argument
4. Write message id
5. Wait for response register to become !- 0.
6. release mutex
7. do a switch-case on the read response, treating ok
as success an anything else as an error.

Regards,

Hans





> 
> And yes, response register always returns 'val > 0'. Refer to
> AMD_PMC_RESULT_* macros.
> 
> Maybe instead of doing 'return rc', should I just do 'return -EBUSY' ?
> 
> +	if (rc) {
> +		dev_err(dev->dev, "SMU response timed out\n");
> +		return rc;
> +	}
> +  	return 0;
> 
> Am I missing anything obvious frmo your comments?
> 
> Thanks,
> Shyam
>