| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4a6e9ec838d6afc1ca300e6aee7b8999@codeaurora.org>
Date: Tue, 18 Jul 2017 10:39:48 +0530
From: kgunda@...eaurora.org
To: Stephen Boyd <sboyd@...eaurora.org>
Cc: gregkh@...uxfoundation.org,
Abhijeet Dharmapurikar <adharmap@...eaurora.org>,
David Collins <collinsd@...eaurora.org>,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH V3 3/4] spmi: pmic-arb: add support for HW version 5
On 2017-07-14 23:00, Stephen Boyd wrote:
> On 07/11, Kiran Gunda wrote:
>> @@ -420,7 +440,8 @@ static int pmic_arb_write_cmd(struct
>> spmi_controller *ctrl, u8 opc, u8 sid,
>>
>
> Mostly style nitpicks!
>
Will check and address in the next patch.
>> /* Start the transaction */
>> pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
>> - rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr);
>> + rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr,
>> + PMIC_ARB_CHANNEL_RW);
>> raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
>>
>> return rc;
>> @@ -681,12 +702,19 @@ static int
>> qpnpint_irq_domain_dt_translate(struct irq_domain *d,
>> ppid = intspec[0] << 8 | intspec[1];
>> rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
>> if (rc < 0) {
>> - dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = 0x%x, periph
>> = 0x%x, irq = %x rc = %d\n",
>> + dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph =
>> %#x, irq = %u rc = %d\n",
>> intspec[0], intspec[1], intspec[2], rc);
>
> Unrelated change?
>
This just to print the irq number in decimal format.
>> return rc;
>> }
>>
>> apid = rc;
>> + if (pmic_arb->apid_data[apid].irq_ee != pmic_arb->ee) {
>> + dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph =
>> %#x, irq = %u: ee=%u but owner=%u\n",
>> + intspec[0], intspec[1], intspec[2], pmic_arb->ee,
>> + pmic_arb->apid_data[apid].irq_ee);
>> + return -ENODEV;
>> + }
>> +
>> /* Keep track of {max,min}_apid for bounding search during interrupt
>> */
>> if (apid > pmic_arb->max_apid)
>> pmic_arb->max_apid = apid;
>> return apid_valid & ~PMIC_ARB_APID_VALID;
>> }
>>
>> +static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
>> +{
>> + struct apid_data *apid_info = pmic_arb->apid_data;
>> + struct apid_data *prev_apid_info;
>> + u16 i, j, ppid;
>> + bool valid, is_irq_ee;
>> + u32 regval, offset;
>> +
>> + /*
>> + * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to
>> PPID.
>
> Is this comment stale? PMIC_ARB_REG_CHNL macro was deleted?
>
will remove this line in the next patch.
>> + * ppid_to_apid is an in-memory invert of that table. In order to
>> allow
>> + * multiple EEs to write to a single PPID in arbiter version 5,
>> there
>> + * is more than one APID mapped to each PPID. The owner field for
>> each
>> + * of these mappings specifies the EE which is allowed to write to
>> the
>> + * APID. The owner of the last (highest) APID for a given PPID will
>> + * receive interrupts from the PPID.
>> + */
>> + for (i = 0; ; i++, apid_info++) {
>> + offset = pmic_arb->ver_ops->apid_map_offset(i);
>> + if (offset >= pmic_arb->core_size)
>> + break;
>> +
>> + regval = readl_relaxed(pmic_arb->core + offset);
>> + if (!regval)
>> + continue;
>> + ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
>> + is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
>> +
>> + regval = readl_relaxed(pmic_arb->cnfg +
>> + SPMI_OWNERSHIP_TABLE_REG(i));
>> + apid_info->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>> +
>> + apid_info->irq_ee = is_irq_ee ?
>> + apid_info->write_ee : INVALID_EE;
>
> Perhaps apid_info can be renamed to apidd (for apid descriptor)
> or apidi (for apid info) and then this line is short enough to
> fit on one line?
>
ok. will change it in next patch.
>> +
>> + valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
>> + j = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
>
> Maybe j can be 'apid'. Slightly more informative and usually 'j'
> is reserved for iterating, which in this case we're not doing.
> We're just directly indexing an apid into a table.
>
Will change in the next patch.
>> + prev_apid_info = &pmic_arb->apid_data[j];
>> +
>> + if (valid && is_irq_ee &&
>> + prev_apid_info->write_ee == pmic_arb->ee) {
>> + /*
>> + * Duplicate PPID mapping after the one for this EE;
>> + * override the irq owner
>> + */
>> + prev_apid_info->irq_ee = apid_info->irq_ee;
>> + } else if (!valid || is_irq_ee) {
>> + /* First PPID mapping or duplicate for another EE */
>> + pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
>> + }
>> +
>> + apid_info->ppid = ppid;
>> + pmic_arb->last_apid = i;
>> + }
>> +
>> + /* Dump the mapping table for debug purposes. */
>> + dev_dbg(&pmic_arb->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
>> + for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
>> + valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
>> + i = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
>> + if (valid) {
>> + apid_info = &pmic_arb->apid_data[i];
>> + dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
>> + ppid, i, apid_info->write_ee, apid_info->irq_ee);
>> + }
>
> Could be
>
> for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
> apid = pmic_arb->ppid_to_apid[ppid];
> if (apid & PMIC_ARB_APID_VALID) {
> apid &= ~PMIC_ARB_VALID;
> apidd = &pmic_arb->apid_data[apid];
> dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
> ppid, apid, apidd->write_ee, apidd->irq_ee);
> }
> }
>
> Which maybe is clearer because it uses less local variables that
> don't get used more than once.
yes. Will change it in the next patch.
Powered by blists - more mailing lists