Change in ...coreboot[master]: usbdebug: Use fixed size field
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; -- To view, visit https://review.coreboot.org/c/coreboot/+/31182 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60382664a53650b225abc1f77c87ed4e121d429e Gerrit-Change-Number: 31182 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-MessageType: newchange
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? -- To view, visit https://review.coreboot.org/c/coreboot/+/31182 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60382664a53650b225abc1f77c87ed4e121d429e Gerrit-Change-Number: 31182 Gerrit-PatchSet: 2 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 01 Feb 2019 10:13:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/31182 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60382664a53650b225abc1f77c87ed4e121d429e Gerrit-Change-Number: 31182 Gerrit-PatchSet: 3 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-MessageType: newpatchset
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/31182 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60382664a53650b225abc1f77c87ed4e121d429e Gerrit-Change-Number: 31182 Gerrit-PatchSet: 3 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sat, 02 Feb 2019 14:24:36 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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); -- To view, visit https://review.coreboot.org/c/coreboot/+/31182 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I60382664a53650b225abc1f77c87ed4e121d429e Gerrit-Change-Number: 31182 Gerrit-PatchSet: 4 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Julius Werner <jwerner@chromium.org> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: merged
participants (2)
-
Kyösti Mälkki (Code Review) -
Nico Huber (Code Review)