Re: [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids

[Prakash, Prashanth]

On 10/3/2017 5:01 AM, George Cherian wrote:
> Hi Prakash,
>
> On 09/29/2017 04:49 AM, Prakash, Prashanth wrote:
>> Hi George,
>>
>> On 9/19/2017 11:24 PM, George Cherian wrote:
>>> Based on ACPI 6.2 Section 8.4.7.1.9 If the PCC register space is used,
>>> all PCC registers, for all processors in the same performance
>>> domain (as defined by _PSD), must be defined to be in the same subspace.
>>> Based on Section 14.1 of ACPI specification, it is possible to have a
>>> maximum of 256 PCC subspace ids. Add support of multiple PCC subspace id
>>> instead of using a single global pcc_data structure.
>>>
>>> While at that fix the time_delta check in send_pcc_cmd() so that last_mpar_reset
>>> and mpar_count is initialized properly.
>>>
>>> Signed-off-by: George Cherian <george.cherian@xxxxxxxxxx>
>>> ---
>>> Â drivers/acpi/cppc_acpi.c | 243 +++++++++++++++++++++++++++++------------------
>>> Â 1 file changed, 153 insertions(+), 90 deletions(-)
>>>
>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>> index e5b47f0..3ae79ef 100644
>>> --- a/drivers/acpi/cppc_acpi.c
>>> +++ b/drivers/acpi/cppc_acpi.c
>>> @@ -75,13 +75,16 @@ struct cppc_pcc_data {
>>> Â ÂÂÂÂÂ /* Wait queue for CPUs whose requests were batched */
>>> ÂÂÂÂÂ wait_queue_head_t pcc_write_wait_q;
>>> +ÂÂÂ ktime_t last_cmd_cmpl_time;
>>> +ÂÂÂ ktime_t last_mpar_reset;
>>> +ÂÂÂ int mpar_count;
>>> +ÂÂÂ int refcount;
>>> Â };
>>> Â -/* Structure to represent the single PCC channel */
>>> -static struct cppc_pcc_data pcc_data = {
>>> -ÂÂÂ .pcc_subspace_idx = -1,
>>> -ÂÂÂ .platform_owns_pcc = true,
>>> -};
>>> +/* Array to represent the PCC channel per subspace id */
>>> +static struct cppc_pcc_data *pcc_data[MAX_PCC_SUBSPACES];
>>> +/* The cpu_pcc_subspace_idx containsper CPU subspace id */
>>> +static DEFINE_PER_CPU(int, cpu_pcc_subspace_idx);
>>> Â Â /*
>>> ÂÂ * The cpc_desc structure contains the ACPI register details
>>> @@ -93,7 +96,8 @@ static struct cppc_pcc_data pcc_data = {
>>> Â static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>>> Â Â /* pcc mapped address + header size + offset within PCC subspace */
>>> -#define GET_PCC_VADDR(offs) (pcc_data.pcc_comm_addr + 0x8 + (offs))
>>> +#define GET_PCC_VADDR(offs, pcc_ss_id) (pcc_data[pcc_ss_id]->pcc_comm_addr + \
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 0x8 + (offs))
>>> Â Â /* Check if a CPC register is in PCC */
>>> Â #define CPC_IN_PCC(cpc) ((cpc)->type == ACPI_TYPE_BUFFER &&ÂÂÂÂÂÂÂ \
>>> @@ -188,13 +192,16 @@ static struct kobj_type cppc_ktype = {
>>> ÂÂÂÂÂ .default_attrs = cppc_attrs,
>>> Â };
>>> Â -static int check_pcc_chan(bool chk_err_bit)
>>> +static int check_pcc_chan(int pcc_ss_id, bool chk_err_bit)
>>> Â {
>>> ÂÂÂÂÂ int ret = -EIO, status = 0;
>>> -ÂÂÂ struct acpi_pcct_shared_memory __iomem *generic_comm_base = pcc_data.pcc_comm_addr;
>>> -ÂÂÂ ktime_t next_deadline = ktime_add(ktime_get(), pcc_data.deadline);
>>> +ÂÂÂ struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>> +ÂÂÂ struct acpi_pcct_shared_memory __iomem *generic_comm_base =
>>> +ÂÂÂÂÂÂÂ pcc_ss_data->pcc_comm_addr;
>>> +ÂÂÂ ktime_t next_deadline = ktime_add(ktime_get(),
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pcc_ss_data->deadline);
>>> Â -ÂÂÂ if (!pcc_data.platform_owns_pcc)
>>> +ÂÂÂ if (!pcc_ss_data->platform_owns_pcc)
>>> ÂÂÂÂÂÂÂÂÂ return 0;
>>> Â ÂÂÂÂÂ /* Retry in case the remote processor was too slow to catch up. */
>>> @@ -219,7 +226,7 @@ static int check_pcc_chan(bool chk_err_bit)
>>> ÂÂÂÂÂ }
>>> Â ÂÂÂÂÂ if (likely(!ret))
>>> -ÂÂÂÂÂÂÂ pcc_data.platform_owns_pcc = false;
>>> +ÂÂÂÂÂÂÂ pcc_ss_data->platform_owns_pcc = false;
>>> ÂÂÂÂÂ else
>>> ÂÂÂÂÂÂÂÂÂ pr_err("PCC check channel failed. Status=%x\n", status);
>>> Â @@ -230,13 +237,12 @@ static int check_pcc_chan(bool chk_err_bit)
>>> ÂÂ * This function transfers the ownership of the PCC to the platform
>>> ÂÂ * So it must be called while holding write_lock(pcc_lock)
>>> ÂÂ */
>>> -static int send_pcc_cmd(u16 cmd)
>>> +static int send_pcc_cmd(int pcc_ss_id, u16 cmd)
>>> Â {
>>> ÂÂÂÂÂ int ret = -EIO, i;
>>> +ÂÂÂ struct cppc_pcc_data *pcc_ss_data = pcc_data[pcc_ss_id];
>>> ÂÂÂÂÂ struct acpi_pcct_shared_memory *generic_comm_base =
>>> -ÂÂÂÂÂÂÂ (struct acpi_pcct_shared_memory *) pcc_data.pcc_comm_addr;
>>> -ÂÂÂ static ktime_t last_cmd_cmpl_time, last_mpar_reset;
>>> -ÂÂÂ static int mpar_count;
>>> +ÂÂÂÂÂÂÂ (struct acpi_pcct_shared_memory *)pcc_ss_data->pcc_comm_addr;
>>> ÂÂÂÂÂ unsigned int time_delta;
>>> Â ÂÂÂÂÂ /*
>>> @@ -249,24 +255,25 @@ static int send_pcc_cmd(u16 cmd)
>>> ÂÂÂÂÂÂÂÂÂÂ * before write completion, so first send a WRITE command to
>>> ÂÂÂÂÂÂÂÂÂÂ * platform
>>> ÂÂÂÂÂÂÂÂÂÂ */
>>> -ÂÂÂÂÂÂÂ if (pcc_data.pending_pcc_write_cmd)
>>> -ÂÂÂÂÂÂÂÂÂÂÂ send_pcc_cmd(CMD_WRITE);
>>> +ÂÂÂÂÂÂÂ if (pcc_ss_data->pending_pcc_write_cmd)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ send_pcc_cmd(pcc_ss_id, CMD_WRITE);
>>> Â -ÂÂÂÂÂÂÂ ret = check_pcc_chan(false);
>>> +ÂÂÂÂÂÂÂ ret = check_pcc_chan(pcc_ss_id, false);
>>> ÂÂÂÂÂÂÂÂÂ if (ret)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ goto end;
>>> ÂÂÂÂÂ } else /* CMD_WRITE */
>>> -ÂÂÂÂÂÂÂ pcc_data.pending_pcc_write_cmd = FALSE;
>>> +ÂÂÂÂÂÂÂ pcc_ss_data->pending_pcc_write_cmd = FALSE;
>>> Â ÂÂÂÂÂ /*
>>> ÂÂÂÂÂÂ * Handle the Minimum Request Turnaround Time(MRTT)
>>> ÂÂÂÂÂÂ * "The minimum amount of time that OSPM must wait after the completion
>>> ÂÂÂÂÂÂ * of a command before issuing the next command, in microseconds"
>>> ÂÂÂÂÂÂ */
>>> -ÂÂÂ if (pcc_data.pcc_mrtt) {
>>> -ÂÂÂÂÂÂÂ time_delta = ktime_us_delta(ktime_get(), last_cmd_cmpl_time);
>>> -ÂÂÂÂÂÂÂ if (pcc_data.pcc_mrtt > time_delta)
>>> -ÂÂÂÂÂÂÂÂÂÂÂ udelay(pcc_data.pcc_mrtt - time_delta);
>>> +ÂÂÂ if (pcc_ss_data->pcc_mrtt) {
>>> +ÂÂÂÂÂÂÂ time_delta = ktime_us_delta(ktime_get(),
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pcc_ss_data->last_cmd_cmpl_time);
>>> +ÂÂÂÂÂÂÂ if (pcc_ss_data->pcc_mrtt > time_delta)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ udelay(pcc_ss_data->pcc_mrtt - time_delta);
>>> ÂÂÂÂÂ }
>>> Â ÂÂÂÂÂ /*
>>> @@ -280,18 +287,19 @@ static int send_pcc_cmd(u16 cmd)
>>> ÂÂÂÂÂÂ * not send the request to the platform after hitting the MPAR limit in
>>> ÂÂÂÂÂÂ * any 60s window
>>> ÂÂÂÂÂÂ */
>>> -ÂÂÂ if (pcc_data.pcc_mpar) {
>>> -ÂÂÂÂÂÂÂ if (mpar_count == 0) {
>>> -ÂÂÂÂÂÂÂÂÂÂÂ time_delta = ktime_ms_delta(ktime_get(), last_mpar_reset);
>>> -ÂÂÂÂÂÂÂÂÂÂÂ if (time_delta < 60 * MSEC_PER_SEC) {
>>> +ÂÂÂ if (pcc_ss_data->pcc_mpar) {
>>> +ÂÂÂÂÂÂÂ if (pcc_ss_data->mpar_count == 0) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ time_delta = ktime_ms_delta(ktime_get(),
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pcc_ss_data->last_mpar_reset);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ if ((time_delta < 60 * MSEC_PER_SEC) && pcc_ss_data->last_mpar_reset) {
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_debug("PCC cmd not sent due to MPAR limit");
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = -EIO;
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto end;
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ }
>>> -ÂÂÂÂÂÂÂÂÂÂÂ last_mpar_reset = ktime_get();
>>> -ÂÂÂÂÂÂÂÂÂÂÂ mpar_count = pcc_data.pcc_mpar;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pcc_ss_data->last_mpar_reset = ktime_get();
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pcc_ss_data->mpar_count = pcc_ss_data->pcc_mpar;
>>> ÂÂÂÂÂÂÂÂÂ }
>>> -ÂÂÂÂÂÂÂ mpar_count--;
>>> +ÂÂÂÂÂÂÂ pcc_ss_data->mpar_count--;
>>> ÂÂÂÂÂ }
>>> Â ÂÂÂÂÂ /* Write to the shared comm region. */
>>> @@ -300,10 +308,10 @@ static int send_pcc_cmd(u16 cmd)
>>> ÂÂÂÂÂ /* Flip CMD COMPLETE bit */
>>> ÂÂÂÂÂ writew_relaxed(0, &generic_comm_base->status);
>>> Â -ÂÂÂ pcc_data.platform_owns_pcc = true;
>>> +ÂÂÂ pcc_ss_data->platform_owns_pcc = true;
>>> Â ÂÂÂÂÂ /* Ring doorbell */
>>> -ÂÂÂ ret = mbox_send_message(pcc_data.pcc_channel, &cmd);
>>> +ÂÂÂ ret = mbox_send_message(pcc_ss_data->pcc_channel, &cmd);
>>> ÂÂÂÂÂ if (ret < 0) {
>>> ÂÂÂÂÂÂÂÂÂ pr_err("Err sending PCC mbox message. cmd:%d, ret:%d\n",
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ cmd, ret);
>>> @@ -311,15 +319,15 @@ static int send_pcc_cmd(u16 cmd)
>>> ÂÂÂÂÂ }
>>> Â ÂÂÂÂÂ /* wait for completion and check for PCC errro bit */
>>> -ÂÂÂ ret = check_pcc_chan(true);
>>> +ÂÂÂ ret = check_pcc_chan(pcc_ss_id, true);
>>> Â -ÂÂÂ if (pcc_data.pcc_mrtt)
>>> -ÂÂÂÂÂÂÂ last_cmd_cmpl_time = ktime_get();
>>> +ÂÂÂ if (pcc_ss_data->pcc_mrtt)
>>> +ÂÂÂÂÂÂÂ pcc_ss_data->last_cmd_cmpl_time = ktime_get();
>>> Â -ÂÂÂ if (pcc_data.pcc_channel->mbox->txdone_irq)
>>> -ÂÂÂÂÂÂÂ mbox_chan_txdone(pcc_data.pcc_channel, ret);
>>> +ÂÂÂ if (pcc_ss_data->pcc_channel->mbox->txdone_irq)
>>> +ÂÂÂÂÂÂÂ mbox_chan_txdone(pcc_ss_data->pcc_channel, ret);
>>> ÂÂÂÂÂ else
>>> -ÂÂÂÂÂÂÂ mbox_client_txdone(pcc_data.pcc_channel, ret);
>>> +ÂÂÂÂÂÂÂ mbox_client_txdone(pcc_ss_data->pcc_channel, ret);
>>> Â Â end:
>>> ÂÂÂÂÂ if (cmd == CMD_WRITE) {
>>> @@ -329,12 +337,12 @@ static int send_pcc_cmd(u16 cmd)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (!desc)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
>>> Â -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (desc->write_cmd_id == pcc_data.pcc_write_cnt)
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (desc->write_cmd_id == pcc_ss_data->pcc_write_cnt)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ desc->write_cmd_status = ret;
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ }
>>> ÂÂÂÂÂÂÂÂÂ }
>>> -ÂÂÂÂÂÂÂ pcc_data.pcc_write_cnt++;
>>> -ÂÂÂÂÂÂÂ wake_up_all(&pcc_data.pcc_write_wait_q);
>>> +ÂÂÂÂÂÂÂ pcc_ss_data->pcc_write_cnt++;
>>> +ÂÂÂÂÂÂÂ wake_up_all(&pcc_ss_data->pcc_write_wait_q);
>>> ÂÂÂÂÂ }
>>> Â ÂÂÂÂÂ return ret;
>>> @@ -536,16 +544,16 @@ int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
>>> Â }
>>> Â EXPORT_SYMBOL_GPL(acpi_get_psd_map);
>>> Â -static int register_pcc_channel(int pcc_subspace_idx)
>>> +static int register_pcc_channel(int pcc_ss_idx)
>>> Â {
>>> ÂÂÂÂÂ struct acpi_pcct_hw_reduced *cppc_ss;
>>> ÂÂÂÂÂ u64 usecs_lat;
>>> Â -ÂÂÂ if (pcc_subspace_idx >= 0) {
>>> -ÂÂÂÂÂÂÂ pcc_data.pcc_channel = pcc_mbox_request_channel(&cppc_mbox_cl,
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pcc_subspace_idx);
>>> +ÂÂÂ if (pcc_ss_idx >= 0) {
>>> +ÂÂÂÂÂÂÂ pcc_data[pcc_ss_idx]->pcc_channel =
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pcc_mbox_request_channel(&cppc_mbox_cl,ÂÂÂ pcc_ss_idx);
>>> Â -ÂÂÂÂÂÂÂ if (IS_ERR(pcc_data.pcc_channel)) {
>>> +ÂÂÂÂÂÂÂ if (IS_ERR(pcc_data[pcc_ss_idx]->pcc_channel)) {
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("Failed to find PCC communication channel\n");
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ return -ENODEV;
>>> ÂÂÂÂÂÂÂÂÂ }
>>> @@ -556,7 +564,7 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>> ÂÂÂÂÂÂÂÂÂÂ * PCC channels) and stored pointers to the
>>> ÂÂÂÂÂÂÂÂÂÂ * subspace communication region in con_priv.
>>> ÂÂÂÂÂÂÂÂÂÂ */
>>> -ÂÂÂÂÂÂÂ cppc_ss = (pcc_data.pcc_channel)->con_priv;
>>> +ÂÂÂÂÂÂÂ cppc_ss = (pcc_data[pcc_ss_idx]->pcc_channel)->con_priv;
>>> Â ÂÂÂÂÂÂÂÂÂ if (!cppc_ss) {
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("No PCC subspace found for CPPC\n");
>>> @@ -569,19 +577,20 @@ static int register_pcc_channel(int pcc_subspace_idx)
>>> ÂÂÂÂÂÂÂÂÂÂ * So add an arbitrary amount of wait on top of Nominal.
>>> ÂÂÂÂÂÂÂÂÂÂ */
>>> ÂÂÂÂÂÂÂÂÂ usecs_lat = NUM_RETRIES * cppc_ss->latency;
>>> -ÂÂÂÂÂÂÂ pcc_data.deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>> -ÂÂÂÂÂÂÂ pcc_data.pcc_mrtt = cppc_ss->min_turnaround_time;
>>> -ÂÂÂÂÂÂÂ pcc_data.pcc_mpar = cppc_ss->max_access_rate;
>>> -ÂÂÂÂÂÂÂ pcc_data.pcc_nominal = cppc_ss->latency;
>>> -
>>> -ÂÂÂÂÂÂÂ pcc_data.pcc_comm_addr = acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>> -ÂÂÂÂÂÂÂ if (!pcc_data.pcc_comm_addr) {
>>> +ÂÂÂÂÂÂÂ pcc_data[pcc_ss_idx]->deadline = ns_to_ktime(usecs_lat * NSEC_PER_USEC);
>>> +ÂÂÂÂÂÂÂ pcc_data[pcc_ss_idx]->pcc_mrtt = cppc_ss->min_turnaround_time;
>>> +ÂÂÂÂÂÂÂ pcc_data[pcc_ss_idx]->pcc_mpar = cppc_ss->max_access_rate;
>>> +ÂÂÂÂÂÂÂ pcc_data[pcc_ss_idx]->pcc_nominal = cppc_ss->latency;
>>> +
>>> +ÂÂÂÂÂÂÂ pcc_data[pcc_ss_idx]->pcc_comm_addr =
>>> +ÂÂÂÂÂÂÂÂÂÂÂ acpi_os_ioremap(cppc_ss->base_address, cppc_ss->length);
>>> +ÂÂÂÂÂÂÂ if (!pcc_data[pcc_ss_idx]->pcc_comm_addr) {
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_err("Failed to ioremap PCC comm region mem\n");
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ return -ENOMEM;
>>> ÂÂÂÂÂÂÂÂÂ }
>>> Â ÂÂÂÂÂÂÂÂÂ /* Set flag so that we dont come here for each CPU. */
>>> -ÂÂÂÂÂÂÂ pcc_data.pcc_channel_acquired = true;
>>> +ÂÂÂÂÂÂÂ pcc_data[pcc_ss_idx]->pcc_channel_acquired = true;
>>> ÂÂÂÂÂ }
>>> Â ÂÂÂÂÂ return 0;
>>> @@ -600,6 +609,39 @@ bool __weak cpc_ffh_supported(void)
>>> ÂÂÂÂÂ return false;
>>> Â }
>>> Â +
>>> +/**
>>> + * pcc_data_alloc() - Allocate the pcc_data memory for pcc subspace
>>> + *
>>> + * Check and allocate the cppc_pcc_data memory.
>>> + * In some processor configurations it is possible that same subspace
>>> + * is shared between multiple CPU's. This is seen especially in CPU's
>>> + * with hardware multi-threading support.
>>> + *
>>> + * Return: 0 for success, errno for failure
>>> + */
>>> +int pcc_data_alloc(int pcc_ss_id)
>>> +{
>>> +ÂÂÂ int loop;
>>> +
>>> +ÂÂÂ if (pcc_ss_id < 0)
>> Above should be (pcc_ss_id < 0 || pcc_ss_id >= MAX_PCC_SUBSPACES)
> Yes we can have this additional check.
>>> +ÂÂÂÂÂÂÂ return -EINVAL;
>>> +
>>> +ÂÂÂ for (loop = 0; pcc_data[loop] != NULL; loop++) {
>>> +ÂÂÂÂÂÂÂ if (pcc_data[loop]->pcc_subspace_idx == pcc_ss_id) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ pcc_data[loop]->refcount++;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ return 0;
>>> +ÂÂÂÂÂÂÂ }
>>> +ÂÂÂ }
>> Why do we need the above for loop? can't it be direct array lookup?
>> if (pcc_data[pcc_ss_id]) {
>> ÂÂÂÂ //increment ref_count and return
>> }
> Yes, we could get rid of the loop and increment the reference directly if already allocated.
>
>>
>> Also, we should remove the pcc_subspace_idx from cppc_pcc_data structure,
>> it is no longer useful and probably adds to confusion.
> Please see below
>>> +
>>> +ÂÂÂ pcc_data[pcc_ss_id] = kzalloc(sizeof(struct cppc_pcc_data), GFP_KERNEL);
>>> +ÂÂÂ if (!pcc_data[pcc_ss_id])
>>> +ÂÂÂÂÂÂÂ return -ENOMEM;
>>> +ÂÂÂ pcc_data[pcc_ss_id]->pcc_subspace_idx = pcc_ss_id;
>>> +ÂÂÂ pcc_data[pcc_ss_id]->refcount++;
>>> +
>>> +ÂÂÂ return 0;
>>> +}
>>> Â /*
>>> ÂÂ * An example CPC table looks like the following.
>>> ÂÂ *
>>> @@ -661,6 +703,7 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>> ÂÂÂÂÂ struct device *cpu_dev;
>>> ÂÂÂÂÂ acpi_handle handle = pr->handle;
>>> ÂÂÂÂÂ unsigned int num_ent, i, cpc_rev;
>>> +ÂÂÂ int pcc_subspace_id = -1;
>>> ÂÂÂÂÂ acpi_status status;
>>> ÂÂÂÂÂ int ret = -EFAULT;
>>> Â @@ -733,12 +776,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ * so extract it only once.
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ */
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ if (gas_t->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (pcc_data.pcc_subspace_idx < 0)
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pcc_data.pcc_subspace_idx = gas_t->access_width;
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ else if (pcc_data.pcc_subspace_idx != gas_t->access_width) {
>>> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_debug("Mismatched PCC ids.\n");
>> We need to retain the above checks to make sure all PCC registers
>> within a _CPC package is under same subspace. The Spec still requires:
>> "If the PCC register space is used, all PCC registers, for all processors in
>> the same performance domain (as defined by _PSD), must be defined
>> to be in the same subspace."
>
> So that means we need to maintain the pcc_subspace_idx in cppc_pcc_data.
> If so, then I can move the check inside pcc_data_alloc().
We need the pcc_subspace_idx for checking only withing the context of this
function, so tracking that data in a local variable will be sufficient.
>>
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pcc_subspace_id = gas_t->access_width;
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (pcc_data_alloc(pcc_subspace_id))
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto out_free;
>> We need to call pcc_data_alloc(to increment the reference) only once per CPU,
>> otherwise acpi_cppc_processor_exit( ) will never free the memory allocated in
>> pcc_data.
>
> It is infact called only once per CPU, The refcounting was added in case if multiple CPU's share the same domain for eg:
> In instances, where CPU have hardware multi-threading support.
> or in cases where all CPU's are controlled using single subspace (as earlier).
No, it is getting called for every PCC registers within a _CPC package, instead it
needs to be called only once per _CPC package irrespective of number of PCC
registers within the_CPCÂ package.
>
> Do you see any issue with acpi_cppc_processor_exit() in the earlier cases, where refcount remains > 0 and the memory not getting freed-up?
>>
>>
>> -- 
>> Thanks,
>> Prashanth
>>