* Re: [PATCH] pinctrl: sh-pfc: Unlock on error in sh_pfc_func_set_mux()
From: Geert Uytterhoeven @ 2019-08-27 9:49 UTC (permalink / raw)
To: Dan Carpenter, Linus Walleij
Cc: Geert Uytterhoeven, Yoshihiro Shimoda, Linux-Renesas,
open list:GPIO SUBSYSTEM, kernel-janitors
In-Reply-To: <20190827093927.GB8443@mwanda>
On Tue, Aug 27, 2019 at 11:39 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We need to unlock and enable IRQs before we return on this error path.
Wow, how could we have missed that?!?
Thanks!
> Fixes: 8a0cc47ccc7c ("pinctrl: sh-pfc: Rollback to mux if required when the gpio is freed")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Linus: As we're already past rc6, I don't plan to send another pull
request for v5.4.
Hence can you please take this one directly?
Thanks!
> --- a/drivers/pinctrl/sh-pfc/pinctrl.c
> +++ b/drivers/pinctrl/sh-pfc/pinctrl.c
> @@ -361,8 +361,10 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
> * This driver cannot manage both gpio and mux when the gpio
> * pin is already enabled. So, this function fails.
> */
> - if (cfg->gpio_enabled)
> - return -EBUSY;
> + if (cfg->gpio_enabled) {
> + ret = -EBUSY;
> + goto done;
> + }
>
> ret = sh_pfc_config_mux(pfc, grp->mux[i], PINMUX_TYPE_FUNCTION);
> if (ret < 0)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH] pinctrl: sh-pfc: Unlock on error in sh_pfc_func_set_mux()
From: Dan Carpenter @ 2019-08-27 9:39 UTC (permalink / raw)
To: Geert Uytterhoeven, Yoshihiro Shimoda
Cc: Linus Walleij, linux-renesas-soc, linux-gpio, kernel-janitors
We need to unlock and enable IRQs before we return on this error path.
Fixes: 8a0cc47ccc7c ("pinctrl: sh-pfc: Rollback to mux if required when the gpio is freed")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/pinctrl/sh-pfc/pinctrl.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
index 99f4ebd69861..212a4a9c3a8f 100644
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -361,8 +361,10 @@ static int sh_pfc_func_set_mux(struct pinctrl_dev *pctldev, unsigned selector,
* This driver cannot manage both gpio and mux when the gpio
* pin is already enabled. So, this function fails.
*/
- if (cfg->gpio_enabled)
- return -EBUSY;
+ if (cfg->gpio_enabled) {
+ ret = -EBUSY;
+ goto done;
+ }
ret = sh_pfc_config_mux(pfc, grp->mux[i], PINMUX_TYPE_FUNCTION);
if (ret < 0)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/6 v2] gpio: Add support for hierarchical IRQ domains
From: Marc Zyngier @ 2019-08-27 9:00 UTC (permalink / raw)
To: Lina Iyer
Cc: Linus Walleij, Brian Masney, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Thomas Gleixner, Jon Hunter,
Sowjanya Komatineni, Bitan Biswas, linux-tegra, David Daney,
Masahiro Yamada, Thierry Reding
In-Reply-To: <20190826201400.GC26639@codeaurora.org>
On 26/08/2019 21:14, Lina Iyer wrote:
> On Mon, Aug 26 2019 at 13:02 -0600, Marc Zyngier wrote:
>> On Mon, 26 Aug 2019 10:15:26 -0600
>> Lina Iyer <ilina@codeaurora.org> wrote:
>>
>>> On Fri, Aug 23 2019 at 03:12 -0600, Marc Zyngier wrote:
>>>> On 23/08/2019 09:24, Linus Walleij wrote:
>>>>> Hi Brian,
>>>>>
>>>>> I tried to look into this ssbi-gpio problem...
>>>>>
>>>>> Paging in Marc Z as well.
>>>>>
>>>>> On Fri, Aug 16, 2019 at 3:10 AM Brian Masney <masneyb@onstation.org> wrote:
>>>>>
>>>>>> I started to convert ssbi-gpio over to this and pm8xxx_gpio_to_irq() has
>>>>>> this little snippet that's different from spmi-gpio:
>>>>>>
>>>>>> [ fwspec mapping code ]
>>>>>>
>>>>>> /*
>>>>>> * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
>>>>>> * line level.
>>>>>> */
>>>>>> pin->irq = ret;
>>>>>>
>>>>>> Here's the relevant code in pm8xxx_gpio_get():
>>>>>>
>>>>>> if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
>>>>>> ret = pin->output_value;
>>>>>> } else if (pin->irq >= 0) {
>>>>>> ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
>>>>>> ...
>>>>>> }
>>>>>
>>>>> It's a bit annoying that this API (irq_get_irqchip_state()) is relying on
>>>>> the global irq numbers and the internal function using irqdesc
>>>>> __irq_get_irqchip_state() is not exported.
>>>>>
>>>>> We should be encouraged to operate on IRQ descriptors rather
>>>>> than IRQ numbers when doing this kind of deep core work,
>>>>> right?
>>>>
>>>> Certainly, I'd like to gradually move over from the IRQ number to an
>>>> irq_desc. In interrupt-heavy contexts, this ends up being quite time
>>>> consuming. I have an old patch series somewhere changing irq domains to
>>>> use irq_descs internally instead of IRQ numbers, which I should maybe
>>>> revive.
>>>>
>>>> As for __irq_get_irqchip_state, the main issue is that it doesn't
>>>> perform any locking on its own, so you'd have to either provide your
>>>> own, or wrap it with something else.
>>>>
>>> Marc, on a related note, What are your thoughts on a
>>> irq_set_irqchip_state? We are running into an issue where chip state
>>> clear is required as well as clearing out that of the parent. Do you see
>>> problems in doing that from an irqchip driver?
>>
>> [desperately trying to switch to my korg address...]
>>
>> I'm not sure I fully get the question: if this is whether it is worth
>> implementing it, it usually is a good idea if the HW supports is. But I
>> have the feeling that this isn't your question, so maybe explaining
>> your use-case would help.
>>
>> Do you mean using irq_set_irqchip_state from within an irqchip driver?
>> That'd be quite odd, as this function is usually used when something
>> *outside* of the irqchip driver needs to poke at some internal state
>> because "it knows best" (see for example what KVM does with the active
>> state).
>>
> Yeah, I meant to say, when invoked on a parent irqchip from a child
> irqchip.
>
> In my case when GPIO is used as an interrupt. Even though the interrupt
> is not enabled, the GPIO could cause the GIC_ISPEND to be set at GIC. We
> were wondering if we could use the irq_set_irqchip_state to clear
> pending state at GPIO's irqchip parent and all the way to GIC, when the
> GPIO is enabled as an interrupt.
You can of course chain the call from one level to its parent. It's a
bit odd to have to clear a pending bit in this context, but I guess
that's a HW-specific limitation.
M.
--
Jazz is not dead, it just smells funny...
^ permalink raw reply
* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
From: Harish Jenny K N @ 2019-08-27 7:47 UTC (permalink / raw)
To: Linus Walleij, Rob Herring
Cc: Bartosz Golaszewski, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan
In-Reply-To: <978af20e-12aa-a8e9-5da9-9af6d6b8f553@mentor.com>
Hi Rob,
On 19/08/19 3:06 PM, Harish Jenny K N wrote:
> Hi Rob,
>
>
> On 10/08/19 2:21 PM, Linus Walleij wrote:
>> On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
>>> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> There is some level of ambition here which is inherently a bit fuzzy
>>>> around the edges. ("How long is the coast of Britain?" comes to mind.)
>>>>
>>>> Surely the intention of device tree is not to recreate the schematic
>>>> in all detail. What we want is a model of the hardware that will
>>>> suffice for the operating system usecases.
>>>>
>>>> But sometimes the DTS files will become confusing: why is this
>>>> component using GPIO_ACTIVE_LOW when another system
>>>> doesn't have that flag? If there is an explicit inverter, the
>>>> DTS gets more readable for a human.
>>>>
>>>> But arguable that is case for adding inverters as syntactic
>>>> sugar in the DTS compiler instead...
>>> If you really want something more explicit, then add a new GPIO
>>> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
>>> That also solves the abstract it for userspace problem.
>> I think there are some intricate ontologies at work here.
>>
>> Consider this example: a GPIO is controlling a chip select
>> regulator, say Acme Foo. The chip select
>> has a pin named CSN. We know from convention that the
>> "N" at the end of that pin name means "negative" i.e. active
>> low, and that is how the electronics engineers think about
>> that chip select line: it activates the IC when
>> the line goes low.
>>
>> The regulator subsystem and I think all subsystems in the
>> Linux kernel say the consumer pin should be named and
>> tagged after the datsheet of the regulator.
>>
>> So it has for example:
>>
>> foo {
>> compatible = "acme,foo";
>> cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
>> };
>>
>> (It would be inappropriate to name it "csn-gpios" since
>> we have an established flag for active low. But it is another
>> of these syntactic choices where people likely do mistakes.)
>>
>> I think it would be appropriate for the DT binding to say
>> that this flag must always be GPIO_ACTIVE_LOW since
>> the bindings are seen from the component point of view,
>> and thus this is always active low.
>>
>> It would even be reasonable for a yaml schema to enfore
>> this, if it could. It is defined as active low after all.
>>
>> Now if someone adds an inverter on that line between
>> gpio0 and Acme Foo it looks like this:
>>
>> foo {
>> compatible = "acme,foo";
>> cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>> };
>>
>> And now we get cognitive dissonance or whatever I should
>> call it: someone reading this DTS sheet and the data
>> sheet for the component Acme Foo to troubleshoot
>> this will be confused: this component has CS active
>> low and still it is specified as active high? Unless they
>> also look at the schematic or the board and find the
>> inverter things are pretty muddy and they will likely curse
>> and solve the situation with the usual trial-and-error,
>> inserting some random cursewords as a comment.
>>
>> With an intermediate inverter node, the cs-gpios
>> can go back to GPIO_ACTIVE_LOW and follow
>> the bindings:
>>
>> inv0: inverter {
>> compatible = "gpio-inverter";
>> gpio-controller;
>> #gpio-cells = <1>;
>> inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>> };
>>
>> foo {
>> compatible = "acme,foo";
>> cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
>> };
>>
>> And now Acme Foo bindings can keep enforcing cs-gpios
>> to always be tagged GPIO_ACTIVE_LOW.
>
> Can you please review/let us know your opinion on this ? I think the idea here is to also isolate the changes to a separate consumer driver and avoid getting inversions inside gpiolib.
>
>
> Thanks.
>
>
> Regards,
>
> Harish Jenny K N
>
Can you please comment on this ?
Thanks,
Harish Jenny K N
^ permalink raw reply
* Re: [PATCH 0/3] CP115 pinctrl support
From: Linus Walleij @ 2019-08-27 7:50 UTC (permalink / raw)
To: Miquel Raynal
Cc: Yan Markman, Thomas Petazzoni, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Gregory Clement, Antoine Tenart, Maxime Chevallier, Nadav Haklai,
open list:GPIO SUBSYSTEM, Linux ARM, Grzegorz Jaszczyk,
Marcin Wojtas, Stefan Chulski
In-Reply-To: <20190824133317.371dec4f@xps13>
On Sat, Aug 24, 2019 at 1:33 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> Linus Walleij <linus.walleij@linaro.org> wrote on Thu, 15 Aug 2019
> > OK then maybe I am a bit impatient.
>
> Actually Gregory is on vacation until September, so if we still are in
> time for this merge window I suppose you can take it.
OK I applied the patches.
If someone is upset we can always revert them in the -rc phase.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 3/3] pinctrl: mvebu: add additional variant for standalone CP115
From: Linus Walleij @ 2019-08-27 7:49 UTC (permalink / raw)
To: Miquel Raynal
Cc: Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Thomas Petazzoni, Gregory Clement, Antoine Tenart,
Maxime Chevallier, Nadav Haklai, open list:GPIO SUBSYSTEM,
Linux ARM, Grzegorz Jaszczyk, Marcin Wojtas, Stefan Chulski,
Yan Markman
In-Reply-To: <20190805101607.29811-4-miquel.raynal@bootlin.com>
On Mon, Aug 5, 2019 at 12:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> From: Grzegorz Jaszczyk <jaz@semihalf.com>
>
> With CP115 standalone modules, all MPP configuration are
> possible. Handle this new possibility thanks to the new
> "marvell,cp115-standalone-pinctrl" compatible property.
>
> Signed-off-by: Grzegorz Jaszczyk <jaz@semihalf.com>
> [<miquel.raynal@bootlin.com>: mention the new compatible in the
> commit log]
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Patch applied.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 1/3] pinctrl: mvebu: Add CP110 missing pin functionality
From: Linus Walleij @ 2019-08-27 7:48 UTC (permalink / raw)
To: Miquel Raynal
Cc: Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Thomas Petazzoni, Gregory Clement, Antoine Tenart,
Maxime Chevallier, Nadav Haklai, open list:GPIO SUBSYSTEM,
Linux ARM, Grzegorz Jaszczyk, Marcin Wojtas, Stefan Chulski,
Yan Markman, Konstantin Porotchkin
In-Reply-To: <20190805101607.29811-2-miquel.raynal@bootlin.com>
On Mon, Aug 5, 2019 at 12:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> From: Konstantin Porotchkin <kostap@marvell.com>
>
> Add missing definition for function 0xe on CP-110 MPP-62.
> The pin function is Data Strobe for SDIO interface.
>
> Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Patch applied.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 2/3] dt-bindings: cp110: document the new CP115 pinctrl compatible
From: Linus Walleij @ 2019-08-27 7:46 UTC (permalink / raw)
To: Miquel Raynal
Cc: Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Thomas Petazzoni, Gregory Clement, Antoine Tenart,
Maxime Chevallier, Nadav Haklai, open list:GPIO SUBSYSTEM,
Linux ARM, Grzegorz Jaszczyk, Marcin Wojtas, Stefan Chulski,
Yan Markman
In-Reply-To: <20190805101607.29811-3-miquel.raynal@bootlin.com>
On Mon, Aug 5, 2019 at 12:16 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> From: Grzegorz Jaszczyk <jaz@semihalf.com>
>
> A new compatible is going to be used for Armada CP115 pinctrl block,
> document it.
>
> Signed-off-by: Grzegorz Jaszczyk <jaz@semihalf.com>
> [<miquel.raynal@bootlin.com>: split the documentation out of the
> driver commit]
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Patch applied with Rob's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH 2/2] gpio: pca953x.c: Use pca953x_read_regs instead of regmap_bulk_read
From: David Jander @ 2019-08-27 6:46 UTC (permalink / raw)
To: Linus Walleij; +Cc: Bartosz Golaszewski, linux-gpio, linux-kernel, David Jander
In-Reply-To: <20190827064629.90214-1-david@protonic.nl>
The register number needs to be translated for chips with more than 8
ports. This patch fixes a bug causing all chips with more than 8 GPIO pins
to not work correctly.
Signed-off-by: David Jander <david@protonic.nl>
---
drivers/gpio/gpio-pca953x.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 30072a570bc2..48fea4c68e8d 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -606,8 +606,7 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
u8 invert_irq_mask[MAX_BANK];
u8 reg_direction[MAX_BANK];
- regmap_bulk_read(chip->regmap, chip->regs->direction, reg_direction,
- NBANK(chip));
+ pca953x_read_regs(chip, chip->regs->direction, reg_direction);
if (chip->driver_data & PCA_PCAL) {
/* Enable latch on interrupt-enabled inputs */
@@ -710,8 +709,7 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
return false;
/* Remove output pins from the equation */
- regmap_bulk_read(chip->regmap, chip->regs->direction, reg_direction,
- NBANK(chip));
+ pca953x_read_regs(chip, chip->regs->direction, reg_direction);
for (i = 0; i < NBANK(chip); i++)
cur_stat[i] &= reg_direction[i];
@@ -789,8 +787,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
* interrupt. We have to rely on the previous read for
* this purpose.
*/
- regmap_bulk_read(chip->regmap, chip->regs->direction, reg_direction,
- NBANK(chip));
+ pca953x_read_regs(chip, chip->regs->direction, reg_direction);
for (i = 0; i < NBANK(chip); i++)
chip->irq_stat[i] &= reg_direction[i];
mutex_init(&chip->irq_lock);
--
2.19.1
^ permalink raw reply related
* [PATCH 1/2] gpio: gpio-pca953x.c: Correct type of reg_direction
From: David Jander @ 2019-08-27 6:46 UTC (permalink / raw)
To: Linus Walleij; +Cc: Bartosz Golaszewski, linux-gpio, linux-kernel, David Jander
The type of reg_direction needs to match the type of the regmap, which is
u8.
Signed-off-by: David Jander <david@protonic.nl>
---
drivers/gpio/gpio-pca953x.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 378b206d2dc9..30072a570bc2 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -604,7 +604,7 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
u8 new_irqs;
int level, i;
u8 invert_irq_mask[MAX_BANK];
- int reg_direction[MAX_BANK];
+ u8 reg_direction[MAX_BANK];
regmap_bulk_read(chip->regmap, chip->regs->direction, reg_direction,
NBANK(chip));
@@ -679,7 +679,7 @@ static bool pca953x_irq_pending(struct pca953x_chip *chip, u8 *pending)
bool pending_seen = false;
bool trigger_seen = false;
u8 trigger[MAX_BANK];
- int reg_direction[MAX_BANK];
+ u8 reg_direction[MAX_BANK];
int ret, i;
if (chip->driver_data & PCA_PCAL) {
@@ -768,7 +768,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
{
struct i2c_client *client = chip->client;
struct irq_chip *irq_chip = &chip->irq_chip;
- int reg_direction[MAX_BANK];
+ u8 reg_direction[MAX_BANK];
int ret, i;
if (!client->irq)
--
2.19.1
^ permalink raw reply related
* Re: [PATCH 1/6 v2] gpio: Add support for hierarchical IRQ domains
From: Lina Iyer @ 2019-08-26 20:14 UTC (permalink / raw)
To: Marc Zyngier
Cc: Linus Walleij, Brian Masney, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Thomas Gleixner, Jon Hunter,
Sowjanya Komatineni, Bitan Biswas, linux-tegra, David Daney,
Masahiro Yamada, Thierry Reding
In-Reply-To: <20190826194400.67c6e99b@why>
On Mon, Aug 26 2019 at 13:02 -0600, Marc Zyngier wrote:
>On Mon, 26 Aug 2019 10:15:26 -0600
>Lina Iyer <ilina@codeaurora.org> wrote:
>
>> On Fri, Aug 23 2019 at 03:12 -0600, Marc Zyngier wrote:
>> >On 23/08/2019 09:24, Linus Walleij wrote:
>> >> Hi Brian,
>> >>
>> >> I tried to look into this ssbi-gpio problem...
>> >>
>> >> Paging in Marc Z as well.
>> >>
>> >> On Fri, Aug 16, 2019 at 3:10 AM Brian Masney <masneyb@onstation.org> wrote:
>> >>
>> >>> I started to convert ssbi-gpio over to this and pm8xxx_gpio_to_irq() has
>> >>> this little snippet that's different from spmi-gpio:
>> >>>
>> >>> [ fwspec mapping code ]
>> >>>
>> >>> /*
>> >>> * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
>> >>> * line level.
>> >>> */
>> >>> pin->irq = ret;
>> >>>
>> >>> Here's the relevant code in pm8xxx_gpio_get():
>> >>>
>> >>> if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
>> >>> ret = pin->output_value;
>> >>> } else if (pin->irq >= 0) {
>> >>> ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
>> >>> ...
>> >>> }
>> >>
>> >> It's a bit annoying that this API (irq_get_irqchip_state()) is relying on
>> >> the global irq numbers and the internal function using irqdesc
>> >> __irq_get_irqchip_state() is not exported.
>> >>
>> >> We should be encouraged to operate on IRQ descriptors rather
>> >> than IRQ numbers when doing this kind of deep core work,
>> >> right?
>> >
>> >Certainly, I'd like to gradually move over from the IRQ number to an
>> >irq_desc. In interrupt-heavy contexts, this ends up being quite time
>> >consuming. I have an old patch series somewhere changing irq domains to
>> >use irq_descs internally instead of IRQ numbers, which I should maybe
>> >revive.
>> >
>> >As for __irq_get_irqchip_state, the main issue is that it doesn't
>> >perform any locking on its own, so you'd have to either provide your
>> >own, or wrap it with something else.
>> >
>> Marc, on a related note, What are your thoughts on a
>> irq_set_irqchip_state? We are running into an issue where chip state
>> clear is required as well as clearing out that of the parent. Do you see
>> problems in doing that from an irqchip driver?
>
>[desperately trying to switch to my korg address...]
>
>I'm not sure I fully get the question: if this is whether it is worth
>implementing it, it usually is a good idea if the HW supports is. But I
>have the feeling that this isn't your question, so maybe explaining
>your use-case would help.
>
>Do you mean using irq_set_irqchip_state from within an irqchip driver?
>That'd be quite odd, as this function is usually used when something
>*outside* of the irqchip driver needs to poke at some internal state
>because "it knows best" (see for example what KVM does with the active
>state).
>
Yeah, I meant to say, when invoked on a parent irqchip from a child
irqchip.
In my case when GPIO is used as an interrupt. Even though the interrupt
is not enabled, the GPIO could cause the GIC_ISPEND to be set at GIC. We
were wondering if we could use the irq_set_irqchip_state to clear
pending state at GPIO's irqchip parent and all the way to GIC, when the
GPIO is enabled as an interrupt.
-- Lina
^ permalink raw reply
* Re: [PATCH 1/6 v2] gpio: Add support for hierarchical IRQ domains
From: Marc Zyngier @ 2019-08-26 18:44 UTC (permalink / raw)
To: Lina Iyer
Cc: Linus Walleij, Brian Masney, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Thomas Gleixner, Jon Hunter,
Sowjanya Komatineni, Bitan Biswas, linux-tegra, David Daney,
Masahiro Yamada, Thierry Reding
In-Reply-To: <20190826161526.GB26639@codeaurora.org>
On Mon, 26 Aug 2019 10:15:26 -0600
Lina Iyer <ilina@codeaurora.org> wrote:
> On Fri, Aug 23 2019 at 03:12 -0600, Marc Zyngier wrote:
> >On 23/08/2019 09:24, Linus Walleij wrote:
> >> Hi Brian,
> >>
> >> I tried to look into this ssbi-gpio problem...
> >>
> >> Paging in Marc Z as well.
> >>
> >> On Fri, Aug 16, 2019 at 3:10 AM Brian Masney <masneyb@onstation.org> wrote:
> >>
> >>> I started to convert ssbi-gpio over to this and pm8xxx_gpio_to_irq() has
> >>> this little snippet that's different from spmi-gpio:
> >>>
> >>> [ fwspec mapping code ]
> >>>
> >>> /*
> >>> * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
> >>> * line level.
> >>> */
> >>> pin->irq = ret;
> >>>
> >>> Here's the relevant code in pm8xxx_gpio_get():
> >>>
> >>> if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
> >>> ret = pin->output_value;
> >>> } else if (pin->irq >= 0) {
> >>> ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
> >>> ...
> >>> }
> >>
> >> It's a bit annoying that this API (irq_get_irqchip_state()) is relying on
> >> the global irq numbers and the internal function using irqdesc
> >> __irq_get_irqchip_state() is not exported.
> >>
> >> We should be encouraged to operate on IRQ descriptors rather
> >> than IRQ numbers when doing this kind of deep core work,
> >> right?
> >
> >Certainly, I'd like to gradually move over from the IRQ number to an
> >irq_desc. In interrupt-heavy contexts, this ends up being quite time
> >consuming. I have an old patch series somewhere changing irq domains to
> >use irq_descs internally instead of IRQ numbers, which I should maybe
> >revive.
> >
> >As for __irq_get_irqchip_state, the main issue is that it doesn't
> >perform any locking on its own, so you'd have to either provide your
> >own, or wrap it with something else.
> >
> Marc, on a related note, What are your thoughts on a
> irq_set_irqchip_state? We are running into an issue where chip state
> clear is required as well as clearing out that of the parent. Do you see
> problems in doing that from an irqchip driver?
[desperately trying to switch to my korg address...]
I'm not sure I fully get the question: if this is whether it is worth
implementing it, it usually is a good idea if the HW supports is. But I
have the feeling that this isn't your question, so maybe explaining
your use-case would help.
Do you mean using irq_set_irqchip_state from within an irqchip driver?
That'd be quite odd, as this function is usually used when something
*outside* of the irqchip driver needs to poke at some internal state
because "it knows best" (see for example what KVM does with the active
state).
But again, stating your problem would help.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply
* Re: [PATCH 1/6 v2] gpio: Add support for hierarchical IRQ domains
From: Lina Iyer @ 2019-08-26 16:15 UTC (permalink / raw)
To: Marc Zyngier
Cc: Linus Walleij, Brian Masney, open list:GPIO SUBSYSTEM,
Bartosz Golaszewski, Thomas Gleixner, Jon Hunter,
Sowjanya Komatineni, Bitan Biswas, linux-tegra, David Daney,
Masahiro Yamada, Thierry Reding
In-Reply-To: <63f2d086-eb71-4153-071e-71102fe24a14@arm.com>
On Fri, Aug 23 2019 at 03:12 -0600, Marc Zyngier wrote:
>On 23/08/2019 09:24, Linus Walleij wrote:
>> Hi Brian,
>>
>> I tried to look into this ssbi-gpio problem...
>>
>> Paging in Marc Z as well.
>>
>> On Fri, Aug 16, 2019 at 3:10 AM Brian Masney <masneyb@onstation.org> wrote:
>>
>>> I started to convert ssbi-gpio over to this and pm8xxx_gpio_to_irq() has
>>> this little snippet that's different from spmi-gpio:
>>>
>>> [ fwspec mapping code ]
>>>
>>> /*
>>> * Cache the IRQ since pm8xxx_gpio_get() needs this to get determine the
>>> * line level.
>>> */
>>> pin->irq = ret;
>>>
>>> Here's the relevant code in pm8xxx_gpio_get():
>>>
>>> if (pin->mode == PM8XXX_GPIO_MODE_OUTPUT) {
>>> ret = pin->output_value;
>>> } else if (pin->irq >= 0) {
>>> ret = irq_get_irqchip_state(pin->irq, IRQCHIP_STATE_LINE_LEVEL, &state);
>>> ...
>>> }
>>
>> It's a bit annoying that this API (irq_get_irqchip_state()) is relying on
>> the global irq numbers and the internal function using irqdesc
>> __irq_get_irqchip_state() is not exported.
>>
>> We should be encouraged to operate on IRQ descriptors rather
>> than IRQ numbers when doing this kind of deep core work,
>> right?
>
>Certainly, I'd like to gradually move over from the IRQ number to an
>irq_desc. In interrupt-heavy contexts, this ends up being quite time
>consuming. I have an old patch series somewhere changing irq domains to
>use irq_descs internally instead of IRQ numbers, which I should maybe
>revive.
>
>As for __irq_get_irqchip_state, the main issue is that it doesn't
>perform any locking on its own, so you'd have to either provide your
>own, or wrap it with something else.
>
Marc, on a related note, What are your thoughts on a
irq_set_irqchip_state? We are running into an issue where chip state
clear is required as well as clearing out that of the parent. Do you see
problems in doing that from an irqchip driver?
--Lina
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Enrico Weigelt, metux IT consult @ 2019-08-26 13:29 UTC (permalink / raw)
To: Uwe Kleine-König, Andrew Morton
Cc: Jonathan Corbet, linux-doc, linux-kernel, linux-gpio,
Linus Walleij, Bartosz Golaszewski
In-Reply-To: <20190824233724.1775-1-uwe@kleine-koenig.org>
On 25.08.19 01:37, Uwe Kleine-König wrote:
Hi,
> +static noinline_for_stack > +char *errstr(char *buf, char *end, unsigned long long num,> +
struct printf_spec spec)> +{
#1: why not putting that into some separate strerror() lib function ?
This is something I've been looking for quite some time (actually
already hacked it up somewhere, sometime, but forgotten ...)
#2: why not just having a big case statement and leave the actual lookup
logic to the compiler ? IMHO, could be written in a very compact way
by some macro magic
> + for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) { > + if (num == errorcodes[i].err || num == -errorcodes[i].err) {
why not taking the abs value only once, instead of duplicate comp on
each iteration ?
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Petr Mladek @ 2019-08-26 12:05 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Andrew Morton, Bartosz Golaszewski, Sergey Senozhatsky,
Steven Rostedt, Linus Walleij, Jonathan Corbet, linux-doc,
linux-gpio, linux-kernel
In-Reply-To: <20190825091442.GA5817@taurus.defre.kleine-koenig.org>
On Sun 2019-08-25 11:14:42, Uwe Kleine-König wrote:
> Hello Andrew,
>
> On Sat, Aug 24, 2019 at 04:58:29PM -0700, Andrew Morton wrote:
> > (cc printk maintainers).
>
> Ah, I wasn't aware there is something like them. Thanks
>
> > On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> >
> > > pr_info("probing failed (%dE)\n", ret);
> > >
> > > expands to
> > >
> > > probing failed (EIO)
> > >
> > > if ret holds -EIO (or EIO). This introduces an array of error codes. If
> > > the error code is missing, %dE falls back to %d and so prints the plain
> > > number.
What was the motivation for this patch, please?
Did it look like a good idea?
Did anyone got tired by searching for the error codes many
times a day?
Did the idea came from a developer, support, or user, please?
> add/remove: 2/0 grow/shrink: 4/2 up/down: 1488/-8 (1480)
> Function old new delta
> errorcodes - 1200 +1200
> errstr - 200 +200
> vsnprintf 884 960 +76
> set_precision 148 152 +4
> resource_string 1380 1384 +4
> flags_string 400 404 +4
> num_to_str 288 284 -4
> format_decode 1024 1020 -4
> Total: Before=21686, After=23166, chg +6.82%
>
> But that doesn't seem to include the size increase for all the added
> strings which seems to be around another 1300 bytes.
This non-trivial increase of the size and the table still
includes only part of the error codes.
The array is long, created by cpu&paste, the index of each code
is not obvious.
There are ideas to make the code even more tricky to reduce
the size, keep it fast.
Both, %dE modifier and the output format (ECODE) is non-standard.
Upper letters gain a lot of attention. But the error code is
only helper information. Also many error codes are misleading because
they are used either wrongly or there was no better available.
There is no proof that this approach would be widely acceptable for
subsystem maintainers. Some might not like mass and "blind" code
changes. Some might not like the output at all.
I am not persuaded that all this is worth it. Also I do not like
the non-standard solution.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH 5/6] dt-bindings: pinctrl: k3: Introduce pinmux definitions for J721E
From: Rob Herring @ 2019-08-26 11:25 UTC (permalink / raw)
To: Lokesh Vutla
Cc: Tero Kristo, Nishanth Menon, Linus Walleij, Keerthy,
open list:GPIO SUBSYSTEM, Device Tree Mailing List,
Linux ARM Mailing List
In-Reply-To: <da0286cb-8e4c-e12c-240c-b6de72bdd0ee@ti.com>
On Mon, Aug 26, 2019 at 5:27 AM Lokesh Vutla <lokeshvutla@ti.com> wrote:
>
> Hi Rob,
>
> On 22/08/19 2:32 AM, Rob Herring wrote:
> > On Fri, Aug 09, 2019 at 01:59:46PM +0530, Lokesh Vutla wrote:
> >> Add pinctrl macros for J721E SoC. These macro definitions are
> >> similar to that of AM6, but adding new definitions to avoid
> >> any naming confusions in the soc dts files.
> >>
> >> Acked-by: Nishanth Menon <nm@ti.com>
> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> >> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> >> ---
> >> include/dt-bindings/pinctrl/k3.h | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/include/dt-bindings/pinctrl/k3.h b/include/dt-bindings/pinctrl/k3.h
> >> index 45e11b6170ca..499de6216581 100644
> >> --- a/include/dt-bindings/pinctrl/k3.h
> >> +++ b/include/dt-bindings/pinctrl/k3.h
> >> @@ -32,4 +32,7 @@
> >> #define AM65X_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
> >> #define AM65X_WKUP_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
> >>
> >> +#define J721E_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
> >> +#define J721E_WKUP_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
> >
> > checkpatch reports a parentheses error: (((pa) & 0x1fff) ((val) | (muxmode)))
>
> This was left intentionally as this macro is giving out two entries in DT like
> below:
>
> pinctrl-single,pins = <
> J721E_IOPAD(0x0, PIN_INPUT, 7)
> >;
>
> comes out as
>
> pinctrl-single,pins = <
> 0x0 (PIN_INPUT | 7)
> >;
>
> If parenthesis are added for the whole macro, the following build error occurs:
> DTC arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dtb
> Error: arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts:41.24-25 syntax error
> FATAL ERROR: Unable to parse input tree
Okay, NM.
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 5/6] dt-bindings: pinctrl: k3: Introduce pinmux definitions for J721E
From: Lokesh Vutla @ 2019-08-26 10:26 UTC (permalink / raw)
To: Rob Herring
Cc: Tero Kristo, Nishanth Menon, linus.walleij, Keerthy, linux-gpio,
Device Tree Mailing List, Linux ARM Mailing List
In-Reply-To: <20190821210232.GA22578@bogus>
Hi Rob,
On 22/08/19 2:32 AM, Rob Herring wrote:
> On Fri, Aug 09, 2019 at 01:59:46PM +0530, Lokesh Vutla wrote:
>> Add pinctrl macros for J721E SoC. These macro definitions are
>> similar to that of AM6, but adding new definitions to avoid
>> any naming confusions in the soc dts files.
>>
>> Acked-by: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>> include/dt-bindings/pinctrl/k3.h | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/include/dt-bindings/pinctrl/k3.h b/include/dt-bindings/pinctrl/k3.h
>> index 45e11b6170ca..499de6216581 100644
>> --- a/include/dt-bindings/pinctrl/k3.h
>> +++ b/include/dt-bindings/pinctrl/k3.h
>> @@ -32,4 +32,7 @@
>> #define AM65X_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>> #define AM65X_WKUP_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>>
>> +#define J721E_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>> +#define J721E_WKUP_IOPAD(pa, val, muxmode) (((pa) & 0x1fff)) ((val) | (muxmode))
>
> checkpatch reports a parentheses error: (((pa) & 0x1fff) ((val) | (muxmode)))
This was left intentionally as this macro is giving out two entries in DT like
below:
pinctrl-single,pins = <
J721E_IOPAD(0x0, PIN_INPUT, 7)
>;
comes out as
pinctrl-single,pins = <
0x0 (PIN_INPUT | 7)
>;
If parenthesis are added for the whole macro, the following build error occurs:
DTC arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dtb
Error: arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts:41.24-25 syntax error
FATAL ERROR: Unable to parse input tree
Thanks and regards,
Lokesh
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Rasmus Villemoes @ 2019-08-26 10:13 UTC (permalink / raw)
To: Uwe Kleine-König, Andrew Morton
Cc: Jonathan Corbet, linux-doc, linux-kernel, linux-gpio,
Linus Walleij, Bartosz Golaszewski
In-Reply-To: <20190824233724.1775-1-uwe@kleine-koenig.org>
On 25/08/2019 01.37, Uwe Kleine-König wrote:
> pr_info("probing failed (%dE)\n", ret);
>
> expands to
>
> probing failed (EIO)
>
> if ret holds -EIO (or EIO). This introduces an array of error codes. If
> the error code is missing, %dE falls back to %d and so prints the plain
> number.
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello
>
> there are many code sites that benefit from this. Just grep for
> "(%d)" ...
>
> As an example the follow up patch converts a printk to use this new
> format escape.
>
> Best regards
> Uwe
>
> Documentation/core-api/printk-formats.rst | 3 +
> lib/vsprintf.c | 193 +++++++++++++++++++++-
> 2 files changed, 195 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index c6224d039bcb..81002414f956 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -35,6 +35,9 @@ Integer types
> u64 %llu or %llx
>
>
> +To print the name that corresponds to an integer error constant, use %dE and
> +pass the int.
> +
> If <type> is dependent on a config option for its size (e.g., sector_t,
> blkcnt_t) or is architecture-dependent for its size (e.g., tcflag_t), use a
> format specifier of its largest possible type and explicitly cast to it.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b0967cf17137..672eab8dab84 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
> return buf;
> }
>
> +#define ERRORCODE(x) { .str = #x, .err = x }
> +
> +static const struct {
> + const char *str;
> + int err;
> +} errorcodes[] = {
> + ERRORCODE(EPERM),
...
> + ERRORCODE(ERECALLCONFLICT),
> +};
> +
> +static noinline_for_stack
> +char *errstr(char *buf, char *end, unsigned long long num,
> + struct printf_spec spec)
> +{
> + char *errname = NULL;
> + size_t errnamelen, copy;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> + if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> + errname = errorcodes[i].str;
> + break;
> + }
> + }
Not sure I'm a fan of iterating this array. Yes, the errno values are a
bit sparse, but maybe one could use a dense array with O(1) lookup for
those < 128 and then have an exceptional table like yours for the rest.
But if you do keep this whole array thing, please do as Andrew suggested
and compact it somewhat (4 bytes per entry plus the storage for the
strings themselves is enough, see e.g. e1f0bce3), and put EINVAL and
other common things near the start (at least EINVAL is a lot more common
than ENOEXEC).
> + if (!errname) {
> + /* fall back to ordinary number */
> + return number(buf, end, num, spec);
> + }
> +
> + copy = errnamelen = strlen(errname);
> + if (copy > end - buf)
> + copy = end - buf;
> + buf = memcpy(buf, errname, copy);
This is buggy, AFAICT. buf may be beyond end (that's the convention), so
end-buf (which is a ptrdiff_t, which is probably a signed type, but it
gets converted to a size_t when compared to copy) can be a huge number,
so copy>end-buf is false.
Please just use the string() helper that gets used for printing other
fixed strings (as well as %s input).
And add tests to lib/test_printf.c, that would catch this sort of thing
immediately.
> + return buf + errnamelen;
> +}
> +
> static noinline_for_stack
> char *special_hex_number(char *buf, char *end, unsigned long long num, int size)
> {
> @@ -2566,7 +2752,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
> num = va_arg(args, unsigned int);
> }
>
> - str = number(str, end, num, spec);
> + if (spec.type == FORMAT_TYPE_INT && *fmt == 'E') {
> + fmt++;
> + str = errstr(str, end, num, spec);
drivers/staging/speakup/speakup_bns.c has a %dE, have you checked
whether you're breaking that one? It's hard to grep for all the
variations, %-0*.5dE is also legal and would be caught here.
This has previously been suggested as a %p extension, and I think users
would just as often have an ERR_PTR() as a plain int or long. Since we
already have %p[alphanumeric] as a special kernel thing, why not do
that? It's more convenient to convert from ints/longs to error pointers
pr_err("Uh-oh: %pE", ERR_PTR(ret));
than the other way around
pr_err("Uh-oh: %dE", PTR_ERR(p)); // oops, must be %ldE
Kernel size impact? Have you considered making it possible to opt-out
and have %dE just mean %d?
Rasmus
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Jani Nikula @ 2019-08-26 10:04 UTC (permalink / raw)
To: Andrew Morton, Uwe Kleine-König
Cc: Jonathan Corbet, linux-doc, linux-kernel, linux-gpio,
Linus Walleij, Bartosz Golaszewski, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt
In-Reply-To: <87o90c9rkc.fsf@intel.com>
On Mon, 26 Aug 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Sat, 24 Aug 2019, Andrew Morton <akpm@linux-foundation.org> wrote:
>>> --- a/lib/vsprintf.c
>>> +++ b/lib/vsprintf.c
>>> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
>>> return buf;
>>> }
>>>
>>> +#define ERRORCODE(x) { .str = #x, .err = x }
>>> +
>>> +static const struct {
>>> + const char *str;
>>> + int err;
>>> +} errorcodes[] = {
>>
>> It's a bit of a hack, but an array of char*'s and a separate array of
>> ushorts would save a bit of space.
>
> Or just
>
> #define ERRORCODE(x) [x] = #x
>
> static const char * const errorcodes[] = {
> ERRORCODE(EPERM),
> ERRORCODE(ENOENT),
> ...
> };
>
> Saves space, faster lookup, discovers at build time why EWOULDBLOCK
> would always show up as EAGAIN in the logs. We don't have holes to speak
> of in the error codes.
Meh, failed to notice the range ERESTARTSYS..ERECALLCONFLICT. Other than
that, it's nicer. ;)
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Jani Nikula @ 2019-08-26 9:58 UTC (permalink / raw)
To: Andrew Morton, Uwe Kleine-König
Cc: Jonathan Corbet, linux-doc, linux-kernel, linux-gpio,
Linus Walleij, Bartosz Golaszewski, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt
In-Reply-To: <20190824165829.7d330367992c62dab87f6652@linux-foundation.org>
On Sat, 24 Aug 2019, Andrew Morton <akpm@linux-foundation.org> wrote:
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
>> return buf;
>> }
>>
>> +#define ERRORCODE(x) { .str = #x, .err = x }
>> +
>> +static const struct {
>> + const char *str;
>> + int err;
>> +} errorcodes[] = {
>
> It's a bit of a hack, but an array of char*'s and a separate array of
> ushorts would save a bit of space.
Or just
#define ERRORCODE(x) [x] = #x
static const char * const errorcodes[] = {
ERRORCODE(EPERM),
ERRORCODE(ENOENT),
...
};
Saves space, faster lookup, discovers at build time why EWOULDBLOCK
would always show up as EAGAIN in the logs. We don't have holes to speak
of in the error codes.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH] gpiolib: acpi: Add gpiolib_acpi_run_edge_events_on_boot option and blacklist
From: Andy Shevchenko @ 2019-08-26 9:11 UTC (permalink / raw)
To: Hans de Goede
Cc: Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Benjamin Tissoires, linux-gpio, linux-acpi, linux-kernel, stable,
Daniel Drake, Ian W MORRISON
In-Reply-To: <20190823215255.7631-1-hdegoede@redhat.com>
On Fri, Aug 23, 2019 at 11:52:55PM +0200, Hans de Goede wrote:
> Another day; another DSDT bug we need to workaround...
>
> Since commit ca876c7483b6 ("gpiolib-acpi: make sure we trigger edge events
> at least once on boot") we call _AEI edge handlers at boot.
>
> In some rare cases this causes problems. One example of this is the Minix
> Neo Z83-4 mini PC, this device has a clear DSDT bug where it has some copy
> and pasted code for dealing with Micro USB-B connector host/device role
> switching, while the mini PC does not even have a micro-USB connector.
> This code, which should not be there, messes with the DDC data pin from
> the HDMI connector (switching it to GPIO mode) breaking HDMI support.
>
> To avoid problems like this, this commit adds a new
> gpiolib_acpi_run_edge_events_on_boot kernel commandline option which
> can be "on", "off", or "auto" (default).
>
> In auto mode the default is on and a DMI based blacklist is used,
> the initial version of this blacklist contains the Minix Neo Z83-4
> fixing the HDMI being broken on this device.
> +static int gpiolib_acpi_run_edge_events_on_boot = -1;
> +
> +static int __init gpiolib_acpi_run_edge_events_on_boot_setup(char *arg)
> +{
> + if (!strcmp(arg, "on"))
> + gpiolib_acpi_run_edge_events_on_boot = 1;
> + else if (!strcmp(arg, "off"))
> + gpiolib_acpi_run_edge_events_on_boot = 0;
kstrtobool() ?
> + else if (!strcmp(arg, "auto"))
> + gpiolib_acpi_run_edge_events_on_boot = -1;
> +
> + return 1;
> +}
> +
> +__setup("gpiolib_acpi_run_edge_events_on_boot=",
> + gpiolib_acpi_run_edge_events_on_boot_setup);
Can't we use module_param() ?
The resulting option would be 'gpiolib_acpi.foo=...'
> +{
> + if (gpiolib_acpi_run_edge_events_on_boot == -1) {
> + if (dmi_check_system(run_edge_events_on_boot_blacklist))
> + gpiolib_acpi_run_edge_events_on_boot = 0;
> + else
> + gpiolib_acpi_run_edge_events_on_boot = 1;
> + }
Can we run this at an initcall once and use variable instead of calling a
method below?
> + return gpiolib_acpi_run_edge_events_on_boot;
> +}
> +
> static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
> struct acpi_gpio_event *event)
> {
> @@ -170,10 +211,13 @@ static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
> event->irq_requested = true;
>
> /* Make sure we trigger the initial state of edge-triggered IRQs */
> - value = gpiod_get_raw_value_cansleep(event->desc);
> - if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> - ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> - event->handler(event->irq, event);
> + if (acpi_gpiochip_run_edge_events_on_boot() &&
> + (event->irqflags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING))) {
> + value = gpiod_get_raw_value_cansleep(event->desc);
> + if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> + ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> + event->handler(event->irq, event);
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] pinctrl: st: Include the right header
From: Patrice CHOTARD @ 2019-08-26 7:41 UTC (permalink / raw)
To: Linus Walleij, linux-gpio@vger.kernel.org; +Cc: Maxime Coquelin
In-Reply-To: <20190820111135.10701-1-linus.walleij@linaro.org>
Hi Linus
On 8/20/19 1:11 PM, Linus Walleij wrote:
> The ST pinctrl driver wants to provode a gpio_chip but is not
> including the header for this, fix the inclusion to use the right
> header. <linux/of_gpio.h> has to remain as the driver is calling
> of_get_named_gpio().
>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Maxime Coquelin <maxime.coquelin@st.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Include <linux/of_gpio.h> again, the driver is indeed using
> it.
> - Add an explanatory comment.
> ---
> drivers/pinctrl/pinctrl-st.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/pinctrl-st.c b/drivers/pinctrl/pinctrl-st.c
> index b9688ea548da..25236b716fb3 100644
> --- a/drivers/pinctrl/pinctrl-st.c
> +++ b/drivers/pinctrl/pinctrl-st.c
> @@ -12,8 +12,9 @@
> #include <linux/io.h>
> #include <linux/of.h>
> #include <linux/of_irq.h>
> -#include <linux/of_gpio.h>
> +#include <linux/of_gpio.h> /* of_get_named_gpio() */
> #include <linux/of_address.h>
> +#include <linux/gpio/driver.h>
> #include <linux/regmap.h>
> #include <linux/mfd/syscon.h>
> #include <linux/pinctrl/pinctrl.h>
Reviewed-by: Patrice Chotard <patrice.chotard@st.com>
Thanks
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Sergey Senozhatsky @ 2019-08-26 5:55 UTC (permalink / raw)
To: Andrew Morton
Cc: Uwe Kleine-König, Jonathan Corbet, linux-doc, linux-kernel,
linux-gpio, Linus Walleij, Bartosz Golaszewski, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt
In-Reply-To: <20190824165829.7d330367992c62dab87f6652@linux-foundation.org>
On (08/24/19 16:58), Andrew Morton wrote:
> On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> > pr_info("probing failed (%dE)\n", ret);
> >
> > expands to
> >
> > probing failed (EIO)
> >
> > if ret holds -EIO (or EIO). This introduces an array of error codes. If
> > the error code is missing, %dE falls back to %d and so prints the plain
> > number.
>
> Huh. I'm surprised we don't already have this. Seems that this will
> be applicable in a lot of places? Although we shouldn't go blindly
> converting everything in sight - that would risk breaking userspace
> which parses kernel strings.
>
> Is it really necessary to handle the positive errnos? Does much kernel
> code actually do that (apart from kernel code which is buggy)?
Good point.
POSIX functions on error usually return -1 (negative value) and set errno
(positive value). Positive errno value can be passed to strerror() or
strerror_r() that decode that value and return a human readable
representation. E.g. strerr(9) returns "Bad file descriptor".
We don't have errno. Instead, and I may be wrong on this, kernel functions
are expected to return negative error codes. A very quick grep shows that
there are, however, patterns like "return positive errno".
E.g. drivers/xen/xenbus/xenbus_xs.c: get_error()
return EINVAL;
But this EINVAL eventually becomes negative
err = get_error(ret);
return ERR_PTR(-err);
or net/bluetooth/lib.c: bt_to_errno(). But, once again, bt_to_errno()
return value eventually becomes negative:
err = -bt_to_errno(hdev->req_result);
So errstr() probably can handle only negative values. And, may be,
I'd rename errstr() to strerror(); just because there is a well known
function, which "translates" errnos.
Unlike strerror(), errstr() just returns a macro name. Example:
"Request failed: EJUKEBOX"
EJUKEBOX does not tell me anything. A quick way to find out what does
EJUKEBOX stand for is to grep include/linux/errno.h
#define EJUKEBOX 528 /* Request initiated, but will not complete before timeout */
One still has to grep; either for 528 or for EJUKEBOX. I think that it
might be simpler, however, to grep for EJUKEBOX, because one can grep
the source code immediately, while in case of 528 one has to map 528
to the corresponding macro first and then grep the source code for EJUKEBOX.
Overall %dE looks interesting.
-ss
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Uwe Kleine-König @ 2019-08-25 9:14 UTC (permalink / raw)
To: Andrew Morton
Cc: Jonathan Corbet, linux-doc, linux-kernel, linux-gpio,
Linus Walleij, Bartosz Golaszewski, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt
In-Reply-To: <20190824165829.7d330367992c62dab87f6652@linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 5632 bytes --]
Hello Andrew,
On Sat, Aug 24, 2019 at 04:58:29PM -0700, Andrew Morton wrote:
> (cc printk maintainers).
Ah, I wasn't aware there is something like them. Thanks
> On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> > pr_info("probing failed (%dE)\n", ret);
> >
> > expands to
> >
> > probing failed (EIO)
> >
> > if ret holds -EIO (or EIO). This introduces an array of error codes. If
> > the error code is missing, %dE falls back to %d and so prints the plain
> > number.
>
> Huh. I'm surprised we don't already have this. Seems that this will
> be applicable in a lot of places? Although we shouldn't go blindly
> converting everything in sight - that would risk breaking userspace
> which parses kernel strings.
Uah, even the kernel log is API? But I agree so far that this is merge
window material and shouldn't make it into v5.3 :-)
> Is it really necessary to handle the positive errnos? Does much kernel
> code actually do that (apart from kernel code which is buggy)?
I didn't check; probably not. But the whole positive range seems so
unused and interpreting EIO (and not only -EIO) as "EIO" seems straight
forward. But I don't feel strong either way.
> > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > ---
> > Hello
> >
> > there are many code sites that benefit from this. Just grep for
> > "(%d)" ...
>
> Yup. This observation shouldn't be below the "^---$" ;) An approximate
> grep|wc would be interesting.
I didn't check how many false positives there are using "(%d)", but I'd
use an a bit more elaborate expression for the commit log:
$ git grep '(%d)' | wc -l
7336
$ git grep -E '^\s*(printk|(kv?as|sn?|vs(c?n)?)printf|(kvm|dev|pr)_(emerg|alert|crit|err|warn(ing)?|notice|info|cont|devel|debug|dbg))\(.*(\(%d\)|: %d)\\n' | wc -l
9140
The latter matches several printk-variants emitting a string ending in
one of
'(%d)\n' (1839 matches)
': %d\n' (7301 matches)
. I would expect that many of the 7336 matches for '(%d)' that are not
matched by the longer expression are false negatives because the
function name is in the previous line. So I would estimate around 10k
strings that could benefit from %dE.
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
> > return buf;
> > }
> >
> > +#define ERRORCODE(x) { .str = #x, .err = x }
> > +
> > +static const struct {
> > + const char *str;
> > + int err;
> > +} errorcodes[] = {
>
> It's a bit of a hack, but an array of char*'s and a separate array of
> ushorts would save a bit of space.
Hmm, true. Currently we have 150 entries taking 150 * (sizeof(void *) *
2). Is it worth to think about the hacky solution to go down to 150 *
(sizeof(void *) + 2)?
For an ARM build bloat-o-meter reports (comparing linus/master to linus/master
+ my patch):
add/remove: 2/0 grow/shrink: 4/2 up/down: 1488/-8 (1480)
Function old new delta
errorcodes - 1200 +1200
errstr - 200 +200
vsnprintf 884 960 +76
set_precision 148 152 +4
resource_string 1380 1384 +4
flags_string 400 404 +4
num_to_str 288 284 -4
format_decode 1024 1020 -4
Total: Before=21686, After=23166, chg +6.82%
But that doesn't seem to include the size increase for all the added
strings which seems to be around another 1300 bytes.
> > + ERRORCODE(EPERM),
> > + ERRORCODE(ENOENT),
> > + ERRORCODE(ESRCH),
> >
> > ...
> >
> > +static noinline_for_stack
>
> Why this? I'm suspecting this will actually increase stack use?
I don't know what it does, just copied it from number() which is used
similarly.
> > +char *errstr(char *buf, char *end, unsigned long long num,
> > + struct printf_spec spec)
> > +{
> > + char *errname = NULL;
I missed a warning during my tests, there is a const missing in this
line.
> > + size_t errnamelen, copy;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> > + if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> > + errname = errorcodes[i].str;
> > + break;
> > + }
> > + }
> > +
> > + if (!errname) {
> > + /* fall back to ordinary number */
> > + return number(buf, end, num, spec);
> > + }
> > +
> > + copy = errnamelen = strlen(errname);
> > + if (copy > end - buf)
> > + copy = end - buf;
> > + buf = memcpy(buf, errname, copy);
> > +
> > + return buf + errnamelen;
> > +}
>
> OK, I guess `errstr' is an OK name for a static function
IMHO the name is very generic (which is bad), but it is in good company,
as there is also pointer() and number().
> and we can use this to add a new strerror() should the need arise.
In userspace the purpose of strerror is different. It would yield "I/O
error" not "EIO". So strerror using this array would only be a "strerror
light".
In my first prototype I even used %m instead of %dE, but as %m (in
glibc) doesn't consume an argument and produces the more verbose
variant, I changed my mind and went for %dE. (Also my patch has the
undocumented side effect that you can use %ldE if the error is held by a
long int. I didn't test that though.)
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v1 1/2] vsprintf: introduce %dE for error constants
From: Andrew Morton @ 2019-08-24 23:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Jonathan Corbet, linux-doc, linux-kernel, linux-gpio,
Linus Walleij, Bartosz Golaszewski, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt
In-Reply-To: <20190824233724.1775-1-uwe@kleine-koenig.org>
(cc printk maintainers).
On Sun, 25 Aug 2019 01:37:23 +0200 Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> pr_info("probing failed (%dE)\n", ret);
>
> expands to
>
> probing failed (EIO)
>
> if ret holds -EIO (or EIO). This introduces an array of error codes. If
> the error code is missing, %dE falls back to %d and so prints the plain
> number.
Huh. I'm surprised we don't already have this. Seems that this will
be applicable in a lot of places? Although we shouldn't go blindly
converting everything in sight - that would risk breaking userspace
which parses kernel strings.
Is it really necessary to handle the positive errnos? Does much kernel
code actually do that (apart from kernel code which is buggy)?
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello
>
> there are many code sites that benefit from this. Just grep for
> "(%d)" ...
Yup. This observation shouldn't be below the "^---$" ;) An approximate
grep|wc would be interesting.
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -533,6 +533,192 @@ char *number(char *buf, char *end, unsigned long long num,
> return buf;
> }
>
> +#define ERRORCODE(x) { .str = #x, .err = x }
> +
> +static const struct {
> + const char *str;
> + int err;
> +} errorcodes[] = {
It's a bit of a hack, but an array of char*'s and a separate array of
ushorts would save a bit of space.
> + ERRORCODE(EPERM),
> + ERRORCODE(ENOENT),
> + ERRORCODE(ESRCH),
>
> ...
>
> +static noinline_for_stack
Why this? I'm suspecting this will actually increase stack use?
> +char *errstr(char *buf, char *end, unsigned long long num,
> + struct printf_spec spec)
> +{
> + char *errname = NULL;
> + size_t errnamelen, copy;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(errorcodes); ++i) {
> + if (num == errorcodes[i].err || num == -errorcodes[i].err) {
> + errname = errorcodes[i].str;
> + break;
> + }
> + }
> +
> + if (!errname) {
> + /* fall back to ordinary number */
> + return number(buf, end, num, spec);
> + }
> +
> + copy = errnamelen = strlen(errname);
> + if (copy > end - buf)
> + copy = end - buf;
> + buf = memcpy(buf, errname, copy);
> +
> + return buf + errnamelen;
> +}
OK, I guess `errstr' is an OK name for a static function and we can use
this to add a new strerror() should the need arise.
>
> ...
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox