Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31182
Change subject: usbdebug: Use fixed size field ......................................................................
usbdebug: Use fixed size field
The structure is placed inside CBMEM, one should use types with fixed size.
Change-Id: I60382664a53650b225abc1f77c87ed4e121d429e Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/usb/ehci_debug.c 1 file changed, 17 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31182/1
diff --git a/src/drivers/usb/ehci_debug.c b/src/drivers/usb/ehci_debug.c index cf3976c..3532466 100644 --- a/src/drivers/usb/ehci_debug.c +++ b/src/drivers/usb/ehci_debug.c @@ -28,8 +28,8 @@ #include "ehci.h"
struct ehci_debug_info { - void *ehci_base; - void *ehci_debug; + u64 ehci_base; + u64 ehci_debug;
struct dbgp_pipe ep_pipe[DBGP_MAX_ENDPOINTS]; }; @@ -262,7 +262,9 @@ int dbgp_bulk_write_x(struct dbgp_pipe *pipe, const char *bytes, int size) { struct ehci_debug_info *dbg_info = dbgp_ehci_info(); - return dbgp_bulk_write(dbg_info->ehci_debug, pipe, bytes, size); + struct ehci_dbg_port *port; + port = (void *)(uintptr_t)dbg_info->ehci_debug; + return dbgp_bulk_write(port, pipe, bytes, size); }
static int dbgp_bulk_read(struct ehci_dbg_port *ehci_debug, struct dbgp_pipe *pipe, @@ -296,7 +298,9 @@ int dbgp_bulk_read_x(struct dbgp_pipe *pipe, void *data, int size) { struct ehci_debug_info *dbg_info = dbgp_ehci_info(); - return dbgp_bulk_read(dbg_info->ehci_debug, pipe, data, size); + struct ehci_dbg_port *port; + port = (void *)(uintptr_t)dbg_info->ehci_debug; + return dbgp_bulk_read(port, pipe, data, size); }
void dbgp_mdelay(int ms) @@ -446,8 +450,8 @@
/* Keep all endpoints disabled before any printk() call. */ memset(info, 0, sizeof (*info)); - info->ehci_base = (void *)ehci_bar; - info->ehci_debug = (void *)(ehci_bar + offset); + info->ehci_base = ehci_bar; + info->ehci_debug = ehci_bar + offset; info->ep_pipe[0].status |= DBGP_EP_NOT_PRESENT;
dprintk(BIOS_INFO, "ehci_bar: 0x%x debug_offset 0x%x\n", ehci_bar, offset); @@ -456,7 +460,7 @@ ehci_regs = (struct ehci_regs *)(ehci_bar + HC_LENGTH(read32(&ehci_caps->hc_capbase)));
- struct ehci_dbg_port *ehci_debug = info->ehci_debug; + struct ehci_dbg_port *ehci_debug = (void *)(uintptr_t)info->ehci_debug;
if (CONFIG_USBDEBUG_DEFAULT_PORT > 0) ehci_debug_select_port(CONFIG_USBDEBUG_DEFAULT_PORT); @@ -581,7 +585,8 @@
dbgp_mdelay(100);
- ret = dbgp_probe_gadget(info->ehci_debug, &info->ep_pipe[0]); + struct ehci_dbg_port *port = (void *)(uintptr_t)info->ehci_debug; + ret = dbgp_probe_gadget(port, &info->ep_pipe[0]); if (ret < 0) { dprintk(BIOS_INFO, "Could not probe gadget on debug port.\n"); ret = -6; @@ -653,12 +658,12 @@ void usbdebug_re_enable(unsigned ehci_base) { struct ehci_debug_info *dbg_info = dbgp_ehci_info(); - unsigned diff; + u64 diff; int i;
- diff = (unsigned)dbg_info->ehci_base - ehci_base; + diff = dbg_info->ehci_base - ehci_base; dbg_info->ehci_debug -= diff; - dbg_info->ehci_base = (void *)ehci_base; + dbg_info->ehci_base = ehci_base;
for (i=0; i<DBGP_MAX_ENDPOINTS; i++) if (dbg_info->ep_pipe[i].status & DBGP_EP_VALID) @@ -678,7 +683,7 @@ int usbdebug_hw_init(bool force) { struct ehci_debug_info *dbg_info = dbgp_ehci_info(); - unsigned int ehci_base, dbg_offset; + u32 ehci_base, dbg_offset;
if (dbgp_enabled() && !force) return 0;
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31182 )
Change subject: usbdebug: Use fixed size field ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31182/2/src/drivers/usb/ehci_debug.c File src/drivers/usb/ehci_debug.c:
https://review.coreboot.org/#/c/31182/2/src/drivers/usb/ehci_debug.c@a34 PS2, Line 34: this includes an int?
https://review.coreboot.org/#/c/31182/2/src/drivers/usb/ehci_debug.c@a35 PS2, Line 35: should be __packed?
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31182
to look at the new patch set (#3).
Change subject: usbdebug: Use fixed size field ......................................................................
usbdebug: Use fixed size field
The structure is placed inside CBMEM, one should use types with fixed size. Seems we prefer to prepare for 64-bit builds even for MMIO pointers.
Change-Id: I60382664a53650b225abc1f77c87ed4e121d429e Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/car.ld M src/drivers/usb/ehci_debug.c M src/drivers/usb/ehci_debug.h 3 files changed, 21 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/31182/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31182 )
Change subject: usbdebug: Use fixed size field ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31182 )
Change subject: usbdebug: Use fixed size field ......................................................................
usbdebug: Use fixed size field
The structure is placed inside CBMEM, one should use types with fixed size. Seems we prefer to prepare for 64-bit builds even for MMIO pointers.
Change-Id: I60382664a53650b225abc1f77c87ed4e121d429e Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/31182 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/arch/x86/car.ld M src/drivers/usb/ehci_debug.c M src/drivers/usb/ehci_debug.h 3 files changed, 21 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/arch/x86/car.ld b/src/arch/x86/car.ld index 1b0d996..0d0d585 100644 --- a/src/arch/x86/car.ld +++ b/src/arch/x86/car.ld @@ -68,7 +68,7 @@ #endif _car_ehci_dbg_info_start = .; /* Reserve sizeof(struct ehci_dbg_info). */ - . += 88; + . += 80; _car_ehci_dbg_info_end = .; /* _car_global_start and _car_global_end provide symbols to per-stage * variables that are not shared like the timestamp and the pre-ram diff --git a/src/drivers/usb/ehci_debug.c b/src/drivers/usb/ehci_debug.c index c5fb984..0cb89ce 100644 --- a/src/drivers/usb/ehci_debug.c +++ b/src/drivers/usb/ehci_debug.c @@ -28,11 +28,11 @@ #include "ehci.h"
struct ehci_debug_info { - void *ehci_base; - void *ehci_debug; + u64 ehci_base; + u64 ehci_debug;
struct dbgp_pipe ep_pipe[DBGP_MAX_ENDPOINTS]; -}; +} __packed;
#if IS_ENABLED(CONFIG_DEBUG_CONSOLE_INIT) /* When selected, you can debug the connection of usbdebug dongle. @@ -263,7 +263,9 @@ int dbgp_bulk_write_x(struct dbgp_pipe *pipe, const char *bytes, int size) { struct ehci_debug_info *dbg_info = dbgp_ehci_info(); - return dbgp_bulk_write(dbg_info->ehci_debug, pipe, bytes, size); + struct ehci_dbg_port *port; + port = (void *)(uintptr_t)dbg_info->ehci_debug; + return dbgp_bulk_write(port, pipe, bytes, size); }
static int dbgp_bulk_read(struct ehci_dbg_port *ehci_debug, struct dbgp_pipe *pipe, @@ -297,7 +299,9 @@ int dbgp_bulk_read_x(struct dbgp_pipe *pipe, void *data, int size) { struct ehci_debug_info *dbg_info = dbgp_ehci_info(); - return dbgp_bulk_read(dbg_info->ehci_debug, pipe, data, size); + struct ehci_dbg_port *port; + port = (void *)(uintptr_t)dbg_info->ehci_debug; + return dbgp_bulk_read(port, pipe, data, size); }
void dbgp_mdelay(int ms) @@ -447,8 +451,8 @@
/* Keep all endpoints disabled before any printk() call. */ memset(info, 0, sizeof (*info)); - info->ehci_base = (void *)ehci_bar; - info->ehci_debug = (void *)(ehci_bar + offset); + info->ehci_base = ehci_bar; + info->ehci_debug = ehci_bar + offset; info->ep_pipe[0].status |= DBGP_EP_NOT_PRESENT;
dprintk(BIOS_INFO, "ehci_bar: 0x%x debug_offset 0x%x\n", ehci_bar, offset); @@ -457,7 +461,7 @@ ehci_regs = (struct ehci_regs *)(ehci_bar + HC_LENGTH(read32(&ehci_caps->hc_capbase)));
- struct ehci_dbg_port *ehci_debug = info->ehci_debug; + struct ehci_dbg_port *ehci_debug = (void *)(uintptr_t)info->ehci_debug;
if (CONFIG_USBDEBUG_DEFAULT_PORT > 0) ehci_debug_select_port(CONFIG_USBDEBUG_DEFAULT_PORT); @@ -582,7 +586,8 @@
dbgp_mdelay(100);
- ret = dbgp_probe_gadget(info->ehci_debug, &info->ep_pipe[0]); + struct ehci_dbg_port *port = (void *)(uintptr_t)info->ehci_debug; + ret = dbgp_probe_gadget(port, &info->ep_pipe[0]); if (ret < 0) { dprintk(BIOS_INFO, "Could not probe gadget on debug port.\n"); ret = -6; @@ -654,12 +659,12 @@ void usbdebug_re_enable(unsigned ehci_base) { struct ehci_debug_info *dbg_info = dbgp_ehci_info(); - unsigned diff; + u64 diff; int i;
- diff = (unsigned)dbg_info->ehci_base - ehci_base; + diff = dbg_info->ehci_base - ehci_base; dbg_info->ehci_debug -= diff; - dbg_info->ehci_base = (void *)ehci_base; + dbg_info->ehci_base = ehci_base;
for (i=0; i<DBGP_MAX_ENDPOINTS; i++) if (dbg_info->ep_pipe[i].status & DBGP_EP_VALID) @@ -679,7 +684,7 @@ int usbdebug_hw_init(bool force) { struct ehci_debug_info *dbg_info = dbgp_ehci_info(); - unsigned int ehci_base, dbg_offset; + u32 ehci_base, dbg_offset;
if (dbgp_enabled() && !force) return 0; diff --git a/src/drivers/usb/ehci_debug.h b/src/drivers/usb/ehci_debug.h index efb2766..0ac94bf 100644 --- a/src/drivers/usb/ehci_debug.h +++ b/src/drivers/usb/ehci_debug.h @@ -47,12 +47,12 @@ u8 endpoint; u8 pid; u8 status; - int timeout; + u16 timeout;
u8 bufidx; u8 buflen; char buf[8]; -}; +} __packed;
void dbgp_put(struct dbgp_pipe *pipe); int dbgp_try_get(struct dbgp_pipe *pipe);
-
Kyösti Mälkki (Code Review) -
Nico Huber (Code Review)