Re: [Devel] [PATCH v5 1/2] acpi: apei: remove the unused dead-code for SEA/NMI notification type
by gengdongjiu
Hi,Borislav
On 2017/10/18 0:43, Borislav Petkov wrote:
>> -}
>> -
> So GHES NMI notification method is x86-only AFAIK and HAVE_ACPI_APEI_NMI
> is selected only on x86. Why are you removing those guards? Does ARM
> have ACPI_HEST_NOTIFY_NMI notification type now too?
ARM does not have ACPI_HEST_NOTIFY_NMI notification, which should only used by x86.
In the code, I see those guards are never used. As you see, if the 'CONFIG_HAVE_ACPI_APEI_NMI'
does not defined in [1], it will print error info and goto [2], in the [2], it will return error,
then the probe for GHES NMI is failed. so those guards( ghes_nmi_add() and ghes_nmi_remove()) have no chance
to execute. so I redefine them to NULL for compiling[3].
static int ghes_probe(struct platform_device *ghes_dev)
{
struct acpi_hest_generic *generic;
struct ghes *ghes = NULL;
int rc = -EINVAL;
switch (generic->notify.type) {
...................
case ACPI_HEST_NOTIFY_NMI: [1]
if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_NMI)) {
pr_warn(GHES_PFX "Generic hardware error source: %d notified via NMI interrupt is not supported!\n",
generic->header.source_id);
goto err;
}
..............
}
switch (generic->notify.type) {
...............
case ACPI_HEST_NOTIFY_NMI:
ghes_nmi_add(ghes);
break;
}
..........
err: [2]
if (ghes) {
ghes_fini(ghes);
kfree(ghes);
}
return rc;
}
[3]:
-static inline void ghes_nmi_add(struct ghes *ghes)
-{
- pr_err(GHES_PFX "ID: %d, trying to add NMI notification which is not supported!\n",
- ghes->generic->header.source_id);
- BUG();
-}
-
-static inline void ghes_nmi_remove(struct ghes *ghes)
-{
- pr_err(GHES_PFX "ID: %d, trying to remove NMI notification which is not supported!\n",
- ghes->generic->header.source_id);
- BUG();
-}
-
-static inline void ghes_nmi_init_cxt(void)
-{
-}
+static inline void ghes_nmi_add(struct ghes *ghes) { }
+static inline void ghes_nmi_remove(struct ghes *ghes) { }
+static inline void ghes_nmi_init_cxt(void) { }
4 years, 10 months
Re: [Devel] [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids
by George Cherian
Thanks Prakash for the clarifications.
I have updated the patch set incorporating your comments and posted v5.
Regards
-George
On 10/07/2017 02:40 AM, Prakash, Prashanth wrote:
>
> 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(a)cavium.com>
>>>> ---
>>>> 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
>>>
>
4 years, 10 months
[PATCH v5 0/2] Make cppc acpi driver aware of pcc subspace ids
by George Cherian
The current cppc acpi driver works with only one pcc subspace id.
It maintains and registers only one pcc channel even if the acpi table has
different pcc subspace ids.
As per ACPI 6.2 spec all PCC registers, for all processors in the same
performance domain (as defined by _PSD), must be defined to be in the same
subspace. The series tries to address the same by making cppc acpi driver
aware of multiple possible pcc subspace ids.
Patch 1 : In preparation to share the MAX_PCC_SUBSPACE definition with cppc acpi
driver
Patch 2 : Make the cppc acpi driver aware of multiple pcc subspace ids.
Changes from v4:
- Addressed Prashath's comments on
* Remove pcc_subspace_idx member from struct cppc_pcc_data
* Eliminate the loop and directly increment the refcoun in pcc_data_alloc()
* pcc_data_alloc to be called once per CPU.
* Retain the earlier check PCC registers within same _CPC is under same
subspace.
Changes from v3:
- Address Issue reported by kbuild-robot.
Changes from v2:
- Addressed Prashanth's comments on
* Not to use local variables to update mpar_count, last_mpar_reset and
last_cmd_cmpl_time
* Add check for kzalloc failure in pcc_data_alloc()
* Initialize pcc_subspace_id to -1 in acpi_cppc_processor_probe()
* Check for pcc_subspace_id validity before registering pcc_channel
Changes from v1:
- Add last_cmd_cmpl_time, last_mpar_reset, mpar_count to the cppc_pcc_data to
make it per subspace.
- PCC per subspace dynamic allocation support added instead of static
allocation
- Added a new function pcc_data_alloc, In instances where CPU's with SMT
support same PCC subspace would be used for all CPU's belonging to same
physical core. This function adds the pcc_subspace refcounting and allocates
the cppc_pcc_data per unique subspace idx.
- Added cleanup in acpi_cppc_processor_exit. Free the mbox channel and free
the cppc_pcc_data in case refcount is zero.
George Cherian (2):
mailbox: PCC: Move the MAX_PCC_SUBSPACES definition to header file
ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids
drivers/acpi/cppc_acpi.c | 240 +++++++++++++++++++++++++++++------------------
drivers/mailbox/pcc.c | 1 -
include/acpi/pcc.h | 1 +
3 files changed, 152 insertions(+), 90 deletions(-)
--
2.1.4
4 years, 10 months
Re: [Devel] [PATCH v9 4/4] PCI: hisi: blacklist hip06/hip07 controllers behind SMMUv3
by Shameerali Kolothum Thodi
HI Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Tuesday, October 10, 2017 12:55 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi(a)huawei.com>
> Cc: lorenzo.pieralisi(a)arm.com; marc.zyngier(a)arm.com;
> sudeep.holla(a)arm.com; will.deacon(a)arm.com; robin.murphy(a)arm.com;
> joro(a)8bytes.org; bhelgaas(a)google.com; Gabriele Paoloni
> <gabriele.paoloni(a)huawei.com>; John Garry <john.garry(a)huawei.com>;
> iommu(a)lists.linux-foundation.org; linux-arm-kernel(a)lists.infradead.org;
> linux-acpi(a)vger.kernel.org; linux-pci(a)vger.kernel.org; devel(a)acpica.org;
> Linuxarm <linuxarm(a)huawei.com>; Wangzhou (B)
> <wangzhou1(a)hisilicon.com>; Guohanjun (Hanjun Guo)
> <guohanjun(a)huawei.com>
> Subject: Re: [PATCH v9 4/4] PCI: hisi: blacklist hip06/hip07 controllers behind
> SMMUv3
>
> Please change subject line:
>
> - PCI: hisi: blacklist hip06/hip07 controllers behind SMMUv3
> + PCI: hisi: Blacklist hip06/hip07 controllers behind SMMUv3
Ok.
> On Fri, Oct 06, 2017 at 03:04:50PM +0100, Shameer Kolothum wrote:
> > The HiSilicon erratum 161010801 describes the limitation of HiSilicon
> > platforms hip06/hip07 to support the SMMUv3 mappings for MSI
> > transactions.
>
> I don't suppose there's a URL for this erratum, is there?
We don't have anything public at the moment. This is part of the internal
documentation at the moment.
> > PCIe controller on these platforms has to differentiate the MSI
> > payload against other DMA payload and has to modify the MSI payload.
> > This basically makes it difficult for this platforms to have a SMMU
> > translation for MSI. In order to workaround this, ARM
> > SMMUv3 driver requires a quirk to treat the MSI regions separately.
> > Such a quirk is currently missing for DT based systems and therefore
> > we need to blacklist the hip06/hip07 PCIe controllers.
>
> I don't understand the DT connection here. If this is a hardware erratum, I
> assume the a quirk would be required whether the system uses DT, ACPI,
> etc.
Yes, this is a hardware erratum and we are almost there to add the ACPI
quirk into the SMMUv3 driver for this. But we got the feedback that
either we have to have the DT support or we should blacklist the affected
devices so that from the SMMUv3 driver point of view it will not run into the
quirk unnecessarily[1].
We attempted to provide a DT solution, but it looks like DT requires more
discussions and a more generic approach than ACPI[2]. Hence we are blacklisting
the PCIe for now so that we can have the ACPI support goes through. (Also we
don't have the DT binding in the mainline which support SMMU on these
platforms and also DT is not officially supported for these platforms).
> > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi(a)huawei.com>
>
> I assume this will go via some non-PCI tree. If I were applying this, I would
> look for an ack from Zhou or Gabriele in addition to mine.
>
> > ---
> > drivers/pci/dwc/pcie-hisi.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/pci/dwc/pcie-hisi.c b/drivers/pci/dwc/pcie-hisi.c
> > index a201791..6800747 100644
> > --- a/drivers/pci/dwc/pcie-hisi.c
> > +++ b/drivers/pci/dwc/pcie-hisi.c
> > @@ -270,6 +270,12 @@ static int hisi_pcie_probe(struct platform_device
> *pdev)
> > struct resource *reg;
> > int ret;
> >
> > + if ((IS_BUILTIN(CONFIG_ARM_SMMU_V3)) &&
> > + of_property_read_bool(dev->of_node, "iommu-
> map")) {
>
> Does the presence of "iommu-map" tell you this is an SMMUv3? Could it
> have a different type of IOMMU? I can't tell from reading
> Documentation/devicetree/bindings/pci/pci-iommu.txt.
Only if the SMMUv3 driver is loaded and iommu-map binding property present,
the pcie devices will use SMMU translated iova for MSI doorbell addresses.
> Why do you care whether CONFIG_ARM_SMMU_V3 is set? Does MSI work
> correctly if SMMUv3 is present but not used?
Yes. MSI will work if no SMMUv3 is used.
> Is it really necessary to ignore the PCIe controller completely?
> Could you use the devices below it as long as you disable MSI for them? I
> know there are probably devices that require MSI, so maybe it's easier to
> just ignore everything than to respond to reports of "my device doesn't work
> because it requires MSI."
We are blacklisting MSI for PCIe only if the kernel is using DT and is configured
to use SMMUv3. Otherwise it is fine. And as I said above DT is not officially
supported on these platforms.
Thanks,
Shameer
1. [PATCH v6 3/3] iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801
https://www.spinics.net/lists/arm-kernel/msg602873.html
2. [PATCH v8 3/5] iommu/of: Add msi address regions reservation helper
https://www.spinics.net/lists/arm-kernel/msg609431.html
4 years, 10 months
[PATCH v8 0/5] iommu/smmu-v3: Workaround for hisilicon 161010801 erratum(reserve HW MSI)
by Shameer Kolothum
On certain HiSilicon platforms (hip06/hip07) the GIC ITS and PCIe RC
deviates from the standard implementation and this breaks PCIe MSI
functionality when SMMU is enabled.
The HiSilicon erratum 161010801 describes this limitation of certain
HiSilicon platforms to support the SMMU mappings for MSI transactions.
On these platforms GICv3 ITS translator is presented with the deviceID
by extending the MSI payload data to 64 bits to include the deviceID.
Hence, the PCIe controller on this platforms has to differentiate the MSI
payload against other DMA payload and has to modify the MSI payload.
This basically makes it difficult for this platforms to have a SMMU
translation for MSI.
This patch implements an ACPI and DT based quirk to reserve the hw msi
regions in the smmu-v3 driver which means these address regions will
not be translated and will be excluded from iova allocations.
To implement this quirk, the following changes are incorporated:
1. Added a generic helper function to IORT code to retrieve the
associated ITS base address from a device IORT node.
2. Added a generic helper function to of iommu code to retrieve the
associated msi controller base address from for a PCI RC
msi-mapping and also platform device msi-parent.
3. Added quirk to SMMUv3 to retrieve the HW ITS address and replace
the default SW MSI reserve address based on the IORT SMMU model
or DT bindings.
Changelog:
v7 --> v8
Addressed comments from Rob and Lorenzo:
-Modified to use DT compatible string for errata.
-Changed logic to retrieve the msi-parent for DT case.
v6 --> v7
Addressed request from Will to add DT support for the erratum:
- added bt binding
- add of_iommu_msi_get_resv_regions()
New arm64 silicon errata entry
Rename iort_iommu_{its->msi}_get_resv_regions
v5 --> v6
Addressed comments from Robin and Lorenzo:
-No change to patch#1 .
-Reverted v5 patch#2 as this might break the platforms where this quirk
is not applicable. Provided a generic function in iommu code and added
back the quirk implementation in SMMU v3 driver(patch#3)
v4 --> v5
Addressed comments from Robin and Lorenzo:
-Added a comment to make it clear that, for now, only straightforward
HW topologies are handled while reserving ITS regions(patch #1).
v3 --> v4
Rebased on 4.13-rc1.
Addressed comments from Robin, Will and Lorenzo:
-As suggested by Robin, moved the ITS msi reservation into
iommu_dma_get_resv_regions().
-Added its_count != resv region failure case(patch #1).
v2 --> v3
Addressed comments from Lorenzo and Robin:
-Removed dev_is_pci() check in smmuV3 driver.
-Don't treat device not having an ITS mapping as an error in
iort helper function.
v1 --> v2
-patch 2/2: Invoke iort helper fn based on fwnode type(acpi).
RFCv2 -->PATCH
-Incorporated Lorenzo's review comments.
RFC v1 --> RFC v2
Based on Robin's review comments,
-Removed the generic erratum framework.
-Using IORT/MADT tables to retrieve the ITS base addr instead of vendor specific CSRT table.
John Garry (2):
Doc: iommu/arm-smmu-v3: Add workaround for HiSilicon erratum 161010801
iommu/of: Add msi address regions reservation helper
Shameer Kolothum (3):
ACPI/IORT: Add msi address regions reservation helper
iommu/dma: Add a helper function to reserve HW MSI address regions for
IOMMU drivers
iommu/arm-smmu-v3:Enable ACPI based HiSilicon erratum 161010801
Documentation/arm64/silicon-errata.txt | 1 +
.../devicetree/bindings/iommu/arm,smmu-v3.txt | 9 +-
drivers/acpi/arm64/iort.c | 96 +++++++++++++++++++++-
drivers/iommu/arm-smmu-v3.c | 41 +++++++--
drivers/iommu/dma-iommu.c | 19 +++++
drivers/iommu/of_iommu.c | 95 +++++++++++++++++++++
drivers/irqchip/irq-gic-v3-its.c | 3 +-
include/linux/acpi_iort.h | 7 +-
include/linux/dma-iommu.h | 7 ++
include/linux/of_iommu.h | 10 +++
10 files changed, 276 insertions(+), 12 deletions(-)
--
1.9.1
4 years, 10 months
Re: [Devel] [PATCH 15/18] acpi: use ARRAY_SIZE
by Rafael J. Wysocki
On Tuesday, October 3, 2017 3:16:22 AM CEST Jérémy Lefaure wrote:
> On Mon, 02 Oct 2017 14:27:52 +0200
> "Rafael J. Wysocki" <rjw(a)rjwysocki.net> wrote:
>
> > ACPICA is soewhat special code, though and I'm not taking or ACKing patches
> > for it directly as a rule.
> >
> > For one, I'm not sure if ACPICA can use ARRAY_SIZE at all.
> Why is it special code that can't use ARRAY_SIZE ? Is it because this
> macro is defined in linux/kernel.h ?
Basically, yes.
ACPICA is a separate project and the kernel acquires its source code from
the upstream (which is used by other OSes too). That is not the case for
any other code in the kernel I know about.
We strive to keep the ACPICA code in the kernel as close to the upstream as
reasonably possible for maintenance reasons, so we avoid doing Linux-specific
things in it.
As a rule of thumb, if you do cleanups like this one, better avoid the ACPICA
code. :-)
Thanks,
Rafael
4 years, 10 months
Re: [Devel] [PATCH v4 2/2] ACPI / CPPC: Make cppc acpi driver aware of pcc subspace ids
by George Cherian
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(a)cavium.com>
>> ---
>> 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().
>
>> + 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).
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
>
4 years, 10 months
Re: [Devel] [PATCH 15/18] acpi: use ARRAY_SIZE
by Rafael J. Wysocki
On Sunday, October 1, 2017 9:30:53 PM CEST Jérémy Lefaure wrote:
> Using the ARRAY_SIZE macro improves the readability of the code. It is
> useless to re-invent the ARRAY_SIZE macro so let's use it.
>
> It is useless to re-invent the ARRAY_SIZE macro so let's use it.
ACPICA is soewhat special code, though and I'm not taking or ACKing patches
for it directly as a rule.
For one, I'm not sure if ACPICA can use ARRAY_SIZE at all.
Thanks,
Rafael
4 years, 10 months
