Thanks for clarifying my comments Hans :) On Fri, Jun 25, 2021 at 8:25 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > 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(). I still think it's prudent to check this, just in case. It shouldn't be expensive. > > 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. > I should have placed my comments on the second one. > 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. > I agree.
