Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2

[Alexey Klimov <alexey.klimov@arm.com>]

Hi Hoan,

On Fri, May 06, 2016 at 11:38:34AM -0700, Hoan Tran wrote:
> From: hotran <hotran@apm.com>
> 
> ACPI 6.1 has a PCC HW-Reduced Communication Subspace Type 2 intended for
> use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
> to acknowledge doorbell interrupt. This patch provides the implementation
> for the Communication Subspace Type 2.
> 
> This patch depends on patch [1] which supports PCC subspace type 2 header
> [1] https://lkml.org/lkml/2016/5/5/14
>  - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable

So you finally decided to use separate structure declaration for type 2. Good.

> v2
>  * Remove changes inside "actbl3.h". This file is taken care by ACPICA.
>  * Parse both subspace type 1 and subspace type 2
>  * Remove unnecessary variable initialization
>  * ISR returns IRQ_NONE in case of error
> 
> v1
>  * Initial
> 
> Signed-off-by: Hoan Tran <hotran@apm.com>
> ---
>  drivers/mailbox/pcc.c | 395 +++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 296 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 043828d..58c9a67 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -59,6 +59,7 @@
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/init.h>

[...]

> @@ -307,6 +440,43 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
>  }
>  
>  /**
> + * pcc_parse_subspace_irq - Parse the PCC IRQ and PCC ACK register
> + *		There should be one entry per PCC client.
> + * @mbox_chans: Pointer to the PCC mailbox channel data
> + * @pcct_ss: Pointer to the ACPI subtable header under the PCCT.
> + *
> + * Return: 0 for Success, else errno.
> + *
> + * This gets called for each entry in the PCC table.
> + */
> +static int pcc_parse_subspace_irq(struct pcc_mbox_chan *mbox_chans,
> +		struct acpi_pcct_hw_reduced *pcct_ss)
> +{
> +	mbox_chans->irq = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
> +					    (u32)pcct_ss->flags);
> +	if (mbox_chans->irq <= 0) {
> +		pr_err("PCC GSI %d not registered\n",
> +		       pcct_ss->doorbell_interrupt);
> +		return -EINVAL;
> +	}
> +
> +	if (pcct_ss->header.type
> +		== ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> +		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
> +
> +		mbox_chans->pcc_doorbell_ack_vaddr = acpi_os_ioremap(
> +				pcct2_ss->doorbell_ack_register.address,
> +				pcct2_ss->doorbell_ack_register.bit_width / 8);
> +		if (!mbox_chans->pcc_doorbell_ack_vaddr) {
> +			pr_err("Failed to ioremap PCC ACK register\n");
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>   * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
>   *
>   * Return: 0 for Success, else errno.
> @@ -316,7 +486,8 @@ static int __init acpi_pcc_probe(void)
>  	acpi_size pcct_tbl_header_size;
>  	struct acpi_table_header *pcct_tbl;
>  	struct acpi_subtable_header *pcct_entry;
> -	int count, i;
> +	struct acpi_table_pcct *acpi_pcct_tbl;
> +	int count, i, rc;
>  	acpi_status status = AE_OK;
>  
>  	/* Search for PCCT */
> @@ -334,22 +505,28 @@ static int __init acpi_pcc_probe(void)
>  			ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
>  			parse_pcc_subspace, MAX_PCC_SUBSPACES);
>  
> +	count += acpi_table_parse_entries(ACPI_SIG_PCCT,
> +			sizeof(struct acpi_table_pcct),
> +			ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
> +			parse_pcc_subspace, MAX_PCC_SUBSPACES);
> +
>  	if (count <= 0) {
>  		pr_err("Error parsing PCC subspaces from PCCT\n");
>  		return -EINVAL;
>  	}

Looks like after first call to acpi_table_parse_entries() you may have negative
number in count. And then you add counted number of type 2 subtables to count variable.

I am not aware how pedantic this all should be but you may have more than MAX_PCC_SUBSPACES
subspaces or don't probe any subspaces at all with such approach. Or other side effects.


Best regards,
Alexey Klimov