Re: [PATCH 1/9] ARM: boards: protonic-imx6: prtvt7 hardkey inputs are active low
Hello Ahmad,
On 2022-06-20 09:51, Ahmad Fatoum wrote:
Hello,
On 17.06.22 10:44, Marco Felsch wrote:
On 22-06-17, Sascha Hauer wrote:
Or, it is active low and your patch is correct :D
If they are, can we add a comment or _N suffix to the names?
Does barebox not have gpiod? The board code just should check if it is
active or not. Whatever active means in this case.
There is gpiod_get(), but there's also gpio-keys support. See
drivers/input/specialkeys.c for an example on how to register an input
notifier.
I'm interested in two keys pressed at the same time. When I implement a
notifyer like in specialkeys.c I'll have to check if the other key is pressed
at that time as well.
I noticed that the input subsystem is already maintaining a bitmask with keys
pressed. If I tap into that, I can implement it like so:
diff --git a/arch/arm/boards/protonic-imx6/board.c
b/arch/arm/boards/protonic-imx6/board.c
index 8436903fd8..1e1a22ae8e 100644
--- a/arch/arm/boards/protonic-imx6/board.c
+++ b/arch/arm/boards/protonic-imx6/board.c
@@ -8,11 +8,13 @@
#include
#include
#include
+#include
#include
#include
#include
#include
#include
+#include
#include
#include
#include
@@ -742,17 +744,13 @@ static int prt_imx6_init_kvg_yaco(struct prt_imx6_priv
*priv)
return prt_imx6_init_kvg_power(priv, PW_MODE_KVG_WITH_YACO);
}
-#define GPIO_KEY_F6 (0xe0 + 5)
-#define GPIO_KEY_CYCLE (0xe0 + 2)
-
static int prt_imx6_init_prtvt7(struct prt_imx6_priv *priv)
{
- /* This function relies heavely on the gpio-pca9539 driver */
+ unsigned long keys[BITS_TO_LONGS(KEY_CYCLEWINDOWS)];
- gpio_direction_input(GPIO_KEY_F6);
- gpio_direction_input(GPIO_KEY_CYCLE);
+ input_key_get_status(keys, KEY_CYCLEWINDOWS);
- if (gpio_get_value(GPIO_KEY_CYCLE) || gpio_get_value(GPIO_KEY_F6))
+ if (!(test_bit(KEY_CYCLEWINDOWS, keys) && test_bit(KEY_F6, keys)))
priv->no_usb_check = 1;
return 0;
- Robin
Re: move dma_ops to common code
Hello Antony,
On 16.06.22 09:53, Antony Pavlov wrote:
> Hi!
>
> In 0e885ce81d0e ('RISC-V: dma: support multiple dma_alloc_coherent backends')
> ,
> (see
> https://lore.barebox.org/barebox/20210619045055.779-10-a.fat...@pengutronix.de/)
> multiple dma_alloc_coherent backends was introduced for RISC-V.
>
> At the moment MIPS dma_alloc_coherent stuff is messy and I want to rework it.
> I can reuse some parts of 0e885ce81d0e by moving it to the common code.
> Any comments or suggestions?
I have been meaning to clean this dma API stuff up, so I find it very nice
that you are going to do it :)
Currently, we have two ways to influence this:
- dma_set_ops
- define static inline helpers in and #define macros, so the
generic
implementation isn't used
(see
https://lore.barebox.org/barebox/20220614091556.1018102-1-a.fat...@pengutronix.de/T/#t)
As first step, one could clean up the repetition in the files, e.g.
dma_alloc_coherent is nearly identical in a couple of places. Then as next step
we could have (just example names)
CONFIG_DMA_UNCACHED
The current !PBL and/or !MMU case
CONFIG_DMA_PER_ARCH
For those not using dma_set_ops
CONFIG_DMA_DYNAMIC_OPS
For those using dma_set_ops
This should allows us to get rid of the macros and then each arch could just
select the appropriate symbol.
What do you think?
Cheers,
Ahmad
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Boot hangs during sdhci_transfer_data_dma
Hi, Today I tried to run barebox with CONFIG_KEYBOARD_GPIO=y added to my config. and noticed my board hangs during boot. When I modify the probe function to run without registering the poller[1] it boots as expected. I started digging into the code to see how far the boot gets when I do register the poller. I found that Barebox hangs in a do/while loop in sdhci_transfer_data_dma[2]. The contents of the interrupt status (SDHCI_INT_STATUS) is 0 and stays that way forever trapping the process in the loop. Call stack: initcall -> barebox_of_populate state_probe drivers/misc/state.c state_new_from_nodecommon/state/state.c of_find_path_by_node drivers/of/of_path.c __of_find_path drivers/of/of_path.c device_detectdrivers/base/driver.c mci_detect_carddrivers/mci/mci-core.c mci_card_probe drivers/mci/mci-core.c mci_startupdrivers/mci/mci-core.c mci_startup_mmc drivers/mci/mci-core.c mmc_compare_ext_csds drivers/mci/mci-core.c mci_send_ext_csd drivers/mci/mci-core.c mci_send_cmd drivers/mci/mci-core.c esdhc_send_cmd drivers/mci/imx-esdhc.c __esdhc_send_cmd drivers/mci/imx-esdhc-common.c sdhci_transfer_data_dma drivers/mci/sdhci.c I'm not sure how this happens. It's not the first transfer taking place. I figured that mayby the poller[1] just adds some cpu load that opens up a window for this to occur. Maybe something else cleared the status register right before we entered the loop. Thats when I spotted this read/write construction[3]. It's executed right before we enter the do/while loop and (over)writes to the irq status register. I removed the line with the write command[3] and my board boots as expected. Why are we (over)writing the status register right after reading it? Other theories on how this could occur are also very welcome :) - Robin 1: https://git.pengutronix.de/cgit/barebox/tree/drivers/input/gpio_keys.c#n168 2: https://git.pengutronix.de/cgit/barebox/tree/drivers/mci/sdhci.c#n167 3: https://git.pengutronix.de/cgit/barebox/tree/drivers/mci/imx-esdhc-common.c#n179
Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m
Hi,
On 20.06.22 14:47, Teresa Remmet wrote:
> Am Montag, dem 20.06.2022 um 14:38 +0200 schrieb Ahmad Fatoum:
>> On 20.06.22 14:27, Teresa Remmet wrote:
>>> I have set the DDRC_ADDRMAP7 register manually in the RAM
>>> configuration
>>> in such a case. As I don't see a solution that fits for all. But
>>> would
>>> be happy for one. :)
>>
>> What would the 'neutral' value to write into this register be? zero
>> seems to not be it.
>
> it's
>
> 0xf0f for ADDRMAP7.
>
> Reference Manual says: "If set to 15, row address bit X is set to 0"
Do newer spreadsheets always generate ADDRMAP7 writes even if the
value is zero? If so, we could perhaps initialize it to 0xf0f before
running ddr_cfg_umctl2(). The DDRC seems to be in reset while the
registers are being written, so this might just work.
As 0 is a valid value, I am wondering if this snippet introduced with
42d45ef380c5 ("ARM: imx: Add imx8 support for SDRAM with two or more bank
groups")
is correct:
if (addrmap[8]) {
if (FIELD_GET(DDRC_ADDRMAP8_BG_B0, addrmap[8]) != 0b1)
banks++;
if (FIELD_GET(DDRC_ADDRMAP8_BG_B1, addrmap[8]) != 0b11)
banks++;
}
Thanks,
Ahmad
>
> Regards,
> Teresa
>
>
>>
>> Thanks,
>> Ahmad
>>
>>> Regards,
>>> Teresa
>>>
> ---
> arch/arm/mach-imx/esdctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
> imx/esdctl.c
> index 8dd0ddbbc965..b070ebc62a45 100644
> --- a/arch/arm/mach-imx/esdctl.c
> +++ b/arch/arm/mach-imx/esdctl.c
> @@ -488,7 +488,7 @@ static resource_size_t
> imx8m_ddrc_sdram_size(void __iomem *ddrc)
>
> return imx_ddrc_sdram_size(ddrc, addrmap,
> 12, ARRAY_AND_SIZE(col_b),
> -16, ARRAY_AND_SIZE(row_b),
> +18, ARRAY_AND_SIZE(row_b),
> reduced_adress_space, true);
> }
>
>>
>>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m
Am Montag, dem 20.06.2022 um 14:38 +0200 schrieb Ahmad Fatoum:
> Hello Teresa,
>
> On 20.06.22 14:27, Teresa Remmet wrote:
> > Hello Ahmad,
> >
> > Am Montag, dem 20.06.2022 um 14:02 +0200 schrieb Ahmad Fatoum:
> > > Hello,
> > >
> > > On 20.05.22 16:23, Teresa Remmet wrote:
> > > > As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
> > > > now for the row size calculation we need to increase row_max to
> > > > 18.
> > > >
> > > > For LPDDR4 this only works in combination with ram timings
> > > > created with recent configuration spreadsheet versions.
> > > > With older versions the register DDRC_ADDRMAP7 may not be set
> > > > and
> > > > calculation will lead to wrong results even with this patch.
> > > >
> > > > Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit
> > > > SDRAM
> > > > row size handle")
> > > > Signed-off-by: Teresa Remmet
> > >
> > > I have an out-of-tree i.MX8MM board with LPDDR4 that reported a
> > > correct size of 1G
> > > prior to this patch and now it reports 4G. DDRC_ADDRMAP7 is not
> > > explicitly initialized,
> > > but it's being read as zero, so the calculation seems to still be
> > > broken..
> >
> > yes, this is why I added the note to the commit message. The RAM
> > timings of the board you are using have been created with a old
> > version
> > of the spreadsheet ( < version 19 for i.MX8MM).
> > This issue is worked around when this patch is reverted as
> > DDRC_ADDRMAP7 is not taken into account. But calculating big ram
> > sizes
> > will probably not work then.
> >
> > I have set the DDRC_ADDRMAP7 register manually in the RAM
> > configuration
> > in such a case. As I don't see a solution that fits for all. But
> > would
> > be happy for one. :)
>
> What would the 'neutral' value to write into this register be? zero
> seems to not be it.
it's
0xf0f for ADDRMAP7.
Reference Manual says: "If set to 15, row address bit X is set to 0"
Regards,
Teresa
>
> Thanks,
> Ahmad
>
> > Regards,
> > Teresa
> >
> > > > ---
> > > > arch/arm/mach-imx/esdctl.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
> > > > imx/esdctl.c
> > > > index 8dd0ddbbc965..b070ebc62a45 100644
> > > > --- a/arch/arm/mach-imx/esdctl.c
> > > > +++ b/arch/arm/mach-imx/esdctl.c
> > > > @@ -488,7 +488,7 @@ static resource_size_t
> > > > imx8m_ddrc_sdram_size(void __iomem *ddrc)
> > > >
> > > > return imx_ddrc_sdram_size(ddrc, addrmap,
> > > >12, ARRAY_AND_SIZE(col_b),
> > > > - 16, ARRAY_AND_SIZE(row_b),
> > > > + 18, ARRAY_AND_SIZE(row_b),
> > > >reduced_adress_space, true);
> > > > }
> > > >
>
>
--
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m
Hello Teresa,
On 20.06.22 14:27, Teresa Remmet wrote:
> Hello Ahmad,
>
> Am Montag, dem 20.06.2022 um 14:02 +0200 schrieb Ahmad Fatoum:
>> Hello,
>>
>> On 20.05.22 16:23, Teresa Remmet wrote:
>>> As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
>>> now for the row size calculation we need to increase row_max to 18.
>>>
>>> For LPDDR4 this only works in combination with ram timings
>>> created with recent configuration spreadsheet versions.
>>> With older versions the register DDRC_ADDRMAP7 may not be set and
>>> calculation will lead to wrong results even with this patch.
>>>
>>> Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit SDRAM
>>> row size handle")
>>> Signed-off-by: Teresa Remmet
>>
>> I have an out-of-tree i.MX8MM board with LPDDR4 that reported a
>> correct size of 1G
>> prior to this patch and now it reports 4G. DDRC_ADDRMAP7 is not
>> explicitly initialized,
>> but it's being read as zero, so the calculation seems to still be
>> broken..
>
> yes, this is why I added the note to the commit message. The RAM
> timings of the board you are using have been created with a old version
> of the spreadsheet ( < version 19 for i.MX8MM).
> This issue is worked around when this patch is reverted as
> DDRC_ADDRMAP7 is not taken into account. But calculating big ram sizes
> will probably not work then.
>
> I have set the DDRC_ADDRMAP7 register manually in the RAM configuration
> in such a case. As I don't see a solution that fits for all. But would
> be happy for one. :)
What would the 'neutral' value to write into this register be? zero
seems to not be it.
Thanks,
Ahmad
>
> Regards,
> Teresa
>
>>
>>> ---
>>> arch/arm/mach-imx/esdctl.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
>>> imx/esdctl.c
>>> index 8dd0ddbbc965..b070ebc62a45 100644
>>> --- a/arch/arm/mach-imx/esdctl.c
>>> +++ b/arch/arm/mach-imx/esdctl.c
>>> @@ -488,7 +488,7 @@ static resource_size_t
>>> imx8m_ddrc_sdram_size(void __iomem *ddrc)
>>>
>>> return imx_ddrc_sdram_size(ddrc, addrmap,
>>>12, ARRAY_AND_SIZE(col_b),
>>> - 16, ARRAY_AND_SIZE(row_b),
>>> + 18, ARRAY_AND_SIZE(row_b),
>>>reduced_adress_space, true);
>>> }
>>>
>>
>>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m
Hello Ahmad,
Am Montag, dem 20.06.2022 um 14:02 +0200 schrieb Ahmad Fatoum:
> Hello,
>
> On 20.05.22 16:23, Teresa Remmet wrote:
> > As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
> > now for the row size calculation we need to increase row_max to 18.
> >
> > For LPDDR4 this only works in combination with ram timings
> > created with recent configuration spreadsheet versions.
> > With older versions the register DDRC_ADDRMAP7 may not be set and
> > calculation will lead to wrong results even with this patch.
> >
> > Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit SDRAM
> > row size handle")
> > Signed-off-by: Teresa Remmet
>
> I have an out-of-tree i.MX8MM board with LPDDR4 that reported a
> correct size of 1G
> prior to this patch and now it reports 4G. DDRC_ADDRMAP7 is not
> explicitly initialized,
> but it's being read as zero, so the calculation seems to still be
> broken..
yes, this is why I added the note to the commit message. The RAM
timings of the board you are using have been created with a old version
of the spreadsheet ( < version 19 for i.MX8MM).
This issue is worked around when this patch is reverted as
DDRC_ADDRMAP7 is not taken into account. But calculating big ram sizes
will probably not work then.
I have set the DDRC_ADDRMAP7 register manually in the RAM configuration
in such a case. As I don't see a solution that fits for all. But would
be happy for one. :)
Regards,
Teresa
>
> > ---
> > arch/arm/mach-imx/esdctl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
> > imx/esdctl.c
> > index 8dd0ddbbc965..b070ebc62a45 100644
> > --- a/arch/arm/mach-imx/esdctl.c
> > +++ b/arch/arm/mach-imx/esdctl.c
> > @@ -488,7 +488,7 @@ static resource_size_t
> > imx8m_ddrc_sdram_size(void __iomem *ddrc)
> >
> > return imx_ddrc_sdram_size(ddrc, addrmap,
> >12, ARRAY_AND_SIZE(col_b),
> > - 16, ARRAY_AND_SIZE(row_b),
> > + 18, ARRAY_AND_SIZE(row_b),
> >reduced_adress_space, true);
> > }
> >
>
>
--
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m
Hello,
On 20.05.22 16:23, Teresa Remmet wrote:
> As DDRC_ADDRMAP7_ROW_B16 and DDRC_ADDRMAP7_ROW_B17 are used
> now for the row size calculation we need to increase row_max to 18.
>
> For LPDDR4 this only works in combination with ram timings
> created with recent configuration spreadsheet versions.
> With older versions the register DDRC_ADDRMAP7 may not be set and
> calculation will lead to wrong results even with this patch.
>
> Fixes: dad2b5636bd8 ("ARM: imx: Add imx8 support for 18 bit SDRAM row size
> handle")
> Signed-off-by: Teresa Remmet
I have an out-of-tree i.MX8MM board with LPDDR4 that reported a correct size of
1G
prior to this patch and now it reports 4G. DDRC_ADDRMAP7 is not explicitly
initialized,
but it's being read as zero, so the calculation seems to still be broken..
> ---
> arch/arm/mach-imx/esdctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-imx/esdctl.c
> index 8dd0ddbbc965..b070ebc62a45 100644
> --- a/arch/arm/mach-imx/esdctl.c
> +++ b/arch/arm/mach-imx/esdctl.c
> @@ -488,7 +488,7 @@ static resource_size_t imx8m_ddrc_sdram_size(void __iomem
> *ddrc)
>
> return imx_ddrc_sdram_size(ddrc, addrmap,
> 12, ARRAY_AND_SIZE(col_b),
> -16, ARRAY_AND_SIZE(row_b),
> +18, ARRAY_AND_SIZE(row_b),
> reduced_adress_space, true);
> }
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH v2 3/3] treewide: Simplify setenv() calls
We now have pr_setenv() which is a setenv() variant that takes a
format string. Use it where appropriate.
Signed-off-by: Sascha Hauer
---
commands/clk.c | 10 +++---
commands/crc.c | 14 --
commands/hwclock.c | 4 +---
commands/loadb.c| 4 +---
commands/loads.c| 4 +---
common/bootsource.c | 8 ++--
common/menutree.c | 9 +
7 files changed, 13 insertions(+), 40 deletions(-)
diff --git a/commands/clk.c b/commands/clk.c
index dfbc7c988f..b1741b9da4 100644
--- a/commands/clk.c
+++ b/commands/clk.c
@@ -139,13 +139,9 @@ static int do_clk_get_rate(int argc, char *argv[])
rate = clk_get_rate(clk);
- if (variable_name) {
- char *t;
-
- t = basprintf("%lu", rate);
- setenv(variable_name, t);
- free(t);
- } else
+ if (variable_name)
+ pr_setenv(variable_name, "%lu", rate);
+ else
printf("%lu\n", rate);
return COMMAND_SUCCESS;
diff --git a/commands/crc.c b/commands/crc.c
index 80ecf7fe29..23ffd4360b 100644
--- a/commands/crc.c
+++ b/commands/crc.c
@@ -83,17 +83,11 @@ static int do_crc(int argc, char *argv[])
printf("CRC32 for %s 0x%08lx ... 0x%08lx ==> 0x%08lx",
filename, (ulong)start, (ulong)start + total - 1, crc);
- if (crcvarname) {
- char *crcstr = basprintf("0x%lx", crc);
- setenv(crcvarname, crcstr);
- kfree(crcstr);
- }
+ if (crcvarname)
+ pr_setenv(crcvarname, "0x%lx", crc);
- if (sizevarname) {
- char *sizestr = basprintf("0x%lx", total);
- setenv(sizevarname, sizestr);
- kfree(sizestr);
- }
+ if (sizevarname)
+ pr_setenv(sizevarname, "0x%lx", total);
#ifdef CONFIG_CMD_CRC_CMP
if (vfilename) {
diff --git a/commands/hwclock.c b/commands/hwclock.c
index abb0500e6a..c594e070ac 100644
--- a/commands/hwclock.c
+++ b/commands/hwclock.c
@@ -153,11 +153,9 @@ static int do_hwclock(int argc, char *argv[])
if (env_name) {
unsigned long time;
- char t[12];
rtc_tm_to_time(, );
- snprintf(t, 12, "%lu", time);
- setenv(env_name, t);
+ pr_setenv(env_name, "%lu", time);
} else {
printf("%s\n", time_str());
}
diff --git a/commands/loadb.c b/commands/loadb.c
index 17d3af84b5..7ab989f459 100644
--- a/commands/loadb.c
+++ b/commands/loadb.c
@@ -542,7 +542,6 @@ packet_error:
static ulong load_serial_bin(void)
{
int size, i;
- char buf[32];
/* Try to allocate the buffer we shall write to files */
write_buffer = malloc(MAX_WRITE_BUFFER);
@@ -576,8 +575,7 @@ static ulong load_serial_bin(void)
write_idx = 0;
}
printf("## Total Size = 0x%08x = %d Bytes\n", size, size);
- sprintf(buf, "%X", size);
- setenv("filesize", buf);
+ pr_setenv("filesize", "%X", size);
err_quit:
free(write_buffer);
diff --git a/commands/loads.c b/commands/loads.c
index 8260673c51..7c5df31251 100644
--- a/commands/loads.c
+++ b/commands/loads.c
@@ -65,7 +65,6 @@ static ulong load_serial(ulong offset)
int type; /* return code for record type
*/
ulong addr; /* load address from S-Record
*/
ulong size; /* number of bytes transferred
*/
- charbuf[32];
ulong store_addr;
ulong start_addr = ~0;
ulong end_addr = 0;
@@ -100,8 +99,7 @@ static ulong load_serial(ulong offset)
"## Total Size = 0x%08lX = %ld Bytes\n",
start_addr, end_addr, size, size
);
- sprintf(buf, "%lX", size);
- setenv("filesize", buf);
+ pr_setenv("filesize", "%lX", size);
return addr;
case SREC_START:
break;
diff --git a/common/bootsource.c b/common/bootsource.c
index 1f8d053a81..79dacfd1d0 100644
--- a/common/bootsource.c
+++ b/common/bootsource.c
@@ -113,16 +113,12 @@ void bootsource_set(enum bootsource src)
void bootsource_set_instance(int instance)
{
- char buf[32];
-
bootsource_instance = instance;
if (instance < 0)
- sprintf(buf, "unknown");
+ setenv("bootsource_instance","unknown");
else
- snprintf(buf, sizeof(buf), "%d", instance);
-
- setenv("bootsource_instance", buf);
+ pr_setenv("bootsource_instance", "%d", instance);
}
enum bootsource bootsource_get(void)
diff --git a/common/menutree.c b/common/menutree.c
index 7fa835a7fe..751350d754 100644
--- a/common/menutree.c
+++ b/common/menutree.c
@@ -34,14 +34,7 @@
[PATCH v2 1/3] bootchooser: rename pr_setenv() to bc_setenv()
We are about to introduce a generic function names pr_setenv(), so
rename the existent bootchooser specific function to bc_setenv().
For consistency, rename pr_getenv() accordingly.
Signed-off-by: Sascha Hauer
---
common/bootchooser.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/common/bootchooser.c b/common/bootchooser.c
index 75dfbc6166..eb3dda52ab 100644
--- a/common/bootchooser.c
+++ b/common/bootchooser.c
@@ -128,7 +128,7 @@ static void pr_target(struct bootchooser_target *target)
printf("disabled due to %s\n", reason);
}
-static int pr_setenv(struct bootchooser *bc, const char *fmt, ...)
+static int bc_setenv(struct bootchooser *bc, const char *fmt, ...)
{
va_list ap;
int ret = 0;
@@ -162,7 +162,7 @@ err:
return ret;
}
-static const char *pr_getenv(const char *fmt, ...)
+static const char *bc_getenv(const char *fmt, ...)
{
va_list ap;
char *str;
@@ -258,7 +258,7 @@ static struct bootchooser_target
*bootchooser_target_new(struct bootchooser *bc,
target->remaining_attempts = 0;
}
- val = pr_getenv("%s.boot", target->prefix);
+ val = bc_getenv("%s.boot", target->prefix);
if (!val)
val = target->name;
target->boot = xstrdup(val);
@@ -497,17 +497,17 @@ int bootchooser_save(struct bootchooser *bc)
int ret;
if (bc->last_chosen)
- pr_setenv(bc, "%s.last_chosen=%d", bc->state_prefix,
+ bc_setenv(bc, "%s.last_chosen=%d", bc->state_prefix,
bc->last_chosen->id);
list_for_each_entry(target, >targets, list) {
- ret = pr_setenv(bc, "%s.remaining_attempts=%d",
+ ret = bc_setenv(bc, "%s.remaining_attempts=%d",
target->state_prefix,
target->remaining_attempts);
if (ret)
return ret;
- ret = pr_setenv(bc, "%s.priority=%d",
+ ret = bc_setenv(bc, "%s.priority=%d",
target->state_prefix, target->priority);
if (ret)
return ret;
--
2.30.2
[PATCH v2 2/3] env: let setenv() take printf arguments
It's a common pattern to (ba)sprintf to a string and then call setenv()
with this string. Introduce pr_setenv() as a shortcut to simplify this
pattern.
Signed-off-by: Sascha Hauer
---
common/env.c | 32 ++--
include/environment.h | 7 +++
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/common/env.c b/common/env.c
index 05add63f62..141cd66046 100644
--- a/common/env.c
+++ b/common/env.c
@@ -244,13 +244,12 @@ static int dev_setenv(const char *name, const char *val)
/**
* setenv - set environment variables
- * @_name - Variable name
+ * @name - Variable name
* @value - the value to set, empty string not handled specially
*
* Returns 0 for success and a negative error code otherwise
* Use unsetenv() to unset.
*/
-
int setenv(const char *_name, const char *value)
{
char *name = strdup(_name);
@@ -277,6 +276,35 @@ out:
}
EXPORT_SYMBOL(setenv);
+/**
+ * pr_setenv - set environment variables
+ * @name - Variable name
+ * @fmt - the format string to use
+ *
+ * Returns 0 for success and a negative error code otherwise
+ * Use unsetenv() to unset.
+ */
+int pr_setenv(const char *name, const char *fmt, ...)
+{
+ va_list ap;
+ int ret = 0;
+ char *value;
+ int len;
+
+ va_start(ap, fmt);
+ len = vasprintf(, fmt, ap);
+ va_end(ap);
+
+ if (len < 0)
+ return -ENOMEM;
+
+ ret = setenv(name, value);
+ free(value);
+
+ return ret;
+}
+EXPORT_SYMBOL(setenv);
+
int export(const char *varname)
{
const char *val = getenv_raw(>local, varname);
diff --git a/include/environment.h b/include/environment.h
index 19e522cfb6..1557c3a1d7 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -32,6 +32,7 @@ char *var_name(struct variable_d *);
#ifdef CONFIG_ENVIRONMENT_VARIABLES
const char *getenv(const char *);
int setenv(const char *, const char *);
+int pr_setenv(const char *, const char *fmt, ...) __attribute__
((format(__printf__, 2, 3)));
void export_env_ull(const char *name, unsigned long long val);
int getenv_ull(const char *name, unsigned long long *val);
int getenv_ul(const char *name, unsigned long *val);
@@ -49,6 +50,12 @@ static inline int setenv(const char *var, const char *val)
return 0;
}
+static inline __attribute__ ((format(__printf__, 2, 3))) int pr_setenv(
+ const char *var, const char *fmt, ...)
+{
+ return 0;
+}
+
static inline void export_env_ull(const char *name, unsigned long long val) {}
static inline int getenv_ull(const char *name, unsigned long long *val)
--
2.30.2
Re: [PATCH] state: don't report error for -ENOMEDIUM
On Mon, Jun 20, 2022 at 09:19:36AM +0200, Ahmad Fatoum wrote:
> Since commit 863a2251393e ("state: make first boot less verbose"),
> state_load returns -ENOMEDIUM instead of -ENOENT if we detect
> a first load because all buckets are zero.
>
> This case is expected and shouldn't warrant an error message, so
> adjust callers appropriately.
>
> Signed-off-by: Ahmad Fatoum
> ---
> commands/state.c | 3 +++
> common/efi/payload/init.c | 2 +-
> 2 files changed, 4 insertions(+), 1 deletion(-)
Applied, thanks
Sascha
>
> diff --git a/commands/state.c b/commands/state.c
> index e7cb9902f71a..56ef93b19fa4 100644
> --- a/commands/state.c
> +++ b/commands/state.c
> @@ -53,6 +53,9 @@ static int do_state(int argc, char *argv[])
> ret = state_save(state);
> }
>
> + if (ret == -ENOMEDIUM)
> + ret = 0;
> +
> return ret;
> }
>
> diff --git a/common/efi/payload/init.c b/common/efi/payload/init.c
> index e2bddabc9a42..6976285fb3c3 100644
> --- a/common/efi/payload/init.c
> +++ b/common/efi/payload/init.c
> @@ -359,7 +359,7 @@ static int efi_late_init(void)
> return PTR_ERR(state);
>
> ret = state_load(state);
> - if (ret)
> + if (ret != -ENOMEDIUM)
> pr_warn("Failed to load persistent state, continuing
> with defaults, %d\n",
> ret);
>
> --
> 2.30.2
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/9] ARM: boards: protonic-imx6: prtvt7 hardkey inputs are active low
Hi Ahmand,
On 2022-06-20 09:51, Ahmad Fatoum wrote:
Hello,
On 17.06.22 10:44, Marco Felsch wrote:
On 22-06-17, Sascha Hauer wrote:
Or, it is active low and your patch is correct :D
If they are, can we add a comment or _N suffix to the names?
Does barebox not have gpiod? The board code just should check if it is
active or not. Whatever active means in this case.
There is gpiod_get(), but there's also gpio-keys support. See
drivers/input/specialkeys.c for an example on how to register an input
notifier.
I dont think I can use gpiod_get() due to the device tree format for
gpio-keys. I didn't know about the input_notifier framework. I'll check
if I can get it to work with that as-well.
Robin, did you test this works with barebox v2022.05.0?
I'd have assumed this to be broken by f349b662674e ("gpio: allocate dynamic
gpio numbers top down"). Especially, with deep probe, you can't and
shouldn't
depend on GPIO expanders numbering being fixed. If you use an input
notifier,
you should sidestep this issue altogether.
I just notiched that as well. I was just making some changes including the
feadback from this thread:
diff --git a/arch/arm/boards/protonic-imx6/board.c
b/arch/arm/boards/protonic-imx6/board.c
index cdbb8debe6..374ec11364 100644
--- a/arch/arm/boards/protonic-imx6/board.c
+++ b/arch/arm/boards/protonic-imx6/board.c
@@ -635,17 +635,24 @@ static int prt_imx6_init_kvg_yaco(struct prt_imx6_priv
*priv)
return prt_imx6_init_kvg_power(priv, PW_MODE_KVG_WITH_YACO);
}
-#define GPIO_KEY_F6 (0xe0 + 5)
-#define GPIO_KEY_CYCLE (0xe0 + 2)
-
static int prt_imx6_init_prtvt7(struct prt_imx6_priv *priv)
{
- /* This function relies heavely on the gpio-pca9539 driver */
+ int gpio_f6, gpio_cycle;
+ struct device_d *gpio_expander;
+
+ gpio_expander = get_device_by_name("pca95390");
+ if (!gpio_expander) {
+ dev_err(priv->dev, "Can't find pca9539 gpio expander\n");
+ return -ENODEV;
+ }
+
+ gpio_cycle = gpio_get_num(gpio_expander, 2);
+ gpio_request_one(gpio_cycle, GPIOF_ACTIVE_LOW | GPIOF_DIR_IN,
"Cycle");
- gpio_direction_input(GPIO_KEY_F6);
- gpio_direction_input(GPIO_KEY_CYCLE);
+ gpio_f6 = gpio_get_num(gpio_expander, 5);
+ gpio_request_one(gpio_f6, GPIOF_ACTIVE_LOW | GPIOF_DIR_IN, "F6");
- if (gpio_get_value(GPIO_KEY_CYCLE) && gpio_get_value(GPIO_KEY_F6))
+ if (!(gpio_is_active(gpio_cycle) && gpio_is_active(gpio_f6)))
priv->no_usb_check = 1;
return 0;
I'll make a proposal with the input_notifier aswell.
- Robin
Re: [PATCH] env: let setenv() take printf arguments
On Mon, Jun 20, 2022 at 09:59:00AM +0200, Ahmad Fatoum wrote: > Hello Sascha, > > On 20.06.22 09:47, Sascha Hauer wrote: > > On Mon, Jun 20, 2022 at 09:21:39AM +0200, Ahmad Fatoum wrote: > >> From: Sascha Hauer > >> > >> It's a common pattern to (ba)sprintf to a string and then call setenv() > >> with this string. Let setenv() take printf arguments to make that > >> easier. To avoid the overhead that goes with changing other callers > >> to using setenv(var, "%s", val) to avoid security implications (and > >> GCC warnings), fallback to the non-formatted version when there are > >> only two arguments. > >> > >> Signed-off-by: Sascha Hauer > >> [afa: fall back to non-formatted version on old two arg version] > >> Signed-off-by: Ahmad Fatoum > >> --- > >> Thoughts? > > > > While I'm impressed by this macro I don't like this very much. My desire > > was to simplify things, now with this patch I'm no longer sure I reached > > that goal. > > Usage _is_ simpler. Declaration indeed looks a bit odd, but ¯\_(ツ)_/¯ > > > > > Alternatively we could > > > > a) Drop the original patch > > b) Replace the problematic places with setenv(foo, "%s", > > not_a_string_literal); > > c) Pass -Wno-format-security, The Kernel does this for over a decade. > > Then it probably needs to be revisited there then. > > > My vote is c) > > I am not fine with c). We don't sanitize for % in environment variable values > and ignoring the warning has very clear security implications. Ok, good point. Then there's of course d) keep setenv like it was before and introduce pr_setenv(const char *_name, const char *fmt, ...) Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] env: let setenv() take printf arguments
Hello Sascha,
On 20.06.22 09:47, Sascha Hauer wrote:
> On Mon, Jun 20, 2022 at 09:21:39AM +0200, Ahmad Fatoum wrote:
>> From: Sascha Hauer
>>
>> It's a common pattern to (ba)sprintf to a string and then call setenv()
>> with this string. Let setenv() take printf arguments to make that
>> easier. To avoid the overhead that goes with changing other callers
>> to using setenv(var, "%s", val) to avoid security implications (and
>> GCC warnings), fallback to the non-formatted version when there are
>> only two arguments.
>>
>> Signed-off-by: Sascha Hauer
>> [afa: fall back to non-formatted version on old two arg version]
>> Signed-off-by: Ahmad Fatoum
>> ---
>> Thoughts?
>
> While I'm impressed by this macro I don't like this very much. My desire
> was to simplify things, now with this patch I'm no longer sure I reached
> that goal.
Usage _is_ simpler. Declaration indeed looks a bit odd, but ¯\_(ツ)_/¯
>
> Alternatively we could
>
> a) Drop the original patch
> b) Replace the problematic places with setenv(foo, "%s",
> not_a_string_literal);
> c) Pass -Wno-format-security, The Kernel does this for over a decade.
Then it probably needs to be revisited there then.
> My vote is c)
I am not fine with c). We don't sanitize for % in environment variable values
and ignoring the warning has very clear security implications.
Cheers,
Ahmad
>
> Sascha
>
>> ---
>> common/env.c | 37 +
>> include/environment.h | 19 +--
>> include/linux/kernel.h | 12
>> 3 files changed, 62 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/env.c b/common/env.c
>> index 05add63f625c..c36f6846ee21 100644
>> --- a/common/env.c
>> +++ b/common/env.c
>> @@ -243,15 +243,15 @@ static int dev_setenv(const char *name, const char
>> *val)
>> }
>>
>> /**
>> - * setenv - set environment variables
>> + * __setenv_str - set environment variables
>> * @_name - Variable name
>> * @value - the value to set, empty string not handled specially
>> *
>> * Returns 0 for success and a negative error code otherwise
>> - * Use unsetenv() to unset.
>> + * Use unsetenv() to unset. Don't use directly, use setenv()
>> */
>>
>> -int setenv(const char *_name, const char *value)
>> +int __setenv_str(const char *_name, const char *value)
>> {
>> char *name = strdup(_name);
>> int ret = 0;
>> @@ -275,7 +275,36 @@ out:
>>
>> return ret;
>> }
>> -EXPORT_SYMBOL(setenv);
>> +EXPORT_SYMBOL(__setenv_str);
>> +
>> +/**
>> + * __setenv_fmt - set environment variables
>> + * @name - Variable name
>> + * @fmt - format string describing how to format arguments to come
>> + *
>> + * Returns 0 for success and a negative error code otherwise
>> + * Use unsetenv() to unset. Don't use directly, use setenv()
>> + */
>> +
>> +int __setenv_fmt(const char *name, const char *fmt, ...)
>> +{
>> +va_list ap;
>> +int ret;
>> +char *value;
>> +
>> +va_start(ap, fmt);
>> +ret = vasprintf(, fmt, ap);
>> +va_end(ap);
>> +
>> +if (ret < 0)
>> +return ret;
>> +
>> +ret = __setenv_str(name, value);
>> +
>> +free(value);
>> +return ret;
>> +}
>> +EXPORT_SYMBOL(__setenv_fmt);
>>
>> int export(const char *varname)
>> {
>> diff --git a/include/environment.h b/include/environment.h
>> index 19e522cfb6b4..e5b9a9da3167 100644
>> --- a/include/environment.h
>> +++ b/include/environment.h
>> @@ -7,6 +7,7 @@
>> #ifndef _ENVIRONMENT_H_
>> #define _ENVIRONMENT_H_
>>
>> +#include
>> #include
>> #include
>>
>> @@ -31,7 +32,8 @@ char *var_name(struct variable_d *);
>>
>> #ifdef CONFIG_ENVIRONMENT_VARIABLES
>> const char *getenv(const char *);
>> -int setenv(const char *, const char *);
>> +int __setenv_str(const char *, const char *val);
>> +int __setenv_fmt(const char *, const char *fmt, ...) __printf(2, 3);
>> void export_env_ull(const char *name, unsigned long long val);
>> int getenv_ull(const char *name, unsigned long long *val);
>> int getenv_ul(const char *name, unsigned long *val);
>> @@ -44,7 +46,13 @@ static inline char *getenv(const char *var)
>> return NULL;
>> }
>>
>> -static inline int setenv(const char *var, const char *val)
>> +static inline int __setenv_str(const char *var, const char *val)
>> +{
>> +return 0;
>> +}
>> +
>> +static inline __printf(2, 3) int __setenv_fmt(
>> +const char *var, const char *fmt, ...)
>> {
>> return 0;
>> }
>> @@ -82,6 +90,13 @@ static inline const char *getenv_nonempty(const char *var)
>> }
>> #endif
>>
>> +/*
>> + * avoid the varargs overhead when using a fixed string
>> + */
>> +#undef setenv
>> +#define setenv(args...) \
>> +__optionally_variadic2(__setenv_str, __setenv_fmt, args)
>> +
>> int env_pop_context(void);
>> int env_push_context(void);
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 4483d33e65bb..ebae8f666cf6 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -7,6
Re: [PATCH 1/9] ARM: boards: protonic-imx6: prtvt7 hardkey inputs are active low
Hello,
On 17.06.22 10:44, Marco Felsch wrote:
> On 22-06-17, Sascha Hauer wrote:
>>> Or, it is active low and your patch is correct :D
>>
>> If they are, can we add a comment or _N suffix to the names?
>
> Does barebox not have gpiod? The board code just should check if it is
> active or not. Whatever active means in this case.
There is gpiod_get(), but there's also gpio-keys support. See
drivers/input/specialkeys.c for an example on how to register an input
notifier.
Robin, did you test this works with barebox v2022.05.0?
I'd have assumed this to be broken by f349b662674e ("gpio: allocate dynamic
gpio numbers top down"). Especially, with deep probe, you can't and shouldn't
depend on GPIO expanders numbering being fixed. If you use an input notifier,
you should sidestep this issue altogether.
Cheers,
Ahmad
>
> Regards,
> Marco
>
>>
>> Sascha
>>
>> --
>> Pengutronix e.K. | |
>> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
>> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
>> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
>>
>>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] env: let setenv() take printf arguments
On Mon, Jun 20, 2022 at 09:21:39AM +0200, Ahmad Fatoum wrote:
> From: Sascha Hauer
>
> It's a common pattern to (ba)sprintf to a string and then call setenv()
> with this string. Let setenv() take printf arguments to make that
> easier. To avoid the overhead that goes with changing other callers
> to using setenv(var, "%s", val) to avoid security implications (and
> GCC warnings), fallback to the non-formatted version when there are
> only two arguments.
>
> Signed-off-by: Sascha Hauer
> [afa: fall back to non-formatted version on old two arg version]
> Signed-off-by: Ahmad Fatoum
> ---
> Thoughts?
While I'm impressed by this macro I don't like this very much. My desire
was to simplify things, now with this patch I'm no longer sure I reached
that goal.
Alternatively we could
a) Drop the original patch
b) Replace the problematic places with setenv(foo, "%s", not_a_string_literal);
c) Pass -Wno-format-security, The Kernel does this for over a decade.
My vote is c)
Sascha
> ---
> common/env.c | 37 +
> include/environment.h | 19 +--
> include/linux/kernel.h | 12
> 3 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/common/env.c b/common/env.c
> index 05add63f625c..c36f6846ee21 100644
> --- a/common/env.c
> +++ b/common/env.c
> @@ -243,15 +243,15 @@ static int dev_setenv(const char *name, const char *val)
> }
>
> /**
> - * setenv - set environment variables
> + * __setenv_str - set environment variables
> * @_name - Variable name
> * @value - the value to set, empty string not handled specially
> *
> * Returns 0 for success and a negative error code otherwise
> - * Use unsetenv() to unset.
> + * Use unsetenv() to unset. Don't use directly, use setenv()
> */
>
> -int setenv(const char *_name, const char *value)
> +int __setenv_str(const char *_name, const char *value)
> {
> char *name = strdup(_name);
> int ret = 0;
> @@ -275,7 +275,36 @@ out:
>
> return ret;
> }
> -EXPORT_SYMBOL(setenv);
> +EXPORT_SYMBOL(__setenv_str);
> +
> +/**
> + * __setenv_fmt - set environment variables
> + * @name - Variable name
> + * @fmt - format string describing how to format arguments to come
> + *
> + * Returns 0 for success and a negative error code otherwise
> + * Use unsetenv() to unset. Don't use directly, use setenv()
> + */
> +
> +int __setenv_fmt(const char *name, const char *fmt, ...)
> +{
> + va_list ap;
> + int ret;
> + char *value;
> +
> + va_start(ap, fmt);
> + ret = vasprintf(, fmt, ap);
> + va_end(ap);
> +
> + if (ret < 0)
> + return ret;
> +
> + ret = __setenv_str(name, value);
> +
> + free(value);
> + return ret;
> +}
> +EXPORT_SYMBOL(__setenv_fmt);
>
> int export(const char *varname)
> {
> diff --git a/include/environment.h b/include/environment.h
> index 19e522cfb6b4..e5b9a9da3167 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -7,6 +7,7 @@
> #ifndef _ENVIRONMENT_H_
> #define _ENVIRONMENT_H_
>
> +#include
> #include
> #include
>
> @@ -31,7 +32,8 @@ char *var_name(struct variable_d *);
>
> #ifdef CONFIG_ENVIRONMENT_VARIABLES
> const char *getenv(const char *);
> -int setenv(const char *, const char *);
> +int __setenv_str(const char *, const char *val);
> +int __setenv_fmt(const char *, const char *fmt, ...) __printf(2, 3);
> void export_env_ull(const char *name, unsigned long long val);
> int getenv_ull(const char *name, unsigned long long *val);
> int getenv_ul(const char *name, unsigned long *val);
> @@ -44,7 +46,13 @@ static inline char *getenv(const char *var)
> return NULL;
> }
>
> -static inline int setenv(const char *var, const char *val)
> +static inline int __setenv_str(const char *var, const char *val)
> +{
> + return 0;
> +}
> +
> +static inline __printf(2, 3) int __setenv_fmt(
> + const char *var, const char *fmt, ...)
> {
> return 0;
> }
> @@ -82,6 +90,13 @@ static inline const char *getenv_nonempty(const char *var)
> }
> #endif
>
> +/*
> + * avoid the varargs overhead when using a fixed string
> + */
> +#undef setenv
> +#define setenv(args...) \
> + __optionally_variadic2(__setenv_str, __setenv_fmt, args)
> +
> int env_pop_context(void);
> int env_push_context(void);
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 4483d33e65bb..ebae8f666cf6 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -7,6 +7,7 @@
> #include
> #include
> #include
> +#include
>
> #define ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a) - 1)
> #define ALIGN_DOWN(x, a) ALIGN((x) - ((a) - 1), (a))
> @@ -17,6 +18,17 @@
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> #define ARRAY_AND_SIZE(x)(x), ARRAY_SIZE(x)
>
> +/*
> + * Call func_variadic, when more than 2 arguments and func_fixed otherwise
> + */
> +#define __optionally_variadic2(func_fixed,
[PATCH 2/2] test: self: include new tests in CONFIG_SELFTEST_ENABLE_ALL
We have gained some tests, since last touching this symbol, so update it. Signed-off-by: Ahmad Fatoum --- test/self/Kconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/self/Kconfig b/test/self/Kconfig index a5eac85646b7..680196a4fe29 100644 --- a/test/self/Kconfig +++ b/test/self/Kconfig @@ -28,7 +28,9 @@ config SELFTEST_AUTORUN config SELFTEST_ENABLE_ALL bool "Enable all self-tests" select SELFTEST_PRINTF + select SELFTEST_MALLOC select SELFTEST_PROGRESS_NOTIFIER + select SELFTEST_OF_MANIPULATION select SELFTEST_ENVIRONMENT_VARIABLES if ENVIRONMENT_VARIABLES help Selects all self-tests compatible with current configuration -- 2.30.2
[PATCH 1/2] test: self: add simple environment variable test
setenv can now accept a format string. Add some tests for the old and
new usage.
Signed-off-by: Ahmad Fatoum
---
test/self/Kconfig | 4 +++
test/self/Makefile | 1 +
test/self/envvar.c | 82 ++
3 files changed, 87 insertions(+)
create mode 100644 test/self/envvar.c
diff --git a/test/self/Kconfig b/test/self/Kconfig
index cf11efe54486..a5eac85646b7 100644
--- a/test/self/Kconfig
+++ b/test/self/Kconfig
@@ -29,6 +29,7 @@ config SELFTEST_ENABLE_ALL
bool "Enable all self-tests"
select SELFTEST_PRINTF
select SELFTEST_PROGRESS_NOTIFIER
+ select SELFTEST_ENVIRONMENT_VARIABLES if ENVIRONMENT_VARIABLES
help
Selects all self-tests compatible with current configuration
@@ -51,4 +52,7 @@ config SELFTEST_OF_MANIPULATION
config SELFTEST_PROGRESS_NOTIFIER
bool "progress notifier selftest"
+config SELFTEST_ENVIRONMENT_VARIABLES
+ bool "environment variable selftest"
+
endif
diff --git a/test/self/Makefile b/test/self/Makefile
index 65d01596b806..ca9f9c34d1e5 100644
--- a/test/self/Makefile
+++ b/test/self/Makefile
@@ -5,3 +5,4 @@ obj-$(CONFIG_SELFTEST_MALLOC) += malloc.o
obj-$(CONFIG_SELFTEST_PRINTF) += printf.o
obj-$(CONFIG_SELFTEST_PROGRESS_NOTIFIER) += progress-notifier.o
obj-$(CONFIG_SELFTEST_OF_MANIPULATION) += of_manipulation.o
of_manipulation.dtb.o
+obj-$(CONFIG_SELFTEST_ENVIRONMENT_VARIABLES) += envvar.o
diff --git a/test/self/envvar.c b/test/self/envvar.c
new file mode 100644
index ..a686c6c2e206
--- /dev/null
+++ b/test/self/envvar.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+
+BSELFTEST_GLOBALS();
+
+static int strequal(const char *a, const char *b)
+{
+ if (!a || !b)
+ return a == b;
+
+ return !strcmp(a, b);
+}
+
+static void __expect_getenv(const char *var, const char *expect,
+ const char *func, int line)
+{
+ const char *val;
+
+ total_tests++;
+
+ val = getenv(var);
+ if (!IS_ENABLED(CONFIG_ENVIRONMENT_VARIABLES)) {
+ if (val == NULL) {
+ skipped_tests++;
+ return;
+ }
+ }
+
+ if (!strequal(val, expect)) {
+ failed_tests++;
+ printf("%s:%d: failure: getenv(%s) == \"%s\", but \"%s\"
expected\n",
+ func, line, var, val ?: "", expect ?: "");
+ }
+}
+
+#define expect_getenv(v, e) __expect_getenv(v, e, __func__, __LINE__)
+
+static void test_envvar(void)
+{
+
+ if (IS_ENABLED(CONFIG_ENVIRONMENT_VARIABLES))
+ pr_info("built without environment variable support: Skipping
tests\n");
+
+ expect_getenv("__TEST_VAR1", NULL);
+
+ setenv("__TEST_VAR1", "VALUE1");
+
+ expect_getenv("__TEST_VAR1", "VALUE1");
+
+ unsetenv("__TEST_VAR1");
+
+ expect_getenv("__TEST_VAR1", NULL);
+
+ setenv("__TEST_VAR1", "VALUE1");
+
+ expect_getenv("__TEST_VAR1", "VALUE1");
+
+ setenv("__TEST_VAR1", "VALUE2");
+
+ expect_getenv("__TEST_VAR1", "VALUE2");
+
+ setenv("__TEST_VAR1", "1337");
+
+ expect_getenv("__TEST_VAR1", "1337");
+
+ setenv("__TEST_VAR1", "0x%s", "1337");
+
+ expect_getenv("__TEST_VAR1", "0x1337");
+
+ setenv("__TEST_VAR1", "%ux1%c%x", 0, '3', 0x37);
+
+ expect_getenv("__TEST_VAR1", "0x1337");
+
+ unsetenv("__TEST_VAR1");
+}
+bselftest(core, test_envvar);
--
2.30.2
Re: [PATCH 1/9] ARM: boards: protonic-imx6: prtvt7 hardkey inputs are active low
On Mon, Jun 20, 2022 at 08:48:32AM +0200, Robin van der Gracht wrote: > On 2022-06-16 18:38, Oleksij Rempel wrote: > > Am 16.06.22 um 18:28 schrieb Oleksij Rempel: > > > Hi Robin, > > > > > > On Thu, Jun 16, 2022 at 03:11:06PM +0200, Robin van der Gracht wrote: > > > > The usb check needs to be skipped unless both keys are pressed > > > > simultaneously. > > > > > > > > Signed-off-by: Robin van der Gracht > > > > --- > > > > arch/arm/boards/protonic-imx6/board.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm/boards/protonic-imx6/board.c > > > > b/arch/arm/boards/protonic-imx6/board.c > > > > index cdbb8debe6..8f8a0c745e 100644 > > > > --- a/arch/arm/boards/protonic-imx6/board.c > > > > +++ b/arch/arm/boards/protonic-imx6/board.c > > > > @@ -645,7 +645,7 @@ static int prt_imx6_init_prtvt7(struct > > > > prt_imx6_priv *priv) > > > > gpio_direction_input(GPIO_KEY_F6); > > > > gpio_direction_input(GPIO_KEY_CYCLE); > > > > > > > > - if (gpio_get_value(GPIO_KEY_CYCLE) && > > > > gpio_get_value(GPIO_KEY_F6)) > > > > + if (gpio_get_value(GPIO_KEY_CYCLE) || > > > > gpio_get_value(GPIO_KEY_F6)) > > > > priv->no_usb_check = 1; > > > > > > Hm, you probably wont: > > > if (!(gpio_get_value(GPIO_KEY_CYCLE) && gpio_get_value(GPIO_KEY_F6))) > > > > > > otherwise usb check will be always skipped. > > > > Or, it is active low and your patch is correct :D > > They are active low. I mentoned that in the patch subject ;) > I want both keys pressed simultaniously as a requirement for the usb check > because it adds a delay (autoboot_timeout) to the boot process. Now you can see how important it is to have all needed info in the code, not in the patch subject :) Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] env: let setenv() take printf arguments
From: Sascha Hauer
It's a common pattern to (ba)sprintf to a string and then call setenv()
with this string. Let setenv() take printf arguments to make that
easier. To avoid the overhead that goes with changing other callers
to using setenv(var, "%s", val) to avoid security implications (and
GCC warnings), fallback to the non-formatted version when there are
only two arguments.
Signed-off-by: Sascha Hauer
[afa: fall back to non-formatted version on old two arg version]
Signed-off-by: Ahmad Fatoum
---
Thoughts?
---
common/env.c | 37 +
include/environment.h | 19 +--
include/linux/kernel.h | 12
3 files changed, 62 insertions(+), 6 deletions(-)
diff --git a/common/env.c b/common/env.c
index 05add63f625c..c36f6846ee21 100644
--- a/common/env.c
+++ b/common/env.c
@@ -243,15 +243,15 @@ static int dev_setenv(const char *name, const char *val)
}
/**
- * setenv - set environment variables
+ * __setenv_str - set environment variables
* @_name - Variable name
* @value - the value to set, empty string not handled specially
*
* Returns 0 for success and a negative error code otherwise
- * Use unsetenv() to unset.
+ * Use unsetenv() to unset. Don't use directly, use setenv()
*/
-int setenv(const char *_name, const char *value)
+int __setenv_str(const char *_name, const char *value)
{
char *name = strdup(_name);
int ret = 0;
@@ -275,7 +275,36 @@ out:
return ret;
}
-EXPORT_SYMBOL(setenv);
+EXPORT_SYMBOL(__setenv_str);
+
+/**
+ * __setenv_fmt - set environment variables
+ * @name - Variable name
+ * @fmt - format string describing how to format arguments to come
+ *
+ * Returns 0 for success and a negative error code otherwise
+ * Use unsetenv() to unset. Don't use directly, use setenv()
+ */
+
+int __setenv_fmt(const char *name, const char *fmt, ...)
+{
+ va_list ap;
+ int ret;
+ char *value;
+
+ va_start(ap, fmt);
+ ret = vasprintf(, fmt, ap);
+ va_end(ap);
+
+ if (ret < 0)
+ return ret;
+
+ ret = __setenv_str(name, value);
+
+ free(value);
+ return ret;
+}
+EXPORT_SYMBOL(__setenv_fmt);
int export(const char *varname)
{
diff --git a/include/environment.h b/include/environment.h
index 19e522cfb6b4..e5b9a9da3167 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -7,6 +7,7 @@
#ifndef _ENVIRONMENT_H_
#define _ENVIRONMENT_H_
+#include
#include
#include
@@ -31,7 +32,8 @@ char *var_name(struct variable_d *);
#ifdef CONFIG_ENVIRONMENT_VARIABLES
const char *getenv(const char *);
-int setenv(const char *, const char *);
+int __setenv_str(const char *, const char *val);
+int __setenv_fmt(const char *, const char *fmt, ...) __printf(2, 3);
void export_env_ull(const char *name, unsigned long long val);
int getenv_ull(const char *name, unsigned long long *val);
int getenv_ul(const char *name, unsigned long *val);
@@ -44,7 +46,13 @@ static inline char *getenv(const char *var)
return NULL;
}
-static inline int setenv(const char *var, const char *val)
+static inline int __setenv_str(const char *var, const char *val)
+{
+ return 0;
+}
+
+static inline __printf(2, 3) int __setenv_fmt(
+ const char *var, const char *fmt, ...)
{
return 0;
}
@@ -82,6 +90,13 @@ static inline const char *getenv_nonempty(const char *var)
}
#endif
+/*
+ * avoid the varargs overhead when using a fixed string
+ */
+#undef setenv
+#define setenv(args...) \
+ __optionally_variadic2(__setenv_str, __setenv_fmt, args)
+
int env_pop_context(void);
int env_push_context(void);
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 4483d33e65bb..ebae8f666cf6 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
#define ALIGN(x, a)__ALIGN_MASK(x, (typeof(x))(a) - 1)
#define ALIGN_DOWN(x, a) ALIGN((x) - ((a) - 1), (a))
@@ -17,6 +18,17 @@
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
#define ARRAY_AND_SIZE(x) (x), ARRAY_SIZE(x)
+/*
+ * Call func_variadic, when more than 2 arguments and func_fixed otherwise
+ */
+#define __optionally_variadic2(func_fixed, func_variadic, arg1, arg2, ...) ({ \
+ char ___STR[] = __stringify((__VA_ARGS__)); \
+ sizeof(___STR) > 3 ? \
+ func_variadic(arg1, arg2, ##__VA_ARGS__) \
+ :\
+ func_fixed(arg1, arg2); \
+ })
+
/*
* This looks more complex than it should be. But we need to
* get the type for the ~ right in round_down (it needs to be
--
2.30.2
[PATCH] state: don't report error for -ENOMEDIUM
Since commit 863a2251393e ("state: make first boot less verbose"),
state_load returns -ENOMEDIUM instead of -ENOENT if we detect
a first load because all buckets are zero.
This case is expected and shouldn't warrant an error message, so
adjust callers appropriately.
Signed-off-by: Ahmad Fatoum
---
commands/state.c | 3 +++
common/efi/payload/init.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/commands/state.c b/commands/state.c
index e7cb9902f71a..56ef93b19fa4 100644
--- a/commands/state.c
+++ b/commands/state.c
@@ -53,6 +53,9 @@ static int do_state(int argc, char *argv[])
ret = state_save(state);
}
+ if (ret == -ENOMEDIUM)
+ ret = 0;
+
return ret;
}
diff --git a/common/efi/payload/init.c b/common/efi/payload/init.c
index e2bddabc9a42..6976285fb3c3 100644
--- a/common/efi/payload/init.c
+++ b/common/efi/payload/init.c
@@ -359,7 +359,7 @@ static int efi_late_init(void)
return PTR_ERR(state);
ret = state_load(state);
- if (ret)
+ if (ret != -ENOMEDIUM)
pr_warn("Failed to load persistent state, continuing
with defaults, %d\n",
ret);
--
2.30.2
Re: [PATCH 1/9] ARM: boards: protonic-imx6: prtvt7 hardkey inputs are active low
On 2022-06-16 18:38, Oleksij Rempel wrote: Am 16.06.22 um 18:28 schrieb Oleksij Rempel: Hi Robin, On Thu, Jun 16, 2022 at 03:11:06PM +0200, Robin van der Gracht wrote: The usb check needs to be skipped unless both keys are pressed simultaneously. Signed-off-by: Robin van der Gracht --- arch/arm/boards/protonic-imx6/board.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/boards/protonic-imx6/board.c b/arch/arm/boards/protonic-imx6/board.c index cdbb8debe6..8f8a0c745e 100644 --- a/arch/arm/boards/protonic-imx6/board.c +++ b/arch/arm/boards/protonic-imx6/board.c @@ -645,7 +645,7 @@ static int prt_imx6_init_prtvt7(struct prt_imx6_priv *priv) gpio_direction_input(GPIO_KEY_F6); gpio_direction_input(GPIO_KEY_CYCLE); - if (gpio_get_value(GPIO_KEY_CYCLE) && gpio_get_value(GPIO_KEY_F6)) + if (gpio_get_value(GPIO_KEY_CYCLE) || gpio_get_value(GPIO_KEY_F6)) priv->no_usb_check = 1; Hm, you probably wont: if (!(gpio_get_value(GPIO_KEY_CYCLE) && gpio_get_value(GPIO_KEY_F6))) otherwise usb check will be always skipped. Or, it is active low and your patch is correct :D They are active low. I mentoned that in the patch subject ;) I want both keys pressed simultaniously as a requirement for the usb check because it adds a delay (autoboot_timeout) to the boot process. - Robin
Re: [PATCH 2/9] ARM: boards: protonic-imx6: Update comment to match a recent code update
On 2022-06-17 09:00, Sascha Hauer wrote:
On Thu, Jun 16, 2022 at 03:11:07PM +0200, Robin van der Gracht wrote:
Fixes: 1e817b64bc45968320597344e6b05463edd9c5e5
We don't have this commit in mainline, could you reference the
corresponding upstream commit instead? Please write as
aabbccddeeff ("subject")
I must have mixed those up. The commit that my commit amends:
commit 8d4698e793d823590f050b36b26f67f73d5f9e85
Author: Oleksij Rempel
Date: Mon Mar 28 14:09:56 2022 +0200
ARM: boards: protonic-imx6: fix file system access warning
We should not access a file system from the poller. So, do it from the
worker. This patch will fix warning on FS access for Protonic board
code.
Signed-off-by: Oleksij Rempel
Link:
https://lore.barebox.org/20220328120956.2402132-1-o.rem...@pengutronix.de
Signed-off-by: Sascha Hauer
