Message ID | 1503070110-15018-1-git-send-email-kgunda@codeaurora.org (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/25, Shawn Guo wrote: > 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? Right. Does the GPIO work? If so, it sounds like the read/write access checks in spmi pmic arb don't work properly. > > > 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. > Ah I got confused because I thought we numbered GPIO pins from 0, but on the PMIC we number from 1. I see that base = -1 assignment now. I thought that all pins on the pmic were being configured somewhere because I didn't see a gpio 0 usage in DT.
On Fri, Aug 25, 2017 at 04:18:18PM -0700, Stephen Boyd wrote: > On 08/25, Shawn Guo wrote: > > 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? > > Right. Does the GPIO work? If so, it sounds like the read/write > access checks in spmi pmic arb don't work properly. The check works. With the check in there, PM8916 GPIO doesn't work. However, the consequence is that not only user3 but all GPIO leds under 'leds' node will fail to register, because any GPIO led's failing on create_gpio_led() makes leds-gpio driver probe fail as a while. That's how leds-gpio driver works. Also, per schematics, PM8916 GPIO1 is indeed routed to user3 LED on db410c board. Why do you think apq8016-sbc device tree shouldn't use the GPIO for that at all? Isn't it firmware's fault that the ownership of the peripheral is not properly configured? Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/22/2017 4:55 PM, 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 > > ShawnI just realize this patch is trying to fix this issue from spmi driver level. Actually I had submitted a change in spmi-gpio driver to fix this by ignoring the GPIOs which the IRQ is not owned by APPS processor. The maintainer hasn't reviewed it yet: https://www.spinics.net/lists/linux-arm-msm/msg28849.html I am trying to understand if my patch is still needed if Kiran's patch get merged, the intention for my patch originally is for fixing the same probe failure, but it could hide the GPIOs which are not allowed to use from the pinctrl driver level. Please help to suggest. Thanks Fenglin > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Fri, Aug 18, 2017 at 08:58:29PM +0530, 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> > --- > 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(-) This doesn't apply on top of the other spmi patches I just added to my tree. Did I mess something up here? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 28, 2017 at 01:53:28PM +0200, Greg KH wrote: > On Fri, Aug 18, 2017 at 08:58:29PM +0530, 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> > > --- > > 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(-) > > This doesn't apply on top of the other spmi patches I just added to my > tree. Did I mess something up here? Sorry, Greg. We should have told you that this patch is obsolete and replaced by the one below, which you just applied to char-misc-testing. spmi: pmic-arb: Move the ownership check to irq_chip callback Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 28, 2017 at 04:27:54PM +0800, Fenglin Wu wrote: > On 8/22/2017 4:55 PM, 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 > > > >ShawnI just realize this patch is trying to fix this issue from spmi driver > level. Actually I had submitted a change in spmi-gpio driver to fix > this by ignoring the GPIOs which the IRQ is not owned by APPS > processor. The maintainer hasn't reviewed it yet: > https://www.spinics.net/lists/linux-arm-msm/msg28849.html > I am trying to understand if my patch is still needed if Kiran's patch > get merged, the intention for my patch originally is for fixing the same > probe failure, but it could hide the GPIOs which are not allowed to use > from the pinctrl driver level. Please help to suggest. As I just replied to Greg, this patch is obsolete and replaced by 'spmi: pmic-arb: Move the ownership check to irq_chip callback' [1]. With the patch applied, we can get rid of the spmi-gpio probe failure. So from the point of fixing the issue, your patch is not needed. But I would like to discuss your patch a bit, and will reply to your patch with my comments. Shawn [1] https://lkml.org/lkml/2017/8/23/325 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/26, Shawn Guo wrote: > On Fri, Aug 25, 2017 at 04:18:18PM -0700, Stephen Boyd wrote: > > > > Right. Does the GPIO work? If so, it sounds like the read/write > > access checks in spmi pmic arb don't work properly. > > The check works. With the check in there, PM8916 GPIO doesn't work. > However, the consequence is that not only user3 but all GPIO leds under > 'leds' node will fail to register, because any GPIO led's failing on > create_gpio_led() makes leds-gpio driver probe fail as a while. That's > how leds-gpio driver works. > > Also, per schematics, PM8916 GPIO1 is indeed routed to user3 LED on > db410c board. Why do you think apq8016-sbc device tree shouldn't use > the GPIO for that at all? Isn't it firmware's fault that the ownership > of the peripheral is not properly configured? If the ownership was not properly configured in the firmware, then I imagine it would mean that we can't control the GPIO for the LED. But that doesn't seem to be true. I can see on my board that I get impermissible write failures on the GPIO when controlling the GPIO brightness, but it doesn't actually matter because the led still lights up. So the checks for write/read permission seem incorrect, or they're not being enforced. Anyway, I just wanted to make sure the GPIO still works, and it looks like it does, so removing the permission checks is enough to make me happy.
On Wed, Aug 30, 2017 at 02:02:03PM -0700, Stephen Boyd wrote: > On 08/26, Shawn Guo wrote: > > On Fri, Aug 25, 2017 at 04:18:18PM -0700, Stephen Boyd wrote: > > > > > > Right. Does the GPIO work? If so, it sounds like the read/write > > > access checks in spmi pmic arb don't work properly. > > > > The check works. With the check in there, PM8916 GPIO doesn't work. > > However, the consequence is that not only user3 but all GPIO leds under > > 'leds' node will fail to register, because any GPIO led's failing on > > create_gpio_led() makes leds-gpio driver probe fail as a while. That's > > how leds-gpio driver works. > > > > Also, per schematics, PM8916 GPIO1 is indeed routed to user3 LED on > > db410c board. Why do you think apq8016-sbc device tree shouldn't use > > the GPIO for that at all? Isn't it firmware's fault that the ownership > > of the peripheral is not properly configured? > > If the ownership was not properly configured in the firmware, > then I imagine it would mean that we can't control the GPIO for > the LED. But that doesn't seem to be true. I can see on my board > that I get impermissible write failures on the GPIO when > controlling the GPIO brightness, but it doesn't actually matter > because the led still lights up. So the checks for write/read > permission seem incorrect, or they're not being enforced. I'm not sure what is happening on your side. As I said above, with the 4.13-rc series, leds-gpio driver doesn't probe at all, due to the impermissible write to PM8916 GPIO in function create_gpio_led(), and none of the LEDs lights up on my board. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/31, Shawn Guo wrote: > On Wed, Aug 30, 2017 at 02:02:03PM -0700, Stephen Boyd wrote: > > On 08/26, Shawn Guo wrote: > > > On Fri, Aug 25, 2017 at 04:18:18PM -0700, Stephen Boyd wrote: > > > > > > > > Right. Does the GPIO work? If so, it sounds like the read/write > > > > access checks in spmi pmic arb don't work properly. > > > > > > The check works. With the check in there, PM8916 GPIO doesn't work. > > > However, the consequence is that not only user3 but all GPIO leds under > > > 'leds' node will fail to register, because any GPIO led's failing on > > > create_gpio_led() makes leds-gpio driver probe fail as a while. That's > > > how leds-gpio driver works. > > > > > > Also, per schematics, PM8916 GPIO1 is indeed routed to user3 LED on > > > db410c board. Why do you think apq8016-sbc device tree shouldn't use > > > the GPIO for that at all? Isn't it firmware's fault that the ownership > > > of the peripheral is not properly configured? > > > > If the ownership was not properly configured in the firmware, > > then I imagine it would mean that we can't control the GPIO for > > the LED. But that doesn't seem to be true. I can see on my board > > that I get impermissible write failures on the GPIO when > > controlling the GPIO brightness, but it doesn't actually matter > > because the led still lights up. So the checks for write/read > > permission seem incorrect, or they're not being enforced. > > I'm not sure what is happening on your side. As I said above, with the > 4.13-rc series, leds-gpio driver doesn't probe at all, due to the > impermissible write to PM8916 GPIO in function create_gpio_led(), and > none of the LEDs lights up on my board. > Yep. I understand all that. Sorry, I forgot to mention I modified the SPMI PMIC arb code on v4.13-rc7 to continue even though a permission fault may happen by deleting the 'return -EPERM' lines. So the LED GPIO driver is still probing for me, and I see that the GPIOs work regardless of any permission problems that may have been enforced in the hardware. I thought the permission checks that the software is looking at to return EPERM were enforced in hardware, but that doesn't seem to be the case. That's all I was wondering about.
On Thu, Aug 31, 2017 at 06:30:48PM -0700, Stephen Boyd wrote: > On 08/31, Shawn Guo wrote: > > On Wed, Aug 30, 2017 at 02:02:03PM -0700, Stephen Boyd wrote: > > > On 08/26, Shawn Guo wrote: > > > > On Fri, Aug 25, 2017 at 04:18:18PM -0700, Stephen Boyd wrote: > > > > > > > > > > Right. Does the GPIO work? If so, it sounds like the read/write > > > > > access checks in spmi pmic arb don't work properly. > > > > > > > > The check works. With the check in there, PM8916 GPIO doesn't work. > > > > However, the consequence is that not only user3 but all GPIO leds under > > > > 'leds' node will fail to register, because any GPIO led's failing on > > > > create_gpio_led() makes leds-gpio driver probe fail as a while. That's > > > > how leds-gpio driver works. > > > > > > > > Also, per schematics, PM8916 GPIO1 is indeed routed to user3 LED on > > > > db410c board. Why do you think apq8016-sbc device tree shouldn't use > > > > the GPIO for that at all? Isn't it firmware's fault that the ownership > > > > of the peripheral is not properly configured? > > > > > > If the ownership was not properly configured in the firmware, > > > then I imagine it would mean that we can't control the GPIO for > > > the LED. But that doesn't seem to be true. I can see on my board > > > that I get impermissible write failures on the GPIO when > > > controlling the GPIO brightness, but it doesn't actually matter > > > because the led still lights up. So the checks for write/read > > > permission seem incorrect, or they're not being enforced. > > > > I'm not sure what is happening on your side. As I said above, with the > > 4.13-rc series, leds-gpio driver doesn't probe at all, due to the > > impermissible write to PM8916 GPIO in function create_gpio_led(), and > > none of the LEDs lights up on my board. > > > > Yep. I understand all that. > > Sorry, I forgot to mention I modified the SPMI PMIC arb code on > v4.13-rc7 to continue even though a permission fault may happen > by deleting the 'return -EPERM' lines. So the LED GPIO driver is > still probing for me, and I see that the GPIOs work regardless of > any permission problems that may have been enforced in the > hardware. I thought the permission checks that the software is > looking at to return EPERM were enforced in hardware, but that > doesn't seem to be the case. That's all I was wondering about. Ah, okay. You were asking about the check in hardware, while I was talking about the check in software. Yes, now we are on the same page: the permission check in hardware seems not enforced. That's why LED works after we merely remove the check in SPMI PMIC arb driver code. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {