| 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
| ||
|
Date: Thu, 20 Jul 2017 12:14:19 +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,
linux-arm-msm-owner@...r.kernel.org
Subject: Re: [PATCH V3 2/5] spmi: pmic-arb: rename pa_xx to pmic_arb_xx and
other code cleanup
On 2017-07-19 04:16, Stephen Boyd wrote:
> On 07/18, Kiran Gunda wrote:
>> This patch cleans up the following.
>>
>> - Rename the "pa" to "pmic_arb".
>> - Rename the spmi_pmic_arb *dev to spmi_pmic_arb *pmic_arb.
>> - Rename the pa_{read,write}_data() functions to
>> pmic_arb_{read,write}_data().
>> - Rename channel to APID.
>> - Rename the HWIRQ_*() macros to hwirq_to_*().
>> - Clean up qpnpint_irq_set_type() and pmic_arb_find_apid()
>> functions.
>
> This patch still does too much at once. It's really hard to
> verify it. Please split it up by squashing this patch in, and
> then making another patch to do the qpnpint_irq_set_type() change
> and the pmic_arb_find_apid() changes. That's what I did to even
> look at this change sanely. The goal being to make a patch that
> does nothing besides rename things and can be verified to not
> actually change the generated code with scripts/objdiff.
>
ok. will split it up next series.
> More comments after this patch.
>
> diff --git a/drivers/spmi/spmi-pmic-arb.c
> b/drivers/spmi/spmi-pmic-arb.c
> index 91b068b1c83d..2e34dd66feec 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -513,7 +513,6 @@ static void periph_interrupt(struct spmi_pmic_arb
> *pmic_arb, u16 apid)
> static void pmic_arb_chained_irq(struct irq_desc *desc)
> {
> struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
> - const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
> struct irq_chip *chip = irq_desc_get_chip(desc);
> void __iomem *intr = pmic_arb->intr;
> int first = pmic_arb->min_apid >> 5;
> @@ -525,7 +524,7 @@ static void pmic_arb_chained_irq(struct irq_desc
> *desc)
>
> for (i = first; i <= last; ++i) {
> status = readl_relaxed(intr +
> - ver_ops->owner_acc_status(pmic_arb->ee, i));
> + pmic_arb->ver_ops->owner_acc_status(pmic_arb->ee, i));
> while (status) {
> id = ffs(status) - 1;
> status &= ~BIT(id);
> @@ -589,34 +588,34 @@ static int qpnpint_irq_set_type(struct irq_data
> *d, unsigned int flow_type)
> {
> struct spmi_pmic_arb_qpnpint_type type;
> u8 irq = hwirq_to_irq(d->hwirq);
> + u8 bit_mask_irq = BIT(irq);
>
> qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
>
> if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
> - type.type |= BIT(irq);
> + type.type |= bit_mask_irq;
> if (flow_type & IRQF_TRIGGER_RISING)
> - type.polarity_high |= BIT(irq);
> + type.polarity_high |= bit_mask_irq;
> if (flow_type & IRQF_TRIGGER_FALLING)
> - type.polarity_low |= BIT(irq);
> -
> - qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
> - sizeof(type));
> - irq_set_handler_locked(d, handle_edge_irq);
> + type.polarity_low |= bit_mask_irq;
> } else {
> if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
> (flow_type & (IRQF_TRIGGER_LOW)))
> return -EINVAL;
>
> - type.type &= ~BIT(irq); /* level trig */
> + type.type &= ~bit_mask_irq; /* level trig */
> if (flow_type & IRQF_TRIGGER_HIGH)
> - type.polarity_high |= BIT(irq);
> + type.polarity_high |= bit_mask_irq;
> else
> - type.polarity_low |= BIT(irq);
> + type.polarity_low |= bit_mask_irq;
> + }
>
> - qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
> - sizeof(type));
> + qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
> +
> + if (flow_type & IRQ_TYPE_EDGE_BOTH)
> + irq_set_handler_locked(d, handle_edge_irq);
> + else
> irq_set_handler_locked(d, handle_level_irq);
> - }
>
> return 0;
> }
> @@ -763,22 +762,22 @@ static int pmic_arb_offset_v1(struct
> spmi_pmic_arb *pmic_arb, u8 sid, u16 addr,
>
> static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16
> ppid)
> {
> - struct apid_data *apidd = &pmic_arb->apid_data[pmic_arb->last_apid];
> u32 regval, offset;
> - u16 id, apid;
> + u16 apid;
> + u16 id;
>
> /*
> - * PMIC_ARB_REG_APID is a table in HW mapping apid to ppid.
> + * PMIC_ARB_REG_CHNL is a table in HW mapping channel to ppid.
> * ppid_to_apid is an in-memory invert of that table.
> */
> - for (apid = pmic_arb->last_apid; ; apid++, apidd++) {
> + for (apid = pmic_arb->last_apid; ; apid++) {
> offset = PMIC_ARB_REG_APID(apid);
> if (offset >= pmic_arb->core_size)
> break;
>
> regval = readl_relaxed(pmic_arb->cnfg +
> - SPMI_OWNERSHIP_TABLE_REG(apid));
> - apidd->owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
> + SPMI_OWNERSHIP_TABLE_REG(apid));
> + pmic_arb->apid_data[apid].owner =
> SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>
> regval = readl_relaxed(pmic_arb->core + offset);
> if (!regval)
> @@ -786,7 +785,7 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb
> *pmic_arb, u16 ppid)
>
> id = (regval >> 8) & PMIC_ARB_PPID_MASK;
> pmic_arb->ppid_to_apid[id] = apid | PMIC_ARB_APID_VALID;
> - apidd->ppid = id;
> + pmic_arb->apid_data[apid].ppid = id;
> if (id == ppid) {
> apid |= PMIC_ARB_APID_VALID;
> break;
> @@ -797,34 +796,35 @@ static u16 pmic_arb_find_apid(struct
> spmi_pmic_arb *pmic_arb, u16 ppid)
> return apid;
> }
>
> -static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u8
> sid,
> - u16 addr, u16 *apid)
> +
> +static int
> +pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr,
> u16 *apid)
> {
> u16 ppid = (sid << 8) | (addr >> 8);
> u16 apid_valid;
>
> - apid_valid = pmic_arb->ppid_to_apid[ppid];
> + apid_valid = pa->ppid_to_apid[ppid];
> if (!(apid_valid & PMIC_ARB_APID_VALID))
> - apid_valid = pmic_arb_find_apid(pmic_arb, ppid);
> + apid_valid = pmic_arb_find_apid(pa, ppid);
> if (!(apid_valid & PMIC_ARB_APID_VALID))
> return -ENODEV;
>
> - *apid = apid_valid & ~PMIC_ARB_APID_VALID;
> + *apid = (apid_valid & ~PMIC_ARB_APID_VALID);
> return 0;
> }
>
> /* v2 offset per ppid and per ee */
> -static int pmic_arb_offset_v2(struct spmi_pmic_arb *pmic_arb, u8 sid,
> u16 addr,
> - u32 *offset)
> +static int
> +pmic_arb_offset_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u32
> *offset)
> {
> u16 apid;
> int rc;
>
> - rc = pmic_arb_ppid_to_apid_v2(pmic_arb, sid, addr, &apid);
> + rc = pmic_arb_ppid_to_apid_v2(pa, sid, addr, &apid);
> if (rc < 0)
> return rc;
>
> - *offset = 0x1000 * pmic_arb->ee + 0x8000 * apid;
> + *offset = 0x1000 * pa->ee + 0x8000 * apid;
> return 0;
> }
>
> @@ -930,7 +930,6 @@ static int spmi_pmic_arb_probe(struct
> platform_device *pdev)
> struct spmi_controller *ctrl;
> struct resource *res;
> void __iomem *core;
> - u32 *mapping_table;
> u32 channel, ee, hw_ver;
> int err;
>
> @@ -1040,14 +1039,13 @@ static int spmi_pmic_arb_probe(struct
> platform_device *pdev)
> }
>
> pmic_arb->ee = ee;
> - mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
> - sizeof(*mapping_table), GFP_KERNEL);
> - if (!mapping_table) {
> + pmic_arb->mapping_table = devm_kcalloc(&ctrl->dev,
> PMIC_ARB_MAX_PERIPHS - 1,
> + sizeof(*pmic_arb->mapping_table), GFP_KERNEL);
> + if (!pmic_arb->mapping_table) {
> err = -ENOMEM;
> goto err_put_ctrl;
> }
>
> - pmic_arb->mapping_table = mapping_table;
> /* Initialize max_apid/min_apid to the opposite bounds, during
> * the irq domain translation, we are sure to update these */
> pmic_arb->max_apid = 0;
>
>> -static void periph_interrupt(struct spmi_pmic_arb *pa, u16 apid)
>> +static void periph_interrupt(struct spmi_pmic_arb *pmic_arb, u16
>> apid)
>> {
>> + const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
>> + u8 sid = (pmic_arb->apid_data[apid].ppid >> 8) & 0xF;
>> + u8 per = pmic_arb->apid_data[apid].ppid & 0xFF;
>> unsigned int irq;
>> u32 status;
>> int id;
>> - u8 sid = (pa->apid_data[apid].ppid >> 8) & 0xF;
>> - u8 per = pa->apid_data[apid].ppid & 0xFF;
>
> Why did these two move? Should stay in the same place and take
> the rename.
>
Sure. will do it in next patch series.
>>
>> - status = readl_relaxed(pa->intr + pa->ver_ops->irq_status(apid));
>> + status = readl_relaxed(pmic_arb->intr + ver_ops->irq_status(apid));
>> while (status) {
>> id = ffs(status) - 1;
>> status &= ~BIT(id);
>> - irq = irq_find_mapping(pa->domain, HWIRQ(sid, per, id, apid));
>> + irq = irq_find_mapping(pmic_arb->domain,
>> + spec_to_hwirq(sid, per, id, apid));
>> if (irq == 0) {
>> - cleanup_irq(pa, apid, id);
>> + cleanup_irq(pmic_arb, apid, id);
>> continue;
>> }
>> generic_handle_irq(irq);
>> @@ -515,11 +512,12 @@ static void periph_interrupt(struct
>> spmi_pmic_arb *pa, u16 apid)
>>
>> static void pmic_arb_chained_irq(struct irq_desc *desc)
>> {
>> - struct spmi_pmic_arb *pa = irq_desc_get_handler_data(desc);
>> + struct spmi_pmic_arb *pmic_arb = irq_desc_get_handler_data(desc);
>> + const struct pmic_arb_ver_ops *ver_ops = pmic_arb->ver_ops;
>> struct irq_chip *chip = irq_desc_get_chip(desc);
>> - void __iomem *intr = pa->intr;
>> - int first = pa->min_apid >> 5;
>> - int last = pa->max_apid >> 5;
>> + void __iomem *intr = pmic_arb->intr;
>> + int first = pmic_arb->min_apid >> 5;
>> + int last = pmic_arb->max_apid >> 5;
>> u32 status, enable;
>> int i, id, apid;
>>
>> @@ -527,15 +525,15 @@ static void pmic_arb_chained_irq(struct irq_desc
>> *desc)
>>
>> for (i = first; i <= last; ++i) {
>> status = readl_relaxed(intr +
>> - pa->ver_ops->owner_acc_status(pa->ee, i));
>> + ver_ops->owner_acc_status(pmic_arb->ee, i));
>> while (status) {
>> id = ffs(status) - 1;
>> status &= ~BIT(id);
>> apid = id + i * 32;
>> enable = readl_relaxed(intr +
>> - pa->ver_ops->acc_enable(apid));
>> + pmic_arb->ver_ops->acc_enable(apid));
>
> This still uses pmic_arb->ver_ops instead of ver_ops?
>
will correct it next patch series.
>> if (enable & SPMI_PIC_ACC_ENABLE_BIT)
>> - periph_interrupt(pa, apid);
>> + periph_interrupt(pmic_arb, apid);
>> }
>> }
>>
>> @@ -589,35 +588,35 @@ static void qpnpint_irq_unmask(struct irq_data
>> *d)
>> static int qpnpint_irq_set_type(struct irq_data *d, unsigned int
>> flow_type)
>> {
>> struct spmi_pmic_arb_qpnpint_type type;
>> - u8 irq = HWIRQ_IRQ(d->hwirq);
>> - u8 bit_mask_irq = BIT(irq);
>> + u8 irq = hwirq_to_irq(d->hwirq);
>>
>> qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
>>
>> if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
>> - type.type |= bit_mask_irq;
>> + type.type |= BIT(irq);
>> if (flow_type & IRQF_TRIGGER_RISING)
>> - type.polarity_high |= bit_mask_irq;
>> + type.polarity_high |= BIT(irq);
>> if (flow_type & IRQF_TRIGGER_FALLING)
>> - type.polarity_low |= bit_mask_irq;
>> + type.polarity_low |= BIT(irq);
>> +
>> + qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
>> + sizeof(type));
>> + irq_set_handler_locked(d, handle_edge_irq);
>> } else {
>> if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
>> (flow_type & (IRQF_TRIGGER_LOW)))
>> return -EINVAL;
>>
>> - type.type &= ~bit_mask_irq; /* level trig */
>> + type.type &= ~BIT(irq); /* level trig */
>> if (flow_type & IRQF_TRIGGER_HIGH)
>> - type.polarity_high |= bit_mask_irq;
>> + type.polarity_high |= BIT(irq);
>> else
>> - type.polarity_low |= bit_mask_irq;
>> - }
>> -
>> - qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, sizeof(type));
>> + type.polarity_low |= BIT(irq);
>>
>> - if (flow_type & IRQ_TYPE_EDGE_BOTH)
>> - irq_set_handler_locked(d, handle_edge_irq);
>> - else
>> + qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type,
>> + sizeof(type));
>> irq_set_handler_locked(d, handle_level_irq);
>> + }
>
> Not sure I suggested this, but I would keep the REG_SET_TYPE call
> outside of the if statements to consolidate them, and also grow a
> local variable to hold the handler (aptly called handler) that
> then can be assigned with a single call to
> irq_set_handler_locked(). Please do this in a different patch.
ok ... I will modify it as per your current suggestion.
Powered by blists - more mailing lists