* [RFT v2 02/10] pinctrl: samsung: Document suspend and resume members
From: Krzysztof Kozlowski @ 2018-07-23 17:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180723175302.22535-1-krzk@kernel.org>
Add missing documentation for suspend and resume members of struct
samsung_pin_ctrl and samsung_pinctrl_drv_data.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
---
drivers/pinctrl/samsung/pinctrl-samsung.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index f0cda9424dfe..aac16cc8362a 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -223,6 +223,10 @@ struct samsung_retention_data {
* interrupts for the controller.
* @eint_wkup_init: platform specific callback to setup the external wakeup
* interrupts for the controller.
+ * @suspend: platform specific suspend callback, executed during pin controller
+ * device suspend, see samsung_pinctrl_suspend()
+ * @resume: platform specific resume callback, executed during pin controller
+ * device suspend, see samsung_pinctrl_resume()
*/
struct samsung_pin_ctrl {
const struct samsung_pin_bank_data *pin_banks;
@@ -255,6 +259,10 @@ struct samsung_pin_ctrl {
* @pin_base: starting system wide pin number.
* @nr_pins: number of pins supported by the controller.
* @retention_ctrl: retention control runtime data.
+ * @suspend: platform specific suspend callback, executed during pin controller
+ * device suspend, see samsung_pinctrl_suspend()
+ * @resume: platform specific resume callback, executed during pin controller
+ * device suspend, see samsung_pinctrl_resume()
*/
struct samsung_pinctrl_drv_data {
struct list_head node;
--
2.14.1
^ permalink raw reply related
* [RFT v2 02/10] pinctrl: samsung: Document suspend and resume members
From: Krzysztof Kozlowski @ 2018-07-23 17:52 UTC (permalink / raw)
To: Tomasz Figa, Krzysztof Kozlowski, Sylwester Nawrocki,
Linus Walleij, Rob Herring, Mark Rutland, Kukjin Kim,
Russell King, Kyungmin Park, linux-arm-kernel, linux-samsung-soc,
linux-gpio, devicetree, linux-kernel
Cc: Paweł Chmiel, Sylwester Nawrocki, Chanwoo Choi, Alim Akhtar,
Pankaj Dubey
In-Reply-To: <20180723175302.22535-1-krzk@kernel.org>
Add missing documentation for suspend and resume members of struct
samsung_pin_ctrl and samsung_pinctrl_drv_data.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
---
drivers/pinctrl/samsung/pinctrl-samsung.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index f0cda9424dfe..aac16cc8362a 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -223,6 +223,10 @@ struct samsung_retention_data {
* interrupts for the controller.
* @eint_wkup_init: platform specific callback to setup the external wakeup
* interrupts for the controller.
+ * @suspend: platform specific suspend callback, executed during pin controller
+ * device suspend, see samsung_pinctrl_suspend()
+ * @resume: platform specific resume callback, executed during pin controller
+ * device suspend, see samsung_pinctrl_resume()
*/
struct samsung_pin_ctrl {
const struct samsung_pin_bank_data *pin_banks;
@@ -255,6 +259,10 @@ struct samsung_pin_ctrl {
* @pin_base: starting system wide pin number.
* @nr_pins: number of pins supported by the controller.
* @retention_ctrl: retention control runtime data.
+ * @suspend: platform specific suspend callback, executed during pin controller
+ * device suspend, see samsung_pinctrl_suspend()
+ * @resume: platform specific resume callback, executed during pin controller
+ * device suspend, see samsung_pinctrl_resume()
*/
struct samsung_pinctrl_drv_data {
struct list_head node;
--
2.14.1
^ permalink raw reply related
* [RFT v2 01/10] pinctrl: samsung: Define suspend and resume callbacks for all banks and SoCs
From: Krzysztof Kozlowski @ 2018-07-23 17:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180723175302.22535-1-krzk@kernel.org>
Suspend and resume callbacks in Exynos/S5Pv210 pin controller drivers,
save and restore state of registers. This operations should be done for
all banks which have external interrupts (as denoted by using
EXYNOS_PIN_BANK_EINTG/EINTW macros).
Add all banks of Exynos5260 and Exynos5420. This is necessary step for
supporting suspend to RAM on these SoCs.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
---
drivers/pinctrl/samsung/pinctrl-exynos-arm.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
index d82820fc349a..44c6b753f692 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
@@ -616,16 +616,22 @@ static const struct samsung_pin_ctrl exynos5260_pin_ctrl[] __initconst = {
.nr_banks = ARRAY_SIZE(exynos5260_pin_banks0),
.eint_gpio_init = exynos_eint_gpio_init,
.eint_wkup_init = exynos_eint_wkup_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
}, {
/* pin-controller instance 1 data */
.pin_banks = exynos5260_pin_banks1,
.nr_banks = ARRAY_SIZE(exynos5260_pin_banks1),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
}, {
/* pin-controller instance 2 data */
.pin_banks = exynos5260_pin_banks2,
.nr_banks = ARRAY_SIZE(exynos5260_pin_banks2),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
},
};
@@ -842,30 +848,40 @@ static const struct samsung_pin_ctrl exynos5420_pin_ctrl[] __initconst = {
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks0),
.eint_gpio_init = exynos_eint_gpio_init,
.eint_wkup_init = exynos_eint_wkup_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos5420_retention_data,
}, {
/* pin-controller instance 1 data */
.pin_banks = exynos5420_pin_banks1,
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks1),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos5420_retention_data,
}, {
/* pin-controller instance 2 data */
.pin_banks = exynos5420_pin_banks2,
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks2),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos5420_retention_data,
}, {
/* pin-controller instance 3 data */
.pin_banks = exynos5420_pin_banks3,
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks3),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos5420_retention_data,
}, {
/* pin-controller instance 4 data */
.pin_banks = exynos5420_pin_banks4,
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks4),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos4_audio_retention_data,
},
};
--
2.14.1
^ permalink raw reply related
* [RFT v2 01/10] pinctrl: samsung: Define suspend and resume callbacks for all banks and SoCs
From: Krzysztof Kozlowski @ 2018-07-23 17:52 UTC (permalink / raw)
To: Tomasz Figa, Krzysztof Kozlowski, Sylwester Nawrocki,
Linus Walleij, Rob Herring, Mark Rutland, Kukjin Kim,
Russell King, Kyungmin Park, linux-arm-kernel, linux-samsung-soc,
linux-gpio, devicetree, linux-kernel
Cc: Paweł Chmiel, Sylwester Nawrocki, Chanwoo Choi, Alim Akhtar,
Pankaj Dubey
In-Reply-To: <20180723175302.22535-1-krzk@kernel.org>
Suspend and resume callbacks in Exynos/S5Pv210 pin controller drivers,
save and restore state of registers. This operations should be done for
all banks which have external interrupts (as denoted by using
EXYNOS_PIN_BANK_EINTG/EINTW macros).
Add all banks of Exynos5260 and Exynos5420. This is necessary step for
supporting suspend to RAM on these SoCs.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Sylwester Nawrocki <snawrocki@kernel.org>
Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
---
drivers/pinctrl/samsung/pinctrl-exynos-arm.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
index d82820fc349a..44c6b753f692 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
@@ -616,16 +616,22 @@ static const struct samsung_pin_ctrl exynos5260_pin_ctrl[] __initconst = {
.nr_banks = ARRAY_SIZE(exynos5260_pin_banks0),
.eint_gpio_init = exynos_eint_gpio_init,
.eint_wkup_init = exynos_eint_wkup_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
}, {
/* pin-controller instance 1 data */
.pin_banks = exynos5260_pin_banks1,
.nr_banks = ARRAY_SIZE(exynos5260_pin_banks1),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
}, {
/* pin-controller instance 2 data */
.pin_banks = exynos5260_pin_banks2,
.nr_banks = ARRAY_SIZE(exynos5260_pin_banks2),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
},
};
@@ -842,30 +848,40 @@ static const struct samsung_pin_ctrl exynos5420_pin_ctrl[] __initconst = {
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks0),
.eint_gpio_init = exynos_eint_gpio_init,
.eint_wkup_init = exynos_eint_wkup_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos5420_retention_data,
}, {
/* pin-controller instance 1 data */
.pin_banks = exynos5420_pin_banks1,
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks1),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos5420_retention_data,
}, {
/* pin-controller instance 2 data */
.pin_banks = exynos5420_pin_banks2,
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks2),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos5420_retention_data,
}, {
/* pin-controller instance 3 data */
.pin_banks = exynos5420_pin_banks3,
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks3),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos5420_retention_data,
}, {
/* pin-controller instance 4 data */
.pin_banks = exynos5420_pin_banks4,
.nr_banks = ARRAY_SIZE(exynos5420_pin_banks4),
.eint_gpio_init = exynos_eint_gpio_init,
+ .suspend = exynos_pinctrl_suspend,
+ .resume = exynos_pinctrl_resume,
.retention_data = &exynos4_audio_retention_data,
},
};
--
2.14.1
^ permalink raw reply related
* [RFT v2 00/10] pinctrl: samsung: Remove ugly hack for sharing eint_wakeup_mask
From: Krzysztof Kozlowski @ 2018-07-23 17:52 UTC (permalink / raw)
To: linux-arm-kernel
Hi All,
Changes since v1
================
1. Add Tomasz's ack.
2. Reword description in patch 6/10.
Tests
=====
This is both request for comments and requests for tests. Only basic
tests were done, including suspend to RAM on Odroid U3 (Exynos4412)
with max7768 RTC wakeup. Please kindly test it with devices capable of
suspending and resuming. I am mostly thinking about S5Pv210-based (Aria),
Trats, Trats2 and TM2 (Exynos5433). Existing platforms should not be
broken however changing external interrupt wakeup mask was not done
on Exynos5433.
Description
===========
The Exynos/S5Pv210 machine suspend code needs to write the external
interrupt mask during suspend. The mask is controlled by pin controller
driver: the exynos_wkup_irq_set_wake() in IRQ chip for these wakeup
interrupts.
Therefore pinctrl driver code exposed an exynos_get_eint_wake_mask()
function which was later used as an extern in machine code.
This is quite ugly way of combining driver and machine code,
not portable triggering Sparse and GCC warnings.
This might break suspend capability S5Pv210 on with older DTBs (thus
breaks DTB compatibility), however:
1. just "might" because in case of using older DTB, the wakeup mask
will not be changed during suspend and default reset value (all
interrupts non-masked) should work,
2. mainline support for S5Pv210 with DTB is limited and suspend
to RAM already might be broken.
Dependencies
============
1. The first seven patches should be taken through one tree,
preferably samsung-pinctrl,
2. The DTS patch (7/10) for S5Pv210 should go into next cycle,
3. The remaining patches (8-10) should go after all previous,
so probably another release cycle.
Best regards,
Krzysztof
Krzysztof Kozlowski (10):
pinctrl: samsung: Define suspend and resume callbacks for all banks
and SoCs
pinctrl: samsung: Document suspend and resume members
pinctrl: samsung: Document hidden requirement about one external
wakeup
pinctrl: samsung: Add dedicated compatible for S5Pv210 wakeup
interrupts
ARM: exynos: Define EINT_WAKEUP_MASK registers for S5Pv210 and
Exynos5433
pinctrl: samsung: Write external wakeup interrupt mask
ARM: dts: s5pv210: Switch to S5Pv210 specific pinctrl wakeup
compatible
ARM: s5pv210: Remove legacy setting of external wakeup interrupts
ARM: exynos: Remove legacy setting of external wakeup interrupts
pinctrl: samsung: Remove legacy API for handling external wakeup
interrupts mask
.../bindings/pinctrl/samsung-pinctrl.txt | 11 ++-
arch/arm/boot/dts/s5pv210.dtsi | 2 +-
arch/arm/mach-exynos/common.h | 2 -
arch/arm/mach-exynos/suspend.c | 16 +++--
arch/arm/mach-s5pv210/common.h | 1 -
arch/arm/mach-s5pv210/pm.c | 16 +++--
drivers/pinctrl/samsung/pinctrl-exynos-arm.c | 16 +++++
drivers/pinctrl/samsung/pinctrl-exynos.c | 78 +++++++++++++++++++---
drivers/pinctrl/samsung/pinctrl-samsung.h | 11 +++
include/linux/soc/samsung/exynos-regs-pmu.h | 8 ++-
10 files changed, 136 insertions(+), 25 deletions(-)
--
2.14.1
^ permalink raw reply
* [RFT v2 00/10] pinctrl: samsung: Remove ugly hack for sharing eint_wakeup_mask
From: Krzysztof Kozlowski @ 2018-07-23 17:52 UTC (permalink / raw)
To: Tomasz Figa, Krzysztof Kozlowski, Sylwester Nawrocki,
Linus Walleij, Rob Herring, Mark Rutland, Kukjin Kim,
Russell King, Kyungmin Park, linux-arm-kernel, linux-samsung-soc,
linux-gpio, devicetree, linux-kernel
Cc: Paweł Chmiel, Sylwester Nawrocki, Chanwoo Choi, Alim Akhtar,
Pankaj Dubey
Hi All,
Changes since v1
================
1. Add Tomasz's ack.
2. Reword description in patch 6/10.
Tests
=====
This is both request for comments and requests for tests. Only basic
tests were done, including suspend to RAM on Odroid U3 (Exynos4412)
with max7768 RTC wakeup. Please kindly test it with devices capable of
suspending and resuming. I am mostly thinking about S5Pv210-based (Aria),
Trats, Trats2 and TM2 (Exynos5433). Existing platforms should not be
broken however changing external interrupt wakeup mask was not done
on Exynos5433.
Description
===========
The Exynos/S5Pv210 machine suspend code needs to write the external
interrupt mask during suspend. The mask is controlled by pin controller
driver: the exynos_wkup_irq_set_wake() in IRQ chip for these wakeup
interrupts.
Therefore pinctrl driver code exposed an exynos_get_eint_wake_mask()
function which was later used as an extern in machine code.
This is quite ugly way of combining driver and machine code,
not portable triggering Sparse and GCC warnings.
This might break suspend capability S5Pv210 on with older DTBs (thus
breaks DTB compatibility), however:
1. just "might" because in case of using older DTB, the wakeup mask
will not be changed during suspend and default reset value (all
interrupts non-masked) should work,
2. mainline support for S5Pv210 with DTB is limited and suspend
to RAM already might be broken.
Dependencies
============
1. The first seven patches should be taken through one tree,
preferably samsung-pinctrl,
2. The DTS patch (7/10) for S5Pv210 should go into next cycle,
3. The remaining patches (8-10) should go after all previous,
so probably another release cycle.
Best regards,
Krzysztof
Krzysztof Kozlowski (10):
pinctrl: samsung: Define suspend and resume callbacks for all banks
and SoCs
pinctrl: samsung: Document suspend and resume members
pinctrl: samsung: Document hidden requirement about one external
wakeup
pinctrl: samsung: Add dedicated compatible for S5Pv210 wakeup
interrupts
ARM: exynos: Define EINT_WAKEUP_MASK registers for S5Pv210 and
Exynos5433
pinctrl: samsung: Write external wakeup interrupt mask
ARM: dts: s5pv210: Switch to S5Pv210 specific pinctrl wakeup
compatible
ARM: s5pv210: Remove legacy setting of external wakeup interrupts
ARM: exynos: Remove legacy setting of external wakeup interrupts
pinctrl: samsung: Remove legacy API for handling external wakeup
interrupts mask
.../bindings/pinctrl/samsung-pinctrl.txt | 11 ++-
arch/arm/boot/dts/s5pv210.dtsi | 2 +-
arch/arm/mach-exynos/common.h | 2 -
arch/arm/mach-exynos/suspend.c | 16 +++--
arch/arm/mach-s5pv210/common.h | 1 -
arch/arm/mach-s5pv210/pm.c | 16 +++--
drivers/pinctrl/samsung/pinctrl-exynos-arm.c | 16 +++++
drivers/pinctrl/samsung/pinctrl-exynos.c | 78 +++++++++++++++++++---
drivers/pinctrl/samsung/pinctrl-samsung.h | 11 +++
include/linux/soc/samsung/exynos-regs-pmu.h | 8 ++-
10 files changed, 136 insertions(+), 25 deletions(-)
--
2.14.1
^ permalink raw reply
* Re: [PATCH v2 00/21] dom0less step1: boot multiple domains from device tree
From: Stefano Stabellini @ 2018-07-23 17:52 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, andrii_anisov, andrew.cooper3, xen-devel,
jbeulich, nd
In-Reply-To: <5411a218-46e1-5f16-64db-eae36476bb81@arm.com>
On Mon, 23 Jul 2018, Julien Grall wrote:
> On 18/07/18 18:48, Stefano Stabellini wrote:
> > On Wed, 18 Jul 2018, Julien Grall wrote:
> > > Hi Stefano,
> > >
> > > On 13/07/18 21:54, Stefano Stabellini wrote:
> > > > On Thu, 12 Jul 2018, Julien Grall wrote:
> > > > > Hi,
> > > > >
> > > > > Would it be possible to provide a branch with the patch applied? It
> > > > > would
> > > > > be
> > > > > nice to have that for every version, so I can easily know on which
> > > > > version
> > > > > of
> > > > > you are based and avoid spending time trying to apply it :).
> > > >
> > > > Makes sense, I'll do from next time
> > >
> > > Could you provide one from this version? So I can review some of your
> > > patches
> > > more easily.
> >
> > http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git
> > dom0less-v2
>
> Thanks. I will have a look at the vpl011 patches. I think the rest is either
> reviewed or will require changes based on other comments.
>
> Let me know if I missed anything.
No, I think it is OK. Thank you.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* Re: [Qemu-devel] [PATCH] ppc/xics: fix ICP reset path
From: Greg Kurz @ 2018-07-23 17:51 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-devel, qemu-ppc, David Gibson, Bharata B Rao,
Satheesh Rajendran
In-Reply-To: <821367d0-c3ef-43bd-071c-b2ff50dae7bd@kaod.org>
On Mon, 23 Jul 2018 13:36:36 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> On 07/12/2018 12:01 PM, Greg Kurz wrote:
> > Recent cleanup in commit a028dd423ee6 dropped the ICPStateClass::reset
> > handler. It is now up to child ICP classes to call the DeviceClass::reset
> > handler of the parent class, thanks to device_class_set_parent_reset().
> > This is a better object programming pattern,
>
> I think that the device_class_set_parent* routines just obfuscate a little
> more the object programming approach of QEMU.
>
Really ?
> > but unfortunately it causes QEMU to crash during CPU hotplug:
>
> I guess I did not try that :/ Would it be complex to add a spapr unit test
> for CPU hotplug ? It would be useful.
>
Actually, there's one in tests/cpu-plug-test.c but it runs with
accel=qtest, and thus it doesn't exercise the KVM ICP code.
I've patched it to use accel=kvm:tcg on pseries: it would have caught
the SEGV indeed. There's another problem though: QEMU sometimes stays
stuck in kvm_vcpu_ioctl() in 'disk sleep' state, causing the test
to hang until someone kills QEMU with SIGKILL.
Not sure yet what's happening...
> > (qemu) device_add host-spapr-cpu-core,id=core1,core-id=1
> > Segmentation fault (core dumped)
> >
> > When the hotplug path tries to reset the ICP device, we end up calling:
> >
> > static void icp_kvm_reset(DeviceState *dev)
> > {
> > ICPStateClass *icpc = ICP_GET_CLASS(dev);
> >
> > icpc->parent_reset(dev);
> >
> > but icpc->parent_reset is NULL... This happens because icp_kvm_class_init()
> > calls:
> >
> > device_class_set_parent_reset(dc, icp_kvm_reset,
> > &icpc->parent_reset);
> >
> > but dc->reset, ie, DeviceClass::reset for the TYPE_ICP type, is
> > itself NULL.>
> > This patch hence sets DeviceClass::reset for the TYPE_ICP type to
> > point to icp_reset(). It then registers a reset handler that calls
> > DeviceClass::reset. If the ICP subtype has configured its own reset
> > handler with device_class_set_parent_reset(), this ensures it will
> > be called first and it can then call ICPStateClass::parent_reset
> > safely. This fixes the reset path for the TYPE_KVM_ICP type, which
> > is the only subtype that defines its own reset function.
>
> So, now, isn't ICP reset called twice on KVM ?
>
No. We have one path for cold plugged CPUS:
#0 icp_reset (dev=0x113db100) at /home/greg/Work/qemu/qemu-spapr/hw/intc/xics.c:296
#1 0x0000000010146d0c in icp_kvm_reset (dev=0x113db100) at /home/greg/Work/qemu/qemu-spapr/hw/intc/xics_kvm.c:121
#2 0x00000000101434b0 in icp_reset_handler (dev=0x113db100) at /home/greg/Work/qemu/qemu-spapr/hw/intc/xics.c:310
#3 0x00000000104c3744 in qemu_devices_reset () at /home/greg/Work/qemu/qemu-spapr/hw/core/reset.c:69
#4 0x00000000101d61c8 in spapr_machine_reset () at /home/greg/Work/qemu/qemu-spapr/hw/ppc/spapr.c:1639
#5 0x00000000103de654 in qemu_system_reset (reason=SHUTDOWN_CAUSE_NONE) at /home/greg/Work/qemu/qemu-spapr/vl.c:1645
...
and one path for hot plugged CPUs:
#0 icp_reset (dev=0x11268e40) at /home/greg/Work/qemu/qemu-spapr/hw/intc/xics.c:296
#1 0x0000000010146d0c in icp_kvm_reset (dev=0x11268e40) at /home/greg/Work/qemu/qemu-spapr/hw/intc/xics_kvm.c:121
#2 0x00000000104be300 in device_reset (dev=0x11268e40) at /home/greg/Work/qemu/qemu-spapr/hw/core/qdev.c:1082
#3 0x00000000104bd62c in device_set_realized (obj=0x11268e40, value=true, errp=0x7fffffffcfa8) at /home/greg/Work/qemu/qemu-spapr/hw/core/qdev.c:867
...
> Anyway, we can sort that out if necessary.
>
> Thanks,
>
> C.
>
> > Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com>
> > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > Fixes: a028dd423ee6dfd091a8c63028240832bf10f671
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > hw/intc/xics.c | 14 +++++++++++---
> > 1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index b9f1a3c97214..c90c893228dc 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -291,7 +291,7 @@ static const VMStateDescription vmstate_icp_server = {
> > },
> > };
> >
> > -static void icp_reset(void *dev)
> > +static void icp_reset(DeviceState *dev)
> > {
> > ICPState *icp = ICP(dev);
> >
> > @@ -303,6 +303,13 @@ static void icp_reset(void *dev)
> > qemu_set_irq(icp->output, 0);
> > }
> >
> > +static void icp_reset_handler(void *dev)
> > +{
> > + DeviceClass *dc = DEVICE_GET_CLASS(dev);
> > +
> > + dc->reset(dev);
> > +}
> > +
> > static void icp_realize(DeviceState *dev, Error **errp)
> > {
> > ICPState *icp = ICP(dev);
> > @@ -345,7 +352,7 @@ static void icp_realize(DeviceState *dev, Error **errp)
> > return;
> > }
> >
> > - qemu_register_reset(icp_reset, dev);
> > + qemu_register_reset(icp_reset_handler, dev);
> > vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> > }
> >
> > @@ -354,7 +361,7 @@ static void icp_unrealize(DeviceState *dev, Error **errp)
> > ICPState *icp = ICP(dev);
> >
> > vmstate_unregister(NULL, &vmstate_icp_server, icp);
> > - qemu_unregister_reset(icp_reset, dev);
> > + qemu_unregister_reset(icp_reset_handler, dev);
> > }
> >
> > static void icp_class_init(ObjectClass *klass, void *data)
> > @@ -363,6 +370,7 @@ static void icp_class_init(ObjectClass *klass, void *data)
> >
> > dc->realize = icp_realize;
> > dc->unrealize = icp_unrealize;
> > + dc->reset = icp_reset;
> > }
> >
> > static const TypeInfo icp_info = {
> >
>
^ permalink raw reply
* Re: [PATCH v07 2/9] hotplug/cpu: Add operation queuing function
From: Nathan Fontenot @ 2018-07-23 17:51 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev; +Cc: John Allen, Tyrel Datwyler, Thomas Falcon
In-Reply-To: <a2c023e9-6997-d1a3-0110-fa8c788e64ee@linux.vnet.ibm.com>
On 07/13/2018 03:18 PM, Michael Bringmann wrote:
> migration/dlpar: This patch adds function dlpar_queue_action()
> which will queued up information about a CPU/Memory 'readd'
> operation according to resource type, action code, and DRC index.
> At a subsequent point, the list of operations can be run/played
> in series. Examples of such oprations include 'readd' of CPU
> and Memory blocks identified as having changed their associativity
> during an LPAR migration event. >
> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
> ---
> Changes in patch:
> -- Correct drc_index before adding to pseries_hp_errorlog struct
> -- Correct text of notice
> -- Revise queuing model to save up all of the DLPAR actions for
> later execution.
> -- Restore list init statement missing from patch
> -- Move call to apply queued operations into 'mobility.c'
> -- Compress some code
> -- Rename some of queueing function APIs
> -- Revise implementation to push execution of queued operations
> to a workqueue task.
> -- Cleanup reference to outdated queuing operation.
> ---
> arch/powerpc/include/asm/rtas.h | 2 +
> arch/powerpc/platforms/pseries/dlpar.c | 61 +++++++++++++++++++++++++++++
> arch/powerpc/platforms/pseries/mobility.c | 4 ++
> arch/powerpc/platforms/pseries/pseries.h | 2 +
> 4 files changed, 69 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index 71e393c..4f601c7 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -310,12 +310,14 @@ struct pseries_hp_errorlog {
> struct { __be32 count, index; } ic;
> char drc_name[1];
> } _drc_u;
> + struct list_head list;
> };
>
> #define PSERIES_HP_ELOG_RESOURCE_CPU 1
> #define PSERIES_HP_ELOG_RESOURCE_MEM 2
> #define PSERIES_HP_ELOG_RESOURCE_SLOT 3
> #define PSERIES_HP_ELOG_RESOURCE_PHB 4
> +#define PSERIES_HP_ELOG_RESOURCE_PMT 5
>
> #define PSERIES_HP_ELOG_ACTION_ADD 1
> #define PSERIES_HP_ELOG_ACTION_REMOVE 2
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index a0b20c0..7264b8e 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -25,6 +25,7 @@
> #include <asm/prom.h>
> #include <asm/machdep.h>
> #include <linux/uaccess.h>
> +#include <linux/delay.h>
> #include <asm/rtas.h>
>
> static struct workqueue_struct *pseries_hp_wq;
> @@ -329,6 +330,8 @@ int dlpar_release_drc(u32 drc_index)
> return 0;
> }
>
> +static int dlpar_pmt(struct pseries_hp_errorlog *work);
> +
> static int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
> {
> int rc;
> @@ -357,6 +360,9 @@ static int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
> case PSERIES_HP_ELOG_RESOURCE_CPU:
> rc = dlpar_cpu(hp_elog);
> break;
> + case PSERIES_HP_ELOG_RESOURCE_PMT:
> + rc = dlpar_pmt(hp_elog);
> + break;
> default:
> pr_warn_ratelimited("Invalid resource (%d) specified\n",
> hp_elog->resource);
> @@ -407,6 +413,61 @@ void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog,
> }
> }
>
> +LIST_HEAD(dlpar_delayed_list);
> +
> +int dlpar_queue_action(int resource, int action, u32 drc_index)
> +{
> + struct pseries_hp_errorlog *hp_errlog;
> +
> + hp_errlog = kmalloc(sizeof(struct pseries_hp_errorlog), GFP_KERNEL);
> + if (!hp_errlog)
> + return -ENOMEM;
> +
> + hp_errlog->resource = resource;
> + hp_errlog->action = action;
> + hp_errlog->id_type = PSERIES_HP_ELOG_ID_DRC_INDEX;
> + hp_errlog->_drc_u.drc_index = cpu_to_be32(drc_index);
> +
> + list_add_tail(&hp_errlog->list, &dlpar_delayed_list);
> +
> + return 0;
> +}
> +
> +static int dlpar_pmt(struct pseries_hp_errorlog *work)
> +{
> + struct list_head *pos, *q;
> +
> + ssleep(15);
> +
> + list_for_each_safe(pos, q, &dlpar_delayed_list) {
> + struct pseries_hp_errorlog *tmp;
> +
> + tmp = list_entry(pos, struct pseries_hp_errorlog, list);
> + handle_dlpar_errorlog(tmp);
> +
> + list_del(pos);
> + kfree(tmp);
> +
> + ssleep(10);
> + }
> +
> + return 0;
> +}
> +
> +int dlpar_queued_actions_run(void)
> +{
> + if (!list_empty(&dlpar_delayed_list)) {
> + struct pseries_hp_errorlog hp_errlog;
> +
> + hp_errlog.resource = PSERIES_HP_ELOG_RESOURCE_PMT;
> + hp_errlog.action = 0;
> + hp_errlog.id_type = 0;
> +
> + queue_hotplug_event(&hp_errlog, 0, 0); > + }
> + return 0;
> +}
I'm a bit confused by this. Is there a reason this needs to queue a
hotplug event instead of just walking the list as is done in dlpar_pmt?
-Nathan
> +
> static int dlpar_parse_resource(char **cmd, struct pseries_hp_errorlog *hp_elog)
> {
> char *arg;
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index f6364d9..d0d1cae 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -378,6 +378,10 @@ static ssize_t migration_store(struct class *class,
> return rc;
>
> post_mobility_fixup();
> +
> + /* Apply any necessary changes identified during fixup */
> + dlpar_queued_actions_run();
> +
> return count;
> }
>
> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
> index 60db2ee..72ca996 100644
> --- a/arch/powerpc/platforms/pseries/pseries.h
> +++ b/arch/powerpc/platforms/pseries/pseries.h
> @@ -61,6 +61,8 @@ extern struct device_node *dlpar_configure_connector(__be32,
>
> void queue_hotplug_event(struct pseries_hp_errorlog *hp_errlog,
> struct completion *hotplug_done, int *rc);
> +int dlpar_queue_action(int resource, int action, u32 drc_index);
> +int dlpar_queued_actions_run(void);
> #ifdef CONFIG_MEMORY_HOTPLUG
> int dlpar_memory(struct pseries_hp_errorlog *hp_elog);
> #else
>
^ permalink raw reply
* Re: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields
From: Rob Herring @ 2018-07-23 17:51 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Andrew Jeffery, Mark Rutland, devicetree, Greg Kroah-Hartman,
Eugene.Cho, a.amelkin, linux-kernel@vger.kernel.org, Joel Stanley,
stewart, OpenBMC Maillist,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <c69d0e7c3a95d83da87b566a62619462671a233b.camel@kernel.crashing.org>
On Wed, Jul 18, 2018 at 5:58 PM Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Wed, 2018-07-18 at 13:50 -0600, Rob Herring wrote:
> >
> > > So Rob, I think that's precisely where the disconnect is.
> > >
> > > I think we all (well hopefully) agree that those few tunables don't fit
> > > in any existing subystem and aren't likely to ever do (famous last
> > > words...).
> > >
> > > Where we disagree is we want to make this parametrized via the DT, and
> > > you want us to hard wire the list in some kind of SoC driver for a
> > > given SoC family/version.
> > >
> > > The reason I think hard wiring the list in the driver is not a great
> > > solution is that that list in itself is prone to variations, possibly
> > > fairly often, between boards, vendors, versions of boards, etc...
> >
> > Can we separate the list of what's enabled from the details of the
> > registers? Even if we put all this into DT, we may still want to have
> > some separation for dts file structure. Some portion of this has to be
> > SoC specific because you are simply exposing SoC registers.
>
> Not sure I understand what you mean by "what's enabled".
For example with VGA muxing, you can separate the details of the register field location and whether you want that tunable or not for a given platform.
> The goal here is to expose register "fields" (nor raw values, some
> registers need locking for RMW etc... the kernel would handle that via
> syscon locking I expect), so basically named "tunables" to userspace.
>
> Userspace is the one that knows the values for a given system.
>
> > > We can't know for sure every SoC tunable (out of the gazillions in
> > > those chips) are going to be needed for a given system. We know which
> > > ones we do use for ours, and that's a couple of handfuls, but it could
> > > be that Dell need a slightly different set, and so might Yadro, or so
> > > might our next board revision for that matter.
> >
> > That's very hand wavy. Do you have some concrete examples (i.e. dts
> > files) showing the differences.
>
> We could list what we have on the pile for some of our IBM systems
> today, but we would need Dell and Yadro to chime in with what they
> need.
>
> I still, from experience with that stuff and gut feeling, am pretty
> sure it's going to be a moving list, and updating the kernel driver
> constantly isn't going to fly.
>
> > One problem I'm having with this is you are still going to need per
> > board specifics in userspace.
>
> Yes. Userspace is ultimately the one that knows what needs to be done
> on a given machine.
>
> > You may be moving some of details out,
> > but moving to DT is not going to completely eliminate that.
>
> We aren't trying to either. We are trying to make sure we don't need to
> change the *kernel* all the time, in part bcs we are pushing hard for
> OpenBMC vendors to use upstream with, if possible, no vendor changes.
>
> So userspace has to know the board specific tunables anyways. Today in
> many systems, it does that by whacking /dev/mem. I guess you can
> understand why that is bad :-)
>
> > I agree
> > that not using /dev/mem is a good thing, but there are several ways
> > you could do that independent of any DT binding.
>
> Such as ? The only other option is to have one or more kernel drivers
> that have built-in the precise and complete list of those tunables that
> need to be exposed.
UIO is one way. That at least would restrict things to a specific address range(s) compared to /dev/mem, but would still possibly leave other registers exposed.
Another option would be with your proposed driver to provide a userspace interface to register the register fields you need. Not sure if allowing userspace to be able to specify that is a security concern (though allowing a DT update would be the same).
> It's not impossible, but I worry that it's not going to scale terribly
> well, and that vendors will constantly "fork" that driver to add
> different things to that list.
>
> I might be wrong here, so I'd like for Eugene (Dell) and Alexander
> (Yadro) to chime in, but experience with BMCs has shown that we
> regularily , as we add a feature or rewrite something, need to find
> another new magic SoC tunable the HW manufacturer hid somewhere that
> will make our stuff work.
>
> > > Now, updating the device-tree in the board flash with whatever vendor
> > > specific information is needed is a LOT easier than getting the kernel
> > > driver constantly updated. The device-tree after all is there to
> > > reflect among other things system specific ways in which the SoC is
> > > wired and configured. This is rather close...
> >
> > Sadly, updating my kernel is easier than updating my PC firmware
> > (though packaged firmware on my current laptop changes that). I can
> > assure you that ARM boards are generally much worse in that regard.
> > BTW, you may want to pay attention to EBBR[1][2]. Not sure how much
> > you care for BMCs, but there may be some interest.
>
> You are conflating your host kernel and your BMC here. The BMC kernel
> is part of the "firmware", as is its DTS and the BMC userspace.
>
> (Again this isn't the host DTS, this is the BMC DTS).
Yes, I get that. My point was just about what's easier to do actual updates for. But mainline vs. forks of the kernel is a different thing.
> They get all updated together. My point isn't about the ease or
> difficulty for a *user* to udpate their BMC, in that case the solutions
> above are equivalent.
>
> The point is from a system vendor perspective. A system vendor using
> OpenBMC will *customize* their BMC build in various ways. Typically
> they *will* have a custom DT since this is what represents their
> specific system and they will have some specific userspace bits.
>
> However, we are trying very hard for them *not* to fork the kernel, and
> if possible move OpenBMC towards a fully upstream kernel (still working
> on getting all the SoC drivers cleaned up and pushed up but it's moving
> in the right direction).
>
> So from a vendor perspective, such as us IBM, we *alread* have custom
> DTs and customized userspace, that's part of the normal flow of
> deploying a BMC.
>
> However, we are trying *NOT* to have a custom kernel (and we don't, at
> the moment there is an OpenBMC kernel accross vendors, though it's not
> *yet* fully upstream).
So if you are only trying to avoid kernel changes, then what is the advantage of putting this in DT and passing it thru to userspace. Why not just put all
>
> So if the solution proposed is prone to requiring frequent changes to a
> kernel driver, that solution makes the above a lot more difficult and
> will encourage vendors to keep forking kernels.
>
> This is what we are trying to avoid.
>
> Cheers,
> Ben.
>
> > Rob
> >
> > [1] https://github.com/ARM-software/ebbr
> > [2] https://lists.linaro.org/pipermail/boot-architecture/
^ permalink raw reply
* Re: rte_mbuf library likely()/unlikely()
From: Honnappa Nagarahalli @ 2018-07-23 17:51 UTC (permalink / raw)
To: Morten Brørup, Olivier Matz; +Cc: dev@dpdk.org
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35B421EE@smartserver.smartshare.dk>
Do you see any performance improvements with these changes?
-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of Morten Brørup
Sent: Monday, July 23, 2018 8:54 AM
To: Olivier Matz <olivier.matz@6wind.com>
Cc: dev@dpdk.org
Subject: [dpdk-dev] rte_mbuf library likely()/unlikely()
Hi Olivier,
I noticed that __rte_pktmbuf_read() could do with an unlikely(), so I went through the entire library. Here are my suggested modifications.
diff -bu rte_mbuf.c.orig rte_mbuf.c
--- rte_mbuf.c.orig 2018-07-23 15:13:22.000000000 +0200
+++ rte_mbuf.c 2018-07-23 15:32:53.000000000 +0200
@@ -173,19 +173,19 @@
{
unsigned int nb_segs, pkt_len;
- if (m == NULL)
+ if (unlikely(m == NULL))
rte_panic("mbuf is NULL\n");
/* generic checks */
- if (m->pool == NULL)
+ if (unlikely(m->pool == NULL))
rte_panic("bad mbuf pool\n");
- if (m->buf_iova == 0)
+ if (unlikely(m->buf_iova == 0))
rte_panic("bad IO addr\n");
- if (m->buf_addr == NULL)
+ if (unlikely(m->buf_addr == NULL))
rte_panic("bad virt addr\n");
uint16_t cnt = rte_mbuf_refcnt_read(m);
- if ((cnt == 0) || (cnt == UINT16_MAX))
+ if (unlikely((cnt == 0) || (cnt == UINT16_MAX)))
rte_panic("bad ref cnt\n");
/* nothing to check for sub-segments */
@@ -193,7 +193,7 @@
return;
/* data_len is supposed to be not more than pkt_len */
- if (m->data_len > m->pkt_len)
+ if (unlikely(m->data_len > m->pkt_len))
rte_panic("bad data_len\n");
nb_segs = m->nb_segs;
@@ -204,9 +204,9 @@
pkt_len -= m->data_len;
} while ((m = m->next) != NULL);
- if (nb_segs)
+ if (unlikely(nb_segs))
rte_panic("bad nb_segs\n");
- if (pkt_len)
+ if (unlikely(pkt_len))
rte_panic("bad pkt_len\n");
}
@@ -249,7 +249,7 @@
const struct rte_mbuf *seg = m;
uint32_t buf_off = 0, copy_len;
- if (off + len > rte_pktmbuf_pkt_len(m))
+ if (unlikely(off + len > rte_pktmbuf_pkt_len(m)))
return NULL;
while (off >= rte_pktmbuf_data_len(seg)) {
@@ -257,7 +257,7 @@
seg = seg->next;
}
- if (off + len <= rte_pktmbuf_data_len(seg))
+ if (likely(off + len <= rte_pktmbuf_data_len(seg)))
return rte_pktmbuf_mtod_offset(seg, char *, off);
/* rare case: header is split among several segments */
@@ -344,7 +344,7 @@
unsigned int i;
int ret;
- if (buflen == 0)
+ if (unlikely(buflen == 0))
return -1;
buf[0] = '\0';
@@ -355,9 +355,9 @@
if (name == NULL)
name = rx_flags[i].default_name;
ret = snprintf(buf, buflen, "%s ", name);
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
@@ -440,7 +440,7 @@
unsigned int i;
int ret;
- if (buflen == 0)
+ if (unlikely(buflen == 0))
return -1;
buf[0] = '\0';
@@ -451,9 +451,9 @@
if (name == NULL)
name = tx_flags[i].default_name;
ret = snprintf(buf, buflen, "%s ", name);
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
diff -bu rte_mbuf.h.orig rte_mbuf.h
--- rte_mbuf.h.orig 2018-07-23 15:13:26.000000000 +0200
+++ rte_mbuf.h 2018-07-23 15:24:25.000000000 +0200
@@ -1007,7 +1007,7 @@
{
struct rte_mbuf *m;
- if (rte_mempool_get(mp, (void **)&m) < 0)
+ if (unlikely(rte_mempool_get(mp, (void **)&m) < 0))
return NULL;
MBUF_RAW_ALLOC_CHECK(m);
return m;
@@ -1268,7 +1268,7 @@
static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
{
struct rte_mbuf *m;
- if ((m = rte_mbuf_raw_alloc(mp)) != NULL)
+ if (likely((m = rte_mbuf_raw_alloc(mp)) != NULL))
rte_pktmbuf_reset(m);
return m;
}
@@ -1696,7 +1696,7 @@
{
struct rte_mbuf *m_next;
- if (m != NULL)
+ if (likely(m != NULL))
__rte_mbuf_sanity_check(m, 1);
while (m != NULL) {
@@ -2099,7 +2099,7 @@
struct rte_mbuf *cur_tail;
/* Check for number-of-segments-overflow */
- if (head->nb_segs + tail->nb_segs > RTE_MBUF_MAX_NB_SEGS)
+ if (unlikely(head->nb_segs + tail->nb_segs > RTE_MBUF_MAX_NB_SEGS))
return -EOVERFLOW;
/* Chain 'tail' onto the old tail */
@@ -2147,28 +2147,28 @@
m->outer_l3_len;
/* Headers are fragmented */
- if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
+ if (unlikely(rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len))
return -ENOTSUP;
/* IP checksum can be counted only for IPv4 packet */
- if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
+ if (unlikely((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6)))
return -EINVAL;
/* IP type not set when required */
if (ol_flags & (PKT_TX_L4_MASK | PKT_TX_TCP_SEG))
- if (!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6)))
+ if (unlikely(!(ol_flags & (PKT_TX_IPV4 | PKT_TX_IPV6))))
return -EINVAL;
/* Check requirements for TSO packet */
if (ol_flags & PKT_TX_TCP_SEG)
- if ((m->tso_segsz == 0) ||
+ if (unlikely((m->tso_segsz == 0) ||
((ol_flags & PKT_TX_IPV4) &&
- !(ol_flags & PKT_TX_IP_CKSUM)))
+ !(ol_flags & PKT_TX_IP_CKSUM))))
return -EINVAL;
/* PKT_TX_OUTER_IP_CKSUM set for non outer IPv4 packet. */
- if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) &&
- !(ol_flags & PKT_TX_OUTER_IPV4))
+ if (unlikely((ol_flags & PKT_TX_OUTER_IP_CKSUM) &&
+ !(ol_flags & PKT_TX_OUTER_IPV4)))
return -EINVAL;
return 0;
diff -bu rte_mbuf_ptype.c.orig rte_mbuf_ptype.c
--- rte_mbuf_ptype.c.orig 2018-07-23 15:45:49.000000000 +0200
+++ rte_mbuf_ptype.c 2018-07-23 15:44:59.000000000 +0200
@@ -118,15 +118,15 @@
{
int ret;
- if (buflen == 0)
+ if (unlikely(buflen == 0))
return -1;
buf[0] = '\0';
if ((ptype & RTE_PTYPE_ALL_MASK) == RTE_PTYPE_UNKNOWN) {
ret = snprintf(buf, buflen, "UNKNOWN");
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
return 0;
}
@@ -134,9 +134,9 @@
if ((ptype & RTE_PTYPE_L2_MASK) != 0) {
ret = snprintf(buf, buflen, "%s ",
rte_get_ptype_l2_name(ptype));
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
@@ -144,9 +144,9 @@
if ((ptype & RTE_PTYPE_L3_MASK) != 0) {
ret = snprintf(buf, buflen, "%s ",
rte_get_ptype_l3_name(ptype));
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
@@ -154,9 +154,9 @@
if ((ptype & RTE_PTYPE_L4_MASK) != 0) {
ret = snprintf(buf, buflen, "%s ",
rte_get_ptype_l4_name(ptype));
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
@@ -164,9 +164,9 @@
if ((ptype & RTE_PTYPE_TUNNEL_MASK) != 0) {
ret = snprintf(buf, buflen, "%s ",
rte_get_ptype_tunnel_name(ptype));
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
@@ -174,9 +174,9 @@
if ((ptype & RTE_PTYPE_INNER_L2_MASK) != 0) {
ret = snprintf(buf, buflen, "%s ",
rte_get_ptype_inner_l2_name(ptype));
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
@@ -184,9 +184,9 @@
if ((ptype & RTE_PTYPE_INNER_L3_MASK) != 0) {
ret = snprintf(buf, buflen, "%s ",
rte_get_ptype_inner_l3_name(ptype));
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
@@ -194,9 +194,9 @@
if ((ptype & RTE_PTYPE_INNER_L4_MASK) != 0) {
ret = snprintf(buf, buflen, "%s ",
rte_get_ptype_inner_l4_name(ptype));
- if (ret < 0)
+ if (unlikely(ret < 0))
return -1;
- if ((size_t)ret >= buflen)
+ if (unlikely((size_t)ret >= buflen))
return -1;
buf += ret;
buflen -= ret;
Med venlig hilsen / kind regards
Morten Brørup
CTO
SmartShare Systems A/S
Tonsbakken 16-18
DK-2740 Skovlunde
Denmark
Office +45 70 20 00 93
Direct +45 89 93 50 22
Mobile +45 25 40 82 12
mb@smartsharesystems.com <mailto:mb@smartsharesystems.com>
www.smartsharesystems.com <https://www.smartsharesystems.com/>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply
* Re: [PATCH 1/1] i2c: rcar: handle RXDMA HW behaviour on Gen3
From: Wolfram Sang @ 2018-07-23 17:50 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Wolfram Sang, Linux I2C, Linux-Renesas, Yoshihiro Shimoda
In-Reply-To: <20180629075404.ivnaauv2hdir32ig@ninjato>
[-- Attachment #1: Type: text/plain, Size: 943 bytes --]
On Fri, Jun 29, 2018 at 09:54:04AM +0200, Wolfram Sang wrote:
>
> > > + /* Gen3 needs a reset before allowing RXDMA once */
> > > + if (priv->devtype == I2C_RCAR_GEN3) {
> >
> > So on R-Car Gen3 the device is always reset, even if no reads will be done?
> > Or is that something you cannot check for at this point?
>
> You got a point. I copied the behaviour from the BSP to do this as early
> as possible. Still, it might be worthwhile to merge this into
> rcar_i2c_request_dma() and move the call to it to the front. Will check.
I will apply this patch as is because it is easier to backport to stable
this way.
You are still right, we can do better and not issue a reset any time. To
get there, though, I want two kinds of refactoring beforehand. And I
think it is best to do this on top of this patch and refactor it along.
BSP backporters can then decide if they want the refactored version or
not.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] drm/fb_helper: Add drm_fb_helper_output_poll_changed_with_rpm()
From: Lyude Paul @ 2018-07-23 17:50 UTC (permalink / raw)
To: Lukas Wunner
Cc: nouveau, Gustavo Padovan, Maarten Lankhorst, Sean Paul,
David Airlie, Ben Skeggs, Daniel Vetter, Ville Syrjälä,
dri-devel, linux-kernel
In-Reply-To: <20180721093955.GA32572@wunner.de>
On Sat, 2018-07-21 at 11:39 +0200, Lukas Wunner wrote:
> On Thu, Jul 19, 2018 at 08:08:15PM -0400, Lyude Paul wrote:
> > On Thu, 2018-07-19 at 09:49 +0200, Lukas Wunner wrote:
> > > On Wed, Jul 18, 2018 at 04:56:39PM -0400, Lyude Paul wrote:
> > > > When DP MST hubs get confused, they can occasionally stop responding
> > > > for
> > > > a good bit of time up until the point where the DRM driver manages to
> > > > do the right DPCD accesses to get it to start responding again. In a
> > > > worst case scenario however, this process can take upwards of 10+
> > > > seconds.
> > > >
> > > > Currently we use the default output_poll_changed handler
> > > > drm_fb_helper_output_poll_changed() to handle output polling, which
> > > > doesn't happen to grab any power references on the device when
> > > > polling.
> > > > If we're unlucky enough to have a hub (such as Lenovo's infamous
> > > > laptop
> > > > docks for the P5x/P7x series) that's easily startled and confused,
> > > > this
> > > > can lead to a pretty nasty deadlock situation that looks like this:
> > > >
> > > > - Hotplug event from hub happens, we enter
> > > > drm_fb_helper_output_poll_changed() and start communicating with the
> > > > hub
> > > > - While we're in drm_fb_helper_output_poll_changed() and attempting to
> > > > communicate with the hub, we end up confusing it and cause it to
> > > > stop
> > > > responding for at least 10 seconds
> > > > - After 5 seconds of being in drm_fb_helper_output_poll_changed(), the
> > > > pm core attempts to put the GPU into autosuspend, which ends up
> > > > calling drm_kms_helper_poll_disable()
> > > > - While the runtime PM core is waiting in
> > > > drm_kms_helper_poll_disable()
> > > > for the output poll to finish, we end up finally detecting an MST
> > > > display
> > > > - We notice the new display and tries to enable it, which triggers
> > > > an atomic commit which triggers a call to pm_runtime_get_sync()
> > > > - the output poll thread deadlocks the pm core waiting for the pm core
> > > > to finish the autosuspend request while the pm core waits for the
> > > > output poll thread to finish
> > >
> > > The correct fix is to call pm_runtime_get_sync() *conditionally* in
> > > the atomic commit which enables the display, using the same conditional
> > > as d61a5c106351, i.e. if (!drm_kms_helper_is_poll_worker()).
>
> First of all, I was mistaken when I wrote above that a check for
> !drm_kms_helper_is_poll_worker() would solve the problem. Sorry!
> It doesn't because the call to pm_runtime_get_sync() is not happening
> in output_poll_execute() but in drm_dp_mst_link_probe_work().
>
> Looking once more at the three stack traces you've provided, we've got:
> - output_poll_execute() stuck waiting for fb_helper->lock
> which is held by drm_dp_mst_link_probe_work()
> - rpm_suspend() stuck waiting for output_poll_execute() to finish
> - drm_dp_mst_link_probe_work() stuck waiting in rpm_resume()
>
> For the moment we can ignore the first task, i.e. output_poll_execute(),
> and focus on the latter two.
>
> As said I'm unfamiliar with MST but browsing through drm_dp_mst_topology.c
> I notice that drm_dp_mst_link_probe_work() is the ->work element in
> drm_dp_mst_topology_mgr() and is queued on HPD. I further notice that
> the work item is flushed on ->runtime_suspend:
>
> nouveau_pmops_runtime_suspend()
> nouveau_do_suspend()
> nouveau_display_suspend()
> nouveau_display_fini()
> disp->fini() == nv50_display_fini()
> nv50_mstm_fini()
> drm_dp_mst_topology_mgr_suspend()
> flush_work(&mgr->work);
>
> And before the work item is flushed, the HPD source is quiesced.
>
> So it looks like drm_dp_mst_link_probe_work() can only ever run
> while the GPU is runtime resumed, it never runs while the GPU is
> runtime suspended. This means that you don't have to acquire any
> runtime PM references in or below drm_dp_mst_link_probe_work().
> Au contraire, you must not acquire any because it will deadlock while
> the GPU is runtime suspending. If there are functions which are
> called from drm_dp_mst_link_probe_work() as well as from other contexts,
> and those other contexts need a runtime PM ref to be acquired,
> you need to acquire the runtime PM ref conditionally on not being
> drm_dp_mst_link_probe_work() (using the current_work() technique).
>
> Alternatively, move acquisition of the runtime PM ref further up in
> the call chain to those other contexts.
>
>
> > Anyway-that's why your explanation doesn't make sense: the deadlock is
> > happening because we're calling pm_runtime_get_sync(). If we were to
> > make that call conditional (e.g. drm_kms_helper_is_poll_worker()),
> > all that would mean is that we wouldn't grab any runtime power reference
> > and the GPU would immediately suspend once the atomic commit finished,
> > as the suspend request in Thread 5 would finally get unblocked and thus
> > ----suspend.
>
> Right, that seems to be a bug nouveau_pmops_runtime_suspend():
>
> If a display is plugged in while the GPU is about to runtime suspend,
> the display may be lit up by output_poll_execute() but the GPU will
> then nevertheless be powered off.
>
> I guess after calling drm_kms_helper_poll_disable() we should re-check
> if a crtc has been activated. This should have bumped the runtime PM
> refcount and have_disp_power_ref should be true. In that case, the
> nouveau_pmops_runtime_suspend() should return -EBUSY to abort the
> runtime_suspend.
>
> The same check seems necessary after flushing drm_dp_mst_link_probe_work():
> If the work item lit up a new display, all previous suspend steps need
> to be unwound and -EBUSY needs to be returned to the PM core.
>
> Communication with an MST hub exceeding the autosuspend timeout is
> just one scenario where this bug manifests itself.
>
> BTW, drm_kms_helper_poll_disable() seems to be called twice in the
> runtime_suspend code path, once in nouveau_pmops_runtime_suspend()
> and a second time in nouveau_display_fini().
>
> A stupid question, I notice that nv50_display_fini() calls nv50_mstm_fini()
> only if encoder_type != DRM_MODE_ENCODER_DPMST. Why isn't that == ?
Because there's a difference between DPMST connectors and encoders vs. the
rest of the device's encoders. Every DP MST topology will take up a single
"physical" DP connector on the device that will be marked as disconnected.
This connector also owns the "mstm" (MST manager, referred to as the
drm_dp_mst_topology_mgr in DRM), which through the callbacks nouveau provides
is responsible for creating the fake DP MST ports and encoders. All of these
fake ports will have DPMST encoders as opposed to the physical DP ports, which
will have TMDS encoders. Hence-mstms are only on physical connectors with
TMDS, not fake connectors with DPMST.
>
> Thanks,
>
> Lukas
--
Cheers,
Lyude Paul
^ permalink raw reply
* Re: [PATCH 1/2] drm/fb_helper: Add drm_fb_helper_output_poll_changed_with_rpm()
From: Lyude Paul @ 2018-07-23 17:50 UTC (permalink / raw)
To: Lukas Wunner
Cc: David Airlie, nouveau, linux-kernel, dri-devel, Daniel Vetter,
Ben Skeggs
In-Reply-To: <20180721093955.GA32572@wunner.de>
On Sat, 2018-07-21 at 11:39 +0200, Lukas Wunner wrote:
> On Thu, Jul 19, 2018 at 08:08:15PM -0400, Lyude Paul wrote:
> > On Thu, 2018-07-19 at 09:49 +0200, Lukas Wunner wrote:
> > > On Wed, Jul 18, 2018 at 04:56:39PM -0400, Lyude Paul wrote:
> > > > When DP MST hubs get confused, they can occasionally stop responding
> > > > for
> > > > a good bit of time up until the point where the DRM driver manages to
> > > > do the right DPCD accesses to get it to start responding again. In a
> > > > worst case scenario however, this process can take upwards of 10+
> > > > seconds.
> > > >
> > > > Currently we use the default output_poll_changed handler
> > > > drm_fb_helper_output_poll_changed() to handle output polling, which
> > > > doesn't happen to grab any power references on the device when
> > > > polling.
> > > > If we're unlucky enough to have a hub (such as Lenovo's infamous
> > > > laptop
> > > > docks for the P5x/P7x series) that's easily startled and confused,
> > > > this
> > > > can lead to a pretty nasty deadlock situation that looks like this:
> > > >
> > > > - Hotplug event from hub happens, we enter
> > > > drm_fb_helper_output_poll_changed() and start communicating with the
> > > > hub
> > > > - While we're in drm_fb_helper_output_poll_changed() and attempting to
> > > > communicate with the hub, we end up confusing it and cause it to
> > > > stop
> > > > responding for at least 10 seconds
> > > > - After 5 seconds of being in drm_fb_helper_output_poll_changed(), the
> > > > pm core attempts to put the GPU into autosuspend, which ends up
> > > > calling drm_kms_helper_poll_disable()
> > > > - While the runtime PM core is waiting in
> > > > drm_kms_helper_poll_disable()
> > > > for the output poll to finish, we end up finally detecting an MST
> > > > display
> > > > - We notice the new display and tries to enable it, which triggers
> > > > an atomic commit which triggers a call to pm_runtime_get_sync()
> > > > - the output poll thread deadlocks the pm core waiting for the pm core
> > > > to finish the autosuspend request while the pm core waits for the
> > > > output poll thread to finish
> > >
> > > The correct fix is to call pm_runtime_get_sync() *conditionally* in
> > > the atomic commit which enables the display, using the same conditional
> > > as d61a5c106351, i.e. if (!drm_kms_helper_is_poll_worker()).
>
> First of all, I was mistaken when I wrote above that a check for
> !drm_kms_helper_is_poll_worker() would solve the problem. Sorry!
> It doesn't because the call to pm_runtime_get_sync() is not happening
> in output_poll_execute() but in drm_dp_mst_link_probe_work().
>
> Looking once more at the three stack traces you've provided, we've got:
> - output_poll_execute() stuck waiting for fb_helper->lock
> which is held by drm_dp_mst_link_probe_work()
> - rpm_suspend() stuck waiting for output_poll_execute() to finish
> - drm_dp_mst_link_probe_work() stuck waiting in rpm_resume()
>
> For the moment we can ignore the first task, i.e. output_poll_execute(),
> and focus on the latter two.
>
> As said I'm unfamiliar with MST but browsing through drm_dp_mst_topology.c
> I notice that drm_dp_mst_link_probe_work() is the ->work element in
> drm_dp_mst_topology_mgr() and is queued on HPD. I further notice that
> the work item is flushed on ->runtime_suspend:
>
> nouveau_pmops_runtime_suspend()
> nouveau_do_suspend()
> nouveau_display_suspend()
> nouveau_display_fini()
> disp->fini() == nv50_display_fini()
> nv50_mstm_fini()
> drm_dp_mst_topology_mgr_suspend()
> flush_work(&mgr->work);
>
> And before the work item is flushed, the HPD source is quiesced.
>
> So it looks like drm_dp_mst_link_probe_work() can only ever run
> while the GPU is runtime resumed, it never runs while the GPU is
> runtime suspended. This means that you don't have to acquire any
> runtime PM references in or below drm_dp_mst_link_probe_work().
> Au contraire, you must not acquire any because it will deadlock while
> the GPU is runtime suspending. If there are functions which are
> called from drm_dp_mst_link_probe_work() as well as from other contexts,
> and those other contexts need a runtime PM ref to be acquired,
> you need to acquire the runtime PM ref conditionally on not being
> drm_dp_mst_link_probe_work() (using the current_work() technique).
>
> Alternatively, move acquisition of the runtime PM ref further up in
> the call chain to those other contexts.
>
>
> > Anyway-that's why your explanation doesn't make sense: the deadlock is
> > happening because we're calling pm_runtime_get_sync(). If we were to
> > make that call conditional (e.g. drm_kms_helper_is_poll_worker()),
> > all that would mean is that we wouldn't grab any runtime power reference
> > and the GPU would immediately suspend once the atomic commit finished,
> > as the suspend request in Thread 5 would finally get unblocked and thus
> > ----suspend.
>
> Right, that seems to be a bug nouveau_pmops_runtime_suspend():
>
> If a display is plugged in while the GPU is about to runtime suspend,
> the display may be lit up by output_poll_execute() but the GPU will
> then nevertheless be powered off.
>
> I guess after calling drm_kms_helper_poll_disable() we should re-check
> if a crtc has been activated. This should have bumped the runtime PM
> refcount and have_disp_power_ref should be true. In that case, the
> nouveau_pmops_runtime_suspend() should return -EBUSY to abort the
> runtime_suspend.
>
> The same check seems necessary after flushing drm_dp_mst_link_probe_work():
> If the work item lit up a new display, all previous suspend steps need
> to be unwound and -EBUSY needs to be returned to the PM core.
>
> Communication with an MST hub exceeding the autosuspend timeout is
> just one scenario where this bug manifests itself.
>
> BTW, drm_kms_helper_poll_disable() seems to be called twice in the
> runtime_suspend code path, once in nouveau_pmops_runtime_suspend()
> and a second time in nouveau_display_fini().
>
> A stupid question, I notice that nv50_display_fini() calls nv50_mstm_fini()
> only if encoder_type != DRM_MODE_ENCODER_DPMST. Why isn't that == ?
Because there's a difference between DPMST connectors and encoders vs. the
rest of the device's encoders. Every DP MST topology will take up a single
"physical" DP connector on the device that will be marked as disconnected.
This connector also owns the "mstm" (MST manager, referred to as the
drm_dp_mst_topology_mgr in DRM), which through the callbacks nouveau provides
is responsible for creating the fake DP MST ports and encoders. All of these
fake ports will have DPMST encoders as opposed to the physical DP ports, which
will have TMDS encoders. Hence-mstms are only on physical connectors with
TMDS, not fake connectors with DPMST.
>
> Thanks,
>
> Lukas
--
Cheers,
Lyude Paul
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* [PATCH 2/2] cpufreq: Fix a circular lock dependency problem
From: Waiman Long @ 2018-07-23 17:49 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Thomas Gleixner, Peter Zijlstra,
Ingo Molnar
Cc: linux-kernel, linux-pm, Paul E. McKenney, Greg Kroah-Hartman,
Konrad Rzeszutek Wilk, Waiman Long
In-Reply-To: <1532368179-15263-1-git-send-email-longman@redhat.com>
With lockdep turned on, the following circular lock dependency problem
was reported:
[ 57.470040] ======================================================
[ 57.502900] WARNING: possible circular locking dependency detected
[ 57.535208] 4.18.0-0.rc3.1.el8+7.x86_64+debug #1 Tainted: G
[ 57.577761] ------------------------------------------------------
[ 57.609714] tuned/1505 is trying to acquire lock:
[ 57.633808] 00000000559deec5 (cpu_hotplug_lock.rw_sem){++++}, at: store+0x27/0x120
[ 57.672880]
[ 57.672880] but task is already holding lock:
[ 57.702184] 000000002136ca64 (kn->count#118){++++}, at: kernfs_fop_write+0x1d0/0x410
[ 57.742176]
[ 57.742176] which lock already depends on the new lock.
[ 57.742176]
[ 57.785220]
[ 57.785220] the existing dependency chain (in reverse order) is:
:
[ 58.932512] other info that might help us debug this:
[ 58.932512]
[ 58.973344] Chain exists of:
[ 58.973344] cpu_hotplug_lock.rw_sem --> subsys mutex#5 --> kn->count#118
[ 58.973344]
[ 59.030795] Possible unsafe locking scenario:
[ 59.030795]
[ 59.061248] CPU0 CPU1
[ 59.085377] ---- ----
[ 59.108160] lock(kn->count#118);
[ 59.124935] lock(subsys mutex#5);
[ 59.156330] lock(kn->count#118);
[ 59.186088] lock(cpu_hotplug_lock.rw_sem);
[ 59.208541]
[ 59.208541] *** DEADLOCK ***
In the cpufreq_register_driver() function, the lock sequence is:
cpus_read_lock --> kn->count
For the cpufreq sysfs store method, the lock sequence is:
kn->count --> cpus_read_lock
These sequences are actually safe as they are taking a share lock on
cpu_hotplug_lock. However, the current lockdep code doesn't check for
share locking when detecting circular lock dependency. Fixing that
could be a substantial effort.
Instead, we can work around this problem by using cpus_read_trylock()
in the store method which is much simpler. The chance of not getting
the read lock is extremely small. If that happens, the userspace
application that writes the sysfs file will get an error.
Signed-off-by: Waiman Long <longman@redhat.com>
---
drivers/cpufreq/cpufreq.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b0dfd32..9cf02d7 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -922,8 +922,22 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
struct cpufreq_policy *policy = to_policy(kobj);
struct freq_attr *fattr = to_attr(attr);
ssize_t ret = -EINVAL;
+ int retries = 3;
- cpus_read_lock();
+ /*
+ * cpus_read_trylock() is used here to work around a circular lock
+ * dependency problem with respect to the cpufreq_register_driver().
+ * With a simple retry loop, the chance of not able to get the
+ * read lock is extremely small.
+ */
+ while (!cpus_read_trylock()) {
+ if (retries-- <= 0)
+ return -EBUSY;
+ /*
+ * Sleep for about 50ms and retry again.
+ */
+ msleep(50);
+ }
if (cpu_online(policy->cpu)) {
down_write(&policy->rwsem);
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/2] cpu/hotplug: Add a cpus_read_trylock() function
From: Waiman Long @ 2018-07-23 17:49 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Thomas Gleixner, Peter Zijlstra,
Ingo Molnar
Cc: linux-kernel, linux-pm, Paul E. McKenney, Greg Kroah-Hartman,
Konrad Rzeszutek Wilk, Waiman Long
In-Reply-To: <1532368179-15263-1-git-send-email-longman@redhat.com>
There are use cases where it can be useful to have a cpus_read_trylock()
function to work around circular lock dependency problem involving
the cpu_hotplug_lock.
Signed-off-by: Waiman Long <longman@redhat.com>
---
include/linux/cpu.h | 2 ++
kernel/cpu.c | 6 ++++++
2 files changed, 8 insertions(+)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index a97a63e..e850bfe 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -103,6 +103,7 @@ static inline void cpu_maps_update_done(void)
extern void cpus_write_unlock(void);
extern void cpus_read_lock(void);
extern void cpus_read_unlock(void);
+extern int cpus_read_trylock(void);
extern void lockdep_assert_cpus_held(void);
extern void cpu_hotplug_disable(void);
extern void cpu_hotplug_enable(void);
@@ -115,6 +116,7 @@ static inline void cpus_write_lock(void) { }
static inline void cpus_write_unlock(void) { }
static inline void cpus_read_lock(void) { }
static inline void cpus_read_unlock(void) { }
+static inline int cpus_read_trylock(void) { return true; }
static inline void lockdep_assert_cpus_held(void) { }
static inline void cpu_hotplug_disable(void) { }
static inline void cpu_hotplug_enable(void) { }
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 0db8938..307486b 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -290,6 +290,12 @@ void cpus_read_lock(void)
}
EXPORT_SYMBOL_GPL(cpus_read_lock);
+int cpus_read_trylock(void)
+{
+ return percpu_down_read_trylock(&cpu_hotplug_lock);
+}
+EXPORT_SYMBOL_GPL(cpus_read_trylock);
+
void cpus_read_unlock(void)
{
percpu_up_read(&cpu_hotplug_lock);
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/2] cpufreq: Fix a circular lock dependency problem
From: Waiman Long @ 2018-07-23 17:49 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Thomas Gleixner, Peter Zijlstra,
Ingo Molnar
Cc: linux-kernel, linux-pm, Paul E. McKenney, Greg Kroah-Hartman,
Konrad Rzeszutek Wilk, Waiman Long
This patchset works around a circular lock dependency issue in the
cpufreq driver reported by lockdep. The two locks involved are the
cpu_hotplup_lock and the reference count of a sysfs file.
The cpufreq_register_driver() function uses the lock sequence:
cpus_read_lock --> kn->count
Whereas the cpufreq sysfs store method uses the sequence:
kn->count --> cpus_read_lock
This is not really an issue as a shared lock is used on the
cpu_hotplup_lock. However, the lockdep code isn't able to handle
shared locking. So one way to work around this is to define a
cpus_read_trylock() function and uses it in the store method instead.
Waiman Long (2):
cpu/hotplug: Add a cpus_read_trylock() function
cpufreq: Fix a circular lock dependency problem
drivers/cpufreq/cpufreq.c | 16 +++++++++++++++-
include/linux/cpu.h | 2 ++
kernel/cpu.c | 6 ++++++
3 files changed, 23 insertions(+), 1 deletion(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH net-next] selftests: forwarding: gre_multipath: Drop IPv6 tests
From: David Miller @ 2018-07-23 16:47 UTC (permalink / raw)
To: petrm; +Cc: netdev, linux-kselftest, shuah, idosch, dsahern
In-Reply-To: <cf0e380baf6a6aa5cf384017e8d69fec79750cfe.1532341813.git.petrm@mellanox.com>
From: Petr Machata <petrm@mellanox.com>
Date: Mon, 23 Jul 2018 12:33:08 +0200
> Support for device-only IPv6 multipath next hops was dropped in
> commit 33bd5ac54dc4 ("net/ipv6: Revert attempt to simplify route replace
> and append") and as of commit b5d2d75e079a ("net/ipv6: Do not allow
> device only routes via the multipath API"), attempts to add a next hop
> like that yield an explicit diagnostic.
>
> Correspondingly, drop the IPv6 parts of GRE multipath test that are
> supposed to test that code.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
Applied, thank you.
^ permalink raw reply
* [PATCH] pack-protocol: mention and point to docs for protocol v2
From: Brandon Williams @ 2018-07-23 17:48 UTC (permalink / raw)
To: git; +Cc: Brandon Williams
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/technical/pack-protocol.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
index 7fee6b780a..25acd9edb1 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -50,7 +50,8 @@ Each Extra Parameter takes the form of `<key>=<value>` or `<key>`.
Servers that receive any such Extra Parameters MUST ignore all
unrecognized keys. Currently, the only Extra Parameter recognized is
-"version=1".
+"version" with a vlue of '1' or '2'. See protocol-v2.txt for more
+information on protocol version 2.
Git Transport
-------------
--
2.18.0.233.g985f88cf7e-goog
^ permalink raw reply related
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 17:48 UTC (permalink / raw)
To: linux-arm-kernel
In most of cases, interrupt bits are set one by one but there are
also a lot of other cases that Aspeed I2C IP sends multiple
interrupt bits with combining master and slave events using a
single interrupt call. It happens much in multi-master environment
than single-master. For an example, when master is waiting for a
NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
RX_DONE interrupts could come along with the NORMAL_STOP in case of
an another master immediately sends data just after acquiring the
bus. In this case, the NORMAL_STOP interrupt should be handled
by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
handled by slave_irq. This commit modifies irq hadling logic to
handle the master/slave combined events properly.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -82,6 +82,11 @@
#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
#define ASPEED_I2CD_INTR_TX_NAK BIT(1)
#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+#define ASPEED_I2CD_INTR_ERRORS \
+ (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
+ ASPEED_I2CD_INTR_SCL_TIMEOUT | \
+ ASPEED_I2CD_INTR_ABNORMAL | \
+ ASPEED_I2CD_INTR_ARBIT_LOSS)
#define ASPEED_I2CD_INTR_ALL \
(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
#if IS_ENABLED(CONFIG_I2C_SLAVE)
static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
command = readl(bus->base + ASPEED_I2C_CMD_REG);
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
/* Slave was requested, restart state machine. */
- if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
/* Slave is not currently active, irq was for someone else. */
- if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
- irq_handled = false;
- goto out;
- }
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
+ return false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
dev_err(bus->dev, "Unexpected ACK on read request.\n");
bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
-
i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
break;
case ASPEED_I2C_SLAVE_READ_PROCESSED:
- status_ack |= ASPEED_I2CD_INTR_TX_ACK;
- if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
+ if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
dev_err(bus->dev,
"Expected ACK after processed read.\n");
i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
@@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
break;
}
- if (status_ack != irq_status)
- dev_err(bus->dev,
- "irq handled != irq. expected %x, but was %x\n",
- irq_status, status_ack);
- writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
-
-out:
- return irq_handled;
+ bus->irq_status ^= status_ack;
+ return !bus->irq_status;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
{
- u32 irq_status, status_ack = 0, command = 0;
+ u32 status_ack = 0, command = 0;
struct i2c_msg *msg;
u8 recv_byte;
int ret;
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
- /* Ack all interrupt bits. */
- writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
-
- if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
goto out_complete;
+ } else {
+ /* Master is not currently active, irq was for someone else. */
+ if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
+ goto out_no_complete;
}
/*
@@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* should clear the command queue effectively taking us back to the
* INACTIVE state.
*/
- ret = aspeed_i2c_is_irq_error(irq_status);
- if (ret < 0) {
+ ret = aspeed_i2c_is_irq_error(bus->irq_status);
+ if (ret) {
dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
- irq_status);
+ bus->irq_status);
bus->cmd_err = ret;
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
goto out_complete;
}
/* We are in an invalid state; reset bus to a known state. */
if (!bus->msgs) {
- dev_err(bus->dev, "bus in unknown state\n");
+ dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
+ bus->irq_status);
bus->cmd_err = -EIO;
- if (bus->master_state != ASPEED_I2C_MASTER_STOP)
+ if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
+ bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
aspeed_i2c_do_stop(bus);
goto out_no_complete;
}
@@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* then update the state and handle the new state below.
*/
if (bus->master_state == ASPEED_I2C_MASTER_START) {
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
- pr_devel("no slave present at %02x\n", msg->addr);
+ if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_TX_NAK))) {
+ bus->cmd_err = -ENXIO;
+ bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ goto out_complete;
+ }
+ pr_devel("no slave present at %02x", msg->addr);
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->cmd_err = -ENXIO;
aspeed_i2c_do_stop(bus);
@@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
switch (bus->master_state) {
case ASPEED_I2C_MASTER_TX:
- if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
+ if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
dev_dbg(bus->dev, "slave NACKed TX\n");
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
goto error_and_stop;
- } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ } else if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_TX_ACK))) {
dev_err(bus->dev, "slave failed to ACK TX\n");
goto error_and_stop;
}
@@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
goto out_no_complete;
case ASPEED_I2C_MASTER_RX_FIRST:
/* RX may not have completed yet (only address cycle) */
- if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
+ if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
goto out_no_complete;
/* fallthrough intended */
case ASPEED_I2C_MASTER_RX:
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
+ if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
dev_err(bus->dev, "master failed to RX\n");
goto error_and_stop;
}
@@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
}
goto out_no_complete;
case ASPEED_I2C_MASTER_STOP:
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
- dev_err(bus->dev, "master failed to STOP\n");
+ if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_NORMAL_STOP))) {
+ dev_err(bus->dev,
+ "master failed to STOP irq_status:0x%x\n",
+ bus->irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we have already tried. */
} else {
@@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
case ASPEED_I2C_MASTER_INACTIVE:
dev_err(bus->dev,
"master received interrupt 0x%08x, but is inactive\n",
- irq_status);
+ bus->irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we should be inactive. */
goto out_complete;
@@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
bus->master_xfer_result = bus->msgs_index + 1;
complete(&bus->cmd_complete);
out_no_complete:
- if (irq_status != status_ack)
- dev_err(bus->dev,
- "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
- irq_status, status_ack);
- return !!irq_status;
+ bus->irq_status ^= status_ack;
+ return !bus->irq_status;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
- bool ret;
+ u32 irq_received;
spin_lock(&bus->lock);
+ irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ bus->irq_status = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
- if (aspeed_i2c_slave_irq(bus)) {
- dev_dbg(bus->dev, "irq handled by slave.\n");
- ret = true;
- goto out;
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ if (!aspeed_i2c_master_irq(bus))
+ aspeed_i2c_slave_irq(bus);
+ } else {
+ if (!aspeed_i2c_slave_irq(bus))
+ aspeed_i2c_master_irq(bus);
}
+#else
+ aspeed_i2c_master_irq(bus);
#endif /* CONFIG_I2C_SLAVE */
- ret = aspeed_i2c_master_irq(bus);
+ if (bus->irq_status)
+ dev_err(bus->dev,
+ "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+ irq_received, irq_received ^ bus->irq_status);
-out:
+ /* Ack all interrupt bits. */
+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
spin_unlock(&bus->lock);
- return ret ? IRQ_HANDLED : IRQ_NONE;
+ return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.18.0
^ permalink raw reply related
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 17:48 UTC (permalink / raw)
To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
Andrew Jeffery, linux-i2c, openbmc, linux-arm-kernel,
linux-aspeed, linux-kernel
Cc: Jarkko Nikula, James Feist, Vernon Mauery, Jae Hyun Yoo
In most of cases, interrupt bits are set one by one but there are
also a lot of other cases that Aspeed I2C IP sends multiple
interrupt bits with combining master and slave events using a
single interrupt call. It happens much in multi-master environment
than single-master. For an example, when master is waiting for a
NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
RX_DONE interrupts could come along with the NORMAL_STOP in case of
an another master immediately sends data just after acquiring the
bus. In this case, the NORMAL_STOP interrupt should be handled
by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
handled by slave_irq. This commit modifies irq hadling logic to
handle the master/slave combined events properly.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -82,6 +82,11 @@
#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
#define ASPEED_I2CD_INTR_TX_NAK BIT(1)
#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+#define ASPEED_I2CD_INTR_ERRORS \
+ (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
+ ASPEED_I2CD_INTR_SCL_TIMEOUT | \
+ ASPEED_I2CD_INTR_ABNORMAL | \
+ ASPEED_I2CD_INTR_ARBIT_LOSS)
#define ASPEED_I2CD_INTR_ALL \
(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
#if IS_ENABLED(CONFIG_I2C_SLAVE)
static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
command = readl(bus->base + ASPEED_I2C_CMD_REG);
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
/* Slave was requested, restart state machine. */
- if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
/* Slave is not currently active, irq was for someone else. */
- if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
- irq_handled = false;
- goto out;
- }
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
+ return false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
dev_err(bus->dev, "Unexpected ACK on read request.\n");
bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
-
i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
break;
case ASPEED_I2C_SLAVE_READ_PROCESSED:
- status_ack |= ASPEED_I2CD_INTR_TX_ACK;
- if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
+ if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
dev_err(bus->dev,
"Expected ACK after processed read.\n");
i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
@@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
break;
}
- if (status_ack != irq_status)
- dev_err(bus->dev,
- "irq handled != irq. expected %x, but was %x\n",
- irq_status, status_ack);
- writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
-
-out:
- return irq_handled;
+ bus->irq_status ^= status_ack;
+ return !bus->irq_status;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
{
- u32 irq_status, status_ack = 0, command = 0;
+ u32 status_ack = 0, command = 0;
struct i2c_msg *msg;
u8 recv_byte;
int ret;
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
- /* Ack all interrupt bits. */
- writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
-
- if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
goto out_complete;
+ } else {
+ /* Master is not currently active, irq was for someone else. */
+ if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
+ goto out_no_complete;
}
/*
@@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* should clear the command queue effectively taking us back to the
* INACTIVE state.
*/
- ret = aspeed_i2c_is_irq_error(irq_status);
- if (ret < 0) {
+ ret = aspeed_i2c_is_irq_error(bus->irq_status);
+ if (ret) {
dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
- irq_status);
+ bus->irq_status);
bus->cmd_err = ret;
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
goto out_complete;
}
/* We are in an invalid state; reset bus to a known state. */
if (!bus->msgs) {
- dev_err(bus->dev, "bus in unknown state\n");
+ dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
+ bus->irq_status);
bus->cmd_err = -EIO;
- if (bus->master_state != ASPEED_I2C_MASTER_STOP)
+ if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
+ bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
aspeed_i2c_do_stop(bus);
goto out_no_complete;
}
@@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* then update the state and handle the new state below.
*/
if (bus->master_state == ASPEED_I2C_MASTER_START) {
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
- pr_devel("no slave present at %02x\n", msg->addr);
+ if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_TX_NAK))) {
+ bus->cmd_err = -ENXIO;
+ bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ goto out_complete;
+ }
+ pr_devel("no slave present at %02x", msg->addr);
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->cmd_err = -ENXIO;
aspeed_i2c_do_stop(bus);
@@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
switch (bus->master_state) {
case ASPEED_I2C_MASTER_TX:
- if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
+ if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
dev_dbg(bus->dev, "slave NACKed TX\n");
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
goto error_and_stop;
- } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ } else if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_TX_ACK))) {
dev_err(bus->dev, "slave failed to ACK TX\n");
goto error_and_stop;
}
@@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
goto out_no_complete;
case ASPEED_I2C_MASTER_RX_FIRST:
/* RX may not have completed yet (only address cycle) */
- if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
+ if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
goto out_no_complete;
/* fallthrough intended */
case ASPEED_I2C_MASTER_RX:
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
+ if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
dev_err(bus->dev, "master failed to RX\n");
goto error_and_stop;
}
@@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
}
goto out_no_complete;
case ASPEED_I2C_MASTER_STOP:
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
- dev_err(bus->dev, "master failed to STOP\n");
+ if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_NORMAL_STOP))) {
+ dev_err(bus->dev,
+ "master failed to STOP irq_status:0x%x\n",
+ bus->irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we have already tried. */
} else {
@@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
case ASPEED_I2C_MASTER_INACTIVE:
dev_err(bus->dev,
"master received interrupt 0x%08x, but is inactive\n",
- irq_status);
+ bus->irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we should be inactive. */
goto out_complete;
@@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
bus->master_xfer_result = bus->msgs_index + 1;
complete(&bus->cmd_complete);
out_no_complete:
- if (irq_status != status_ack)
- dev_err(bus->dev,
- "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
- irq_status, status_ack);
- return !!irq_status;
+ bus->irq_status ^= status_ack;
+ return !bus->irq_status;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
- bool ret;
+ u32 irq_received;
spin_lock(&bus->lock);
+ irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ bus->irq_status = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
- if (aspeed_i2c_slave_irq(bus)) {
- dev_dbg(bus->dev, "irq handled by slave.\n");
- ret = true;
- goto out;
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ if (!aspeed_i2c_master_irq(bus))
+ aspeed_i2c_slave_irq(bus);
+ } else {
+ if (!aspeed_i2c_slave_irq(bus))
+ aspeed_i2c_master_irq(bus);
}
+#else
+ aspeed_i2c_master_irq(bus);
#endif /* CONFIG_I2C_SLAVE */
- ret = aspeed_i2c_master_irq(bus);
+ if (bus->irq_status)
+ dev_err(bus->dev,
+ "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+ irq_received, irq_received ^ bus->irq_status);
-out:
+ /* Ack all interrupt bits. */
+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
spin_unlock(&bus->lock);
- return ret ? IRQ_HANDLED : IRQ_NONE;
+ return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.18.0
^ permalink raw reply related
* [PATCH i2c-next] i2c: aspeed: Handle master/slave combined irq events properly
From: Jae Hyun Yoo @ 2018-07-23 17:48 UTC (permalink / raw)
To: linux-aspeed
In most of cases, interrupt bits are set one by one but there are
also a lot of other cases that Aspeed I2C IP sends multiple
interrupt bits with combining master and slave events using a
single interrupt call. It happens much in multi-master environment
than single-master. For an example, when master is waiting for a
NORMAL_STOP interrupt in its MASTER_STOP state, SLAVE_MATCH and
RX_DONE interrupts could come along with the NORMAL_STOP in case of
an another master immediately sends data just after acquiring the
bus. In this case, the NORMAL_STOP interrupt should be handled
by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
handled by slave_irq. This commit modifies irq hadling logic to
handle the master/slave combined events properly.
Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
drivers/i2c/busses/i2c-aspeed.c | 137 ++++++++++++++++++--------------
1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index efb89422d496..24d43f143a55 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -82,6 +82,11 @@
#define ASPEED_I2CD_INTR_RX_DONE BIT(2)
#define ASPEED_I2CD_INTR_TX_NAK BIT(1)
#define ASPEED_I2CD_INTR_TX_ACK BIT(0)
+#define ASPEED_I2CD_INTR_ERRORS \
+ (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
+ ASPEED_I2CD_INTR_SCL_TIMEOUT | \
+ ASPEED_I2CD_INTR_ABNORMAL | \
+ ASPEED_I2CD_INTR_ARBIT_LOSS)
#define ASPEED_I2CD_INTR_ALL \
(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
@@ -150,6 +155,7 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ u32 irq_status;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -229,36 +235,30 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
#if IS_ENABLED(CONFIG_I2C_SLAVE)
static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
{
- u32 command, irq_status, status_ack = 0;
+ u32 command, status_ack = 0;
struct i2c_client *slave = bus->slave;
- bool irq_handled = true;
u8 value;
- if (!slave) {
- irq_handled = false;
- goto out;
- }
+ if (!slave)
+ return false;
command = readl(bus->base + ASPEED_I2C_CMD_REG);
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
/* Slave was requested, restart state machine. */
- if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
bus->slave_state = ASPEED_I2C_SLAVE_START;
}
/* Slave is not currently active, irq was for someone else. */
- if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
- irq_handled = false;
- goto out;
- }
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
+ return false;
dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
- irq_status, command);
+ bus->irq_status, command);
/* Slave was sent something. */
- if (irq_status & ASPEED_I2CD_INTR_RX_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_RX_DONE) {
value = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
/* Handle address frame. */
if (bus->slave_state == ASPEED_I2C_SLAVE_START) {
@@ -273,28 +273,29 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
}
/* Slave was asked to stop. */
- if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
- if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_NAK) {
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->slave_state = ASPEED_I2C_SLAVE_STOP;
}
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK) {
+ status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+ }
switch (bus->slave_state) {
case ASPEED_I2C_SLAVE_READ_REQUESTED:
- if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+ if (bus->irq_status & ASPEED_I2CD_INTR_TX_ACK)
dev_err(bus->dev, "Unexpected ACK on read request.\n");
bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
-
i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
break;
case ASPEED_I2C_SLAVE_READ_PROCESSED:
- status_ack |= ASPEED_I2CD_INTR_TX_ACK;
- if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
+ if (!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))
dev_err(bus->dev,
"Expected ACK after processed read.\n");
i2c_slave_event(slave, I2C_SLAVE_READ_PROCESSED, &value);
@@ -317,14 +318,8 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
break;
}
- if (status_ack != irq_status)
- dev_err(bus->dev,
- "irq handled != irq. expected %x, but was %x\n",
- irq_status, status_ack);
- writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
-
-out:
- return irq_handled;
+ bus->irq_status ^= status_ack;
+ return !bus->irq_status;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -382,19 +377,19 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
{
- u32 irq_status, status_ack = 0, command = 0;
+ u32 status_ack = 0, command = 0;
struct i2c_msg *msg;
u8 recv_byte;
int ret;
- irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
- /* Ack all interrupt bits. */
- writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
-
- if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
+ if (bus->irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
goto out_complete;
+ } else {
+ /* Master is not currently active, irq was for someone else. */
+ if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
+ goto out_no_complete;
}
/*
@@ -402,20 +397,23 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* should clear the command queue effectively taking us back to the
* INACTIVE state.
*/
- ret = aspeed_i2c_is_irq_error(irq_status);
- if (ret < 0) {
+ ret = aspeed_i2c_is_irq_error(bus->irq_status);
+ if (ret) {
dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
- irq_status);
+ bus->irq_status);
bus->cmd_err = ret;
bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ status_ack |= (bus->irq_status & ASPEED_I2CD_INTR_ERRORS);
goto out_complete;
}
/* We are in an invalid state; reset bus to a known state. */
if (!bus->msgs) {
- dev_err(bus->dev, "bus in unknown state\n");
+ dev_err(bus->dev, "bus in unknown state irq_status: 0x%x\n",
+ bus->irq_status);
bus->cmd_err = -EIO;
- if (bus->master_state != ASPEED_I2C_MASTER_STOP)
+ if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
+ bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
aspeed_i2c_do_stop(bus);
goto out_no_complete;
}
@@ -427,8 +425,14 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
* then update the state and handle the new state below.
*/
if (bus->master_state == ASPEED_I2C_MASTER_START) {
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
- pr_devel("no slave present at %02x\n", msg->addr);
+ if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_TX_NAK))) {
+ bus->cmd_err = -ENXIO;
+ bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+ goto out_complete;
+ }
+ pr_devel("no slave present at %02x", msg->addr);
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
bus->cmd_err = -ENXIO;
aspeed_i2c_do_stop(bus);
@@ -447,11 +451,12 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
switch (bus->master_state) {
case ASPEED_I2C_MASTER_TX:
- if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
+ if (unlikely(bus->irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
dev_dbg(bus->dev, "slave NACKed TX\n");
status_ack |= ASPEED_I2CD_INTR_TX_NAK;
goto error_and_stop;
- } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+ } else if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_TX_ACK))) {
dev_err(bus->dev, "slave failed to ACK TX\n");
goto error_and_stop;
}
@@ -470,11 +475,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
goto out_no_complete;
case ASPEED_I2C_MASTER_RX_FIRST:
/* RX may not have completed yet (only address cycle) */
- if (!(irq_status & ASPEED_I2CD_INTR_RX_DONE))
+ if (!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))
goto out_no_complete;
/* fallthrough intended */
case ASPEED_I2C_MASTER_RX:
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
+ if (unlikely(!(bus->irq_status & ASPEED_I2CD_INTR_RX_DONE))) {
dev_err(bus->dev, "master failed to RX\n");
goto error_and_stop;
}
@@ -505,8 +510,11 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
}
goto out_no_complete;
case ASPEED_I2C_MASTER_STOP:
- if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
- dev_err(bus->dev, "master failed to STOP\n");
+ if (unlikely(!(bus->irq_status &
+ ASPEED_I2CD_INTR_NORMAL_STOP))) {
+ dev_err(bus->dev,
+ "master failed to STOP irq_status:0x%x\n",
+ bus->irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we have already tried. */
} else {
@@ -518,7 +526,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
case ASPEED_I2C_MASTER_INACTIVE:
dev_err(bus->dev,
"master received interrupt 0x%08x, but is inactive\n",
- irq_status);
+ bus->irq_status);
bus->cmd_err = -EIO;
/* Do not STOP as we should be inactive. */
goto out_complete;
@@ -540,33 +548,40 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
bus->master_xfer_result = bus->msgs_index + 1;
complete(&bus->cmd_complete);
out_no_complete:
- if (irq_status != status_ack)
- dev_err(bus->dev,
- "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
- irq_status, status_ack);
- return !!irq_status;
+ bus->irq_status ^= status_ack;
+ return !bus->irq_status;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
- bool ret;
+ u32 irq_received;
spin_lock(&bus->lock);
+ irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+ bus->irq_status = irq_received;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
- if (aspeed_i2c_slave_irq(bus)) {
- dev_dbg(bus->dev, "irq handled by slave.\n");
- ret = true;
- goto out;
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ if (!aspeed_i2c_master_irq(bus))
+ aspeed_i2c_slave_irq(bus);
+ } else {
+ if (!aspeed_i2c_slave_irq(bus))
+ aspeed_i2c_master_irq(bus);
}
+#else
+ aspeed_i2c_master_irq(bus);
#endif /* CONFIG_I2C_SLAVE */
- ret = aspeed_i2c_master_irq(bus);
+ if (bus->irq_status)
+ dev_err(bus->dev,
+ "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+ irq_received, irq_received ^ bus->irq_status);
-out:
+ /* Ack all interrupt bits. */
+ writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
spin_unlock(&bus->lock);
- return ret ? IRQ_HANDLED : IRQ_NONE;
+ return bus->irq_status ? IRQ_NONE : IRQ_HANDLED;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.18.0
^ permalink raw reply related
* Re: [PATCH v6 16/17] media: v4l2: async: Remove notifier subdevs array
From: Steve Longerbeam @ 2018-07-23 16:44 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, Steve Longerbeam, Mauro Carvalho Chehab,
Sakari Ailus, Niklas Söderlund, Sebastian Reichel,
Hans Verkuil, open list
In-Reply-To: <20180723123557.bfxxsqqhlaj3ccwc@valkosipuli.retiisi.org.uk>
On 07/23/2018 05:35 AM, Sakari Ailus wrote:
> Hi Steve,
>
> Thanks for the update.
>
> On Mon, Jul 09, 2018 at 03:39:16PM -0700, Steve Longerbeam wrote:
>> All platform drivers have been converted to use
>> v4l2_async_notifier_add_subdev(), in place of adding
>> asd's to the notifier subdevs array. So the subdevs
>> array can now be removed from struct v4l2_async_notifier,
>> and remove the backward compatibility support for that
>> array in v4l2-async.c.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> This set removes the subdevs and num_subdevs fieldsfrom the notifier (as
> discussed previously) but it doesn't include the corresponding
> driver changes. Is there a patch missing from the set?
Hi Sakari, yes somehow patch 15/17 (the large patch to all drivers)
got dropped by the ML, maybe because the cc-list was too big?
I will resend with only linux-media and cc: you.
Steve
^ permalink raw reply
* Re: [PATCH 05/11] touchscreen: elants: Use octal permissions
From: Dmitry Torokhov @ 2018-07-23 17:46 UTC (permalink / raw)
To: Joe Perches
Cc: Greg Kroah-Hartman, dev-harsh1998, trivial, Simon Budig,
Andi Shyti, Luca Ceresoli, Guenter Roeck, linux-input,
linux-kernel
In-Reply-To: <e257bc89eb082e9fdc342d6f794c15a2805c98a9.camel@perches.com>
On Mon, Jul 23, 2018 at 10:25:10AM -0700, Joe Perches wrote:
> On Mon, 2018-07-23 at 10:18 -0700, Dmitry Torokhov wrote:
> > On Mon, Jul 23, 2018 at 03:32:00PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Jul 23, 2018 at 06:49:20PM +0530, dev-harsh1998 wrote:
> > > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
> > > > +static DEVICE_ATTR(iap_mode, S_IRUGO, show_iap_mode, NULL);
> > > >
> > > > WARNING: Symbolic permissions 'S_IWUSR' are not preferred. Consider using octal permissions '0200'.
> > > > +static DEVICE_ATTR(update_fw, S_IWUSR, NULL, write_update_fw)
> > > >
> > > > WARNING: Symbolic permissions 'S_IRUGO' are not preferred. Consider using octal permissions '0444'.
> > > > + .dattr = __ATTR(_field, S_IRUGO, \
> > > >
> > > > Signed-off-by: Harshit Jain <harshitjain6751@gmail.com>
> > >
> > > This name doesn't match up with the From: line above :(
> > >
> > > Please fix up and try again.
> >
> > dtor@dtor-ws:~/kernel/linux-next$ git grep S_IRU | wc -l
> > 7605
> >
> > We either need to run a tree-wide script or leave this alone. FWIW I am
> > perfectly fine with either octals or symbolic names so I do not see
> > benefit of doing conversion for code that is not known to be broken.
>
> About half of those are in one subsystem (drivers/hwmon)
>
> $ git grep -w S_IRUGO | cut -f1,2 -d'/' | \
> sort | uniq -c | sort -rn | head -10 | cat -n
> 1 3846 drivers/hwmon
> 2 748 drivers/scsi
> 3 215 drivers/infiniband
> 4 168 drivers/usb
> 5 109 drivers/media
> 6 106 drivers/input
> 7 102 drivers/platform
> 8 101 drivers/misc
> 9 101 drivers/gpu
> 10 91 drivers/edac
>
> The generic reason is octal is readable and S_<FOO> is unintelligible.
>
> https://lkml.org/lkml/2016/8/2/1945
Yes, but please also read this one as well:
https://lkml.org/lkml/2016/8/2/2062
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v3 3/3] dmaengine: imx-sdma: allocate max 20 bds for one transfer
From: Robin Gong @ 2018-07-23 17:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1532367972-29707-1-git-send-email-yibin.gong@nxp.com>
If multi-bds used in one transfer, all bds should be consisten
memory.To easily follow it, enlarge the dma pool size into 20 bds,
and it will report error if the number of bds is over than 20. For
dmatest, the max count for single transfer is NUM_BD *
SDMA_BD_MAX_CNT = 20 * 65535 = ~1.28MB.
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
drivers/dma/imx-sdma.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
index b4ec2d2..5973489 100644
--- a/drivers/dma/imx-sdma.c
+++ b/drivers/dma/imx-sdma.c
@@ -298,6 +298,15 @@ struct sdma_context_data {
u32 scratch7;
} __attribute__ ((packed));
+/*
+ * All bds in one transfer should be consitent on SDMA. To easily follow it,just
+ * set the dma pool size as the enough bds. For example, in dmatest case, the
+ * max 20 bds means the max for single transfer is NUM_BD * SDMA_BD_MAX_CNT = 20
+ * * 65535 = ~1.28MB. 20 bds supposed to be enough basically.If it's still not
+ * enough in some specific cases, enlarge it here.Warning message would also
+ * appear if the bd numbers is over than 20.
+ */
+#define NUM_BD 20
struct sdma_engine;
@@ -1273,7 +1282,7 @@ static int sdma_alloc_chan_resources(struct dma_chan *chan)
goto disable_clk_ahb;
sdmac->bd_pool = dma_pool_create("bd_pool", chan->device->dev,
- sizeof(struct sdma_buffer_descriptor),
+ NUM_BD * sizeof(struct sdma_buffer_descriptor),
32, 0);
return 0;
@@ -1314,6 +1323,12 @@ static struct sdma_desc *sdma_transfer_init(struct sdma_channel *sdmac,
{
struct sdma_desc *desc;
+ if (bds > NUM_BD) {
+ dev_err(sdmac->sdma->dev, "%d bds exceed the max %d\n",
+ bds, NUM_BD);
+ goto err_out;
+ }
+
desc = kzalloc((sizeof(*desc)), GFP_NOWAIT);
if (!desc)
goto err_out;
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.