Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1715255 > unrolled thread

[PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally

Back to article view | Back to linux.kernel

Contents

  [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally Kiran Gunda <kgunda@codeaurora.org> - 2017-08-18 17:30 +0200
    Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally Stephen Boyd <sboyd@codeaurora.org> - 2017-08-22 01:20 +0200
      Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally Shawn Guo <shawnguo@kernel.org> - 2017-08-22 11:00 +0200
        Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally Stephen Boyd <sboyd@codeaurora.org> - 2017-08-22 22:40 +0200
          Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally kgunda@codeaurora.org - 2017-08-23 15:00 +0200
          Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally Shawn Guo <shawnguo@kernel.org> - 2017-08-24 14:20 +0200
            Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally Stephen Boyd <sboyd@codeaurora.org> - 2017-08-24 20:40 +0200
              Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally Shawn Guo <shawnguo@kernel.org> - 2017-08-25 09:50 +0200
      Re: [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally Shawn Guo <shawnguo@kernel.org> - 2017-08-22 11:10 +0200

#1715255 — [PATCH V2] spmi: pmic-arb: Enforce the ownership check optionally

The peripheral ownership check is not necessary on single master
platforms. Hence, enforce the peripheral ownership check optionally.

Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
Tested-by: Shawn Guo <shawnguo@kernel.org>
---
v2:
	Fixed the commit message.
	Added Shawn's 'Tested-by' tag.

v1:
	This patch depends on the below patch series. Please take this patch
	along with this series.

	[PATCH V2 00/12]: spmi: pmic-arb: Support for HW v5 and other fixes

 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt | 6 ++++++
 drivers/spmi/spmi-pmic-arb.c                                  | 7 ++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
index e16b9b5..da708e8 100644
--- a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
+++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
@@ -42,6 +42,10 @@ Required properties:
     cell 4: interrupt flags indicating level-sense information, as defined in
             dt-bindings/interrupt-controller/irq.h
 
+Optional properties:
+- qcom,enforce-ownership : A boolean property. If defined the peripheral
+			   ownership check is enforced. Otherwise the ownership
+			   check is ignored.
 Example:
 
 	spmi {
@@ -62,4 +66,6 @@ Example:
 
 		interrupt-controller;
 		#interrupt-cells = <4>;
+
+		qcom,enforce-ownership;
 	};
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index ca9bdd3..354c949 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -167,6 +167,7 @@ struct spmi_pmic_arb {
 	u16			*ppid_to_apid;
 	u16			last_apid;
 	struct apid_data	apid_data[PMIC_ARB_MAX_PERIPHS];
+	bool			enforce_ownership;
 };
 
 /**
@@ -707,7 +708,8 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
 	}
 
 	apid = rc;
-	if (pmic_arb->apid_data[apid].irq_ee != pmic_arb->ee) {
+	if (pmic_arb->enforce_ownership &&
+			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);
@@ -1236,6 +1238,9 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	}
 
 	pmic_arb->ee = ee;
+	pmic_arb->enforce_ownership =
+	    of_property_read_bool(pdev->dev.of_node, "qcom,enforce-ownership");
+
 	mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS,
 					sizeof(*mapping_table), GFP_KERNEL);
 	if (!mapping_table) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1716937

On 08/18/2017 08:28 AM, Kiran Gunda wrote:
> The peripheral ownership check is not necessary on single master
> platforms. Hence, enforce the peripheral ownership check optionally.
>
> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> Tested-by: Shawn Guo <shawnguo@kernel.org>
> ---

This sounds like a band-aid. Isn't the gpio driver going to keep probing
all the pins that are not supposed to be accessed due to security
constraints? What exactly is failing in the gpio case?

Also, I thought we were getting rid of the ownership checks? Or at
least, putting them behind some debug kernel feature check or something?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1717199

On Mon, Aug 21, 2017 at 04:18:58PM -0700, Stephen Boyd wrote:
> On 08/18/2017 08:28 AM, Kiran Gunda wrote:
> > The peripheral ownership check is not necessary on single master
> > platforms. Hence, enforce the peripheral ownership check optionally.
> >
> > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> > Tested-by: Shawn Guo <shawnguo@kernel.org>
> > ---
> 
> This sounds like a band-aid. Isn't the gpio driver going to keep probing
> all the pins that are not supposed to be accessed due to security
> constraints? What exactly is failing in the gpio case?

There is a platform_irq_count() call in pinctrl-spmi-gpio probe
function.  Due to the owner check in spmi-pmic-arb IRQ domain
qpnpint_irq_domain_dt_translate() function, the call will return irq
number as zero and cause pmic_gpio_probe() fail with -EINVAL error.

[    1.608516] [<ffff00000860e51c>] qpnpint_irq_domain_dt_translate+0x168/0x194
[    1.613557] [<ffff000008117040>] irq_create_fwspec_mapping+0x17c/0x2d8
[    1.620672] [<ffff000008117200>] irq_create_of_mapping+0x64/0x74
[    1.627008] [<ffff0000087b4fac>] of_irq_get+0x54/0x64
[    1.633169] [<ffff00000856b824>] platform_get_irq+0x20/0x150
[    1.638117] [<ffff00000856b97c>] platform_irq_count+0x28/0x44
[    1.643850] [<ffff0000083cf12c>] pmic_gpio_probe+0x50/0x544

Shawn

[toc] | [prev] | [next] | [standalone]

#1717818

On 08/22, Shawn Guo wrote:
> On Mon, Aug 21, 2017 at 04:18:58PM -0700, Stephen Boyd wrote:
> > On 08/18/2017 08:28 AM, Kiran Gunda wrote:
> > > The peripheral ownership check is not necessary on single master
> > > platforms. Hence, enforce the peripheral ownership check optionally.
> > >
> > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> > > Tested-by: Shawn Guo <shawnguo@kernel.org>
> > > ---
> > 
> > This sounds like a band-aid. Isn't the gpio driver going to keep probing
> > all the pins that are not supposed to be accessed due to security
> > constraints? What exactly is failing in the gpio case?
> 
> There is a platform_irq_count() call in pinctrl-spmi-gpio probe
> function.  Due to the owner check in spmi-pmic-arb IRQ domain
> qpnpint_irq_domain_dt_translate() function, the call will return irq
> number as zero and cause pmic_gpio_probe() fail with -EINVAL error.
> 
> [    1.608516] [<ffff00000860e51c>] qpnpint_irq_domain_dt_translate+0x168/0x194
> [    1.613557] [<ffff000008117040>] irq_create_fwspec_mapping+0x17c/0x2d8
> [    1.620672] [<ffff000008117200>] irq_create_of_mapping+0x64/0x74
> [    1.627008] [<ffff0000087b4fac>] of_irq_get+0x54/0x64
> [    1.633169] [<ffff00000856b824>] platform_get_irq+0x20/0x150
> [    1.638117] [<ffff00000856b97c>] platform_irq_count+0x28/0x44
> [    1.643850] [<ffff0000083cf12c>] pmic_gpio_probe+0x50/0x544
> 

Hmm. Ok. I guess platform_irq_count() has to go and create irq
mappings if they haven't been created yet and that then causes us
to check if we can even get the interrupt for this particular
irq? There are some interrupt lines that are not routed to the
application processor in the system, so the irq_ee (irq execution
environment) is different. This check is there to avoid creating
flow handlers for irqs that can't be triggered.

I can see how trying to request that irq doesn't make sense,
because it won't ever happen. But preventing that from being
translated is confusing. Perhaps we can move the check for irq_ee
to the irq_request_resources() callback in the irqchip? That way,
we can fail installing the flow handler for the interrupt we
can't ever receive, but otherwise translate the interrupt number
so we can keep counting them.

Also, I see that on v4.13-rc series the read/write checks are
causing the led driver to fail in a different way:

    spmi spmi-0: error: impermissible write to peripheral sid:0 addr:0xc040
    qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x40 failed
    leds-gpio soc:leds: Error applying setting, reverse things back
    spmi spmi-0: error: impermissible write to peripheral sid:0 addr:0xc041
    qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x41 failed
    leds-gpio: probe of soc:leds failed with error -1 

Are you seeing similar behavior?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1718334

On 2017-08-23 02:01, Stephen Boyd wrote:
> On 08/22, Shawn Guo wrote:
>> On Mon, Aug 21, 2017 at 04:18:58PM -0700, Stephen Boyd wrote:
>> > On 08/18/2017 08:28 AM, Kiran Gunda wrote:
>> > > The peripheral ownership check is not necessary on single master
>> > > platforms. Hence, enforce the peripheral ownership check optionally.
>> > >
>> > > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
>> > > Tested-by: Shawn Guo <shawnguo@kernel.org>
>> > > ---
>> >
>> > This sounds like a band-aid. Isn't the gpio driver going to keep probing
>> > all the pins that are not supposed to be accessed due to security
>> > constraints? What exactly is failing in the gpio case?
>> 
>> There is a platform_irq_count() call in pinctrl-spmi-gpio probe
>> function.  Due to the owner check in spmi-pmic-arb IRQ domain
>> qpnpint_irq_domain_dt_translate() function, the call will return irq
>> number as zero and cause pmic_gpio_probe() fail with -EINVAL error.
>> 
>> [    1.608516] [<ffff00000860e51c>] 
>> qpnpint_irq_domain_dt_translate+0x168/0x194
>> [    1.613557] [<ffff000008117040>] 
>> irq_create_fwspec_mapping+0x17c/0x2d8
>> [    1.620672] [<ffff000008117200>] irq_create_of_mapping+0x64/0x74
>> [    1.627008] [<ffff0000087b4fac>] of_irq_get+0x54/0x64
>> [    1.633169] [<ffff00000856b824>] platform_get_irq+0x20/0x150
>> [    1.638117] [<ffff00000856b97c>] platform_irq_count+0x28/0x44
>> [    1.643850] [<ffff0000083cf12c>] pmic_gpio_probe+0x50/0x544
>> 
> 
> Hmm. Ok. I guess platform_irq_count() has to go and create irq
> mappings if they haven't been created yet and that then causes us
> to check if we can even get the interrupt for this particular
> irq? There are some interrupt lines that are not routed to the
> application processor in the system, so the irq_ee (irq execution
> environment) is different. This check is there to avoid creating
> flow handlers for irqs that can't be triggered.
> 
> I can see how trying to request that irq doesn't make sense,
> because it won't ever happen. But preventing that from being
> translated is confusing. Perhaps we can move the check for irq_ee
> to the irq_request_resources() callback in the irqchip? That way,
> we can fail installing the flow handler for the interrupt we
> can't ever receive, but otherwise translate the interrupt number
> so we can keep counting them.
> 
Hi Stephen,
The idea to move the ownership check to irq_request_resources sounds 
good.
I am dropping this patch and sent the new patch to move the irq 
ownership to
irq_request_resource. Following is the patchwork link.

Shawn, can you please give a try with it?
https://patchwork.kernel.org/patch/9917315/

> Also, I see that on v4.13-rc series the read/write checks are
> causing the led driver to fail in a different way:
> 
>     spmi spmi-0: error: impermissible write to peripheral sid:0 
> addr:0xc040
>     qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x40 failed
>     leds-gpio soc:leds: Error applying setting, reverse things back
>     spmi spmi-0: error: impermissible write to peripheral sid:0 
> addr:0xc041
>     qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x41 failed
>     leds-gpio: probe of soc:leds failed with error -1
> 
> Are you seeing similar behavior?
With the new patch series these errors will go away, as we are removing 
the ownership
checks from the read/write path.

[toc] | [prev] | [next] | [standalone]

#1719190

On Tue, Aug 22, 2017 at 01:31:32PM -0700, Stephen Boyd wrote:
> Also, I see that on v4.13-rc series the read/write checks are
> causing the led driver to fail in a different way:
> 
>     spmi spmi-0: error: impermissible write to peripheral sid:0 addr:0xc040
>     qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x40 failed
>     leds-gpio soc:leds: Error applying setting, reverse things back
>     spmi spmi-0: error: impermissible write to peripheral sid:0 addr:0xc041
>     qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x41 failed
>     leds-gpio: probe of soc:leds failed with error -1 
> 
> Are you seeing similar behavior?

Yes.  I forgot to mention that, and leds-gpio failure is gone after
applying Kiran's patch below.

spmi: pmic-arb: remove the read/write access checks

Shawn

[toc] | [prev] | [next] | [standalone]

#1719501

On 08/24, Shawn Guo wrote:
> On Tue, Aug 22, 2017 at 01:31:32PM -0700, Stephen Boyd wrote:
> > Also, I see that on v4.13-rc series the read/write checks are
> > causing the led driver to fail in a different way:
> > 
> >     spmi spmi-0: error: impermissible write to peripheral sid:0 addr:0xc040
> >     qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x40 failed
> >     leds-gpio soc:leds: Error applying setting, reverse things back
> >     spmi spmi-0: error: impermissible write to peripheral sid:0 addr:0xc041
> >     qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x41 failed
> >     leds-gpio: probe of soc:leds failed with error -1 
> > 
> > Are you seeing similar behavior?
> 
> Yes.  I forgot to mention that, and leds-gpio failure is gone after
> applying Kiran's patch below.
> 
> spmi: pmic-arb: remove the read/write access checks
> 

Sure. Removing the checks will silence the warnings, but it still
means that we're attempting to configure GPIOs that we shouldn't
be configuring. Is there some sort of default configuration that
gets applied to all pins by default?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1719807

On Thu, Aug 24, 2017 at 11:37:01AM -0700, Stephen Boyd wrote:
> On 08/24, Shawn Guo wrote:
> > On Tue, Aug 22, 2017 at 01:31:32PM -0700, Stephen Boyd wrote:
> > > Also, I see that on v4.13-rc series the read/write checks are
> > > causing the led driver to fail in a different way:
> > > 
> > >     spmi spmi-0: error: impermissible write to peripheral sid:0 addr:0xc040
> > >     qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x40 failed
> > >     leds-gpio soc:leds: Error applying setting, reverse things back
> > >     spmi spmi-0: error: impermissible write to peripheral sid:0 addr:0xc041
> > >     qcom-spmi-gpio 200f000.spmi:pm8916@0:gpios@c000: write 0x41 failed
> > >     leds-gpio: probe of soc:leds failed with error -1 
> > > 
> > > Are you seeing similar behavior?
> > 
> > Yes.  I forgot to mention that, and leds-gpio failure is gone after
> > applying Kiran's patch below.
> > 
> > spmi: pmic-arb: remove the read/write access checks
> > 
> 
> Sure. Removing the checks will silence the warnings, but it still
> means that we're attempting to configure GPIOs that we shouldn't
> be configuring.

The driver is attempting to configure the GPIOs that device tree tells
to.

	led@3 {                        
		label = "apq8016-sbc:green:user3";
		gpios = <&pm8916_gpios 1 GPIO_ACTIVE_HIGH>;
		linux,default-trigger = "mmc1";
		default-state = "off";         
	};

Are you saying, in case of user3 led above, device tree shouldn't use
GPIO <&pm8916_gpios 1> there at all?

> Is there some sort of default configuration that
> gets applied to all pins by default?

I do not quite understand what you are asking and how that is related to
the thing we discuss here.  But my understanding is that spmi_arb
read/write access is used not only by pinctrl API to set up pinmux for
GPIO function, but also by GPIO API to actually drive the GPIO.

Shawn

[toc] | [prev] | [next] | [standalone]

#1717203

On Mon, Aug 21, 2017 at 04:18:58PM -0700, Stephen Boyd wrote:
> On 08/18/2017 08:28 AM, Kiran Gunda wrote:
> > The peripheral ownership check is not necessary on single master
> > platforms. Hence, enforce the peripheral ownership check optionally.
> >
> > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> > Tested-by: Shawn Guo <shawnguo@kernel.org>
> > ---
> 
> This sounds like a band-aid. Isn't the gpio driver going to keep probing
> all the pins that are not supposed to be accessed due to security
> constraints? What exactly is failing in the gpio case?
> 
> Also, I thought we were getting rid of the ownership checks? Or at
> least, putting them behind some debug kernel feature check or something?

I'm wondering that too.  Since we have the following patch to remove the
check on read/write access anyway, why are we adding the check in .xlate
hook?

spmi: pmic-arb: remove the read/write access checks

Shawn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web