Re: [PATCH v8 0/3] ARM: Implement MODULE_PLT support in FTRACE
Hi!
On 09/04/2021 17:33, Qais Yousef wrote:
> I still think the ifdefery in patch 3 is ugly. Any reason my suggestion didn't
> work out for you? I struggle to see how this is better and why it was hard to
> incorporate my suggestion.
>
> For example
>
> - old = ftrace_call_replace(ip, adjust_address(rec, addr));
> +#ifdef CONFIG_ARM_MODULE_PLTS
> + /* mod is only supplied during module loading */
> + if (!mod)
> + mod = rec->arch.mod;
> + else
> + rec->arch.mod = mod;
> +#endif
> +
> + old = ftrace_call_replace(ip, aaddr,
> + !IS_ENABLED(CONFIG_ARM_MODULE_PLTS)
> || !mod);
> +#ifdef CONFIG_ARM_MODULE_PLTS
> + if (!old && mod) {
> + aaddr = get_module_plt(mod, ip, aaddr);
> + old = ftrace_call_replace(ip, aaddr, true);
> + }
> +#endif
> +
>
> There's an ifdef, followed by a code that embeds
> !IS_ENABLED(CONFIG_ARM_MODULE_PLTS) followed by another ifdef :-/
No, it's actually two small ifdefed blocks added before and after an original
call,
which parameters have been modified as well. The issue with arch.mod was
explained
by Steven Rostedt, maybe you've missed his email.
> And there was no need to make the new warn arg visible all the way to
> ftrace_call_repalce() and all of its users.
>
> FWIW
>
> Tested-by: Qais Yousef
--
Best regards,
Alexander Sverdlin.
Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
Hello Qais,
On 24/03/2021 16:57, Qais Yousef wrote:
>>> FWIW my main concern is about duplicating the range check in
>>> ftrace_call_replace() and using magic values that already exist in
>>> __arm_gen_branch_{arm, thumb2}() and better remain encapsulated there.
>> could you please check the negative limits? I have an opinion, my limits are
>> correct. I could add extra parameter to arm_gen_branch_link(), but for this
>> I first need to fix its negative limits, which, I believe, well...
>> Approximate :)
> Can you elaborate please?
>
> If you look at Arm ARM [1] the ranges are defined in page 347
>
> Encoding T1 Even numbers in the range –16777216 to 16777214.
> Encoding T2 Multiples of 4 in the range –16777216 to 16777212.
> Encoding A1 Multiples of 4 in the range –33554432 to 33554428.
> Encoding A2 Even numbers in the range –33554432 to 33554430.
>
> which matches what's in the code (T1 for thumb2 and A1 for arm).
>
> Why do you think it's wrong?
thanks for checking this! I'll re-send v8 with your proposal.
--
Best regards,
Alexander Sverdlin.
Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
Hi Qais,
On 23/03/2021 23:22, Qais Yousef wrote:
>>> Yes you're right. I was a bit optimistic on CONFIG_DYNAMIC_FTRACE will imply
>>> CONFIG_ARM_MODULE_PLTS is enabled too.
>>>
>>> It only has an impact on reducing ifdefery when calling
>>>
>>> ftrace_call_replace_mod(rec->arch.mod, ...)
>>>
>>> Should be easy to wrap rec->arch.mod with its own accessor that will return
>>> NULL if !CONFIG_ARM_MODULE_PLTS or just ifdef the functions.
>>>
>>> Up to Alexander to pick what he prefers :-)
>> well, I of course prefer v7 as-is, because this review is running longer
>> than two
>> years and I actually hope these patches to be finally merged at some point.
>> But you are welcome to optimize them with follow up patches :)
> I appreciate that and thanks a lot for your effort. My attempt to review and
> test here is to help in getting this merged.
>
> FWIW my main concern is about duplicating the range check in
> ftrace_call_replace() and using magic values that already exist in
> __arm_gen_branch_{arm, thumb2}() and better remain encapsulated there.
could you please check the negative limits? I have an opinion, my limits are
correct. I could add extra parameter to arm_gen_branch_link(), but for this
I first need to fix its negative limits, which, I believe, well... Approximate
:)
--
Best regards,
Alexander Sverdlin.
Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
Hi Qais, On 22/03/2021 17:32, Qais Yousef wrote: > Yes you're right. I was a bit optimistic on CONFIG_DYNAMIC_FTRACE will imply > CONFIG_ARM_MODULE_PLTS is enabled too. > > It only has an impact on reducing ifdefery when calling > > ftrace_call_replace_mod(rec->arch.mod, ...) > > Should be easy to wrap rec->arch.mod with its own accessor that will return > NULL if !CONFIG_ARM_MODULE_PLTS or just ifdef the functions. > > Up to Alexander to pick what he prefers :-) well, I of course prefer v7 as-is, because this review is running longer than two years and I actually hope these patches to be finally merged at some point. But you are welcome to optimize them with follow up patches :) -- Best regards, Alexander Sverdlin.
Re: [PATCH v3] gpio: pl061: Support implementations without GPIOINTR line
Hello Linus, Andy, On 20/03/2021 12:10, Linus Walleij wrote: >>> I'm wondering if the GPIO library support for IRQ hierarchy is what >>> you are looking for. > It is indeed what should be used. and what has been used in my patch? >> do you have a better suggestion? That's why I reference the below discussion >> from 2017, where >> Linus Walleij suggested to use it. Well, the initial patch was just 11-liner >> and PL061 is >> rather counterexample and doesn't fit well into the existing hierarchical >> infrastructure. >> >>>> Link: >>>> https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=jhgb74hbcqihotexs+svyo6sr...@mail.gmail.com/ > Don't trust that guy. He's inexperienced with the new API and talks a lot > of crap. ;) > Yes, that's my impression now as well ;) [...] > 4. Implement pl061_child_to_parent_nokia_rock_n_roll() >Just use hardcoded hardware IRQ offsets like other drivers such as >the ixp4xx does. Do not attempt to read any parent IRQs from >the device tree, and assign no IRQ in the device tree. > > This is a side effect of the essentially per-soc pecularities around > interrupts. The interrupt is not cascaded so it need special > handling. > > I think it can be done with quite little code. Guys, have you actually looked onto my patch before these reviews? -- Best regards, Alexander Sverdlin.
Re: [PATCH v3] gpio: pl061: Support implementations without GPIOINTR line
Hello Andy, >> From: Alexander Sverdlin >> >> There are several implementations of PL061 which lack GPIOINTR signal in >> hardware and only have individual GPIOMIS[7:0] interrupts. Use the >> hierarchical interrupt support of the gpiolib in these cases (if at least 8 >> IRQs are configured for the PL061). >> >> One in-tree example is arch/arm/boot/dts/axm55xx.dtsi, PL061 instances have >> 8 IRQs defined, but current driver supports only the first one, so only one >> pin would work as IRQ trigger. > > an IRQ > > I'm wondering if the GPIO library support for IRQ hierarchy is what > you are looking for. do you have a better suggestion? That's why I reference the below discussion from 2017, where Linus Walleij suggested to use it. Well, the initial patch was just 11-liner and PL061 is rather counterexample and doesn't fit well into the existing hierarchical infrastructure. >> Link: >> https://lore.kernel.org/linux-gpio/CACRpkdZpYzpMDWqJobSYH=jhgb74hbcqihotexs+svyo6sr...@mail.gmail.com/ [...] >> --- a/drivers/gpio/Kconfig >> +++ b/drivers/gpio/Kconfig >> @@ -469,6 +469,7 @@ config GPIO_PL061 >> depends on ARM_AMBA >> select IRQ_DOMAIN >> select GPIOLIB_IRQCHIP >> + select IRQ_DOMAIN_HIERARCHY -- Best regards, Alexander Sverdlin.
Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
Hello Florian, On 12/03/2021 19:35, Florian Fainelli wrote: >>> https://www.spinics.net/lists/arm-kernel/msg878599.html >> I am testing with your module. I can't reproduce the problem you describe >> with >> it as I stated. >> >> I will try to spend more time on it on the weekend. > Alexander, do you load one or multiple instances of that fat module? One cannot have "multiple instances of the same module"... > The test module does a 6 * 1024 * 1024 / 2 = 3 million repetitions of > the "nop" instruction which should be 32-bits wide in ARM mode and > 16-bits wide in Thumb mode, right? > > In ARM mode we have a 14MB module space, so 3 * 1024 * 1024 * 4 = 12MB, > which should still fit within if you have no module loaded, however a > second instance of the module should make us spill into vmalloc space. > > In Thumb mode, we have a 6MB module space, so 3 * 1024 * 1024 * 2 = 6MB > so we may spill, but maybe not. > > I was not able to reproduce the warning with just one module, but with > two (cannot have the same name BTW), it kicked in. ... well, may be the size was arbitrary chosen to not fit into our module space and we have more modules already loaded. But you are free to adjust the amount of NOPs! :) -- Best regards, Alexander Sverdlin.
Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
Hi! On 10/03/2021 17:14, Florian Fainelli wrote: >>>>> I tried on 5.12-rc2 and 5.11 but couldn't reproduce the problem using your >>> I still can't reproduce on 5.12-rc2. >>> >>> I do have CONFIG_ARM_MODULE_PLTS=y. Do you need to do something else after >>> loading the module? I tried starting ftrace, but maybe there's a particular >>> combination required? >> You need to load a BIG module, so big that it has no place in the modules >> area >> any more and goes to vmalloc area. > You absolutely need a very big module maybe more than one. When I tested > this, I could use the two proprietary modules (*sigh*) that I needed to > exercise against and loading one but not the other was not enough to > make the second module loading spill into vmalloc space. Here is what I use instead of these real world "proprietary" modules (which of course were the real trigger for the patch): https://www.spinics.net/lists/arm-kernel/msg878599.html -- Best regards, Alexander Sverdlin.
Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
Hi! On 09/03/2021 18:42, Qais Yousef wrote: >>> I tried on 5.12-rc2 and 5.11 but couldn't reproduce the problem using your > I still can't reproduce on 5.12-rc2. > > I do have CONFIG_ARM_MODULE_PLTS=y. Do you need to do something else after > loading the module? I tried starting ftrace, but maybe there's a particular > combination required? You need to load a BIG module, so big that it has no place in the modules area any more and goes to vmalloc area. >>> instructions on the other email. But most likely because I'm hitting another >>> problem that could be masking it. I'm not sure it is related or just >>> randomly >>> happened to hit it. >>> >>> Did you see something similar? >> [...] >> >>> [0.00] [] (ftrace_bug) from [] >>> (ftrace_process_locs+0x2b0/0x518) >>> [0.00] r7:c3817ac4 r6:c38040c0 r5:0a3c r4:000134e4 >>> [0.00] [] (ftrace_process_locs) from [] >>> (ftrace_init+0xc8/0x174) >>> [0.00] r10:c2ffa000 r9:c2be8a78 r8:c2c5d1fc r7:c2c0c208 >>> r6:0001 r5:c2d0908c >>> [0.00] r4:c362f518 >>> [0.00] [] (ftrace_init) from [] >>> (start_kernel+0x2f4/0x5b8) >>> [0.00] r9:c2be8a78 r8:dbfffec0 r7: r6:c36385cc >>> r5:c2d08f00 r4:c2ffa000 >>> [0.00] [] (start_kernel) from [<>] (0x0) >> This means, FTRACE has more problems with your kernel/compiler/platform, >> I've addressed similar issue >> in the past, but my patch should be long merged: >> >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1817963.html >> >> Could it be the same problem as here: >> https://www.spinics.net/lists/arm-kernel/msg854022.html >> >> Seems that the size check deserves something line BUILD_BUG_ON() with >> FTRACE... > So I only see this when I convert all modules to be built-in > > sed -i 's/=m/=y/' .config > > FWIW, I see the problem with your patch applied too. Trying to dig more into > it.. Then it's definitely the problem explained in the second link. If you have THUMB2 kernel, maybe you have to switch to ARM. -- Best regards, Alexander Sverdlin.
Re: [PATCH v7 2/2] ARM: ftrace: Add MODULE_PLTS support
ake_call(struct dyn_ftrace *rec, unsigned
>> long addr)
>> {
>> unsigned long new, old;
>> unsigned long ip = rec->ip;
>> +unsigned long aaddr = adjust_address(rec, addr);
>>
>> old = ftrace_nop_replace(rec);
>>
>> -new = ftrace_call_replace(ip, adjust_address(rec, addr));
>> +new = ftrace_call_replace(ip, aaddr);
>> +
>> +#ifdef CONFIG_ARM_MODULE_PLTS
>> +if (!new) {
>> +struct module *mod = rec->arch.mod;
>> +
>> +if (mod) {
>
> What would happen if !new and !mod?
I believe, that's exactly what happens in the dump you experience with your
kernel.
This is not covered by this patch, this patch covers the issue with modules in
vmalloc area.
>> +aaddr = get_module_plt(mod, ip, aaddr);
>> +new = ftrace_call_replace(ip, aaddr);
>
> I assume we're guaranteed to have a sensible value returned in 'new' here?
Otherwise you'd see the dump you see :)
It relies on the already existing error handling.
>> +}
>> +}
>> +#endif
--
Best regards,
Alexander Sverdlin.
Re: [PATCH v7 0/2] ARM: Implement MODULE_PLT support in FTRACE
Hello Linus,
On 02/03/2021 09:29, Linus Walleij wrote:
>>>> FTRACE's function tracer currently doesn't always work on ARM with
>>>> MODULE_PLT option enabled. If the module is loaded too far, FTRACE's
First of all:
config ARM_MODULE_PLTS
bool "Use PLTs to allow module memory to spill over into vmalloc area"
[...]
> Any suggestions on a quick use case that illustrates how the problem
> manifest and how to test it is gone? The errors in patch 2, what do
> I need to configure in to get them? Does it manifest at modprobe?
And then I use this module to test for the problem:
/******
* Author: Alexander Sverdlin
*
* Copyright (c) 2018 Nokia
*
* SPDX-License-Identifier: GPL-2.0
*
* This module is intended to test ARM MODULE_PLT functionality
**/
#include
#include
#include
static int fatmod_init(void)
{
asm volatile (".rept (6 * 1024 * 1024 / 2)\n\t"
"nop\n\t"
".endr");
return 0;
}
module_init(fatmod_init);
static void __exit fatmod_exit(void)
{
}
module_exit(fatmod_exit);
MODULE_AUTHOR("Alexander Sverdlin ");
MODULE_DESCRIPTION("ARM MODULE_PLT test module");
MODULE_LICENSE("GPL");
--
Best regards,
Alexander Sverdlin.
[PATCH] gpio: omap: Honor "aliases" node
Currently the naming of the GPIO chips depends on their order in the DT,
but also on the kernel version (I've noticed the change from v5.10.x to
v5.11). Honor the persistent enumeration in the "aliases" node like other
GPIO drivers do.
Signed-off-by: Alexander Sverdlin
---
Yes, I noticed checkpatch "WARNING: DT binding docs and includes should be
a separate patch."
However, the parts below are tiny and barely make sense separately.
Documentation/devicetree/bindings/gpio/gpio-omap.txt | 6 ++
drivers/gpio/gpio-omap.c | 5 +
2 files changed, 11 insertions(+)
diff --git a/Documentation/devicetree/bindings/gpio/gpio-omap.txt
b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
index e57b2cb28f6c..6050db3fd84e 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-omap.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-omap.txt
@@ -30,9 +30,15 @@ OMAP specific properties:
- ti,gpio-always-on: Indicates if a GPIO bank is always powered and
so will never lose its logic state.
+Note: GPIO ports can have an alias correctly numbered in "aliases" node for
+persistent enumeration.
Example:
+aliases {
+ gpio0 =
+};
+
gpio0: gpio@44e07000 {
compatible = "ti,omap4-gpio";
reg = <0x44e07000 0x1000>;
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 41952bb818ad..dd2a8f6d920f 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1014,6 +1014,11 @@ static int omap_gpio_chip_init(struct gpio_bank *bank,
struct irq_chip *irqc)
bank->chip.parent = _mpuio_device.dev;
bank->chip.base = OMAP_MPUIO(0);
} else {
+#ifdef CONFIG_OF_GPIO
+ ret = of_alias_get_id(bank->chip.of_node, "gpio");
+ if (ret >= 0)
+ gpio = ret * bank->width;
+#endif
label = devm_kasprintf(bank->chip.parent, GFP_KERNEL,
"gpio-%d-%d",
gpio, gpio + bank->width - 1);
if (!label)
--
2.30.1
Re: [PATCH] ARM: ep93xx: don't use clang IAS for crunch
Hi! On Fri, 2021-02-26 at 17:43 +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > Randconfig builds with ep93xx fail with the clang integrated > assembler that does not understand the maverick crunch extensions: > > arch/arm/mach-ep93xx/crunch-bits.S:94:2: error: invalid instruction > cfstr64 mvdx0, [r1, #0] @ save 64b registers > > It is unclear if anyone is still using support for crunch: gcc-4.8 dropped > it in 2012 when it was already too broken to be used reliabled. glibc > support existed as an external patch but was never merged upstream. > We could consider removing the last bits of the kernel support as well. This was my impression already in 2006, that Cirrus is not going to work on Crunch support. From my PoV it's OK to remove the support in the kernel completely. > Turn off the integrated assembler for this file for now. > > Signed-off-by: Arnd Bergmann > --- > arch/arm/mach-ep93xx/Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mach-ep93xx/Makefile b/arch/arm/mach-ep93xx/Makefile > index 86768495f61d..f686577ba059 100644 > --- a/arch/arm/mach-ep93xx/Makefile > +++ b/arch/arm/mach-ep93xx/Makefile > @@ -7,7 +7,7 @@ obj-y := core.o clock.o timer-ep93xx.o > obj-$(CONFIG_EP93XX_DMA) += dma.o > > obj-$(CONFIG_CRUNCH) += crunch.o crunch-bits.o > -AFLAGS_crunch-bits.o := -Wa,-mcpu=ep9312 > +AFLAGS_crunch-bits.o := -Wa,-mcpu=ep9312 $(cc-option, > -fno-integrated-as) > > obj-$(CONFIG_MACH_ADSSPHERE) += adssphere.o > obj-$(CONFIG_MACH_EDB93XX) += edb93xx.o -- Alexander Sverdlin.
[PATCH] spi: omap2-mcspi: Activate pinctrl idle state during runtime suspend
Set the (optional) idle pinctrl state during runtime suspend. This is the
same schema used in PL022 driver and can help with HW designs sharing
the SPI lines for different purposes.
Signed-off-by: Alexander Sverdlin
---
drivers/spi/spi-omap2-mcspi.c | 24 ++--
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index d4c9510af393..999c22736416 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -1327,6 +1327,17 @@ static int omap2_mcspi_controller_setup(struct
omap2_mcspi *mcspi)
return 0;
}
+static int omap_mcspi_runtime_suspend(struct device *dev)
+{
+ int error;
+
+ error = pinctrl_pm_select_idle_state(dev);
+ if (error)
+ dev_warn(dev, "%s: failed to set pins: %i\n", __func__, error);
+
+ return 0;
+}
+
/*
* When SPI wake up from off-mode, CS is in activate state. If it was in
* inactive state when driver was suspend, then force it to inactive state at
@@ -1338,6 +1349,11 @@ static int omap_mcspi_runtime_resume(struct device *dev)
struct omap2_mcspi *mcspi = spi_master_get_devdata(master);
struct omap2_mcspi_regs *ctx = >ctx;
struct omap2_mcspi_cs *cs;
+ int error;
+
+ error = pinctrl_pm_select_default_state(dev);
+ if (error)
+ dev_warn(dev, "%s: failed to set pins: %i\n", __func__, error);
/* McSPI: context restore */
mcspi_write_reg(master, OMAP2_MCSPI_MODULCTRL, ctx->modulctrl);
@@ -1566,11 +1582,6 @@ static int __maybe_unused omap2_mcspi_resume(struct
device *dev)
struct omap2_mcspi *mcspi = spi_master_get_devdata(master);
int error;
- error = pinctrl_pm_select_default_state(dev);
- if (error)
- dev_warn(mcspi->dev, "%s: failed to set pins: %i\n",
-__func__, error);
-
error = spi_master_resume(master);
if (error)
dev_warn(mcspi->dev, "%s: master resume failed: %i\n",
@@ -1582,7 +1593,8 @@ static int __maybe_unused omap2_mcspi_resume(struct
device *dev)
static const struct dev_pm_ops omap2_mcspi_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(omap2_mcspi_suspend,
omap2_mcspi_resume)
- .runtime_resume = omap_mcspi_runtime_resume,
+ .runtime_suspend= omap_mcspi_runtime_suspend,
+ .runtime_resume = omap_mcspi_runtime_resume,
};
static struct platform_driver omap2_mcspi_driver = {
--
2.29.2
[PATCH] leds: trigger: timer: Optionally stop timer trigger on reboot
This functionality is similar to heartbeat and activity triggers and
turns the timer-triggered LEDs off right before reboot. It's configurable
via new module parameter "reboot_off" to preserve original behaviour.
Signed-off-by: Alexander Sverdlin
---
drivers/leds/trigger/ledtrig-timer.c | 39 +++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/trigger/ledtrig-timer.c
b/drivers/leds/trigger/ledtrig-timer.c
index 7c14983781ee..3eadcb0a629a 100644
--- a/drivers/leds/trigger/ledtrig-timer.c
+++ b/drivers/leds/trigger/ledtrig-timer.c
@@ -16,6 +16,11 @@
#include
#include
#include
+#include
+
+static bool reboot_off;
+module_param(reboot_off, bool, 0444);
+MODULE_PARM_DESC(reboot_off, "Switch LED off on reboot");
static ssize_t led_delay_on_show(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -97,7 +102,39 @@ static struct led_trigger timer_led_trigger = {
.deactivate = timer_trig_deactivate,
.groups = timer_trig_groups,
};
-module_led_trigger(timer_led_trigger);
+
+static int timer_reboot_notifier(struct notifier_block *nb, unsigned long code,
+void *unused)
+{
+ led_trigger_unregister(_led_trigger);
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block timer_reboot_nb = {
+ .notifier_call = timer_reboot_notifier,
+};
+
+static int __init timer_trig_init(void)
+{
+ int ret;
+
+ ret = led_trigger_register(_led_trigger);
+ if (ret)
+ return ret;
+ if (reboot_off)
+ register_reboot_notifier(_reboot_nb);
+ return 0;
+}
+
+static void __exit timer_trig_exit(void)
+{
+ /* Not afraid of -ENOENT */
+ unregister_reboot_notifier(_reboot_nb);
+ led_trigger_unregister(_led_trigger);
+}
+
+module_init(timer_trig_init);
+module_exit(timer_trig_exit);
MODULE_AUTHOR("Richard Purdie ");
MODULE_DESCRIPTION("Timer LED trigger");
--
2.19.1
Re: [PATCH v6 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips
Hello Nikita!
On Tue, 2021-02-09 at 16:31 +0300, Nikita Shubin wrote:
> Fixes the following warnings which results in interrupts disabled on
> port B/F:
>
> gpio gpiochip1: (B): detected irqchip that is shared with multiple gpiochips:
> please fix the driver.
> gpio gpiochip5: (F): detected irqchip that is shared with multiple gpiochips:
> please fix the driver.
>
> - added separate irqchip for each interrupt capable gpiochip
> - provided unique names for each irqchip
>
> Fixes: d2b091961510 ("gpio: ep93xx: Pass irqchip when adding gpiochip")
> Signed-off-by: Nikita Shubin
Tested-by: Alexander Sverdlin
> ---
> v5->v6:
> - add devm_kasprintf() return value check and move it out from
> ep93xx_init_irq_chip()
> - removed ep93xx_gpio_irq_chip
> - pass girq->chip instead of removed ep93xx_gpio_irq_chip to
> irq_set_chip_and_handler for port F
> ---
> drivers/gpio/gpio-ep93xx.c | 30 +++---
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 64d6c2b4282e..94d9fa0d6aa7 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -38,6 +38,7 @@
> #define EP93XX_GPIO_F_IRQ_BASE 80
>
> struct ep93xx_gpio_irq_chip {
> + struct irq_chip ic;
> u8 irq_offset;
> u8 int_unmasked;
> u8 int_enabled;
> @@ -263,15 +264,6 @@ static int ep93xx_gpio_irq_type(struct irq_data *d,
> unsigned int type)
> return 0;
> }
>
> -static struct irq_chip ep93xx_gpio_irq_chip = {
> - .name = "GPIO",
> - .irq_ack= ep93xx_gpio_irq_ack,
> - .irq_mask_ack = ep93xx_gpio_irq_mask_ack,
> - .irq_mask = ep93xx_gpio_irq_mask,
> - .irq_unmask = ep93xx_gpio_irq_unmask,
> - .irq_set_type = ep93xx_gpio_irq_type,
> -};
> -
> /*
> * gpiolib interface for EP93xx on-chip GPIOs
> */
> @@ -331,6 +323,15 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc,
> unsigned offset)
> return EP93XX_GPIO_F_IRQ_BASE + offset;
> }
>
> +static void ep93xx_init_irq_chip(struct device *dev, struct irq_chip *ic)
> +{
> + ic->irq_ack = ep93xx_gpio_irq_ack;
> + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack;
> + ic->irq_mask = ep93xx_gpio_irq_mask;
> + ic->irq_unmask = ep93xx_gpio_irq_unmask;
> + ic->irq_set_type = ep93xx_gpio_irq_type;
> +}
> +
> static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
> struct platform_device *pdev,
> struct ep93xx_gpio *epg,
> @@ -352,6 +353,8 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip
> *egc,
>
> girq = >irq;
> if (bank->has_irq || bank->has_hierarchical_irq) {
> + struct irq_chip *ic;
> +
> gc->set_config = ep93xx_gpio_set_config;
> egc->eic = devm_kcalloc(dev, 1,
> sizeof(*egc->eic),
> @@ -359,7 +362,12 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip
> *egc,
> if (!egc->eic)
> return -ENOMEM;
> egc->eic->irq_offset = bank->irq;
> - girq->chip = _gpio_irq_chip;
> + ic = >eic->ic;
> + ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s",
> bank->label);
> + if (!ic->name)
> + return -ENOMEM;
> + ep93xx_init_irq_chip(dev, ic);
> + girq->chip = ic;
> }
>
> if (bank->has_irq) {
> @@ -401,7 +409,7 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip
> *egc,
> gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
> irq_set_chip_data(gpio_irq, >gc[5]);
> irq_set_chip_and_handler(gpio_irq,
> - _gpio_irq_chip,
> + girq->chip,
> handle_level_irq);
> irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST);
> }
--
Alexander Sverdlin.
Re: [PATCH v5 2/7] gpio: ep93xx: Fix single irqchip with multi gpiochips
Hello Nikita,
On Mon, 2021-02-08 at 11:59 +0300, Nikita Shubin wrote:
> Fixes the following warnings which results in interrupts disabled on
> port B/F:
>
> gpio gpiochip1: (B): detected irqchip that is shared with multiple gpiochips:
> please fix the driver.
> gpio gpiochip5: (F): detected irqchip that is shared with multiple gpiochips:
> please fix the driver.
>
> - added separate irqchip for each interrupt capable gpiochip
> - provided unique names for each irqchip
>
> Fixes: d2b091961510 ("gpio: ep93xx: Pass irqchip when adding gpiochip")
> Signed-off-by: Nikita Shubin
I performed a bootup with the whole patch-series and
confirm that the warning is gone. Thank you for looking into this!
Reviewed-by: Alexander Sverdlin
Tested-by: Alexander Sverdlin
> ---
> v4->v5:
> - generate IRQ chip's names dynamicaly from label
> ---
> drivers/gpio/gpio-ep93xx.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 64d6c2b4282e..3d8eb8769470 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -38,6 +38,7 @@
> #define EP93XX_GPIO_F_IRQ_BASE 80
>
> struct ep93xx_gpio_irq_chip {
> + struct irq_chip ic;
> u8 irq_offset;
> u8 int_unmasked;
> u8 int_enabled;
> @@ -331,6 +332,16 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc,
> unsigned offset)
> return EP93XX_GPIO_F_IRQ_BASE + offset;
> }
>
> +static void ep93xx_init_irq_chip(struct device *dev, struct irq_chip *ic,
> const char *label)
> +{
> + ic->name = devm_kasprintf(dev, GFP_KERNEL, "gpio-irq-%s", label);
> + ic->irq_ack = ep93xx_gpio_irq_ack;
> + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack;
> + ic->irq_mask = ep93xx_gpio_irq_mask;
> + ic->irq_unmask = ep93xx_gpio_irq_unmask;
> + ic->irq_set_type = ep93xx_gpio_irq_type;
> +}
> +
> static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
> struct platform_device *pdev,
> struct ep93xx_gpio *epg,
> @@ -352,6 +363,8 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip
> *egc,
>
> girq = >irq;
> if (bank->has_irq || bank->has_hierarchical_irq) {
> + struct irq_chip *ic;
> +
> gc->set_config = ep93xx_gpio_set_config;
> egc->eic = devm_kcalloc(dev, 1,
> sizeof(*egc->eic),
> @@ -359,7 +372,9 @@ static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip
> *egc,
> if (!egc->eic)
> return -ENOMEM;
> egc->eic->irq_offset = bank->irq;
> - girq->chip = _gpio_irq_chip;
> + ic = >eic->ic;
> + ep93xx_init_irq_chip(dev, ic, bank->label);
> + girq->chip = ic;
> }
>
> if (bank->has_irq) {
--
Alexander Sverdlin.
Re: [PATCH v5 1/7] gpio: ep93xx: fix BUG_ON port F usage
Hi Nikita!
On Mon, 2021-02-08 at 11:59 +0300, Nikita Shubin wrote:
> Two index spaces and ep93xx_gpio_port are confusing.
>
> Instead add a separate struct to store necessary data and remove
> ep93xx_gpio_port.
>
> - add struct to store IRQ related data for each IRQ capable chip
> - replace offset array with defined offsets
> - add IRQ registers offset for each IRQ capable chip into
> ep93xx_gpio_banks
>
> [ cut here ]
> kernel BUG at drivers/gpio/gpio-ep93xx.c:64!
> ---[ end trace 3f6544e133e9f5ae ]---
>
> Fixes: fd935fc421e74 ("gpio: ep93xx: Do not pingpong irq numbers")
> Signed-off-by: Nikita Shubin
I performed a bootup with the whole patch-series and
confirm that the BUG is gone.
Reviewed-by: Alexander Sverdlin
Tested-by: Alexander Sverdlin
> ---
> v4->v5:
> - make to_ep93xx_gpio_irq_chip() static
> ---
> drivers/gpio/gpio-ep93xx.c | 186 -
> 1 file changed, 99 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 226da8df6f10..64d6c2b4282e 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -25,6 +25,9 @@
> /* Maximum value for gpio line identifiers */
> #define EP93XX_GPIO_LINE_MAX 63
>
> +/* Number of GPIO chips in EP93XX */
> +#define EP93XX_GPIO_CHIP_NUM 8
> +
> /* Maximum value for irq capable line identifiers */
> #define EP93XX_GPIO_LINE_MAX_IRQ 23
>
> @@ -34,74 +37,74 @@
> */
> #define EP93XX_GPIO_F_IRQ_BASE 80
>
> -struct ep93xx_gpio {
> - void __iomem*base;
> - struct gpio_chipgc[8];
> +struct ep93xx_gpio_irq_chip {
> + u8 irq_offset;
> + u8 int_unmasked;
> + u8 int_enabled;
> + u8 int_type1;
> + u8 int_type2;
> + u8 int_debounce;
> };
>
> -/*
> - * Interrupt handling for EP93xx on-chip GPIOs
> - */
> -static unsigned char gpio_int_unmasked[3];
> -static unsigned char gpio_int_enabled[3];
> -static unsigned char gpio_int_type1[3];
> -static unsigned char gpio_int_type2[3];
> -static unsigned char gpio_int_debounce[3];
> -
> -/* Port ordering is: A B F */
> -static const u8 int_type1_register_offset[3] = { 0x90, 0xac, 0x4c };
> -static const u8 int_type2_register_offset[3] = { 0x94, 0xb0, 0x50 };
> -static const u8 eoi_register_offset[3] = { 0x98, 0xb4, 0x54 };
> -static const u8 int_en_register_offset[3] = { 0x9c, 0xb8, 0x58 };
> -static const u8 int_debounce_register_offset[3]= { 0xa8, 0xc4, 0x64
> };
> -
> -static void ep93xx_gpio_update_int_params(struct ep93xx_gpio *epg, unsigned
> port)
> -{
> - BUG_ON(port > 2);
> +struct ep93xx_gpio_chip {
> + struct gpio_chipgc;
> + struct ep93xx_gpio_irq_chip *eic;
> +};
>
> - writeb_relaxed(0, epg->base + int_en_register_offset[port]);
> +struct ep93xx_gpio {
> + void __iomem*base;
> + struct ep93xx_gpio_chip gc[EP93XX_GPIO_CHIP_NUM];
> +};
>
> - writeb_relaxed(gpio_int_type2[port],
> - epg->base + int_type2_register_offset[port]);
> +#define to_ep93xx_gpio_chip(x) container_of(x, struct ep93xx_gpio_chip, gc)
>
> - writeb_relaxed(gpio_int_type1[port],
> - epg->base + int_type1_register_offset[port]);
> +static struct ep93xx_gpio_irq_chip *to_ep93xx_gpio_irq_chip(struct gpio_chip
> *gc)
> +{
> + struct ep93xx_gpio_chip *egc = to_ep93xx_gpio_chip(gc);
>
> - writeb(gpio_int_unmasked[port] & gpio_int_enabled[port],
> - epg->base + int_en_register_offset[port]);
> + return egc->eic;
> }
>
> -static int ep93xx_gpio_port(struct gpio_chip *gc)
> +/*
> + * Interrupt handling for EP93xx on-chip GPIOs
> + */
> +#define EP93XX_INT_TYPE1_OFFSET0x00
> +#define EP93XX_INT_TYPE2_OFFSET0x04
> +#define EP93XX_INT_EOI_OFFSET 0x08
> +#define EP93XX_INT_EN_OFFSET 0x0c
> +#define EP93XX_INT_STATUS_OFFSET 0x10
> +#define EP93XX_INT_RAW_STATUS_OFFSET 0x14
> +#define EP93XX_INT_DEBOUNCE_OFFSET 0x18
> +
> +static void ep93xx_gpio_update_int_params(struct ep93xx_gpio *epg,
> + struct ep93xx_gpio_irq_chip *eic)
> {
> - struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> -
Re: [PATCH] staging: octeon: remove braces from single-line block
Hi!
On 06/02/2021 21:17, Phillip Potter wrote:
> This removes the braces from the if statement that checks the
> physical node return value in cvm_oct_phy_setup_device, as this
> block contains only one statement. Fixes a style warning.
>
> Signed-off-by: Phillip Potter
Reviewed-by: Alexander Sverdlin
> ---
> drivers/staging/octeon/ethernet-mdio.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/octeon/ethernet-mdio.c
> b/drivers/staging/octeon/ethernet-mdio.c
> index 0bf545849b11..b0fd083a5bf2 100644
> --- a/drivers/staging/octeon/ethernet-mdio.c
> +++ b/drivers/staging/octeon/ethernet-mdio.c
> @@ -146,9 +146,8 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
> goto no_phy;
>
> phy_node = of_parse_phandle(priv->of_node, "phy-handle", 0);
> - if (!phy_node && of_phy_is_fixed_link(priv->of_node)) {
> + if (!phy_node && of_phy_is_fixed_link(priv->of_node))
> phy_node = of_node_get(priv->of_node);
> - }
> if (!phy_node)
> goto no_phy;
>
--
Best regards,
Alexander Sverdlin.
Re: [PATCH v4 1/7] gpio: gpio-ep93xx: fix BUG_ON port F usage
ANK(_label, _data, _dir, _irq, _base, _has_irq,
> _has_hier, _irq_base) \
> { \
> .label = _label, \
> .data = _data,\
> .dir= _dir, \
> + .irq= _irq, \
> .base = _base,\
> .has_irq= _has_irq, \
> .has_hierarchical_irq = _has_hier, \
> @@ -295,16 +300,16 @@ struct ep93xx_gpio_bank {
>
> static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
> /* Bank A has 8 IRQs */
> - EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, 64),
> + EP93XX_GPIO_BANK("A", 0x00, 0x10, 0x90, 0, true, false, 64),
> /* Bank B has 8 IRQs */
> - EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72),
> - EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
> - EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
> - EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
> + EP93XX_GPIO_BANK("B", 0x04, 0x14, 0xac, 8, true, false, 72),
> + EP93XX_GPIO_BANK("C", 0x08, 0x18, 0x00, 40, false, false, 0),
> + EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 0x00, 24, false, false, 0),
> + EP93XX_GPIO_BANK("E", 0x20, 0x24, 0x00, 32, false, false, 0),
> /* Bank F has 8 IRQs */
> - EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0),
> - EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
> - EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
> + EP93XX_GPIO_BANK("F", 0x30, 0x34, 0x4c, 16, false, true, 0),
> + EP93XX_GPIO_BANK("G", 0x38, 0x3c, 0x00, 48, false, false, 0),
> + EP93XX_GPIO_BANK("H", 0x40, 0x44, 0x00, 56, false, false, 0),
> };
>
> static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned offset,
> @@ -326,13 +331,14 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc,
> unsigned offset)
> return EP93XX_GPIO_F_IRQ_BASE + offset;
> }
>
> -static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
> +static int ep93xx_gpio_add_bank(struct ep93xx_gpio_chip *egc,
> struct platform_device *pdev,
> struct ep93xx_gpio *epg,
> struct ep93xx_gpio_bank *bank)
> {
> void __iomem *data = epg->base + bank->data;
> void __iomem *dir = epg->base + bank->dir;
> + struct gpio_chip *gc = >gc;
> struct device *dev = >dev;
> struct gpio_irq_chip *girq;
> int err;
> @@ -347,6 +353,12 @@ static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
> girq = >irq;
> if (bank->has_irq || bank->has_hierarchical_irq) {
> gc->set_config = ep93xx_gpio_set_config;
> + egc->eic = devm_kcalloc(dev, 1,
> + sizeof(*egc->eic),
> + GFP_KERNEL);
> + if (!egc->eic)
> + return -ENOMEM;
> + egc->eic->irq_offset = bank->irq;
> girq->chip = _gpio_irq_chip;
> }
>
> @@ -415,7 +427,7 @@ static int ep93xx_gpio_probe(struct platform_device *pdev)
> return PTR_ERR(epg->base);
>
> for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) {
> - struct gpio_chip *gc = >gc[i];
> + struct ep93xx_gpio_chip *gc = >gc[i];
> struct ep93xx_gpio_bank *bank = _gpio_banks[i];
>
> if (ep93xx_gpio_add_bank(gc, pdev, epg, bank))
--
Alexander Sverdlin.
Re: [PATCH v4 2/7] gpio: gpio-ep93xx: Fix single irqchip with multi gpiochips
Hi!
On Fri, 2021-02-05 at 11:05 +0300, Nikita Shubin wrote:
> Fixes the following warnings which results in interrupts disabled on
> port B/F:
>
> gpio gpiochip1: (B): detected irqchip that is shared with multiple gpiochips:
> please fix the driver.
> gpio gpiochip5: (F): detected irqchip that is shared with multiple gpiochips:
> please fix the driver.
>
> - added separate irqchip for each interrupt capable gpiochip
> - provided unique names for each irqchip
>
> Fixes: d2b091961510 ("gpio: ep93xx: Pass irqchip when adding gpiochip")
> Signed-off-by: Nikita Shubin
> ---
> drivers/gpio/gpio-ep93xx.c | 38 --
> 1 file changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index e3b5e2c37259..3c9f7233e62d 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -38,6 +38,7 @@
> #define EP93XX_GPIO_F_IRQ_BASE 80
>
> struct ep93xx_gpio_irq_chip {
> + struct irq_chip ic;
> u8 irq_offset;
> u8 int_unmasked;
> u8 int_enabled;
> @@ -284,9 +285,11 @@ struct ep93xx_gpio_bank {
> boolhas_irq;
> boolhas_hierarchical_irq;
> unsigned intirq_base;
> + const char *irq_name;
> };
>
> -#define EP93XX_GPIO_BANK(_label, _data, _dir, _irq, _base, _has_irq,
> _has_hier, _irq_base) \
> +#define EP93XX_GPIO_BANK(_label, _data, _dir, _irq, _base, _has_irq,
> _has_hier, _irq_base, \
> +_irq_name) \
> { \
> .label = _label, \
> .data = _data,\
> @@ -296,20 +299,21 @@ struct ep93xx_gpio_bank {
> .has_irq= _has_irq, \
> .has_hierarchical_irq = _has_hier, \
> .irq_base = _irq_base,\
> + .irq_name = _irq_name,\
> }
>
> static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
> /* Bank A has 8 IRQs */
> - EP93XX_GPIO_BANK("A", 0x00, 0x10, 0x90, 0, true, false, 64),
> + EP93XX_GPIO_BANK("A", 0x00, 0x10, 0x90, 0, true, false, 64,
> "gpio-irq-a"),
> /* Bank B has 8 IRQs */
> - EP93XX_GPIO_BANK("B", 0x04, 0x14, 0xac, 8, true, false, 72),
> - EP93XX_GPIO_BANK("C", 0x08, 0x18, 0x00, 40, false, false, 0),
> - EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 0x00, 24, false, false, 0),
> - EP93XX_GPIO_BANK("E", 0x20, 0x24, 0x00, 32, false, false, 0),
> + EP93XX_GPIO_BANK("B", 0x04, 0x14, 0xac, 8, true, false, 72,
> "gpio-irq-b"),
> + EP93XX_GPIO_BANK("C", 0x08, 0x18, 0x00, 40, false, false, 0, 0),
> + EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 0x00, 24, false, false, 0, 0),
> + EP93XX_GPIO_BANK("E", 0x20, 0x24, 0x00, 32, false, false, 0, 0),
> /* Bank F has 8 IRQs */
> - EP93XX_GPIO_BANK("F", 0x30, 0x34, 0x4c, 16, false, true, 0),
> - EP93XX_GPIO_BANK("G", 0x38, 0x3c, 0x00, 48, false, false, 0),
> - EP93XX_GPIO_BANK("H", 0x40, 0x44, 0x00, 56, false, false, 0),
> + EP93XX_GPIO_BANK("F", 0x30, 0x34, 0x4c, 16, false, true, 0,
> "gpio-irq-b"),
This copy-paste error makes it obvious, that this information is
superfluous...
> + EP93XX_GPIO_BANK("G", 0x38, 0x3c, 0x00, 48, false, false, 0, 0),
> + EP93XX_GPIO_BANK("H", 0x40, 0x44, 0x00, 56, false, false, 0, 0),
> };
>
> static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned offset,
> @@ -331,6 +335,16 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc,
> unsigned offset)
> return EP93XX_GPIO_F_IRQ_BASE + offset;
> }
>
> +static void ep93xx_init_irq_chip(struct irq_chip *ic, const char *irq_name)
> +{
> + ic->name = irq_name;
Assuming you pass "label" here, you could do something like
ic->name = kasprintf(GFP_KERNEL, "gpio-irq-%s", irq_name);
> + ic->irq_ack = ep93xx_gpio_irq_ack;
> + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack;
> + ic->irq_mask = ep93xx_gpio_irq_mask;
> + ic->irq_unmask = ep93xx_gpio_irq_unmask;
> + ic->irq_set_type = ep93xx_gpio_irq_type;
> +}
--
Alexander Sverdlin.
Re: [PATCH v3 1/7] gpio: gpio-ep93xx: fix BUG_ON port F usage
Hi Nikita,
On Thu, 2021-02-04 at 17:00 +0300, nikita.shu...@maquefel.me wrote:
> > I considered your offer of using array with holes.
> >
> > It looks pretty ugly to me, couse it leads to bloated arrays:
> >
> > static unsigned char gpio_int_unmasked[EP93XX_GPIO_CHIP_NUM];
> > static unsigned char gpio_int_enabled[EP93XX_GPIO_CHIP_NUM];
> > static unsigned char gpio_int_type1[EP93XX_GPIO_CHIP_NUM];
> > static unsigned char gpio_int_type2[EP93XX_GPIO_CHIP_NUM];
> > static unsigned char gpio_int_debounce[EP93XX_GPIO_CHIP_NUM];
> >
> > /* Port ordering is: A B F */
> > static const u8 int_type1_register_offset[EP93XX_GPIO_CHIP_NUM] = {
> > 0x90, 0xac, 0x0, 0x0, 0x0, 0x4c };
> > static const u8 int_type2_register_offset[EP93XX_GPIO_CHIP_NUM] = {
> > 0x94, 0xb0, 0x0, 0x0, 0x0, 0x50 };
> > static const u8 eoi_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x98,
> > 0xb4, 0x0, 0x0, 0x0, 0x54 };
> > static const u8 int_en_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x9c,
> > 0xb8, 0x0, 0x0, 0x0, 0x58 };
> > static const u8 int_debounce_register_offset[EP93XX_GPIO_CHIP_NUM] = {
> > 0xa8, 0xc4, 0x0, 0x0, 0x0, 0x64 };
> >
> > Is this really the thing we want ?
>
> Even in this form it's less error-prone than to have two
> index-spaces, and hidden conversion from one numbering scheme
> to other.
>
> Alternatives that I see are:
> 1.
> https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
>
> 2.
> Embedd the necessary values into struct ep93xx_gpio_bank.
> This option can probably simplify the handling of the names
> for irq chips as well.
> Thank you very much for your comments, and how about a 3rd option ? :
>
> It also makes easier to add 'struct irqchip' in following patch.
> struct ep93xx_gpio_irq_chip {
> u8 irq_offset;
> u8 int_unmasked;
> u8 int_enabled;
> u8 int_type1;
> u8 int_type2;
> u8 int_debounce;
> };
>
> struct ep93xx_gpio_chip {
> struct gpio_chip gc;
> struct ep93xx_gpio_irq_chip *eic;
> };
>
> struct ep93xx_gpio {
> void __iomem *base;
> struct ep93xx_gpio_chip gc[8];
> };
>
> static const u8 int_register_offset[8] = { 0x90, 0xac, [5] = 0x4c };
> #define EP93XX_INT_TYPE1_OFFSET 0x00
> #define EP93XX_INT_TYPE2_OFFSET 0x04
> #define EP93XX_INT_EOI_OFFSET 0x08
> #define EP93XX_INT_EN_OFFSET 0x0c
> #define EP93XX_INT_STATUS_OFFSET 0x10
> #define EP93XX_INT_RAW_STATUS_OFFSET 0x14
> #define EP93XX_INT_DEBOUNCE_OFFSET 0x18
Makes sense to me.
--
Alexander Sverdlin.
Re: [PATCH v3 1/7] gpio: gpio-ep93xx: fix BUG_ON port F usage
Hi Nikita,
On Thu, 2021-02-04 at 15:55 +0300, nikita.shu...@maquefel.me wrote:
> I considered your offer of using array with holes.
>
> It looks pretty ugly to me, couse it leads to bloated arrays:
>
> static unsigned char gpio_int_unmasked[EP93XX_GPIO_CHIP_NUM];
> static unsigned char gpio_int_enabled[EP93XX_GPIO_CHIP_NUM];
> static unsigned char gpio_int_type1[EP93XX_GPIO_CHIP_NUM];
> static unsigned char gpio_int_type2[EP93XX_GPIO_CHIP_NUM];
> static unsigned char gpio_int_debounce[EP93XX_GPIO_CHIP_NUM];
>
> /* Port ordering is: A B F */
> static const u8 int_type1_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x90,
> 0xac, 0x0, 0x0, 0x0, 0x4c };
> static const u8 int_type2_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x94,
> 0xb0, 0x0, 0x0, 0x0, 0x50 };
> static const u8 eoi_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x98, 0xb4,
> 0x0, 0x0, 0x0, 0x54 };
> static const u8 int_en_register_offset[EP93XX_GPIO_CHIP_NUM] = { 0x9c,
> 0xb8, 0x0, 0x0, 0x0, 0x58 };
> static const u8 int_debounce_register_offset[EP93XX_GPIO_CHIP_NUM] = {
> 0xa8, 0xc4, 0x0, 0x0, 0x0, 0x64 };
>
> Is this really the thing we want ?
Even in this form it's less error-prone than to have two
index-spaces, and hidden conversion from one numbering scheme
to other.
Alternatives that I see are:
1.
https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
2.
Embedd the necessary values into struct ep93xx_gpio_bank.
This option can probably simplify the handling of the names
for irq chips as well.
> 28.01.2021, 19:19, "Alexander Sverdlin" :
> > Hello Nikita,
> >
> > On Thu, 2021-01-28 at 18:11 +0200, Andy Shevchenko wrote:
> > > > +/*
> > > > + * F Port index in GPIOCHIP'S array is 5
> > > > + * but we use index 2 for stored values and offsets
> > > > + */
> > > > +#define EP93XX_GPIO_F_PORT_INDEX 5
> > >
> > > Hmm... Why not to use an array with holes instead.
> > >
> > > ...
> > >
> > > > + if (port == EP93XX_GPIO_F_PORT_INDEX)
> > > > + port = 2;
> > >
> > > Sorry, but I'm not in favour of this as it adds confusion.
> > > See above for the potential way to solve.
> >
> > well, I was thinking the same yesterday. It just adds another
> > level on confusion into the code, which even the author got
> > wrong :)
> >
> > Array with holes would be more obvious, but one can also embedd
> > the necessary values into struct ep93xx_gpio_bank.
> >
> > --
> > Alexander Sverdlin.
> >
> >
--
Alexander Sverdlin.
Re: [PATCH v3 4/7] gpio: ep93xx: drop to_irq binding
Hi!
On Thu, 2021-01-28 at 15:21 +0300, Nikita Shubin wrote:
> As ->to_irq is redefined in gpiochip_add_irqchip, having it defined
> in
> driver is useless, so let's drop it.
>
> Signed-off-by: Nikita Shubin
Acked-by: Alexander Sverdlin
> ---
> drivers/gpio/gpio-ep93xx.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index dc88115e34da..ee1cb3b894db 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -337,11 +337,6 @@ static int ep93xx_gpio_set_config(struct
> gpio_chip *gc, unsigned offset,
> return 0;
> }
>
> -static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned
> offset)
> -{
> - return EP93XX_GPIO_F_IRQ_BASE + offset;
> -}
> -
> static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
> {
> int i;
> @@ -429,7 +424,6 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
> }
> girq->default_type = IRQ_TYPE_NONE;
> girq->handler = handle_level_irq;
> - gc->to_irq = ep93xx_gpio_f_to_irq;
> girq->first = EP93XX_GPIO_F_IRQ_BASE;
> }
>
--
Alexander Sverdlin.
Re: [PATCH v3 6/7] gpio: ep93xx: refactor ep93xx_gpio_add_bank
On Thu, 2021-01-28 at 15:21 +0300, Nikita Shubin wrote:
> - replace plain numbers with girq->num_parents in devm_kcalloc
> - replace plain numbers with girq->num_parents for port F
> - refactor i - 1 to i + 1 to make loop more readable
> - combine getting IRQ's loop and setting handler's into single loop
>
> Signed-off-by: Nikita Shubin
Acked-by: Alexander Sverdlin
> ---
> v2->v3
> - use ->num_parents instead of ARRAY_SIZE()
> ---
> drivers/gpio/gpio-ep93xx.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index d69ec09cd618..df55aa13bd9a 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -384,7 +384,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>
> girq->parent_handler = ep93xx_gpio_ab_irq_handler;
> girq->num_parents = 1;
> - girq->parents = devm_kcalloc(dev, 1,
> + girq->parents = devm_kcalloc(dev, girq->num_parents,
> sizeof(*girq->parents),
> GFP_KERNEL);
> if (!girq->parents)
> @@ -406,15 +406,14 @@ static int ep93xx_gpio_add_bank(struct
> gpio_chip *gc,
> */
> girq->parent_handler = ep93xx_gpio_f_irq_handler;
> girq->num_parents = 8;
> - girq->parents = devm_kcalloc(dev, 8,
> + girq->parents = devm_kcalloc(dev, girq->num_parents,
> sizeof(*girq->parents),
> GFP_KERNEL);
> if (!girq->parents)
> return -ENOMEM;
> /* Pick resources 1..8 for these IRQs */
> - for (i = 1; i <= 8; i++)
> - girq->parents[i - 1] = platform_get_irq(pdev,
> i);
> - for (i = 0; i < 8; i++) {
> + for (i = 0; i < girq->num_parents; i++) {
> + girq->parents[i] = platform_get_irq(pdev, i +
> 1);
> gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
> irq_set_chip_data(gpio_irq, >gc[5]);
> irq_set_chip_and_handler(gpio_irq,
--
Alexander Sverdlin.
Re: [PATCH v3 3/7] gpio: gpio-ep93xx: Fix wrong irq numbers in port F
Hi! On Thu, 2021-01-28 at 15:21 +0300, Nikita Shubin wrote: > Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE. > > So we need to specify girq->first otherwise: > > "If device tree is used, then first_irq will be 0 and > irqs get mapped dynamically on the fly" > > And that's not the thing we want. > > Signed-off-by: Nikita Shubin Acked-by: Alexander Sverdlin > --- > drivers/gpio/gpio-ep93xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index b990d37da143..dc88115e34da 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -430,6 +430,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > girq->default_type = IRQ_TYPE_NONE; > girq->handler = handle_level_irq; > gc->to_irq = ep93xx_gpio_f_to_irq; > + girq->first = EP93XX_GPIO_F_IRQ_BASE; > } > > return devm_gpiochip_add_data(dev, gc, epg); -- Alexander Sverdlin.
Re: [PATCH v3 7/7] gpiod: ep93xx: refactor base IRQ number
Hi!
On Thu, 2021-01-28 at 15:21 +0300, Nikita Shubin wrote:
> - use predefined constants instead of plain numbers
> - use provided bank IRQ number instead of defined constant
> for port F
>
> Signed-off-by: Nikita Shubin
Acked-by: Alexander Sverdlin
> ---
> drivers/gpio/gpio-ep93xx.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index df55aa13bd9a..b1501607e8ed 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -28,6 +28,8 @@
> /* Maximum value for irq capable line identifiers */
> #define EP93XX_GPIO_LINE_MAX_IRQ 23
>
> +#define EP93XX_GPIO_A_IRQ_BASE 64
> +#define EP93XX_GPIO_B_IRQ_BASE 72
> /*
> * Static mapping of GPIO bank F IRQS:
> * F0..F7 (16..24) to irq 80..87.
> @@ -311,14 +313,14 @@ struct ep93xx_gpio_bank {
>
> static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
> /* Bank A has 8 IRQs */
> - EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, 64),
> + EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false,
> EP93XX_GPIO_A_IRQ_BASE),
> /* Bank B has 8 IRQs */
> - EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72),
> + EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false,
> EP93XX_GPIO_B_IRQ_BASE),
> EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
> EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
> EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
> /* Bank F has 8 IRQs */
> - EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0),
> + EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true,
> EP93XX_GPIO_F_IRQ_BASE),
> EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
> EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
> };
> @@ -414,7 +416,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
> /* Pick resources 1..8 for these IRQs */
> for (i = 0; i < girq->num_parents; i++) {
> girq->parents[i] = platform_get_irq(pdev, i +
> 1);
> - gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
> + gpio_irq = bank->irq_base + i;
> irq_set_chip_data(gpio_irq, >gc[5]);
> irq_set_chip_and_handler(gpio_irq,
>
> _gpio_irq_chip,
> @@ -423,7 +425,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
> }
> girq->default_type = IRQ_TYPE_NONE;
> girq->handler = handle_level_irq;
> - girq->first = EP93XX_GPIO_F_IRQ_BASE;
> + girq->first = bank->irq_base;
> }
>
> return devm_gpiochip_add_data(dev, gc, epg);
--
Alexander Sverdlin.
Re: [PATCH v3 1/7] gpio: gpio-ep93xx: fix BUG_ON port F usage
Hello Nikita, On Thu, 2021-01-28 at 18:11 +0200, Andy Shevchenko wrote: > > +/* > > + * F Port index in GPIOCHIP'S array is 5 > > + * but we use index 2 for stored values and offsets > > + */ > > +#define EP93XX_GPIO_F_PORT_INDEX 5 > > Hmm... Why not to use an array with holes instead. > > ... > > > + if (port == EP93XX_GPIO_F_PORT_INDEX) > > + port = 2; > > Sorry, but I'm not in favour of this as it adds confusion. > See above for the potential way to solve. well, I was thinking the same yesterday. It just adds another level on confusion into the code, which even the author got wrong :) Array with holes would be more obvious, but one can also embedd the necessary values into struct ep93xx_gpio_bank. -- Alexander Sverdlin.
Re: [PATCH 3/6] MIPS: Octeon: qspinlock: Flush write buffer
Hi! On 28/01/2021 12:35, Peter Zijlstra wrote: >> My point was that original MIPS spinlocks had this write-buffer-flush and >> it got lost on the conversion to qspinlocks. The referenced commit just >> allows to see the last MIPS-specific implementation before deletion. > Hardware that needs a store-buffer flush after release is highly suspect > and needs big and explicit comments. Not vague hints. I have a feeling that you are not going to suggest the comments for the code and one has to guess what is it you have in mind? Do you think the proper approach would be to undelete MIPS spinlocks and make these broken qspinlocks a configurable option for MIPS? I don't even mind if they will be default option for those not interested in performance or latency. -- Best regards, Alexander Sverdlin.
Re: [PATCH 1/6] MIPS: Octeon: Implement __smp_store_release()
Hi!
On 28/01/2021 12:33, Peter Zijlstra wrote:
> On Thu, Jan 28, 2021 at 08:27:29AM +0100, Alexander Sverdlin wrote:
>
>>>> +#define __smp_store_release(p, v) \
>>>> +do {
>>>> \
>>>> + compiletime_assert_atomic_type(*p); \
>>>> + __smp_wmb();\
>>>> + __smp_rmb();\
>>>> + WRITE_ONCE(*p, v); \
>>>> +} while (0)
>>> This is wrong in general since smp_rmb() will only provide order between
>>> two loads and smp_store_release() is a store.
>>>
>>> If this is correct for all MIPS, this needs a giant comment on exactly
>>> how that smp_rmb() makes sense here.
>>
>> ... the macro is provided for Octeon only, and __smp_rmb() is actually a NOP
>> there, but I thought to "document" the flow of thoughts from the discussion
>> above by including it anyway.
>
> Random discussions on the internet do not absolve you from having to
> write coherent comments. Especially so where memory ordering is
> concerned.
I actually hoped you will remember the discussion you've participated 5 years
ago and (in my understanding) actually already agreed that the solution itself
is not broken:
https://lore.kernel.org/lkml/20151112180003.ge17...@twins.programming.kicks-ass.net/
Could you please just suggest the proper comment you expect to be added here,
because there is no doubts, you have much more experience here than me?
> This, from commit 6b07d38aaa52 ("MIPS: Octeon: Use optimized memory
> barrier primitives."):
>
> #define smp_mb__before_llsc() smp_wmb()
> #define __smp_mb__before_llsc() __smp_wmb()
>
> is also dodgy as hell and really wants a comment too. I'm not buying the
> Changelog of that commit either, __smp_mb__before_llsc should also
> ensure the LL cannot happen earlier, but SYNCW has no effect on loads.
> So what stops the load from being speculated?
>
>
--
Best regards,
Alexander Sverdlin.
Re: [PATCH 1/6] MIPS: Octeon: Implement __smp_store_release()
Hello Peter,
On 28/01/2021 12:33, Peter Zijlstra wrote:
> This, from commit 6b07d38aaa52 ("MIPS: Octeon: Use optimized memory
> barrier primitives."):
>
> #define smp_mb__before_llsc() smp_wmb()
> #define __smp_mb__before_llsc() __smp_wmb()
>
> is also dodgy as hell and really wants a comment too. I'm not buying the
> Changelog of that commit either, __smp_mb__before_llsc should also
> ensure the LL cannot happen earlier, but SYNCW has no effect on loads.
> So what stops the load from being speculated?
hmm, the commit message you point to above, says:
"Since Octeon does not do speculative reads, this functions as a full barrier."
--
Best regards,
Alexander Sverdlin.
Re: [PATCH v2 0/9] gpio: ep93xx: fixes series patch
Hello Linus, On Wed, 2021-01-27 at 22:54 +0100, Linus Walleij wrote: > > Series of patches to fix ep93xx gpio driver to make IRQ's working. > > > > The following are fix patches (these are enough to get gpio-ep93xx > > working with modern kernel): > > I see that there is a strange level of attention to patches to this > platform! > > Since you fix all my mistakes made in converting this driver > in the past I will just say: > Reviewed-by: Linus Walleij > > For all of them. > > There are some nitpicks from the reviewers to fix up but > overall this looks very very good. as we don't have a dedicated EP93xx tree, would you like to take the series in one of your trees? -- Alexander Sverdlin.
Re: [PATCH v2 2/9] gpio: ep93xx: Fix single irqchip with multi gpiochips
Hi!
On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> Fixes the following warnings which results in interrupts disabled on
> port B/F:
>
> gpio gpiochip1: (B): detected irqchip that is shared with multiple
> gpiochips: please fix the driver.
> gpio gpiochip5: (F): detected irqchip that is shared with multiple
> gpiochips: please fix the driver.
>
> - added separate irqchip for each interrupt capable gpiochip
> - provided unique names for each irqchip
> - reworked ep93xx_gpio_port to make it usable before chip_add_data
> in ep93xx_init_irq_chips
>
> Fixes: a8173820f441 ("gpio: gpiolib: Allow GPIO IRQs to lazy
> disable")
> Signed-off-by: Nikita Shubin
Yes, it indeed fixes the warnigs mentioned above,
Tested-by: Alexander Sverdlin
> ---
> drivers/gpio/gpio-ep93xx.c | 45 ++--
> --
> 1 file changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 0d0435c07a5a..2eea02c906e0 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -34,9 +34,12 @@
> */
> #define EP93XX_GPIO_F_IRQ_BASE 80
>
> +#define EP93XX_GPIO_IRQ_CHIPS_NUM 3
> +
> struct ep93xx_gpio {
> void __iomem*base;
> struct gpio_chipgc[8];
> + struct irq_chip ic[EP93XX_GPIO_IRQ_CHIPS_NUM];
> };
>
> /*
> @@ -55,6 +58,11 @@ static unsigned char gpio_int_type2[3];
> static unsigned char gpio_int_debounce[3];
>
> /* Port ordering is: A B F */
> +static const char * const irq_chip_names[] = {
> + "gpio-irq-a",
> + "gpio-irq-b",
> + "gpio-irq-f"
> +};
> static const u8 int_type1_register_offset[3] = { 0x90, 0xac, 0x4c
> };
> static const u8 int_type2_register_offset[3] = { 0x94, 0xb0, 0x50
> };
> static const u8 eoi_register_offset[3] = { 0x98, 0xb4, 0x54
> };
> @@ -77,9 +85,8 @@ static void ep93xx_gpio_update_int_params(struct
> ep93xx_gpio *epg, unsigned port
> epg->base + int_en_register_offset[port]);
> }
>
> -static int ep93xx_gpio_port(struct gpio_chip *gc)
> +static int ep93xx_gpio_port(struct ep93xx_gpio *epg, struct
> gpio_chip *gc)
> {
> - struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> int port = 0;
>
> while (port < ARRAY_SIZE(epg->gc) && gc != >gc[port])
> @@ -101,7 +108,7 @@ static void ep93xx_gpio_int_debounce(struct
> gpio_chip *gc,
> unsigned int offset, bool
> enable)
> {
> struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> - int port = ep93xx_gpio_port(gc);
> + int port = ep93xx_gpio_port(epg, gc);
> int port_mask = BIT(offset);
>
> if (enable)
> @@ -163,7 +170,7 @@ static void ep93xx_gpio_irq_ack(struct irq_data
> *d)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> - int port = ep93xx_gpio_port(gc);
> + int port = ep93xx_gpio_port(epg, gc);
> int port_mask = BIT(d->irq & 7);
>
> if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) {
> @@ -178,7 +185,7 @@ static void ep93xx_gpio_irq_mask_ack(struct
> irq_data *d)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> - int port = ep93xx_gpio_port(gc);
> + int port = ep93xx_gpio_port(epg, gc);
> int port_mask = BIT(d->irq & 7);
>
> if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH)
> @@ -194,7 +201,7 @@ static void ep93xx_gpio_irq_mask(struct irq_data
> *d)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> - int port = ep93xx_gpio_port(gc);
> + int port = ep93xx_gpio_port(epg, gc);
>
> gpio_int_unmasked[port] &= ~BIT(d->irq & 7);
> ep93xx_gpio_update_int_params(epg, port);
> @@ -204,7 +211,7 @@ static void ep93xx_gpio_irq_unmask(struct
> irq_data *d)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> - int port = ep93xx_gpio_port(gc);
> + int port = ep93xx_gpio_port(epg, gc);
>
> gpio_int_unmasked[port] |= BIT(d->irq & 7);
> ep93xx_gpio_update_int_params(epg, port);
> @@ -219,7 +226,7 @@ static int ep93xx_gpio_irq_type(struct irq_data
> *d, unsigned int type)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct ep
Re: [PATCH v2 3/9] gpio: ep93xx: Fix wrong irq numbers in port F
Hi! On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE. > > So we need to specify girq->first otherwise: > > "If device tree is used, then first_irq will be 0 and > irqs get mapped dynamically on the fly" > > And that's not the thing we want. > > Signed-off-by: Nikita Shubin I have no code out-of-the-box to test the GPIO interrupts on EP93xx, so I just did a bootup with this patch. But the change looks fine to me: Acked-by: Alexander Sverdlin > --- > drivers/gpio/gpio-ep93xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index 2eea02c906e0..9c3d049e5af7 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -430,6 +430,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > girq->default_type = IRQ_TYPE_NONE; > girq->handler = handle_level_irq; > gc->to_irq = ep93xx_gpio_f_to_irq; > + girq->first = EP93XX_GPIO_F_IRQ_BASE; > } > > return devm_gpiochip_add_data(dev, gc, epg); -- Alexander Sverdlin.
Re: [PATCH 1/2] qspinlock: Ensure writes are pushed out of core write buffer
Hi! On 27/01/2021 23:43, Peter Zijlstra wrote: > On Wed, Jan 27, 2021 at 09:01:08PM +0100, Alexander A Sverdlin wrote: >> From: Alexander Sverdlin >> >> Ensure writes are pushed out of core write buffer to prevent waiting code >> on another cores from spinning longer than necessary. > Our smp_wmb() as defined does not have that property. You're relying on > some arch specific details which do not belong in common code. Yes, my intention was SYNCW on Octeon, which by accident is smp_wmb(). Do you think that the core write buffer is only Octeon feature and there will be no others? Should I re-implement arch_mcs_spin_lock_contended() for Octeon only, as it has been done for ARM? >> diff --git a/kernel/locking/mcs_spinlock.h b/kernel/locking/mcs_spinlock.h >> index 5e10153..10e497a 100644 >> --- a/kernel/locking/mcs_spinlock.h >> +++ b/kernel/locking/mcs_spinlock.h >> @@ -89,6 +89,11 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct >> mcs_spinlock *node) >> return; >> } >> WRITE_ONCE(prev->next, node); >> +/* >> + * This is necessary to make sure that the corresponding "while" in the >> + * mcs_spin_unlock() doesn't loop forever >> + */ > This comment is utterly inadequate, since it does not describe an > explicit ordering between two (or more) stores. > >> +smp_wmb(); >> >> /* Wait until the lock holder passes the lock down. */ >> arch_mcs_spin_lock_contended(>locked); >> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c >> index cbff6ba..577fe01 100644 >> --- a/kernel/locking/qspinlock.c >> +++ b/kernel/locking/qspinlock.c >> @@ -469,6 +469,12 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, >> u32 val) >> >> /* Link @node into the waitqueue. */ >> WRITE_ONCE(prev->next, node); >> +/* >> + * This is necessary to make sure that the corresponding >> + * smp_cond_load_relaxed() below (running on another core) >> + * doesn't spin forever. >> + */ >> +smp_wmb(); > That's insane, cache coherency should not allow that to happen in the > first place. Our smp_wmb() cannot help with that. > -- Best regards, Alexander Sverdlin.
Re: [PATCH 1/2] qspinlock: Ensure writes are pushed out of core write buffer
Hi! On 27/01/2021 23:21, Will Deacon wrote: > On Wed, Jan 27, 2021 at 09:01:08PM +0100, Alexander A Sverdlin wrote: >> From: Alexander Sverdlin >> >> Ensure writes are pushed out of core write buffer to prevent waiting code >> on another cores from spinning longer than necessary. >> >> 6 threads running tight spinlock loop competing for the same lock >> on 6 cores on MIPS/Octeon do 100 iterations... >> >> before the patch in: 4.3 sec >> after the patch in: 1.2 sec > If you only have 6 cores, I'm not sure qspinlock makes any sense... That's my impression as well and I even proposed to revert qspinlocks for MIPS: https://lore.kernel.org/linux-mips/20170610002644.8434-1-paul.bur...@imgtec.com/T/#ma1506c80472129b2ac41554cc2d863c9a24518c0 >> Same 6-core Octeon machine: >> sysbench --test=mutex --num-threads=64 --memory-scope=local run >> >> w/o patch: 1.53s >> with patch: 1.28s >> >> This will also allow to remove the smp_wmb() in >> arch/arm/include/asm/mcs_spinlock.h (was it actually addressing the same >> issue?). >> >> Finally our internal quite diverse test suite of different IPC/network >> aspects didn't detect any regressions on ARM/ARM64/x86_64. >> >> Signed-off-by: Alexander Sverdlin >> --- >> kernel/locking/mcs_spinlock.h | 5 + >> kernel/locking/qspinlock.c| 6 ++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/kernel/locking/mcs_spinlock.h b/kernel/locking/mcs_spinlock.h >> index 5e10153..10e497a 100644 >> --- a/kernel/locking/mcs_spinlock.h >> +++ b/kernel/locking/mcs_spinlock.h >> @@ -89,6 +89,11 @@ void mcs_spin_lock(struct mcs_spinlock **lock, struct >> mcs_spinlock *node) >> return; >> } >> WRITE_ONCE(prev->next, node); >> +/* >> + * This is necessary to make sure that the corresponding "while" in the >> + * mcs_spin_unlock() doesn't loop forever >> + */ >> +smp_wmb(); > If it loops forever, that's broken hardware design; store buffers need to > drain. I don't think we should add unconditional barriers to bodge this. The comment is a bit exaggerating the situation, but it's undeterministic and you see the measurements above. Something is wrong in the qspinlocks code, please consider this patch "RFC", but something has to be done here. -- Best regards, Alexander Sverdlin.
Re: [PATCH 3/6] MIPS: Octeon: qspinlock: Flush write buffer
Hi!
On 27/01/2021 23:34, Peter Zijlstra wrote:
> On Wed, Jan 27, 2021 at 09:36:24PM +0100, Alexander A Sverdlin wrote:
>> From: Alexander Sverdlin
>>
>> Flushing the write buffer brings aroung 10% performace on the tight
>> uncontended spinlock loops on Octeon. Refer to commit 500c2e1fdbcc
>> ("MIPS: Optimize spinlocks.").
> No objection to the patch, but I don't find the above referenced commit
> to be enlightening wrt nudge_writes(). The best it has to offer is the
> comment that's already in the code.
My point was that original MIPS spinlocks had this write-buffer-flush and
it got lost on the conversion to qspinlocks. The referenced commit just
allows to see the last MIPS-specific implementation before deletion.
>> Signed-off-by: Alexander Sverdlin
>> ---
>> arch/mips/include/asm/spinlock.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/mips/include/asm/spinlock.h
>> b/arch/mips/include/asm/spinlock.h
>> index 8a88eb2..0a707f3 100644
>> --- a/arch/mips/include/asm/spinlock.h
>> +++ b/arch/mips/include/asm/spinlock.h
>> @@ -24,6 +24,9 @@ static inline void queued_spin_unlock(struct qspinlock
>> *lock)
>> /* This could be optimised with ARCH_HAS_MMIOWB */
>> mmiowb();
>> smp_store_release(>locked, 0);
>> +#ifdef CONFIG_CPU_CAVIUM_OCTEON
>> +nudge_writes();
>> +#endif
>> }
>>
>> #include
--
Best regards,
Alexander Sverdlin.
Re: [PATCH 1/6] MIPS: Octeon: Implement __smp_store_release()
Hello Peter,
On 27/01/2021 23:32, Peter Zijlstra wrote:
>> Link: https://lore.kernel.org/lkml/5644d08d.4080...@caviumnetworks.com/
please, check the discussion pointed by the link above...
>> Signed-off-by: Alexander Sverdlin
>> ---
>> arch/mips/include/asm/barrier.h | 9 +
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/mips/include/asm/barrier.h
>> b/arch/mips/include/asm/barrier.h
>> index 49ff172..24c3f2c 100644
>> --- a/arch/mips/include/asm/barrier.h
>> +++ b/arch/mips/include/asm/barrier.h
>> @@ -113,6 +113,15 @@ static inline void wmb(void)
>> ".set arch=octeon\n\t" \
>> "syncw\n\t" \
>> ".set pop" : : : "memory")
>> +
>> +#define __smp_store_release(p, v) \
>> +do {
>> \
>> +compiletime_assert_atomic_type(*p); \
>> +__smp_wmb();\
>> +__smp_rmb();\
>> +WRITE_ONCE(*p, v); \
>> +} while (0)
> This is wrong in general since smp_rmb() will only provide order between
> two loads and smp_store_release() is a store.
>
> If this is correct for all MIPS, this needs a giant comment on exactly
> how that smp_rmb() makes sense here.
... the macro is provided for Octeon only, and __smp_rmb() is actually a NOP
there, but I thought to "document" the flow of thoughts from the discussion
above by including it anyway.
--
Best regards,
Alexander Sverdlin.
Re: [PATCH v2 2/9] gpio: ep93xx: Fix single irqchip with multi gpiochips
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct ep93xx_gpio *epg = gpiochip_get_data(gc);
> - int port = ep93xx_gpio_port(gc);
> + int port = ep93xx_gpio_port(epg, gc);
> int offset = d->irq & 7;
> int port_mask = BIT(offset);
> irq_flow_handler_t handler;
> @@ -335,6 +342,22 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip
> *gc, unsigned offset)
> return EP93XX_GPIO_F_IRQ_BASE + offset;
> }
>
> +static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
> +{
> + int i;
> +
> + for (i = 0; i < EP93XX_GPIO_IRQ_CHIPS_NUM; i++) {
> + struct irq_chip *ic = >ic[i];
> +
> + ic->name = irq_chip_names[i];
> + ic->irq_ack = ep93xx_gpio_irq_ack;
> + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack;
> + ic->irq_mask = ep93xx_gpio_irq_mask;
> + ic->irq_unmask = ep93xx_gpio_irq_unmask;
> + ic->irq_set_type = ep93xx_gpio_irq_type;
> + }
> +}
> +
> static int ep93xx_gpio_add_bank(struct gpio_chip *gc,
> struct platform_device *pdev,
> struct ep93xx_gpio *epg,
> @@ -345,6 +368,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
> struct device *dev = >dev;
> struct gpio_irq_chip *girq;
> int err;
> + int port;
>
> err = bgpio_init(gc, dev, 1, data, NULL, NULL, dir, NULL, 0);
> if (err)
> @@ -356,7 +380,8 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
> girq = >irq;
> if (bank->has_irq || bank->has_hierarchical_irq) {
> gc->set_config = ep93xx_gpio_set_config;
> - girq->chip = _gpio_irq_chip;
> + port = ep93xx_gpio_port(epg, gc);
> + girq->chip = >ic[port];
> }
>
> if (bank->has_irq) {
> @@ -423,6 +448,8 @@ static int ep93xx_gpio_probe(struct
> platform_device *pdev)
> if (IS_ERR(epg->base))
> return PTR_ERR(epg->base);
>
> + ep93xx_init_irq_chips(epg);
> +
> for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) {
> struct gpio_chip *gc = >gc[i];
> struct ep93xx_gpio_bank *bank =
> _gpio_banks[i];
--
Alexander Sverdlin.
Re: [PATCH v2 7/9] gpio: ep93xx: separate IRQ's setup
erarchical.
> - */
> - girq->parent_handler = ep93xx_gpio_f_irq_handler;
> - girq->num_parents = 8;
> - girq->parents = devm_kcalloc(dev, girq->num_parents,
> - sizeof(*girq->parents),
> - GFP_KERNEL);
> - if (!girq->parents)
> - return -ENOMEM;
> - /* Pick resources 1..8 for these IRQs */
> - for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
> - girq->parents[i] = platform_get_irq(pdev, i +
> 1);
> - gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
> - irq_set_chip_data(gpio_irq, >gc[5]);
> - irq_set_chip_and_handler(gpio_irq,
> -
> _gpio_irq_chip,
> - handle_level_irq);
> - irq_clear_status_flags(gpio_irq,
> IRQ_NOREQUEST);
> - }
> - girq->default_type = IRQ_TYPE_NONE;
> - girq->handler = handle_level_irq;
> - girq->first = EP93XX_GPIO_F_IRQ_BASE;
> + err = ep93xx_gpio_add_f_irq_chip(pdev, girq,
> EP93XX_GPIO_F_IRQ_BASE);
> + if (err)
> + return err;
> }
>
> return devm_gpiochip_add_data(dev, gc, epg);
--
Alexander Sverdlin.
Re: [PATCH v2 6/9] gpio: ep93xx: refactor ep93xx_gpio_add_bank
Hello Nikita,
On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> - replace plain numbers with girq->num_parents in devm_kcalloc
> - replace plain numbers with ARRAY_SIZE(girq->parents) for port F
> - refactor i - 1 to i + 1 to make loop more readable
> - combine getting IRQ's loop and setting handler's into single loop
>
> Signed-off-by: Nikita Shubin
> ---
> drivers/gpio/gpio-ep93xx.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 8f66e3ca0cfb..e4270b4e5f26 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -384,7 +384,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>
> girq->parent_handler = ep93xx_gpio_ab_irq_handler;
> girq->num_parents = 1;
> - girq->parents = devm_kcalloc(dev, 1,
> + girq->parents = devm_kcalloc(dev, girq->num_parents,
> sizeof(*girq->parents),
> GFP_KERNEL);
> if (!girq->parents)
> @@ -406,15 +406,14 @@ static int ep93xx_gpio_add_bank(struct
> gpio_chip *gc,
> */
> girq->parent_handler = ep93xx_gpio_f_irq_handler;
> girq->num_parents = 8;
> - girq->parents = devm_kcalloc(dev, 8,
> + girq->parents = devm_kcalloc(dev, girq->num_parents,
> sizeof(*girq->parents),
> GFP_KERNEL);
> if (!girq->parents)
> return -ENOMEM;
> /* Pick resources 1..8 for these IRQs */
> - for (i = 1; i <= 8; i++)
> - girq->parents[i - 1] = platform_get_irq(pdev,
> i);
> - for (i = 0; i < 8; i++) {
> + for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
Why do you use ARRAY_SIZE() here instead of ->num_parents like above?
> + girq->parents[i] = platform_get_irq(pdev, i +
> 1);
> gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i;
> irq_set_chip_data(gpio_irq, >gc[5]);
> irq_set_chip_and_handler(gpio_irq,
--
Alexander Sverdlin.
Re: [PATCH v2 5/9] gpio: ep93xx: Fix typo s/hierarchial/hierarchical
Hi! On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > Fix typo in comment. > > Signed-off-by: Nikita Shubin Acked-by: Alexander Sverdlin > --- > drivers/gpio/gpio-ep93xx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index dee19372ebbd..8f66e3ca0cfb 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -402,7 +402,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > > /* > * FIXME: convert this to use hierarchical IRQ > support! > - * this requires fixing the root irqchip to be > hierarchial. > + * this requires fixing the root irqchip to be > hierarchical. > */ > girq->parent_handler = ep93xx_gpio_f_irq_handler; > girq->num_parents = 8; -- Alexander Sverdlin.
Re: [PATCH v2 4/9] gpio: ep93xx: drop to_irq binding
Hi!
On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> As ->to_irq is redefined in gpiochip_add_irqchip, having it defined
> in
> driver is useless, so let's drop it.
>
> Signed-off-by: Nikita Shubin
Reviewed-by: Alexander Sverdlin
> ---
> drivers/gpio/gpio-ep93xx.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index 9c3d049e5af7..dee19372ebbd 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -337,11 +337,6 @@ static int ep93xx_gpio_set_config(struct
> gpio_chip *gc, unsigned offset,
> return 0;
> }
>
> -static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned
> offset)
> -{
> - return EP93XX_GPIO_F_IRQ_BASE + offset;
> -}
> -
> static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg)
> {
> int i;
> @@ -429,7 +424,6 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
> }
> girq->default_type = IRQ_TYPE_NONE;
> girq->handler = handle_level_irq;
> - gc->to_irq = ep93xx_gpio_f_to_irq;
> girq->first = EP93XX_GPIO_F_IRQ_BASE;
> }
>
--
Alexander Sverdlin.
Re: [PATCH v2 8/9] gpio: ep93xx: refactor base IRQ number
Hi!
On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote:
> - use predefined constants instead of plain numbers
> - use provided bank IRQ number instead of defined constant
> for port F
>
> Signed-off-by: Nikita Shubin
Reviewed-by: Alexander Sverdlin
> ---
> drivers/gpio/gpio-ep93xx.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c
> index b212c007240e..2aee13b5067d 100644
> --- a/drivers/gpio/gpio-ep93xx.c
> +++ b/drivers/gpio/gpio-ep93xx.c
> @@ -28,6 +28,8 @@
> /* Maximum value for irq capable line identifiers */
> #define EP93XX_GPIO_LINE_MAX_IRQ 23
>
> +#define EP93XX_GPIO_A_IRQ_BASE 64
> +#define EP93XX_GPIO_B_IRQ_BASE 72
> /*
> * Static mapping of GPIO bank F IRQS:
> * F0..F7 (16..24) to irq 80..87.
> @@ -311,14 +313,14 @@ struct ep93xx_gpio_bank {
>
> static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = {
> /* Bank A has 8 IRQs */
> - EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, 64),
> + EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false,
> EP93XX_GPIO_A_IRQ_BASE),
> /* Bank B has 8 IRQs */
> - EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72),
> + EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false,
> EP93XX_GPIO_B_IRQ_BASE),
> EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0),
> EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0),
> EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0),
> /* Bank F has 8 IRQs */
> - EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0),
> + EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true,
> EP93XX_GPIO_F_IRQ_BASE),
> EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0),
> EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0),
> };
> @@ -445,7 +447,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip
> *gc,
>
> /* Only bank F has especially funky IRQ handling */
> if (bank->has_hierarchical_irq) {
> - err = ep93xx_gpio_add_f_irq_chip(pdev, girq,
> EP93XX_GPIO_F_IRQ_BASE);
> + err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank-
> >irq_base);
> if (err)
> return err;
> }
--
Alexander Sverdlin.
Re: [PATCH v2 9/9] gpio: ep93xx: replace bools with idx for IRQ ports
irq_chip *girq,
> + struct gpio_chip *gc,
> unsigned int irq_base)
> {
> int gpio_irq;
> int i;
> struct device *dev = >dev;
> + struct gpio_irq_chip *girq = >irq;
>
> /*
> * FIXME: convert this to use hierarchical IRQ support!
> @@ -397,10 +399,10 @@ static int ep93xx_gpio_add_f_irq_chip(struct
> platform_device *pdev,
> if (!girq->parents)
> return -ENOMEM;
> /* Pick resources 1..8 for these IRQs */
> - for (i = 0; i < ARRAY_SIZE(girq->parents); i++) {
> + for (i = 0; i < girq->num_parents; i++) {
> girq->parents[i] = platform_get_irq(pdev, i + 1);
> gpio_irq = irq_base + i;
> - irq_set_chip_data(gpio_irq, >gc[5]);
> + irq_set_chip_data(gpio_irq, gc);
> irq_set_chip_and_handler(gpio_irq,
> _gpio_irq_chip,
> handle_level_irq);
> @@ -433,21 +435,22 @@ static int ep93xx_gpio_add_bank(struct
> gpio_chip *gc,
> gc->base = bank->base;
>
> girq = >irq;
> - if (bank->has_irq || bank->has_hierarchical_irq) {
> + if (bank->irq_base != 0) {
> gc->set_config = ep93xx_gpio_set_config;
> port = ep93xx_gpio_port(epg, gc);
> girq->chip = >ic[port];
> }
>
> - if (bank->has_irq) {
> - err = ep93xx_gpio_add_ab_irq_chip(pdev, girq, bank-
> >irq_base);
> + if (bank->idx == EP93XX_GPIO_A_PORT_INDEX ||
> + bank->idx == EP93XX_GPIO_B_PORT_INDEX) {
> + err = ep93xx_gpio_add_ab_irq_chip(pdev, gc, bank-
> >irq_base);
> if (err)
> return err;
> }
>
> /* Only bank F has especially funky IRQ handling */
> - if (bank->has_hierarchical_irq) {
> - err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank-
> >irq_base);
> + if (bank->idx == EP93XX_GPIO_F_PORT_INDEX) {
> + err = ep93xx_gpio_add_f_irq_chip(pdev, gc, bank-
> >irq_base);
> if (err)
> return err;
> }
--
Alexander Sverdlin.
Re: [PATCH v6 0/2] ARM: Implement MODULE_PLT support in FTRACE
Hello Florian, On 27/01/2021 05:48, Florian Fainelli wrote: >> FTRACE's function tracer currently doesn't always work on ARM with >> MODULE_PLT option enabled. If the module is loaded too far, FTRACE's >> code modifier cannot cope with introduced veneers and turns the >> function tracer off globally. >> >> ARM64 already has a solution for the problem, refer to the following >> patches: >> >> arm64: ftrace: emit ftrace-mod.o contents through code >> arm64: module-plts: factor out PLT generation code for ftrace >> arm64: ftrace: fix !CONFIG_ARM64_MODULE_PLTS kernels >> arm64: ftrace: fix building without CONFIG_MODULES >> arm64: ftrace: add support for far branches to dynamic ftrace >> arm64: ftrace: don't validate branch via PLT in ftrace_make_nop() >> >> But the presented ARM variant has just a half of the footprint in terms of >> the changed LoCs. It also retains the code validation-before-modification >> instead of switching it off. > We have been using those patches and I was wondering what happened after > this version since they did not show up upstream nor in Russell's patch > tracker? Would you be willing to resubmit them? it's a pity, but nobody ever showed any interest to take them to fix real problem in FTRACE. A grain of salt would be to remind, that no-one also replied that the series helped him or a Tested-by: tag. This would probably wake more interest from maintainers side. Of course I can re-send them because I personally re-base them regularly. -- Best regards, Alexander Sverdlin.
Re: [PATCH] MIPS: OCTEON: fix unreachable code in octeon_irq_init_ciu
Hello Menglong!
Thank you for the fix! A small correction below:
On 11/01/2021 12:36, menglong8.d...@gmail.com wrote:
> From: Menglong Dong
>
> The type of 'r' in octeon_irq_init_ciu is 'unsigned int', so 'r < 0'
> can't be true.
>
> Fix this by change the type of 'r' and 'i' from 'unsigned int'
> to 'int'. As 'i' won't be negative, this change works.
>
> Fixes: 64b139f97c01("MIPS: OCTEON: irq: add CIB and other fixes")
The patch actually fixes my commit 99fbc70f8547
("MIPS: Octeon: irq: Alloc desc before configuring IRQ").
With "Fixes:" tag corrected, you can put my
Reviewed-by: Alexander Sverdlin
> Signed-off-by: Menglong Dong
> ---
> arch/mips/cavium-octeon/octeon-irq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/cavium-octeon/octeon-irq.c
> b/arch/mips/cavium-octeon/octeon-irq.c
> index bd47e15d02c7..be5d4afcd30f 100644
> --- a/arch/mips/cavium-octeon/octeon-irq.c
> +++ b/arch/mips/cavium-octeon/octeon-irq.c
> @@ -1444,7 +1444,7 @@ static void octeon_irq_setup_secondary_ciu2(void)
> static int __init octeon_irq_init_ciu(
> struct device_node *ciu_node, struct device_node *parent)
> {
> - unsigned int i, r;
> + int i, r;
> struct irq_chip *chip;
> struct irq_chip *chip_edge;
> struct irq_chip *chip_mbox;
>
--
Best regards,
Alexander Sverdlin.
[PATCH] serial: 8250_omap: Avoid FIFO corruption caused by MDR1 access
It has been observed that once per 300-1300 port openings the first
transmitted byte is being corrupted on AM3352 ("v" written to FIFO appeared
as "e" on the wire). It only happened if single byte has been transmitted
right after port open, which means, DMA is not used for this transfer and
the corruption never happened afterwards.
Therefore I've carefully re-read the MDR1 errata (link below), which says
"when accessing the MDR1 registers that causes a dummy under-run condition
that will freeze the UART in IrDA transmission. In UART mode, this may
corrupt the transferred data". Strictly speaking,
omap_8250_mdr1_errataset() performs a read access and if the value is the
same as should be written, exits without errata-recommended FIFO reset.
A brief check of the serial_omap_mdr1_errataset() from the competing
omap-serial driver showed it has no read access of MDR1. After removing the
read access from omap_8250_mdr1_errataset() the data corruption never
happened any more.
Link: https://www.ti.com/lit/er/sprz360i/sprz360i.pdf
Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
Cc: sta...@vger.kernel.org
Signed-off-by: Alexander Sverdlin
---
drivers/tty/serial/8250/8250_omap.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_omap.c
b/drivers/tty/serial/8250/8250_omap.c
index 562087df7d33..0cc6d35a0815 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -184,11 +184,6 @@ static void omap_8250_mdr1_errataset(struct uart_8250_port
*up,
struct omap8250_priv *priv)
{
u8 timeout = 255;
- u8 old_mdr1;
-
- old_mdr1 = serial_in(up, UART_OMAP_MDR1);
- if (old_mdr1 == priv->mdr1)
- return;
serial_out(up, UART_OMAP_MDR1, priv->mdr1);
udelay(2);
--
2.29.2
Re: ks-sa-rng.c:undefined reference to `devm_platform_ioremap_resource'
Hi! On 27/11/2020 06:48, Herbert Xu wrote: > On Fri, Nov 13, 2020 at 11:02:13PM +0800, kernel test robot wrote: >> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git >> master >> head: 585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba >> commit: 90c4b29eb1e555fee66f8329a18cb8a070090ad6 hwrng: ks-sa - Enable >> COMPILE_TEST >> date: 12 months ago >> config: s390-randconfig-r022-20201113 (attached as .config) >> compiler: s390-linux-gcc (GCC) 9.3.0 >> reproduce (this is a W=1 build): >> wget >> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O >> ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=90c4b29eb1e555fee66f8329a18cb8a070090ad6 >> git remote add linus >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git >> git fetch --no-tags linus master >> git checkout 90c4b29eb1e555fee66f8329a18cb8a070090ad6 >> # save the attached .config to linux build tree >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross >> ARCH=s390 >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot >> >> All errors (new ones prefixed by >>): > >>s390-linux-ld: drivers/char/hw_random/ks-sa-rng.o: in function >> `ks_sa_rng_probe': >>>> ks-sa-rng.c:(.text+0x2fa): undefined reference to >>>> `devm_platform_ioremap_resource' > > ---8<--- > This patch adds a dependency for KEYSTONE on HAS_IOMEM and OF to > prevent COMPILE_TEST build failures. > > Reported-by: kernel test robot > Signed-off-by: Herbert Xu Reviewed-by: Alexander Sverdlin > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig > index ab33a2e17cdf..9ff4fb3236f7 100644 > --- a/drivers/char/hw_random/Kconfig > +++ b/drivers/char/hw_random/Kconfig > @@ -508,6 +508,7 @@ config HW_RANDOM_NPCM > > config HW_RANDOM_KEYSTONE > depends on ARCH_KEYSTONE || COMPILE_TEST > + depends on HAS_IOMEM && OF > default HW_RANDOM > tristate "TI Keystone NETCP SA Hardware random number generator" > help > -- Best regards, Alexander Sverdlin.
Re: [PATCH] MIPS: reserve the memblock right after the kernel
Hello Serge, On 16/11/2020 23:31, Serge Semin wrote: >> As completely unrelated optimization I can remove the same memblock_add() of >> the >> kernel sections from the Octeon platform code. > Why not as long as it will work. AFAICS the octeon platform code does > some kernel start address adjustment while the generic MIPS code > doesn't. Are you sure using the generic version for octeon won't cause > any problem? as I interpret this adjustment, this is open-coded virt_to_phys. In my tests both code blocks reserve the same memory (well, generic code claims the rest of the last page, and I'm going to fix this). -- Best regards, Alexander Sverdlin.
Re: [PATCH] MIPS: reserve the memblock right after the kernel
Hello Serge, Thomas,
On 13/11/2020 10:17, Alexander Sverdlin wrote:
>> So IMHO what could be the best conclusion in the framework of this patch:
>> 1) As Thomas said any platform-specific reservation should be done in the
>> platform-specific code. That means if octeon needs some memory behind
>> the kernel being reserved, then it should be done for example in
>> prom_init().
>> 2) The check_kernel_sections_mem() method can be removed. But it
>> should be done carefully. We at least need to try to find all the
>> platforms, which rely on its functionality.
> Thanks for looking into this! I agree with your analysis, I'll try to rework,
> removing check_kernel_sections_mem().
but now, after grepping inside arch/mips, I found that only Octeon does
memblock_add()
of the area between _text and _and explicitly.
Therefore, maybe many other platforms indeed rely on
check_kernel_sections_mem()?
Maybe the proper way would be really to remote the PFN_UP()/PFN_DOWN() from
check_kernel_sections_mem(), which is not necessary after commit b10d6bca8720
("arch, drivers: replace for_each_membock() with for_each_mem_range()")
which fixed the resource_init()?
As completely unrelated optimization I can remove the same memblock_add() of the
kernel sections from the Octeon platform code.
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] MIPS: reserve the memblock right after the kernel
Hi Jiaxun, On 13/11/2020 03:30, Jiaxun Yang wrote: >> 2) The check_kernel_sections_mem() method can be removed. But it >> should be done carefully. We at least need to try to find all the >> platforms, which rely on its functionality. > It have been more than 10 years since introducing this this check, but > I believe there must be a reason at the time. could you maybe share the reason you've submitted it with us? > Also currently we have some unmaintained platform and it's impossible > to test on them for us. Do you at least know the list of platforms this patch is required for? -- Best regards, Alexander Sverdlin.
Re: [PATCH] MIPS: reserve the memblock right after the kernel
Hi! On 13/11/2020 03:30, Jiaxun Yang wrote: >> 2) The check_kernel_sections_mem() method can be removed. But it >> should be done carefully. We at least need to try to find all the >> platforms, which rely on its functionality. > It have been more than 10 years since introducing this this check, but > I believe there must be a reason at the time. It doesn't look like 10 years to me :) commit a94e4f24ec836c8984f839594bad7454184975b1 Author: Jiaxun Yang Date: Mon Aug 19 22:23:13 2019 +0800 MIPS: init: Drop boot_mem_map at least not so long in public, that's why we only noticed recently, that is breaks Octeon platform. -- Best regards, Alexander Sverdlin.
Re: [PATCH] MIPS: reserve the memblock right after the kernel
Hello Serge, Thomas,
On 11/11/2020 15:52, Serge Semin wrote:
>>> Could you send a patch, which removes check_kernel_section_mem completly ?
finally I think you are right and this would be the right way.
>> this will expose one issue:
>> platforms usually do it in a sane way, like it was done last 15 years, namely
>> add kernel image without non-complete pages on the boundaries.
>> This will lead to the situation, that request_resource() will fail at least
>> for .bss section of the kernel and it will not be properly displayed under
>> /proc/iomem (and probably same problem will appear, which initially motivated
>> the creation of check_kernel_section_mem()).
>
> Are you saying that some old platforms rely on the
> check_kernel_section_mem() method adding the memory occupied by the
> kernel to the system? If so, do you have an example of such?
Initially I was confused why the below patch didn't solve the issue on Octeon:
@@ -532,8 +532,8 @@ static void __init request_crashkernel(struct resource *res)
static void __init check_kernel_sections_mem(void)
{
- phys_addr_t start = PFN_PHYS(PFN_DOWN(__pa_symbol(&_text)));
- phys_addr_t size = PFN_PHYS(PFN_UP(__pa_symbol(&_end))) - start;
+ phys_addr_t start = __pa_symbol(&_text);
+ phys_addr_t size = __pa_symbol(&_end) - start;
... and finally I understood, that the reason was in fact that I tested on Linux
v5.4, which still had this code to reserve RAM resources:
for_each_memblock(memory, region) {
phys_addr_t start =
PFN_PHYS(memblock_region_memory_base_pfn(region));
phys_addr_t end =
PFN_PHYS(memblock_region_memory_end_pfn(region)) - 1;
struct resource *res;
...
res->start = start;
res->end = end;
res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
res->name = "System RAM";
request_resource(_resource, res);
so I suppose that's where this evil truncation happened. Nowdays this is
different
and I believe we can try to remove check_kernel_sections_mem() completely and
this will solve the memory corruption on Octeon.
> So IMHO what could be the best conclusion in the framework of this patch:
> 1) As Thomas said any platform-specific reservation should be done in the
> platform-specific code. That means if octeon needs some memory behind
> the kernel being reserved, then it should be done for example in
> prom_init().
> 2) The check_kernel_sections_mem() method can be removed. But it
> should be done carefully. We at least need to try to find all the
> platforms, which rely on its functionality.
Thanks for looking into this! I agree with your analysis, I'll try to rework,
removing check_kernel_sections_mem().
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] MIPS: reserve the memblock right after the kernel
Hello Thomas, On 10/11/2020 10:55, Thomas Bogendoerfer wrote: >>>> Linux doesn't own the memory immediately after the kernel image. On Octeon >>>> bootloader places a shared structure right close after the kernel _end, >>>> refer to "struct cvmx_bootinfo *octeon_bootinfo" in cavium-octeon/setup.c. >>>> >>>> If check_kernel_sections_mem() rounds the PFNs up, first memblock_alloc() >>>> inside early_init_dt_alloc_memory_arch() <= device_tree_init() returns >>>> memory block overlapping with the above octeon_bootinfo structure, which >>>> is being overwritten afterwards. >>> as this special for Octeon how about added the memblock_reserve >>> in octen specific code ? >> while the shared structure which is being corrupted is indeed >> Octeon-specific, >> the wrong assumption that the memory right after the kernel can be allocated >> by memblock >> allocator and re-used somewhere in Linux is in MIPS-generic >> check_kernel_sections_mem(). > ok, I see your point. IMHO this whole check_kernel_sections_mem() should > be removed. IMHO memory adding should only be done my memory detection code. > > Could you send a patch, which removes check_kernel_section_mem completly ? this will expose one issue: platforms usually do it in a sane way, like it was done last 15 years, namely add kernel image without non-complete pages on the boundaries. This will lead to the situation, that request_resource() will fail at least for .bss section of the kernel and it will not be properly displayed under /proc/iomem (and probably same problem will appear, which initially motivated the creation of check_kernel_section_mem()). As I understood, the issue is that memblock API operates internally on the page granularity (at least there are many ROUND_DOWN() inside for the size or upper boundary), so for request_resource() to success one has to claim the rest of the .bss last page. And with current memblock API memblock_reserve() must appear somewhere, being this ARCH or platform code. -- Best regards, Alexander Sverdlin.
Re: [PATCH] MIPS: reserve the memblock right after the kernel
Hi Thomas, On 09/11/2020 11:34, Alexander Sverdlin wrote: >>> Linux doesn't own the memory immediately after the kernel image. On Octeon >>> bootloader places a shared structure right close after the kernel _end, >>> refer to "struct cvmx_bootinfo *octeon_bootinfo" in cavium-octeon/setup.c. >>> >>> If check_kernel_sections_mem() rounds the PFNs up, first memblock_alloc() >>> inside early_init_dt_alloc_memory_arch() <= device_tree_init() returns >>> memory block overlapping with the above octeon_bootinfo structure, which >>> is being overwritten afterwards. >> as this special for Octeon how about added the memblock_reserve >> in octen specific code ? > while the shared structure which is being corrupted is indeed Octeon-specific, > the wrong assumption that the memory right after the kernel can be allocated > by memblock > allocator and re-used somewhere in Linux is in MIPS-generic > check_kernel_sections_mem(). > > I personally will be fine with repairing Octeon only as I don't have other > MIPS > targets to care about, but maybe someone else in the MIPS community will find > this fix useful... another reason for not putting that to Octeon platform code was the fact, that one would need to put the knowledge about wrong assumption of ARCH (MIPS) code into different code area of Octeon platform. If at some point in time check_kernel_sections_mem() will be altered/fixed, nobody will understand why Octeon still has this workaround. -- Best regards, Alexander Sverdlin.
Re: [PATCH] MIPS: reserve the memblock right after the kernel
Hello Thomas, On 07/11/2020 10:40, Thomas Bogendoerfer wrote: >> Linux doesn't own the memory immediately after the kernel image. On Octeon >> bootloader places a shared structure right close after the kernel _end, >> refer to "struct cvmx_bootinfo *octeon_bootinfo" in cavium-octeon/setup.c. >> >> If check_kernel_sections_mem() rounds the PFNs up, first memblock_alloc() >> inside early_init_dt_alloc_memory_arch() <= device_tree_init() returns >> memory block overlapping with the above octeon_bootinfo structure, which >> is being overwritten afterwards. > as this special for Octeon how about added the memblock_reserve > in octen specific code ? while the shared structure which is being corrupted is indeed Octeon-specific, the wrong assumption that the memory right after the kernel can be allocated by memblock allocator and re-used somewhere in Linux is in MIPS-generic check_kernel_sections_mem(). I personally will be fine with repairing Octeon only as I don't have other MIPS targets to care about, but maybe someone else in the MIPS community will find this fix useful... -- Best regards, Alexander Sverdlin.
Re: [PATCH] arm: Add clk_get_rate input parameter null check
Hello Wang!
On Fri, 2020-11-06 at 09:46 +0800, Wang Qing wrote:
> The input parameter of clk_get_rate() is checked with IS_ERR(),
> so here we need to check null on clk.
Thank you for the patch!
> Signed-off-by: Wang Qing
Acked-by: Alexander Sverdlin
> ---
> arch/arm/mach-ep93xx/clock.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-
> ep93xx/clock.c
> index 2810eb5..4313b2f
> --- a/arch/arm/mach-ep93xx/clock.c
> +++ b/arch/arm/mach-ep93xx/clock.c
> @@ -321,6 +321,9 @@ static unsigned long get_uart_rate(struct clk
> *clk)
>
> unsigned long clk_get_rate(struct clk *clk)
> {
> + if (!clk)
> + return 0;
> +
> if (clk->get_rate)
> return clk->get_rate(clk);
>
--
Alexander Sverdlin.
Re: [PATCH] mtd: spi-nor: Don't copy self-pointing struct around
Hello Tudor,
On 07/10/2020 10:48, tudor.amba...@microchip.com wrote:
>> From: Alexander Sverdlin
>>
>> spi_nor_parse_sfdp() modifies the passed structure so that it points to
>> itself (params.erase_map.regions to params.erase_map.uniform_region). This
>> makes it impossible to copy the local struct anywhere else.
>>
>> Therefore only use memcpy() in backup-restore scenario. The bug may show up
>> like below:
>>
>> BUG: unable to handle page fault for address: c9b377f8
>> Oops: [#1] PREEMPT SMP NOPTI
>> CPU: 4 PID: 3500 Comm: flashcp Tainted: G O 5.4.53-... #1
>> ...
>> RIP: 0010:spi_nor_erase+0x8e/0x5c0
>> Code: 64 24 18 89 db 4d 8b b5 d0 04 00 00 4c 89 64 24 18 4c 89 64 24 20 eb
>> 12 a8 10 0f 85 59 02 00 00 49 83 c6 10 0f 84 4f 02 00 00 <49> 8b 06 48 89 c2
>> 48 83 e2 c0 48 89 d1 49 03 4e 08 48 39 cb 73 d8
>> RSP: 0018:c9000217fc48 EFLAGS: 00010206
>> RAX: 0074 RBX: RCX: 0074
>> RDX: 8884550c9980 RSI: 88844f9c0bc0 RDI: 88844ede7bb8
>> RBP: 0074 R08: 815bfbe0 R09: 88844f9c0bc0
>> R10: R11: R12: c9000217fc60
>> R13: 88844ede7818 R14: c9b377f8 R15:
>> FS: 7f4699780500() GS:88846ff0() knlGS:
>> CS: 0010 DS: ES: CR0: 80050033
>> CR2: c9b377f8 CR3: 0004538ee000 CR4: 00340fe0
>> Call Trace:
>> part_erase+0x27/0x50
>> mtdchar_ioctl+0x831/0xba0
>> ? filemap_map_pages+0x186/0x3d0
>> ? do_filp_open+0xad/0x110
>> ? _copy_to_user+0x22/0x30
>> ? cp_new_stat+0x150/0x180
>> mtdchar_unlocked_ioctl+0x2a/0x40
>> do_vfs_ioctl+0xa0/0x630
>> ? __do_sys_newfstat+0x3c/0x60
>> ksys_ioctl+0x70/0x80
>> __x64_sys_ioctl+0x16/0x20
>> do_syscall_64+0x6a/0x200
>> ? prepare_exit_to_usermode+0x50/0xd0
>> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> RIP: 0033:0x7f46996b6817
>>
>> Fixes: 1c1d8d98e1c7 ("mtd: spi-nor: Split spi_nor_init_params()")
> I think the correct Fixes tag is:
> Fixes: c46872170a54 ("mtd: spi-nor: Move erase_map to 'struct
> spi_nor_flash_parameter'")
yes, I think you are right regarding the exact hash!
Thank you for checking this!
>> Cc: sta...@vger.kernel.org
>> Tested-by: Baurzhan Ismagulov
>> Co-developed-by: Matija Glavinic Pecotic
>>
>> Signed-off-by: Matija Glavinic Pecotic
>>
>> Signed-off-by: Alexander Sverdlin
>> ---
>> drivers/mtd/spi-nor/core.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 2add4a0..cce0670 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2701,11 +2701,10 @@ static void spi_nor_sfdp_init_params(struct spi_nor
>> *nor)
>>
>> memcpy(_params, nor->params, sizeof(sfdp_params));
>>
>> - if (spi_nor_parse_sfdp(nor, _params)) {
>> + if (spi_nor_parse_sfdp(nor, nor->params)) {
>> + memcpy(nor->params, _params, sizeof(*nor->params));
>> nor->addr_width = 0;
>> nor->flags &= ~SNOR_F_4B_OPCODES;
>> - } else {
>> - memcpy(nor->params, _params, sizeof(*nor->params));
> neat!
> With the Fixes tag fixed, one can add:
> Reviewed-by: Tudor Ambarus
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] mtd: revert "spi-nor: intel: provide a range for poll_timout"
Hello Tudor, On 22/07/2020 19:03, tudor.amba...@microchip.com wrote: > On 7/22/20 7:37 PM, Alexander Sverdlin wrote: [...] >> I've performed my testing as well and got the following results: >> >> Vanilla Linux 4.9 (i.e. before the introduction of the offending >> patch): >> >> dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k >> 1280+0 records in >> 1280+0 records out >> 5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.91981 s, 1.3 MB/s >> >> Vanilla 4.19 (i.e. with offending patch): >> >> dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k >> 1280+0 records in >> 1280+0 records out >> 5242880 bytes (5.2 MB, 5.0 MiB) copied, 6.70891 s, 781 kB/s >> >> 4.19 + revert: >> >> dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k >> 1280+0 records in >> 1280+0 records out >> 5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.90503 s, 1.3 MB/s >> >> Therefore it looks good from my PoV: >> >> Tested-by: Alexander Sverdlin [...] > would you put 10 us here >>> INTEL_SPI_TIMEOUT * 1000); >>> } >>> >>> @@ -301,7 +301,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi >>> *ispi) >>> u32 val; >>> >>> return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val, >>> - !(val & SSFSTS_CTL_SCIP), 40, >>> + !(val & SSFSTS_CTL_SCIP), 0, > > also here, and re-do a test? I'm curios if the performance will be > as it was before. with 10us it looks like this: dd if=/dev/flash/by-name/... of=/dev/null bs=4k 1280+0 records in 1280+0 records out 5242880 bytes (5.2 MB, 5.0 MiB) copied, 4.33816 s, 1.2 MB/s Which means, there is a performance regression and it would depend on the test case, how bad it will be... -- Best regards, Alexander Sverdlin.
Re: [PATCH] mtd: spi-nor: intel-spi: Simulate WRDI command
Hi!
On 22/07/2020 17:18, tudor.amba...@microchip.com wrote:
[...]
>>>> After spi_nor_write_disable() return code checks were introduced in the
>>>> spi-nor front end intel-spi backend stopped to work because WRDI was never
>>>> supported and always failed.
>>>>
>>>> Just pretend it was sucessful and ignore the command itself. HW sequencer
>>>> shall do the right thing automatically, while with SW sequencer we cannot
>>>> do it anyway, because the only tool we had was preopcode and it makes no
>>>> sense for WRDI.
>>>>
>>>> Cc: sta...@vger.kernel.org
>>>> Fixes: bce679e5ae3a ("mtd: spi-nor: Check for errors after each Register
>>>> Operation")
>>>> Signed-off-by: Alexander Sverdlin
>>>> ---
>>>> drivers/mtd/spi-nor/controllers/intel-spi.c | 8
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c
>>>> b/drivers/mtd/spi-nor/controllers/intel-spi.c
>>>> index 61d2a0a..134b356 100644
>>>> --- a/drivers/mtd/spi-nor/controllers/intel-spi.c
>>>> +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
>>>> @@ -612,6 +612,14 @@ static int intel_spi_write_reg(struct spi_nor *nor,
>>>> u8 opcode, const u8 *buf,
>>>> return 0;
>>>> }
>>>>
>>>> + /*
>>>> +* We hope that HW sequencer will do the right thing automatically
>>>> and
>>>> +* with the SW seuencer we cannot use preopcode any way, so just
>>>> ignore
>>
>> Typo, should be sequencer.
>>
>> Otherwise looks good to me.
>>
> It looks good to me too. Should I add your R-b tag when applying?
> I can fix the typo.
Thank you, Mika, Tudor!
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] mtd: revert "spi-nor: intel: provide a range for poll_timout"
Hello Luis, thank you for the patch! On 11/06/2020 00:46, Luis Alberto Herrera wrote: > This change reverts aba3a882a178: "mtd: spi-nor: intel: provide a range > for poll_timout". That change introduces a performance regression when > reading sequentially from flash. Logging calls to intel_spi_read without > this change we get: > > Start MTD read > [ 20.045527] intel_spi_read(from=180, len=40) > [ 20.045527] intel_spi_read(from=180, len=40) > [ 282.199274] intel_spi_read(from=1c0, len=40) > [ 282.199274] intel_spi_read(from=1c0, len=40) > [ 544.351528] intel_spi_read(from=200, len=40) > [ 544.351528] intel_spi_read(from=200, len=40) > End MTD read > > With this change: > > Start MTD read > [ 21.942922] intel_spi_read(from=1c0, len=40) > [ 21.942922] intel_spi_read(from=1c0, len=40) > [ 23.784058] intel_spi_read(from=200, len=40) > [ 23.784058] intel_spi_read(from=200, len=40) > [ 25.625006] intel_spi_read(from=240, len=40) > [ 25.625006] intel_spi_read(from=240, len=40) > End MTD read I've performed my testing as well and got the following results: Vanilla Linux 4.9 (i.e. before the introduction of the offending patch): dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in 1280+0 records out 5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.91981 s, 1.3 MB/s Vanilla 4.19 (i.e. with offending patch): dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in 1280+0 records out 5242880 bytes (5.2 MB, 5.0 MiB) copied, 6.70891 s, 781 kB/s 4.19 + revert: dd if=/dev/flash/by-name/XXX of=/dev/null bs=4k 1280+0 records in 1280+0 records out 5242880 bytes (5.2 MB, 5.0 MiB) copied, 3.90503 s, 1.3 MB/s Therefore it looks good from my PoV: Tested-by: Alexander Sverdlin > Signed-off-by: Luis Alberto Herrera > Acked-by: Mika Westerberg > --- > drivers/mtd/spi-nor/controllers/intel-spi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c > b/drivers/mtd/spi-nor/controllers/intel-spi.c > index 61d2a0ad2131..2b89361a0d3a 100644 > --- a/drivers/mtd/spi-nor/controllers/intel-spi.c > +++ b/drivers/mtd/spi-nor/controllers/intel-spi.c > @@ -292,7 +292,7 @@ static int intel_spi_wait_hw_busy(struct intel_spi *ispi) > u32 val; > > return readl_poll_timeout(ispi->base + HSFSTS_CTL, val, > - !(val & HSFSTS_CTL_SCIP), 40, > + !(val & HSFSTS_CTL_SCIP), 0, > INTEL_SPI_TIMEOUT * 1000); > } > > @@ -301,7 +301,7 @@ static int intel_spi_wait_sw_busy(struct intel_spi *ispi) > u32 val; > > return readl_poll_timeout(ispi->sregs + SSFSTS_CTL, val, > - !(val & SSFSTS_CTL_SCIP), 40, > + !(val & SSFSTS_CTL_SCIP), 0, > INTEL_SPI_TIMEOUT * 1000); > } > > -- Best regards, Alexander Sverdlin.
[PATCH] mtd: spi-nor: intel-spi: Simulate WRDI command
From: Alexander Sverdlin
After spi_nor_write_disable() return code checks were introduced in the
spi-nor front end intel-spi backend stopped to work because WRDI was never
supported and always failed.
Just pretend it was sucessful and ignore the command itself. HW sequencer
shall do the right thing automatically, while with SW sequencer we cannot
do it anyway, because the only tool we had was preopcode and it makes no
sense for WRDI.
Cc: sta...@vger.kernel.org
Fixes: bce679e5ae3a ("mtd: spi-nor: Check for errors after each Register
Operation")
Signed-off-by: Alexander Sverdlin
---
drivers/mtd/spi-nor/controllers/intel-spi.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c
b/drivers/mtd/spi-nor/controllers/intel-spi.c
index 61d2a0a..134b356 100644
--- a/drivers/mtd/spi-nor/controllers/intel-spi.c
+++ b/drivers/mtd/spi-nor/controllers/intel-spi.c
@@ -612,6 +612,14 @@ static int intel_spi_write_reg(struct spi_nor *nor, u8
opcode, const u8 *buf,
return 0;
}
+ /*
+* We hope that HW sequencer will do the right thing automatically and
+* with the SW seuencer we cannot use preopcode any way, so just ignore
+* write disable operation and pretend it was completed successfully.
+*/
+ if (opcode == SPINOR_OP_WRDI)
+ return 0;
+
writel(0, ispi->base + FADDR);
/* Write the value beforehand */
--
2.10.2
Re: [PATCH v3] watchdog: initialize device before misc_register
Hello Krzysztof,
On 16/07/2020 13:32, krzysztof.sob...@nokia.com wrote:
> When watchdog device is being registered, it calls misc_register that
> makes watchdog available for systemd to open. This is a data race
> scenario, because when device is open it may still have device struct
> not initialized - this in turn causes a crash. This patch moves
> device initialization before misc_register call and it solves the
> problem printed below.
[...]
thank you for looking into this!
> Fixes: 72139dfa2464 ("watchdog: Fix the race between the release of
> watchdog_core_data and cdev")
> Reviewed-by: Guenter Roeck
Reviewed-by: Alexander Sverdlin
> Signed-off-by: Krzysztof Sobota
> ---
> v1 -> v2:
> * removed Change-Id tag
> * added Review-by tag
> v2 -> v3
> * convert spaces to tabs
> * convert (hopefully) mail to plaintext
> ---
> drivers/watchdog/watchdog_dev.c | 18 +-
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 10b2090f3e5e..1c322caecf7f 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -947,6 +947,15 @@ static int watchdog_cdev_register(struct watchdog_device
> *wdd)
> if (IS_ERR_OR_NULL(watchdog_kworker))
> return -ENODEV;
>
> + device_initialize(_data->dev);
> + wd_data->dev.devt = MKDEV(MAJOR(watchdog_devt), wdd->id);
> + wd_data->dev.class = _class;
> + wd_data->dev.parent = wdd->parent;
> + wd_data->dev.groups = wdd->groups;
> + wd_data->dev.release = watchdog_core_data_release;
> + dev_set_drvdata(_data->dev, wdd);
> + dev_set_name(_data->dev, "watchdog%d", wdd->id);
> +
> kthread_init_work(_data->work, watchdog_ping_work);
> hrtimer_init(_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> wd_data->timer.function = watchdog_timer_expired;
> @@ -967,15 +976,6 @@ static int watchdog_cdev_register(struct watchdog_device
> *wdd)
> }
> }
>
> - device_initialize(_data->dev);
> - wd_data->dev.devt = MKDEV(MAJOR(watchdog_devt), wdd->id);
> - wd_data->dev.class = _class;
> - wd_data->dev.parent = wdd->parent;
> - wd_data->dev.groups = wdd->groups;
> - wd_data->dev.release = watchdog_core_data_release;
> - dev_set_drvdata(_data->dev, wdd);
> - dev_set_name(_data->dev, "watchdog%d", wdd->id);
> -
> /* Fill in the data structures */
> cdev_init(_data->cdev, _fops);
>
> --
> 2.14.0
>
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] Replace HTTP links with HTTPS ones: EP93XX
Hello Alexander, On 06/07/2020 08:27, Alexander A. Klimov wrote: > Rationale: > Reduces attack surface on kernel devs opening the links for MITM > as HTTPS traffic is much harder to manipulate. > > Deterministic algorithm: > For each file: > If not .svg: > For each line: > If doesn't contain `\bxmlns\b`: > For each link, `\bhttp://[^# \t\r\n]*(?:\w|/)`: > If both the HTTP and HTTPS versions > return 200 OK and serve the same content: > Replace HTTP with HTTPS. > > Signed-off-by: Alexander A. Klimov Acked-by: Alexander Sverdlin > --- > Continuing my work started at 93431e0607e5. > > If there are any URLs to be removed completely or at least not HTTPSified: > Just clearly say so and I'll *undo my change*. > See also https://lkml.org/lkml/2020/6/27/64 > > If there are any valid, but yet not changed URLs: > See https://lkml.org/lkml/2020/6/26/837 > > arch/arm/mach-ep93xx/clock.c | 2 +- > arch/arm/mach-ep93xx/soc.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c > index 2810eb5b2aca..48efefbb54f8 100644 > --- a/arch/arm/mach-ep93xx/clock.c > +++ b/arch/arm/mach-ep93xx/clock.c > @@ -571,7 +571,7 @@ static int __init ep93xx_clock_init(void) > /* >* EP93xx SSP clock rate was doubled in version E2. For more information >* see: > - * http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf > + * https://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf >*/ > if (ep93xx_chip_revision() < EP93XX_CHIP_REV_E2) > clk_spi.rate /= 2; > diff --git a/arch/arm/mach-ep93xx/soc.h b/arch/arm/mach-ep93xx/soc.h > index f2dace1c9154..9e4a3b245434 100644 > --- a/arch/arm/mach-ep93xx/soc.h > +++ b/arch/arm/mach-ep93xx/soc.h > @@ -28,7 +28,7 @@ > * Configurations. Please refer to "AN273: EP93xx Silicon Rev E Design > * Guidelines" for more information. This document can be found at: > * > - * http://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf > + * https://www.cirrus.com/en/pubs/appNote/AN273REV4.pdf > */ > > #define EP93XX_CS0_PHYS_BASE_ASYNC 0x /* ASDO Pin = 0 */
Multicast from underlying MACVLAN interface towards MACVLAN
droom: : 00 28 b3 4d 84 88 ff ff b2 72 b9 5e 00 00 00 00 skb headroom: 0010: 00 00 00 00 00 00 00 00 00 00 skb linear: : 33 33 00 00 00 01 02 40 43 80 00 00 86 dd 60 09 skb linear: 0010: 88 bd 00 38 3a ff fe 80 00 00 00 00 00 00 00 40 skb linear: 0020: 43 ff fe 80 00 00 ff 02 00 00 00 00 00 00 00 00 skb linear: 0030: 00 00 00 00 00 01 86 00 61 00 40 00 00 2d 00 00 skb linear: 0040: 00 00 00 00 00 00 03 04 40 e0 00 00 01 2c 00 00 skb linear: 0050: 00 78 00 00 00 00 fd 5f 42 68 23 87 a8 81 00 00 skb linear: 0060: 00 00 00 00 00 00 01 01 02 40 43 80 00 00 skb tailroom: : 00 f0 01 00 00 00 00 00 a4 73 00 00 00 00 00 00 skb tailroom: 0010: a4 73 00 00 00 00 00 00 00 10 00 00 00 00 00 00 skb tailroom: 0020: 01 00 00 00 06 00 00 00 40 66 02 00 00 00 00 00 skb tailroom: 0030: 40 76 02 00 00 00 00 00 Call Trace: dump_stack+0x69/0x9b debug_hdr+0x4c/0x60 eth_header+0x71/0xe0 vlan_dev_hard_header+0x58/0x140 [8021q] neigh_connected_output+0xa9/0x100 ip6_finish_output2+0x24a/0x590 [ipv6] ? ip6_cork_release.isra.1+0x64/0x90 [ipv6] ? __ip6_make_skb+0x38d/0x680 [ipv6] ? ip6_output+0x6c/0x140 [ipv6] ip6_output+0x6c/0x140 [ipv6] ip6_send_skb+0x1e/0x60 [ipv6] rawv6_sendmsg+0xc4b/0xe10 [ipv6] ? proc_put_long+0xd0/0xd0 ? rw_copy_check_uvector+0x4e/0x110 ? sock_sendmsg+0x36/0x40 sock_sendmsg+0x36/0x40 ___sys_sendmsg+0x2b6/0x2d0 ? proc_dointvec+0x23/0x30 ? addrconf_sysctl_forward+0x8d/0x250 [ipv6] ? dev_forward_change+0x130/0x130 [ipv6] ? _raw_spin_unlock+0x12/0x30 ? proc_sys_call_handler.isra.14+0x9f/0x110 ? __call_rcu+0x213/0x510 ? get_max_files+0x10/0x10 ? trace_hardirqs_on+0x2c/0xe0 ? __sys_sendmsg+0x63/0xa0 __sys_sendmsg+0x63/0xa0 do_syscall_64+0x6c/0x1e0 entry_SYSCALL_64_after_hwframe+0x49/0xbe I would appreciate any hint, how to approach this problem! I can try to come up with a patch, but as this is so central thing in the IP protocol, I'd like to hear some opinions first... -- Best regards, Alexander Sverdlin.
Re: [PATCH 2/6] ARM: ep93xx: enable SPARSE_IRQ
Hi! On 20/10/2019 13:49, Arnd Bergmann wrote: >>> Ah, that makes sense. so all interrupt numbers need to >>> be shifted by a fixed number (e.g. 1) like we did for >>> other platforms (see attachment). >> Yes, the below patch resolved both GPIO and DMA issues. ^^^ >> Previous patch (selecting IRQ_DOMAIN_HIERARCHY) is not >> required. >> >> If you re-spin all 3 ep93xx-relevant patches together, you can put my >> Tested-by: Alexander Sverdlin >> on them. > Awesome, thanks for testing. > > I only remember sending two patches for ep93xx: > ARM: ep93xx: make mach/ep93xx-regs.h local > ARM: ep93xx: enable SPARSE_IRQ > > and have added the Tested-by tag to them now. Is there a third one > I missed? The patch shifting the IRQ-numbering by one is a prerequisite for the two above patches, right? -- Alex.
Re: [PATCH 2/6] ARM: ep93xx: enable SPARSE_IRQ
Hi!
On Sat, 19 Oct 2019 22:44:18 +0200
Arnd Bergmann wrote:
> > > > # cat /proc/interrupts
> > > >CPU0
> > > > 39:146 VIC 7 Edge eth0
> > > > 51: 162161 VIC 19 Edge ep93xx timer
> > > > 52:139 VIC 20 Edge uart-pl010
> > > > 53: 4 VIC 21 Edge ep93xx-spi
> > > > 60: 0 VIC 28 Edge ep93xx-i2s
> > > > Err: 0
> > >
> > > I guess that is partial success: some irqs do work ;-)
> >
> > Yep, VIC1 is working, while VIC0 is not.
> >
> > > The two interrupts that did not get registered are for the
> > > dmaengine driver, and that makes sense given the error
> > > message about the DMA not working. No idea how
> > > that would be a result of the irq changes though.
> >
> > Seems, that it has exposed some incompatibilities of
> > starting IRQ 0 in EP93xx platform fir VIC0 and VIC code
> > itself, which assumes 0 means "auto assignment" (refer
> > to vic_init()).
>
> Ah, that makes sense. so all interrupt numbers need to
> be shifted by a fixed number (e.g. 1) like we did for
> other platforms (see attachment).
Yes, the below patch resolved both GPIO and DMA issues.
Previous patch (selecting IRQ_DOMAIN_HIERARCHY) is not
required.
If you re-spin all 3 ep93xx-relevant patches together, you can put my
Tested-by: Alexander Sverdlin
on them.
> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> index 6fb19a393fd2..f0a71d4e076f 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -47,6 +47,7 @@
> #include
>
> #include "soc.h"
> +#include "irqs.h"
>
> /*
> * Static I/O mappings that are needed for all EP93xx platforms
> @@ -75,8 +76,8 @@ void __init ep93xx_map_io(void)
> */
> void __init ep93xx_init_irq(void)
> {
> - vic_init(EP93XX_VIC1_BASE, 0, EP93XX_VIC1_VALID_IRQ_MASK, 0);
> - vic_init(EP93XX_VIC2_BASE, 32, EP93XX_VIC2_VALID_IRQ_MASK, 0);
> + vic_init(EP93XX_VIC1_BASE, IRQ_EP93XX_VIC0, EP93XX_VIC1_VALID_IRQ_MASK,
> 0);
> + vic_init(EP93XX_VIC2_BASE, IRQ_EP93XX_VIC1, EP93XX_VIC2_VALID_IRQ_MASK,
> 0);
> }
>
>
> diff --git a/arch/arm/mach-ep93xx/irqs.h b/arch/arm/mach-ep93xx/irqs.h
> index 3ffdb3a2f3e4..353201b90c66 100644
> --- a/arch/arm/mach-ep93xx/irqs.h
> +++ b/arch/arm/mach-ep93xx/irqs.h
> @@ -2,69 +2,73 @@
> #ifndef __ASM_ARCH_IRQS_H
> #define __ASM_ARCH_IRQS_H
>
> -#define IRQ_EP93XX_COMMRX2
> -#define IRQ_EP93XX_COMMTX3
> -#define IRQ_EP93XX_TIMER14
> -#define IRQ_EP93XX_TIMER25
> -#define IRQ_EP93XX_AACINTR 6
> -#define IRQ_EP93XX_DMAM2P0 7
> -#define IRQ_EP93XX_DMAM2P1 8
> -#define IRQ_EP93XX_DMAM2P2 9
> -#define IRQ_EP93XX_DMAM2P3 10
> -#define IRQ_EP93XX_DMAM2P4 11
> -#define IRQ_EP93XX_DMAM2P5 12
> -#define IRQ_EP93XX_DMAM2P6 13
> -#define IRQ_EP93XX_DMAM2P7 14
> -#define IRQ_EP93XX_DMAM2P8 15
> -#define IRQ_EP93XX_DMAM2P9 16
> -#define IRQ_EP93XX_DMAM2M0 17
> -#define IRQ_EP93XX_DMAM2M1 18
> -#define IRQ_EP93XX_GPIO0MUX 19
> -#define IRQ_EP93XX_GPIO1MUX 20
> -#define IRQ_EP93XX_GPIO2MUX 21
> -#define IRQ_EP93XX_GPIO3MUX 22
> -#define IRQ_EP93XX_UART1RX 23
> -#define IRQ_EP93XX_UART1TX 24
> -#define IRQ_EP93XX_UART2RX 25
> -#define IRQ_EP93XX_UART2TX 26
> -#define IRQ_EP93XX_UART3RX 27
> -#define IRQ_EP93XX_UART3TX 28
> -#define IRQ_EP93XX_KEY 29
> -#define IRQ_EP93XX_TOUCH 30
> +#define IRQ_EP93XX_VIC0 1
> +
> +#define IRQ_EP93XX_COMMRX(IRQ_EP93XX_VIC0 + 2)
> +#define IRQ_EP93XX_COMMTX(IRQ_EP93XX_VIC0 + 3)
> +#define IRQ_EP93XX_TIMER1(IRQ_EP93XX_VIC0 + 4)
> +#define IRQ_EP93XX_TIMER2(IRQ_EP93XX_VIC0 + 5)
> +#define IRQ_EP93XX_AACINTR (IRQ_EP93XX_VIC0 + 6)
> +#define IRQ_EP93XX_DMAM2P0 (IRQ_EP93XX_VIC0 + 7)
> +#define IRQ_EP93XX_DMAM2P1 (IRQ_EP93XX_VIC0 + 8)
> +#define IRQ_EP93XX_DMAM2P2 (IRQ_EP93XX_VIC0 + 9)
> +#define IRQ_EP93XX_DMAM2P3 (IRQ_EP93XX_VIC0 + 10)
> +#define IRQ_EP93XX_DMAM2P4 (IRQ_EP93XX_VIC0 + 11)
> +#define IRQ_EP93XX_DMAM2P5 (IRQ_EP93
Re: [PATCH 2/6] ARM: ep93xx: enable SPARSE_IRQ
Hi! On Sat, 19 Oct 2019 22:08:40 +0200 Arnd Bergmann wrote: > > # cat /proc/interrupts > >CPU0 > > 39:146 VIC 7 Edge eth0 > > 51: 162161 VIC 19 Edge ep93xx timer > > 52:139 VIC 20 Edge uart-pl010 > > 53: 4 VIC 21 Edge ep93xx-spi > > 60: 0 VIC 28 Edge ep93xx-i2s > > Err: 0 > > I guess that is partial success: some irqs do work ;-) Yep, VIC1 is working, while VIC0 is not. > The two interrupts that did not get registered are for the > dmaengine driver, and that makes sense given the error > message about the DMA not working. No idea how > that would be a result of the irq changes though. Seems, that it has exposed some incompatibilities of starting IRQ 0 in EP93xx platform fir VIC0 and VIC code itself, which assumes 0 means "auto assignment" (refer to vic_init()). But there are more problems I didn't resolve yet. -- Alexander Sverdlin.
Re: [PATCH 2/6] ARM: ep93xx: enable SPARSE_IRQ
Hello Arnd,
On Fri, 18 Oct 2019 18:29:15 +0200
Arnd Bergmann wrote:
> Without CONFIG_SPARSE_IRQ, we rely on mach/irqs.h to define NR_IRQS
> globally. Do the minimal conversion by setting .nr_irqs in each
> machine descriptor.
>
> Only the vision_ep9307 machine has extra IRQs for GPIOs, so make
> .nr_irqs the original value there, while using the plain NR_EP93XX_IRQS
> everywhere else.
This patch causes multiple problems on EDB9302:
1. WARNINGs during gpiochip registration, for instance:
[ cut here ]
WARNING: CPU: 0 PID: 1 at kernel/irq/chip.c:1013 __irq_do_set_handler+0x94/0x188
CPU: 0 PID: 1 Comm: swapper Tainted: GW 5.4.0-rc3 #1
Hardware name: Cirrus Logic EDB9302 Evaluation Board
[] (unwind_backtrace) from [] (show_stack+0x10/0x18)
[] (show_stack) from [] (dump_stack+0x18/0x24)
[] (dump_stack) from [] (__warn+0xa4/0xc8)
[] (__warn) from [] (warn_slowpath_fmt+0xa8/0xb8)
[] (warn_slowpath_fmt) from []
(__irq_do_set_handler+0x94/0x188)
[] (__irq_do_set_handler) from []
(irq_set_chained_handler_and_data+0x48/0x7c)
[] (irq_set_chained_handler_and_data) from []
(gpiochip_add_data_with_key+0x6d4/0xabc)
[] (gpiochip_add_data_with_key) from []
(devm_gpiochip_add_data+0x40/0x88)
[] (devm_gpiochip_add_data) from []
(ep93xx_gpio_probe+0x1ac/0x280)
[] (ep93xx_gpio_probe) from []
(platform_drv_probe+0x28/0x6c)
[] (platform_drv_probe) from [] (really_probe+0x1c8/0x340)
[] (really_probe) from [] (bus_for_each_drv+0x58/0xc0)
[] (bus_for_each_drv) from [] (__device_attach+0xb4/0x104)
[] (__device_attach) from [] (bus_probe_device+0x8c/0x94)
[] (bus_probe_device) from [] (device_add+0x3d0/0x59c)
[] (device_add) from [] (platform_device_add+0x100/0x20c)
[] (platform_device_add) from []
(ep93xx_init_devices+0x16c/0x20c)
[] (ep93xx_init_devices) from []
(edb93xx_init_machine+0xc/0x84)
[] (edb93xx_init_machine) from []
(customize_machine+0x20/0x38)
[] (customize_machine) from [] (do_one_initcall+0x78/0x1a0)
[] (do_one_initcall) from []
(kernel_init_freeable+0x104/0x1b8)
[] (kernel_init_freeable) from [] (kernel_init+0x8/0xf8)
[] (kernel_init) from [] (ret_from_fork+0x14/0x24)
Exception stack(0xc4433fb0 to 0xc4433ff8)
3fa0:
3fc0:
3fe0: 0013
---[ end trace 8f9e35e2d6224882 ]---
2. Broken sound (I2S), this looks like below in the log:
ep93xx-i2s ep93xx-i2s: Missing dma channel for stream: 0
CS4271: ASoC: pcm constructor failed: -22
edb93xx-audio edb93xx-audio: ASoC: can't create pcm CS4271 HiFi :-22
And /proc/interrupts has two entries less. Without patch:
# cat /proc/interrupts
CPU0
7: 0 VIC 7 Edge i2s-pcm-out
8: 0 VIC 8 Edge i2s-pcm-in
39: 2 VIC 7 Edge eth0
51: 7532 VIC 19 Edge ep93xx timer
52:144 VIC 20 Edge uart-pl010
53: 4 VIC 21 Edge ep93xx-spi
60: 0 VIC 28 Edge ep93xx-i2s
Err: 0
With patch:
# cat /proc/interrupts
CPU0
39:146 VIC 7 Edge eth0
51: 162161 VIC 19 Edge ep93xx timer
52:139 VIC 20 Edge uart-pl010
53: 4 VIC 21 Edge ep93xx-spi
60: 0 VIC 28 Edge ep93xx-i2s
Err: 0
I will try to look into I2S problem...
> ---
> It's been a while since I did this, no idea what else is needed
> here or if this is correct at all.
>
> Cc: Hartley Sweeten
> Cc: Alexander Sverdlin
> Cc: Hubert Feurstein
> Cc: Lukasz Majewski
> Signed-off-by: Arnd Bergmann
> ---
> arch/arm/Kconfig | 2 ++
> arch/arm/mach-ep93xx/adssphere.c | 1 +
> arch/arm/mach-ep93xx/edb93xx.c | 8
> arch/arm/mach-ep93xx/gesbc9312.c | 1 +
> arch/arm/mach-ep93xx/{include/mach => }/irqs.h | 7 ---
> arch/arm/mach-ep93xx/micro9.c | 4
> arch/arm/mach-ep93xx/simone.c | 1 +
> arch/arm/mach-ep93xx/snappercl15.c | 1 +
> arch/arm/mach-ep93xx/soc.h | 1 +
> arch/arm/mach-ep93xx/ts72xx.c | 3 ++-
> arch/arm/mach-ep93xx/vision_ep9307.c | 1 +
> 11 files changed, 22 insertions(+), 8 deletions(-)
> rename arch/arm/mach-ep93xx/{include/mach => }/irqs.h (94%)
--
Alexander Sverdlin.
Re: [PATCH] ARM: ep93xx: Mark expected switch fall-through
Hi! On 08/08/2019 04:38, Gustavo A. R. Silva wrote: Mark switch cases where we are expecting to fall through. Fix the following warnings (Building: arm-ep93xx_defconfig arm): arch/arm/mach-ep93xx/crunch.c: In function 'crunch_do': arch/arm/mach-ep93xx/crunch.c:46:3: warning: this statement may fall through [-Wimplicit-fallthrough=] memset(crunch_state, 0, sizeof(*crunch_state)); ^~ arch/arm/mach-ep93xx/crunch.c:53:2: note: here case THREAD_NOTIFY_EXIT: ^~~~ Notice that, in this particular case, the code comment is modified in accordance with what GCC is expecting to find. Reported-by: kbuild test robot Acked-by: Alexander Sverdlin Signed-off-by: Gustavo A. R. Silva --- arch/arm/mach-ep93xx/crunch.c | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/mach-ep93xx/crunch.c b/arch/arm/mach-ep93xx/crunch.c index 1c9a4be8b503..1c05c5bf7e5c 100644 --- a/arch/arm/mach-ep93xx/crunch.c +++ b/arch/arm/mach-ep93xx/crunch.c @@ -49,6 +49,7 @@ static int crunch_do(struct notifier_block *self, unsigned long cmd, void *t) * FALLTHROUGH: Ensure we don't try to overwrite our newly * initialised state information on the first fault. */ + /* Fall through */ case THREAD_NOTIFY_EXIT: crunch_task_release(thread);
Re: [PATCH 2/4] ARM: ep93xx: keypad: stop using mach/platform.h
Hi! On 15/04/2019 21:47, Arnd Bergmann wrote: >>> We can communicate the clock rate using platform data rather than setting a >>> flag to use a particular value in the driver, which is cleaner and avoids >>> the dependency. >>> >>> No platform in the kernel currently defines the ep93xx keypad device >>> structure, so this >>> is a rather pointless excercise. Any out of tree users are probably dead >>> now, but if not, >>> they have to change their platform code to match the new platform_data >>> structure. >> >> >>> diff --git a/include/linux/platform_data/keypad-ep93xx.h >>> b/include/linux/platform_data/keypad-ep93xx.h >>> index 0e36818e3680..3054fced8509 100644 >>> --- a/include/linux/platform_data/keypad-ep93xx.h >>> +++ b/include/linux/platform_data/keypad-ep93xx.h >>> @@ -9,8 +9,7 @@ struct matrix_keymap_data; >>> #define EP93XX_KEYPAD_DIAG_MODE (1<<1) /* diagnostic mode */ >>> #define EP93XX_KEYPAD_BACK_DRIVE (1<<2) /* back driving mode */ >>> #define EP93XX_KEYPAD_TEST_MODE (1<<3) /* scan only column 0 >>> */ >>> -#define EP93XX_KEYPAD_KDIV (1<<4) /* 1/4 clock or 1/16 clock */ >>> -#define EP93XX_KEYPAD_AUTOREPEAT (1<<5) /* enable key autorepeat */ >>> +#define EP93XX_KEYPAD_AUTOREPEAT (1<<4) /* enable key autorepeat */ >> You have re-defined the keypad register bits here. >> >> The KDIV bit changes the clock rate. The AUTOREPEAT bit enables key >> autorepeat. > As far as I can tell, they are not register bits, just software flags > for communicating between a board file and the driver, so I > assumed I could freely reorganize them. > > Did I miss something? They are indeed only software flags (just checked datasheet), so you are only changing platform data format. But as I do not know any keypad user in person, I'd rely on Hartley's opinion in this case (if it's a good idea to change platform data or not). -- Alex.
Re: [PATCH 4/4] ARM: ep93xx: move private headers out of mach/*
On 15/04/2019 21:31, Arnd Bergmann wrote:
> gpio-ep93xx.h, hardware.h, and platform.h are only used in
> arch/arm/mach-ep93xx, so we can move them one there and no
> longer expose them to device drivers.
Acked-by: Alexander Sverdlin
> Signed-off-by: Arnd Bergmann
> ---
> arch/arm/mach-ep93xx/adssphere.c | 2 +-
> arch/arm/mach-ep93xx/clock.c | 2 +-
> arch/arm/mach-ep93xx/core.c | 6 +++---
> arch/arm/mach-ep93xx/dma.c| 2 +-
> arch/arm/mach-ep93xx/edb93xx.c| 4 ++--
> arch/arm/mach-ep93xx/gesbc9312.c | 2 +-
> arch/arm/mach-ep93xx/{include/mach => }/gpio-ep93xx.h | 0
> arch/arm/mach-ep93xx/{include/mach => }/hardware.h| 2 +-
> arch/arm/mach-ep93xx/micro9.c | 2 +-
> arch/arm/mach-ep93xx/{include/mach => }/platform.h| 0
> arch/arm/mach-ep93xx/simone.c | 4 ++--
> arch/arm/mach-ep93xx/snappercl15.c| 4 ++--
> arch/arm/mach-ep93xx/ts72xx.c | 4 ++--
> arch/arm/mach-ep93xx/vision_ep9307.c | 4 ++--
> 14 files changed, 19 insertions(+), 19 deletions(-)
> rename arch/arm/mach-ep93xx/{include/mach => }/gpio-ep93xx.h (100%)
> rename arch/arm/mach-ep93xx/{include/mach => }/hardware.h (96%)
> rename arch/arm/mach-ep93xx/{include/mach => }/platform.h (100%)
>
> diff --git a/arch/arm/mach-ep93xx/adssphere.c
> b/arch/arm/mach-ep93xx/adssphere.c
> index bda6c3a5c923..5d3a3e302012 100644
> --- a/arch/arm/mach-ep93xx/adssphere.c
> +++ b/arch/arm/mach-ep93xx/adssphere.c
> @@ -15,7 +15,7 @@
> #include
> #include
>
> -#include
> +#include "hardware.h"
>
> #include
> #include
> diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
> index 9f43362eb62d..b9f523d9dc8c 100644
> --- a/arch/arm/mach-ep93xx/clock.c
> +++ b/arch/arm/mach-ep93xx/clock.c
> @@ -22,7 +22,7 @@
> #include
> #include
>
> -#include
> +#include "hardware.h"
>
> #include
>
> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> index 3d245668846d..cc1382f879af 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -39,13 +39,13 @@
> #include
> #include
>
> -#include
> +#include "hardware.h"
> #include
> #include
> #include
> #include
>
> -#include
> +#include "gpio-ep93xx.h"
>
> #include
> #include
> @@ -125,7 +125,7 @@ void ep93xx_devcfg_set_clear(unsigned int set_bits,
> unsigned int clear_bits)
> /**
> * ep93xx_chip_revision() - returns the EP93xx chip revision
> *
> - * See for more information.
> + * See "platform.h" for more information.
> */
> unsigned int ep93xx_chip_revision(void)
> {
> diff --git a/arch/arm/mach-ep93xx/dma.c b/arch/arm/mach-ep93xx/dma.c
> index 88a4c9b089a5..821427107b11 100644
> --- a/arch/arm/mach-ep93xx/dma.c
> +++ b/arch/arm/mach-ep93xx/dma.c
> @@ -26,7 +26,7 @@
> #include
>
> #include
> -#include
> +#include "hardware.h"
>
> #include "soc.h"
>
> diff --git a/arch/arm/mach-ep93xx/edb93xx.c b/arch/arm/mach-ep93xx/edb93xx.c
> index 8e89ec8b6f0f..d96dd014dd23 100644
> --- a/arch/arm/mach-ep93xx/edb93xx.c
> +++ b/arch/arm/mach-ep93xx/edb93xx.c
> @@ -32,10 +32,10 @@
>
> #include
>
> -#include
> +#include "hardware.h"
> #include
> #include
> -#include
> +#include "gpio-ep93xx.h"
>
> #include
> #include
> diff --git a/arch/arm/mach-ep93xx/gesbc9312.c
> b/arch/arm/mach-ep93xx/gesbc9312.c
> index 0cca5b183309..ac48e3476587 100644
> --- a/arch/arm/mach-ep93xx/gesbc9312.c
> +++ b/arch/arm/mach-ep93xx/gesbc9312.c
> @@ -15,7 +15,7 @@
> #include
> #include
>
> -#include
> +#include "hardware.h"
>
> #include
> #include
> diff --git a/arch/arm/mach-ep93xx/include/mach/gpio-ep93xx.h
> b/arch/arm/mach-ep93xx/gpio-ep93xx.h
> similarity index 100%
> rename from arch/arm/mach-ep93xx/include/mach/gpio-ep93xx.h
> rename to arch/arm/mach-ep93xx/gpio-ep93xx.h
> diff --git a/arch/arm/mach-ep93xx/include/mach/hardware.h
> b/arch/arm/mach-ep93xx/hardware.h
> similarity index 96%
> rename from arch/arm/mach-ep93xx/include/mach/hardware.h
> rename to arch/arm/mach-ep93xx/hardware.h
> index 8938906e780a..e7d850e04782 100644
> --- a/arch/arm/mach-ep93xx/include/mach/hardware.h
> +++ b/arch/arm/mach-ep93xx/hardware.h
> @@ -6,7 +6,7 @@
> #ifndef __ASM_ARCH_HARDWARE_H
> #define __A
Re: [PATCH 3/4] ARM: ep93xx: move pinctrl interfaces into include/linux/soc
On 15/04/2019 21:25, Arnd Bergmann wrote:
> ep93xx does not have a proper pinctrl driver, but does things
> ad-hoc through mach/platform.h, which is also used for setting
> up the boards.
>
> To avoid using mach/*.h headers completely, let's move the interfaces
> into include/linux/soc/. This is far from great, but gets the job
> done here, without the need for a proper pinctrl driver.
Acked-by: Alexander Sverdlin
> Signed-off-by: Arnd Bergmann
> ---
> arch/arm/mach-ep93xx/clock.c | 1 +
> arch/arm/mach-ep93xx/core.c | 2 ++
> arch/arm/mach-ep93xx/include/mach/platform.h | 16 -
> drivers/ata/pata_ep93xx.c| 2 +-
> drivers/input/keyboard/ep93xx_keypad.c | 3 +-
> drivers/pwm/pwm-ep93xx.c | 2 +-
> include/linux/soc/cirrus/ep93xx.h| 37
> sound/soc/cirrus/edb93xx.c | 2 +-
> sound/soc/cirrus/ep93xx-ac97.c | 1 +
> sound/soc/cirrus/ep93xx-i2s.c| 3 +-
> sound/soc/cirrus/simone.c| 2 +-
> sound/soc/cirrus/snappercl15.c | 2 +-
> 12 files changed, 48 insertions(+), 25 deletions(-)
> create mode 100644 include/linux/soc/cirrus/ep93xx.h
>
> diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
> index d2eee707d27f..9f43362eb62d 100644
> --- a/arch/arm/mach-ep93xx/clock.c
> +++ b/arch/arm/mach-ep93xx/clock.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> index 706515faee06..3d245668846d 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -43,6 +43,8 @@
> #include
> #include
> #include
> +#include
> +
> #include
>
> #include
> diff --git a/arch/arm/mach-ep93xx/include/mach/platform.h
> b/arch/arm/mach-ep93xx/include/mach/platform.h
> index 43446f33c2be..b4045a186239 100644
> --- a/arch/arm/mach-ep93xx/include/mach/platform.h
> +++ b/arch/arm/mach-ep93xx/include/mach/platform.h
> @@ -19,14 +19,6 @@ struct ep93xx_spi_info;
> void ep93xx_map_io(void);
> void ep93xx_init_irq(void);
>
> -#define EP93XX_CHIP_REV_D0 3
> -#define EP93XX_CHIP_REV_D1 4
> -#define EP93XX_CHIP_REV_E0 5
> -#define EP93XX_CHIP_REV_E1 6
> -#define EP93XX_CHIP_REV_E2 7
> -
> -unsigned int ep93xx_chip_revision(void);
> -
> void ep93xx_register_flash(unsigned int width,
> resource_size_t start, resource_size_t size);
>
> @@ -36,19 +28,11 @@ void ep93xx_register_spi(struct ep93xx_spi_info *info,
>struct spi_board_info *devices, int num);
> void ep93xx_register_fb(struct ep93xxfb_mach_info *data);
> void ep93xx_register_pwm(int pwm0, int pwm1);
> -int ep93xx_pwm_acquire_gpio(struct platform_device *pdev);
> -void ep93xx_pwm_release_gpio(struct platform_device *pdev);
> void ep93xx_register_keypad(struct ep93xx_keypad_platform_data *data);
> -int ep93xx_keypad_acquire_gpio(struct platform_device *pdev);
> -void ep93xx_keypad_release_gpio(struct platform_device *pdev);
> void ep93xx_register_i2s(void);
> -int ep93xx_i2s_acquire(void);
> -void ep93xx_i2s_release(void);
> void ep93xx_register_ac97(void);
> void ep93xx_register_ide(void);
> void ep93xx_register_adc(void);
> -int ep93xx_ide_acquire_gpio(struct platform_device *pdev);
> -void ep93xx_ide_release_gpio(struct platform_device *pdev);
>
> struct device *ep93xx_init_devices(void);
> extern void ep93xx_timer_init(void);
> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index cc6d06c1b2c7..db271b705529 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
> @@ -44,7 +44,7 @@
> #include
>
> #include
> -#include
> +#include
>
> #define DRV_NAME "ep93xx-ide"
> #define DRV_VERSION "1.0"
> diff --git a/drivers/input/keyboard/ep93xx_keypad.c
> b/drivers/input/keyboard/ep93xx_keypad.c
> index 71472f6257c0..575dac52f7b4 100644
> --- a/drivers/input/keyboard/ep93xx_keypad.c
> +++ b/drivers/input/keyboard/ep93xx_keypad.c
> @@ -27,8 +27,7 @@
> #include
> #include
> #include
> -
> -#include
> +#include
> #include
>
> /*
> diff --git a/drivers/pwm/pwm-ep93xx.c b/drivers/pwm/pwm-ep93xx.c
> index bbf10ae02f0e..fa168581e6b8 100644
> --- a/drivers/pwm/pwm-ep93xx.c
> +++ b/drivers/pwm/pwm-ep93xx.c
> @@ -35,7 +35,7 @@
>
> #include
>
> -#include/* for ep93xx_pwm_{acquire,release}_gpio() */
> +#include /* for
> ep93xx_pwm_{acquire,release}_gpio() */
>
Re: kmsg: lseek errors confuse glibc's dprintf
Hello Mike and all, On Thu, 15 Jan 2015 17:31:32 + Mike Crowe wrote: > glibc's dprintf implementation does not work correctly with /dev/kmsg file > descriptors because glibc treats receiving EBADF and EINVAL from lseek when > trying to determine the current file position as errors. See > https://sourceware.org/bugzilla/show_bug.cgi?id=17830 we need to conclude on this issue. This is a real bug which is ignored for 4 years now. Mike, would you like to re-send a formal patch? I can do it as well, preserving a link to your original patch/report. In case you'd like to post it yourself, I can be a tester and/or provide a reproducer. > >>From what I can tell prior to Kay Sievers printk record commit > e11fea92e13fb91c50bacca799a6131c81929986, calling lseek(fd, 0, SEEK_CUR) > with such a file descriptor would not return an error. > > Prior to Kay's change, Arnd Bergmann's commit > 6038f373a3dc1f1c26496e60b6c40b164716f07e seemed to go to some lengths to > preserve the successful return code rather than returning (the perhaps more > logical) -ESPIPE. > > glibc is happy with either a successful return or -ESPIPE. > > For maximum compatibility it seems that success should be returned but > given Kay's new seek interface perhaps this isn't helpful. > > This patch ensures that such a seek succeeds: > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 02d6b6d..b3ff6f0 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t > offset, int whence) > loff_t ret = 0; > > if (!user) > - return -EBADF; > + return (whence == SEEK_CUR) ? 0 : -EBADF; > if (offset) > return -ESPIPE; > > @@ -718,6 +718,11 @@ static loff_t devkmsg_llseek(struct file *file, loff_t > offset, int whence) > user->idx = log_next_idx; > user->seq = log_next_seq; > break; > + case SEEK_CUR: > + /* For compatibility with userspace requesting the > + * current file position. */ > + ret = 0; > + break; > default: > ret = -EINVAL; > } > > (although it could be argued that the !user case should return -ESPIPE > rather than EBADF since the file descriptor _is_ valid.) > > and this patch causes a failure that glibc is prepared to accept: > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 02d6b6d..f6b0c93 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -693,7 +693,7 @@ static loff_t devkmsg_llseek(struct file *file, loff_t > offset, int whence) > loff_t ret = 0; > > if (!user) > - return -EBADF; > + return -ESPIPE; > if (offset) > return -ESPIPE; > > @@ -718,6 +718,11 @@ static loff_t devkmsg_llseek(struct file *file, loff_t > offset, int whence) > user->idx = log_next_idx; > user->seq = log_next_seq; > break; > + case SEEK_CUR: > + /* For compatibility with userspace expecting SEEK_CUR > + * to not yield EINVAL. */ > + ret = -ESPIPE; > + break; > default: > ret = -EINVAL; > } > > Either makes dprintf work, but is either the right solution? > > Thanks. > > Mike. -- Alexander Sverdlin.
Re: [PATCH] arch: arm: Kconfig: pedantic formatting
Hi! On 11/03/2019 14:56, Enrico Weigelt, metux IT consult wrote: > Formatting of Kconfig files doesn't look so pretty, so let the > Great White Handkerchief come around and clean it up. > > Signed-off-by: Enrico Weigelt, metux IT consult > --- > arch/arm/Kconfig | 24 +++--- > arch/arm/mach-aspeed/Kconfig | 10 - > arch/arm/mach-ep93xx/Kconfig | 8 > arch/arm/mach-hisi/Kconfig| 14 ++--- > arch/arm/mach-ixp4xx/Kconfig | 32 ++--- > arch/arm/mach-mmp/Kconfig | 2 +- > arch/arm/mach-omap1/Kconfig | 36 > arch/arm/mach-omap2/Kconfig | 12 +-- > arch/arm/mach-prima2/Kconfig | 6 +++--- > arch/arm/mach-s3c24xx/Kconfig | 32 ++--- > arch/arm/mach-s3c64xx/Kconfig | 16 +++ > arch/arm/mach-sa1100/Kconfig | 4 ++-- > arch/arm/mach-vt8500/Kconfig | 6 +++--- > arch/arm/mach-w90x900/Kconfig | 6 +++--- > arch/arm/mm/Kconfig | 48 > +-- > arch/arm/plat-samsung/Kconfig | 26 +++ > 16 files changed, 140 insertions(+), 142 deletions(-) > [...] > diff --git a/arch/arm/mach-ep93xx/Kconfig b/arch/arm/mach-ep93xx/Kconfig > index c095236..597ea24 100644 > --- a/arch/arm/mach-ep93xx/Kconfig > +++ b/arch/arm/mach-ep93xx/Kconfig > @@ -125,10 +125,10 @@ config MACH_MICRO9S > Contec Micro9-Slim board. > > config MACH_SIM_ONE > -bool "Support Simplemachines Sim.One board" > -help > - Say 'Y' here if you want your kernel to support the > - Simplemachines Sim.One board. > + bool "Support Simplemachines Sim.One board" > + help > + Say 'Y' here if you want your kernel to support the > + Simplemachines Sim.One board. > > config MACH_SNAPPER_CL15 > bool "Support Bluewater Systems Snapper CL15 Module" For EP93xx part: Acked-by: Alexander Sverdlin It makes no sense to split the patch, one of the ARM maintainers shall decide on this patch anyway. In my personal opinion such a patch makes little sense if it's not tree-wide. -- Regards, Alexander.
Re: [PATCH v1 3/3] drivers/tty: increase priority for tty_buffer_worker
Hello Oleksij,
On Thu, 10 Jan 2019 11:12:32 +0100
Oleksij Rempel wrote:
> sched_priority = 1 is enough to dramatically reduce latency
> on have system load produced by tasks with default user space prio.
>
> Signed-off-by: Oleksij Rempel
Tested-by: Alexander Sverdlin
> ---
> drivers/tty/tty_buffer.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 18bd7f48a319..7cf42f6570a0 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -13,11 +13,13 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> #include
> +#include
>
>
> #define MIN_TTYB_SIZE256
> @@ -567,7 +569,15 @@ bool tty_buffer_queue_work(struct tty_port *port)
>
> void tty_buffer_init_kthread(void)
> {
> - kthread_run(kthread_worker_fn, _buffer_worker, "tty");
> + struct sched_param param = { .sched_priority = 1 };
> + struct task_struct *kworker_task;
> +
> + kworker_task = kthread_run(kthread_worker_fn, _buffer_worker,
> "tty");
> + if (IS_ERR(kworker_task)) {
> + pr_err("failed to create message pump task\n");
> + return;
> + }
> + sched_setscheduler(kworker_task, SCHED_FIFO, );
> }
>
> /**
--
Alexander Sverdlin.
Re: [PATCH v1 2/3] drivers/tty: convert tty_port to use kthread_worker
Hello Oleksij,
On Thu, 10 Jan 2019 11:12:31 +0100
Oleksij Rempel wrote:
> From: Steven Walter
>
> Use kthread_worker instead of workqueues. For now there is only a
> single workqueue, but the intention is to bring back the "low_latency"
> tty option, along with a second high-priority kthread worker.
>
> Even without a second worker this patch allows to give a higher priority
> to tty processing by modifying the priority of the corresponding
> kthread.
>
> Signed-off-by: Steven Walter
> Tested-by: Oleksij Rempel
Tested-by: Alexander Sverdlin
In general I agree with Linus, that a thread with some random
priority is suboptimal, and we actually should move this
work into the context of the receiving thread, to properly
inherit its priority. But this would be quite amount of rework.
In the meanwhile this patch series restores the "low_latency"
tty option. Thanks for your efforts!
> ---
> drivers/tty/tty_buffer.c | 21 ++---
> drivers/tty/tty_io.c | 1 +
> include/linux/tty.h | 7 ---
> 3 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index e0090e65d83a..18bd7f48a319 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -4,6 +4,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -497,7 +498,7 @@ receive_buf(struct tty_port *port, struct tty_buffer
> *head, int count)
> *'consumer'
> */
>
> -static void flush_to_ldisc(struct work_struct *work)
> +static void flush_to_ldisc(struct kthread_work *work)
> {
> struct tty_port *port = container_of(work, struct tty_port, buf.work);
> struct tty_bufhead *buf = >buf;
> @@ -557,10 +558,16 @@ void tty_flip_buffer_push(struct tty_port *port)
> }
> EXPORT_SYMBOL(tty_flip_buffer_push);
>
> +static DEFINE_KTHREAD_WORKER(tty_buffer_worker);
> +
> bool tty_buffer_queue_work(struct tty_port *port)
> {
> - struct tty_bufhead *buf = >buf;
> - return schedule_work(>work);
> + return kthread_queue_work(_buffer_worker, >buf.work);
> +}
> +
> +void tty_buffer_init_kthread(void)
> +{
> + kthread_run(kthread_worker_fn, _buffer_worker, "tty");
> }
>
> /**
> @@ -582,7 +589,7 @@ void tty_buffer_init(struct tty_port *port)
> init_llist_head(>free);
> atomic_set(>mem_used, 0);
> atomic_set(>priority, 0);
> - INIT_WORK(>work, flush_to_ldisc);
> + kthread_init_work(>work, flush_to_ldisc);
> buf->mem_limit = TTYB_DEFAULT_MEM_LIMIT;
> }
>
> @@ -614,12 +621,12 @@ bool tty_buffer_restart_work(struct tty_port *port)
> return tty_buffer_queue_work(port);
> }
>
> -bool tty_buffer_cancel_work(struct tty_port *port)
> +void tty_buffer_cancel_work(struct tty_port *port)
> {
> - return cancel_work_sync(>buf.work);
> + tty_buffer_flush_work(port);
> }
>
> void tty_buffer_flush_work(struct tty_port *port)
> {
> - flush_work(>buf.work);
> + kthread_flush_work(>buf.work);
> }
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index bfe9ad85b362..5fd7cecbe4e7 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3476,6 +3476,7 @@ void console_sysfs_notify(void)
> */
> int __init tty_init(void)
> {
> + tty_buffer_init_kthread();
> cdev_init(_cdev, _fops);
> if (cdev_add(_cdev, MKDEV(TTYAUX_MAJOR, 0), 1) ||
> register_chrdev_region(MKDEV(TTYAUX_MAJOR, 0), 1, "/dev/tty") < 0)
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 226a9eff0766..924c1093967e 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -3,9 +3,9 @@
> #define _LINUX_TTY_H
>
> #include
> +#include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -84,7 +84,7 @@ static inline char *flag_buf_ptr(struct tty_buffer *b, int
> ofs)
>
> struct tty_bufhead {
> struct tty_buffer *head;/* Queue head */
> - struct work_struct work;
> + struct kthread_work work;
> struct mutex lock;
> atomic_t priority;
> struct tty_buffer sentinel;
> @@ -510,8 +510,9 @@ extern void tty_buffer_flush(struct tty_struct *tty,
> struct tty_ldisc *ld);
> extern bool tty_buffer_queue_work(struct tty_port *port);
> extern void tty_buffer_init(struct tty_port *port);
> extern void tty_buffer_set_lock_subclass(struct tty_port *port);
> +extern void tty_buffer_init_kthread(void);
> extern bool tty_buffer_restart_work(struct tty_port *port);
> -extern bool tty_buffer_cancel_work(struct tty_port *port);
> +extern void tty_buffer_cancel_work(struct tty_port *port);
> extern void tty_buffer_flush_work(struct tty_port *port);
> extern speed_t tty_termios_baud_rate(struct ktermios *termios);
> extern speed_t tty_termios_input_baud_rate(struct ktermios *termios);
--
Alexander Sverdlin.
Re: [PATCH v1 1/3] drivers/tty: refactor functions for flushing/queuing work
Hello Oleksij,
On Thu, 10 Jan 2019 11:12:30 +0100
Oleksij Rempel wrote:
> From: Steven Walter
>
> Preparation for converting to kthread_worker
>
> Signed-off-by: Steven Walter
> Tested-by: Oleksij Rempel
Tested-by: Alexander Sverdlin
> ---
> drivers/tty/tty_buffer.c | 12 +---
> include/linux/tty.h | 1 +
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> index 77070c2d1240..e0090e65d83a 100644
> --- a/drivers/tty/tty_buffer.c
> +++ b/drivers/tty/tty_buffer.c
> @@ -72,7 +72,7 @@ void tty_buffer_unlock_exclusive(struct tty_port *port)
> atomic_dec(>priority);
> mutex_unlock(>lock);
> if (restart)
> - queue_work(system_unbound_wq, >work);
> + tty_buffer_queue_work(port);
> }
> EXPORT_SYMBOL_GPL(tty_buffer_unlock_exclusive);
>
> @@ -410,7 +410,7 @@ void tty_schedule_flip(struct tty_port *port)
>* flush_to_ldisc() sees buffer data.
>*/
> smp_store_release(>tail->commit, buf->tail->used);
> - queue_work(system_unbound_wq, >work);
> + tty_buffer_queue_work(port);
> }
> EXPORT_SYMBOL(tty_schedule_flip);
>
> @@ -557,6 +557,12 @@ void tty_flip_buffer_push(struct tty_port *port)
> }
> EXPORT_SYMBOL(tty_flip_buffer_push);
>
> +bool tty_buffer_queue_work(struct tty_port *port)
> +{
> + struct tty_bufhead *buf = >buf;
> + return schedule_work(>work);
> +}
> +
> /**
> * tty_buffer_init - prepare a tty buffer structure
> * @tty: tty to initialise
> @@ -605,7 +611,7 @@ void tty_buffer_set_lock_subclass(struct tty_port *port)
>
> bool tty_buffer_restart_work(struct tty_port *port)
> {
> - return queue_work(system_unbound_wq, >buf.work);
> + return tty_buffer_queue_work(port);
> }
>
> bool tty_buffer_cancel_work(struct tty_port *port)
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index bfa4e2ee94a9..226a9eff0766 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -507,6 +507,7 @@ extern void session_clear_tty(struct pid *session);
> extern void no_tty(void);
> extern void tty_buffer_free_all(struct tty_port *port);
> extern void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld);
> +extern bool tty_buffer_queue_work(struct tty_port *port);
> extern void tty_buffer_init(struct tty_port *port);
> extern void tty_buffer_set_lock_subclass(struct tty_port *port);
> extern bool tty_buffer_restart_work(struct tty_port *port);
--
Alexander Sverdlin.
Re: [PATCH] arch/arm/mach-ep93xx: Remove duplicate header
On 17/01/2019 15:37, Brajeswar Ghosh wrote: > Remove mach/gpio-ep93xx.h which is included more than once > > Signed-off-by: Brajeswar Ghosh Acked-by: Alexander Sverdlin Arnd, would you take it into one of your ARM trees? > --- > arch/arm/mach-ep93xx/ts72xx.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/arm/mach-ep93xx/ts72xx.c b/arch/arm/mach-ep93xx/ts72xx.c > index c6a533699b00..85b74ac943f0 100644 > --- a/arch/arm/mach-ep93xx/ts72xx.c > +++ b/arch/arm/mach-ep93xx/ts72xx.c > @@ -26,7 +26,6 @@ > #include > #include > #include > -#include > > #include > #include
Re: [PATCH 01/12] tools include: Adopt linux/bits.h
Hello Arnaldo,
On 09/10/2018 02:54, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo
>
> So that we reduce the difference of tools/include/linux/bitops.h to the
> original kernel file, include/linux/bitops.h, trying to remove the need
> to define BITS_PER_LONG, to avoid clashes with asm/bitsperlong.h.
thanks for looking into this, but I don't quite get your plan here,
you neither remove redefinition of BITS_PER_LONG in this patch, nor
in any following patch in the series.
Have you forgot to include another patch or shall I try to come up with
a patch removing BITS_PER_LONG from bitops.h?
> And the things removed from tools/include/linux/bitops.h are really in
> linux/bits.h, so that we can have a copy and then
> tools/perf/check_headers.sh will tell us when new stuff gets added to
> linux/bits.h so that we can check if it is useful and if any adjustment
> needs to be done to the tools/{include,arch}/ copies.
>
> Cc: Adrian Hunter
> Cc: Alexander Sverdlin
> Cc: David Ahern
> Cc: Jiri Olsa
> Cc: Namhyung Kim
> Cc: Wang Nan
> Link: https://lkml.kernel.org/n/tip-y1sqyydvfzo0bjjoj4zsl...@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo
> ---
> tools/include/linux/bitops.h | 7 ++-
> tools/include/linux/bits.h | 26 ++
> tools/perf/check-headers.sh | 1 +
> 3 files changed, 29 insertions(+), 5 deletions(-)
> create mode 100644 tools/include/linux/bits.h
>
> diff --git a/tools/include/linux/bitops.h b/tools/include/linux/bitops.h
> index acc704bd3998..0b0ef3abc966 100644
> --- a/tools/include/linux/bitops.h
> +++ b/tools/include/linux/bitops.h
> @@ -3,8 +3,6 @@
> #define _TOOLS_LINUX_BITOPS_H_
>
> #include
> -#include
> -
> #ifndef __WORDSIZE
> #define __WORDSIZE (__SIZEOF_LONG__ * 8)
> #endif
> @@ -12,10 +10,9 @@
> #ifndef BITS_PER_LONG
> # define BITS_PER_LONG __WORDSIZE
> #endif
> +#include
> +#include
>
> -#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
> -#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
> -#define BITS_PER_BYTE8
> #define BITS_TO_LONGS(nr)DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
> #define BITS_TO_U64(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE *
> sizeof(u64))
> #define BITS_TO_U32(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE *
> sizeof(u32))
> diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> new file mode 100644
> index ..2b7b532c1d51
> --- /dev/null
> +++ b/tools/include/linux/bits.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_BITS_H
> +#define __LINUX_BITS_H
> +#include
> +
> +#define BIT(nr) (1UL << (nr))
> +#define BIT_ULL(nr) (1ULL << (nr))
> +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
> +#define BIT_ULL_MASK(nr) (1ULL << ((nr) % BITS_PER_LONG_LONG))
> +#define BIT_ULL_WORD(nr) ((nr) / BITS_PER_LONG_LONG)
> +#define BITS_PER_BYTE8
> +
> +/*
> + * Create a contiguous bitmask starting at bit position @l and ending at
> + * position @h. For example
> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x00e0.
> + */
> +#define GENMASK(h, l) \
> + (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h
> +
> +#define GENMASK_ULL(h, l) \
> + (((~0ULL) - (1ULL << (l)) + 1) & \
> + (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h
> +
> +#endif /* __LINUX_BITS_H */
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index 466540ee8ea7..c72cc73a6b09 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -14,6 +14,7 @@ include/uapi/linux/sched.h
> include/uapi/linux/stat.h
> include/uapi/linux/vhost.h
> include/uapi/sound/asound.h
> +include/linux/bits.h
> include/linux/hash.h
> include/uapi/linux/hw_breakpoint.h
> arch/x86/include/asm/disabled-features.h
>
--
Best regards,
Alexander Sverdlin.
Re: [PATCH 01/12] tools include: Adopt linux/bits.h
Hello Arnaldo,
On 09/10/2018 02:54, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo
>
> So that we reduce the difference of tools/include/linux/bitops.h to the
> original kernel file, include/linux/bitops.h, trying to remove the need
> to define BITS_PER_LONG, to avoid clashes with asm/bitsperlong.h.
thanks for looking into this, but I don't quite get your plan here,
you neither remove redefinition of BITS_PER_LONG in this patch, nor
in any following patch in the series.
Have you forgot to include another patch or shall I try to come up with
a patch removing BITS_PER_LONG from bitops.h?
> And the things removed from tools/include/linux/bitops.h are really in
> linux/bits.h, so that we can have a copy and then
> tools/perf/check_headers.sh will tell us when new stuff gets added to
> linux/bits.h so that we can check if it is useful and if any adjustment
> needs to be done to the tools/{include,arch}/ copies.
>
> Cc: Adrian Hunter
> Cc: Alexander Sverdlin
> Cc: David Ahern
> Cc: Jiri Olsa
> Cc: Namhyung Kim
> Cc: Wang Nan
> Link: https://lkml.kernel.org/n/tip-y1sqyydvfzo0bjjoj4zsl...@git.kernel.org
> Signed-off-by: Arnaldo Carvalho de Melo
> ---
> tools/include/linux/bitops.h | 7 ++-
> tools/include/linux/bits.h | 26 ++
> tools/perf/check-headers.sh | 1 +
> 3 files changed, 29 insertions(+), 5 deletions(-)
> create mode 100644 tools/include/linux/bits.h
>
> diff --git a/tools/include/linux/bitops.h b/tools/include/linux/bitops.h
> index acc704bd3998..0b0ef3abc966 100644
> --- a/tools/include/linux/bitops.h
> +++ b/tools/include/linux/bitops.h
> @@ -3,8 +3,6 @@
> #define _TOOLS_LINUX_BITOPS_H_
>
> #include
> -#include
> -
> #ifndef __WORDSIZE
> #define __WORDSIZE (__SIZEOF_LONG__ * 8)
> #endif
> @@ -12,10 +10,9 @@
> #ifndef BITS_PER_LONG
> # define BITS_PER_LONG __WORDSIZE
> #endif
> +#include
> +#include
>
> -#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
> -#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
> -#define BITS_PER_BYTE8
> #define BITS_TO_LONGS(nr)DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
> #define BITS_TO_U64(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE *
> sizeof(u64))
> #define BITS_TO_U32(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE *
> sizeof(u32))
> diff --git a/tools/include/linux/bits.h b/tools/include/linux/bits.h
> new file mode 100644
> index ..2b7b532c1d51
> --- /dev/null
> +++ b/tools/include/linux/bits.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_BITS_H
> +#define __LINUX_BITS_H
> +#include
> +
> +#define BIT(nr) (1UL << (nr))
> +#define BIT_ULL(nr) (1ULL << (nr))
> +#define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG))
> +#define BIT_WORD(nr) ((nr) / BITS_PER_LONG)
> +#define BIT_ULL_MASK(nr) (1ULL << ((nr) % BITS_PER_LONG_LONG))
> +#define BIT_ULL_WORD(nr) ((nr) / BITS_PER_LONG_LONG)
> +#define BITS_PER_BYTE8
> +
> +/*
> + * Create a contiguous bitmask starting at bit position @l and ending at
> + * position @h. For example
> + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x00e0.
> + */
> +#define GENMASK(h, l) \
> + (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h
> +
> +#define GENMASK_ULL(h, l) \
> + (((~0ULL) - (1ULL << (l)) + 1) & \
> + (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h
> +
> +#endif /* __LINUX_BITS_H */
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index 466540ee8ea7..c72cc73a6b09 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -14,6 +14,7 @@ include/uapi/linux/sched.h
> include/uapi/linux/stat.h
> include/uapi/linux/vhost.h
> include/uapi/sound/asound.h
> +include/linux/bits.h
> include/linux/hash.h
> include/uapi/linux/hw_breakpoint.h
> arch/x86/include/asm/disabled-features.h
>
--
Best regards,
Alexander Sverdlin.
Re: [PATCH 02/12] perf auxtrace: Include missing asm/bitsperlong.h to get BITS_PER_LONG
Hi! On 09/10/2018 02:54, Arnaldo Carvalho de Melo wrote: > From: Arnaldo Carvalho de Melo > > The auxtrace.h header references BITS_PER_LONG without including the > header where it is defined, getting it by luck from some other header, > fix it. > > Cc: Adrian Hunter > Cc: Alexander Sverdlin > Cc: David Ahern > Cc: Jiri Olsa > Cc: Namhyung Kim > Cc: Wang Nan > Link: https://lkml.kernel.org/n/tip-v04ydmbh7tvpcctf3zld9...@git.kernel.org Reviewed-by: Alexander Sverdlin > Signed-off-by: Arnaldo Carvalho de Melo > --- > tools/perf/util/auxtrace.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h > index 0a6ce9c4fc11..d88f6e9eb461 100644 > --- a/tools/perf/util/auxtrace.h > +++ b/tools/perf/util/auxtrace.h > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > > #include "../perf.h" > #include "event.h" > -- Best regards, Alexander Sverdlin.
Re: [PATCH 02/12] perf auxtrace: Include missing asm/bitsperlong.h to get BITS_PER_LONG
Hi! On 09/10/2018 02:54, Arnaldo Carvalho de Melo wrote: > From: Arnaldo Carvalho de Melo > > The auxtrace.h header references BITS_PER_LONG without including the > header where it is defined, getting it by luck from some other header, > fix it. > > Cc: Adrian Hunter > Cc: Alexander Sverdlin > Cc: David Ahern > Cc: Jiri Olsa > Cc: Namhyung Kim > Cc: Wang Nan > Link: https://lkml.kernel.org/n/tip-v04ydmbh7tvpcctf3zld9...@git.kernel.org Reviewed-by: Alexander Sverdlin > Signed-off-by: Arnaldo Carvalho de Melo > --- > tools/perf/util/auxtrace.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h > index 0a6ce9c4fc11..d88f6e9eb461 100644 > --- a/tools/perf/util/auxtrace.h > +++ b/tools/perf/util/auxtrace.h > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > > #include "../perf.h" > #include "event.h" > -- Best regards, Alexander Sverdlin.
Re: [PATCH] tools: Remove conflicting BITS_PER_LONG define
Hello Arnaldo,
>>>> Em Wed, Sep 12, 2018 at 07:02:32PM +0200, Alexander Sverdlin escreveu:
>>>>> CC .../tools/objtool/builtin-check.o
>>>>> ...
>>>>> In file included from
>>>>> .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0,
>>>>> from .../tools/include/asm-generic/bitops/__ffs.h:6,
>>>>> from .../tools/include/asm-generic/bitops.h:16,
>>>>> from .../tools/include/linux/bitops.h:35,
>>>>> from .../tools/include/linux/hashtable.h:13,
>>>>> from elf.h:24,
>>>>> from check.h:22,
>>>>> from builtin-check.c:30:
>>>>> .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG"
>>>>> redefined [-Werror]
>>>>> #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__)
[...]
finally I more or less know what happens here. In the actual Linux we have two
files defining the
same define:
>> # 1 ".../tools/include/linux/bitops.h" 1
#ifndef BITS_PER_LONG
# define BITS_PER_LONG __WORDSIZE
#endif
>> # 6 ".../tools/include/asm-generic/bitsperlong.h" 2
#ifdef __SIZEOF_LONG__
#define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__)
#else
#define BITS_PER_LONG __WORDSIZE
#endif
So the two defines only work together if bitsperlong.h is included first.
In objtool both files are included and for most people bitsperlong.h is indeed
included first.
> I'll try and get one for building a x86_64 tools/perf,
> tools/lib/{api,bpf,traceevent} to see if I manage to reproduce the
> problem you're reporting.
One way to reproduce the reverted include order is to take a HOST compiler
built against old Linux headers, namely 2.6.30 and older.
This is because of the changes in asm/types.h.
It would be possible to put a guard into bitsperlong.h (#ifndef BITS_PER_LONG),
but it just doesn't look correct to me that we have two files defining the same
thing once quite simple, in the second case even more tricky.
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] tools: Remove conflicting BITS_PER_LONG define
Hello Arnaldo,
>>>> Em Wed, Sep 12, 2018 at 07:02:32PM +0200, Alexander Sverdlin escreveu:
>>>>> CC .../tools/objtool/builtin-check.o
>>>>> ...
>>>>> In file included from
>>>>> .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0,
>>>>> from .../tools/include/asm-generic/bitops/__ffs.h:6,
>>>>> from .../tools/include/asm-generic/bitops.h:16,
>>>>> from .../tools/include/linux/bitops.h:35,
>>>>> from .../tools/include/linux/hashtable.h:13,
>>>>> from elf.h:24,
>>>>> from check.h:22,
>>>>> from builtin-check.c:30:
>>>>> .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG"
>>>>> redefined [-Werror]
>>>>> #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__)
[...]
finally I more or less know what happens here. In the actual Linux we have two
files defining the
same define:
>> # 1 ".../tools/include/linux/bitops.h" 1
#ifndef BITS_PER_LONG
# define BITS_PER_LONG __WORDSIZE
#endif
>> # 6 ".../tools/include/asm-generic/bitsperlong.h" 2
#ifdef __SIZEOF_LONG__
#define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__)
#else
#define BITS_PER_LONG __WORDSIZE
#endif
So the two defines only work together if bitsperlong.h is included first.
In objtool both files are included and for most people bitsperlong.h is indeed
included first.
> I'll try and get one for building a x86_64 tools/perf,
> tools/lib/{api,bpf,traceevent} to see if I manage to reproduce the
> problem you're reporting.
One way to reproduce the reverted include order is to take a HOST compiler
built against old Linux headers, namely 2.6.30 and older.
This is because of the changes in asm/types.h.
It would be possible to put a guard into bitsperlong.h (#ifndef BITS_PER_LONG),
but it just doesn't look correct to me that we have two files defining the same
thing once quite simple, in the second case even more tricky.
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] tools: Remove conflicting BITS_PER_LONG define
Hi!
On 19/09/2018 15:03, Arnaldo Carvalho de Melo wrote:
> That is indeed a cross build environment I'm not regularly testing, I'm
> trying these cross builds:
Probably my assumption was wrong about cross compiler.
HOST tool fails (objtool) and it's being build by separate i686->i686 compiler.
>9 android-ndk:r12b-arm : Ok arm-linux-androideabi-gcc (GCC)
> 4.9.x 20150123 (prerelease)
> 10 android-ndk:r15c-arm : Ok arm-linux-androideabi-gcc (GCC)
> 4.9.x 20150123 (prerelease)
> 19 debian:experimental-x-arm64 : Ok aarch64-linux-gnu-gcc (Debian
> 8.2.0-4) 8.2.0
> 20 debian:experimental-x-mips: Ok mips-linux-gnu-gcc (Debian
> 8.1.0-12) 8.1.0
> 21 debian:experimental-x-mips64 : Ok mips64-linux-gnuabi64-gcc (Debian
> 8.1.0-12) 8.1.0
> 22 debian:experimental-x-mipsel : Ok mipsel-linux-gnu-gcc (Debian
> 8.1.0-12) 8.1.0
> 28 fedora:24-x-ARC-uClibc: Ok arc-linux-gcc (ARCompact ISA Linux
> uClibc toolchain 2017.09-rc2) 7.1.1 20170710
> 46 ubuntu:14.04.4-x-linaro-arm64 : Ok aarch64-linux-gnu-gcc (Linaro GCC
> 5.5-2017.10) 5.5.0
> 48 ubuntu:16.04-x-arm: Ok arm-linux-gnueabihf-gcc
> (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 49 ubuntu:16.04-x-arm64 : Ok aarch64-linux-gnu-gcc
> (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 50 ubuntu:16.04-x-powerpc: Ok powerpc-linux-gnu-gcc (Ubuntu
> 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 51 ubuntu:16.04-x-powerpc64 : Ok powerpc64-linux-gnu-gcc (Ubuntu/IBM
> 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 52 ubuntu:16.04-x-powerpc64el: Ok powerpc64le-linux-gnu-gcc
> (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 53 ubuntu:16.04-x-s390 : Ok s390x-linux-gnu-gcc (Ubuntu
> 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 57 ubuntu:18.04-x-arm: Ok arm-linux-gnueabihf-gcc
> (Ubuntu/Linaro 7.3.0-16ubuntu3) 7.3.0
> 58 ubuntu:18.04-x-arm64 : Ok aarch64-linux-gnu-gcc
> (Ubuntu/Linaro 7.3.0-16ubuntu3) 7.3.0
> 59 ubuntu:18.04-x-m68k : Ok m68k-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 60 ubuntu:18.04-x-powerpc: Ok powerpc-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 61 ubuntu:18.04-x-powerpc64 : Ok powerpc64-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 62 ubuntu:18.04-x-powerpc64el: Ok powerpc64le-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 63 ubuntu:18.04-x-riscv64: Ok riscv64-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 64 ubuntu:18.04-x-s390 : Ok s390x-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 65 ubuntu:18.04-x-sh4: Ok sh4-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 66 ubuntu:18.04-x-sparc64: Ok sparc64-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
I think the big list is mostly irrelevant. I was wondering why only x86 fails
from my list of
different targets (ARM, ARM64, MIPS64, x86, x86_64, PPC), but it turns out that
objtool is only
built for HAVE_STACK_VALIDATION targets (means, x86 only?).
I've tried to build it for MIPS64 with make tools/objtool and it fails in the
same way.
One special thing about my compiler is really old glibc it's being built against
(hello RHEL :) maybe this causes reverse order of includes.
Nevertheless two conflicting defines of BITS_PER_LONG will only work with one
particular
order of includes but not opposite.
> I'll try and get one for building a x86_64 tools/perf,
> tools/lib/{api,bpf,traceevent} to see if I manage to reproduce the
> problem you're reporting.
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] tools: Remove conflicting BITS_PER_LONG define
Hi!
On 19/09/2018 15:03, Arnaldo Carvalho de Melo wrote:
> That is indeed a cross build environment I'm not regularly testing, I'm
> trying these cross builds:
Probably my assumption was wrong about cross compiler.
HOST tool fails (objtool) and it's being build by separate i686->i686 compiler.
>9 android-ndk:r12b-arm : Ok arm-linux-androideabi-gcc (GCC)
> 4.9.x 20150123 (prerelease)
> 10 android-ndk:r15c-arm : Ok arm-linux-androideabi-gcc (GCC)
> 4.9.x 20150123 (prerelease)
> 19 debian:experimental-x-arm64 : Ok aarch64-linux-gnu-gcc (Debian
> 8.2.0-4) 8.2.0
> 20 debian:experimental-x-mips: Ok mips-linux-gnu-gcc (Debian
> 8.1.0-12) 8.1.0
> 21 debian:experimental-x-mips64 : Ok mips64-linux-gnuabi64-gcc (Debian
> 8.1.0-12) 8.1.0
> 22 debian:experimental-x-mipsel : Ok mipsel-linux-gnu-gcc (Debian
> 8.1.0-12) 8.1.0
> 28 fedora:24-x-ARC-uClibc: Ok arc-linux-gcc (ARCompact ISA Linux
> uClibc toolchain 2017.09-rc2) 7.1.1 20170710
> 46 ubuntu:14.04.4-x-linaro-arm64 : Ok aarch64-linux-gnu-gcc (Linaro GCC
> 5.5-2017.10) 5.5.0
> 48 ubuntu:16.04-x-arm: Ok arm-linux-gnueabihf-gcc
> (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 49 ubuntu:16.04-x-arm64 : Ok aarch64-linux-gnu-gcc
> (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 50 ubuntu:16.04-x-powerpc: Ok powerpc-linux-gnu-gcc (Ubuntu
> 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 51 ubuntu:16.04-x-powerpc64 : Ok powerpc64-linux-gnu-gcc (Ubuntu/IBM
> 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 52 ubuntu:16.04-x-powerpc64el: Ok powerpc64le-linux-gnu-gcc
> (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 53 ubuntu:16.04-x-s390 : Ok s390x-linux-gnu-gcc (Ubuntu
> 5.4.0-6ubuntu1~16.04.9) 5.4.0 20160609
> 57 ubuntu:18.04-x-arm: Ok arm-linux-gnueabihf-gcc
> (Ubuntu/Linaro 7.3.0-16ubuntu3) 7.3.0
> 58 ubuntu:18.04-x-arm64 : Ok aarch64-linux-gnu-gcc
> (Ubuntu/Linaro 7.3.0-16ubuntu3) 7.3.0
> 59 ubuntu:18.04-x-m68k : Ok m68k-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 60 ubuntu:18.04-x-powerpc: Ok powerpc-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 61 ubuntu:18.04-x-powerpc64 : Ok powerpc64-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 62 ubuntu:18.04-x-powerpc64el: Ok powerpc64le-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 63 ubuntu:18.04-x-riscv64: Ok riscv64-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 64 ubuntu:18.04-x-s390 : Ok s390x-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 65 ubuntu:18.04-x-sh4: Ok sh4-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
> 66 ubuntu:18.04-x-sparc64: Ok sparc64-linux-gnu-gcc (Ubuntu
> 7.3.0-16ubuntu3) 7.3.0
I think the big list is mostly irrelevant. I was wondering why only x86 fails
from my list of
different targets (ARM, ARM64, MIPS64, x86, x86_64, PPC), but it turns out that
objtool is only
built for HAVE_STACK_VALIDATION targets (means, x86 only?).
I've tried to build it for MIPS64 with make tools/objtool and it fails in the
same way.
One special thing about my compiler is really old glibc it's being built against
(hello RHEL :) maybe this causes reverse order of includes.
Nevertheless two conflicting defines of BITS_PER_LONG will only work with one
particular
order of includes but not opposite.
> I'll try and get one for building a x86_64 tools/perf,
> tools/lib/{api,bpf,traceevent} to see if I manage to reproduce the
> problem you're reporting.
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] tools: Remove conflicting BITS_PER_LONG define
On 12/09/2018 21:21, Arnaldo Carvalho de Melo wrote: > Em Wed, Sep 12, 2018 at 04:01:07PM -0300, Arnaldo Carvalho de Melo escreveu: >> Em Wed, Sep 12, 2018 at 07:02:32PM +0200, Alexander Sverdlin escreveu: >>> CC .../tools/objtool/builtin-check.o >>> ... >>> In file included from >>> .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0, >>> from .../tools/include/asm-generic/bitops/__ffs.h:6, >>> from .../tools/include/asm-generic/bitops.h:16, >>> from .../tools/include/linux/bitops.h:35, >>> from .../tools/include/linux/hashtable.h:13, >>> from elf.h:24, >>> from check.h:22, >>> from builtin-check.c:30: >>> .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG" >>> redefined [-Werror] >>> #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) >>> >>> Include instead as other headers do. >> >> Please try test building all tools in tools/ >> >> This broke make -C tools/perf/ > > Also, where is the build of objtool failing? I've prepared some examples for you with "-fdirectives-only -save-temps": This is what happens ultimately: In file included from .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0, from .../tools/include/asm-generic/bitops/__ffs.h:6, from .../tools/include/asm-generic/bitops.h:16, from .../tools/include/linux/bitops.h:35, from .../tools/include/linux/hashtable.h:13, from elf.h:24, from check.h:22, from builtin-check.c:30: .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG" redefined [-Werror] #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) In file included from .../tools/include/linux/hashtable.h:13:0, from elf.h:24, from check.h:22, from builtin-check.c:30: .../tools/include/linux/bitops.h:13:0: note: this is the location of the previous definition # define BITS_PER_LONG __WORDSIZE And this are relevant parts from builtin-check.i file: # 1 "builtin-check.c" # 1 ".../tools/objtool//" # 1 "" ... #define __SIZEOF_LONG__ 4 ... # 1 ".../sys-root/usr/include/bits/wordsize.h" 1 3 4 ... #define __WORDSIZE 32 ... # 1 ".../sys-root/usr/include/bits/wordsize.h" 1 3 4 ... #define __WORDSIZE 32 ... # 1 ".../sys-root/usr/include/bits/wordsize.h" 1 3 4 ... #define __WORDSIZE 32 ... # 1 ".../sys-root/usr/include/bits/wordsize.h" 1 3 4 ... #define __WORDSIZE 32 ... # 1 ".../tools/include/linux/bitops.h" 1 ... #define BITS_PER_LONG __WORDSIZE ... # 6 ".../tools/include/asm-generic/bitsperlong.h" 2 ... #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) It could be that your compiler doesn't define __SIZEOF_LONG__, while my does. It could be that it's caused by the fact that I'm using a "cross compiler" (i686 compiler to build x86_64 kernel), maybe I'm just using a newer compiler than you. -- Best regards, Alexander Sverdlin.
Re: [PATCH] tools: Remove conflicting BITS_PER_LONG define
On 12/09/2018 21:21, Arnaldo Carvalho de Melo wrote: > Em Wed, Sep 12, 2018 at 04:01:07PM -0300, Arnaldo Carvalho de Melo escreveu: >> Em Wed, Sep 12, 2018 at 07:02:32PM +0200, Alexander Sverdlin escreveu: >>> CC .../tools/objtool/builtin-check.o >>> ... >>> In file included from >>> .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0, >>> from .../tools/include/asm-generic/bitops/__ffs.h:6, >>> from .../tools/include/asm-generic/bitops.h:16, >>> from .../tools/include/linux/bitops.h:35, >>> from .../tools/include/linux/hashtable.h:13, >>> from elf.h:24, >>> from check.h:22, >>> from builtin-check.c:30: >>> .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG" >>> redefined [-Werror] >>> #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) >>> >>> Include instead as other headers do. >> >> Please try test building all tools in tools/ >> >> This broke make -C tools/perf/ > > Also, where is the build of objtool failing? I've prepared some examples for you with "-fdirectives-only -save-temps": This is what happens ultimately: In file included from .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0, from .../tools/include/asm-generic/bitops/__ffs.h:6, from .../tools/include/asm-generic/bitops.h:16, from .../tools/include/linux/bitops.h:35, from .../tools/include/linux/hashtable.h:13, from elf.h:24, from check.h:22, from builtin-check.c:30: .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG" redefined [-Werror] #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) In file included from .../tools/include/linux/hashtable.h:13:0, from elf.h:24, from check.h:22, from builtin-check.c:30: .../tools/include/linux/bitops.h:13:0: note: this is the location of the previous definition # define BITS_PER_LONG __WORDSIZE And this are relevant parts from builtin-check.i file: # 1 "builtin-check.c" # 1 ".../tools/objtool//" # 1 "" ... #define __SIZEOF_LONG__ 4 ... # 1 ".../sys-root/usr/include/bits/wordsize.h" 1 3 4 ... #define __WORDSIZE 32 ... # 1 ".../sys-root/usr/include/bits/wordsize.h" 1 3 4 ... #define __WORDSIZE 32 ... # 1 ".../sys-root/usr/include/bits/wordsize.h" 1 3 4 ... #define __WORDSIZE 32 ... # 1 ".../sys-root/usr/include/bits/wordsize.h" 1 3 4 ... #define __WORDSIZE 32 ... # 1 ".../tools/include/linux/bitops.h" 1 ... #define BITS_PER_LONG __WORDSIZE ... # 6 ".../tools/include/asm-generic/bitsperlong.h" 2 ... #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) It could be that your compiler doesn't define __SIZEOF_LONG__, while my does. It could be that it's caused by the fact that I'm using a "cross compiler" (i686 compiler to build x86_64 kernel), maybe I'm just using a newer compiler than you. -- Best regards, Alexander Sverdlin.
Re: [PATCH] tools: Remove conflicting BITS_PER_LONG define
On 12/09/2018 21:01, Arnaldo Carvalho de Melo wrote: >> CC .../tools/objtool/builtin-check.o >> ... >> In file included from .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0, >> from .../tools/include/asm-generic/bitops/__ffs.h:6, >> from .../tools/include/asm-generic/bitops.h:16, >> from .../tools/include/linux/bitops.h:35, >> from .../tools/include/linux/hashtable.h:13, >> from elf.h:24, >> from check.h:22, >> from builtin-check.c:30: >> .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG" >> redefined [-Werror] >> #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) >> >> Include instead as other headers do. > Please try test building all tools in tools/ > > This broke make -C tools/perf/ I agree, this brakes perf, so we need to work further on that... -- Best regards, Alexander Sverdlin.
Re: [PATCH] tools: Remove conflicting BITS_PER_LONG define
On 12/09/2018 21:01, Arnaldo Carvalho de Melo wrote: >> CC .../tools/objtool/builtin-check.o >> ... >> In file included from .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0, >> from .../tools/include/asm-generic/bitops/__ffs.h:6, >> from .../tools/include/asm-generic/bitops.h:16, >> from .../tools/include/linux/bitops.h:35, >> from .../tools/include/linux/hashtable.h:13, >> from elf.h:24, >> from check.h:22, >> from builtin-check.c:30: >> .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG" >> redefined [-Werror] >> #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) >> >> Include instead as other headers do. > Please try test building all tools in tools/ > > This broke make -C tools/perf/ I agree, this brakes perf, so we need to work further on that... -- Best regards, Alexander Sverdlin.
[PATCH] tools: Remove conflicting BITS_PER_LONG define
CC .../tools/objtool/builtin-check.o ... In file included from .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0, from .../tools/include/asm-generic/bitops/__ffs.h:6, from .../tools/include/asm-generic/bitops.h:16, from .../tools/include/linux/bitops.h:35, from .../tools/include/linux/hashtable.h:13, from elf.h:24, from check.h:22, from builtin-check.c:30: .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG" redefined [-Werror] #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) Include instead as other headers do. Signed-off-by: Alexander Sverdlin --- Seen during x86_64 build. Seems that most of the compilers do not define __SIZEOF_LONG__, but my does. tools/include/linux/bitops.h | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tools/include/linux/bitops.h b/tools/include/linux/bitops.h index acc704b..4ae5232 100644 --- a/tools/include/linux/bitops.h +++ b/tools/include/linux/bitops.h @@ -3,16 +3,13 @@ #define _TOOLS_LINUX_BITOPS_H_ #include +#include #include #ifndef __WORDSIZE #define __WORDSIZE (__SIZEOF_LONG__ * 8) #endif -#ifndef BITS_PER_LONG -# define BITS_PER_LONG __WORDSIZE -#endif - #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) #define BITS_PER_BYTE 8 -- 2.4.6
[PATCH] tools: Remove conflicting BITS_PER_LONG define
CC .../tools/objtool/builtin-check.o ... In file included from .../tools/arch/x86/include/uapi/asm/bitsperlong.h:11:0, from .../tools/include/asm-generic/bitops/__ffs.h:6, from .../tools/include/asm-generic/bitops.h:16, from .../tools/include/linux/bitops.h:35, from .../tools/include/linux/hashtable.h:13, from elf.h:24, from check.h:22, from builtin-check.c:30: .../tools/include/asm-generic/bitsperlong.h:8:0: error: "BITS_PER_LONG" redefined [-Werror] #define BITS_PER_LONG (__CHAR_BIT__ * __SIZEOF_LONG__) Include instead as other headers do. Signed-off-by: Alexander Sverdlin --- Seen during x86_64 build. Seems that most of the compilers do not define __SIZEOF_LONG__, but my does. tools/include/linux/bitops.h | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tools/include/linux/bitops.h b/tools/include/linux/bitops.h index acc704b..4ae5232 100644 --- a/tools/include/linux/bitops.h +++ b/tools/include/linux/bitops.h @@ -3,16 +3,13 @@ #define _TOOLS_LINUX_BITOPS_H_ #include +#include #include #ifndef __WORDSIZE #define __WORDSIZE (__SIZEOF_LONG__ * 8) #endif -#ifndef BITS_PER_LONG -# define BITS_PER_LONG __WORDSIZE -#endif - #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_LONG)) #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) #define BITS_PER_BYTE 8 -- 2.4.6
Re: [PATCH v2] serial: 8250_of: Fix for lack of interrupt support
Hello John!
On 30/08/2018 11:08, John Garry wrote:
> In commit c58caaab3bf8 ("serial: 8250: of: Defer probe on missing IRQ"), a
> check was added for the UART driver being probed prior to the parent IRQ
> controller.
>
> Unfortunately this breaks certain boards which have no interrupt support,
> like Huawei D03.
>
> Indeed, the 8250 DT bindings state that interrupts should be supported -
> not must.
>
> To fix, switch from irq_of_parse_and_map() to of_irq_get(), which
> does relay whether the IRQ host controller domain is not ready, i.e.
> defer probe, instead of assuming it.
>
> Fixes: c58caaab3bf8 ("serial: 8250: of: Defer probe on missing IRQ")
> Signed-off-by: John Garry
This indeed looks like a proper way to handle both cases:
Reviewed-by: Alexander Sverdlin
Tested-by: Alexander Sverdlin
> ---
>
> Change in v2:
> - fix check on irq value
>
> Note: I think that it would better if we could try to get the interrupt
> before clk+pm enabling, so we don't need to disable later when
> deferring, but this is not a fix.
>
> diff --git a/drivers/tty/serial/8250/8250_of.c
> b/drivers/tty/serial/8250/8250_of.c
> index af8beef..877fd7f 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -58,7 +58,7 @@ static int of_platform_serial_setup(struct platform_device
> *ofdev,
> struct resource resource;
> struct device_node *np = ofdev->dev.of_node;
> u32 clk, spd, prop;
> - int ret;
> + int ret, irq;
>
> memset(port, 0, sizeof *port);
>
> @@ -143,21 +143,27 @@ static int of_platform_serial_setup(struct
> platform_device *ofdev,
> if (ret >= 0)
> port->line = ret;
>
> - port->irq = irq_of_parse_and_map(np, 0);
> - if (!port->irq) {
> - ret = -EPROBE_DEFER;
> - goto err_unprepare;
> + irq = of_irq_get(np, 0);
> + if (irq < 0) {
> + if (irq == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto err_unprepare;
> + }
> + /* IRQ support not mandatory */
> + irq = 0;
> }
>
> + port->irq = irq;
> +
> info->rst = devm_reset_control_get_optional_shared(>dev, NULL);
> if (IS_ERR(info->rst)) {
> ret = PTR_ERR(info->rst);
> - goto err_dispose;
> + goto err_unprepare;
> }
>
> ret = reset_control_deassert(info->rst);
> if (ret)
> - goto err_dispose;
> + goto err_unprepare;
>
> port->type = type;
> port->uartclk = clk;
> @@ -184,8 +190,6 @@ static int of_platform_serial_setup(struct
> platform_device *ofdev,
> port->handle_irq = fsl8250_handle_irq;
>
> return 0;
> -err_dispose:
> - irq_dispose_mapping(port->irq);
> err_unprepare:
> clk_disable_unprepare(info->clk);
> err_pmruntime:
>
--
Best regards,
Alexander Sverdlin.
Re: [PATCH v2] serial: 8250_of: Fix for lack of interrupt support
Hello John!
On 30/08/2018 11:08, John Garry wrote:
> In commit c58caaab3bf8 ("serial: 8250: of: Defer probe on missing IRQ"), a
> check was added for the UART driver being probed prior to the parent IRQ
> controller.
>
> Unfortunately this breaks certain boards which have no interrupt support,
> like Huawei D03.
>
> Indeed, the 8250 DT bindings state that interrupts should be supported -
> not must.
>
> To fix, switch from irq_of_parse_and_map() to of_irq_get(), which
> does relay whether the IRQ host controller domain is not ready, i.e.
> defer probe, instead of assuming it.
>
> Fixes: c58caaab3bf8 ("serial: 8250: of: Defer probe on missing IRQ")
> Signed-off-by: John Garry
This indeed looks like a proper way to handle both cases:
Reviewed-by: Alexander Sverdlin
Tested-by: Alexander Sverdlin
> ---
>
> Change in v2:
> - fix check on irq value
>
> Note: I think that it would better if we could try to get the interrupt
> before clk+pm enabling, so we don't need to disable later when
> deferring, but this is not a fix.
>
> diff --git a/drivers/tty/serial/8250/8250_of.c
> b/drivers/tty/serial/8250/8250_of.c
> index af8beef..877fd7f 100644
> --- a/drivers/tty/serial/8250/8250_of.c
> +++ b/drivers/tty/serial/8250/8250_of.c
> @@ -58,7 +58,7 @@ static int of_platform_serial_setup(struct platform_device
> *ofdev,
> struct resource resource;
> struct device_node *np = ofdev->dev.of_node;
> u32 clk, spd, prop;
> - int ret;
> + int ret, irq;
>
> memset(port, 0, sizeof *port);
>
> @@ -143,21 +143,27 @@ static int of_platform_serial_setup(struct
> platform_device *ofdev,
> if (ret >= 0)
> port->line = ret;
>
> - port->irq = irq_of_parse_and_map(np, 0);
> - if (!port->irq) {
> - ret = -EPROBE_DEFER;
> - goto err_unprepare;
> + irq = of_irq_get(np, 0);
> + if (irq < 0) {
> + if (irq == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto err_unprepare;
> + }
> + /* IRQ support not mandatory */
> + irq = 0;
> }
>
> + port->irq = irq;
> +
> info->rst = devm_reset_control_get_optional_shared(>dev, NULL);
> if (IS_ERR(info->rst)) {
> ret = PTR_ERR(info->rst);
> - goto err_dispose;
> + goto err_unprepare;
> }
>
> ret = reset_control_deassert(info->rst);
> if (ret)
> - goto err_dispose;
> + goto err_unprepare;
>
> port->type = type;
> port->uartclk = clk;
> @@ -184,8 +190,6 @@ static int of_platform_serial_setup(struct
> platform_device *ofdev,
> port->handle_irq = fsl8250_handle_irq;
>
> return 0;
> -err_dispose:
> - irq_dispose_mapping(port->irq);
> err_unprepare:
> clk_disable_unprepare(info->clk);
> err_pmruntime:
>
--
Best regards,
Alexander Sverdlin.
Re: [PATCH 4/6] mips: factor out RapidIO Kconfig entry
Hi! On 31/07/18 00:50, Alexei Colin wrote: > The menu entry is now defined in the rapidio subtree. > > Platforms with a PCI bus will be offered the RapidIO menu since they may > be want support for a RapidIO PCI device. Platforms without a PCI bus > that might include a RapidIO IP block will need to "select HAS_RAPIDIO" > in the platform-/machine-specific "config ARCH_*" Kconfig entry. > > Cc: Andrew Morton > Cc: Alexander Sverdlin > Cc: John Paul Walters > Cc: linux-m...@linux-mips.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Alexei Colin With patch [1/6] together this looks fine to me. Reviewed-by: Alexander Sverdlin > --- > arch/mips/Kconfig | 11 --- > 1 file changed, 11 deletions(-) > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig > index 10256056647c..92b9262ee731 100644 > --- a/arch/mips/Kconfig > +++ b/arch/mips/Kconfig > @@ -3106,17 +3106,6 @@ config ZONE_DMA32 > > source "drivers/pcmcia/Kconfig" > > -config HAS_RAPIDIO > - bool > - default n > - > -config RAPIDIO > - tristate "RapidIO support" > - depends on HAS_RAPIDIO || PCI > - help > - If you say Y here, the kernel will include drivers and > - infrastructure code to support RapidIO interconnect devices. > - > source "drivers/rapidio/Kconfig" > > endmenu -- Best regards, Alexander Sverdlin.
Re: [PATCH 4/6] mips: factor out RapidIO Kconfig entry
Hi! On 31/07/18 00:50, Alexei Colin wrote: > The menu entry is now defined in the rapidio subtree. > > Platforms with a PCI bus will be offered the RapidIO menu since they may > be want support for a RapidIO PCI device. Platforms without a PCI bus > that might include a RapidIO IP block will need to "select HAS_RAPIDIO" > in the platform-/machine-specific "config ARCH_*" Kconfig entry. > > Cc: Andrew Morton > Cc: Alexander Sverdlin > Cc: John Paul Walters > Cc: linux-m...@linux-mips.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Alexei Colin With patch [1/6] together this looks fine to me. Reviewed-by: Alexander Sverdlin > --- > arch/mips/Kconfig | 11 --- > 1 file changed, 11 deletions(-) > > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig > index 10256056647c..92b9262ee731 100644 > --- a/arch/mips/Kconfig > +++ b/arch/mips/Kconfig > @@ -3106,17 +3106,6 @@ config ZONE_DMA32 > > source "drivers/pcmcia/Kconfig" > > -config HAS_RAPIDIO > - bool > - default n > - > -config RAPIDIO > - tristate "RapidIO support" > - depends on HAS_RAPIDIO || PCI > - help > - If you say Y here, the kernel will include drivers and > - infrastructure code to support RapidIO interconnect devices. > - > source "drivers/rapidio/Kconfig" > > endmenu -- Best regards, Alexander Sverdlin.
Re: [PATCH] rapidio: fix rio_dma_transfer error handling
On 12/04/18 17:06, Ioan Nicu wrote:
> Some of the mport_dma_req structure members were initialized late
> inside the do_dma_request() function, just before submitting the
> request to the dma engine. But we have some error branches before
> that. In case of such an error, the code would return on the error
> path and trigger the calling of dma_req_free() with a req structure
> which is not completely initialized. This causes a NULL pointer
> dereference in dma_req_free().
>
> This patch fixes these error branches by making sure that all
> necessary mport_dma_req structure members are initialized in
> rio_dma_transfer() immediately after the request structure gets
> allocated.
>
> Signed-off-by: Ioan Nicu <ioan.nicu....@nokia.com>
Tested-by: Alexander Sverdlin <alexander.sverd...@nokia.com>
> ---
> drivers/rapidio/devices/rio_mport_cdev.c | 19 +--
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/rapidio/devices/rio_mport_cdev.c
> b/drivers/rapidio/devices/rio_mport_cdev.c
> index 9d27016c899e..0434ab7b6497 100644
> --- a/drivers/rapidio/devices/rio_mport_cdev.c
> +++ b/drivers/rapidio/devices/rio_mport_cdev.c
> @@ -740,10 +740,7 @@ static int do_dma_request(struct mport_dma_req *req,
> tx->callback = dma_xfer_callback;
> tx->callback_param = req;
>
> - req->dmach = chan;
> - req->sync = sync;
> req->status = DMA_IN_PROGRESS;
> - init_completion(>req_comp);
> kref_get(>refcount);
>
> cookie = dmaengine_submit(tx);
> @@ -831,13 +828,20 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
> if (!req)
> return -ENOMEM;
>
> - kref_init(>refcount);
> -
> ret = get_dma_channel(priv);
> if (ret) {
> kfree(req);
> return ret;
> }
> + chan = priv->dmach;
> +
> + kref_init(>refcount);
> + init_completion(>req_comp);
> + req->dir = dir;
> + req->filp = filp;
> + req->priv = priv;
> + req->dmach = chan;
> + req->sync = sync;
>
> /*
>* If parameter loc_addr != NULL, we are transferring data from/to
> @@ -925,11 +929,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
> xfer->offset, xfer->length);
> }
>
> - req->dir = dir;
> - req->filp = filp;
> - req->priv = priv;
> - chan = priv->dmach;
> -
> nents = dma_map_sg(chan->device->dev,
> req->sgt.sgl, req->sgt.nents, dir);
> if (nents == 0) {
--
Best regards,
Alexander Sverdlin.
Re: [PATCH] rapidio: fix rio_dma_transfer error handling
On 12/04/18 17:06, Ioan Nicu wrote:
> Some of the mport_dma_req structure members were initialized late
> inside the do_dma_request() function, just before submitting the
> request to the dma engine. But we have some error branches before
> that. In case of such an error, the code would return on the error
> path and trigger the calling of dma_req_free() with a req structure
> which is not completely initialized. This causes a NULL pointer
> dereference in dma_req_free().
>
> This patch fixes these error branches by making sure that all
> necessary mport_dma_req structure members are initialized in
> rio_dma_transfer() immediately after the request structure gets
> allocated.
>
> Signed-off-by: Ioan Nicu
Tested-by: Alexander Sverdlin
> ---
> drivers/rapidio/devices/rio_mport_cdev.c | 19 +--
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/rapidio/devices/rio_mport_cdev.c
> b/drivers/rapidio/devices/rio_mport_cdev.c
> index 9d27016c899e..0434ab7b6497 100644
> --- a/drivers/rapidio/devices/rio_mport_cdev.c
> +++ b/drivers/rapidio/devices/rio_mport_cdev.c
> @@ -740,10 +740,7 @@ static int do_dma_request(struct mport_dma_req *req,
> tx->callback = dma_xfer_callback;
> tx->callback_param = req;
>
> - req->dmach = chan;
> - req->sync = sync;
> req->status = DMA_IN_PROGRESS;
> - init_completion(>req_comp);
> kref_get(>refcount);
>
> cookie = dmaengine_submit(tx);
> @@ -831,13 +828,20 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
> if (!req)
> return -ENOMEM;
>
> - kref_init(>refcount);
> -
> ret = get_dma_channel(priv);
> if (ret) {
> kfree(req);
> return ret;
> }
> + chan = priv->dmach;
> +
> + kref_init(>refcount);
> + init_completion(>req_comp);
> + req->dir = dir;
> + req->filp = filp;
> + req->priv = priv;
> + req->dmach = chan;
> + req->sync = sync;
>
> /*
>* If parameter loc_addr != NULL, we are transferring data from/to
> @@ -925,11 +929,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode,
> xfer->offset, xfer->length);
> }
>
> - req->dir = dir;
> - req->filp = filp;
> - req->priv = priv;
> - chan = priv->dmach;
> -
> nents = dma_map_sg(chan->device->dev,
> req->sgt.sgl, req->sgt.nents, dir);
> if (nents == 0) {
--
Best regards,
Alexander Sverdlin.
