Re: [PATCH] efi_loader: capsule: detect possible ESP device path
_file_system_handle));
> + for (index = 0; index < number_handles; index++) {
> +
> EFI_CALL(efi_handle_protocol(simple_file_system_handle[index],
> + _guid_device_path,
> + (void **)_dp));
> +
> + log_debug("Search ESP %pD\n", temp_dp);
> + temp_size = efi_dp_size(temp_dp) - sizeof(struct
> efi_device_path);
> + if (size <= temp_size && !memcmp(temp_dp, boot_dev,
> size)) {
> + if (device_is_present_and_system_part(temp_dp))
> {
You could combine these conditions:
if (size <= temp_size &&
memcmp(temp_dp, boot_dev, size) == 0 &&
device_is_present_and_system_part(temp_dp)) {
efi_free_pool(simple_file_system_handle);
return temp_dp;
}
> +
> efi_free_pool(simple_file_system_handle);
> + return temp_dp;
> + }
> + }
> + }
> +
> + if (simple_file_system_handle)
> + efi_free_pool(simple_file_system_handle);
> + }
> +
> + return full_path;
> +}
> +
> /**
> * find_boot_device - identify the boot device
> *
> @@ -938,6 +1037,7 @@ static efi_status_t find_boot_device(void)
> int i, num;
> struct efi_simple_file_system_protocol *volume;
> struct efi_device_path *boot_dev = NULL;
> + struct efi_device_path *full_path = NULL;
No need to initialize this to NULL, it disables static checker warnings
for uninitialized variables so it can lead to bugs.
> efi_status_t ret;
>
> /* find active boot device in BootNext */
> @@ -961,8 +1061,14 @@ static efi_status_t find_boot_device(void)
> if (device_is_present_and_system_part(boot_dev)) {
> goto found;
> } else {
> - efi_free_pool(boot_dev);
> - boot_dev = NULL;
> + full_path =
> get_esp_from_boot_option_file_path(boot_dev);
> + if (full_path) {
> + boot_dev = full_path;
Later, we free full_path but do we ever free the original boot_dev?
> + goto found;
> + } else {
> + efi_free_pool(boot_dev);
> + boot_dev = NULL;
> + }
Better to delete indenting where you can:
full_path =
get_esp_from_boot_option_file_path(boot_dev);
if (full_path) {
boot_dev = full_path;
goto found;
}
efi_free_pool(boot_dev);
boot_dev = NULL;
regards,
dan carpenter
Re: [PATCH] board: amlogic: fix buffler overflow in serial & usid read
On Tue, Mar 19, 2024 at 03:53:24PM +0100, Neil Armstrong wrote:
> While meson_sm_read_efuse() doesn't overflow, the string is not
> zero terminated and env_set() will buffer overflow and add random
> characters to environment.
>
In the Linux kernel we would give this a CVE because it's information
disclosure bug...
> Signed-off-by: Neil Armstrong
> ---
> board/amlogic/jethub-j80/jethub-j80.c | 6 --
> board/amlogic/p200/p200.c | 3 ++-
> board/amlogic/p201/p201.c | 3 ++-
> board/amlogic/p212/p212.c | 3 ++-
> board/amlogic/q200/q200.c | 3 ++-
> 5 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/board/amlogic/jethub-j80/jethub-j80.c
> b/board/amlogic/jethub-j80/jethub-j80.c
> index 185880de13..d10492cc46 100644
> --- a/board/amlogic/jethub-j80/jethub-j80.c
> +++ b/board/amlogic/jethub-j80/jethub-j80.c
> @@ -28,8 +28,8 @@
> int misc_init_r(void)
> {
> u8 mac_addr[EFUSE_MAC_SIZE];
This one is also a problem. You can't pass non-terminated strings to
eth_env_set_enetaddr(). We call strlen() on it in either hsearch_r() or
env_get_from_linear().
All the other functions had a mac_addr[] issue as well.
Btw, this kind of bug is a good candidate for a static checker warning.
I can create a Smatch check for this. It would probably be easier in
Coccinelle even, but I'm the Smatch maintainer.
regards,
dan carpenter
> - char serial[EFUSE_SN_SIZE];
> - char usid[EFUSE_USID_SIZE];
> + char serial[EFUSE_SN_SIZE + 1];
> + char usid[EFUSE_USID_SIZE + 1];
> ssize_t len;
> unsigned int adcval;
> int ret;
> @@ -46,6 +46,7 @@ int misc_init_r(void)
> if (!env_get("serial")) {
> len = meson_sm_read_efuse(EFUSE_SN_OFFSET, serial,
> EFUSE_SN_SIZE);
> + serial[len] = '\0';
> if (len == EFUSE_SN_SIZE)
> env_set("serial", serial);
Re: [PATCH] fs: ext4: Fixed file permissions
On Thu, Mar 14, 2024 at 02:41:29PM +0800, Jixiong Hu wrote: > Modified the ext4fs_write function to create a new file that > inherits the inode->mode of existing file. To fix an issue > where file permissions are changed after modifying the contents > of an existing file. I'm not an expert, but honestly the current behavior sound more correct than the proposed behavior. What's the issue specifically so we can understand it better? Also bug fixes need a Fixes tag. The patch has a number of style issues and the existing_file_inode allocation is not always freed so those memory leaks would need to be fixed. For the style issues run ./scripts/checkpatch.pl on your patch. regards, dan carpenter
Re: [PATCH 01/19] spi: cadence_qspi: Add support for DDR PHY mode
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index faf02c7778..5895b5de09 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1511,8 +1511,10 @@ static const struct flash_info *spi_nor_read_id(struct
> spi_nor *nor)
> info = spi_nor_ids;
> for (; info->name; info++) {
> if (info->id_len) {
> - if (!memcmp(info->id, id, info->id_len))
> + if ((!memcmp(info->id, id, info->id_len)) &&
> + memcpy(nor->spi->device_id, id,
> SPI_NOR_MAX_ID_LEN)) {
Please, don't put a memcpy() into a condition. It looks like a memcmp()
to the eye.
> return info;
> + }
if (!memcmp(info->id, id, info->id_len)) {
memcpy(nor->spi->device_id, id, SPI_NOR_MAX_ID_LEN);
return info;
}
> }
> }
>
[ snip ]
> static int cadence_spi_mem_exec_op(struct spi_slave *spi,
> const struct spi_mem_op *op)
> {
> @@ -353,6 +649,9 @@ static int cadence_spi_mem_exec_op(struct spi_slave *spi,
> break;
> }
>
> + if (op->cmd.dtr)
> + err = cadence_spi_setup_ddrmode(spi, op);
> +
> return err;
I think there should be another if (err) return err after the end of the
switch statement.
> }
>
regards,
dan carpenter
Re: [PATCH] fdt_support: fix fdt_copy_fixed_partitions function()
On Fri, Mar 08, 2024 at 02:34:04PM +0100, Patrice Chotard wrote:
> Move variable declaration at the beginning of the function.
>
The problem, presumably, is that when declarations are in the middle of
a block then it triggers a GCC warning. "declarations after code" or
whatever... The commit message is not really clear.
And when I built this file I don't get a warning. Is there a specific
config required to trigger the warning?
Btw, the Linux kernel recently silenced this warning because it doesn't
work well with the cleanup.h code... It will be interesting to see if
people abandon this style guideline.
regards,
dan carpenter
> Fixes: 163c5f60ebb4 ("fdt_support: add fdt_copy_fixed_partitions function")
>
> Signed-off-by: Patrice Chotard
> ---
Re: [PATCH 1/2] sandbox: select CONFIG_64BIT for SANDBOX64
On Wed, Mar 06, 2024 at 09:50:06AM +0100, Heinrich Schuchardt wrote: > On 3/5/24 16:16, Dan Carpenter wrote: > > Select CONFIG_64BIT so that we pass the -m64 option (instead of -m32) to > > static analysis tools. > > > > Signed-off-by: Dan Carpenter > > --- > > arch/sandbox/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig > > index 0ce77de2fcb4..c431da60e8c4 100644 > > --- a/arch/sandbox/Kconfig > > +++ b/arch/sandbox/Kconfig > > @@ -12,6 +12,7 @@ config SYS_CPU > > > > config SANDBOX64 > > bool "Use 64-bit addresses" > > + select 64BIT > > SANDBOX64 seems only to control the size of phys_addr_t, phys_size_t, > dma_addr_t. Heh. I assumed SANDBOX64 was much more important than it was... > > Please, have a look at symbol SANDBOX_BITS_PER_LONG. It is controlled by > HOST_64BIT, not by SANDBOX64. Isn't this closer related to the -m32/-m64 > choice? What about if I just did something like this. It seems to work okay. regards, dan carpenter diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig index 0ce77de2fcb4..ebffad32e94f 100644 --- a/arch/sandbox/Kconfig +++ b/arch/sandbox/Kconfig @@ -46,6 +46,7 @@ config HOST_32BIT config HOST_64BIT def_bool $(cc-define,_LP64) + select 64BIT config HOST_HAS_SDL def_bool $(success,sdl2-config --version)
Re: [PATCH v2 5/5] power: regulator: tps65941: Add TPS65224 PMIC regulator support
On Fri, Mar 08, 2024 at 02:40:11PM +0530, Dhruva Gole wrote: > > + > > + if (val < 0 || val > mask) > > + return -EINVAL; > > Probably combine this with below if's, eitherway is fine. > > > + > > + if (val <= reg_base) > > + return base; > > + > > + if (val >= reg_max) > > + return max; > > Why all these if's when they can be elifs? > > > + This was just a cut and paste of my suggested code. I really like my code... It's like a tortoise eating the three bears' food... This is invalid. This is low. This is high. This poridge is just right. regards, dan carpenter
Re: [PATCH] cmd: sbi: Correctly display unknown implementation IDs
On Wed, Mar 06, 2024 at 03:44:02PM +0100, Heinrich Schuchardt wrote:
> For an unknown implementation ID an output like
>
> SBI 1.0Unknown implementation ID 16777216
> Extensions:
> sbi_set_timer
> ...
>
> was shown. The number 16777216 is not the implementation ID.
>
> * Show the correct number
> * Use a hexadecimal output format
> * Add a missing line feed
>
> Signed-off-by: Heinrich Schuchardt
Reviewed-by: Dan Carpenter
Fixes: 89a86dcf6191 ("cmd: sbi: show SBI implementation version")
regards,
dan carpenter
Re: [PATCH v3] board: rockchip: add Rockchip Toybrick TB-RK3588X board
On Tue, Mar 05, 2024 at 09:29:22AM +0800, zhangzj wrote:
> +
> +#ifdef CONFIG_OF_BOARD_SETUP
> +static int rk3588_add_reserved_memory_fdt_nodes(void *new_blob)
> +{
> + struct fdt_memory gap1 = {
> + .start = 0x3fc00,
> + .end = 0x3fc4f,
> + };
> + struct fdt_memory gap2 = {
> + .start = 0x3fff0,
> + .end = 0x3,
> + };
> + unsigned long flags = FDTDEC_RESERVED_MEMORY_NO_MAP;
> + unsigned int ret;
"ret" should be int. This doesn't affect runtime at all though.
#nitpicking
regards,
dan carpenter
> +
> + /*
> + * Inject the reserved-memory nodes into the DTS
> + */
> + ret = fdtdec_add_reserved_memory(new_blob, "gap1", , NULL, 0,
> + NULL, flags);
> + if (ret)
> + return ret;
> +
> + return fdtdec_add_reserved_memory(new_blob, "gap2", , NULL, 0,
> + NULL, flags);
> +}
[PATCH 2/2] sandbox: select CONFIG_64BIT for X86_64
Select CONFIG_64BIT so that we pass the -m64 option (instead of -m32) to static analysis tools. Signed-off-by: Dan Carpenter --- arch/x86/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 99e59d94c606..fb7772936cd5 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -44,6 +44,7 @@ endchoice config X86_64 bool + select 64BIT config SPL_X86_64 bool -- 2.43.0
[PATCH 1/2] sandbox: select CONFIG_64BIT for SANDBOX64
Select CONFIG_64BIT so that we pass the -m64 option (instead of -m32) to static analysis tools. Signed-off-by: Dan Carpenter --- arch/sandbox/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/sandbox/Kconfig b/arch/sandbox/Kconfig index 0ce77de2fcb4..c431da60e8c4 100644 --- a/arch/sandbox/Kconfig +++ b/arch/sandbox/Kconfig @@ -12,6 +12,7 @@ config SYS_CPU config SANDBOX64 bool "Use 64-bit addresses" + select 64BIT select PHYS_64BIT depends on HOST_64BIT -- 2.43.0
[PATCH 0/2] Select CONFIG_64BIT for sandbox64 and x86_64
I had previously set CONFIG_64BIT for arm64. This patchset does the same thing for sandbox64 and x86_64. (Mips and riscv were already doing it). This CONFIG option is used in the Makefile to determine if it's a 32 or 64 bit system for the CHECKER. Makefile 1052 # the checker needs the correct machine size 1053 CHECKFLAGS += $(if $(CONFIG_64BIT),-m64,-m32) Dan Carpenter (2): sandbox: select CONFIG_64BIT for SANDBOX64 sandbox: select CONFIG_64BIT for X86_64 arch/sandbox/Kconfig | 1 + arch/x86/Kconfig | 1 + 2 files changed, 2 insertions(+) -- 2.43.0
Re: [PATCH v2] initcall: break loop immediately on failure
On Tue, Mar 05, 2024 at 02:55:13PM +, Caleb Connolly wrote: > The current ordering always results in func pointing to the next > function in the init_sequence. e.g. if fdtdec_setup() fails, ret will > be set to the error code, then func will be updated to point to > initf_malloc(), only then is ret checked and the loop broken. The end > result of this is that the "initcall failed at ..." error will point you > to initf_malloc(), when the error actually occured in fdtdec_setup()! > > This can be quite confusing and result in a lot of time wasted debugging > code that has nothing to do with the failure (ask me how I know :P). > > Adjust the for loop to check ret immediately after the call and break > early so that func will correctly reference the failed function. > > Signed-off-by: Caleb Connolly > --- > Changes in v2: > - Don't drop the initialisation of ret (thanks Dan!). > - Link to v1: > https://lore.kernel.org/u-boot/20240219183519.2183405-1-caleb.conno...@linaro.org/ > > Cc: Dan Carpenter Awesome. Thanks! Reviewed-by: Dan Carpenter regards, dan carpenter
Re: [PATCH v2 2/2] arm64: Enable CONFIG_64BIT for static analysis
On Mon, Mar 04, 2024 at 02:44:12PM +0100, Heinrich Schuchardt wrote: > On 04.03.24 08:04, Dan Carpenter wrote: > > In the Makefile there is a line that says this: > > > > # the checker needs the correct machine size > > CHECKFLAGS += $(if $(CONFIG_64BIT),-m64,-m32) > > > > Set CONFIG_64BIT for ARM64 so that we pass -m64 to the static checkers > > instead of -m32. > > > > Signed-off-by: Dan Carpenter > > --- > > v2: split the patch into two patches > > > > arch/arm/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > > index fde85dc0d537..76d6a8cc6886 100644 > > --- a/arch/arm/Kconfig > > +++ b/arch/arm/Kconfig > > @@ -6,6 +6,7 @@ config SYS_ARCH > > config ARM64 > > bool > > + select 64BIT > > How about other 64bit architectures (sandbox64, x86_64)? > You're right. That was laziness on my part. Plus, I didn't know about sandbox64. But I think once we fix the two you mentioned then everything will be updated. $ grep -l 64 arch/*/Kconfig arch/arm/Kconfig arch/mips/Kconfig arch/riscv/Kconfig arch/sandbox/Kconfig arch/x86/Kconfig I'll send those patches tomorrow. regards, dan carpenter
Re: RE: [PATCH] net: phy: ncsi: Correct the endian of the checksum
Anyway, looks good! I already gave my reviewed by. Thanks for your work on this. regards, dan carpenter
[PATCH v2 2/2] arm64: Enable CONFIG_64BIT for static analysis
In the Makefile there is a line that says this: # the checker needs the correct machine size CHECKFLAGS += $(if $(CONFIG_64BIT),-m64,-m32) Set CONFIG_64BIT for ARM64 so that we pass -m64 to the static checkers instead of -m32. Signed-off-by: Dan Carpenter --- v2: split the patch into two patches arch/arm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index fde85dc0d537..76d6a8cc6886 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -6,6 +6,7 @@ config SYS_ARCH config ARM64 bool + select 64BIT select PHYS_64BIT select SYS_CACHE_SHIFT_6 imply SPL_SEPARATE_BSS -- 2.43.0
[PATCH v2 1/2] Kconfig: move CONFIG_32/64BIT to arch/Kconfig
These configs are used in multiple places so put them in a shared Kconfig file. Signed-off-by: Dan Carpenter --- v2: split the patch into two patches arch/Kconfig | 6 ++ arch/mips/Kconfig | 6 -- arch/riscv/Kconfig | 6 -- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/arch/Kconfig b/arch/Kconfig index b6fb9e927337..70f3b85b449a 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -20,6 +20,12 @@ config SYS_CACHE_SHIFT_6 config SYS_CACHE_SHIFT_7 bool +config 32BIT + bool + +config 64BIT + bool + config SYS_CACHELINE_SIZE int default 128 if SYS_CACHE_SHIFT_7 diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index f0704d97f79b..eb7f3ad23762 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -473,12 +473,6 @@ config MIPS_TUNE_74KC config MIPS_TUNE_OCTEON3 bool -config 32BIT - bool - -config 64BIT - bool - config SWAP_IO_SPACE bool diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index ac52c5e6dafa..6c26f91f1669 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -288,12 +288,6 @@ endmenu config RISCV_ISA_A def_bool y -config 32BIT - bool - -config 64BIT - bool - config DMA_ADDR_T_64BIT bool default y if 64BIT -- 2.43.0
[PATCH v2 0/2] arm64: Enable CONFIG_64BIT for static analysis
The makefiles currently pass -m32 to Smatch static checker when I'm building on arm64. Also the arch is set to "arm" and Smatch thinks "arm" is 32 bits and "arm64" is 64 bits. With this patchset we pass -m64 and Smatch works correctly. Changes since v1: Break up the patch into two patches. Dan Carpenter (2): Kconfig: move CONFIG_32/64BIT to arch/Kconfig ARM: Enable CONFIG_64BIT for static analysis arch/Kconfig | 6 ++ arch/arm/Kconfig | 1 + arch/mips/Kconfig | 6 -- arch/riscv/Kconfig | 6 -- 4 files changed, 7 insertions(+), 12 deletions(-) -- 2.43.0
Re: 回覆: [PATCH] net: phy: ncsi: Correct the endian of the checksum
On Sun, Mar 03, 2024 at 02:14:43AM +, Jacky Chou wrote: > Hi Dan Carpenter, > > I have verified it on the little-endian platform, such as ASPEED AST2600. Awesome. Thanks for this. > I think put_unaligned_be32() and htonl() functions have no effect on > big-endian platforms. > And keep put_unaligned_be32() to help access the unaligned memory, such as > pchecksum variable. Yes. I know that. What I'm just puzzled by is how we ever merged this code when it doesn't work for little endian systems. How was it tested originally? How do the errors look like? Perhaps they're not as bad I assume from looking at the code... regards, dan carpenter
Re: [PATCH] mtd: nand: arasan: Update the correct return codes
On Fri, Mar 01, 2024 at 04:51:09PM +0530, Venkatesh Yadav Abbarapu wrote:
> The below exception observed on QEMU, as it doesn't support
> NAND controller.
>
> "Synchronous Abort" handler, esr 0x9605, far 0x17acfc878
> elr: 0803ad40 lr : 0805f438 (reloc)
> elr: 7fcb4d40 lr : 7fcd9438
> x0 : 7bbfc880 x1 : ff10
> x2 : 7fcf059c x3 : 7bbfc870
> x4 : 7fd9a388 x5 : 00017acfc870
> x6 : x7 : 7bbfd0e0
> x8 : 3dd4 x9 : 7bbeec0c
> x10: 0001 x11: 3f8c
> x12: 7bbeecfc x13: 7bbeeeb0
> x14: 7bbeeeb0 x15: 7bbee474
> x16: 7fcef18c x17:
> x18: 7bbf9d70 x19: 7bbfc888
> x20: 7bbfc870 x21: 7fd68ddb
> x22: ffed x23: 7bbfc878
> x24: x25:
> x26: x27:
> x28: x29: 7bbeed10
>
> Code: 927ff8c1 924000c6 8b010065 f9400887 (f94004a2)
> Resetting CPU ...
>
> Updating the correct return codes rather than hardcoding, remove the
> free as there is no memory allocated using malloc.
>
> Signed-off-by: Venkatesh Yadav Abbarapu
The crash is caused by the use after free because we shouldn't
free(nand)? Returning the correct error codes is nice, but it shouldn't
cause a crash...
Fixes: 3dd0f8cccd6d ("mtd: nand: Remove hardcoded base address of nand")
regards,
dan carpenter
Re: [UBOOT PATCH v3] mtd: nand: arasan: Print warning for unsupported ecc modes
On Fri, Mar 01, 2024 at 04:42:54PM +0530, Venkatesh Yadav Abbarapu wrote:
> @@ -1263,6 +1264,12 @@ static int arasan_probe(struct udevice *dev)
> goto fail;
> }
>
> + str = ofnode_read_string(nand_chip->flash_node, "nand-ecc-mode");
> + if (strcmp(str, "hw")) {
> + printf("%s ecc is not supported, switch to hw ecc\n", str);
I'm a newbie to device trees, but the two other callers assume that it's
possible for ofnode_read_string(nand->flash_node, "nand-ecc-mode") to
return NULL so probably we should add a NULL check here too.
if (!str || strcmp(str, "hw") != 0) {
regards,
dan carpenter
Re: [PATCH 4/8] clk: qcom: add support for power domains uclass
On Thu, Feb 29, 2024 at 02:21:08PM +, Volodymyr Babchuk wrote:
> @@ -223,7 +229,7 @@ U_BOOT_DRIVER(qcom_clk) = {
> int qcom_cc_bind(struct udevice *parent)
> {
> struct msm_clk_data *data = (struct msm_clk_data
> *)dev_get_driver_data(parent);
> - struct udevice *clkdev, *rstdev;
> + struct udevice *clkdev, *rstdev, *pwrdev;
> struct driver *drv;
> int ret;
>
> @@ -253,6 +259,20 @@ int qcom_cc_bind(struct udevice *parent)
> if (ret)
> device_unbind(clkdev);
Change this to:
if (ret)
goto unbind_clkdev;
>
> + if (!data->power_domains)
> + return ret;
Then this becomes:
if (!data->power_domains)
return 0;
> +
> + /* Get a handle to the common power domain handler */
> + drv = lists_driver_lookup_name("qcom_power");
> + if (!drv)
> + return -ENOENT;
if (!drv) {
ret = -ENOENT;
goto unbind_rstdev;
}
> +
> + /* Register the power domain controller */
> + ret = device_bind_with_driver_data(parent, drv, "qcom_power",
> (ulong)data,
> +dev_ofnode(parent), );
> + if (ret)
> + device_unbind(pwrdev);
pwrdev wasn't bound. Free the last *successful* allocation which was
still rstdev.
if (ret)
goto unbind_rstdev;
return 0;
unbind_rstdev:
device_unbind(rstdev);
unbind_clkdev:
device_unbind(clkdev);
return ret;
> +
> return ret;
> }
>
regards,
dan carpenter
Re: ECDSA related PRs
On Thu, Feb 22, 2024 at 03:07:01PM -0800, Bob Wolff wrote: > Peter, > Thanks for helping lead me down the right path here. > > WRT tinycrypt, the license is quite permissive. > https://github.com/intel/tinycrypt > > Also, I'd like your advice - I was thinking for the larger patch that I'd > do it in two commits. The first would be the addition of the tinycrypt > files and the second is the actual changes and additions to support ecdsa > verification. I doubt that's controversial. However when I run a trial > `patman` against the tinycrypt commit, I geta huge number of issues: > *checkpatch.pl <http://checkpatch.pl> found 186 error(s), 380 > warning(s), 481 checks(s)* > > What's your advice on this? I would tend to think we'd want to /not/ change > the source files directly for such purposes so that updates could be > brought in with greater ease. Yeah. For this kind of thing you wouldn't want to make a bunch of checkpatch changes. They sometimes pull crypto and compression libraries into the Linux kernel pretty much unmodified as well for the same reason. Igor's proposal about pull this stuff from the Linux kernel instead seems like a good idea though. regards, dan carpenter
Re: [PATCH 1/2] opos6uldev: make the LCD work again
On Tue, Feb 27, 2024 at 04:40:01PM +0100, Sébastien Szymanski wrote:
> Commit 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with
> linux") removed the display timings from the board device tree whereas
> they are still needed by the mxsfb driver.
> Add the timings back (the correct ones) in the
> imx6ul-opos6uldev-u-boot.dtsi file and remove them from the
> opos6uldev.env file.
>
> Update the opos6uldev_defconfig file so that the LCD turns on at boot.
>
> Fixes: 5d7a95f4 ("imx6ul/imx6ull: synchronise device trees with linux")
> Signed-off-by: Sébastien Szymanski
Huh. This is the commit that did that upstream.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d9aa4d4fca67823838fe9861456201430c545e69
It's interesting how the timings in linux were always slightly different
from in u-boot.
regards,
dan carpenter
Re: [PATCH 05/10] mach-snapdragon: Update gd->ram_size in msm_fixup_memory
On Mon, Feb 26, 2024 at 03:38:02PM +0530, Varadarajan Narayanan wrote: > > * Update FDT only if 'blob' is not null It seems that this is patching up a bug that was introduced in patch 1/10. Please fold it into patch 1. > @@ -91,9 +93,9 @@ int msm_fixup_memory(void *blob) > return -ENODEV; > } > > - ret = fdt_fixup_memory_banks(blob, bank_start, bank_size, count); > - if (ret) > - return ret; > + if (blob) > + return fdt_fixup_memory_banks(blob, bank_start, > + bank_size, count); > > - return 0; > + return ret; Please leave this as return 0; regards, dan carpenter
Re: [PATCH RESEND] serial: pl01x: set baudrate when probing
On Sun, Feb 25, 2024 at 08:38:33AM +0800, Yang Xiwen via B4 Relay wrote:
> #if CONFIG_IS_ENABLED(OF_PLATDATA)
> struct dtd_serial_pl01x *dtplat = >dtplat;
> @@ -301,10 +302,14 @@ int pl01x_serial_probe(struct udevice *dev)
> #endif
> priv->type = plat->type;
>
> - if (!plat->skip_init)
> - return pl01x_generic_serial_init(priv->regs, priv->type);
> - else
> + if (!plat->skip_init) {
> + ret = pl01x_generic_serial_init(priv->regs, priv->type);
> + if (!ret)
This if statement seems to be reversed.
regards,
dan carpenter
> + return ret;
> + return pl01x_serial_setbrg(dev, gd->baudrate);
> + } else {
> return 0;
> + }
> }
Re: [PATCH v1 5/5] power: regulator: tps65941: Add TPS65224 PMIC regulator support
On Fri, Feb 23, 2024 at 02:42:12PM +0530, Bhargav Raviprakash wrote:
> + int mask = TPS65224_LDO_VOLT_MASK >> 1;
> +
> + if (idx > 0) {
> + base = TPS65224_LDO23_VOLT_MIN;
> + max = TPS65224_LDO23_VOLT_MAX;
> + reg_base = TPS65224_LDO23_VOLT_MIN_HEX;
> + reg_max = TPS65224_LDO23_VOLT_MAX_HEX;
> + }
> +
> + val = val >> 1;
> + if (val > mask || val < 0)
> + return -EINVAL;
> + else if (val >= reg_max)
> + return max;
> + else if (val <= reg_base)
> + return base;
> + else if (val >= 0)
> + return base + (step * (val - reg_base));
> + else
> + return -EINVAL;
Instead of "if (val >= 0)" it would be more clear to write
"if (value > reg_base)". Or something like this:
val = val >> 1;
if (val < 0 || val > mask)
return -EINVAL;
if (val <= reg_base)
return base;
if (val >= reg_max)
return max;
return base + (step * (val - reg_base));
regards,
dan carpenter
Re: [PATCH] ARM: Enable CONFIG_64BIT for static analysis
On Thu, Feb 22, 2024 at 08:16:19AM -0500, Tom Rini wrote:
>
> That we have 'config 64BIT' in arch/{mips,riscv}/Kconfig as well is a
> leftover of thinking this is like the Linux Kernel where there's no
> top-level arch/Kconfig file, can you please move the two existing config
> 64BIT entries to arch/Kconfig and then select it for ARM64? Thanks.
Sure. Even better.
regards,
dan carpenter
[PATCH] ARM: Enable CONFIG_64BIT for static analysis
In the Makefile there is a line that says this: # the checker needs the correct machine size CHECKFLAGS += $(if $(CONFIG_64BIT),-m64,-m32) So set CONFIG_64BIT so that we don't pass -m32 to the static checker. Signed-off-by: Dan Carpenter --- arch/arm/Kconfig | 4 1 file changed, 4 insertions(+) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index fde85dc0d537..4c7be7cf9c33 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1,11 +1,15 @@ menu "ARM architecture" depends on ARM +config 64BIT + bool + config SYS_ARCH default "arm" config ARM64 bool + select 64BIT select PHYS_64BIT select SYS_CACHE_SHIFT_6 imply SPL_SEPARATE_BSS -- 2.43.0
[PATCH] bootstd: fix build error when CONFIG_MMC is disabled
This code assumes that CONFIG_MMC and it causes a build error when
the config is disabled.
aarch64-linux-gnu-ld.bfd: test/boot/bootstd_common.o: in function
`bootstd_test_check_mmc_hunter':
test/boot/bootstd_common.c:83:(.text.bootstd_test_check_mmc_hunter+0x70):
undefined reference to `_u_boot_list_2_bootdev_hunter_2_mmc_bootdev_hunter'
Fixes: 66e3dce78750 ("bootstd: Allow hunting for a bootdev by label")
Signed-off-by: Dan Carpenter
---
test/boot/bootstd_common.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/test/boot/bootstd_common.c b/test/boot/bootstd_common.c
index e71a2975c53c..cc97e255e5cb 100644
--- a/test/boot/bootstd_common.c
+++ b/test/boot/bootstd_common.c
@@ -74,6 +74,9 @@ int bootstd_test_check_mmc_hunter(struct unit_test_state *uts)
struct bootstd_priv *std;
uint seq;
+ if (!IS_ENABLED(CONFIG_MMC))
+ return 0;
+
/* get access to the used hunters */
ut_assertok(bootstd_get_priv());
--
2.43.0
[PATCH] bootflow: Fix build error when BOOTMETH_CROS is disabled
The bootflow testing assumes that BOOTMETH_CROS is enabled but it
might not be which leads to a build error.
aarch64-linux-gnu-ld.bfd: test/boot/bootflow.o: in function `prep_mmc_bootdev':
test/boot/bootflow.c:549:(.text.prep_mmc_bootdev+0x1c8):
undefined reference to `_u_boot_list_2_driver_2_bootmeth_cros'
Fixes: d08db02d2d3d ("bootstd: Add a test for bootmeth_cros")
Signed-off-by: Dan Carpenter
---
test/boot/bootflow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test/boot/bootflow.c b/test/boot/bootflow.c
index fa54dde661c8..4845b7121c84 100644
--- a/test/boot/bootflow.c
+++ b/test/boot/bootflow.c
@@ -544,7 +544,7 @@ static int prep_mmc_bootdev(struct unit_test_state *uts,
const char *mmc_dev,
"bootmeth_script", 0, ofnode_null(), ));
/* Enable the cros bootmeth if needed */
- if (bind_cros) {
+ if (IS_ENABLED(CONFIG_BOOTMETH_CROS) && bind_cros) {
ut_assertok(uclass_first_device_err(UCLASS_BOOTSTD, ));
ut_assertok(device_bind(bootstd, DM_DRIVER_REF(bootmeth_cros),
"cros", 0, ofnode_null(), ));
--
2.43.0
Re: [PATCH] implement policy_pcr commands to lock NV-indexes behind a PCR
I'm kind of new to u-boot and I'm not really able to review this code
as well as I should.
But also I can't apply the patch. It seems white space damaged? The
kernel has a good document on how to do this. I'm pretty sure u-boot
does as well but I'm new.
https://www.kernel.org/doc/Documentation/process/email-clients.rst
Please run your patch through the scripts/checkpatch.pl script. Stuff
like this triggers a warning:
> +static int do_tpm_nv_write_value(struct cmd_tbl *cmdtp, int flag,
> + int argc, char *const argv[]) //TODO: session handle
> from auth session!
> +{
> + struct udevice *dev;
> + u32 nv_addr, nv_size, rc;
> + void *session_addr = NULL;
> + int ret;
> +
> + ret = get_tpm();
> + if (ret)
> + return ret;
> +
> + if (argc < 4)
> + return CMD_RET_USAGE;
WARNING: suspect code indent for conditional statements (0, 0)
#250: FILE: cmd/tpm-v2.c:437:
+ if (ret)
+ return ret;
WARNING: suspect code indent for conditional statements (0, 0)
#253: FILE: cmd/tpm-v2.c:440:
+ if (argc < 4)
+ return CMD_RET_USAGE;
Also the subject should have a subsystem prefix and the information from
the email should be moved into the commit message. Currently the commit
message is empty.
> diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> index 33dd103767..5b60883777 100644
> --- a/include/tpm-v2.h
> +++ b/include/tpm-v2.h
> @@ -301,7 +301,8 @@ enum tpm2_startup_types {
> */
> enum tpm2_handles {
> TPM2_RH_OWNER = 0x4001,
> - TPM2_RS_PW= 0x4009,
> + TPM2_RH_NULL= 0x4007,
> + TPM2_RS_PW = 0x4009,
Changing TPM2_RS_PW is an unrelated whitespace change. Do that as a
separate patch. But I don't get it at all because the TPM2_RS_PW enum
has always been indented correctly as far as I can see. So it's a
puzzle.
I mean there are a lot of TODOs and I understand that you just wanted a
high level review but I kept getting distracted and lost and I couldn't
apply the patch so it was just really hard to figure out what was going
on. :(
regards,
dan carpenter
Re: [PATCH] initcall: break loop immediately on failure
On Mon, Feb 19, 2024 at 06:35:03PM +, Caleb Connolly wrote:
> The current ordering always results in func pointing to the next
> function in the init_sequence. e.g. if fdtdec_setup() fails, ret will
> be set to the error code, then func will be updated to point to
> initf_malloc(), only then is ret checked and the loop broken. The end
> result of this is that the "initcall failed at ..." error will point you
> to initf_malloc(), when the error actually occured in fdtdec_setup()!
>
> This can be quite confusing and result in a lot of time wasted debugging
> code that has nothing to do with the failure (ask me how I know :P).
Heh. Subtle.
>
> Adjust the for loop to check ret immediately after the call and break
> early so that func will correctly reference the failed function.
>
> Signed-off-by: Caleb Connolly
> ---
> lib/initcall.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/initcall.c b/lib/initcall.c
> index ce317af213ab..128242e5ff55 100644
> --- a/lib/initcall.c
> +++ b/lib/initcall.c
> @@ -52,11 +52,11 @@ int initcall_run_list(const init_fnc_t init_sequence[])
> ulong reloc_ofs = calc_reloc_ofs();
> const init_fnc_t *ptr;
> enum event_t type;
> init_fnc_t func;
> - int ret = 0;
> + int ret;
We need to keep this initialization. The bug would be when we're
building for CONFIG_ARC and this is called from board_init_f_r(). The
array is empty except for the NULL sentinal at the end. We don't
enter the loop in that case.
regards,
dan carpenter
>
> - for (ptr = init_sequence; func = *ptr, !ret && func; ptr++) {
> + for (ptr = init_sequence; func = *ptr, func; ptr++) {
> type = initcall_is_event(func);
>
> if (type) {
> if (!CONFIG_IS_ENABLED(EVENT))
> @@ -70,8 +70,10 @@ int initcall_run_list(const init_fnc_t init_sequence[])
> debug("initcall: %p\n", (char *)func - reloc_ofs);
> }
>
> ret = type ? event_notify_null(type) : func();
> + if (ret)
> + break;
> }
>
> if (ret) {
> if (CONFIG_IS_ENABLED(EVENT)) {
> --
> 2.43.1
>
Re: [PATCH] video: simplefb: modernise DT parsing
On Fri, Feb 16, 2024 at 06:38:06PM +, Caleb Connolly wrote:
> @@ -41,17 +41,25 @@ static int simple_video_probe(struct udevice *dev)
>
> debug("%s: Query resolution...\n", __func__);
>
> - uc_priv->xsize = fdtdec_get_uint(blob, node, "width", 0);
> - uc_priv->ysize = fdtdec_get_uint(blob, node, "height", 0);
> - uc_priv->rot = fdtdec_get_uint(blob, node, "rot", 0);
> - if (uc_priv->rot > 3) {
> - log_debug("%s: invalid rot\n", __func__);
> - return log_msg_ret("rot", -EINVAL);
> + ret = ofnode_read_u32(node, "width", );
> + ret = ret ?: ofnode_read_u32(node, "height", );
> + if (ret || !width || !height) {
> + log_err("%s: invalid width or height: %d\n", __func__, ret);
> + return ret;
This should be something like:
return ret ?: -EINVAL;
Perhaps print the width and height in the error message as well.
regards,
dan carpenter
Re: [PATCH v2] dma: ti: k3-udma: Fix error handling for setup_resources() in udma_probe()
On Tue, Feb 20, 2024 at 03:34:51PM +0530, Siddharth Vadapalli wrote:
> In udma_probe() the return value of setup_resources() is stored in the
> u32 "ch_count" member of "struct udma_dev", due to which any negative
> return value which indicates an error is masked.
>
> Fix this by storing the return value of setup_resources() in the already
> declared integer variable "ret", followed by assigning it to the "ch_count"
> member of "struct udma_dev" in case of no error.
>
> While at it, change the "return ret" at the end of udma_probe() to a
> "return 0", to explicitly indicate that probe was successful.
>
> Fixes: a8837cf43839 ("dma: ti: k3-udma: Query DMA channels allocated from
> Resource Manager")
> Signed-off-by: Siddharth Vadapalli
> ---
Thanks!
Reviewed-by: Dan Carpenter
regards,
dan carpenter
Re: [PATCH] dma: ti: k3-udma: Fix error handling for setup_resources() in udma_probe()
On Fri, Feb 16, 2024 at 04:11:05PM +0530, Siddharth Vadapalli wrote: > diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c > index eea9ec9659..8a6625f034 100644 > --- a/drivers/dma/ti/k3-udma.c > +++ b/drivers/dma/ti/k3-udma.c > @@ -1770,9 +1770,11 @@ static int udma_probe(struct udevice *dev) > return PTR_ERR(ud->ringacc); > > ud->dev = dev; > - ud->ch_count = setup_resources(ud); > - if (ud->ch_count <= 0) > - return ud->ch_count; > + ret = setup_resources(ud); > + if (ret <= 0) > + return ret; The code was like this originally, but setup_resources() can't actually return zero so it would be nicer to say: ret = setup_resources(ud); if (ret < 0) return ret; regards, dan carpenter > + > + ud->ch_count = ret; >
Re: [PATCH v3 2/3] board: Add support for Sielaff i.MX6 Solo board
On Tue, Feb 20, 2024 at 09:38:01AM +0100, Frieder Schrempf wrote: > Hi Dan, > > On 20.02.24 06:56, Dan Carpenter wrote: > > On Thu, Feb 15, 2024 at 02:35:20PM +0100, Frieder Schrempf wrote: > >> +int board_mmc_getcd(struct mmc *mmc) > > > > This function is never called. Also for bool functions make them type > > bool and name them so that it's clear they return true/false such as > > board_mmc_getcd_was_successful() but less wordy. > > What makes you think so? This is an implementation for the existing > prototype from mmc.h. As far as I can see this is called by the mmc > driver and I can't change it in any way. > Ah, yes. You're right. My bad. regards, dan carpenter
Re: [PATCH v4 03/39] mmc: msm_sdhci: use modern clock handling
On Thu, Feb 15, 2024 at 08:52:21PM +, Caleb Connolly wrote:
> static int msm_sdc_clk_init(struct udevice *dev)
> {
> - int node = dev_of_offset(dev);
> - uint clk_rate = fdtdec_get_uint(gd->fdt_blob, node, "clock-frequency",
> - 40);
> - uint clkd[2]; /* clk_id and clk_no */
> - int clk_offset;
> - struct udevice *clk_dev;
> - struct clk clk;
> - int ret;
> + struct msm_sdhc *prv = dev_get_priv(dev);
> + ofnode node = dev_ofnode(dev);
> + uint clk_rate;
> + int ret, i = 0, n_clks;
> + const char *clk_name;
>
> - ret = fdtdec_get_int_array(gd->fdt_blob, node, "clock", clkd, 2);
> + ret = ofnode_read_u32(node, "clock-frequency", _rate);
> if (ret)
> - return ret;
> + clk_rate = 40;
>
> - clk_offset = fdt_node_offset_by_phandle(gd->fdt_blob, clkd[0]);
> - if (clk_offset < 0)
> - return clk_offset;
> -
> - ret = uclass_get_device_by_of_offset(UCLASS_CLK, clk_offset, _dev);
> - if (ret)
> + ret = clk_get_bulk(dev, >clks);
> + if (ret) {
> + log_warning("Couldn't get mmc clocks: %d\n", ret);
> return ret;
> + }
>
> - clk.id = clkd[1];
> - ret = clk_request(clk_dev, );
> - if (ret < 0)
> + ret = clk_enable_bulk(>clks);
> + if (ret) {
> + log_warning("Couldn't enable mmc clocks: %d\n", ret);
> return ret;
> + }
>
> - ret = clk_set_rate(, clk_rate);
> - if (ret < 0)
> - return ret;
> + /* If clock-names is unspecified, then the first clock is the core
> clock */
> + if (!ofnode_get_property(node, "clock-names", _clks)) {
> + if (!clk_set_rate(>clks.clks[0], clk_rate)) {
> + log_warning("Couldn't set core clock rate: %d\n", ret);
s/ret/clk_rate/
regards,
dan carpenter
> + return -EINVAL;
> + }
> + }
> +
> + /* Find the index of the "core" clock */
> + while (i < n_clks) {
> + ofnode_read_string_index(node, "clock-names", i, _name);
> + if (!strcmp(clk_name, "core"))
> + break;
> + i++;
> + }
> +
> + if (i >= prv->clks.count) {
> + log_warning("Couldn't find core clock (index %d but only have
> %d clocks)\n", i,
> +prv->clks.count);
> + return -EINVAL;
> + }
> +
> + /* The clock is already enabled by the clk_bulk above */
> + ret = clk_set_rate(>clks.clks[i], clk_rate);
> + /* If we get a rate of 0 then something has probably gone wrong. */
> + if (ret == 0) {
> + log_warning("Couldn't set core clock rate to %u! Driver
> returned rate of 0\n", clk_rate);
> + return -EINVAL;
> + }
>
> return 0;
> }
Re: [PATCH v3 2/3] board: Add support for Sielaff i.MX6 Solo board
On Thu, Feb 15, 2024 at 02:35:20PM +0100, Frieder Schrempf wrote:
> +int board_mmc_getcd(struct mmc *mmc)
This function is never called. Also for bool functions make them type
bool and name them so that it's clear they return true/false such as
board_mmc_getcd_was_successful() but less wordy.
> +{
> + struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;
> + int ret = 0;
> +
> + switch (cfg->esdhc_base) {
> + case USDHC3_BASE_ADDR:
> + ret = !gpio_get_value(USDHC3_CD_GPIO);
> + break;
> + }
> +
> + return ret;
> +}
> +
> +int board_mmc_init(struct bd_info *bis)
> +{
> + int i, ret;
> +
> + /*
> + * According to the board_mmc_init() the following map is done:
> + * (U-boot device node)(Physical Port)
> + * mmc0USDHC1
> + * mmc1USDHC2
> + */
> + for (i = 0; i < CFG_SYS_FSL_USDHC_NUM; i++) {
> + switch (i) {
> + case 0:
> + imx_iomux_v3_setup_multiple_pads(usdhc3_pads,
> +
> ARRAY_SIZE(usdhc3_pads));
> + gpio_direction_input(USDHC3_CD_GPIO);
> + usdhc_cfg[0].sdhc_clk = mxc_get_clock(MXC_ESDHC3_CLK);
> + break;
> + default:
> + printf("Warning: you configured more USDHC controllers \
> + (%d) than supported by the board\n", i + 1);
> + return -EINVAL;
This will look weird if it's ever printed:
"Warning: you configured more USDHC controllers
(%d) than supported by the board\n"
There is a checkpatch warnings for this.
WARNING: Avoid line continuations in quoted strings
#1137: FILE: board/sielaff/imx6dl-sielaff/spl.c:96:
+ printf("Warning: you configured more USDHC controllers \
regards,
dan carpenter
Re: [PATCH] command: add FDT setup for bootelf by flag
On Sun, Feb 11, 2024 at 11:42:48PM +0300, Maxim Moskalets wrote:
> diff --git a/cmd/elf.c b/cmd/elf.c
> index b7b9f506a5..4d365771eb 100644
> --- a/cmd/elf.c
> +++ b/cmd/elf.c
> @@ -38,6 +38,8 @@ static unsigned long do_bootelf_exec(ulong (*entry)(int,
> char * const[]),
> /* Interpreter command to boot an arbitrary ELF image from memory */
> int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> {
> + unsigned long fdt_addr;
> + struct bootm_headers img = { 0 };
> unsigned long addr; /* Address of the ELF image */
> unsigned long rc; /* Return value from user code */
> char *sload = NULL;
> @@ -68,6 +70,18 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc,
> char *const argv[])
> else
> addr = load_elf_image_shdr(addr);
>
> + if (!env_get_elf_need_fdt()) {
Should the ! be there? This looks reversed...
> + if (argc >= 1 && strict_strtoul(argv[0], 16, _addr) !=
> -EINVAL) {
> + printf("Got FDT at 0x%08lx ...\n", fdt_addr);
> +
> + if (image_setup_libfdt(, (void *)fdt_addr, 0,
> NULL)) {
> + printf("ERROR: Failed to process device
> tree\n");
> + return 1;
> + }
> + }
> + }
> +
> +
> if (!env_get_autostart())
> return rcode;
There are a few style nits that I have with this change like the double
blank line at the end. Try running scripts/checkpatch.pl on your patch.
We could also combine the conditions and pull the code in a tab. Also I
recognize that you just copied the != -EINVAL from a few lines earlier
and it does work, but it's better to check for == 0 instead.
if (env_get_elf_need_fdt() && argc >= 1 &&
strict_strtoul(argv[0], 16, _addr) == 0) {
printf("Got FDT at 0x%08lx ...\n", fdt_addr);
if (image_setup_libfdt(, (void *)fdt_addr, 0, NULL)) {
printf("ERROR: Failed to process device tree\n");
return 1;
}
}
regards,
dan carpenter
Re: [PATCH v2 1/7] common: avb_verify: don't call mmc_switch_part for SD
On Fri, Feb 09, 2024 at 08:20:39PM +0100, Igor Opaniuk wrote: > From: Igor Opaniuk > > mmc_switch_part() is used for switching between hw partitions > on eMMC (boot0, boot1, user, rpmb). > There is no need to do that for SD card. > Is this a clean up or a bugfix? What are the visible effects for the user? regards, dan carpenter
Re: [PATCH] xyzModem: Correct xmodem blk verification conditions
On Tue, Feb 06, 2024 at 09:05:33PM +0800, jihongbin wrote:
> It may be that there are relatively few people using this function, or the
> length of the transmitted data is less than 128byte * 255 byte, so the
> triggering conditions for the above problems are not encountered;
>
> I actually discovered this problem during testing. Continuous transmission
> always fails at block 255.
>
> The following is the protocol's definition of blk cblk, cblk = 255 - blk
> The original content of the agreement
> (https://www.menie.org/georges/embedded/xmodem_specification.html) is as
> follows:
> 3. MESSAGE BLOCK LEVEL PROTOCOL
> Each block of the transfer looks like:
> <255-blk #><--128 data bytes-->
> in which:
> = 01 hex
> = binary number, starts at 01 increments by 1, and
> wraps 0FFH to 00H (not to 01)
> <255-blk #> = blk # after going thru 8080 "CMA" instr, i.e.
> each bit complemented in the 8-bit block number.
> Formally, this is the "ones complement".
>
> = the sum of the data bytes only. Toss any carry.
>
Thanks, this is good information. Unfortunately, your patch is not
correct.
Originally you wrote "When the blk sequence number is 255 and cblk is
0, the original XOR condition produces a result of 0, and the judgment
condition will be unsuccessful."
If blk is 255 the cblk should be zero as you say.
common/xyzModem.c
375/* Validate the message */
376if ((xyz.blk ^ xyz.cblk) != (unsigned char) 0xFF)
377 {
378ZM_DEBUG (zm_dprintf
379 ("Framing error - blk: %x/%x/%x\n", xyz.blk, xyz.cblk,
380 (xyz.blk ^ xyz.cblk)));
0xff ^ 0 is equal to 0xFF so it won't print an error. Good!
With your patch, there is an issue with type promotion so the condition
is always true and it will mark everything as a "Framming error".
- if ((xyz.blk ^ xyz.cblk) != (unsigned char) 0xFF)
+ if (~xyz.blk != xyz.cblk)
Both xyz.blk and xyz.cblk are type unsigned char. If you take the
~ of an unsigned char it's going to be 0xffXX where XX are the
a variable bits. That's never going to be equal to xyz.cblk.
You could truncate it to char like this:
+ if ((unsigned char)~xyz.blk != xyz.cblk)
But then it works exactly the same as the original condition.
regards,
dan carpenter
Re: [PATCH v2 1/4] imx8mp-phyboard-pollux-rdk: sync with kernel devicetree from v6.8-rc2
On Tue, Feb 06, 2024 at 10:36:33AM +, Teresa Remmet wrote: > Hello Benjamin, > > > Am Mittwoch, dem 31.01.2024 um 09:45 +0100 schrieb Benjamin Hahn: > > Signed-off-by: Benjamin Hahn > > please add a proper patch description. Adding only a subject line > ist not enough. > One idea that I had is when people are synching with mainline they could put the oneline summary of the stuff that's getting merged. In this case it's the 6 most recent commits. $ git log --oneline arch/arm64/boot/dts/freescale/imx8mp-phyboard-pollux-rdk.dts 4a58fcdb1818 arm64: dts: imx8mp-phyboard-pollux: Add support for RS232/RS485 3bd7fdcc359e arm64: dts: imx8mp-phyboard-pollux: Add gpio-line-names f5faa633daf8 arm64: dts: imx8mp-phyboard-pollux: Enable USB support 27c0dc128d04 arm64: dts: imx8mp-phyboard-pollux: Add flexcan support fa2a1ec50456 arm64: dts: imx8mp-phyboard-pollux: Add missing usdhc clocks assignment 055e38c76388 arm64: dts: imx8mp-phyboard-pollux-rdk: Fix led sub-node names regards, dan carpenter
Re: [PATCH] net: phy: ncsi: Correct the endian of the checksum
On Mon, Feb 05, 2024 at 04:02:28PM +0800, Jacky Chou wrote:
> There is no need to perform the endian twice here.
>
> Signed-off-by: Jacky Chou
Reviewed-by: Dan Carpenter
Fixes: f641a8ac93e0 ("phy: Add support for the NC-SI protocol")
How did this ever work? Was this always tested on big endian hardware
or was nothing verifying the checksums?
regards,
dan carpenter
Re: [PATCH] xyzModem: Correct xmodem blk verification conditions
On Mon, Feb 05, 2024 at 02:58:50PM +0800, Hongbin Ji wrote:
> When the blk sequence number is 255 and cblk is 0, the original XOR condition
> produces a result of 0,and the judgment condition will be unsuccessful.
>
This code is 18 years old so it's surprising that it's causing an issue
now. This was added in commit f2841d377060 ("Add support for ymodem
protocol (loady command). Patch by Stefano Babic, 29 Mar 2006").
Not just 255 and zero but any time "blk == 255 - cblk" then your new
condition will be true where the original condition was false. Is that
really intentional? Is this from reviewing documentation or something?
What documents? Or is it from testing?
regards,
dan carpenter
Re: [PATCH] net: macb: Add support for fixed link
On Sat, Feb 03, 2024 at 07:06:52PM +0100, belouargamoha...@gmail.com wrote:
> @@ -1276,6 +1297,30 @@ int __weak macb_late_eth_of_to_plat(struct udevice
> *dev)
> static int macb_eth_of_to_plat(struct udevice *dev)
> {
> struct eth_pdata *pdata = dev_get_plat(dev);
> + struct macb_device *macb = dev_get_priv(dev);
> + void *blob = (void *)gd->fdt_blob;
> + int node = dev_of_offset(dev);
> + int fl_node, speed_fdt;
> +
> + /* fetch 'fixed-link' property */
> + fl_node = fdt_subnode_offset(blob, node, "fixed-link");
> + if (fl_node != -FDT_ERR_NOTFOUND) {
Why not check for if (fl_node >= 0)? The fdt_subnode_offset() function
can return a variety of error codes.
regards,
dan carpenter
> + /* set phy_addr to invalid value for fixed link */
> + macb->phy_addr = PHY_MAX_ADDR + 1;
> + macb->duplex = fdtdec_get_bool(blob, fl_node, "full-duplex");
> + speed_fdt = fdtdec_get_int(blob, fl_node, "speed", 0);
> + if (speed_fdt == 100) {
> + macb->speed = 1;
> + }
> + else if (speed_fdt == 10) {
> + macb->speed = 0;
> + }
> + else {
> + printf("%s: The given speed %d of ethernet in the DT is
> not supported\n",
> + __func__, speed_fdt);
> + return -EINVAL;
> + }
> + }
>
Re: [PATCH 1/3] usb: dwc3: handle return value of clk_get_rate() correctly
On Sat, Feb 03, 2024 at 07:01:54AM +0800, Yang Xiwen via B4 Relay wrote: > From: Yang Xiwen > > clk_get_rate() return -ve on error, not 0. Fix it by replacing judging > NULL with IS_ERR_VALUE(). s/NULL/zero/. I'd be surprised if clk_get_rate() *never* returns zero. The Linux kernel's clk_get_rate() function returns zero and not error codes, btw. regards, dan carpenter
Re: [RFC PATCH v2 2/2] board: ad401: example of fastboot oem board realization
On Thu, Feb 01, 2024 at 12:20:27PM +0300, Alexey Romanov wrote:
> +static int fastboot_nand_write_tpl(struct mtd_info *mtd, void *buffer,
> +u32 offset, size_t size, int flags)
> +{
> + int boot_cpy_num = meson_bootloader_copy_num(BOOT_TPL);
> + u64 size_per_copy = meson_bootloader_copy_size(mtd, BOOT_TPL);
> + int i;
> +
> + for (i = 0; i < boot_cpy_num; i++) {
> + size_t retlen, len = size;
> + int ret;
> +
> + ret = nand_write_skip_bad(mtd, offset + (i * size_per_copy),
> + , , offset + size_per_copy,
^^
Sorry if I'm missing something obvious, but why is the limit
"offset + size_per_copy"? I would have expected it to be just
"size_per_copy" or maybe some kind of limit minus the offset...
> + buffer, flags);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
regards,
dan carpenter
Re: [PATCH 06/13] clk/qcom: sdm845: add USB clocks
On Wed, Jan 31, 2024 at 03:16:58PM +, Caleb Connolly wrote:
> @@ -121,6 +130,26 @@ static int sdm845_clk_enable(struct clk *clk)
>
> debug("%s: clk %s\n", __func__, sdm845_clks[clk->id].name);
>
> + switch (clk->id) {
> + case GCC_USB30_PRIM_MASTER_CLK:
> + gdsc_enable(priv->base + USB30_PRIM_GDSCR);
> + qcom_gate_clk_en(priv, GCC_USB_PHY_CFG_AHB2PHY_CLK);
> + /* These numbers are just pulled from the frequency tables in
> the Linux driver */
> + clk_rcg_set_rate_mnd(priv->base, USB30_PRIM_MASTER_CLK_CMD_RCGR,
> + (4.5 * 2) - 1, 0, 0, 1 << 8, 8);
> + clk_rcg_set_rate_mnd(priv->base,
> USB30_PRIM_MOCK_UTMI_CLK_CMD_RCGR,
> + 1, 0, 0, 0, 8);
> + clk_rcg_set_rate_mnd(priv->base, USB3_PRIM_PHY_AUX_CMD_RCGR,
> + 1, 0, 0, 0, 8);
> + case GCC_USB30_SEC_MASTER_CLK:
Is this supposed to break? Otherwise can we add a "fallthrough;"
annotation?
> + gdsc_enable(priv->base + USB30_SEC_GDSCR);
> + qcom_gate_clk_en(priv, GCC_USB3_SEC_PHY_AUX_CLK);
> +
> + qcom_gate_clk_en(priv, GCC_USB3_SEC_CLKREF_CLK);
> + qcom_gate_clk_en(priv, GCC_USB3_SEC_PHY_COM_AUX_CLK);
> + break;
> + }
regards,
dan carpenter
Re: [PATCH 07/13] gpio: msm_gpio: add .set_flags op
On Wed, Jan 31, 2024 at 03:16:59PM +, Caleb Connolly wrote:
> diff --git a/drivers/gpio/msm_gpio.c b/drivers/gpio/msm_gpio.c
> index 80cd28bb231f..0230305af299 100644
> --- a/drivers/gpio/msm_gpio.c
> +++ b/drivers/gpio/msm_gpio.c
> @@ -72,6 +72,23 @@ static int msm_gpio_direction_output(struct udevice *dev,
> unsigned int gpio,
> return 0;
> }
>
> +static int msm_gpio_set_flags(struct udevice *dev, unsigned int gpio, ulong
> flags)
> +{
> + if (flags & GPIOD_IS_OUT_ACTIVE) {
> + return msm_gpio_direction_output(dev, gpio, 1);
> + } else if (flags & GPIOD_IS_OUT) {
> + return msm_gpio_direction_output(dev, gpio, 0);
> + } else if (flags & GPIOD_IS_IN) {
> + return msm_gpio_direction_input(dev, gpio);
^^
> + if (flags & GPIOD_PULL_UP)
> + return msm_gpio_set_value(dev, gpio, 1);
> + else if (flags & GPIOD_PULL_DOWN)
> + return msm_gpio_set_value(dev, gpio, 0);
These lines are unreachable code.
> + }
> +
> + return 0;
> +}
regards,
dan carpenter
Re: [PATCH v2 2/3] driver: rng: Fix SMCCC TRNG crash
On Wed, Jan 31, 2024 at 02:48:58PM +0300, Dan Carpenter wrote:
> Etienne should have been on the CC list. He's in
> ./scripts/get_maintainer.pl so I'm not sure what went wrong there...
> I've added him.
>
Hm... Etienne's @linaro.org address bounces. Apparently MAINTAINERS
needs to be updated.
regards,
dan carpenter
> On Wed, Jan 31, 2024 at 11:12:16AM +, Weizhao Ouyang wrote:
> > Fix a SMCCC TRNG null pointer crash due to a failed smccc feature
> > binding.
> >
> > Reviewed-by: Heinrich Schuchardt
> > Signed-off-by: Weizhao Ouyang
> > ---
> > drivers/rng/smccc_trng.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/rng/smccc_trng.c b/drivers/rng/smccc_trng.c
> > index 3a4bb33941..3087cb991a 100644
> > --- a/drivers/rng/smccc_trng.c
> > +++ b/drivers/rng/smccc_trng.c
> > @@ -166,7 +166,7 @@ static int smccc_trng_probe(struct udevice *dev)
> > struct smccc_trng_priv *priv = dev_get_priv(dev);
> > struct arm_smccc_res res;
> >
> > - if (!(smccc_trng_is_supported(smccc->invoke_fn)))
> > + if (!smccc || !(smccc_trng_is_supported(smccc->invoke_fn)))
>
> To me it seems a bit strange that dm_priv_to_rw() can return NULL...
> Anyway, probably the Fixes tag should point to when the driver was
> added.
>
> Fixes: 53355bb86c25 ("drivers: rng: add smccc trng driver")
>
> regards,
> dan carpenter
Re: [PATCH v2 2/3] driver: rng: Fix SMCCC TRNG crash
Etienne should have been on the CC list. He's in
./scripts/get_maintainer.pl so I'm not sure what went wrong there...
I've added him.
On Wed, Jan 31, 2024 at 11:12:16AM +, Weizhao Ouyang wrote:
> Fix a SMCCC TRNG null pointer crash due to a failed smccc feature
> binding.
>
> Reviewed-by: Heinrich Schuchardt
> Signed-off-by: Weizhao Ouyang
> ---
> drivers/rng/smccc_trng.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/rng/smccc_trng.c b/drivers/rng/smccc_trng.c
> index 3a4bb33941..3087cb991a 100644
> --- a/drivers/rng/smccc_trng.c
> +++ b/drivers/rng/smccc_trng.c
> @@ -166,7 +166,7 @@ static int smccc_trng_probe(struct udevice *dev)
> struct smccc_trng_priv *priv = dev_get_priv(dev);
> struct arm_smccc_res res;
>
> - if (!(smccc_trng_is_supported(smccc->invoke_fn)))
> + if (!smccc || !(smccc_trng_is_supported(smccc->invoke_fn)))
To me it seems a bit strange that dm_priv_to_rw() can return NULL...
Anyway, probably the Fixes tag should point to when the driver was
added.
Fixes: 53355bb86c25 ("drivers: rng: add smccc trng driver")
regards,
dan carpenter
[bug report] lib: sparse: Make CHUNK_TYPE_RAW buffer aligned
Hello qianfan Zhao,
The patch 62649165cb02: "lib: sparse: Make CHUNK_TYPE_RAW buffer
aligned" from Nov 16, 2021, leads to the following Smatch static
checker warning:
lib/image-sparse.c:214 write_sparse_image()
warn: unsigned 'blks' is never less than zero.
lib/image-sparse.c
109 int write_sparse_image(struct sparse_storage *info,
110const char *part_name, void *data, char
*response)
111 {
112 lbaint_t blk;
113 lbaint_t blkcnt;
114 lbaint_t blks;
^
This is a u64
115 uint64_t bytes_written = 0;
116 unsigned int chunk;
117 unsigned int offset;
118 uint64_t chunk_data_sz;
119 uint32_t *fill_buf = NULL;
120 uint32_t fill_val;
121 sparse_header_t *sparse_header;
122 chunk_header_t *chunk_header;
123 uint32_t total_blocks = 0;
124 int fill_buf_num_blks;
125 int i;
126 int j;
127
128 fill_buf_num_blks = CONFIG_IMAGE_SPARSE_FILLBUF_SIZE /
info->blksz;
[ snip ]
211
212 blks = write_sparse_chunk_raw(info, blk, blkcnt,
213 data, response);
--> 214 if (blks < 0)
Can't be less than zero
215 return -1;
216
217 blk += blks;
218 bytes_written += ((u64)blkcnt) * info->blksz;
219 total_blocks += chunk_header->chunk_sz;
220 data += chunk_data_sz;
221 break;
222
223 case CHUNK_TYPE_FILL:
224 if (chunk_header->total_sz !=
regards,
dan carpenter
[PATCH] blk: host_dev: Fix error code in host_sb_attach_file()
This error path should return -EINVAL instead of success.
Fixes: e261fbf34785 ("blk: host_dev: Sanity check on the size of host backing
file")
Signed-off-by: Dan Carpenter
---
drivers/block/host_dev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/block/host_dev.c b/drivers/block/host_dev.c
index 30c74157934a..52313435a0cb 100644
--- a/drivers/block/host_dev.c
+++ b/drivers/block/host_dev.c
@@ -61,6 +61,7 @@ static int host_sb_attach_file(struct udevice *dev, const
char *filename)
if (size % desc->blksz) {
printf("The size of host backing file '%s' is not multiple of "
"the device block size\n", filename);
+ ret = -EINVAL;
goto err_fname;
}
desc->lba = size / desc->blksz;
--
2.43.0
[PATCH] power: regulator: Fix error code in regulator_list_autoset()
This condition has a bitwise & vs logical && typo so it only preserves
odd number error codes.
Fixes: 3b880757abca ("dm: regulator: uclass driver code cleanup")
Signed-off-by: Dan Carpenter
---
drivers/power/regulator/regulator-uclass.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/regulator/regulator-uclass.c
b/drivers/power/regulator/regulator-uclass.c
index 77d101f262e2..de2bb3b1cd88 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -390,7 +390,7 @@ int regulator_list_autoset(const char *list_platname[],
ret = regulator_autoset_by_name(list_platname[i], );
if (ret != -EMEDIUMTYPE && verbose)
regulator_show(dev, ret);
- if (ret & !error)
+ if (ret && !error)
error = ret;
if (list_devp)
--
2.43.0
[PATCH] button: qcom-pmic: fix some error checking
The pmic_reg_read() function can return errors. Add a check for that.
Fixes: 4e8aa0065d4b ("button: qcom-pmic: introduce Qualcomm PMIC button driver")
Signed-off-by: Dan Carpenter
---
This patch is mostly to make static checkers happy but it's obvious and
harmless.
drivers/button/button-qcom-pmic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/button/button-qcom-pmic.c
b/drivers/button/button-qcom-pmic.c
index 34a976d1e6c6..e778e51a4f36 100644
--- a/drivers/button/button-qcom-pmic.c
+++ b/drivers/button/button-qcom-pmic.c
@@ -86,7 +86,7 @@ static int qcom_pwrkey_probe(struct udevice *dev)
}
ret = pmic_reg_read(priv->pmic, priv->base + REG_SUBTYPE);
- if ((ret & 0x7) == 0) {
+ if (ret < 0 || (ret & 0x7) == 0) {
printf("%s: unexpected PMCI function subtype %d\n", dev->name,
ret);
return -ENXIO;
}
--
2.43.0
Re: [PATCH] fsl-layerscape/soc.c: do not destroy bootcmd environment
On Tue, Jan 30, 2024 at 03:26:56PM +0100, Mike Looijmans wrote:
> When an XXX_BOOTCOMMAND isn't defined, the result is that bootcmd is set
> to some random memory content. Fix it so that the function does nothing
> in that case and leaves the bootcmd environment unmodified.
>
> Signed-off-by: Mike Looijmans
Thanks, Mike.
Fixes: 2141d250f510 ("armv8: fsl-layerscape: bootcmd identification for
TFABOOT")
Reviewed-by: Dan Carpenter
regards,
dan carpenter
Re: [PATCH v3 14/36] board: dragonboard410c: upstream DT compat
On Tue, Jan 30, 2024 at 02:05:02PM +, Caleb Connolly wrote:
> @@ -119,6 +120,26 @@ static int ehci_usb_of_to_plat(struct udevice *dev)
> return 0;
> }
>
> +static int ehci_usb_of_bind(struct udevice *dev)
> +{
> + ofnode ulpi_node = ofnode_first_subnode(dev_ofnode(dev));
> + ofnode phy_node;
> +
> + if (!ofnode_valid(ulpi_node))
> + return 0;
> +
> + phy_node = ofnode_first_subnode(ulpi_node);
> + if (!ofnode_valid(phy_node)) {
> + printf("%s: ulpi subnode with no phy\n", __func__);
> + return -ENOENT;
> + }
> +
> + return device_bind_driver_to_node(dev, "msm8916_usbphy",
> "msm8916_usbphy",
> + phy_node, NULL);
> +
> + return 0;
Delete the extra return 0.
> +}
regards,
dan carpenter
Re: [PATCH v3 03/36] mmc: msm_sdhci: use modern clock handling
On Tue, Jan 30, 2024 at 02:04:51PM +, Caleb Connolly wrote:
> +
> + /* The clock is already enabled by the clk_bulk above */
> + ret = clk_set_rate(>clks.clks[i], clk_rate);
> + if (!ret) {
> + printf("Couldn't set core clock rate: %d\n", ret);
> + return -EINVAL;
The if statement looks reversed. Or if we want clk_set_rate() to fail
then there isn't a reason to print "ret" in the error message because
we know that's zero.
> + }
>
> return 0;
> }
regards,
dan carpenter
Re: [PATCH v10 1/8] mtd: spi-nor: Add parallel and stacked memories support
On Tue, Jan 30, 2024 at 10:14:16AM +0530, Venkatesh Yadav Abbarapu wrote:
> @@ -1444,28 +1465,66 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> {
> struct spi_nor *nor = mtd_to_spi_nor(mtd);
> int ret;
> + u32 offset = from;
Declare offset as loff_t otherwise it's limited to 4GB. I don't know if
we support more than 4GB here but it's just weird and forces us to add
casts later...
> + u32 stack_shift = 0;
This variable is never used.
> + u32 read_len = 0;
> + u32 rem_bank_len = 0;
> + u8 bank;
> + u8 is_ofst_odd = 0;
If you wanted, you could make this bool true/false.
>
> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
>
> + if ((nor->flags & SNOR_F_HAS_PARALLEL) && (offset & 1)) {
> + /* We can hit this case when we use file system like ubifs */
> + from = (loff_t)(from - 1);
> + len = (size_t)(len + 1);
These casts are unnecessary. Just write:
from--;
len++;
> + is_ofst_odd = 1;
> + }
> +
> while (len) {
> - loff_t addr = from;
> - size_t read_len = len;
> + if (nor->addr_width == 3) {
> + if (nor->flags & SNOR_F_HAS_PARALLEL) {
> + bank = (u32)from / (SZ_16M << 0x01);
> + rem_bank_len = ((SZ_16M << 0x01) *
> + (bank + 1)) - from;
> + } else {
> + bank = (u32)from / SZ_16M;
> + rem_bank_len = (SZ_16M * (bank + 1)) - from;
> + }
> + }
If nor->addr_width != 3 then rem_bank_len is zero and I think we're
going to have a problem...
> + offset = from;
> +
> + if (nor->flags & SNOR_F_HAS_STACKED) {
> + stack_shift = 1;
> + if (offset >= (mtd->size / 2)) {
> + offset = offset - (mtd->size / 2);
> + nor->spi->flags |= SPI_XFER_U_PAGE;
> + } else {
> + nor->spi->flags &= ~SPI_XFER_U_PAGE;
> + }
> + }
>
> -#ifdef CONFIG_SPI_FLASH_BAR
> - u32 remain_len;
> + if (nor->flags & SNOR_F_HAS_PARALLEL)
> + offset /= 2;
>
> - ret = write_bar(nor, addr);
> - if (ret < 0)
> - return log_ret(ret);
> - remain_len = (SZ_16M * (nor->bank_curr + 1)) - addr;
> + if (nor->addr_width == 3) {
> +#ifdef CONFIG_SPI_FLASH_BAR
> + ret = write_bar(nor, offset);
> + if (ret < 0)
> + return log_ret(ret);
> +#endif
> + }
>
> - if (len < remain_len)
> + if (len < rem_bank_len)
> read_len = len;
> else
> - read_len = remain_len;
> -#endif
> + read_len = rem_bank_len;
If rem_bank_len is zero then read_len is zero.
> +
> + ret = spi_nor_wait_till_ready(nor);
> + if (ret)
> + goto read_err;
>
> - ret = nor->read(nor, addr, read_len, buf);
> + ret = nor->read(nor, offset, read_len, buf);
> if (ret == 0) {
> /* We shouldn't see 0-length reads */
> ret = -EIO;
When read_len is zero we return -EIO.
> @@ -1474,8 +1533,15 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> if (ret < 0)
> goto read_err;
>
> - *retlen += ret;
> - buf += ret;
> + if (is_ofst_odd == 1) {
> + memcpy(buf, (buf + 1), (len - 1));
This needs to be memmove(). memcpy() is undefined for overlapped
buffers.
> + *retlen += (ret - 1);
> + buf += ret - 1;
> + is_ofst_odd = 0;
> + } else {
> + *retlen += ret;
> + buf += ret;
> + }
> from += ret;
> len -= ret;
> }
regards,
dan carpenter
[bug report] cmd: mbr: Allow 4 MBR partitions without need for extended
Hello Alexander Gendin,
The patch 04291ee0aba6: "cmd: mbr: Allow 4 MBR partitions without
need for extended" from Oct 9, 2023, leads to the following Smatch
static checker warning:
test/cmd/mbr.c:243 mbr_test_run()
warn: sizeof(NUMBER)?
test/cmd/mbr.c
233 ulong mbr_wa, ebr_wa, ra, ebr_blk, mbr_parts_max;
234 struct udevice *dev;
235 ofnode root, node;
236
237 /* Enable the mmc6 node for this test */
238 root = oftree_root(oftree_default());
239 node = ofnode_find_subnode(root, "mmc6");
240 ut_assert(ofnode_valid(node));
241 ut_assertok(lists_bind_fdt(gd->dm_root, node, , NULL,
false));
242
--> 243 mbr_parts_max = sizeof('\0') + 2 +
^^^
You probably wanted this to a readable way to explain that we are
leaving space for the NUL char, however this size is 4 not 1. (I guess
this is from type promotion?)
244 strlen(mbr_parts_header) +
245 strlen(mbr_parts_p1) +
246 strlen(mbr_parts_p2) +
247 strlen(mbr_parts_p3) +
248 max(strlen(mbr_parts_p4), strlen(mbr_parts_p5)) +
249 strlen(mbr_parts_tail);
250 ut_assertf(sizeof(mbr_parts_buf) >= mbr_parts_max, "Buffer
avail: %ld; buffer req: %ld\n",
251 sizeof(mbr_parts_buf), mbr_parts_max);
252
regards,
dan carpenter
Re: [PATCH v1 0/1] Fix booting kernels with ATAGS and extlinux
On Wed, Jan 24, 2024 at 10:27:30PM +0200, Svyatoslav Ryhel wrote: > Currently, if boot with extlinux.conf and do not set the fdt > U-Boot will provide its own device tree. This behavior is > beneficial if the U-Boot device tree is in sync with Linux, > but it totally halts the booting of pre-dtb kernels (3.4 for > example) since it uses ATAGs. To fix this, pass `-` in the > fdt extlinux field as a signal that no tree should be used. Passing - doesn't seem like the most intuitive thing... Is there a different argument we could use? regards, dan carpenter
Re: [RFC PATCH v3 14/15] board: ti: am65x: Add check for k3-am654-icssg2 in board_fit_config_match()
On Wed, Jan 24, 2024 at 11:27:12AM +0300, Dan Carpenter wrote:
> On Wed, Jan 24, 2024 at 12:19:29PM +0530, MD Danish Anwar wrote:
> > When CONFIG_TI_ICSSG_PRUETH is enabled, add config name check for the
> > icssg2 overlay in board_fit_config_match() API.
> >
> > Signed-off-by: MD Danish Anwar
> > ---
> > board/ti/am65x/evm.c | 11 +++
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/board/ti/am65x/evm.c b/board/ti/am65x/evm.c
> > index df209021c1..0b661f0084 100644
> > --- a/board/ti/am65x/evm.c
> > +++ b/board/ti/am65x/evm.c
> > @@ -90,10 +90,13 @@ int dram_init_banksize(void)
> > #ifdef CONFIG_SPL_LOAD_FIT
> > int board_fit_config_name_match(const char *name)
> > {
> > -#ifdef CONFIG_TARGET_AM654_A53_EVM
> > - if (!strcmp(name, "k3-am654-base-board"))
> > - return 0;
> > -#endif
> > + if (IS_ENABLED(CONFIG_TI_ICSSG_PRUETH)) {
> > + if (!strcmp(name, "k3-am654-icssg2"))
> > + return 0;
> > + } else {
> > + if (!strcmp(name, "k3-am654-base-board"))
> > + return 0;
> > + }
> >
> > return -1;
> > }
>
> It probably should support both configs being enabled.
>
> if (IS_ENABLED(CONFIG_TI_ICSSG_PRUETH) &&
> strcmp(name, "k3-am654-icssg2") == 0)
> return 0;
>
> if (IS_ENABLED(TARGET_AM654_A53_EVM) &&
> strcmp(name, "k3-am654-base-board"))
I reversed this strcmp()...
strcmp(name, "k3-am654-base-board") == 0)
regards,
dan carpenter
Re: [RFC PATCH v3 14/15] board: ti: am65x: Add check for k3-am654-icssg2 in board_fit_config_match()
On Wed, Jan 24, 2024 at 12:19:29PM +0530, MD Danish Anwar wrote:
> When CONFIG_TI_ICSSG_PRUETH is enabled, add config name check for the
> icssg2 overlay in board_fit_config_match() API.
>
> Signed-off-by: MD Danish Anwar
> ---
> board/ti/am65x/evm.c | 11 +++
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/board/ti/am65x/evm.c b/board/ti/am65x/evm.c
> index df209021c1..0b661f0084 100644
> --- a/board/ti/am65x/evm.c
> +++ b/board/ti/am65x/evm.c
> @@ -90,10 +90,13 @@ int dram_init_banksize(void)
> #ifdef CONFIG_SPL_LOAD_FIT
> int board_fit_config_name_match(const char *name)
> {
> -#ifdef CONFIG_TARGET_AM654_A53_EVM
> - if (!strcmp(name, "k3-am654-base-board"))
> - return 0;
> -#endif
> + if (IS_ENABLED(CONFIG_TI_ICSSG_PRUETH)) {
> + if (!strcmp(name, "k3-am654-icssg2"))
> + return 0;
> + } else {
> + if (!strcmp(name, "k3-am654-base-board"))
> + return 0;
> + }
>
> return -1;
> }
It probably should support both configs being enabled.
if (IS_ENABLED(CONFIG_TI_ICSSG_PRUETH) &&
strcmp(name, "k3-am654-icssg2") == 0)
return 0;
if (IS_ENABLED(TARGET_AM654_A53_EVM) &&
strcmp(name, "k3-am654-base-board"))
return 0;
return -1;
regards,
dan carpenter
[PATCH] btrfs: fix some error checking for btrfs_decompress()
The btrfs_decompress() function mostly (u32)-1 on error but it can
also return -EPERM or other kernel error codes from zstd_decompress().
The "ret" variable is an int, so we could just check for negatives.
Signed-off-by: Dan Carpenter
---
fs/btrfs/inode.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 38e285bf94b0..4691612eda33 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -390,7 +390,7 @@ int btrfs_read_extent_inline(struct btrfs_path *path,
csize);
ret = btrfs_decompress(btrfs_file_extent_compression(leaf, fi),
cbuf, csize, dbuf, dsize);
- if (ret == (u32)-1) {
+ if (ret < 0) {
ret = -EIO;
goto out;
}
@@ -500,7 +500,7 @@ int btrfs_read_extent_reg(struct btrfs_path *path,
ret = btrfs_decompress(btrfs_file_extent_compression(leaf, fi), cbuf,
csize, dbuf, dsize);
- if (ret == (u32)-1) {
+ if (ret < 0) {
ret = -EIO;
goto out;
}
--
2.39.2
Re: [PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()
On Tue, Aug 01, 2023 at 11:05:11AM +0200, Marek Behún wrote: > nice catch :) Dan, I always wanted to ask, since I've seen many such > "nice catches" over different subsystems from you. Do you write some > tools to find these? Or do you use coccinelle, or something? > Thanks! I'm using Smatch. https://github.com/error27/smatch/ It's handy for me that u-boot and the Linux kernel have so much in common and I can re-use a the kernel checks. I had to make some changes to Smatch to make it work but those are pushed now. U-boot is something that Linaro cares about so if you have static checker ideas then feel free to email the smatch mailing list. sma...@vger.kernel.org regards, dan carpenter
[PATCH] test: fix a couple NULL vs IS_ERR() checks
The x509_cert_parse() and pkcs7_parse_message() functions return error pointers. They don't return NULL. Update the checks accordingly. Signed-off-by: Dan Carpenter --- test/lib/asn1.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/lib/asn1.c b/test/lib/asn1.c index 8661fdd30687..a66cdd77df0a 100644 --- a/test/lib/asn1.c +++ b/test/lib/asn1.c @@ -120,7 +120,7 @@ static int lib_asn1_x509(struct unit_test_state *uts) cert = x509_cert_parse(cert_data, cert_data_len); - ut_assertf(cert != NULL, "decoding failed\n"); + ut_assertf(!IS_ERR(cert), "decoding failed\n"); ut_assertf(!strcmp(cert->subject, "Linaro: Tester"), "subject doesn't match\n"); ut_assertf(!strcmp(cert->issuer, "Linaro: Tester"), @@ -313,7 +313,7 @@ static int lib_asn1_pkcs7(struct unit_test_state *uts) pkcs7 = pkcs7_parse_message(image_pk7, image_pk7_len); - ut_assertf(pkcs7 != NULL, "decoding failed\n"); + ut_assertf(!IS_ERR(pkcs7), "decoding failed\n"); ut_assertf(pkcs7->data_len == 104, "signature size doesn't match\n"); ut_assertf(pkcs7->signed_infos != NULL, "sign-info doesn't exist\n"); ut_assertf(pkcs7->signed_infos->msgdigest_len == 32, -- 2.39.2
[PATCH] expo: allocate correct ammount of memory
This should be allocating the memory for "item" instead of "menu".
The item struct is 48 bytes instead of 96 (assuming a 64bit system)
so this saves a little memory.
Signed-off-by: Dan Carpenter
---
boot/scene_menu.c | 2 +-
1 file changed, 1 insertions(+), 1 deletions(-)
diff --git a/boot/scene_menu.c b/boot/scene_menu.c
index 8a355f838cc8..57ffb523ff3f 100644
--- a/boot/scene_menu.c
+++ b/boot/scene_menu.c
@@ -416,7 +416,7 @@ int scene_menuitem(struct scene *scn, uint menu_id, const
char *name, uint id,
if (!scene_obj_find(scn, label_id, SCENEOBJT_TEXT))
return log_msg_ret("txt", -EINVAL);
- item = calloc(1, sizeof(struct scene_obj_menu));
+ item = calloc(1, sizeof(struct scene_menitem));
if (!item)
return log_msg_ret("item", -ENOMEM);
item->name = strdup(name);
--
2.39.2
[PATCH] cmd: improve string matching for hex
Match the "=0x" instead of just "=0".
Signed-off-by: Dan Carpenter
---
We sometimes two character partial matching for commands so people can
type "re" instead of "read". But here reading two characters doesn't
seem correct.
cmd/nvedit_efi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/nvedit_efi.c b/cmd/nvedit_efi.c
index 24944ab81e23..7a30b5cc8f87 100644
--- a/cmd/nvedit_efi.c
+++ b/cmd/nvedit_efi.c
@@ -262,7 +262,7 @@ static int append_value(char **bufp, size_t *sizep, char
*data)
char *tmp_buf = NULL, *new_buf = NULL, *value;
unsigned long len = 0;
- if (!strncmp(data, "=0x", 2)) { /* hexadecimal number */
+ if (!strncmp(data, "=0x", 3)) { /* hexadecimal number */
union {
u8 u8;
u16 u16;
--
2.39.2
Re: [PATCH] addrmap: Fix off by one in addrmap_set_entry()
On Wed, Jul 26, 2023 at 06:49:44PM -0600, Simon Glass wrote:
> Hi Dan,
>
> On Tue, 25 Jul 2023 at 09:40, Dan Carpenter wrote:
> >
> > The > comparison needs to be changed to >= to prevent an out of bounds
> > write on th next line.
> >
> > Signed-off-by: Dan Carpenter
> > ---
> > lib/addr_map.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/addr_map.c b/lib/addr_map.c
> > index 9b3e0a544e47..86e932e4b561 100644
> > --- a/lib/addr_map.c
> > +++ b/lib/addr_map.c
> > @@ -59,7 +59,7 @@ void *addrmap_phys_to_virt(phys_addr_t paddr)
> > void addrmap_set_entry(unsigned long vaddr, phys_addr_t paddr,
> > phys_size_t size, int idx)
> > {
> > - if (idx > CONFIG_SYS_NUM_ADDR_MAP)
> > + if (idx >= CONFIG_SYS_NUM_ADDR_MAP)
> > return;
>
> It looks like this function should return an error.
>
If we hit this error path there probably isn't a reasonable way to
recover. Maybe we could add a WARN()?
regards,
dan carpenter
Re: [PATCH v2] efi_loader: Fix memory corruption on 32bit systems
On Thu, Jul 27, 2023 at 11:22:15AM +0300, Ilias Apalodimas wrote:
> Hi Dan,
>
> [...]
>
> > @@ -313,7 +313,7 @@ static int cmp_pe_section(const void *arg1, const void
> > *arg2)
> > *
> > * Return: valid pointer to a image, return NULL if allocation fails.
> > */
> > -void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
> > +void *efi_prepare_aligned_image(void *efi, size_t *efi_size)
> > {
> > size_t new_efi_size;
> > void *new_efi;
> > @@ -600,7 +600,7 @@ static bool efi_image_authenticate(void *efi, size_t
> > efi_size)
> > if (!efi_secure_boot_enabled())
> > return true;
> >
> > - new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
> > + new_efi = efi_prepare_aligned_image(efi, _size);
> > if (!new_efi)
> > return false;
> >
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 49f8a5e77cbf..d57afd0c498b 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -882,7 +882,7 @@ out:
> > *
> > * Return: status code
> > */
> > -static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
> > +static efi_status_t tcg2_hash_pe_image(void *efi, size_t efi_size,
> >struct tpml_digest_values *digest_list)
>
> Unfortunately the rabbit hole is a bit deeper with this one.
> tcg2_hash_pe_image() is called in
> - tcg2_measure_pe_image(). This one is called in efi_load_pe() and the type
> is indeed a size_t there, so that's fine
> - efi_tcg2_hash_log_extend_event(), this one is different...
> The function is described by the EFI spec [0] which mandates a u64... I
> think that was the reason efi_prepare_aligned_image() is using a u64 to
> begin with. This one uses the size only though not the pointer, but in a
> 32bit platform it would truncate s size > UINT_MAX.
>
> [0]
> https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
I have maybe misread something... I don't think this is a real issue.
32bit systems aren't going to be able to allocate that much memory
anyway. Also there are a lot of size_t parameters already so it's not
a new issue.
regards,
dan carpenter
Re: [PATCH] efi_loader: fix an IS_ERR() vs NULL check
On Thu, Jul 27, 2023 at 11:28:52AM +0300, Ilias Apalodimas wrote:
> Hi Dan, Heinrich
>
> On Thu, 27 Jul 2023 at 11:25, Heinrich Schuchardt wrote:
> >
> > On 7/27/23 09:16, Dan Carpenter wrote:
> > > The efi_parse_pkcs7_header() function returns NULL on error so the check
> > > for IS_ERR() should be changed to a NULL check.
> > >
> > > Signed-off-by: Dan Carpenter
> > > ---
> > > lib/efi_loader/efi_capsule.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 7a6f195cbc02..c98cff812f10 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void
> > > *capsule, efi_uintn_t capsule_s
> > >
> > > auth_hdr->auth_info.hdr.dwLength
> > >- sizeof(auth_hdr->auth_info),
> > >);
> > > - if (IS_ERR(capsule_sig)) {
> > > + if (!capsule_sig) {
> >
> > Thanks for addressing this bug.
> >
> > Can we get rid of all the IS_ERR() in efi_capsule_authenticate() and use
> > IS_ERR_OR_NULL() here?
> >
> > Best regards
>
> Heinrich is right here. This seems more fundamentally broken as
> efi_parse_pkcs7_header() can return an IS_ERR() or NULL...
No, it can only return NULL. That's also how the function is
documented.
* Return: Pointer to pkcs7_message structure on success, NULL on error
regards,
dan carpenter
Re: [PATCH] efi_loader: fix an IS_ERR() vs NULL check
On Thu, Jul 27, 2023 at 10:25:55AM +0200, Heinrich Schuchardt wrote:
> On 7/27/23 09:16, Dan Carpenter wrote:
> > The efi_parse_pkcs7_header() function returns NULL on error so the check
> > for IS_ERR() should be changed to a NULL check.
> >
> > Signed-off-by: Dan Carpenter
> > ---
> > lib/efi_loader/efi_capsule.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 7a6f195cbc02..c98cff812f10 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void
> > *capsule, efi_uintn_t capsule_s
> > auth_hdr->auth_info.hdr.dwLength
> > - sizeof(auth_hdr->auth_info),
> > );
> > - if (IS_ERR(capsule_sig)) {
> > + if (!capsule_sig) {
>
> Thanks for addressing this bug.
>
> Can we get rid of all the IS_ERR() in efi_capsule_authenticate() and use
> IS_ERR_OR_NULL() here?
Ugh... Please don't do that. In u-boot it probably doesn't matter but
in the kernel they mean very different things. A NULL in that context
means the feature is optional and disabled deliberately but an error
pointer means there was an error.
int blink_LEDS()
{
led = get_leds();
if (IS_ERR_OR_NULL(led)) {
if (IS_ERR(led))
print("LEDs are dead\n");
return PTR_ERR(led); <-- returns success if led is NULL
}
return led->blink();
}
So checking efi_capsule_authenticate() for IS_ERR_OR_NULL() means it's
optional and more options means more complications.
regards,
dan carpenter
[bug report] bootcount: add a new driver with syscon as backend
Hello Nandor Han,
The patch c50b21b70523: "bootcount: add a new driver with syscon as
backend" from Jun 10, 2021 , leads to the following Smatch static
checker warning:
drivers/bootcount/bootcount_syscon.c:56 bootcount_syscon_set()
warn: double left shift '(regval & 65535) << ((priv->size) << 3)'
drivers/bootcount/bootcount_syscon.c
44 static int bootcount_syscon_set(struct udevice *dev, const u32 val)
45 {
46 struct bootcount_syscon_priv *priv = dev_get_priv(dev);
47 u32 regval;
48
49 if ((val & priv->magic_mask) != 0)
50 return -EINVAL;
51
52 regval = (priv->magic & priv->magic_mask) | (val &
~priv->magic_mask);
53
54 if (priv->size == 2) {
55 regval &= 0x;
--> 56 regval |= (regval & 0x) << BYTES_TO_BITS(priv->size);
I don't understand what's going on here but it doesn't look correct.
The 0x mask is a no-op because we already masked it on the previous
line. priv->size is either 2 or 4. So in bits that's 16 or 32.
But regval is a u32 and so shifting by 32 is undefined.
57 }
58
59 debug("%s: Prepare to write reg value: 0x%08x with register
mask: 0x%08x\n",
60 __func__, regval, priv->reg_mask);
61
62 return regmap_update_bits(priv->regmap, priv->reg_addr,
priv->reg_mask,
63 regval);
64 }
regards,
dan carpenter
[PATCH] efi_loader: fix an IS_ERR() vs NULL check
The efi_parse_pkcs7_header() function returns NULL on error so the check
for IS_ERR() should be changed to a NULL check.
Signed-off-by: Dan Carpenter
---
lib/efi_loader/efi_capsule.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 7a6f195cbc02..c98cff812f10 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -368,9 +368,8 @@ efi_status_t efi_capsule_authenticate(const void *capsule,
efi_uintn_t capsule_s
auth_hdr->auth_info.hdr.dwLength
- sizeof(auth_hdr->auth_info),
);
- if (IS_ERR(capsule_sig)) {
+ if (!capsule_sig) {
debug("Parsing variable's pkcs7 header failed\n");
- capsule_sig = NULL;
goto out;
}
--
2.39.2
[PATCH] efi_loader: fix uninitialized variable bug in efi_set_load_options()
Check for efi_search_protocol() failure before dereferencing "handler" to avoid a crash. Signed-off-by: Dan Carpenter --- lib/efi_loader/efi_load_options.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/efi_loader/efi_load_options.c b/lib/efi_loader/efi_load_options.c index 3cfddee014e9..5f62184da1cd 100644 --- a/lib/efi_loader/efi_load_options.c +++ b/lib/efi_loader/efi_load_options.c @@ -31,10 +31,10 @@ efi_status_t efi_set_load_options(efi_handle_t handle, efi_status_t ret; ret = efi_search_protocol(handle, _guid_loaded_image, ); - loaded_image_info = handler->protocol_interface; if (ret != EFI_SUCCESS) return EFI_INVALID_PARAMETER; + loaded_image_info = handler->protocol_interface; loaded_image_info->load_options = load_options; loaded_image_info->load_options_size = load_options_size; -- 2.39.2
[PATCH] cramfs: clean up some error messages
This line break is not done correctly. We don't want to have all those
tabs in the printed output.
Signed-off-by: Dan Carpenter
---
fs/cramfs/cramfs.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/cramfs/cramfs.c b/fs/cramfs/cramfs.c
index 6c017cebc50f..abb2de34eb05 100644
--- a/fs/cramfs/cramfs.c
+++ b/fs/cramfs/cramfs.c
@@ -166,8 +166,7 @@ static unsigned long cramfs_resolve (unsigned long begin,
unsigned long offset,
unsigned long ret;
char *link;
if (p && strlen(p)) {
- printf ("unsupported symlink to \
-non-terminal path\n");
+ printf ("unsupported symlink to
non-terminal path\n");
return 0;
}
link = cramfs_uncompress_link (begin,
@@ -177,8 +176,7 @@ static unsigned long cramfs_resolve (unsigned long begin,
unsigned long offset,
namelen, namelen, name);
return 0;
} else if (link[0] == '/') {
- printf ("unsupported symlink to \
-absolute path\n");
+ printf ("unsupported symlink to
absolute path\n");
free (link);
return 0;
}
--
2.39.2
[PATCH] test: unicode: fix a sizeof() vs ARRAY_SIZE() bug
The u16_strlcat() is in units of u16 not bytes. So the limit needs to be ARRAY_SIZE() instead of sizeof(). Signed-off-by: Dan Carpenter --- test/unicode_ut.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unicode_ut.c b/test/unicode_ut.c index b27d7116b9ee..62ff5d10bf94 100644 --- a/test/unicode_ut.c +++ b/test/unicode_ut.c @@ -807,12 +807,12 @@ static int unicode_test_u16_strlcat(struct unit_test_state *uts) /* dest and src are empty string */ memset(buf, 0, sizeof(buf)); - ret = u16_strlcat(buf, _src, sizeof(buf)); + ret = u16_strlcat(buf, _src, ARRAY_SIZE(buf)); ut_asserteq(1, ret); /* dest is empty string */ memset(buf, 0, sizeof(buf)); - ret = u16_strlcat(buf, src, sizeof(buf)); + ret = u16_strlcat(buf, src, ARRAY_SIZE(buf)); ut_asserteq(5, ret); ut_assert(!unicode_test_u16_strcmp(buf, src, 40)); @@ -820,7 +820,7 @@ static int unicode_test_u16_strlcat(struct unit_test_state *uts) memset(buf, 0xCD, (sizeof(buf) - sizeof(u16))); buf[39] = 0; memcpy(buf, dest, sizeof(dest)); - ret = u16_strlcat(buf, _src, sizeof(buf)); + ret = u16_strlcat(buf, _src, ARRAY_SIZE(buf)); ut_asserteq(6, ret); ut_assert(!unicode_test_u16_strcmp(buf, dest, 40)); -- 2.39.2
[PATCH] cmd: pxe_utils: add some missing tabs
These lines are supposed to be indented one more tab. Otherwise it's confusing to read. Signed-off-by: Dan Carpenter --- boot/pxe_utils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c index d13c47dd9429..ac1414a5f26d 100644 --- a/boot/pxe_utils.c +++ b/boot/pxe_utils.c @@ -702,8 +702,8 @@ static int label_boot(struct pxe_context *ctx, struct pxe_label *label) } } - if (label->kaslrseed) - label_boot_kaslrseed(); + if (label->kaslrseed) + label_boot_kaslrseed(); #ifdef CONFIG_OF_LIBFDT_OVERLAY if (label->fdtoverlays) -- 2.39.2
[PATCH v2] efi_loader: Fix memory corruption on 32bit systems
The issue is this line:
new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
The efi_size variable is type size_t and on a 32 bit system that's 32
bits. The u64 type is obviously 64 bits. So we write 8 bytes to a 4
byte buffer which corrupts memory.
Fix this by changing the type of efi_prepare_aligned_image() to a
size_t pointer.
Signed-off-by: Dan Carpenter
---
v2: Change efi_prepare_aligned_image() instead of changing
efi_image_authenticate(). This is a cleaner way to fix the problem.
include/efi_loader.h | 2 +-
lib/efi_loader/efi_image_loader.c | 4 ++--
lib/efi_loader/efi_tcg2.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index b5fa0fe01ded..9c1a9ed16af6 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -1022,7 +1022,7 @@ bool efi_secure_boot_enabled(void);
bool efi_capsule_auth_enabled(void);
-void *efi_prepare_aligned_image(void *efi, u64 *efi_size);
+void *efi_prepare_aligned_image(void *efi, size_t *efi_size);
bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
WIN_CERTIFICATE **auth, size_t *auth_len);
diff --git a/lib/efi_loader/efi_image_loader.c
b/lib/efi_loader/efi_image_loader.c
index 26df0da16c93..64980008403b 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -313,7 +313,7 @@ static int cmp_pe_section(const void *arg1, const void
*arg2)
*
* Return: valid pointer to a image, return NULL if allocation fails.
*/
-void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
+void *efi_prepare_aligned_image(void *efi, size_t *efi_size)
{
size_t new_efi_size;
void *new_efi;
@@ -600,7 +600,7 @@ static bool efi_image_authenticate(void *efi, size_t
efi_size)
if (!efi_secure_boot_enabled())
return true;
- new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
+ new_efi = efi_prepare_aligned_image(efi, _size);
if (!new_efi)
return false;
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 49f8a5e77cbf..d57afd0c498b 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -882,7 +882,7 @@ out:
*
* Return: status code
*/
-static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
+static efi_status_t tcg2_hash_pe_image(void *efi, size_t efi_size,
struct tpml_digest_values *digest_list)
{
WIN_CERTIFICATE *wincerts = NULL;
--
2.39.2
Re: [PATCH] efi_loader: Fix memory corruption on 32bit systems
On Wed, Jul 26, 2023 at 10:11:04AM +0300, Ilias Apalodimas wrote: > Hi Dan > > On Wed, 26 Jul 2023 at 09:55, Dan Carpenter wrote: > > > > It's pretty unlikely that anyone is going to be using EFI authentication > > on a 32bit system. However, if you did, the efi_prepare_aligned_image() > > function would write 8 bytes of data to the _size variable and it > > can only hold 4 bytes so that corrupts memory. > > > > Signed-off-by: Dan Carpenter > > --- > > lib/efi_loader/efi_image_loader.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/lib/efi_loader/efi_image_loader.c > > b/lib/efi_loader/efi_image_loader.c > > index 26df0da16c93..3d5eef7dc3c2 100644 > > --- a/lib/efi_loader/efi_image_loader.c > > +++ b/lib/efi_loader/efi_image_loader.c > > @@ -592,6 +592,7 @@ static bool efi_image_authenticate(void *efi, size_t > > efi_size) > > struct efi_signature_store *db = NULL, *dbx = NULL; > > void *new_efi = NULL; > > u8 *auth, *wincerts_end; > > + u64 size = efi_size; > > size_t auth_size; > > bool ret = false; > > > > @@ -600,11 +601,11 @@ static bool efi_image_authenticate(void *efi, size_t > > efi_size) > > if (!efi_secure_boot_enabled()) > > return true; > > > > - new_efi = efi_prepare_aligned_image(efi, (u64 *)_size); > > Can't we change the prototype to access a size_t instead? Yep, you're right. That's a better fix. Will do. regards, dan carpenter
[bug report] fs: fat: create correct short names
Hello Heinrich Schuchardt,
The patch 28cef9ca2e86: "fs: fat: create correct short names" from
Nov 20, 2020, leads to the following Smatch static
checker warning:
fs/fat/fat_write.c:61 str2fat()
warn: impossible condition '(c > 127) => ((-128)-127 > 127)'
fs/fat/fat_write.c:136 set_name()
warn: impossible condition '(*dirent.name == 229) => ((-128)-127 == 229)'
fs/fat/fat_write.c
105 static int set_name(fat_itr *itr, const char *filename, char *shortname)
106 {
107 char *period;
108 char *pos;
109 int period_location;
110 char buf[13];
111 int i;
112 int ret;
113 struct nameext dirent;
114
115 if (!filename)
116 return -EIO;
117
118 /* Initialize buffer */
119 memset(, ' ', sizeof(dirent));
120
121 /* Convert filename to upper case short name */
122 period = strrchr(filename, '.');
123 pos = (char *)filename;
124 if (*pos == '.') {
125 pos = period + 1;
126 period = 0;
127 }
128 if (period)
129 str2fat(dirent.ext, period + 1, sizeof(dirent.ext));
130 period_location = str2fat(dirent.name, pos,
sizeof(dirent.name));
131 if (period_location < 0)
132 return period_location;
133 if (*dirent.name == ' ')
134 *dirent.name = '_';
135 /* 0xe5 signals a deleted directory entry. Replace it by 0x05.
*/
--> 136 if (*dirent.name == 0xe5)
dirent.name is a char type. Chars are signed on probably most people's
systems. The Linux kernel recently made char unsigned for everyone.
Anyway in a char 0xe5 is negative but as a literal, it's positive so
this condition cannot be true.
137 *dirent.name = 0x05;
138
139 /* If filename and short name are the same, quit. */
140 sprintf(buf, "%.*s.%.3s", period_location, dirent.name,
dirent.ext);
141 if (!strcmp(buf, filename)) {
142 ret = 1;
regards,
dan carpenter
[PATCH] regulator: preserve error code properly in regulator_list_autoset()
This code has a & vs && typo so it only preserves odd value error codes and not even value error codes. Signed-off-by: Dan Carpenter --- drivers/power/regulator/regulator-uclass.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index 3a6ba69f6d5f..52dd1bd3eafd 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -380,7 +380,7 @@ int regulator_list_autoset(const char *list_platname[], ret = regulator_autoset_by_name(list_platname[i], ); if (ret != -EMEDIUMTYPE && verbose) regulator_show(dev, ret); - if (ret & !error) + if (ret && !error) error = ret; if (list_devp) -- 2.39.2
[PATCH] remoteproc: uclass: Clean up a return
We know that "pa" is non-NULL so it's nicer to just return zero instead
of return !pa. This has no effect on runtime behavior.
Signed-off-by: Dan Carpenter
---
drivers/remoteproc/rproc-uclass.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/remoteproc/rproc-uclass.c
b/drivers/remoteproc/rproc-uclass.c
index 50bcc9030e98..19fe4053be81 100644
--- a/drivers/remoteproc/rproc-uclass.c
+++ b/drivers/remoteproc/rproc-uclass.c
@@ -689,7 +689,7 @@ static int alloc_vring(struct udevice *dev, struct
fw_rsc_vdev *rsc, int i)
debug("alloc_mem(%#x, %d): %p\n", size, order, pa);
vring->da = (uintptr_t)pa;
- return !pa;
+ return 0;
}
static int handle_vdev(struct udevice *dev, struct fw_rsc_vdev *rsc,
--
2.39.2
[PATCH] fdt: off by one in ofnode_lookup_fdt()
The "oftree_count" is the number of entries which have been set in
the oftree_list[] array. If all the entries have been initialized then
this off by one would result in reading one element beyond the end
of the array.
Signed-off-by: Dan Carpenter
---
drivers/core/ofnode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 8df16e56af5c..a4dc9bde085c 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -103,7 +103,7 @@ void *ofnode_lookup_fdt(ofnode node)
if (gd->flags & GD_FLG_RELOC) {
uint i = OFTREE_TREE_ID(node.of_offset);
- if (i > oftree_count) {
+ if (i >= oftree_count) {
log_debug("Invalid tree ID %x\n", i);
return NULL;
}
--
2.39.2
[PATCH] fs: btrfs: Prevent error pointer dereference in list_subvolums()
If btrfs_read_fs_root() fails with -ENOENT, then we go to the next entry. Fine. But if it fails for a different reason then we need to clean up and return an error code. In the current code it doesn't clean up but instead dereferences "root" and crashes. Signed-off-by: Dan Carpenter --- I didn't CC the btrfs mailing list. Perhaps, I should have? fs/btrfs/subvolume.c | 1 + 1 file changed, 1 insertion(+) diff --git a/fs/btrfs/subvolume.c b/fs/btrfs/subvolume.c index d446e7a2c418..68ca7e48e48e 100644 --- a/fs/btrfs/subvolume.c +++ b/fs/btrfs/subvolume.c @@ -199,6 +199,7 @@ static int list_subvolums(struct btrfs_fs_info *fs_info) ret = PTR_ERR(root); if (ret == -ENOENT) goto next; + goto out; } ret = list_one_subvol(root, result); if (ret < 0) -- 2.39.2
[PATCH] cros_ec: Fix an error code is cros_ec_get_sku_id()
The ec_command_inptr() function returns negative error codes or
the number of bytes that it was able to read. The cros_ec_get_sku_id()
function should return negative error codes. Right now it returns
positive error codes or negative byte counts.
Signed-off-by: Dan Carpenter
---
drivers/misc/cros_ec.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/cros_ec.c b/drivers/misc/cros_ec.c
index 621d1752176a..9c1e6a5e3e70 100644
--- a/drivers/misc/cros_ec.c
+++ b/drivers/misc/cros_ec.c
@@ -1100,8 +1100,11 @@ int cros_ec_get_sku_id(struct udevice *dev)
ret = ec_command_inptr(dev, EC_CMD_GET_SKU_ID, 0, NULL, 0,
(uint8_t **), sizeof(*r));
- if (ret != sizeof(*r))
- return -ret;
+ if (ret != sizeof(*r)) {
+ if (ret >= 0)
+ ret = -EIO;
+ return ret;
+ }
return r->sku_id;
}
--
2.39.2
[PATCH] efi_loader: Fix memory corruption on 32bit systems
It's pretty unlikely that anyone is going to be using EFI authentication
on a 32bit system. However, if you did, the efi_prepare_aligned_image()
function would write 8 bytes of data to the _size variable and it
can only hold 4 bytes so that corrupts memory.
Signed-off-by: Dan Carpenter
---
lib/efi_loader/efi_image_loader.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/efi_loader/efi_image_loader.c
b/lib/efi_loader/efi_image_loader.c
index 26df0da16c93..3d5eef7dc3c2 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -592,6 +592,7 @@ static bool efi_image_authenticate(void *efi, size_t
efi_size)
struct efi_signature_store *db = NULL, *dbx = NULL;
void *new_efi = NULL;
u8 *auth, *wincerts_end;
+ u64 size = efi_size;
size_t auth_size;
bool ret = false;
@@ -600,11 +601,11 @@ static bool efi_image_authenticate(void *efi, size_t
efi_size)
if (!efi_secure_boot_enabled())
return true;
- new_efi = efi_prepare_aligned_image(efi, (u64 *)_size);
+ new_efi = efi_prepare_aligned_image(efi, );
if (!new_efi)
return false;
- if (!efi_image_parse(new_efi, efi_size, , ,
+ if (!efi_image_parse(new_efi, size, , ,
_len)) {
log_err("Parsing PE executable image failed\n");
goto out;
--
2.39.2
[PATCH] video: Add parentheses around VNBYTES() macro
The VNBYTES() macro needs to have parentheses to prevent some (harmless)
macro expansion bugs. The VNBYTES() macro is used like this:
VID_TO_PIXEL(x) * VNBYTES(vid_priv->bpix)
The * operation is done before the / operation. It still ends up with
the same results, but it's not ideal.
Signed-off-by: Dan Carpenter
---
include/video.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/video.h b/include/video.h
index 9729fa348aa5..b364bf91825f 100644
--- a/include/video.h
+++ b/include/video.h
@@ -58,7 +58,7 @@ enum video_log2_bpp {
* Convert enum video_log2_bpp to bytes and bits. Note we omit the outer
* brackets to allow multiplication by fractional pixels.
*/
-#define VNBYTES(bpix) (1 << (bpix)) / 8
+#define VNBYTES(bpix) ((1 << (bpix)) / 8)
#define VNBITS(bpix) (1 << (bpix))
--
2.39.2
[PATCH] cmd: Fix an error code in cmd_mux_find()
This returns the wrong variable. It ends up returning NULL when it was suppose to return an error pointer. Signed-off-by: Dan Carpenter --- cmd/mux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/mux.c b/cmd/mux.c index 833266f08b1e..c75907af7726 100644 --- a/cmd/mux.c +++ b/cmd/mux.c @@ -49,7 +49,7 @@ static struct mux_control *cmd_mux_find(char *const argv[]) chip = dev_get_uclass_priv(dev); if (!chip) - return ERR_PTR(ret); + return ERR_PTR(-EINVAL); if (id >= chip->controllers) return ERR_PTR(-EINVAL); -- 2.39.2
[bug report] cros_ec: Add vstore support
Hello Simon Glass,
The patch 10f746591fba: "cros_ec: Add vstore support" from Jan 16,
2021 (linux-next), leads to the following Smatch static checker
warning:
drivers/misc/cros_ec_sandbox.c:543 process_cmd() error: buffer overflow
'ec->slot' 4 <= 31
drivers/misc/cros_ec_sandbox.c:556 process_cmd() error: buffer overflow
'ec->slot' 4 <= 31
drivers/misc/cros_ec_sandbox.c
521 len = sizeof(*resp);
522 break;
523 }
524 case EC_CMD_VSTORE_INFO: {
525 struct ec_response_vstore_info *resp = resp_data;
526 int i;
527
528 resp->slot_count = VSTORE_SLOT_COUNT;
There are two related defines. VSTORE_SLOT_COUNT (4) is the number of
elements in ec->slot[].
529 resp->slot_locked = 0;
530 for (i = 0; i < VSTORE_SLOT_COUNT; i++) {
531 if (ec->slot[i].locked)
532 resp->slot_locked |= 1 << i;
533 }
534 len = sizeof(*resp);
535 break;
536 };
537 case EC_CMD_VSTORE_WRITE: {
538 const struct ec_params_vstore_write *req = req_data;
539 struct vstore_slot *slot;
540
541 if (req->slot >= EC_VSTORE_SLOT_MAX)
542 return -EINVAL;
--> 543 slot = >slot[req->slot];
But here the check is for EC_VSTORE_SLOT_MAX (32) so Smatch thinks that
32 is more than 4 so this is an out of bounds. Should the limit be
smaller or the array larger?
544 slot->locked = true;
545 memcpy(slot->data, req->data, EC_VSTORE_SLOT_SIZE);
546 len = 0;
547 break;
548 }
549 case EC_CMD_VSTORE_READ: {
550 const struct ec_params_vstore_read *req = req_data;
551 struct ec_response_vstore_read *resp = resp_data;
552 struct vstore_slot *slot;
553
554 if (req->slot >= EC_VSTORE_SLOT_MAX)
555 return -EINVAL;
556 slot = >slot[req->slot];
Same.
557 memcpy(resp->data, slot->data, EC_VSTORE_SLOT_SIZE);
558 len = sizeof(*resp);
559 break;
560 }
561 case EC_CMD_PWM_GET_DUTY: {
562 const struct ec_params_pwm_get_duty *req = req_data;
regards,
dan carpenter
[PATCH] addrmap: Fix off by one in addrmap_set_entry()
The > comparison needs to be changed to >= to prevent an out of bounds
write on th next line.
Signed-off-by: Dan Carpenter
---
lib/addr_map.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/addr_map.c b/lib/addr_map.c
index 9b3e0a544e47..86e932e4b561 100644
--- a/lib/addr_map.c
+++ b/lib/addr_map.c
@@ -59,7 +59,7 @@ void *addrmap_phys_to_virt(phys_addr_t paddr)
void addrmap_set_entry(unsigned long vaddr, phys_addr_t paddr,
phys_size_t size, int idx)
{
- if (idx > CONFIG_SYS_NUM_ADDR_MAP)
+ if (idx >= CONFIG_SYS_NUM_ADDR_MAP)
return;
address_map[idx].vaddr = vaddr;
--
2.39.2
[PATCH] cmd: Fix a size parameter in test_readonly()
The parentheses are in the wrong place so this passes the number of
bytes to write as "sizeof(index_0) != TPM_SUCCESS" when just
"sizeof(index_0)" was intended. (1 byte vs 4 bytes).
Signed-off-by: Dan Carpenter
---
cmd/tpm_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cmd/tpm_test.c b/cmd/tpm_test.c
index c4ed8e590120..9bdc9c660fd0 100644
--- a/cmd/tpm_test.c
+++ b/cmd/tpm_test.c
@@ -294,8 +294,8 @@ static int test_readonly(struct udevice *dev)
*/
index_0 += 1;
if (tpm_nv_write_value(dev, INDEX0, (uint8_t *)_0,
- sizeof(index_0) !=
- TPM_SUCCESS)) {
+ sizeof(index_0)) !=
+ TPM_SUCCESS) {
pr_err("\tcould not write index 0\n");
}
tpm_nv_write_value_lock(dev, INDEX0);
--
2.39.2
