[PATCH 0/3] ratp: fixes

[PATCH 0/3] ratp: fixes

[Sascha Hauer]

The ratp commands have a wrong alignment on X86 sandbox builds which
currently breaks the allyes sandbox build. Also the ratp commands are
placed in a readonly section, but are nevertheless modified. This series
contains fixes for both issues.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Sascha Hauer (3):
      ratp: Drop wrong alignment annotation
      ratp: do not export ratp command list
      ratp: make ratp commands const

 common/ratp/ratp.c | 37 +++++++++++++++++++++++--------------
 include/ratp_bb.h  | 10 ++--------
 2 files changed, 25 insertions(+), 22 deletions(-)
---
base-commit: b4c0c4615ace1795aa75ced21caa1e78b665cde0
change-id: 20250407-ratp-fixes-438ad710b68f

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

[PATCH 1/3] ratp: Drop wrong alignment annotation

[Sascha Hauer]

When ratp command handling was introduced in ff612b866f301 ("ratp:
implement generic command support") we had an explicit 64bit alignment
for the ratp command array in sandbox. This was removed in 52e5c35671
("X86: lds: remove unnecessary alignments"). With this it can happen
that the ratp command section starts at a non 64bit aligned address, but
the first command in that section will be placed at the first 64bit
boundary. __barebox_ratp_cmd_start will no longer point to the actual
command then and the array iteration fails.

Just drop the wrong alignment annotation.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/ratp_bb.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/include/ratp_bb.h b/include/ratp_bb.h
index 418be6fe7b..c6c7c4bc23 100644
--- a/include/ratp_bb.h
+++ b/include/ratp_bb.h
@@ -57,12 +57,7 @@ struct ratp_command {
 			       int req_len,
 			       struct ratp_bb **rsp,
 			       int *rsp_len);
-}
-#ifdef __x86_64__
-/* This is required because the linker will put symbols on a 64 bit alignment */
-__attribute__((aligned(64)))
-#endif
-;
+};
 
 #define BAREBOX_RATP_CMD_START(_name)							\
 extern const struct ratp_command __barebox_ratp_cmd_##_name;				\

-- 
2.39.5

[PATCH 2/3] ratp: do not export ratp command list

[Sascha Hauer]

ratp_command_list is not even declared outside common/ratp/ratp.c, so
make it static.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/ratp/ratp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
index 5cbdb8bd5f..925e1d637f 100644
--- a/common/ratp/ratp.c
+++ b/common/ratp/ratp.c
@@ -28,8 +28,7 @@
 #include <fs.h>
 #include <console_countdown.h>
 
-LIST_HEAD(ratp_command_list);
-EXPORT_SYMBOL(ratp_command_list);
+static LIST_HEAD(ratp_command_list);
 
 #define for_each_ratp_command(cmd) list_for_each_entry(cmd, &ratp_command_list, list)
 

-- 
2.39.5

[PATCH 3/3] ratp: make ratp commands const

[Sascha Hauer]

The ratp commands are placed in the readonly data section, but are
written to due to the list entry in the command.

Make the ratp commands actually const by moving the list entry to a
separate container struct.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/ratp/ratp.c | 34 ++++++++++++++++++++++------------
 include/ratp_bb.h  |  3 +--
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/common/ratp/ratp.c b/common/ratp/ratp.c
index 925e1d637f..a59dd56265 100644
--- a/common/ratp/ratp.c
+++ b/common/ratp/ratp.c
@@ -30,8 +30,6 @@
 
 static LIST_HEAD(ratp_command_list);
 
-#define for_each_ratp_command(cmd) list_for_each_entry(cmd, &ratp_command_list, list)
-
 struct ratp_bb_command_return {
 	uint32_t errno;
 };
@@ -56,30 +54,42 @@ struct ratp_ctx {
 	bool wq_registered;
 };
 
+struct ratp_command_entry {
+	struct list_head list;
+	const struct ratp_command *cmd;
+};
+
 static int compare_ratp_command(struct list_head *a, struct list_head *b)
 {
-	int id_a = list_entry(a, struct ratp_command, list)->request_id;
-	int id_b = list_entry(b, struct ratp_command, list)->request_id;
+	int id_a = list_entry(a, const struct ratp_command_entry, list)->cmd->request_id;
+	int id_b = list_entry(b, const struct ratp_command_entry, list)->cmd->request_id;
 
 	return (id_a - id_b);
 }
 
-int register_ratp_command(struct ratp_command *cmd)
+int register_ratp_command(const struct ratp_command *cmd)
 {
+	struct ratp_command_entry *entry;
+
 	debug("register ratp command: request %hu, response %hu\n",
 	      cmd->request_id, cmd->response_id);
-	list_add_sort(&cmd->list, &ratp_command_list, compare_ratp_command);
+
+	entry = xzalloc(sizeof(*entry));
+	entry->cmd = cmd;
+
+	list_add_sort(&entry->list, &ratp_command_list, compare_ratp_command);
+
 	return 0;
 }
 EXPORT_SYMBOL(register_ratp_command);
 
-static struct ratp_command *find_ratp_request(uint16_t request_id)
+static const struct ratp_command *find_ratp_request(uint16_t request_id)
 {
-	struct ratp_command *cmdtp;
+	struct ratp_command_entry *entry;
 
-	for_each_ratp_command(cmdtp)
-		if (request_id == cmdtp->request_id)
-			return cmdtp;
+	list_for_each_entry(entry, &ratp_command_list, list)
+		if (request_id == entry->cmd->request_id)
+			return entry->cmd;
 
 	return NULL;	/* not found */
 }
@@ -210,7 +220,7 @@ static int ratp_bb_dispatch(struct ratp_ctx *ctx, const void *buf, int len)
 	int dlen = len - sizeof(struct ratp_bb);
 	int ret = 0;
 	uint16_t type = be16_to_cpu(rbb->type);
-	struct ratp_command *cmd;
+	const struct ratp_command *cmd;
 	struct ratp_work *rw;
 
 	/* See if there's a command registered to this type */
diff --git a/include/ratp_bb.h b/include/ratp_bb.h
index c6c7c4bc23..746f6155dd 100644
--- a/include/ratp_bb.h
+++ b/include/ratp_bb.h
@@ -50,7 +50,6 @@ int  barebox_ratp_fs_mount(const char *path);
  */
 
 struct ratp_command {
-	struct list_head  list;
 	uint16_t          request_id;
 	uint16_t          response_id;
 	int		(*cmd)(const struct ratp_bb *req,
@@ -67,6 +66,6 @@ const struct ratp_command __barebox_ratp_cmd_##_name					\
 #define BAREBOX_RATP_CMD_END								\
 };
 
-int register_ratp_command(struct ratp_command *cmd);
+int register_ratp_command(const struct ratp_command *cmd);
 
 #endif /* __RATP_BB_H */

-- 
2.39.5

Re: [PATCH 1/3] ratp: Drop wrong alignment annotation

[Ahmad Fatoum]

On 07.04.25 09:29, Sascha Hauer wrote:
> When ratp command handling was introduced in ff612b866f301 ("ratp:
> implement generic command support") we had an explicit 64bit alignment

s/64bit/64 byte/

> for the ratp command array in sandbox. This was removed in 52e5c35671
> ("X86: lds: remove unnecessary alignments"). With this it can happen
> that the ratp command section starts at a non 64bit aligned address, but
> the first command in that section will be placed at the first 64bit
> boundary. __barebox_ratp_cmd_start will no longer point to the actual
> command then and the array iteration fails.
> 
> Just drop the wrong alignment annotation.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  include/ratp_bb.h | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/include/ratp_bb.h b/include/ratp_bb.h
> index 418be6fe7b..c6c7c4bc23 100644
> --- a/include/ratp_bb.h
> +++ b/include/ratp_bb.h
> @@ -57,12 +57,7 @@ struct ratp_command {
>  			       int req_len,
>  			       struct ratp_bb **rsp,
>  			       int *rsp_len);
> -}
> -#ifdef __x86_64__
> -/* This is required because the linker will put symbols on a 64 bit alignment */
> -__attribute__((aligned(64)))
> -#endif
> -;
> +};
>  
>  #define BAREBOX_RATP_CMD_START(_name)							\
>  extern const struct ratp_command __barebox_ratp_cmd_##_name;				\
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 0/3] ratp: fixes

[Sascha Hauer]

On Mon, 07 Apr 2025 09:29:52 +0200, Sascha Hauer wrote:
> The ratp commands have a wrong alignment on X86 sandbox builds which
> currently breaks the allyes sandbox build. Also the ratp commands are
> placed in a readonly section, but are nevertheless modified. This series
> contains fixes for both issues.
> 
> 

Applied, thanks!

[1/3] ratp: Drop wrong alignment annotation
      https://git.pengutronix.de/cgit/barebox/commit/?id=02b0308c5880 (link may not be stable)
[2/3] ratp: do not export ratp command list
      https://git.pengutronix.de/cgit/barebox/commit/?id=1929b2d0c762 (link may not be stable)
[3/3] ratp: make ratp commands const
      https://git.pengutronix.de/cgit/barebox/commit/?id=e912ead13b75 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

Re: [PATCH 1/3] ratp: Drop wrong alignment annotation

[Sascha Hauer]

On Mon, Apr 07, 2025 at 09:59:13AM +0200, Ahmad Fatoum wrote:
> On 07.04.25 09:29, Sascha Hauer wrote:
> > When ratp command handling was introduced in ff612b866f301 ("ratp:
> > implement generic command support") we had an explicit 64bit alignment
> 
> s/64bit/64 byte/

FIxed while applying, thanks

Sascha

> 
> > for the ratp command array in sandbox. This was removed in 52e5c35671
> > ("X86: lds: remove unnecessary alignments"). With this it can happen
> > that the ratp command section starts at a non 64bit aligned address, but
> > the first command in that section will be placed at the first 64bit
> > boundary. __barebox_ratp_cmd_start will no longer point to the actual
> > command then and the array iteration fails.
> > 
> > Just drop the wrong alignment annotation.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  include/ratp_bb.h | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/include/ratp_bb.h b/include/ratp_bb.h
> > index 418be6fe7b..c6c7c4bc23 100644
> > --- a/include/ratp_bb.h
> > +++ b/include/ratp_bb.h
> > @@ -57,12 +57,7 @@ struct ratp_command {
> >  			       int req_len,
> >  			       struct ratp_bb **rsp,
> >  			       int *rsp_len);
> > -}
> > -#ifdef __x86_64__
> > -/* This is required because the linker will put symbols on a 64 bit alignment */
> > -__attribute__((aligned(64)))
> > -#endif
> > -;
> > +};
> >  
> >  #define BAREBOX_RATP_CMD_START(_name)							\
> >  extern const struct ratp_command __barebox_ratp_cmd_##_name;				\
> > 
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |