Date:	Tue, 10 May 2016 21:05:12 -0700
From:	Hoan Tran <hotran@....com>
To:	Alexey Klimov <alexey.klimov@....com>
Cc:	Jassi Brar <jassisinghbrar@...il.com>,
	Ashwin Chaugule <ashwin.chaugule@...aro.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Robert Moore <robert.moore@...el.com>,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
	Loc Ho <lho@....com>, Duc Dang <dhdang@....com>
Subject: Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication
 Subspace Type 2

Hi Alexey,

On Tue, May 10, 2016 at 3:34 AM, Alexey Klimov <alexey.klimov@....com> wrote:
> On Mon, May 09, 2016 at 10:38:24AM -0700, Hoan Tran wrote:
>> Hi Alexey,
>>
>> On Mon, May 9, 2016 at 2:43 AM, Alexey Klimov <alexey.klimov@....com> wrote:
>> > Hi Hoan,
>> >
>> > On Fri, May 06, 2016 at 11:38:34AM -0700, Hoan Tran wrote:
>> >> From: hotran <hotran@....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@....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.
>> I should check the "count" at first call acpi_table_parse_entries().
>> If "count < 0", assign count=0, then call the next
>> acpi_table_parse_entries().
>>
>> Thanks for your review
>> -Hoan
>
> That second call to acpi_table_parse_entries() might overwrite count with negative number again.
Yes, it could
>
> What about the following below?
>
> int count;
> int sum = 0;
>
> count = acpi_table_parse_entries(type1);
> sum += count >= 0 ? count : 0;
>
> count = acpi_table_parse_entries(type2);
> sum += count >= 0 ? count : 0;
>
> if (sum <= 0 || sum >= MAX_PCC_SUBSPACES) {
>         pr_err();
>         return -ENODEV;
> }
>
> It's possible that I overlooked some corner case.
> BTW, zero number of subspaces here doesn't indicate error actually (that's probably not the scope of this change).
OK, I will use this suggestion with minor changes
int count;
int sum = 0;

count = acpi_table_parse_entries(type1);
sum += count > 0 ? count : 0;

count = acpi_table_parse_entries(type2);
sum += count > 0 ? count : 0;

if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
         pr_err();
         return -ENODEV;
}

Thanks and Regards
Hoan
>
> Best regards,
> Alexey
>

-- 
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments, is 
for the sole use of the intended recipient(s) and contains information that 
is confidential and proprietary to Applied Micro Circuits Corporation or 
its subsidiaries. It is to be used solely for the purpose of furthering the 
parties' business relationship. All unauthorized review, use, disclosure or 
distribution is prohibited. If you are not the intended recipient, please 
contact the sender by reply e-mail and destroy all copies of the original 
message.

Powered by blists - more mailing lists