Re: [PATCH] usb: ehci: prevent bad PORTSC register access
On Tue, 25 Aug 2015 15:59:58 +0300
Peter Mamonov pmamo...@gmail.com wrote:
From: Kuo-Jung Su dant...@faraday-tech.com
1. The 'index' of ehci_submit_root() is not always 0.
e.g.
While it gets invoked from usb_get_descriptor(),
the 'index' is always a '0'. (See ch.9 of USB2.0)
2. The PORTSC register is not always required, and thus it
should only report a port error when necessary.
It would cause a port scan failure if the ehci_submit_root()
always gets terminated by a port error.
Signed-off-by: Kuo-Jung Su dant...@faraday-tech.com
Signed-off-by: Peter Mamonov pmamo...@gmail.com
---
drivers/usb/host/ehci-hcd.c | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 58c22db..1146b71 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -476,13 +476,8 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
int len, srclen;
uint32_t reg;
uint32_t *status_reg;
+ int port = le16_to_cpu(req-index);
- if (le16_to_cpu(req-index) = CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
- dev_err(ehci-dev, The request port(%d) is not configured\n,
- le16_to_cpu(req-index) - 1);
- return -1;
- }
- status_reg = (uint32_t *)ehci-hcor-or_portsc[le16_to_cpu(req-index)
- 1];
srclen = 0;
dev_dbg(ehci-dev, req=%u (%#x), type=%u (%#x), value=%u, index=%u\n,
@@ -493,6 +488,21 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
typeReq = req-request | (req-requesttype 8);
switch (typeReq) {
+ case USB_REQ_GET_STATUS | ((USB_RT_PORT | USB_DIR_IN) 8):
+ case USB_REQ_SET_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) 8):
+ case USB_REQ_CLEAR_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) 8):
+ if (!port || port CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
+ printf(The request port(%d) is not configured\n, port
- 1);
+ return -1;
+ }
+ status_reg = (uint32_t *)ehci-hcor-or_portsc[port - 1];
+ break;
+ default:
+ status_reg = NULL;
+ break;
+ }
+
+ switch (typeReq) {
case DeviceRequest | USB_REQ_GET_DESCRIPTOR:
switch (le16_to_cpu(req-value) 8) {
case USB_DT_DEVICE:
@@ -571,7 +581,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
if (reg EHCI_PS_OCA)
tmpbuf[0] |= USB_PORT_STAT_OVERCURRENT;
if (reg EHCI_PS_PR
- (ehci-portreset (1 le16_to_cpu(req-index {
+ (ehci-portreset (1 port))) {
int ret;
/* force reset to complete */
reg = reg ~(EHCI_PS_PR | EHCI_PS_CLEAR);
@@ -581,7 +591,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
tmpbuf[0] |= USB_PORT_STAT_RESET;
else
dev_err(ehci-dev, port(%d) reset error\n,
- le16_to_cpu(req-index) - 1);
+ port - 1);
}
if (reg EHCI_PS_PP)
tmpbuf[1] |= USB_PORT_STAT_POWER 8;
@@ -608,7 +618,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
tmpbuf[2] |= USB_PORT_STAT_C_ENABLE;
if (reg EHCI_PS_OCC)
tmpbuf[2] |= USB_PORT_STAT_C_OVERCURRENT;
- if (ehci-portreset (1 le16_to_cpu(req-index)))
+ if (ehci-portreset (1 port))
tmpbuf[2] |= USB_PORT_STAT_C_RESET;
srcptr = tmpbuf;
@@ -634,7 +644,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
EHCI_PS_IS_LOWSPEED(reg)) {
/* Low speed device, give up ownership. */
dev_dbg(ehci-dev, port %d low speed --
companion\n,
- req-index - 1);
+ port - 1);
reg |= EHCI_PS_PO;
ehci_writel(status_reg, reg);
break;
@@ -651,7 +661,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
*/
ehci_powerup_fixup(ehci);
mdelay(50);
- ehci-portreset |= 1 le16_to_cpu(req-index);
+ ehci-portreset |= 1 port;
/* terminate the reset */
Re: [PATCH v2] ARM: socfpga: select OFTREE and OFDEVICE
On Wed, Aug 12, 2015 at 10:45:38AM +0200, Lucas Stach wrote: SoCFPGA is completely multi-image enabled and probes the barebox from a built-in DT, so there is no point in building a barebox image that isn't able to probe from DT. Signed-off-by: Lucas Stach l.st...@pengutronix.de --- v2: Only select OF symbols if building main barebox binary. --- Applied, thanks Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 1/3] commands: Add MMC ext. CSD register tool
Hi Daniel,
Nice command. Several comments inline, there is still some way to go to
get this into shape.
On Mon, Aug 24, 2015 at 01:32:13PM +0200, Daniel Schultz wrote:
+
+static int print_field(u8 *reg, int index)
+{
+ int rev;
+ u32 val;
+ u32 tmp;
+ u64 tmp64;
+
+ rev = reg[EXT_CSD_REV];
+
+ if (rev = 7)
Additional print_field_v7/v6/v5() functions will reduce the indentation
level by one and help violating the 80 character limit less often.
+ switch (index) {
+ case EXT_CSD_CMDQ_MODE_EN:
+ print_field_caption(CMDQ_MODE_EN, RWE_P);
+ val = get_field_val(CMDQ_MODE_EN, 0, 0x1);
+ if (val)
+ printf(\tCommand queuing is enabled\n);
+ else
+ printf(\tCommand queuing is disabled\n);
Your printfs are very inefficient. You should use something like:
if (val)
str = en;
else
str = dis;
printf(Command queuing is %sabled\n, str);
Same goes for many other printfs. This will result in much less similar
strings in the binary.
+ return 1;
+
+ case EXT_CSD_SECURE_REMOVAL_TYPE:
+ print_field_caption(SECURE_REMOVAL_TYPE, RWaR);
+ val = get_field_val(SECURE_REMOVAL_TYPE, 0, 0xF);
+ switch (val) {
+ case 0x0:
+ printf(\t[3-0] Supported Secure Removal Type:
information removed by an erase of the physical memory\n);
+ break;
+ case 0x1:
+ printf(\t[3-0] Supported Secure Removal Type:
information removed by an overwriting the addressed locations with a
character followed by an erase\n);
+ break;
+ case 0x2:
+ printf(\t[3-0] Supported Secure Removal Type:
information removed by an overwriting the addressed locations with a
character, its complement, then a random character\n);
+ break;
+ case 0x3:
+ printf(\t[3-0] Supported Secure Removal Type:
information removed using a vendor defined\n);
+ break;
This is *very* verbose. Could you write this a bit more compact, maybe
case 0x0:
str = erase;
break;
case 0x1:
str = overwrite, then erase;
break;
case 0x2:
str = overwrite, complement, then random;
break;
case 0x3:
str = vendor defined;
break;
printf([3-0] Supported Secure Removal Type: %s\n, str);
Background is that this information only makes sense when being somewhat
familiar with the spec. Without knowing the spec at all even the more
verbose printfs do not help, but when knowing the spec a more compact
writing is enough to remember what is meant.
+
+static void print_register_raw(u8 *reg, int index)
+{
+ int i;
+
+ if (index == 0)
+ for (i = 0; i EXT_CSD_BLOCKSIZE; i++) {
+ if (i % 8 == 0)
+ printf(%u:, i);
+ printf( %#02x, reg[i]);
+ if (i % 8 == 7)
+ printf(\n);
+ }
memory_display(reg, 0, EXT_CSD_BLOCKSIZE, 1, 0);
Should do it here.
+static int do_extcsd(int argc, char *argv[])
+{
+ struct device_d *dev;
+ struct mci *mci;
+ struct mci_host *host;
+ u8 *dst;
+ int retval = 0;
+ int opt;
+ char*devname;
+ int index = 0;
+ int value = 0;
+ int write_operation = 0;
+ int always_write = 0;
+ int print_as_raw = 0;
+
+ if (argv[1] == NULL)
+ return COMMAND_ERROR_USAGE;
argc contains the number of arguments. When argc is 1 then argv[1] is
undefined. It may or may not be NULL in this case. You want to use if
(argc 2) here.
+
+ devname = argv[1];
You should use argv[optind] after parsing the arguments for the devname.
With this not only extcsd devname [OPTIONS] works but also extcsd
[OPTIONS] devname. Don't forget to check if there actually is
argv[optind] by
if (optind == argc)
return COMMAND_ERROR_USAGE;
+ if (!strncmp(devname, /dev/, 5))
+ devname += 5;
+
+ while ((opt = getopt(argc, argv, i:v:yr)) 0)
+ switch (opt) {
+ case 'i':
+ index = simple_strtoul(optarg, NULL, 0);
+ break;
+ case
[PATCH] usb: ehci-hcd: initialize ehci-qh_list[] with zeros
Without this initialization ehci-qh_list[0].qh_endpt2 is left uninitialized,
which causes problems with some EHCI host controllers.
Das u-boot uses the same strategy:
static int ehci_common_init(struct ehci_ctrl *ctrl, uint tweaks)
{
...
qh_list = ctrl-qh_list;
/* Set head of reclaim list */
memset(qh_list, 0, sizeof(*qh_list));
qh_list-qh_link = cpu_to_hc32((unsigned long)qh_list |
QH_LINK_TYPE_QH);
qh_list-qh_endpt1 = cpu_to_hc32(QH_ENDPT1_H(1) |
QH_ENDPT1_EPS(USB_SPEED_HIGH));
qh_list-qh_overlay.qt_next = cpu_to_hc32(QT_NEXT_TERMINATE);
qh_list-qh_overlay.qt_altnext = cpu_to_hc32(QT_NEXT_TERMINATE);
qh_list-qh_overlay.qt_token =
cpu_to_hc32(QT_TOKEN_STATUS(QT_TOKEN_STATUS_HALTED));
...
}
Signed-off-by: Peter Mamonov pmamo...@gmail.com
---
drivers/usb/host/ehci-hcd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index a9039c6..58c22db 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -772,6 +772,8 @@ static int ehci_init(struct usb_host *host)
return ret;
}
+ memset(ehci-qh_list, 0, sizeof(struct QH) * NUM_TD);
+
ehci-qh_list-qh_link = cpu_to_hc32((uint32_t)ehci-qh_list |
QH_LINK_TYPE_QH);
ehci-qh_list-qh_endpt1 = cpu_to_hc32((1 15) | (USB_SPEED_HIGH
12));
ehci-qh_list-qh_curtd = cpu_to_hc32(QT_NEXT_TERMINATE);
--
2.1.4
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 1/3] commands: Add MMC ext. CSD register tool
On Di, 2015-08-25 at 09:06 +0200, Sascha Hauer wrote: Your printfs are very inefficient. You should use something like: if (val) str = en; else str = dis; printf(Command queuing is %sabled\n, str); Same goes for many other printfs. This will result in much less similar strings in the binary. or like this: printf(Command queuing is %sabled\n, val ? en : dis); -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
[PATCH] usb: ehci: prevent bad PORTSC register access
From: Kuo-Jung Su dant...@faraday-tech.com
1. The 'index' of ehci_submit_root() is not always 0.
e.g.
While it gets invoked from usb_get_descriptor(),
the 'index' is always a '0'. (See ch.9 of USB2.0)
2. The PORTSC register is not always required, and thus it
should only report a port error when necessary.
It would cause a port scan failure if the ehci_submit_root()
always gets terminated by a port error.
Signed-off-by: Kuo-Jung Su dant...@faraday-tech.com
Signed-off-by: Peter Mamonov pmamo...@gmail.com
---
drivers/usb/host/ehci-hcd.c | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 58c22db..1146b71 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -476,13 +476,8 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
int len, srclen;
uint32_t reg;
uint32_t *status_reg;
+ int port = le16_to_cpu(req-index);
- if (le16_to_cpu(req-index) = CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
- dev_err(ehci-dev, The request port(%d) is not configured\n,
- le16_to_cpu(req-index) - 1);
- return -1;
- }
- status_reg = (uint32_t *)ehci-hcor-or_portsc[le16_to_cpu(req-index)
- 1];
srclen = 0;
dev_dbg(ehci-dev, req=%u (%#x), type=%u (%#x), value=%u, index=%u\n,
@@ -493,6 +488,21 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
typeReq = req-request | (req-requesttype 8);
switch (typeReq) {
+ case USB_REQ_GET_STATUS | ((USB_RT_PORT | USB_DIR_IN) 8):
+ case USB_REQ_SET_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) 8):
+ case USB_REQ_CLEAR_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) 8):
+ if (!port || port CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS) {
+ printf(The request port(%d) is not configured\n, port
- 1);
+ return -1;
+ }
+ status_reg = (uint32_t *)ehci-hcor-or_portsc[port - 1];
+ break;
+ default:
+ status_reg = NULL;
+ break;
+ }
+
+ switch (typeReq) {
case DeviceRequest | USB_REQ_GET_DESCRIPTOR:
switch (le16_to_cpu(req-value) 8) {
case USB_DT_DEVICE:
@@ -571,7 +581,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
if (reg EHCI_PS_OCA)
tmpbuf[0] |= USB_PORT_STAT_OVERCURRENT;
if (reg EHCI_PS_PR
- (ehci-portreset (1 le16_to_cpu(req-index {
+ (ehci-portreset (1 port))) {
int ret;
/* force reset to complete */
reg = reg ~(EHCI_PS_PR | EHCI_PS_CLEAR);
@@ -581,7 +591,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
tmpbuf[0] |= USB_PORT_STAT_RESET;
else
dev_err(ehci-dev, port(%d) reset error\n,
- le16_to_cpu(req-index) - 1);
+ port - 1);
}
if (reg EHCI_PS_PP)
tmpbuf[1] |= USB_PORT_STAT_POWER 8;
@@ -608,7 +618,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
tmpbuf[2] |= USB_PORT_STAT_C_ENABLE;
if (reg EHCI_PS_OCC)
tmpbuf[2] |= USB_PORT_STAT_C_OVERCURRENT;
- if (ehci-portreset (1 le16_to_cpu(req-index)))
+ if (ehci-portreset (1 port))
tmpbuf[2] |= USB_PORT_STAT_C_RESET;
srcptr = tmpbuf;
@@ -634,7 +644,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
EHCI_PS_IS_LOWSPEED(reg)) {
/* Low speed device, give up ownership. */
dev_dbg(ehci-dev, port %d low speed --
companion\n,
- req-index - 1);
+ port - 1);
reg |= EHCI_PS_PO;
ehci_writel(status_reg, reg);
break;
@@ -651,7 +661,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long
pipe, void *buffer,
*/
ehci_powerup_fixup(ehci);
mdelay(50);
- ehci-portreset |= 1 le16_to_cpu(req-index);
+ ehci-portreset |= 1 port;
/* terminate the reset */
ehci_writel(status_reg, reg ~EHCI_PS_PR);
