[RFC PATCH] [media] rc: filter out not allowed protocols when decoding
From: Du, Changbin changbin...@gmail.com
Each rc-raw device has a property allowed_protos stored in structure
ir_raw_event_ctrl. But it didn't work because all decoders would be
called when decoding. This path makes only allowed protocol decoders
been invoked.
Signed-off-by: Du, Changbin changbin...@gmail.com
---
drivers/media/rc/ir-raw.c |8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/media/rc/ir-raw.c b/drivers/media/rc/ir-raw.c
index a820251..198b6d8 100644
--- a/drivers/media/rc/ir-raw.c
+++ b/drivers/media/rc/ir-raw.c
@@ -63,8 +63,12 @@ static int ir_raw_event_thread(void *data)
spin_unlock_irq(raw-lock);
mutex_lock(ir_raw_handler_lock);
- list_for_each_entry(handler, ir_raw_handler_list, list)
- handler-decode(raw-dev, ev);
+ list_for_each_entry(handler, ir_raw_handler_list, list) {
+ /* use all protocol by default */
+ if (raw-dev-allowed_protos == RC_TYPE_UNKNOWN ||
+ raw-dev-allowed_protos handler-protocols)
+ handler-decode(raw-dev, ev);
+ }
raw-prev_ev = ev;
mutex_unlock(ir_raw_handler_lock);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Resend PATCH] media: rc: ati_remote.c: code style and compile warning fixing
diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c index 7be377f..0df66ac 100644 --- a/drivers/media/rc/ati_remote.c +++ b/drivers/media/rc/ati_remote.c @@ -23,6 +23,8 @@ *Vincent Vanackere vanack...@lif.univ-mrs.fr *Added support for the Lola remote contributed by: *Seth Cohn sethc...@yahoo.com + * Jul 2012: Du, Changbin changbin...@gmail.com + *Code style and compile warning fixing You shouldn't be changing the driver's authorship just due to codingstyle and warning fixes. Btw, Please split Coding Style form Compilation warnings, as they're two different matters. Sorry, I didn't know this rule. I just want to make a track for me. OK, I will resend this patch and remove me from it. BTW, I am looking for something to learn these basic rules when sending patches. Could you tell me where I can find it? Many thanks! [Du, Changbin] Thanks! Mauro -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] [media] rc: ati_remote.c: code style fixing
From: Du, Changbin changbin...@gmail.com
changes:
1. wrap some lines that are longer than 80 characters.
2. remove local function prototype declarations which do not
need.
3. replace TAB character with a space character in function
comments.
Signed-off-by: Du, Changbin changbin...@gmail.com
---
changes for v2:
remove compile warning fixing
---
drivers/media/rc/ati_remote.c | 133 +
1 file changed, 80 insertions(+), 53 deletions(-)
diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index 7be377f..8fa72e2 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -147,7 +147,8 @@ static bool mouse = true;
module_param(mouse, bool, 0444);
MODULE_PARM_DESC(mouse, Enable mouse device, default = yes);
-#define dbginfo(dev, format, arg...) do { if (debug) dev_info(dev , format ,
## arg); } while (0)
+#define dbginfo(dev, format, arg...) \
+ do { if (debug) dev_info(dev , format , ## arg); } while (0)
#undef err
#define err(format, arg...) printk(KERN_ERR format , ## arg)
@@ -191,17 +192,41 @@ static const char *get_medion_keymap(struct usb_interface
*interface)
return RC_MAP_MEDION_X10;
}
-static const struct ati_receiver_type type_ati = { .default_keymap =
RC_MAP_ATI_X10 };
-static const struct ati_receiver_type type_medion = { .get_default_keymap
= get_medion_keymap };
-static const struct ati_receiver_type type_firefly = { .default_keymap =
RC_MAP_SNAPSTREAM_FIREFLY };
+static const struct ati_receiver_type type_ati = {
+ .default_keymap = RC_MAP_ATI_X10
+};
+static const struct ati_receiver_type type_medion = {
+ .get_default_keymap = get_medion_keymap
+};
+static const struct ati_receiver_type type_firefly = {
+ .default_keymap = RC_MAP_SNAPSTREAM_FIREFLY
+};
static struct usb_device_id ati_remote_table[] = {
- { USB_DEVICE(ATI_REMOTE_VENDOR_ID, LOLA_REMOTE_PRODUCT_ID),
.driver_info = (unsigned long)type_ati },
- { USB_DEVICE(ATI_REMOTE_VENDOR_ID, LOLA2_REMOTE_PRODUCT_ID),
.driver_info = (unsigned long)type_ati },
- { USB_DEVICE(ATI_REMOTE_VENDOR_ID, ATI_REMOTE_PRODUCT_ID),
.driver_info = (unsigned long)type_ati },
- { USB_DEVICE(ATI_REMOTE_VENDOR_ID, NVIDIA_REMOTE_PRODUCT_ID),
.driver_info = (unsigned long)type_ati },
- { USB_DEVICE(ATI_REMOTE_VENDOR_ID, MEDION_REMOTE_PRODUCT_ID),
.driver_info = (unsigned long)type_medion },
- { USB_DEVICE(ATI_REMOTE_VENDOR_ID, FIREFLY_REMOTE_PRODUCT_ID),
.driver_info = (unsigned long)type_firefly },
+ {
+ USB_DEVICE(ATI_REMOTE_VENDOR_ID, LOLA_REMOTE_PRODUCT_ID),
+ .driver_info = (unsigned long)type_ati
+ },
+ {
+ USB_DEVICE(ATI_REMOTE_VENDOR_ID, LOLA2_REMOTE_PRODUCT_ID),
+ .driver_info = (unsigned long)type_ati
+ },
+ {
+ USB_DEVICE(ATI_REMOTE_VENDOR_ID, ATI_REMOTE_PRODUCT_ID),
+ .driver_info = (unsigned long)type_ati
+ },
+ {
+ USB_DEVICE(ATI_REMOTE_VENDOR_ID, NVIDIA_REMOTE_PRODUCT_ID),
+ .driver_info = (unsigned long)type_ati
+ },
+ {
+ USB_DEVICE(ATI_REMOTE_VENDOR_ID, MEDION_REMOTE_PRODUCT_ID),
+ .driver_info = (unsigned long)type_medion
+ },
+ {
+ USB_DEVICE(ATI_REMOTE_VENDOR_ID, FIREFLY_REMOTE_PRODUCT_ID),
+ .driver_info = (unsigned long)type_firefly
+ },
{} /* Terminating entry */
};
@@ -296,25 +321,8 @@ static const struct {
{KIND_END, 0x00, EV_MAX + 1, 0, 0}
};
-/* Local function prototypes */
-static int ati_remote_sendpacket (struct ati_remote *ati_remote, u16
cmd, unsigned char *data);
-static void ati_remote_irq_out (struct urb *urb);
-static void ati_remote_irq_in (struct urb *urb);
-static void ati_remote_input_report(struct urb *urb);
-static int ati_remote_initialize (struct ati_remote *ati_remote);
-static int ati_remote_probe(struct usb_interface *interface, const
struct usb_device_id *id);
-static void ati_remote_disconnect (struct usb_interface *interface);
-
-/* usb specific object to register with the usb subsystem */
-static struct usb_driver ati_remote_driver = {
- .name = ati_remote,
- .probe= ati_remote_probe,
- .disconnect = ati_remote_disconnect,
- .id_table = ati_remote_table,
-};
-
/*
- * ati_remote_dump_input
+ * ati_remote_dump_input
*/
static void ati_remote_dump(struct device *dev, unsigned char *data,
unsigned int len)
@@ -326,12 +334,14 @@ static void ati_remote_dump(struct device *dev, unsigned
char *data,
dev_warn(dev, Weird key %02x %02x %02x %02x\n,
data[0], data[1], data[2], data[3]);
else
[Resend PATCH v2] usb: gadget: s3c-hsotg: fix core reset timeout failure
From: Du, Changbin changbin...@gmail.com
The timeout values were 1000 and timeout issue occured many times on my
s3c6410 Soc based board (mostly when booting whith USB cable not
connected). This patch increase the values to 1 to guarantee the
success of reset.
Having set timeout to 1, I printed the remained timeout values
which could cause timeout issue before this change (tested several
times).
the first timeout value remained:
timeout = 8079
timeout = 8079
timeout = 8078
timeout = 8081
the second timeout value remained:
timeout = 7940
timeout = 7945
timeout = 7940
timeout = 7938
Seeing from above values, I think the value 1 is big enough.
Signed-off-by: Du, Changbin changbin...@gmail.com
---
Changes for v2:
Fixed wrapped line done by my mail client
---
drivers/usb/gadget/s3c-hsotg.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c
index f4abb0e..f3e2234 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -2215,7 +2215,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
/* issue soft reset */
writel(GRSTCTL_CSftRst, hsotg-regs + GRSTCTL);
- timeout = 1000;
+ timeout = 1;
do {
grstctl = readl(hsotg-regs + GRSTCTL);
} while ((grstctl GRSTCTL_CSftRst) timeout-- 0);
@@ -2225,7 +2225,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
return -EINVAL;
}
- timeout = 1000;
+ timeout = 1;
while (1) {
u32 grstctl = readl(hsotg-regs + GRSTCTL);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] dynamic_debug: add wildcard support to filter files/functions/modules
From: Du, Changbin changbin...@gmail.com
This patch add wildcard '*'(matches zero or more characters) and '?'
(matches one character) support when qurying debug flags.
Now we can open debug messages using keywords. eg:
1. open debug logs in all usb drivers
echo file drivers/usb/* +p debugfs/dynamic_debug/control
2. open debug logs for usb xhci code
echo file *xhci* +p debugfs/dynamic_debug/control
Signed-off-by: Du, Changbin changbin...@gmail.com
---
lib/dynamic_debug.c | 27 ++-
1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 99fec3a..cdcce58 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -127,6 +127,21 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query-first_lineno, query-last_lineno);
}
+static int match_pattern(char *pat, char *str)
+{
+ switch (*pat) {
+ case '\0':
+ return !*str;
+ case '*':
+ return match_pattern(pat+1, str) ||
+ (*str match_pattern(pat, str+1));
+ case '?':
+ return *str match_pattern(pat+1, str+1);
+ default:
+ return *pat == *str match_pattern(pat+1, str+1);
+ }
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -147,7 +162,7 @@ static int ddebug_change(const struct ddebug_query *query,
list_for_each_entry(dt, ddebug_tables, link) {
/* match against the module name */
- if (query-module strcmp(query-module, dt-mod_name))
+ if (query-module !match_pattern(query-module,
dt-mod_name))
continue;
for (i = 0; i dt-num_ddebugs; i++) {
@@ -155,14 +170,16 @@ static int ddebug_change(const struct ddebug_query *query,
/* match against the source filename */
if (query-filename
- strcmp(query-filename, dp-filename)
- strcmp(query-filename, kbasename(dp-filename))
- strcmp(query-filename, trim_prefix(dp-filename)))
+ !match_pattern(query-filename, dp-filename)
+ !match_pattern(query-filename,
+ kbasename(dp-filename))
+ !match_pattern(query-filename,
+ trim_prefix(dp-filename)))
continue;
/* match against the function */
if (query-function
- strcmp(query-function, dp-function))
+ !match_pattern(query-function, dp-function))
continue;
/* match against the format */
--
1.8.1.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 0/3] add wildcard support for dynamic debug
From: Du, Changbin changbin...@gmail.com These patches are to make it easier to filter kernel debug logs which we want. Whith wildcard support, below command can enable all usb debug logs: #echo file drivers/usb/* +p debugfs/dynamic_debug/control This patch only enables two wildcard: '*' - matches zero or more characters '?' - matches one character Changes since v4: moved matching function to lib/parser.c Du, Changbin (3): lib/parser.c: add match_wildcard function dynamic_debug: add wildcard support to filter files/functions/modules dynamic-debug-howto.txt: update since new wildcard support Documentation/dynamic-debug-howto.txt | 9 +++ include/linux/parser.h| 1 + lib/dynamic_debug.c | 15 +++ lib/parser.c | 51 +++ 4 files changed, 71 insertions(+), 5 deletions(-) -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 1/3] lib/parser.c: add match_wildcard function
From: Du, Changbin changbin...@gmail.com
match_wildcard function is a simple implementation of wildcard
matching algorithm. It only supports two usual wildcardes:
'*' - matches zero or more characters
'?' - matches one character
This algorithm is safe since it's of non-recursion.
Signed-off-by: Du, Changbin changbin...@gmail.com
---
include/linux/parser.h | 1 +
lib/parser.c | 51 ++
2 files changed, 52 insertions(+)
diff --git a/include/linux/parser.h b/include/linux/parser.h
index ea2281e..39d5b79 100644
--- a/include/linux/parser.h
+++ b/include/linux/parser.h
@@ -29,5 +29,6 @@ int match_token(char *, const match_table_t table,
substring_t args[]);
int match_int(substring_t *, int *result);
int match_octal(substring_t *, int *result);
int match_hex(substring_t *, int *result);
+bool match_wildcard(const char *pattern, const char *str);
size_t match_strlcpy(char *, const substring_t *, size_t);
char *match_strdup(const substring_t *);
diff --git a/lib/parser.c b/lib/parser.c
index 807b2aa..ee52955 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -193,6 +193,56 @@ int match_hex(substring_t *s, int *result)
}
/**
+ * match_wildcard: - parse if a string matches given wildcard pattern
+ * @pattern: wildcard pattern
+ * @str: the string to be parsed
+ *
+ * Description: Parse the string @str to check if matches wildcard
+ * pattern @pattern. The pattern may contain two type wildcardes:
+ * '*' - matches zero or more characters
+ * '?' - matches one character
+ * If it's matched, return true, else return false.
+ */
+bool match_wildcard(const char *pattern, const char *str)
+{
+ const char *s = str;
+ const char *p = pattern;
+ bool star = false;
+
+ while (*s) {
+ switch (*p) {
+ case '?':
+ s++;
+ p++;
+ break;
+ case '*':
+ star = true;
+ str = s;
+ if (!*++p)
+ return true;
+ pattern = p;
+ break;
+ default:
+ if (*s == *p) {
+ s++;
+ p++;
+ } else {
+ if (!star)
+ return false;
+ str++;
+ s = str;
+ p = pattern;
+ }
+ break;
+ }
+ }
+
+ if (*p == '*')
+ ++p;
+ return !*p;
+}
+
+/**
* match_strlcpy: - Copy the characters from a substring_t to a sized buffer
* @dest: where to copy to
* @src: substring_t to copy
@@ -235,5 +285,6 @@ EXPORT_SYMBOL(match_token);
EXPORT_SYMBOL(match_int);
EXPORT_SYMBOL(match_octal);
EXPORT_SYMBOL(match_hex);
+EXPORT_SYMBOL(match_wildcard);
EXPORT_SYMBOL(match_strlcpy);
EXPORT_SYMBOL(match_strdup);
--
1.8.3.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 2/3] dynamic_debug: add wildcard support to filter files/functions/modules
From: Du, Changbin changbin...@gmail.com
Add wildcard '*'(matches zero or more characters) and '?'
(matches one character) support when qurying debug flags.
Now we can open debug messages using keywords. eg:
1. open debug logs in all usb drivers
echo file drivers/usb/* +p debugfs/dynamic_debug/control
2. open debug logs for usb xhci code
echo file *xhci* +p debugfs/dynamic_debug/control
Signed-off-by: Du, Changbin changbin...@gmail.com
---
lib/dynamic_debug.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c37aeac..600ac57 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -8,6 +8,7 @@
* By Greg Banks g...@melbourne.sgi.com
* Copyright (c) 2008 Silicon Graphics Inc. All Rights Reserved.
* Copyright (C) 2011 Bart Van Assche. All Rights Reserved.
+ * Copyright (C) 2013 Du, Changbin changbin...@gmail.com
*/
#define pr_fmt(fmt) KBUILD_MODNAME :%s: fmt, __func__
@@ -24,6 +25,7 @@
#include linux/sysctl.h
#include linux/ctype.h
#include linux/string.h
+#include linux/parser.h
#include linux/string_helpers.h
#include linux/uaccess.h
#include linux/dynamic_debug.h
@@ -147,7 +149,8 @@ static int ddebug_change(const struct ddebug_query *query,
list_for_each_entry(dt, ddebug_tables, link) {
/* match against the module name */
- if (query-module strcmp(query-module, dt-mod_name))
+ if (query-module
+ !match_wildcard(query-module, dt-mod_name))
continue;
for (i = 0; i dt-num_ddebugs; i++) {
@@ -155,14 +158,16 @@ static int ddebug_change(const struct ddebug_query *query,
/* match against the source filename */
if (query-filename
- strcmp(query-filename, dp-filename)
- strcmp(query-filename, kbasename(dp-filename))
- strcmp(query-filename, trim_prefix(dp-filename)))
+ !match_wildcard(query-filename, dp-filename)
+ !match_wildcard(query-filename,
+ kbasename(dp-filename))
+ !match_wildcard(query-filename,
+ trim_prefix(dp-filename)))
continue;
/* match against the function */
if (query-function
- strcmp(query-function, dp-function))
+ !match_wildcard(query-function, dp-function))
continue;
/* match against the format */
--
1.8.3.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 3/3] dynamic-debug-howto.txt: update since new wildcard support
From: Du, Changbin changbin...@gmail.com Add the usage of using new feature wildcard support. Signed-off-by: Du, Changbin changbin...@gmail.com --- Documentation/dynamic-debug-howto.txt | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/dynamic-debug-howto.txt b/Documentation/dynamic-debug-howto.txt index 1bbdcfc..46325eb 100644 --- a/Documentation/dynamic-debug-howto.txt +++ b/Documentation/dynamic-debug-howto.txt @@ -108,6 +108,12 @@ If your query set is big, you can batch them too: ~# cat query-batch-file debugfs/dynamic_debug/control +A another way is to use wildcard. The match rule support '*' (matches +zero or more characters) and '?' (matches exactly one character).For +example, you can match all usb drivers: + + ~# echo file drivers/usb/* +p debugfs/dynamic_debug/control + At the syntactical level, a command comprises a sequence of match specifications, followed by a flags change specification. @@ -315,6 +321,9 @@ nullarbor:~ # echo -n 'func svc_process -p' nullarbor:~ # echo -n 'format nfsd: READ +p' debugfs/dynamic_debug/control +// enable messages in files of which the pathes include string usb +nullarbor:~ # echo -n '*usb* +p' debugfs/dynamic_debug/control + // enable all messages nullarbor:~ # echo -n '+p' debugfs/dynamic_debug/control -- 1.8.3.2 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] dynamic_debug: add wildcard support to filter files/functions/modules
From: Du, Changbin changbin...@gmail.com
This patch add wildcard '*'(matches zero or more characters) and '?'
(matches one character) support when qurying debug flags.
Now we can open debug messages using keywords. eg:
1. open debug logs in all usb drivers
echo file drivers/usb/* +p debugfs/dynamic_debug/control
2. open debug logs for usb xhci code
echo file *xhci* +p debugfs/dynamic_debug/control
Signed-off-by: Du, Changbin changbin...@gmail.com
---
changes since v2:
remove the goto statements.
code style issue.
---
lib/dynamic_debug.c | 49 -
1 file changed, 44 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c37aeac..b166d19 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -127,6 +127,43 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query-first_lineno, query-last_lineno);
}
+/* check if the string matches given pattern which includes wildcards */
+static bool match_pattern(const char *pattern, const char *string)
+{
+ const char *s = string,
+ *p = pattern;
+ bool star = 0;
+
+ while (*s) {
+ switch (*p) {
+ case '?':
+ ++s, ++p;
+ break;
+ case '*':
+ star = true;
+ string = s;
+ if (!*++p)
+ return true;
+ pattern = p;;
+ break;
+ default:
+ if (*s != *p) {
+ if (!star)
+ return false;
+ string++;
+ s = string;
+ p = pattern;
+ break;
+ }
+ ++s, ++p;
+ }
+ }
+
+ if (*p == '*')
+ ++p;
+ return !*p;
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -147,7 +184,7 @@ static int ddebug_change(const struct ddebug_query *query,
list_for_each_entry(dt, ddebug_tables, link) {
/* match against the module name */
- if (query-module strcmp(query-module, dt-mod_name))
+ if (query-module !match_pattern(query-module,
dt-mod_name))
continue;
for (i = 0; i dt-num_ddebugs; i++) {
@@ -155,14 +192,16 @@ static int ddebug_change(const struct ddebug_query *query,
/* match against the source filename */
if (query-filename
- strcmp(query-filename, dp-filename)
- strcmp(query-filename, kbasename(dp-filename))
- strcmp(query-filename, trim_prefix(dp-filename)))
+ !match_pattern(query-filename, dp-filename)
+ !match_pattern(query-filename,
+ kbasename(dp-filename))
+ !match_pattern(query-filename,
+ trim_prefix(dp-filename)))
continue;
/* match against the function */
if (query-function
- strcmp(query-function, dp-function))
+ !match_pattern(query-function, dp-function))
continue;
/* match against the format */
--
1.8.1.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 1/2] dynamic_debug: add wildcard support to filter files/functions/modules
From: Du, Changbin changbin...@gmail.com
This patch add wildcard '*'(matches zero or more characters) and '?'
(matches one character) support when qurying debug flags.
Now we can open debug messages using keywords. eg:
1. open debug logs in all usb drivers
echo file drivers/usb/* +p debugfs/dynamic_debug/control
2. open debug logs for usb xhci code
echo file *xhci* +p debugfs/dynamic_debug/control
Signed-off-by: Du, Changbin changbin...@gmail.com
---
lib/dynamic_debug.c | 54 -
1 file changed, 49 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c37aeac..b953780 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -8,6 +8,7 @@
* By Greg Banks g...@melbourne.sgi.com
* Copyright (c) 2008 Silicon Graphics Inc. All Rights Reserved.
* Copyright (C) 2011 Bart Van Assche. All Rights Reserved.
+ * Copyright (C) 2013 Du, Changbin changbin...@gmail.com
*/
#define pr_fmt(fmt) KBUILD_MODNAME :%s: fmt, __func__
@@ -127,6 +128,46 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query-first_lineno, query-last_lineno);
}
+/* check if the string matches given pattern which includes wildcards */
+static bool match_pattern(const char *pattern, const char *string)
+{
+ const char *s = string;
+ const char *p = pattern;
+ bool star = false;
+
+ while (*s) {
+ switch (*p) {
+ case '?':
+ s++;
+ p++;
+ break;
+ case '*':
+ star = true;
+ string = s;
+ if (!*++p)
+ return true;
+ pattern = p;
+ break;
+ default:
+ if (*s == *p) {
+ s++;
+ p++;
+ } else {
+ if (!star)
+ return false;
+ string++;
+ s = string;
+ p = pattern;
+ }
+ break;
+ }
+ }
+
+ if (*p == '*')
+ ++p;
+ return !*p;
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -147,7 +188,8 @@ static int ddebug_change(const struct ddebug_query *query,
list_for_each_entry(dt, ddebug_tables, link) {
/* match against the module name */
- if (query-module strcmp(query-module, dt-mod_name))
+ if (query-module
+ !match_pattern(query-module, dt-mod_name))
continue;
for (i = 0; i dt-num_ddebugs; i++) {
@@ -155,14 +197,16 @@ static int ddebug_change(const struct ddebug_query *query,
/* match against the source filename */
if (query-filename
- strcmp(query-filename, dp-filename)
- strcmp(query-filename, kbasename(dp-filename))
- strcmp(query-filename, trim_prefix(dp-filename)))
+ !match_pattern(query-filename, dp-filename)
+ !match_pattern(query-filename,
+ kbasename(dp-filename))
+ !match_pattern(query-filename,
+ trim_prefix(dp-filename)))
continue;
/* match against the function */
if (query-function
- strcmp(query-function, dp-function))
+ !match_pattern(query-function, dp-function))
continue;
/* match against the format */
--
1.8.1.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 2/2] dynamic-debug-howto.txt: update since new wildcard support
From: Du, Changbin changbin...@gmail.com Add the usage of using new feature wildcard support. Signed-off-by: Du, Changbin changbin...@gmail.com --- Documentation/dynamic-debug-howto.txt | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/dynamic-debug-howto.txt b/Documentation/dynamic-debug-howto.txt index 1bbdcfc..46325eb 100644 --- a/Documentation/dynamic-debug-howto.txt +++ b/Documentation/dynamic-debug-howto.txt @@ -108,6 +108,12 @@ If your query set is big, you can batch them too: ~# cat query-batch-file debugfs/dynamic_debug/control +A another way is to use wildcard. The match rule support '*' (matches +zero or more characters) and '?' (matches exactly one character).For +example, you can match all usb drivers: + + ~# echo file drivers/usb/* +p debugfs/dynamic_debug/control + At the syntactical level, a command comprises a sequence of match specifications, followed by a flags change specification. @@ -315,6 +321,9 @@ nullarbor:~ # echo -n 'func svc_process -p' nullarbor:~ # echo -n 'format nfsd: READ +p' debugfs/dynamic_debug/control +// enable messages in files of which the pathes include string usb +nullarbor:~ # echo -n '*usb* +p' debugfs/dynamic_debug/control + // enable all messages nullarbor:~ # echo -n '+p' debugfs/dynamic_debug/control -- 1.8.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] dynamic_debug: add wildcard support to filter files/functions/modules
From: Du, Changbin changbin...@gmail.com
This patch add wildcard '*'(matches zero or more characters) and '?'
(matches one character) support when qurying debug flags.
Now we can open debug messages using keywords. eg:
1. open debug logs in all usb drivers
echo file drivers/usb/* +p debugfs/dynamic_debug/control
2. open debug logs for usb xhci code
echo file *xhci* +p debugfs/dynamic_debug/control
Signed-off-by: Du, Changbin changbin...@gmail.com
---
changes since v1:
rewrite match_pattern using non-recursion method.
---
lib/dynamic_debug.c | 47 ++-
1 file changed, 42 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c37aeac..d239207 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -127,6 +127,41 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query-first_lineno, query-last_lineno);
}
+/* check if the string matches given pattern which includes wildcards */
+static int match_pattern(const char *pattern, const char *string)
+{
+ const char *s, *p;
+ int star = 0;
+
+loop:
+ for (s = string, p = pattern; *s; ++s, ++p) {
+ switch (*p) {
+ case '?':
+ break;
+ case '*':
+ star = 1;
+ string = s;
+ pattern = p;
+ if (!*++pattern)
+ return 1;
+ goto loop;
+ default:
+ if (*s != *p)
+ goto star_check;
+ break;
+ }
+ }
+ if (*p == '*')
+ ++p;
+ return (!*p);
+
+star_check:
+ if (!star)
+ return 0;
+ string++;
+ goto loop;
+}
+
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -147,7 +182,7 @@ static int ddebug_change(const struct ddebug_query *query,
list_for_each_entry(dt, ddebug_tables, link) {
/* match against the module name */
- if (query-module strcmp(query-module, dt-mod_name))
+ if (query-module !match_pattern(query-module,
dt-mod_name))
continue;
for (i = 0; i dt-num_ddebugs; i++) {
@@ -155,14 +190,16 @@ static int ddebug_change(const struct ddebug_query *query,
/* match against the source filename */
if (query-filename
- strcmp(query-filename, dp-filename)
- strcmp(query-filename, kbasename(dp-filename))
- strcmp(query-filename, trim_prefix(dp-filename)))
+ !match_pattern(query-filename, dp-filename)
+ !match_pattern(query-filename,
+ kbasename(dp-filename))
+ !match_pattern(query-filename,
+ trim_prefix(dp-filename)))
continue;
/* match against the function */
if (query-function
- strcmp(query-function, dp-function))
+ !match_pattern(query-function, dp-function))
continue;
/* match against the format */
--
1.8.1.2
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [DISCUSSION] USB device remote wakeup is not working for S3 case
You have to make sure that wakeup is also enabled for the host controller the device is attached to. For some host controllers, it may also be necessary to enable wakeup for the root hub. Yes, the root-hub is not wakeup enabled by default, actually hub devices are not enabled. It works after I enable it via sysfs interface. Could we also enable wakeup for usb mouse? Or is there any concern to enable it? Per my opinion, most people may expect clicking mouse can awake system. It doesn't matter to me, but you should ask the people on the linux-input mailing list. Also, what about wakeup for a non-USB mouse? Shouldn't that be enabled as well? Unfortunately the mouse I found are all of usb so cannot confirm it. The concern is if we want usb keyboard/mouse wakeup work by default, it does make sense only if the parent hubs are also enabled by default. This is a policy issue and may be better place it on userspace. I have no better idea :) . Alan Stern Regards Du, Changbin -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[DISCUSSION] USB device remote wakeup is not working for S3 case
When I am checking usb remote wakeup code, I found that usb remote wakeup will not work after system going to S3 sate and I confirmed with my PC. During enumeration, usb device will be set as wakeup capable by usb_set_device_state if it supports. Whether usb driver send SET_FEATURE(REMOTE_WAKUP) usb request when suspending on S3 depend on do_remote_wakeup flag which is set by choose_wakeup(). It can be simply presented as below. do_remote_wakeup = device_may_wakeup(udev-dev); The return value is always false since usb device is not marked as wakeup enabled(that is no one call device_set_wakeup_enable() for usb device). As a result, usb device will not signal wakeup event to host. Maybe we should not allow all remote wakeup supported device can wakeup system by default. But for usb keyboard/mouse, I think it is not reasonable to disable it by default. A simple way to fix this is that replace the device_may_wakeup by device_can_wakeup in choose_wakeup() function, just like on auto suspend case (usb remote wakeup works for rpm). Another way is to make usb device wakeup able by default. But both of them will make all usb devices can wakeup system. Have any better idea? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [DISCUSSION] USB device remote wakeup is not working for S3 case
There's a simple solution: Call device_set_wakeup_enable() for the device!
You can do this from the command line by:
echo auto /sys/bus/usb/devices/.../power/control
where the ... is the pathname for your device.
Yes, this can enable auto-suspend for usb device like a mouse. And remote wakeup
does work for RPM because rpm suspend refers to the needs_remote_wakeup flag.
But this doesn't impact system level suspend. The correct thing is:
echo enabled /sys/bus/usb/devices/.../power/wakeup
This will call device_set_wakeup_enable() for the device and should work. But
unfortunately it seems didn't work even the wakeup file shows enabled which
means the device is wakeup enabled(Tried on 3.16 3.18 kernel). This is a
different
issue, I will check.
if (interface-desc.bInterfaceSubClass ==
USB_INTERFACE_SUBCLASS_BOOT
interface-desc.bInterfaceProtocol ==
USB_INTERFACE_PROTOCOL_KEYBOARD) {
usbhid_set_leds(hid);
device_set_wakeup_enable(dev-dev, 1);
}
How about leaving everything the way it is now? If you want to enable
wakeup for something like a USB mouse, you can write a udev script to do it
as shown above.
Alan Stern
Could we also enable wakeup for usb mouse? Or is there any concern to enable it?
Per my opinion, most people may expect clicking mouse can awake system.
Regards,
Du, Changbin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] workqueue: detect uninitated work_struct and BUG() if true
-Original Message-
From: Tejun Heo [mailto:hte...@gmail.com] On Behalf Of Tejun Heo
Sent: Monday, March 9, 2015 2:23 PM
To: Du, Changbin
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] workqueue: detect uninitated work_struct and BUG() if
true
@@ -1295,6 +1295,9 @@ static void __queue_work(int cpu, struct
workqueue_struct *wq,
*/
WARN_ON_ONCE(!irqs_disabled());
+ if (!work-func)
+ BUG();
+
debug_work_activate(work);
Can't this be part of the debug_work mechanism? I'm a bit wary of adding
essentially arbitrary checks. I'd much prefer if it gets gated by debug
config
somehow.
Thanks.
Tejun
Yes, I found there already have this checking and print a warning in
work_fixup_activate.
So this patch can be droped. Thanks for point out.
static int work_fixup_activate(void *addr, enum debug_obj_state state)
{
struct work_struct *work = addr;
switch (state) {
case ODEBUG_STATE_NOTAVAILABLE:
/*
* This is not really a fixup. The work struct was
* statically initialized. We just make sure that it
* is tracked in the object tracker.
*/
if (test_bit(WORK_STRUCT_STATIC_BIT, work_data_bits(work))) {
debug_object_init(work, work_debug_descr);
debug_object_activate(work, work_debug_descr);
return 0;
}
WARN_ON_ONCE(1);
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] workqueue: detect uninitated work_struct and BUG() if true
From cdebb88ac0fb3f900ef28f28ccb4a12159c295db Mon Sep 17 00:00:00 2001 From: Du, Changbin changbin...@gmail.com Date: Mon, 9 Mar 2015 12:06:43 +0800 Subject: [PATCH] workqueue: detect uninitated work_struct and BUG() if true Recently I encounter a driver issue that caused by missing initializing the work_struct. Then the work never get executed and stay in workqueue forever. This take me some time to locate the error. This issue can be seen early if detect it when queuing a work. Signed-off-by: Du, Changbin changbin...@intel.com --- kernel/workqueue.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/workqueue.c b/kernel/workqueue.c index f288493..5c1a5bc 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -1295,6 +1295,9 @@ static void __queue_work(int cpu, struct workqueue_struct *wq, */ WARN_ON_ONCE(!irqs_disabled()); + if (!work-func) + BUG(); + debug_work_activate(work); /* if draining, only works from the same workqueue are allowed */ -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] usb: gadget: composite: enable BESL support
From a6615937bcd9234e6d6bb817c3701fce44d0a84d Mon Sep 17 00:00:00 2001 From: Felipe Balbi ba...@ti.com Date: Tue, 30 Sep 2014 16:08:03 -0500 Subject: [PATCH] usb: gadget: composite: enable BESL support According to USB 2.0 ECN Errata for Link Power Management (USB2-LPM-Errata-final.pdf), BESL must be enabled if LPM is enabled. This helps with USB30CV TD 9.21 LPM L1 Suspend Resume Test. Cc: sta...@vger.kernel.org # 3.1+: a661593: usb: enable BESL support Signed-off-by: Felipe Balbi ba...@ti.com Signed-off-by: Du, Changbin changbin...@intel.com --- Hi, This patch was introduced on v3.18. However the issue fixed already existed on v3.1. Thank Balbi for pointing it out. So propose to backport it over all 3.1+ stable trees as well. Du, Changbin --- drivers/usb/gadget/composite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index a8c18df..f6a51fd 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -560,7 +560,7 @@ static int bos_desc(struct usb_composite_dev *cdev) usb_ext-bLength = USB_DT_USB_EXT_CAP_SIZE; usb_ext-bDescriptorType = USB_DT_DEVICE_CAPABILITY; usb_ext-bDevCapabilityType = USB_CAP_TYPE_EXT; - usb_ext-bmAttributes = cpu_to_le32(USB_LPM_SUPPORT); + usb_ext-bmAttributes = cpu_to_le32(USB_LPM_SUPPORT | USB_BESL_SUPPORT); /* * The Superspeed USB Capability descriptor shall be implemented by all -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] usb: gadget: composite: enable BESL support
From: Felipe Balbi ba...@ti.com Date: Tue, 30 Sep 2014 16:08:03 -0500 Subject: [PATCH] usb: gadget: composite: enable BESL support missing upstream commit. According to USB 2.0 ECN Errata for Link Power Management (USB2-LPM-Errata-final.pdf), BESL must be enabled if LPM is enabled. This helps with USB30CV TD 9.21 LPM L1 Suspend Resume Test. Cc: sta...@vger.kernel.org # 3.14 this should be backported all the way back to 3.1. The commit which this patch is fixing, was applied on v3.1, so we're probably going to backport to 3.10 and 3.14. When asking for backports, don't consider only your project, think about the kernel/stable releases as a whole. BTW, that should be v3.1+, the + tells the Stable team that from v3.1 forward, all kernels need the backport. Signed-off-by: Felipe Balbi ba...@ti.com Signed-off-by: Du, Changbin changbin...@intel.com --- Hi, This patch was introduced on v3.18. However the issue fixed already existed on v3.14 and v3.14 is a long term support version. the issue already existed on v3.1, why did you decide to backport only to v3.14 ? So propose to backport it over there as well. -- balbi Thanks for pointing it out, Balbi. Sorry for not considering the whole trees. This is my first time to send backport request and now know how do it well. I will resend request with field updated. Du, Changbin Regards -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] usb: gadget: composite: enable BESL support
From a6615937bcd9234e6d6bb817c3701fce44d0a84d Mon Sep 17 00:00:00 2001 From: Felipe Balbi ba...@ti.com Date: Tue, 30 Sep 2014 16:08:03 -0500 Subject: [PATCH] usb: gadget: composite: enable BESL support commit a6615937bcd9234e6d6bb817c3701fce44d0a84d upstream. According to USB 2.0 ECN Errata for Link Power Management (USB2-LPM-Errata-final.pdf), BESL must be enabled if LPM is enabled. This helps with USB30CV TD 9.21 LPM L1 Suspend Resume Test. Signed-off-by: Felipe Balbi ba...@ti.com Signed-off-by: Du, Changbin changbin...@intel.com --- Hi, This patch was introduced on v3.18. However the issue fixed already existed on v3.1. Thank Balbi for pointing it out. So propose to backport it over 3.1+ stable trees as well. Change from v2: add upstream commit id in commit message. Du, Changbin --- drivers/usb/gadget/composite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index a8c18df..f6a51fd 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -560,7 +560,7 @@ static int bos_desc(struct usb_composite_dev *cdev) usb_ext-bLength = USB_DT_USB_EXT_CAP_SIZE; usb_ext-bDescriptorType = USB_DT_DEVICE_CAPABILITY; usb_ext-bDevCapabilityType = USB_CAP_TYPE_EXT; - usb_ext-bmAttributes = cpu_to_le32(USB_LPM_SUPPORT); + usb_ext-bmAttributes = cpu_to_le32(USB_LPM_SUPPORT | USB_BESL_SUPPORT); /* * The Superspeed USB Capability descriptor shall be implemented by all -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4] usb: gadget: composite: enable BESL support
From a6615937bcd9234e6d6bb817c3701fce44d0a84d Mon Sep 17 00:00:00 2001 From: Felipe Balbi ba...@ti.com Date: Tue, 30 Sep 2014 16:08:03 -0500 Subject: [PATCH] usb: gadget: composite: enable BESL support commit a6615937bcd9234e6d6bb817c3701fce44d0a84d upstream. According to USB 2.0 ECN Errata for Link Power Management (USB2-LPM-Errata-final.pdf), BESL must be enabled if LPM is enabled. This helps with USB30CV TD 9.21 LPM L1 Suspend Resume Test. Cc: sta...@vger.kernel.org # 3.1+ Signed-off-by: Felipe Balbi ba...@ti.com Signed-off-by: Du, Changbin changbin...@intel.com --- Hi, This patch was introduced on v3.18. However the issue fixed already existed on v3.1. Thank Balbi for pointing it out. So propose to backport it over all 3.1+ stable trees as well. Du, Changbin --- drivers/usb/gadget/composite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index a8c18df..f6a51fd 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -560,7 +560,7 @@ static int bos_desc(struct usb_composite_dev *cdev) usb_ext-bLength = USB_DT_USB_EXT_CAP_SIZE; usb_ext-bDescriptorType = USB_DT_DEVICE_CAPABILITY; usb_ext-bDevCapabilityType = USB_CAP_TYPE_EXT; - usb_ext-bmAttributes = cpu_to_le32(USB_LPM_SUPPORT); + usb_ext-bmAttributes = cpu_to_le32(USB_LPM_SUPPORT | USB_BESL_SUPPORT); /* * The Superspeed USB Capability descriptor shall be implemented by all -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb: gadget: composite: enable BESL support
From a6615937bcd9234e6d6bb817c3701fce44d0a84d Mon Sep 17 00:00:00 2001 From: Felipe Balbi ba...@ti.com Date: Tue, 30 Sep 2014 16:08:03 -0500 Subject: [PATCH] usb: gadget: composite: enable BESL support According to USB 2.0 ECN Errata for Link Power Management (USB2-LPM-Errata-final.pdf), BESL must be enabled if LPM is enabled. This helps with USB30CV TD 9.21 LPM L1 Suspend Resume Test. Cc: sta...@vger.kernel.org # 3.14 Signed-off-by: Felipe Balbi ba...@ti.com Signed-off-by: Du, Changbin changbin...@intel.com --- Hi, This patch was introduced on v3.18. However the issue fixed already existed on v3.14 and v3.14 is a long term support version. So propose to backport it over there as well. Du, Changbin --- drivers/usb/gadget/composite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index a8c18df..f6a51fd 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -560,7 +560,7 @@ static int bos_desc(struct usb_composite_dev *cdev) usb_ext-bLength = USB_DT_USB_EXT_CAP_SIZE; usb_ext-bDescriptorType = USB_DT_DEVICE_CAPABILITY; usb_ext-bDevCapabilityType = USB_CAP_TYPE_EXT; - usb_ext-bmAttributes = cpu_to_le32(USB_LPM_SUPPORT); + usb_ext-bmAttributes = cpu_to_le32(USB_LPM_SUPPORT | USB_BESL_SUPPORT); /* * The Superspeed USB Capability descriptor shall be implemented by all -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] usb/gadget: make composite gadget meet usb compliance for vbus draw
From 08df419517694c4dd9ff328f5644b46a99c2999e Mon Sep 17 00:00:00 2001
From: Du, Changbin changbin...@intel.com
Date: Thu, 23 Jul 2015 20:08:04 +0800
Subject: [PATCH v2] usb/gadget: make composite gadget meet usb compliance for
vbus draw
USB-IF compliance requirement limits the vbus current according to
current state. USB2 Spec requires that un-configured current must
be = 100mA, for USB3 is 150mA, OTG is 2.5mA. So set corresponding
vbus draw current according to device mode.
Signed-off-by: Du, Changbin changbin...@intel.com
---
drivers/usb/gadget/composite.c | 39 ---
drivers/usb/gadget/configfs.c | 2 +-
include/linux/usb/composite.h | 1 +
include/uapi/linux/usb/ch9.h | 9 +
4 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 4e3447b..b3896e9 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -610,6 +610,21 @@ static void device_qual(struct usb_composite_dev *cdev)
qual-bRESERVED = 0;
}
+static unsigned unconfigured_vbus_draw(struct usb_composite_dev *cdev)
+{
+ struct usb_gadget *g = cdev-gadget;
+ unsigned power;
+
+ if (gadget_is_otg(g))
+ power = USB_OTG_UNCONF_STATE_VBUS_MAX_DRAW;
+ else if (g-speed == USB_SPEED_SUPER)
+ power = USB3_UNCONF_STATE_VBUS_MAX_DRAW;
+ else
+ power = USB2_UNCONF_STATE_VBUS_MAX_DRAW;
+
+ return power;
+}
+
/*-*/
static void reset_config(struct usb_composite_dev *cdev)
@@ -634,7 +649,7 @@ static int set_config(struct usb_composite_dev *cdev,
struct usb_gadget *gadget = cdev-gadget;
struct usb_configuration *c = NULL;
int result = -EINVAL;
- unsignedpower = gadget_is_otg(gadget) ? 8 : 100;
+ unsignedpower = unconfigured_vbus_draw(cdev);
int tmp;
if (number) {
@@ -1829,6 +1844,15 @@ done:
return value;
}
+void composite_reset(struct usb_gadget *gadget)
+{
+ struct usb_composite_dev *cdev = get_gadget_data(gadget);
+
+ DBG(cdev, reset\n);
+ usb_gadget_vbus_draw(gadget, unconfigured_vbus_draw(cdev));
+ composite_disconnect(gadget);
+}
+
void composite_disconnect(struct usb_gadget *gadget)
{
struct usb_composite_dev*cdev = get_gadget_data(gadget);
@@ -2095,7 +2119,7 @@ void composite_suspend(struct usb_gadget *gadget)
cdev-suspended = 1;
- usb_gadget_vbus_draw(gadget, 2);
+ usb_gadget_vbus_draw(gadget, USB_SUSPEND_STATE_VBUS_MAX_DRAW);
}
void composite_resume(struct usb_gadget *gadget)
@@ -2117,10 +2141,11 @@ void composite_resume(struct usb_gadget *gadget)
}
maxpower = cdev-config-MaxPower;
-
- usb_gadget_vbus_draw(gadget, maxpower ?
- maxpower : CONFIG_USB_GADGET_VBUS_DRAW);
- }
+ if (!maxpower)
+ maxpower = CONFIG_USB_GADGET_VBUS_DRAW;
+ } else
+ maxpower = unconfigured_vbus_draw(cdev);
+ usb_gadget_vbus_draw(gadget, maxpower);
cdev-suspended = 0;
}
@@ -2132,7 +2157,7 @@ static const struct usb_gadget_driver
composite_driver_template = {
.unbind = composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
.suspend= composite_suspend,
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 0495c94..e16335d 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1449,7 +1449,7 @@ static const struct usb_gadget_driver
configfs_driver_template = {
.unbind = configfs_composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
.suspend= composite_suspend,
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2511469..825ad39 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -506,6 +506,7 @@ extern struct usb_string *usb_gstrings_attach(struct
usb_composite_dev *cdev,
extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
+extern void composite_reset(struct usb_gadget *gadget);
extern void composite_disconnect(struct usb_gadget *gadget);
extern int composite_setup(struct usb_gadget *gadget,
const struct usb_ctrlrequest *ctrl);
diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
index aa33fd1..ab216bf 100644
--- a/include/uapi/linux/usb/ch9.h
+++ b/include/uapi/linux/usb/ch9.h
RE: [PATCH] usb/gadget: make composite gadget meet usb compliance for vbus draw
From: Andrzej Pietrasiewicz [mailto:andrze...@samsung.com] Hi, What I mean is that in my opinion it should be done in a separate patch, because the newly introduced USB_VBUS_DRAW_SUSPEND is not used anywhere else in your patch. The meaning of this change is use a symbolic name rather than an explicit number and it is unrelated to making composite gadget meet usb compliance for vbus draw. In other words, if you don't do this change at all the compliance is still maintained, because the value of USB_VBUS_DRAW_SUSPEND is 2 anyway, so what the compiler eventually sees is the same whether the change is made or not. My idea: [PATCHv2 0/2] usb gadget vbus draw compilance [PATCHv2 1/2] usb: gadget: composite: avoid using a magic number substituting an explicit 2 with USB_VBUS_DRAW_SUSPEND goes here [PATCHv2 2/2] usb: gadget: composite: meet usb compliance for vbus draw the rest of your changes go here Hi, Andrzej. When I try to split into two patches, I realized that it is not applicable. Because the original code doesn't distinguish different usb types, but new symbols does. So I cannot make a patch just avoid using a magic number without modify the control logic. Hence, I'd prefer doing them in a signal patch. I will send new modified patch. Thanks! Changbin. Andrzej -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] usb/gadget: make composite gadget meet usb compliance for vbus draw
From 0a8e0d63a9887735c6782d7b0c15c2c1fdf1952a Mon Sep 17 00:00:00 2001
From: Du, Changbin changbin...@intel.com
Date: Thu, 23 Jul 2015 20:08:04 +0800
Subject: [PATCH] usb/gadget: make composite gadget meet usb compliance for
vbus draw
USB-IF compliance requirement limits the vbus current according to
current state. USB2 Spec requires that un-configured current must
be = 100mA, for USB3 is 150mA, OTG is 2.5mA. So set corresponding
vbus draw current according to device mode.
Signed-off-by: Du, Changbin changbin...@intel.com
---
drivers/usb/gadget/composite.c | 39 ---
include/linux/usb/composite.h | 8
2 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 4e3447b..fdfd625 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -610,6 +610,21 @@ static void device_qual(struct usb_composite_dev *cdev)
qual-bRESERVED = 0;
}
+static unsigned unconfigured_vbus_draw(struct usb_composite_dev *cdev)
+{
+ struct usb_gadget *g = cdev-gadget;
+ unsigned power;
+
+ if (gadget_is_otg(g))
+ power = USB_OTG_VBUS_DRAW_UNCONF;
+ else if (g-speed == USB_SPEED_SUPER)
+ power = USB3_VBUS_DRAW_UNCONF;
+ else
+ power = USB2_VBUS_DRAW_UNCONF;
+
+ return power;
+}
+
/*-*/
static void reset_config(struct usb_composite_dev *cdev)
@@ -634,7 +649,7 @@ static int set_config(struct usb_composite_dev *cdev,
struct usb_gadget *gadget = cdev-gadget;
struct usb_configuration *c = NULL;
int result = -EINVAL;
- unsignedpower = gadget_is_otg(gadget) ? 8 : 100;
+ unsignedpower = unconfigured_vbus_draw(cdev);
int tmp;
if (number) {
@@ -1829,6 +1844,15 @@ done:
return value;
}
+static void composite_reset(struct usb_gadget *gadget)
+{
+ struct usb_composite_dev *cdev = get_gadget_data(gadget);
+
+ DBG(cdev, reset\n);
+ usb_gadget_vbus_draw(gadget, unconfigured_vbus_draw(cdev));
+ composite_disconnect(gadget);
+}
+
void composite_disconnect(struct usb_gadget *gadget)
{
struct usb_composite_dev*cdev = get_gadget_data(gadget);
@@ -2095,7 +2119,7 @@ void composite_suspend(struct usb_gadget *gadget)
cdev-suspended = 1;
- usb_gadget_vbus_draw(gadget, 2);
+ usb_gadget_vbus_draw(gadget, USB_VBUS_DRAW_SUSPEND);
}
void composite_resume(struct usb_gadget *gadget)
@@ -2117,10 +2141,11 @@ void composite_resume(struct usb_gadget *gadget)
}
maxpower = cdev-config-MaxPower;
-
- usb_gadget_vbus_draw(gadget, maxpower ?
- maxpower : CONFIG_USB_GADGET_VBUS_DRAW);
- }
+ if (!maxpower)
+ maxpower = CONFIG_USB_GADGET_VBUS_DRAW;
+ } else
+ maxpower = unconfigured_vbus_draw(cdev);
+ usb_gadget_vbus_draw(gadget, maxpower);
cdev-suspended = 0;
}
@@ -2132,7 +2157,7 @@ static const struct usb_gadget_driver
composite_driver_template = {
.unbind = composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
.suspend= composite_suspend,
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2511469..90b434d 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -333,6 +333,14 @@ enum {
USB_GADGET_FIRST_AVAIL_IDX,
};
+/* USB2 compliance requires that un-configured current draw = 100mA,
+ * USB3 requires it = 150mA, OTG requires it = 2.5mA.
+ */
+#define USB2_VBUS_DRAW_UNCONF 100
+#define USB3_VBUS_DRAW_UNCONF 150
+#define USB_OTG_VBUS_DRAW_UNCONF 2
+#define USB_VBUS_DRAW_SUSPEND 2
+
/**
* struct usb_composite_driver - groups configurations into a gadget
* @name: For diagnostics, identifies the driver.
--
2.1.4
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] usb/gadget: make composite gadget meet usb compliance for vbus draw
Thanks, Pietrasiewicz.
From: Andrzej Pietrasiewicz [mailto:andrze...@samsung.com]
W dniu 23.07.2015 o 14:34, Du, Changbin pisze:
From 0a8e0d63a9887735c6782d7b0c15c2c1fdf1952a Mon Sep 17 00:00:00
void composite_disconnect(struct usb_gadget *gadget)
{
struct usb_composite_dev*cdev = get_gadget_data(gadget);
@@ -2095,7 +2119,7 @@ void composite_suspend(struct usb_gadget
*gadget)
cdev-suspended = 1;
- usb_gadget_vbus_draw(gadget, 2);
+ usb_gadget_vbus_draw(gadget, USB_VBUS_DRAW_SUSPEND);
}
This looks like an unrelated change. I think it should go first
in a separate patch which eliminates usage of magic numbers.
This change does make sense. As you know, when device is reset, it is in a
'unconfigured'
state. Compliance Test equipment may also measure vbus current at this moment.
}
@@ -2132,7 +2157,7 @@ static const struct usb_gadget_driver
composite_driver_template = {
.unbind = composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
A similar template is in drivers/usb/gadget/configfs.c. Shouldn't the
reset
method be changed there as well?
Yes, it also need to change. I will change it as well.
+/* USB2 compliance requires that un-configured current draw = 100mA,
+ * USB3 requires it = 150mA, OTG requires it = 2.5mA.
+ */
+#define USB2_VBUS_DRAW_UNCONF 100
+#define USB3_VBUS_DRAW_UNCONF 150
+#define USB_OTG_VBUS_DRAW_UNCONF 2
+#define USB_VBUS_DRAW_SUSPEND 2
separate patch
Sorry, I didn't get your idea. Why separate these macros definition?
Thanks,
AP
Regards
Changbin
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] usb/gadget: make composite gadget meet usb compliance for vbus draw
Thanks for explanation. I am agree with you that separate changes into two
patches.
Will send out new patch soon.
Regards
Du, Changbin
From: Andrzej Pietrasiewicz [mailto:andrze...@samsung.com]
Hi,
W dniu 24.07.2015 o 06:11, Du, Changbin pisze:
Thanks, Pietrasiewicz.
From: Andrzej Pietrasiewicz [mailto:andrze...@samsung.com]
W dniu 23.07.2015 o 14:34, Du, Changbin pisze:
From 0a8e0d63a9887735c6782d7b0c15c2c1fdf1952a Mon Sep 17
00:00:00
void composite_disconnect(struct usb_gadget *gadget)
{
struct usb_composite_dev*cdev = get_gadget_data(gadget);
@@ -2095,7 +2119,7 @@ void composite_suspend(struct usb_gadget
*gadget)
cdev-suspended = 1;
- usb_gadget_vbus_draw(gadget, 2);
+ usb_gadget_vbus_draw(gadget, USB_VBUS_DRAW_SUSPEND);
}
This looks like an unrelated change. I think it should go first
in a separate patch which eliminates usage of magic numbers.
This change does make sense. As you know, when device is reset, it is in a
'unconfigured'
state. Compliance Test equipment may also measure vbus current at this
moment.
I am not questioning the change itself.
What I mean is that in my opinion it should be done in a separate patch,
because the newly introduced USB_VBUS_DRAW_SUSPEND is not used
anywhere else in your patch. The meaning of this change is use a symbolic
name rather than an explicit number and it is unrelated to
making composite gadget meet usb compliance for vbus draw. In other
words,
if you don't do this change at all the compliance is still maintained,
because the value of USB_VBUS_DRAW_SUSPEND is 2 anyway, so what the
compiler eventually sees is the same whether the change is made or not.
My idea:
[PATCHv2 0/2] usb gadget vbus draw compilance
[PATCHv2 1/2] usb: gadget: composite: avoid using a magic number
substituting an explicit 2 with USB_VBUS_DRAW_SUSPEND goes
here
[PATCHv2 2/2] usb: gadget: composite: meet usb compliance for vbus draw
the rest of your changes go here
}
@@ -2132,7 +2157,7 @@ static const struct usb_gadget_driver
composite_driver_template = {
.unbind = composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
A similar template is in drivers/usb/gadget/configfs.c. Shouldn't the
reset
method be changed there as well?
Yes, it also need to change. I will change it as well.
+/* USB2 compliance requires that un-configured current draw =
100mA,
+ * USB3 requires it = 150mA, OTG requires it = 2.5mA.
+ */
+#define USB2_VBUS_DRAW_UNCONF100
+#define USB3_VBUS_DRAW_UNCONF150
+#define USB_OTG_VBUS_DRAW_UNCONF 2
+#define USB_VBUS_DRAW_SUSPEND2
separate patch
Sorry, I didn't get your idea. Why separate these macros definition?
Please see above.
Andrzej
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[RESEND PATCH v2] usb/gadget: make composite gadget meet usb compliance for vbus draw
From 08df419517694c4dd9ff328f5644b46a99c2999e Mon Sep 17 00:00:00 2001
From: Du, Changbin changbin...@intel.com
Date: Thu, 23 Jul 2015 20:08:04 +0800
Subject: [PATCH v2] usb/gadget: make composite gadget meet usb compliance for
vbus draw
USB-IF compliance requirement limits the vbus current according to
current state. USB2 Spec requires that un-configured current must
be = 100mA, for USB3 is 150mA, OTG is 2.5mA. So set corresponding
vbus draw current according to device mode.
Signed-off-by: Du, Changbin changbin...@intel.com
---
Resend patch since no response for 4 weeks, probably bee missed.
Changes from v1:
Also update configfs to use new api composite_reset.
---
drivers/usb/gadget/composite.c | 39 ---
drivers/usb/gadget/configfs.c | 2 +-
include/linux/usb/composite.h | 1 +
include/uapi/linux/usb/ch9.h | 9 +
4 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 4e3447b..b3896e9 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -610,6 +610,21 @@ static void device_qual(struct usb_composite_dev *cdev)
qual-bRESERVED = 0;
}
+static unsigned unconfigured_vbus_draw(struct usb_composite_dev *cdev)
+{
+ struct usb_gadget *g = cdev-gadget;
+ unsigned power;
+
+ if (gadget_is_otg(g))
+ power = USB_OTG_UNCONF_STATE_VBUS_MAX_DRAW;
+ else if (g-speed == USB_SPEED_SUPER)
+ power = USB3_UNCONF_STATE_VBUS_MAX_DRAW;
+ else
+ power = USB2_UNCONF_STATE_VBUS_MAX_DRAW;
+
+ return power;
+}
+
/*-*/
static void reset_config(struct usb_composite_dev *cdev)
@@ -634,7 +649,7 @@ static int set_config(struct usb_composite_dev *cdev,
struct usb_gadget *gadget = cdev-gadget;
struct usb_configuration *c = NULL;
int result = -EINVAL;
- unsignedpower = gadget_is_otg(gadget) ? 8 : 100;
+ unsignedpower = unconfigured_vbus_draw(cdev);
int tmp;
if (number) {
@@ -1829,6 +1844,15 @@ done:
return value;
}
+void composite_reset(struct usb_gadget *gadget)
+{
+ struct usb_composite_dev *cdev = get_gadget_data(gadget);
+
+ DBG(cdev, reset\n);
+ usb_gadget_vbus_draw(gadget, unconfigured_vbus_draw(cdev));
+ composite_disconnect(gadget);
+}
+
void composite_disconnect(struct usb_gadget *gadget)
{
struct usb_composite_dev*cdev = get_gadget_data(gadget);
@@ -2095,7 +2119,7 @@ void composite_suspend(struct usb_gadget *gadget)
cdev-suspended = 1;
- usb_gadget_vbus_draw(gadget, 2);
+ usb_gadget_vbus_draw(gadget, USB_SUSPEND_STATE_VBUS_MAX_DRAW);
}
void composite_resume(struct usb_gadget *gadget)
@@ -2117,10 +2141,11 @@ void composite_resume(struct usb_gadget *gadget)
}
maxpower = cdev-config-MaxPower;
-
- usb_gadget_vbus_draw(gadget, maxpower ?
- maxpower : CONFIG_USB_GADGET_VBUS_DRAW);
- }
+ if (!maxpower)
+ maxpower = CONFIG_USB_GADGET_VBUS_DRAW;
+ } else
+ maxpower = unconfigured_vbus_draw(cdev);
+ usb_gadget_vbus_draw(gadget, maxpower);
cdev-suspended = 0;
}
@@ -2132,7 +2157,7 @@ static const struct usb_gadget_driver
composite_driver_template = {
.unbind = composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
.suspend= composite_suspend,
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 0495c94..e16335d 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1449,7 +1449,7 @@ static const struct usb_gadget_driver
configfs_driver_template = {
.unbind = configfs_composite_unbind,
.setup = composite_setup,
- .reset = composite_disconnect,
+ .reset = composite_reset,
.disconnect = composite_disconnect,
.suspend= composite_suspend,
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 2511469..825ad39 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -506,6 +506,7 @@ extern struct usb_string *usb_gstrings_attach(struct
usb_composite_dev *cdev,
extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
+extern void composite_reset(struct usb_gadget *gadget);
extern void composite_disconnect(struct usb_gadget *gadget);
extern int composite_setup(struct usb_gadget *gadget,
const struct usb_ctrlrequest *ctrl);
diff --git a/include/uapi
RE: [RESEND PATCH v2] usb/gadget: make composite gadget meet usb compliance for vbus draw
I am sorry, I forget to remove them. It is generated by git and I send the patch with outlook. I will use git-send-email instead next time if my email account work. Regards Du, Changbin > -Original Message- > From: 'Greg Kroah-Hartman' [mailto:gre...@linuxfoundation.org] > Sent: Monday, August 31, 2015 10:01 PM > To: Du, Changbin > Cc: 'Felipe Balbi'; 'Andrzej Pietrasiewicz'; 'linux-...@vger.kernel.org'; > 'linux- > ker...@vger.kernel.org' > Subject: Re: [RESEND PATCH v2] usb/gadget: make composite gadget meet > usb compliance for vbus draw > > On Mon, Aug 31, 2015 at 04:51:22AM +, Du, Changbin wrote: > > >From 08df419517694c4dd9ff328f5644b46a99c2999e Mon Sep 17 00:00:00 > 2001 > > From: "Du, Changbin" <changbin...@intel.com> > > Date: Thu, 23 Jul 2015 20:08:04 +0800 > > Subject: [PATCH v2] usb/gadget: make composite gadget meet usb > compliance for > > vbus draw > > What is this mess here for? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/2] Two fix for dwc2 gadget driver
> On 11/29/2015 9:29 PM, changbin...@intel.com wrote: > > From: "Du, Changbin" <changbin...@intel.com> > > > > With the first patch, enable a enabled ep will return -EBUSY. > > The second patch forbid queuing on disabled ep to avoid panic. > > > The usb_ep->enabled flag was added in 4.4. > > It looks like these same checks are also added at the API level in the > usb_ep_enable() and usb_ep_disable(). > > In case this is bypassed we should probably add them in the gadget > anyways but using the existing flag. > > Regards, > John > Hmm, just learnt the flag on gadget API layer. And I just see usb_ep_enable return success if it is already enabled. But I think it should return an error to inform the caller. Because the ep configuration may probably be changed. In this case, usb_ep_enable will do different behavior. Hmm, the usb_ep_queue doesn't check the enabled flag. Should be added. Let me have a try. Best Regards, Changbin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep
> >> 2.5.0 > > > > With this patch, ep0 transfer breaks. it because the 'enabled' of ep0 > > is not set. Ep0 is not enabled by usb_ep_enable, but in UDC driver. So > > there need another patch to set ep0's flag also. > > yeah, we don't like regressions :-) So the fix should come before > $subject to avoid a regression. > > -- > balbi It is hard to determine if ep0 is enabled or not in gadget API layer. Because it is controlled by udc driver, it may enable it at pullup, vbussession... But here, we can ignore for control-ep, considering it always enabled during usb session. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] usb: gadget: forbid queuing request to a disabled ep
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3d583a1..b566a4b 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct usb_ep
> *ep,
> static inline int usb_ep_queue(struct usb_ep *ep,
> struct usb_request *req, gfp_t gfp_flags)
> {
> + if (WARN_ON_ONCE(!ep->enabled))
> + return -ESHUTDOWN;
> +
> return ep->ops->queue(ep, req, gfp_flags);
> }
>
> --
> 2.5.0
With this patch, ep0 transfer breaks. it because the 'enabled' of ep0 is not
set. Ep0
is not enabled by usb_ep_enable, but in UDC driver. So there need another patch
to set ep0's flag also.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [lkp] [usb] 22bf2bb019: WARNING: CPU: 0 PID: 0 at include/linux/usb/gadget.h:405 composite_ep0_queue+0xad/0xd0()
> FYI, we noticed the below changes on
>
> https://github.com/0day-ci/linux changbin-du-intel-com/usb-gadget-forbid-
> queuing-request-to-a-disabled-ep/20151214-115939
> commit 22bf2bb019c7f92cda32c46b95715b0b208052d0 ("usb: gadget: forbid
> queuing request to a disabled ep")
>
>
> [8.157994] dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
> [8.209686] usb 1-1: new high-speed USB device number 2 using
> dummy_hcd
> [8.211689] [ cut here ]
> [8.212267] WARNING: CPU: 0 PID: 0 at include/linux/usb/gadget.h:405
> composite_ep0_queue+0xad/0xd0()
> [8.213684] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.0-rc4-1-
> g22bf2bb #1
> [8.214580] 54407de4 4125cd9f 54407e14
> 4109e21e 420f34b4
> [8.215626] 421bdc84 0195 41761a1d 41761a1d
> 4d810ea8 4d810ea8
> [8.216679] ff94 54407e24 4109e2eb 0009 54407e48
> 41761a1d 0001
> [8.217726] Call Trace:
> [8.218029] [<4125cd9f>] dump_stack+0x48/0x69
> [8.218570] [<4109e21e>] warn_slowpath_common+0x7e/0xb0
> [8.219204] [<41761a1d>] ? composite_ep0_queue+0xad/0xd0
> [8.219972] [<41761a1d>] ? composite_ep0_queue+0xad/0xd0
> [8.220741] [<4109e2eb>] warn_slowpath_null+0x1b/0x20
> [8.221362] [<41761a1d>] composite_ep0_queue+0xad/0xd0
> [8.222105] [<41762055>] composite_setup+0x615/0x19e0
> [8.222723] [<41769eba>] ? dummy_timer+0x4a/0xe30
> [8.223299] [<4176a86d>] dummy_timer+0x9fd/0xe30
> [8.223872] [<41769e70>] ? dummy_start+0x140/0x140
> [8.224461] [<410f2cef>] call_timer_fn+0x6f/0xf0
> [8.225025] [<410f2cb2>] ? call_timer_fn+0x32/0xf0
> [8.225612] [<410f2eb1>] run_timer_softirq+0x141/0x1d0
> [8.226234] [<41769e70>] ? dummy_start+0x140/0x140
> [8.226827] [<410a0d8d>] __do_softirq+0xbd/0x1c0
> [8.227397] [<410a0cd0>] ? _local_bh_enable+0x50/0x50
> [8.228013] [<4104f376>] do_softirq_own_stack+0x26/0x40
> [8.228651][<410a1085>] irq_exit+0x65/0x70
> [8.229258] [<4107ea7e>] smp_trace_apic_timer_interrupt+0x5e/0x90
> [8.229993] [<4107eab8>] smp_apic_timer_interrupt+0x8/0x10
> [8.230666] [<41d78a5d>] apic_timer_interrupt+0x2d/0x34
> [8.231309] [<410861f5>] ? native_safe_halt+0x5/0x10
> [8.231917] [<41055198>] default_idle+0x8/0x10
> [8.232467] [<41055919>] arch_cpu_idle+0x9/0x10
> [8.233016] [<410d55d3>] default_idle_call+0x23/0x30
> [8.233627] [<410d573c>] cpu_startup_entry+0x15c/0x240
> [8.234260] [<41d6f31a>] rest_init+0xaa/0xb0
> [8.234782] [<42327b05>] start_kernel+0x3df/0x3e4
> [8.235363] [<423272c3>] i386_start_kernel+0x91/0x95
> [8.235971] ---[ end trace a9973fc5ac55cb31 ]---
> [8.236529] g_ether gadget: ep_queue --> -108
>
>
> Thanks,
> Ying Huang
Hmm, it because the ep0 'enabled' is not set. Ep0 is not enabled by
usb_ep_enable, but in UDC driver.
It need a fix.
This is an interesting robot testing tool. Thanks!
Du, Changbin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] usb: gadget: make usb_ep_enable return -EBUSY if ep has already enabled
> > When usb_ep_enable on a enabled ep, the configuration of the ep > probably > > has changed. In this scenario, the ep configuration in hw should be > > reprogrammed by udc driver. Hence, it is better to return an error to > > inform the caller. > > > > Signed-off-by: Du, Changbin <changbin...@intel.com> > > --- > > include/linux/usb/gadget.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > > index d813bd2..89f9fdd 100644 > > --- a/include/linux/usb/gadget.h > > +++ b/include/linux/usb/gadget.h > > @@ -268,7 +268,7 @@ static inline int usb_ep_enable(struct usb_ep *ep) > > int ret; > > > > if (ep->enabled) > > - return 0; > > + return -EBUSY; > > While at that, can you add a WARN_ON() as well ? > > if (WARN_ON(ep->enabled)) > return -EBUSY; > > -- > balbi I will update the patch, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2] usb: dwc2: add ep enabled flag to avoid double enable/disable
Hi, Balbi,
Please ignore this patch set. Because we have a fix in gadget API layer.
[PATCH] usb: gadget: forbid queuing request to a disabled ep
> > Enabling an already enabled ep is illegal, because the ep may has trbs
> > running. Reprogram the ep may break running transfer. So udc driver
> > must avoid this happening by return an error -EBUSY. Gadget function
> > driver also should avoid such things, but that is out of udc driver.
> >
> > Similarly, disable a disabled ep makes no sense, but no need return
> > an error here.
> >
> > Signed-off-by: Du, Changbin <changbin...@intel.com>
> > ---
> > drivers/usb/dwc2/core.h | 1 +
> > drivers/usb/dwc2/gadget.c | 20 +++-
> > 2 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> > index a66d3cb..cf7eccd 100644
> > --- a/drivers/usb/dwc2/core.h
> > +++ b/drivers/usb/dwc2/core.h
> > @@ -162,6 +162,7 @@ struct dwc2_hsotg_ep {
> > unsigned char mc;
> > unsigned char interval;
> >
> > + unsigned intenabled:1;
> > unsigned inthalted:1;
> > unsigned intperiodic:1;
> > unsigned intisochronous:1;
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 0abf73c..586bbcd 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -2423,6 +2423,7 @@ void dwc2_hsotg_core_init_disconnected(struct
> dwc2_hsotg *hsotg,
> > /* enable, but don't activate EP0in */
> > dwc2_writel(dwc2_hsotg_ep0_mps(hsotg->eps_out[0]-
> >ep.maxpacket) |
> >DXEPCTL_USBACTEP, hsotg->regs + DIEPCTL0);
> > + hsotg->eps_out[0]->enabled = 1;
> >
> > dwc2_hsotg_enqueue_setup(hsotg);
> >
> > @@ -2680,6 +2681,14 @@ static int dwc2_hsotg_ep_enable(struct usb_ep
> *ep,
> > return -EINVAL;
> > }
> >
> > + spin_lock_irqsave(>lock, flags);
> > + if (hs_ep->enabled) {
> > + dev_warn(hsotg->dev, "%s: ep %s already enabled\n",
> > + __func__, hs_ep->name);
>
> this is a rather serious condition. I'd rather use dev_WARN_ONCE():
>
> if (dev_WARN_ONCE(hsotg->dev, hs_ep->enabled,
> "ep %s already enabled\n", hs_ep->name)) {
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] usb: gadget: forbid queuing request to a disabled ep
> >
> > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> > index 3d583a1..0c5d9ea 100644
> > --- a/include/linux/usb/gadget.h
> > +++ b/include/linux/usb/gadget.h
> > @@ -402,6 +402,9 @@ static inline void usb_ep_free_request(struct
> usb_ep *ep,
> > static inline int usb_ep_queue(struct usb_ep *ep,
> >struct usb_request *req, gfp_t gfp_flags)
> > {
> > + if (WARN_ON_ONCE(!ep->enabled && !ep->address))
>
> this will only trigger for a disabled ep0. Are you testing any of your
> patches at all ?
>
> --
> balbi
Oops, I sent a wrong patch. I will send right patch again as v4, very sorry for
this.
The right patch has been verified on 3.14 by back-porting related 1 patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete
> > To avoid this, just dequeue the request first. After usb_ep_dequeue, the > > request must be done or canceled. > > > > With this change, we can ensure no race condition in f_fs driver. But > > actually I found some of the udc driver has analogical issue in its > > dequeue implementation. For example, > > 1) the dequeue function hold the controller's lock. > > 2) before driver request controller to stop transfer, a request > >completed. > > 3) the controller trigger a interrupt, but its irq handler need wait > >dequeue function to release the lock. > > 4) dequeue function give back the request with negative status, and > >release lock. > > 5) irq handler get lock but the request has already been given back. > > > > get unlock? > > During the interrupt handler, it should only handle the "data complete" > interrupt on queued request; if the "data complete" interrupt occurs, but > it belongs to nobody, it will handle noop. > > > Best Regards, > Peter Chen You are right, but the problem is the request->status is wrong. If the data send out but report caller as -EINTR, it will introduce duplicate-send issue. Regards, Du, Changbin -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] usb: f_fs: avoid race condition with ffs_epfile_io_complete
> > > > You are right, but the problem is the request->status is wrong. If the data > > send out but report caller as -EINTR, it will introduce duplicate-send > > issue. > > > > Why -EINTR, the kernel-doc said it should return -ECONNRESET for active > request, see include/linux/usb/gadget.h. > > -- > > Best Regards, > Peter Chen F_fs return -EINTER in its dequeuer case, not udc driver. What I want to say is driver should return the right status for each usb request. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] usb: gadget: acm: set notify_req to NULL after freed to avoid double free
> On 12/26/2015 04:57 AM, changbin...@intel.com wrote: > > From: "Du, Changbin" <changbin...@intel.com> > > > > If acm_bind fails before allocate notification and acm->notify_req is > > not set to NULL after freed last time, double free will happen. > > Looks good to me. > > Similar problem can occur with another USB functions (at least f_ecm, > f_ncm, f_rndis and f_hid handle USB requests in analogical way). Maybe > it's worth to fix them all at once? > > > > > kernel BUG at mm/slub.c:3392! > > invalid opcode: [#1] PREEMPT SMP > > EIP is at kfree+0x172/0x180 > > Call Trace: > > [<80c0e3b6>] ? usb_ep_autoconfig_ss+0x86/0x170 > > [<80c13345>] gs_free_req+0x15/0x30 > > [<80c12df1>] acm_bind+0x1c1/0x2d0 > > [<80c0e9be>] usb_add_function+0x6e/0x120 > > [<80c213cb>] acm_function_bind_config+0x2b/0x90 > > > > Reviewed-by: Robert Baldyga <r.bald...@hackerion.com> > Hmm, you are right. I checked all the fucntions, found these need to be fixed: f_ecm.c f_hid.c f_ncm.c f_phonet.c f_rndis.c f_uvc.c I will update patche to fix them all. thank you. Regards, Du, Changbin
RE: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC
Thanks, Machek, This patch has already been dropped. > On Tue 2016-05-03 11:04:24, changbin...@intel.com wrote: > > From: "Du, Changbin" <changbin...@gmail.com> > > > > On most platforms, there is only one device controller available. > > In this case, we desn't care the UDC's name. So let's ignore the > > name by setting 'UDC' to 'any'. And also we can change UDC name > > at any time if it is not binded (no need set to "" first). > > making "any" special does not look like a good idea. What if it really > is "any"? > > Return nothing instead, not even \n? > > > Signed-off-by: Du, Changbin <changbin...@gmail.com> > > Signed-off-by: Du, Changbin <changbin...@intel.com> > > I don't think this is how you should sign it off. > > Best regards, > > Pavel > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) > http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
RE: [PATCH] usb: core: add debugobjects support for urb object
> On Tue, May 24, 2016 at 03:53:53PM +0800, changbin...@intel.com wrote: > > From: "Du, Changbin" <changbin...@intel.com> > > > > Add debugobject support to track the life time of struct urb. > > This feature help us detect violation of urb operations by > > generating a warning message from debugobject core. And we fix > > the possible issues at runtime to avoid oops if we can. > > > > I have done some tests with some class drivers, no violation > > found in them which is good. Expect this feature can be used > > for debugging future problems. > > > > Signed-off-by: Du, Changbin <changbin...@intel.com> > > I agree with Alan, what use is this code? Existing drivers all work > properly because the reference counting of urbs is already present, why > add duplicate counters? That actually leads to bugs. I don't see the > need for this code, please explain it better if you wish for it to be > accepted. What has it found/fixed that we have not found yet? What > does it allow you to do that you can't do today with the existing code? > > thanks, > > greg k-h > I agree with you two now after checking all urb code. I am sorry to disturb you. I did have an object lifetime issue, but it is of usb_request. I expect the cause is usb_request is freed but still pending in udc core. Then I cannot reproduce it, I even cannot know which function driver the usb_request came from. I originally want to use debugojects to debug that issue, then I though this can also help urb debugging. Obviously I am wrong. As you said reference counting of urbs is already present. :) I may add debugojects for device mode usb_request farther. Sometimes it helps. Thank you, Du, Changbin
RE: [PATCH] usb: dwc3: add debugfs node to dump FIFO/Queue available space
> > This can be used to check some special issues, like whether data is
> > successfully copied from memory to fifo when a trb is blocked.
> >
> > Signed-off-by: Du, Changbin <changbin...@intel.com>
> > ---
> > drivers/usb/dwc3/core.h| 5 +
> > drivers/usb/dwc3/debugfs.c | 45
> +
> > 2 files changed, 50 insertions(+)
>
> Why did you not include the linux-usb@vger mailing list?
>
Just forget it :)
> > static int dwc3_testmode_show(struct seq_file *s, void *unused)
> > {
> > struct dwc3 *dwc = s->private;
> > @@ -648,6 +687,12 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
> > goto err1;
> > }
> >
> > + file = debugfs_create_file("fifo", S_IRUGO, root, dwc,
> _fifo_fops);
> > + if (!file) {
> > + ret = -ENOMEM;
>
> Um, no, that's not the error here. You shouldn't care at all about
> debugfs api call results. Just keep moving on here please.
>
> thanks,
>
> greg k-h
Agree with you. I will create another patch to cleanup this piece of code.
And I found a memory leak issue there, dwc->regset never released. Will also
fix it.
Thanks,
Du, Changbin
RE: [PATCH v2 0/3] Improvement, fix and new entry for dwc3 debugfs
> before I review your patches, one comment > > changbin...@intel.com writes: > > From: "Du, Changbin" <changbin...@intel.com> > > > > The first patch removed unnecessary checking for debugfs api call; > > The second patch fix a memory leak issue; > > The third patch add one new entry to debufs. > > > > Du, Changbin (3): > > usb: dwc3: make dwc3_debugfs_init return value be void > > this is _not_ a fix > > > usb: dwc3: free dwc->regset on dwc3_debugfs_exit > > but this is. Why isn't this, at least, the first patch in the list ? In > fact, it would be preferred that this patch be sent by itself and the > following two patches should be on another branch completely without any > dependencies to the memory leak fix. > > -- > Balbi Sure, Balbi. This will be better. I will send out patch v3 and another independent patch. Also include changelog as Greg required. Thanks for checking. Regards, Du, Changbin
RE: [PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void
> > root = debugfs_create_dir(dev_name(dwc->dev), NULL);
> > - if (!root) {
> > - ret = -ENOMEM;
> > - goto err0;
> > - }
> > + if (IS_ERR_OR_NULL(root))
> > + return;
>
> We can definitely keep on going, but I'd still like to know that we
> enabled CONFIG_DEBUG_FS but failed to create a file or a
> directory. Seems like this should read as follows:
>
> if (IS_ERR_OR_NULL(root)) {
> if (!root)
> dev_err(dwc->dev, "Can't create debugfs root\n");
> return;
> }
>
> ditto to all bellow.
>
Balbi, so you mean we should not let the failure go silent, right?
> > dwc->root = root;
> >
> > dwc->regset = kzalloc(sizeof(*dwc->regset), GFP_KERNEL);
> > if (!dwc->regset) {
> > - ret = -ENOMEM;
> > - goto err1;
> > + debugfs_remove_recursive(root);
>
> you're now duplicating debugfs_remove_recursive(root) in all braches and
> that's error prone. It's probably better to keep our gotos, but change
> them so they read as follows:
>
> if (!dwc->regset)
> goto err1;
>
> [...]
>
> return; /* this is our successful exit point */
>
> err1:
> debugfs_remove_recursive(root);
> kfree(dwc->regset);
>
>
> --
> Balbi
No, no need anymore. Because no branch share this code now.
Then remove the goto would make code a little clear.
Thanks,
Du, Changbin
RE: [PATCH] usb: dwc3: free dwc->regset on dwc3_debugfs_exit
Hi, Balbi, > > Hmm, I agree from this point. I will combine this patch with other two > patches > > (due to their dependency). And I'd like remove the 'dwc->root=NULL' as > well, > > you are creating a dependency that doesn't exist. Please stop that. You > should have two separate branches based on v4.6-rc3 (or, if you prefer, > one based on my testing/fixes and another based on my testing/next). On > one branch you have *only* $subject and you fix *all* the memory > leaks. On the other branch you have the other two patches. > > Ignore the fact that we might have a conflict, that's for git (and > maintainers) to handle when they happen. > > Again, don't create dependencies between fixes for the -rc cycle and > changes for the next merge window. > Thanks for dedicated explanation. I was concern about the conflict. Now it is very clear for me to handle such situation. > > Is it ok for you? > > yeah, please remove root = NULL as that's completely unnecessary, but > split these patches in separate branches and fix all memory leaks. > > -- > balbi
RE: [PATCH] usb: dwc3: free dwc->regset on dwc3_debugfs_exit
> Hi, > > "Du, Changbin" <changbin...@intel.com> writes: > >> > >> >> > + dwc->regset = NULL; > >> >> > >> >> setting regset to NULL is unnecessary. We only call > dwc3_debugfs_exit() > >> >> when removing the driver. > >> >> > >> >> -- > >> >> Balbi > >> > I'd like keep this line even it is unnecessary, because It is a good > >> > habit to > >> > Avoid wild pointers. Just like the dwc->root = NULL. > >> > >> there won't be any wild pointers here, we'll free struct dwc3 *dwc itself. > >> > >> -- > >> Balbi > > I agree the dwc will be freed in current code. But the 'free' logical is out > > of the debugfs code. They should be treat as some logical independent. > Per > > this point, I still think set pointer to null is not bad. For example, if > > dwc3 > core > > code invoke dwc3_debugfs_exit twice by mistake(just an example case, > not > > really), then no crash/impact for the second call. > > the second call should crash because it's clearly wrong ;-) If dwc3 ever > calls dwc3_debugfs_exit() twice, it really deserves to crash. It's > something so wrong that we want the verbosity and urgency of a kernel > oops to make sure we fix it ASAP. > > If, however, we set it to null, it might be years before we notice > anything's wrong. > > -- > Balbi Hmm, I agree from this point. I will combine this patch with other two patches (due to their dependency). And I'd like remove the 'dwc->root=NULL' as well, Is it ok for you? Thx, Du, Changbin
RE: [PATCH] usb: dwc3: free dwc->regset on dwc3_debugfs_exit
> > >> > +dwc->regset = NULL; > >> > >> setting regset to NULL is unnecessary. We only call dwc3_debugfs_exit() > >> when removing the driver. > >> > >> -- > >> Balbi > > I'd like keep this line even it is unnecessary, because It is a good habit > > to > > Avoid wild pointers. Just like the dwc->root = NULL. > > there won't be any wild pointers here, we'll free struct dwc3 *dwc itself. > > -- > Balbi I agree the dwc will be freed in current code. But the 'free' logical is out of the debugfs code. They should be treat as some logical independent. Per this point, I still think set pointer to null is not bad. For example, if dwc3 core code invoke dwc3_debugfs_exit twice by mistake(just an example case, not really), then no crash/impact for the second call. Thanks, Du, Changbin
RE: [PATCH] usb: dwc3: free dwc->regset on dwc3_debugfs_exit
> > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> > index 9ac37fe..9eeb444 100644
> > --- a/drivers/usb/dwc3/debugfs.c
> > +++ b/drivers/usb/dwc3/debugfs.c
> > @@ -687,4 +687,7 @@ void dwc3_debugfs_exit(struct dwc3 *dwc)
> > {
> > debugfs_remove_recursive(dwc->root);
> > dwc->root = NULL;
> > +
> > + kfree(dwc->regset);
>
> we also need a kfree() on dwc3_debugfs_init().
This patch is based on the patch set
[PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void>
So, they do has dependency. :)
> > + dwc->regset = NULL;
>
> setting regset to NULL is unnecessary. We only call dwc3_debugfs_exit()
> when removing the driver.
>
> --
> Balbi
I'd like keep this line even it is unnecessary, because It is a good habit to
Avoid wild pointers. Just like the dwc->root = NULL.
Thanks,
Du, Changbin
RE: [PATCH] usb: dwc3: rename __dwc3_cleanup_done_trbs to __dwc3_cleanup_one_trb
> > @@ -1873,7 +1873,7 @@ static void dwc3_gadget_free_endpoints(struct > dwc3 *dwc) > > > > /* > > -- > > */ > > > > -static int __dwc3_cleanup_done_trbs(struct dwc3 *dwc, struct dwc3_ep > *dep, > > +static int __dwc3_cleanup_one_trb(struct dwc3 *dwc, struct dwc3_ep > *dep, > > I would rather just remove that 's' from the end so the function's name > stays as __dwc3_cleanup_done_trb() ;-) > > Care to do that ? > > thanks > > -- > Balbi IMO, "one_trb" is a little more clear. But both is okay for me. :) Thanks, Du, Changbin
RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space
Hello, Sergei,
> > From: "Du, Changbin" <changbin...@intel.com>
> >
> > For DWC3 USB controller, the Global Debug Queue/FIFO Space Available
> > Register(GDBGFIFOSPACE) can be used to dump FIFO/Queue available
> space.
>
> Space needed before (.
Okay.
>
> > This can be used to check some special issues, like whether data is
> > successfully copied from memory to fifo when a trb is blocked.
> >
> > Signed-off-by: Du, Changbin <changbin...@intel.com>
> > ---
> > v4:
> >Do not fail silently, but print error.
> [...]
> > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> > index 615d4dc..83e5bc1 100644
> > --- a/drivers/usb/dwc3/debugfs.c
> > +++ b/drivers/usb/dwc3/debugfs.c
> [...]
> > @@ -642,10 +681,15 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
> > dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
> > dwc->regset->base = dwc->regs;
> >
> > +
>
> Why?
Thanks for point out, I will remove this additional line.
>
> > file = debugfs_create_regset32("regdump", S_IRUGO, root, dwc-
> >regset);
> > if (!file)
> > dev_err(dwc->dev, "Can't create debugfs regdump\n");
> >
> > + file = debugfs_create_file("fifo", S_IRUGO, root, dwc,
> _fifo_fops);
> > + if (!file)
> > + dev_err(dwc->dev, "Can't create debugfs fifo\n");
> > +
> > if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {
> > file = debugfs_create_file("mode", S_IRUGO | S_IWUSR,
> root,
> > dwc, _mode_fops);
>
> MBR, Sergei
RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space
Hi, Balbi.
Feel free to change it, I may not have enough time on this currently.
"per-endpoint directory" is great idea, then we do not need find out
wanted info from one big file, but just go to specific dir.
Btw, I'd mention that not all out ep has a rx fifo. So in my original patch,
not all FIFO/Queue info are valid. We need pick out the real info we need.
And I didn't find any method to read the FIFO map.
At last, comparing with the FIFO/Queue info, I think software transfer
Requests list, TRBs info, EVENTs history are much more useful for debugging
the driver. If you can also add these info to each EP folder, that is awesome!
:)
Best Regards,
Du, Changbin
> -Original Message-
> From: Felipe Balbi [mailto:ba...@kernel.org]
> Sent: Thursday, April 14, 2016 4:03 PM
> To: Du, Changbin <changbin...@intel.com>
> Cc: gre...@linuxfoundation.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Du, Changbin <changbin...@intel.com>
> Subject: Re: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump
> FIFO/Queue available space
>
>
> Hi,
>
> changbin...@intel.com writes:
> > From: "Du, Changbin" <changbin...@intel.com>
> >
> > For DWC3 USB controller, the Global Debug Queue/FIFO Space Available
> > Register(GDBGFIFOSPACE) can be used to dump FIFO/Queue available
> space.
> > This can be used to check some special issues, like whether data is
> > successfully copied from memory to fifo when a trb is blocked.
> >
> > Signed-off-by: Du, Changbin <changbin...@intel.com>
> > ---
> > v4:
> > Do not fail silently, but print error.
> >
> > ---
> > drivers/usb/dwc3/core.h| 5 +
> > drivers/usb/dwc3/debugfs.c | 44
>
> > 2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 6254b2f..899cf76 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -348,6 +348,11 @@
> > #define DWC3_DSTS_LOWSPEED (2 << 0)
> > #define DWC3_DSTS_FULLSPEED1 (3 << 0)
> >
> > +/* Global Debug Queue/FIFO Space Available Register */
> > +#define DWC3_GDBGFIFOSPACE_NUM(x) (((x) << 0) & 0x1F)
> > +#define DWC3_GDBGFIFOSPACE_TYPE(x) (((x) << 5) & 0xE0)
> > +#define DWC3_GDBGFIFOSPACE_GET_SPACE(x)(((x) >> 16) & 0x)
>
> we always use lower case hex numbers. Also, databook refers to top 16
> bits as "Space Avaiable" so I'd prefer that you called this macro:
>
> DWC3_GDBGFIFOSPACE_SPACE_AVAILABLE(x)
>
> as that will aid with grepping
>
> > diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> > index 615d4dc..83e5bc1 100644
> > --- a/drivers/usb/dwc3/debugfs.c
> > +++ b/drivers/usb/dwc3/debugfs.c
> > @@ -426,6 +426,45 @@ static const struct file_operations
> dwc3_mode_fops = {
> > .release= single_release,
> > };
> >
> > +static int dwc3_fifo_show(struct seq_file *s, void *unused)
>
> you call this file 'fifo' however you print more than just the
> fifos. You printk the request queues, info queue, descriptor fetch
> queue, event queue and protocol status queue.
>
> It seems, to me, you're trying to do way too much in a single file.
>
> > +{
> > + struct dwc3 *dwc = s->private;
> > + unsigned long flags;
> > + unsigned inttype, index;
> > + const char *name;
> > + u32 reg;
> > +
> > + static const char * const fifo_names[] = {
> > + "TxFIFO", "RxFIFO", "TxReqQ", "RxReqQ", "RxInfoQ",
> > + "DescFetchQ", "EventQ", "ProtocolStatusQ"};
> > + spin_lock_irqsave(>lock, flags);
> > + for (type = 0; type < 8; type++) {
>
> type < ARRAY_SIZE(fifo_names) ??
>
> > + name = fifo_names[type];
> > + for (index = 0; index < 32; index++) {
>
> not *all* dwc3 implementations enable all 32 endpoints, that's why we
> have dwc->num_endpoints
>
> > + dwc3_writel(dwc->regs, DWC3_GDBGFIFOSPACE,
> > + DWC3_GDBGFIFOSPACE_NUM(index) |
> > + DWC3_GDBGFIFOSPACE_TYPE(type));
> > + reg = dwc3_readl(dwc->regs,
> DWC3_GDBGFIFOSPACE);
>
> this writel() followed by a readl() could be a nice little helper
> function in core.c. Remember that we need the FIFO Space and link state
RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space
> Hi, > > "Du, Changbin" <changbin...@intel.com> writes: > >> > At last, comparing with the FIFO/Queue info, I think software transfer > >> > Requests list, TRBs info, EVENTs history are much more useful for > >> debugging > >> > the driver. If you can also add these info to each EP folder, that is > awesome! > >> > :) > >> > >> I'll think about adding these but for the lifetime of requests and trbs > >> and events, etc, we have tracepoints for that. I usually do the > >> following when debugging: > >> > >> # mount -t debugfs none /sys/kernel/debug > >> # cd /sys/kernel/debug/tracing > >> # echo 2048 > buffer_size_kb > >> # echo 1 > events/dwc3/enable > >> > >> (do something to break it) > >> > >> # cp trace /mnt/sdcard # or something like that > >> > >> then read the file. You can make it as large or as small as you like > >> (given some constraints, of course ;-) but I've had no issues allocating > >> 128MiB in the past. > >> > >> -- > >> Balbi > > > > Thanks for the sharing, this is a good approach to capture dynamic > > behaviors. But a dump of current state has below advantages: > > 1. a quick view for the pending transfers. Then we can quickly > > checking the transfer status. > > 2. no side-effect. This is important in some case. We usually > > encounter some transfer issues but very hard to reproduce > > it. But we cannot enable trace all the time since performance > > concern. Then I thought it was so great if I can have a look for > > the trb status. :) > > yeah, okay. We can definitely add "current state" of almost anything, > but if you need history, then debugfs is not the best interface and I'd > point you to tracepoints ;-) > > I'll think about how I can add TRB state, seems like we'd need to dump > the entire endpoint ring, and that's 256 TRBs per endpoint :-p Then we > also need to know endpoint's dequeue and enqueue pointer. Oh well, let > me get this first setup of files out of the way, then we can add more > later much more easily. > > -- > Balbi Okay, things need finish step by step. Thank you, Balbi. ( �b- �b)つロ Best Regards, Du, Changbin
RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space
> > At last, comparing with the FIFO/Queue info, I think software transfer > > Requests list, TRBs info, EVENTs history are much more useful for > debugging > > the driver. If you can also add these info to each EP folder, that is > > awesome! > > :) > > I'll think about adding these but for the lifetime of requests and trbs > and events, etc, we have tracepoints for that. I usually do the > following when debugging: > > # mount -t debugfs none /sys/kernel/debug > # cd /sys/kernel/debug/tracing > # echo 2048 > buffer_size_kb > # echo 1 > events/dwc3/enable > > (do something to break it) > > # cp trace /mnt/sdcard # or something like that > > then read the file. You can make it as large or as small as you like > (given some constraints, of course ;-) but I've had no issues allocating > 128MiB in the past. > > -- > Balbi Thanks for the sharing, this is a good approach to capture dynamic behaviors. But a dump of current state has below advantages: 1. a quick view for the pending transfers. Then we can quickly checking the transfer status. 2. no side-effect. This is important in some case. We usually encounter some transfer issues but very hard to reproduce it. But we cannot enable trace all the time since performance concern. Then I thought it was so great if I can have a look for the trb status. :) Best Regards, Du, Changbin
RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int
Hi, > On Fri, 22 Apr 2016, changbin...@intel.com wrote: > > From: "Du, Changbin" <changbin...@intel.com> > > > > The object debugging infrastructure core provides some fixup callbacks > > for the subsystem who use it. These callbacks are called from the debug > > code whenever a problem in debug_object_init is detected. And > > debugobjects core suppose them returns 1 when the fixup was successful, > > otherwise 0. So the return type is boolean. > > > > A bad thing is that debug_object_fixup use the return value for > > arithmetic operation. It confused me that what is the reall return > > What's bad about that? The fact that it's used for arithmethic operation or > that it confused you? > It confused me because this is not a common usage. I was confused that what does he fixup function return? A countable value? But doc says return fixed or not! if (fixup) fixed = fixup(addr, state); debug_objects_fixups += fixed; In common,for int return 0 indicates success, negative for fail, positive for something countable. So I think it is better follow this rule. Here is not of countable, it is Boolean. So why not this? if (fixup && fixup(addr, state)) debug_objects_fixups++; > > Reading over the whole code, I found some place do use the return value > > incorrectly(see next patch). So why use bool type instead? > > Patches which fix a problem need to come first not in the middle of a revamp > series. > Thanks, I am first know of this. > > + bool (*fixup_init)(void *addr, enum debug_obj_state state); > > + bool (*fixup_activate)(void *addr, enum debug_obj_state state); > > + bool (*fixup_destroy)(void *addr, enum debug_obj_state state); > > + bool (*fixup_free)(void *addr, enum debug_obj_state state); > > + bool (*fixup_assert_init)(void *addr, enum debug_obj_state state); > > So this change will introduce a gazillion of compile warnings because the > callbacks in the various usage sites are still having 'int' return type. > No, I modified all the code who use debugojects API. > Thanks, > > tglx Thanks, Du, Changbin
RE: [PATCH] usb: hub: fix panic caused by NULL bos pointer during reset device
> On Tue, Mar 08, 2016 at 05:15:17PM +0800, changbin...@intel.com wrote:
> > From: "Du, Changbin" <changbin...@intel.com>
> >
> > This is a reworked patch based on reverted commit d8f00cd685f5 ("usb:
> > hub: do not clear BOS field during reset device").
> >
> > The privious one caused double mem-free if run to re_enumerate label.
> > New patch title changed to distinguish from old one. And I have tested
> > it with memory debugging options.
> >
> > In function usb_reset_and_verify_device, the old BOS descriptor may
> > still be used before allocating a new one. (usb_disable_lpm function
> > uses it under the situation that it fails at usb_disable_link_state.)
> > So we cannot set the udev->bos to NULL before that, just keep what it
> > was. It will be overwrite when allocating a new one.
> >
> > How to reproduce:
> > 1. connect one usb3 hub to xhci port.
> > 2. connect several lpm-capable super-speed usb disk to the hub.
> > 3. copy big files to the usb disks.
> > 4. disconnect the hub and repeat step 1-4.
> >
> > Crash log:
> > BUG: unable to handle kernel NULL pointer dereference at
> > 0010
> > IP: [] usb_enable_link_state+0x2d/0x2f0
> > Call Trace:
> > [] ? usb_set_lpm_timeout+0x12b/0x140
> > [] usb_enable_lpm+0x81/0xa0
> > [] usb_disable_lpm+0xa8/0xc0
> > [] usb_unlocked_disable_lpm+0x2c/0x50
> > [] usb_reset_and_verify_device+0xc3/0x710
> > [] ? usb_sg_wait+0x13d/0x190
> > [] usb_reset_device+0x133/0x280
> > [] usb_stor_port_reset+0x61/0x70
> > [] usb_stor_invoke_transport+0x88/0x520
> >
> > Signed-off-by: Du, Changbin <changbin...@intel.com>
> > ---
> > drivers/usb/core/hub.c | 14 +-
> > 1 file changed, 9 insertions(+), 5 deletions(-)
>
> Is this patch still needed? I thought we had some other fix in this
> area...
>
> confused,
>
> greg k-h
>
Hi, Greg k-h,
Sorry for it confused you. This patch still need. This is same fix with
previous commit d8f00cd685f5 ("usb: hub: do not clear BOS field
during reset device"). But d8f00cd685f5 is buggy and reverted. This
new patch should be the final fix.
Best Regards,
Du, Changbin
RE: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup
Hi, Balbi,
The step to reproduce this issue is:
1) connect device to a host and wait its enumeration.
2) trigger software disconnect by calling function
usb_gadget_disconnect(), which finally call
dwc3_gadget_pullup(false). Do not reconnect device
(I mean no enumeration go on, keep bit Run/Stop 0.).
At here, gadget driver's disconnect callback should be
Called, right? We has been disconnected. But no, as
You said " not generating disconnect IRQ after you
drop Run/Stop is expected".
And I am testing on an Android device, Android only
use dwc3_gadget_pullup(false) to issue a soft disconnection.
This confused user that the UI still show usb as connected
State, caused by missing a disconnect event.
> Hi,
>
> changbin...@intel.com writes:
> > From: "Du, Changbin" <changbin...@intel.com>
> >
> > The dwc3 controller can't generate a disconnect event after it is
> > stopped. Thus gadget dissconnect callback is not invoked when do
>
> not generating disconnect IRQ after you drop Run/Stop is expected.
>
> > soft dissconnect. Call dissconnect here to workaround this issue.
>
> I'm rather sure this is a bug elsewhere. How do you trigger this ?
>
> > Note, most time we still see disconnect be called that because
> > it is invoked by dwc3_gadget_reset_interrupt(). But if we
> > disconnect cable once pullup disabled quickly, issue can be
> > observed.
>
> I can't understand what you're trying to say with this.
>
> > Signed-off-by: Du, Changbin <changbin...@intel.com>
> > ---
> > drivers/usb/dwc3/gadget.c | 33 -
> > 1 file changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 8e4a1b1..cd73187 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1566,6 +1566,15 @@ static int dwc3_gadget_run_stop(struct dwc3
> *dwc, int is_on, int suspend)
> > return 0;
> > }
> >
> > +static void dwc3_disconnect_gadget(struct dwc3 *dwc)
> > +{
> > + if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> > + spin_unlock(>lock);
> > + dwc->gadget_driver->disconnect(>gadget);
> > + spin_lock(>lock);
> > + }
> > +}
> > +
> > static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> > {
> > struct dwc3 *dwc = gadget_to_dwc(g);
> > @@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget
> *g, int is_on)
> > is_on = !!is_on;
> >
> > spin_lock_irqsave(>lock, flags);
> > + /**
> > +* WORKAROUND: The dwc3 controller can't generate a disconnect
> > +* event after it is stopped. Thus gadget dissconnect callback
> > +* is not invoked when do soft dissconnect. Call dissconnect
> > +* here to workaround this issue.
> > +* Note, most time we still see disconnect be called that because
> > +* it is invoked by dwc3_gadget_reset_interrupt(). But if we
> > +* disconnect cable once pullup disabled quickly, issue can be
> > +* observed.
> > +*/
> > + if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
> > + dev_dbg(dwc->dev, "fake dissconnect event on pullup
> off\n");
> > + dwc3_disconnect_gadget(dwc);
> > + dwc->gadget.speed = USB_SPEED_UNKNOWN;
> > + }
>
> this is *really* wrong. You shouldn't be calling pullup directly. This
> patch looks wrong and you didn't even explain how to trigger this
> problem; when does the problem happen ?
>
> Gadget load/unload does the right thing here, so that can't be the
> case. We also do the right thing on soft_connect sysfs file:
>
> static ssize_t usb_udc_softconn_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t n)
> {
> struct usb_udc *udc = container_of(dev, struct usb_udc,
> dev);
>
> if (!udc->driver) {
> dev_err(dev, "soft-connect without a gadget driver\n");
> return -EOPNOTSUPP;
> }
>
> if (sysfs_streq(buf, "connect")) {
> usb_gadget_udc_start(udc);
> usb_gadget_connect(udc->gadget);
> } else if (sysfs_streq(buf, "disconnect")) {
> usb_gadget_disconnect(udc->gadget);
> udc->driver->disconnect(udc->gadget);
> usb_gadget_udc_stop(udc);
> } else {
> dev_err(dev, "unsupported command '%s'\n", buf);
> return -EINVAL;
> }
>
> return n;
> }
> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,
> usb_udc_softconn_store);
>
> So really, what _is_ the problem ?
>
> --
> balbi
RE: [PATCH] checkpatch: Add support to check already applied git commits
Hi, > From: Joe Perches [mailto:j...@perches.com] > Sent: Monday, April 25, 2016 7:12 AM > To: Andrew Morton <a...@linux-foundation.org>; Andy Whitcroft > <a...@canonical.com> > Cc: Du, Changbin <changbin...@intel.com>; linux-kernel@vger.kernel.org > Subject: [PATCH] checkpatch: Add support to check already applied git > commits > > It's sometimes useful to scan already committed patches. > > Add --git to scan specific or multiple commits. > > Single commits are scanned with > --git > Multiple commits are scanned with > --git > --git - > > Signed-off-by: "Du, Changbin" <changbin...@intel.com> > Signed-off-by: Joe Perches <j...@perches.com> > --- > > A few miscellaneous improvements to Changbin's original patch: > > o Don't exec git for each -, > use a single "git log - " > o Consolidate the git exec for the and - variants > o Output 12 character commit hash ids > o Don't scan git commit merges > o Use -M to reduce the size of rename commits > Thanks, it has been much more better now. I like this new one. This is my first time write a Perl script. :) Also thanks for Sebastian's explanation, I got this idea just because I had the same use case with you. Best Regards, Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
Hi, > > I'd prefer fail the request at all, and it is better done in HW. > > Because per the USB Spec that device can return NAK if a function was > > unable to accept data From the host. The DWC3 has not been design as > > this, if software fail the transfer, it is a little weird for host. > > > > So, now we have 3 choices: > > 1) buffer the excess data > > 2) fail the transfer > > You mean fail when more data has been sent (i.e. drop the whole packet) > or fail at entry to read() if the buffer is not aligned? > I mean the first one. > > 3) drop the excess data, then print an warning message > > > > Which one do you prefer? > > I think f_fs should mimic whatever happens if unaligned request is > queued on dwc3. As far as I understand, this is not 1. > > I’ll be travelling again on Friday so I’ll finish up the patch doing 1 > so we will have a choice between 1 (my patch) and 3 (your patch). > Great! Prefer your patch if #1 works good. > -- > Best regards > ミハウ “퓶퓲퓷퓪86” ナザレヴイツ > «If at first you don’t succeed, give up skydiving» Best Regards, Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> On Wed, May 18 2016, Felipe Balbi wrote: > > we've been through this before. This needs to be done at the gadget > > layer. Gadget driver can over-allocate ahead of time if > > gadget->quirk_ep_out_aligned_size is true, then we avoid memcpy() at > > the UDC driver level. > > Right, all right, so let’s look at it from a regular USB function point > of view. If a USB function allocates a request which is not aligned, > UDC will align the buffer and *drop* excess data. Seeing how ugly > Do you mean UDC driver align the buffer? I searched the code, currently only DWC3 needs buffer size to be aligned to MaxPacketSize on ep out. And the align is done in f_fs driver. > f_fs’s code is becoming, I’m now leaning to letting to f_fs do the same > thing: if user space makes an unaligned read, f_fs aligns the buffer and > then drops excess data. > > Any arguments for f_fs to not drop the data apply to UDC, so they should > behave identically. > I'd prefer fail the request at all, and it is better done in HW. Because per the USB Spec that device can return NAK if a function was unable to accept data From the host. the DWC3 has not been design as this, if software fail the transfer, it is a little weird for host. So, now we have 3 choices: 1) buffer the excess data 2) fail the transfer 3) drop the excess data, then print an warning message Which one do you prefer? > -- > Best regards > ミハウ “퓶퓲퓷퓪86” ナザレヴイツ > «If at first you don’t succeed, give up skydiving» Best Regards, Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> >> thanks Alan Stern and Michal. > >> Here just have a comment - the buffered data need be dropped when > the > >> epfile is closed, because it means the session is terminated. > > > > I blame that on sleep deprivation. Another issue is what to do when > > endpoint is disabled. Should the buffer be cleared as soon as the > > endpoint is disabled? Or maybe when the endpoint is enabled again? Or > > maybe it should never be cleared? > > > > If the buffer is cleared when endpoint is disabled, we again silently > > drop data. On the other hand, if we don’t do that, read() on the > > endpoint will may succeed even if the configuration is disabled which > > may be surprising for users. > > tough decision... but seems like clearing the buffer as soon as ep is > disabled is the way to go. > > -- > Balbi I agree with Balbi, seems it is not easy to maintain the excess buffer... I was to implement it at the beginning but I am not confident everything is done correctly.
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> > The extra bytes can be anything(random), they just data from APP layer. > > It doesn't make sense for you to check. So I will not dump them, sorry. > > interesting, so you claim to have found a bug, but when asked to provide > more information your answer is "no" ? Thanks :-) > Do you really want a random hex string? I don't think it is useful. > >> > The problem is device side app sometimes received incorrect data > caused > >> > by the dropping. Most times the error can be detected by APP itself, but > >> > >> why ? app did e.g. read(5), that caused driver to queue a usb_request > >> with length set to 512. Host sent more data than the expected 5 bytes, > >> why did host do that ? And if that data was needed, why didn't userspace > >> read() more than 5 ? > >> > > > > Well, first, there must be a protocol upon usb between host side and > > device side. > > sorry, I don't know what mean here. USB does not *require* a protocol > running on top of USB. There usually is one, but that's not a > requirement. > Communication between two endpoints must has a protocol, even it may very simple. Without protocol, they cannot exchange information. > > Second device side didn't know how many bytes to receive, it need host > > side tell it. > > well, many protocols work like this. See Mass Storage, for example. > > > But host could be buggy, > > if host is buggy, why should we fix host on the peripheral side ? > True it is bug of host, but is it a reason kernel can drop data then? > > or the application is killed and restart. > > If application is killed (why was the application killed? Which > application was killed?), then why are we still connected to host at > all? It's clear that this gadget can't work without its userspace > counterpart. If that userspace isn't available, we should drop data > pullup and disconnect from host. > Usb no need disconnect if the application exit (host side). Seems you only care about device side. > > These all can lead host send more than device wanted bytes. For sure > > it wrong at host side, but device side don't know. > > but none of this means we have a bug at device side. In fact, by > allowing these extra bytes to reach userspace, we could be creating a > possible attack vector. > > Your explanation is unsatisfactory, so I won't apply your patch, sorry. > > -- > balbi It is fine. Then need userspace take care of all the data it received. Because Kernel may drop some data for it. Kernel ffs driver is unauthentic sometimes. Best Regards, Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> Hi, > > changbin...@intel.com writes: > > From: "Du, Changbin" <changbin...@intel.com> > > > > Since the buffer size for req is rounded up to maxpacketsize, > > then we may end up with more data then user space has space > > for. > > only for OUT direction with the controller you're using ;-) For sure. > > > If it happen, we can keep the excess data for next i/o, or > > report an error. But we cannot silently drop data, because > > USB layer should ensure the data integrality it has transferred, > > otherwise applications may get corrupt data if it doesn't > > detect this case. > > and when has this actually happened ? Host should not send more data in > this case, if it does, it's an error on the host side. Also, returning > -EOVERFLOW is not exactly correct here, because you'd violate POSIX > specification of read(), right ? > This can happen if the host side app force kill-restart, not taking care of this special condition(and we are not documented), or even it is a bug. Usually APPs may has a protocol to control the packet size, but protocol mismatch can happen if either side encounter an error. Anyway, this is real. If kernel return success and drop data, the error may explosion later, or its totally hided (but why some data lost in kernel? Kernel cannot tell userspace we cannot be trusted sometimes, right?). so IMO, if this is an error, we need report an error or fix it, not hide it. The POSIX didn't say read cannot return "-EOVERFLOW", it says: " Other errors may occur, depending on the object connected to fd." If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions? > > Here, we simply report an error to userspace to let userspace > > proccess. Actually, userspace applications should negotiate > > no, this violates POSIX. Care to explain what problem are you actually > facing ? > Why this violates POSIX? Could you give more details? The problem is device side app sometimes received incorrect data caused by the dropping. Most times the error can be detected by APP itself, but sometimes cannot. It depends on the design of its communication protocol. > -- > Balbi Best Regards, Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> On Wed, May 11 2016, Felipe Balbi wrote: > > Also, returning -EOVERFLOW is not exactly correct here, because you'd > > violate POSIX specification of read(), right ? > > Maybe we could piggyback on: > >EINVAL fd was created via a call to timerfd_create(2) and the > wrong size buffer was given to read(); > > But I kinda agree. I’m not sure how much we need to care about this > instead of having user space round their buffers up to the nearest max > packet size boundary. > > -- > Best regards > ミハウ “퓶퓲퓷퓪86” ナザレヴイツ > «If at first you don’t succeed, give up skydiving» This is a good idea that "having user space round their buffers". But kernel Still cannot hide error silently. :)
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
Hi, > >> and when has this actually happened ? Host should not send more data in > >> this case, if it does, it's an error on the host side. Also, returning > >> -EOVERFLOW is not exactly correct here, because you'd violate POSIX > >> specification of read(), right ? > >> > > This can happen if the host side app force kill-restart, not taking care of > > this > > special condition(and we are not documented), or even it is a bug. Usually > APPs > > may has a protocol to control the packet size, but protocol mismatch can > happen > > if either side encounter an error. > > > > Anyway, this is real. If kernel return success and drop data, the error may > > explosion later, or its totally hided (but why some data lost in kernel? > > Kernel cannot tell userspace we cannot be trusted sometimes, right?). > > so IMO, if this is an error, we need report an error or fix it, not hide it. > > > > The POSIX didn't say read cannot return "-EOVERFLOW", it says: > > " Other errors may occur, depending on the object connected to fd." > > > > If "-EOVERFLOW" is not suitable, EFAULT, or any suggestions? > > > >> > Here, we simply report an error to userspace to let userspace > >> > proccess. Actually, userspace applications should negotiate > >> > >> no, this violates POSIX. Care to explain what problem are you actually > >> facing ? > >> > > Why this violates POSIX? Could you give more details? > > read(5) should return at mode 5 bytes. If there are more, than 5 bytes, > we don't error out, we just return the requested 5 bytes and wait for a > further read. > Yes, it is true. As I mentioned before, we also can keep the extra data for next read. This need more work to maintain a buffer. Here I just simply report an error(let userspace know something goes wrong.) before the logic is implemented by someone. (Maybe ioctl approach may be more appropriate for usb transfer, like usbfs.) > What I'm more concerned, however, is why we received more than > expected > data. What's on the extra bytes ? Can you capture dwc3 traces ? Perhaps > add a few traces doing a hexdump (using __print_hex()) of the data in > req->buf. > The extra bytes can be anything(random), they just data from APP layer. It doesn't make sense for you to check. So I will not dump them, sorry. > > The problem is device side app sometimes received incorrect data caused > > by the dropping. Most times the error can be detected by APP itself, but > > why ? app did e.g. read(5), that caused driver to queue a usb_request > with length set to 512. Host sent more data than the expected 5 bytes, > why did host do that ? And if that data was needed, why didn't userspace > read() more than 5 ? > > -- > Balbi Well, first, there must be a protocol upon usb between host side and device side. Second device side didn't know how many bytes to receive, it need host side tell it. But host could be buggy, or the application is killed and restart. These all can lead host send more than device wanted bytes. For sure it wrong at host side, but device side don't know. Best Regards, Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> Hi, > > "Du, Changbin" <changbin...@intel.com> writes: > >> right, and that was my point: if we copy more to userspace, then we have > >> a real big problem. > >> > > Yes, we drop the data because we userspace buffer is not enough this time. > > The problem here is that really can we just drop it silently? Maybe not. > > Yeah, it probably deserves a pr_err() or pr_debug(), but host sending > more data than it should, is another problem altogether which needs to > be addressed at the host. > > Adding a print to aid debugging is a good idea, but bailing out on the > peripheral side is not :-s > Ok, if we think this is a problem at host side that the transfer is not device expected, then device side should not accept the data or deliver the transferred data to userspace. But now we take part of the data to userspace and says it is ok. Do you agree with this point? IMO, we expose usb transfer as a file on device side. But file read() doesn't have a requirement that "sorry, you cannot read so little! you need read all once, else we may drop data for you. :) ". And some library that may retry read() until get enough data (which is normal For a general read). Then sometimes the buffer size for sys_read may not as expected. This is why I think ioctl approach is more appropriate for usb transfer. > -- > Balbi Best Regards, Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> There appears to be no kfifo support for iov_iter though, so I just went > with a simple buffer. > > I haven’t looked at the patch too carefully so this is an RFC rather > than an actual patch at this point. It does compile at least. > > Regardless, the more I thin about it, the more I’m under the impression > that the whole rounding up in f_fs was a mistake. And the more I’m > leaning towards ignoring the excess data set by the host. > > -- >8 -- > Subject: usb: gadget: f_fs: buffer data from ‘oversized’ OUT requests > > f_fs rounds up read(2) requests to a multiple of a max packet size > which means that host may provide more data than user has space for. > So far, the excess data has been silently ignored. > > This introduces a buffer for a tail of such requests so that they are > returned on next read instead of being ignored. > Congratulations finally reach an agreement, thanks Alan Stern and Michal. Here just have a comment - the buffered data need be dropped when the epfile is closed, because it means the session is terminated.
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
Hi, > >> Yeah, it probably deserves a pr_err() or pr_debug(), but host sending > >> more data than it should, is another problem altogether which needs to > >> be addressed at the host. > >> > >> Adding a print to aid debugging is a good idea, but bailing out on the > >> peripheral side is not :-s > >> > > Ok, if we think this is a problem at host side that the transfer is not > > device > > expected, then device side should not accept the data or deliver the > > transferred data to userspace. But now we take part of the data to > userspace > > and says it is ok. > > Do you agree with this point? > > We deliver to userspace the part userspace requested, right? So that's > okay. The USB details WRT e.g. babble or host trying to send more data > than expected, needs to be handled within the kernel. > *babble* error is a good example. As you know, when xhci received more data than expected, how does it process it? It doesn't deliver to software the part userspace requested, but just give an error. And the xhci driver set the urb status to -EOVERFLOW. This is similar to device side between kernel and userspace. > > IMO, we expose usb transfer as a file on device side. But file read() > > doesn't > > have a requirement that "sorry, you cannot read so little! you need read all > > once, else we may drop data for you. :) ". > > but that's not how read() semantics work. When userspace asks to read(x) > bytes, we have three possible outcomes: > > i. We have x bytes to return, so we copy_to_user(x) > > ii. We have y < x bytes to return, so we copy_to_user(y) > > iii. We have y > x bytes to return, so we copy_to_user(x) > I totally agree with these. They are all right. But what if userspace read Twice? EG. When it want read a its packet, it may first read a head size, Then read body. read(20) = read(5) + read(15) If this normal for a file, and works well, right? But if it happens on FunctionFS, the first read success, but the remailing 15 bytes lost because they dropped by kernel. You may think it is host's bug, which also means FunctionFS file does be different. You can compare usb with network, Does network driver drop data? Afaik, You can read any bytes from a socket, and data never lost. For example, the host may send "aaabbb" and "cccddd", and device side app May get "aaaccc" and all read success! Host send also success! But "bbb" "ddd" are lost. The different views we have is on how to process the extra data. So we can focus discussion here. > This is exactly how the kernel is behaving. The only "detail" we have is > that, for some reason, host is sending too much data. what I still don't > know is if this extra data is garbage or something userspace genuinely > cares about. Do you know the answer to this? > IMO, FunctionFS should not care about what the data is. That is userspace' s logic and meaningless for kernel. Kernel should not assume the data is garbage or not. Is it right? PS, for adb, it will re-open FunctionFS file after it detect unexpected data. > > And some library that may retry read() until get enough data (which is > > normal For a general read). Then sometimes the buffer size for > > sys_read may not as expected. This is why I think ioctl approach is > > more appropriate for usb transfer. > > no, this won't change anything. Besides, it's a pointless discussion as > cannot break userspace ABI. GadgetFS and FunctionFS have been shipping > in kernel for many years. > That because developers know the special requirement of FunctionFS, just like adb. > -- > Balbi Do you mind if I modify my original patch for print error instead of return error? Best Regards, Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> >> > These all can lead host send more than device wanted bytes. For sure
> >> > it wrong at host side, but device side don't know.
> >>
> >> but none of this means we have a bug at device side. In fact, by
> >> allowing these extra bytes to reach userspace, we could be creating a
> >> possible attack vector.
> >>
> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
> >>
> >> --
> >> balbi
> > It is fine. Then need userspace take care of all the data it received.
> > Because
> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
> sometimes.
>
> I really cannot understand what you mean sometimes. You're saying that
> userspace needs to take care of all the data it received because kernel
> can drop data. If kernel is dropping data, there's no extra data
> reaching userspace, right?
>
For sure, maybe I didn't describe it well so let you confused. :)
> Is the problem that we *are* giving more data than expected to
> userspace? Are we overflowing some userspace buffer? If that's the case,
> then below should be enough for the time being:
>
No, the problem is we drop data but silently. We cannot give more data to
userspace since buffer is limited.
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 73515d54e1cc..d1bd53c895ca 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -156,6 +156,8 @@ struct ffs_io_data {
> struct usb_request *req;
>
> struct ffs_data *ffs;
> +
> + ssize_t expected_len;
> };
>
> struct ffs_desc_helper {
> @@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>* Controller may require buffer size to be aligned to
>* maxpacketsize of an out endpoint.
>*/
> - if (io_data->read)
> + if (io_data->read) {
> + io_data->expected_len = data_len;
> data_len = usb_ep_align_maybe(gadget, ep->ep,
> data_len);
> + }
> spin_unlock_irq(>ffs->eps_lock);
>
> data = kmalloc(data_len, GFP_KERNEL);
> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>*/
> ret = interrupted ? -EINTR : ep->status;
> if (io_data->read && ret > 0) {
> - ret = copy_to_iter(data, ret, _data->data);
> + if (ret > io_data->expected_len)
> + pr_debug("FFS: size mismatch: %zd for %zd",
> + ret, io_data->expected_len);
> +
> + ret = copy_to_iter(data, io_data->expected_len,
> + _data->data);
> if (!ret)
> ret = -EFAULT;
> }
>
> that we can get merged during v4.7-rc and Cc stable and backport this to
> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
> use put iov_iter into io_data").
>
The different for this code is just give warning but not return error. It is
also
fine for me that at least this let development can find some key message to
find What happed under kernel. But the message should be *error* I think.
And this missed AIO path. This is identify to my patch after remove the
"return -EOVERFLOW;" line.
Byw, we not need add the field "expected_len", we can get it from the
struct ffs_io_data.
If this is fine for you, I can publish a new patch.
> --
> Balbi
Best Regards,
Du, Changbin
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
Hi,
>
> we need a min() here. Better version below:
No need. copy_to_iter will do it for us.
Best Regards,
Du, Changbin
>
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index 73515d54e1cc..6c49b152f46e 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -156,6 +156,8 @@ struct ffs_io_data {
> struct usb_request *req;
>
> struct ffs_data *ffs;
> +
> + ssize_t expected_len;
> };
>
> struct ffs_desc_helper {
> @@ -730,8 +732,10 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>* Controller may require buffer size to be aligned to
>* maxpacketsize of an out endpoint.
>*/
> - if (io_data->read)
> + if (io_data->read) {
> + io_data->expected_len = data_len;
> data_len = usb_ep_align_maybe(gadget, ep->ep,
> data_len);
> + }
> spin_unlock_irq(>ffs->eps_lock);
>
> data = kmalloc(data_len, GFP_KERNEL);
> @@ -811,7 +815,15 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> ffs_io_data *io_data)
>*/
> ret = interrupted ? -EINTR : ep->status;
> if (io_data->read && ret > 0) {
> - ret = copy_to_iter(data, ret, _data->data);
> + ssize_t bytes;
> +
> + if (ret > io_data->expected_len)
> + pr_debug("FFS: size mismatch: %zd for %zd",
> + ret, io_data->expected_len);
> +
> + bytes = min(ret, io_data->expected_len);
> +
> + ret = copy_to_iter(data, bytes, _data->data);
> if (!ret)
> ret = -EFAULT;
> }
>
>
> --
> balbi
RE: [PATCH] usb: gadget: f_fs: report error if excess data received
> Hi,
>
> "Du, Changbin" <changbin...@intel.com> writes:
> >> >> > These all can lead host send more than device wanted bytes. For
> sure
> >> >> > it wrong at host side, but device side don't know.
> >> >>
> >> >> but none of this means we have a bug at device side. In fact, by
> >> >> allowing these extra bytes to reach userspace, we could be creating a
> >> >> possible attack vector.
> >> >>
> >> >> Your explanation is unsatisfactory, so I won't apply your patch, sorry.
> >> >>
> >> >> --
> >> >> balbi
> >> > It is fine. Then need userspace take care of all the data it received.
> Because
> >> > Kernel may drop some data for it. Kernel ffs driver is unauthentic
> >> sometimes.
> >>
> >> I really cannot understand what you mean sometimes. You're saying that
> >> userspace needs to take care of all the data it received because kernel
> >> can drop data. If kernel is dropping data, there's no extra data
> >> reaching userspace, right?
> >>
> > For sure, maybe I didn't describe it well so let you confused. :)
>
> okay
>
> >> Is the problem that we *are* giving more data than expected to
> >> userspace? Are we overflowing some userspace buffer? If that's the case,
> >> then below should be enough for the time being:
> >>
> > No, the problem is we drop data but silently. We cannot give more data to
>
> okay, but does that create any problems for device side userspace? What
> problem is that?
>
> > userspace since buffer is limited.
>
> right, and that was my point: if we copy more to userspace, then we have
> a real big problem.
>
Yes, we drop the data because we userspace buffer is not enough this time.
The problem here is that really can we just drop it silently? Maybe not.
> >> @@ -811,7 +815,12 @@ static ssize_t ffs_epfile_io(struct file *file, struct
> >> ffs_io_data *io_data)
> >> */
> >>ret = interrupted ? -EINTR : ep->status;
> >>if (io_data->read && ret > 0) {
> >> - ret = copy_to_iter(data, ret, _data->data);
> >> + if (ret > io_data->expected_len)
> >> + pr_debug("FFS: size mismatch: %zd for %zd",
> >> + ret, io_data->expected_len);
> >> +
> >> + ret = copy_to_iter(data, io_data->expected_len,
> >> + _data->data);
> >>if (!ret)
> >>ret = -EFAULT;
> >>}
> >>
> >> that we can get merged during v4.7-rc and Cc stable and backport this to
> >> anything containing Al's commit c993c39b8639 ("gadget/function/f_fs.c:
> >> use put iov_iter into io_data").
> >>
> >
> > The different for this code is just give warning but not return
> > error. It is also fine for me that at least this let development can
> > find some key message to find What happed under kernel. But the
> > message should be *error* I think.
>
> I'm fine with pr_error()
>
> > And this missed AIO path. This is identify to my patch after remove the
>
> right, it's more of a debug patch since I don't have the setup to
> trigger this (I'm assuming you're using adb?)
>
Right. And adb can detect this unexpected behavior(data mismatch) quickly
because it has some selfcheck for the data content.
> > "return -EOVERFLOW;" line.
>
> there's one key difference, see below
>
> > Byw, we not need add the field "expected_len", we can get it from the
> > struct ffs_io_data.
>
> without expected_len we can copy more data to userspace, right ? If
> req->actual > data_len_before_aligning_to_maxpacket, then we will copy
> more data then we should to userspace and this was a regression caused
> by Al's commit, AFAICT.
>
No, expected_len equals to iov_iter_count(_data->data), right? So we
do not need a new field.
> > If this is fine for you, I can publish a new patch.
> >
> >> --
> >> Balbi
> >
> > Best Regards,
> > Du, Changbin
>
> --
> balbi
RE: [PATCH] usb: hub: fix panic caused by NULL bos pointer during reset device
> > I think Greg is referring to commit 464ad8c43a9e ("usb: core : hub: Fix
> > BOS 'NULL pointer' kernel panic"), which has already been applied
> > upstream. It looks to me like that patch might have fixed the same
> > problem in a different way, in which case Changbin's patch is not
> > needed. But I haven't been involved in developing or testing that
> > patch, so I can't say for sure. At the very least, 464ad8c43a9e
> > conflicts with Changbin's patch.
> >
> > Changbin, can you take a look at 464ad8c43a9e and see if that fixes the
> > same problem that your patch did?
>
> Given the lack of response here, I've dropped this from my queue. If
> it's still needed, someone must resend it.
>
> thanks,
>
> greg k-h
Hi,
I missed this email because it was junked by my email client. Just checked
patch 464ad8c43a9e, it fix the same issue. So my patch no longer need now.
Thanks for the patch.
Best Regards,
Du, Changbin
RE: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC
Hi, > > On most platforms, there is only one device controller available. > > In this case, we desn't care the UDC's name. So let's ignore the > > name by setting 'UDC' to 'any'. > > Hmm libubsgx allows to do this for a very long time. You simply pass > NULL instead of pointer to usbg_udc. > > It is also possible to do this from command line, just simply: > > $ echo `ls -1 /sys/class/udc | head -n 1` > UDC > > So if we can easily do this from user space what's the benefit of adding > this special "any" keyword to kernel? > Well, it is just for *easy to use*. Looking up /sys/class/udc mostly can be skipped. The UDC core support this convenience behavior, so why don't we export it with a little change? > > And also we can change UDC name > > at any time if it is not binded (no need set to "" first). > > > > Not sure if: > > $ echo "" > UDC > > is really a problem. Personally I'm quite used to situation in which I > have to turn the light off before turning it on once again;) > That is not a problem. But just avoid pseudo 'busy'. If gadget is not bind, it is free to reconfigure it. So seem no need block re-configuration. In a word, this patch is just an improvement, not to fix any issues or add new function. > Cheers, > -- > Krzysztof Opasiak > Samsung R Institute Poland > Samsung Electronics Thanks, Du, Changbin
RE: [PATCH 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC
> >>> On most platforms, there is only one device controller available. > >>> In this case, we desn't care the UDC's name. So let's ignore the > >>> name by setting 'UDC' to 'any'. > >> > >> Hmm libubsgx allows to do this for a very long time. You simply pass > >> NULL instead of pointer to usbg_udc. > >> > >> It is also possible to do this from command line, just simply: > >> > >> $ echo `ls -1 /sys/class/udc | head -n 1` > UDC > >> > >> So if we can easily do this from user space what's the benefit of adding > >> this special "any" keyword to kernel? > >> > > Well, it is just for *easy to use*. Looking up /sys/class/udc mostly > > can be skipped. The UDC core support this convenience behavior, > > so why don't we export it with a little change? > > > > Well, I'm not sure if any configfs interface has been proposed as easy > to use from cmd line. I think they all has been proposed as *usable* > from cmd line but not necessarily *easy to use*. > > That's why most of configfs clients has some support in userspace. For > example for target there is a taget-cli and for usb gadget we have > libusbg/libusbgx. > Glade to know this tool, it is much more easy to use than interact with sysfs. I'd like use it. Just see you are the main contributor of this project. :) > So the functionality which you proposed here is not only already > implemented in libusbgx but also can be easily achieved from cmd line > like I showed above. > > In addition this patch will break existing userspace tools (at least > libusbgx for sure) as it assumes that: > > cat UDC > > should return an empty string or an valid UDC name which can be found > inside /sys/class/udc. If so, this is not good. > >> is really a problem. Personally I'm quite used to situation in which I > >> have to turn the light off before turning it on once again;) > >> > > That is not a problem. But just avoid pseudo 'busy'. If gadget is not > > bind, it is free to reconfigure it. So seem no need block re-configuration. > > > > What do you mean pseudo 'busy'? If we do: > > echo > UDC > Sorry, please ignore this. I find if no UDC available, the config will be queued to a list, and will bind it when a UDC module install. So it is really busy. > then gadget should be really bound to some udc and potentially really busy. > > > In a word, this patch is just an improvement, not to fix any issues or > > add new function. > > So it doesn't add any new functionality and breaks existing user space > tools. > Ok, regarding there is a better tool, this change doesn't make much sense. So just abandon it. > Cheers, > -- > Krzysztof Opasiak > Samsung R Institute Poland > Samsung Electronics Best Regards, Du, Changbin
RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks
> From: Thomas Gleixner [mailto:t...@linutronix.de] > On Sun, 8 May 2016, Du, Changbin wrote: > > > From: Thomas Gleixner [mailto:t...@linutronix.de] > > > > raw_spin_unlock_irqrestore(>lock, flags); > > > > /* > > > > -* Maybe the object is static. Let the type specific > > > > +* Maybe the object is static. Let the type specific > > > > * code decide what to do. > > > > > > Instead of doing white space changes you really want to explain the logic > > > here. > > > > > Comments is in following code. > > Well. It's a comment, but the code you replace has better explanations about > statically initialized objects. This should move here. > > Thanks, > > tglx Ok, let me improve the comment for patch v2. Best Regards, Du, Changbin
RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Can you please fix your mail client. Every mail you send has:
>
> Cc: .....
> "Du, Changbin" <changbin...@intel.com>,
> Du
>
> And that stray 'Du' is just broken.
>
Yes, I should add "" around my name or fix the git-sendemail perl script.
The script may add a "" auto. :)
> > At last, I have a concern about the fixups that can it change the
> > object which is in incorrect state on fixup? Because the 'addr' may
> > not point to any valid object if a non-static object is not tracked.
> > Then Change such object can overwrite someone's memory and cause
> > unexpected behaviour. For example, the timer_fixup_activate bind
> > timer to function stub_timer.
>
> Well, you have the choice of:
>
> 1) Leave the object uninitialized and watch the resulting explosion
>
> 2) Assume that the pointer is a valid object and initialize it
>
> The latter has been chosen as the lesser of two evils.
>
Hmm, seems nothing more we can do to reduce further damage.
> > raw_spin_unlock_irqrestore(>lock, flags);
> > /*
> > -* Maybe the object is static. Let the type specific
> > +* Maybe the object is static. Let the type specific
> > * code decide what to do.
>
> Instead of doing white space changes you really want to explain the logic
> here.
>
Comments is in following code.
> > */
> > - if (debug_object_fixup(descr->fixup_assert_init, addr,
> > - ODEBUG_STATE_NOTAVAILABLE))
> > + if (descr->is_static_object && descr->is_static_object(addr))
> {
> > + /* Make sure that it is tracked in the object tracker */
> > + debug_object_init(addr, descr);
> > + } else {
> > debug_print_object(, "assert_init");
> > + debug_object_fixup(descr->fixup_assert_init, addr,
> > + ODEBUG_STATE_NOTAVAILABLE);
> > + }
> > return;
> > }
>
> Other than the missing comment this looks good.
>
> Thanks,
>
> tglx
Thanks for reviewing.
Du, Changbin
Re: [PATCH 1/2] kconfig/mconf: add jumping tip in title of search result textbox
On Mon, Feb 06, 2017 at 02:00:51PM +0200, Jani Nikula wrote:
> On Mon, 06 Feb 2017, changbin...@intel.com wrote:
> > From: Changbin Du <changbin...@intel.com>
> >
> > Prompt user how to quickly jump to the item he/she is interested in.
>
> :o
>
> All these years. I... I didn't know. Thanks!
>
aha, me too! You know, back to the top menu then look into step by
step...
> > Signed-off-by: Changbin Du <changbin...@intel.com>
> > ---
> > scripts/kconfig/mconf.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/kconfig/mconf.c b/scripts/kconfig/mconf.c
> > index 315ce2c..23d5681 100644
> > --- a/scripts/kconfig/mconf.c
> > +++ b/scripts/kconfig/mconf.c
> > @@ -443,10 +443,10 @@ static void search_conf(void)
> >
> > res = get_relations_str(sym_arr, );
> > set_subtitle();
> > - dres = show_textbox_ext(_("Search Results"), (char *)
> > - str_get(), 0, 0, keys, ,
> > - , _text, (void *)
> > - );
> > + dres = show_textbox_ext(
> > + _("Search Results (type the number to jump)"),
> > + (char *)str_get(), 0, 0, keys, ,
> > + , _text, (void *));
>
> It would be even better and discoverable if this could be turned into a
> dialog menu, so that you could navigate the search results using arrow
> keys and hit enter to choose. But this is already an improvement.
>
Yes, that will have a better experience. :)
> > again = false;
> > for (i = 0; i < JUMP_NB && keys[i]; i++)
> > if (dres == keys[i]) {
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Thanks,
Changbin
signature.asc
Description: PGP signature
Re: [PATCH 2/2] Documentation/kconfig: add search jump feature description
On Mon, Feb 06, 2017 at 07:42:11AM -0700, Jim Davis wrote:
> On Mon, Feb 6, 2017 at 12:46 AM, <changbin...@intel.com> wrote:
> > From: Changbin Du <changbin...@intel.com>
> >
> > Kernel menuconfig support direct jumping function from the search
> > result. This is a very convenient feature but not documented. So
> > add a short description to the kconfig documentation to let more
> > developer know it.
> >
> > Signed-off-by: Changbin Du <changbin...@intel.com>
> > ---
> > Documentation/kbuild/kconfig.txt | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/kbuild/kconfig.txt
> > b/Documentation/kbuild/kconfig.txt
> > index bbc99c0..9916318 100644
> > --- a/Documentation/kbuild/kconfig.txt
> > +++ b/Documentation/kbuild/kconfig.txt
> > @@ -178,6 +178,10 @@ Searching in menuconfig:
> > first (and in alphabetical order), then come all other symbols,
> > sorted in alphabetical order.
> >
> > + In the search result textbox, each symbol has a jump number on
> > + left side if the symbol is jumpable. You can type the nubmer
>
> number ("jumpable" reads a bit oddly, but it is understandable.)
>
How about 'if the symbol is visible'?
> > + to jump to target menu to configurate that symbol.
>
> configure?
>
Thanks, I will correct them.
> --
> Jim
--
Thanks,
Changbin
signature.asc
Description: PGP signature
RE: [Intel-gfx] [PATCH] drm/i915: check if execlist_port is empty before using its content
> On Fri, Dec 23, 2016 at 01:46:36PM +0800, changbin...@intel.com wrote: > > From: "Du, Changbin" <changbin...@intel.com> > > > > This patch fix a crash in function reset_common_ring. In this case, > > the port[0].request is null when reset the render ring, so a null > > dereference exception is raised. We need to check execlist_port status > > first. > > No. The root cause is whatever got you into the illegal condition in the > first place. > -Chris > Thanks, I will restudy the code after process my current job. Since this happen on gvt guest, so this may related to gvt emulation. > -- > Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH v2 0/2] kconfig/mconf: propagate jumping function in search result view
Sencond ping... if this is realy not necessary I give up. On Mon, Mar 06, 2017 at 06:28:50PM +0800, Du, Changbin wrote: > Hello, any new update need for this or new comments? :) > > On Wed, Feb 08, 2017 at 11:09:37AM +0800, changbin...@intel.com wrote: > > From: Changbin Du <changbin...@intel.com> > > > > While I am searching something in menuconfig, I hope if I can jump to > > interested > > items directly. I even try to add this feature. When I check the code just > > found > > it has already been there but not documented. So why let more developers > > know it? > > > > v2: correct spell (Jim) > > > > Changbin Du (2): > > kconfig/mconf: add jumping tip in title of search result textbox > > Documentation/kconfig: add search jump feature description > > > > Documentation/kbuild/kconfig.txt | 4 > > scripts/kconfig/mconf.c | 8 > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > -- > > 2.7.4 > > > > -- > Thanks, > Changbin Du -- Thanks, Changbin Du signature.asc Description: PGP signature
Re: [PATCH] perf lock: subcommands should include common options
On Fri, Mar 17, 2017 at 11:08:34AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Mar 17, 2017 at 01:53:42PM +0800, changbin...@intel.com escreveu:
> > From: Changbin Du <changbin...@intel.com>
> >
> > When I use -i option for report subcommand, it doesn't accept it.
> > We need add common options using OPT_PARENT macro.
> >
> > perf lock report -i lock_perf.data
> > Error: unknown switch `i'
> >
> > Usage: perf lock report []
> >
> > -f, --force don't complain, do it
> > -k, --key key for sorting ...
> >
> > Signed-off-by: Changbin Du <changbin...@intel.com>
> > ---
> > tools/perf/builtin-lock.c | 19 +++
> > 1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index ce3bfb4..710c551 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -947,27 +947,30 @@ static int __cmd_record(int argc, const char **argv)
> >
> > int cmd_lock(int argc, const char **argv, const char *prefix
> > __maybe_unused)
> > {
> > + const struct option lock_options[] = {
> > + OPT_STRING('i', "input", _name, "file", "input file name"),
> > + OPT_INCR('v', "verbose", , "be more verbose (show symbol
> > address, etc)"),
> > + OPT_BOOLEAN('D', "dump-raw-trace", _trace, "dump raw trace in
> > ASCII"),
> > + OPT_END()
> > + };
> > +
> > const struct option info_options[] = {
> > OPT_BOOLEAN('t', "threads", _threads,
> > "dump thread list in perf.data"),
> > OPT_BOOLEAN('m', "map", _map,
> > "map of lock instances (address:name table)"),
> > OPT_BOOLEAN('f', "force", , "don't complain, do it"),
>
> Good catch, OPT_PARENT came after 'perf lock', but you forgot 'f', I'm
> adding it and renaming 'lock_options' to 'lock_input_options', ok?
>
I am okay, just go ahead. thanks.
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH] perf report: show sort_order in title
On Tue, Mar 14, 2017 at 10:04:16AM -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Mar 14, 2017 at 10:16:44AM +0800, Du, Changbin escreveu: > > On Mon, Mar 13, 2017 at 11:57:18AM -0300, Arnaldo Carvalho de Melo wrote: > > > But then, while testing, > > > > Before: > > > > $ perf report > > > Samples: 405 of event 'cycles', Event count (approx.): 101733003 > > > Overhead Command Shared ObjectSymbol > > > 11.15% swapper [kernel.vmlinux] [k] > > > intel_idle > > > > Tip: Save output of perf stat using: perf stat record > > > > After: > > > > $ perf report > > > Samples: 405 of event 'cycles', Event count (approx.): 101733003, Sort > > > by: Children,Overhead,Command,Shared Object,Symbol > > > Overhead Command Shared ObjectSymbol > > > 11.15% swapper [kernel.vmlinux] [k] > > > intel_idle > > > > I see now duplication of info, where is the value? Can you show the > > > usecase in > > > a compelling way? > > > Thanks for trying. The key idea is to show how does the data sort, > > especially > > the first sort key. When I use some GUI based perf tool, I can see how > > my data is sorted by checking the report header status. I think this is > > a good for browser. > > > You are right, the info is duplicated. I got another idea that we show a > > '↓' at > > the header string and only for the first sort key. What do you think? > > > $ perf report > >Samples: 405 of event 'cycles', Event count (approx.): 101733003 > >↓Overhead Command Shared ObjectSymbol > > this is much more compact, but you need to make it abundantly clear what > you are trying to achieve by showind counter examples were what we get > on that line starting with your suggested marker isn't the sort order. > Otherwise even a character is one too much :-) > Yes, I just want get know how does perf data sort. Because sometimes the real sort order doesn't match the '-s' option I given. In this case, I was confused about the sorting before reading into the code. > > 11.15% swapper [kernel.vmlinux] [k] > > intel_idle > > 3.00% firefox libxul.so[.] > > 0x01298b8d > > 1.74% swapper [kernel.vmlinux] [k] > > update_blocked_averages > > > Another idea I want to add is to support dynamic sorting. For me, I use > > perf to > > analysing entire system performance, and the data is very large. Then > > sometimes > > it take as long as ~10 minitues to read perf data. So I think if we can > > change > > sort w/o reload data will be good. > > And in some cases it is even possible! I.e. if you haven't collapsed too > much, you will not have to reprocess the file to get to the new order. > > BTW, have you played with: > > perf top --hierarchy > > Try it with -g and --call-graph dwarf > > Also try: > > perf report --hierarchy > > - Arnaldo Sounds great! I tried '--hierarchy' option, but still don't know how to resort the report. Could you give a hint? -- Thanks, Changbin Du signature.asc Description: PGP signature
Re: [PATCH] perf sort: only insert overhead && overhead_children when no overhead* field given
hi, Arnaldo,
Seems you missed this one. In some case, sort by 'Children' first doesn't make
sense, bacause I only care the 'Self' overhead.
btw, does perf have a option that only calculate the *real* callee
'overhead_acc'?
I mean only count the samples to parent when it is called by parent. For
example,
fun1fun2
foo foo
Then the 'overhead_acc' for fun1 should not include the samples that fun2 call
foo.
Thanks.
changbin
On Mon, Mar 13, 2017 at 04:36:01PM +0800, changbin...@intel.com wrote:
> From: Changbin Du <changbin...@intel.com>
>
> If we always insert 'overhead' and 'overhead_children' as sort keys,
> this make it impossible to sort as overhead (which displayed as Self)
> first. This patch forbid adding any overhead* field if there is one
> already given.
>
> Signed-off-by: Changbin Du <changbin...@intel.com>
> ---
> tools/perf/util/sort.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index b6db140..7695b54 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -2543,6 +2543,13 @@ static char *setup_overhead(char *keys)
> if (sort__mode == SORT_MODE__DIFF)
> return keys;
>
> + /*
> + * Only insert overhead && overhead_children when
> + * no overhead* field given.
> + */
> + if (strstr(keys, "overhead"))
> + return keys;
> +
> keys = prefix_if_not_in("overhead", keys);
>
> if (symbol_conf.cumulate_callchain)
> --
> 2.7.4
>
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
On Tue, Apr 11, 2017 at 12:32:49PM +0200, Jiri Olsa wrote:
> On Tue, Apr 11, 2017 at 06:13:17PM +0800, Du, Changbin wrote:
> > > > >
> > > > yes, this is an option. But for safety, I sugguest do not rely on
> > > > list_del_init.
> > > > No rule rather than create one.
> > > >
> > > > But anyway, both are ok for me. What's your options?
> > >
> > > hum, also I dont think we need to touch that bit at all
> > > if we are going to remove it right away.. how about the
> > > change below?
> > >
> > > jirka
> > >
> > >
> > > ---
> > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > index 5d632dca672a..0ee7db43dd7d 100644
> > > --- a/tools/perf/ui/hist.c
> > > +++ b/tools/perf/ui/hist.c
> > > @@ -613,15 +613,15 @@ void perf_hpp__reset_output_field(struct
> > > perf_hpp_list *list)
> > >
> > > /* reset output fields */
> > > perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > - list_del_init(>list);
> > > - list_del_init(>sort_list);
> > > + list_del(>list);
> > > + /* Remove the fmt from next loop processing. */
> > > + list_del(>sort_list);
> > > fmt_free(fmt);
> > What if the fmt is not linked to sort_list? I see it is possible (please
> > checking perf_hpp__setup_output_field()). I am not sure if we really has
> > sunch case currently, just concern :)
>
> if it's not linked to sort_list, then sort_list is initialized
> and list_del should do no harm
>
ok, then it's fine if you insist.
> jirka
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
On Sun, Apr 09, 2017 at 07:05:52PM +0200, Jiri Olsa wrote: > On Wed, Apr 05, 2017 at 10:44:22AM +0800, Du, Changbin wrote: > > On Tue, Apr 04, 2017 at 12:51:03PM -0300, Arnaldo Carvalho de Melo wrote: > > > Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu: > > > > Hi Arnaldo, > > > > > > > > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo > > > > <a...@kernel.org> wrote: > > > > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin...@intel.com > > > > > escreveu: > > > > >> From: Changbin Du <changbin...@intel.com> > > > > >> > > > > >> Some perf_hpp_fmt both registered at field and sort list. For such > > > > >> instance, we only can free it when removed from the both lists. This > > > > >> function currently only used by self-test code, but still should fix > > > > >> it. > > > > > > > > > > Looks sane, applying, > > > > > > > > > > Jiri, Namhyung, please holler (or ack) if needed, > > > > > > > > Did you actually see the double free problem? AFAICS the old code > > > > > > I assumed that he had seen it, in some self-test code, Changbin, can you > > > please show command output or further describe when this patch would be > > > necessary? > > > > > Arnaldo, I did observe this issue but not in self-test code. The self-test > > code > > uses that function but does not have a case that a fmt linked to two both > > list. > > I found this issue when I try to add 'dynamic sort' feature to perf, which > > I use this function to reset out fields. > > could you post it with the rest of your patches? might be easier to review > jirka, I am sorry that the 'dynamic sort' is only half done. Now I am very busy with work at hand. I will send the rest of patches when I finish them. Could we check out this fix first? > thanks, > jirka -- Thanks, Changbin Du signature.asc Description: PGP signature
Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
On Mon, Apr 10, 2017 at 10:39:50AM +0200, Jiri Olsa wrote:
> On Tue, Apr 04, 2017 at 12:19:40PM -0300, Arnaldo Carvalho de Melo wrote:
>
> SNIP
>
> > > ---
> > > tools/perf/ui/hist.c | 25 +++--
> > > 1 file changed, 15 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > index 5d632dc..f94b301 100644
> > > --- a/tools/perf/ui/hist.c
> > > +++ b/tools/perf/ui/hist.c
> > > @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> > >
> > > void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> > > {
> > > - struct perf_hpp_fmt *fmt, *tmp;
> > > + struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
> > >
> > > /* reset output fields */
> > > - perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > - list_del_init(>list);
> > > - list_del_init(>sort_list);
> > > - fmt_free(fmt);
> > > + perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> > > + list_del_init(_fmt->list);
> > > + /* reset sort keys */
> > > + perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) {
> > > + if (field_fmt == sort_fmt) {
> > > + list_del_init(_fmt->sort_list);
> > > + break;
> > > + }
> > > + }
>
> I agree with Namhyung in here.. seems like the only thing you
> added is to check if the field_fmt was also linked in as a sort
> entry before you call list_del_init on it
>
This is correct.
> which I think should be also done with list_empty function, but
> more importantly I dont see a reason for that.. list_del_init
> call should be fine on empty list
>
You didn't catch the problem here. The problem is double free a fmt.
For exampe, fmt A is linked to both list. Then it will be first free
by the first iteration over list, then it will be freed again at the
second iteration over sort_list. This must cause application crash.
> please describe the issue in more details, perhaps we'ew missing
> something
>
> jirka
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
On Mon, Apr 10, 2017 at 01:33:25PM +0200, Jiri Olsa wrote:
> On Mon, Apr 10, 2017 at 06:21:12PM +0800, Du, Changbin wrote:
> > On Mon, Apr 10, 2017 at 10:39:50AM +0200, Jiri Olsa wrote:
> > > On Tue, Apr 04, 2017 at 12:19:40PM -0300, Arnaldo Carvalho de Melo wrote:
> > >
> > > SNIP
> > >
> > > > > ---
> > > > > tools/perf/ui/hist.c | 25 +++--
> > > > > 1 file changed, 15 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > > > index 5d632dc..f94b301 100644
> > > > > --- a/tools/perf/ui/hist.c
> > > > > +++ b/tools/perf/ui/hist.c
> > > > > @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt)
> > > > >
> > > > > void perf_hpp__reset_output_field(struct perf_hpp_list *list)
> > > > > {
> > > > > - struct perf_hpp_fmt *fmt, *tmp;
> > > > > + struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2;
> > > > >
> > > > > /* reset output fields */
> > > > > - perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > > > - list_del_init(>list);
> > > > > - list_del_init(>sort_list);
> > > > > - fmt_free(fmt);
> > > > > + perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) {
> > > > > + list_del_init(_fmt->list);
> > > > > + /* reset sort keys */
> > > > > + perf_hpp_list__for_each_sort_list_safe(list, sort_fmt,
> > > > > tmp2) {
> > > > > + if (field_fmt == sort_fmt) {
> > > > > + list_del_init(_fmt->sort_list);
> > > > > + break;
> > > > > + }
> > > > > + }
> > >
> > > I agree with Namhyung in here.. seems like the only thing you
> > > added is to check if the field_fmt was also linked in as a sort
> > > entry before you call list_del_init on it
> > >
> > This is correct.
> >
> > > which I think should be also done with list_empty function, but
> > > more importantly I dont see a reason for that.. list_del_init
> > > call should be fine on empty list
> > >
> > You didn't catch the problem here. The problem is double free a fmt.
> > For exampe, fmt A is linked to both list. Then it will be first free
> > by the first iteration over list, then it will be freed again at the
> > second iteration over sort_list. This must cause application crash.
>
> the original code takes it out of both lists,
> so the next itaration won't go over that entry
>
oh, my bad, my desc is wrong. I replayed the crash. The problem is
list_del_init a unlinked entry.
perf: Segmentation fault
backtrace
./perf[0x57394b]
/lib/x86_64-linux-gnu/libc.so.6(+0x354b0)[0x7fb8da3034b0]
./perf(perf_hpp__reset_output_field+0xb7)[0x55dfe7]
./perf(hists__sort_by_fields+0x3d7)[0x509777]
./perf[0x5704c1]
./perf(perf_evlist__tui_browse_hists+0x2e5)[0x5723e5]
./perf(cmd_report+0x1a9b)[0x43b4fb]
./perf[0x494731]
./perf(main+0x704)[0x426304]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7fb8da2ee830]
./perf(_start+0x29)[0x4263f9]
[0x0]
(gdb) print fmt.list
$4 = {next = 0x100, prev = 0x200}// LIST_POISON
(gdb) print fmt.sort_list
$5 = {next = 0x9727d0 <perf_hpp_list+16>, prev = 0x9727d0 <perf_hpp_list+16>}
In this case, the fmt is linked in sort_list, but not in list. So crash
at the list_del_init(>list) of second loop.
Another potential case is the fmt is linked in list, but not in sort_list.
Oh, my brain was broken. correct patch but wrong commit message. :(
Will drop this one and submit a new one.
> jirka
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
> > >
> > yes, this is an option. But for safety, I sugguest do not rely on
> > list_del_init.
> > No rule rather than create one.
> >
> > But anyway, both are ok for me. What's your options?
>
> hum, also I dont think we need to touch that bit at all
> if we are going to remove it right away.. how about the
> change below?
>
> jirka
>
>
> ---
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5d632dca672a..0ee7db43dd7d 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -613,15 +613,15 @@ void perf_hpp__reset_output_field(struct perf_hpp_list
> *list)
>
> /* reset output fields */
> perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> - list_del_init(>list);
> - list_del_init(>sort_list);
> + list_del(>list);
> + /* Remove the fmt from next loop processing. */
> + list_del(>sort_list);
> fmt_free(fmt);
What if the fmt is not linked to sort_list? I see it is possible (please
checking perf_hpp__setup_output_field()). I am not sure if we really has
sunch case currently, just concern :)
> }
>
> /* reset sort keys */
> perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) {
> - list_del_init(>list);
> - list_del_init(>sort_list);
> + list_del(>sort_list);
> fmt_free(fmt);
> }
> }
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
> > (gdb) print fmt.sort_list
> > $5 = {next = 0x9727d0 , prev = 0x9727d0
> > }
> >
> > In this case, the fmt is linked in sort_list, but not in list. So crash
> > at the list_del_init(>list) of second loop.
>
> so the only place I can see the POISON could get there
> is in perf_hpp__column_unregister.. can't we just get
> rid of it like below
>
> jirka
>
>
> ---
> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> index 5d632dca672a..7577effbf746 100644
> --- a/tools/perf/ui/hist.c
> +++ b/tools/perf/ui/hist.c
> @@ -529,7 +529,7 @@ void perf_hpp_list__prepend_sort_field(struct
> perf_hpp_list *list,
>
> void perf_hpp__column_unregister(struct perf_hpp_fmt *format)
> {
> - list_del(>list);
> + list_del_init(>list);
> }
>
yes, this is an option. But for safety, I sugguest do not rely on list_del_init.
No rule rather than create one.
But anyway, both are ok for me. What's your options?
> void perf_hpp__cancel_cumulate(void)
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH] perf report: show sort_order in title
On Mon, Mar 13, 2017 at 11:57:18AM -0300, Arnaldo Carvalho de Melo wrote: > > I'll just rename this to use the tools/perf/ style for such functions, > > making it: > > > > static int hists__scnprintf_sort_fields(hists, buf, size) > > But then, while testing, > > Before: > > $ perf report > Samples: 405 of event 'cycles', Event count (approx.): 101733003 > Overhead Command Shared ObjectSymbol > 11.15% swapper [kernel.vmlinux] [k] > intel_idle > 3.00% firefox libxul.so[.] > 0x01298b8d > 1.74% swapper [kernel.vmlinux] [k] > update_blocked_averages > 1.69% qemu-system-x86 [kernel.vmlinux] [k] __fget > 1.18% swapper [kernel.vmlinux] [k] > update_wall_time > > Tip: Save output of perf stat using: perf stat record > > After: > > $ perf report > Samples: 405 of event 'cycles', Event count (approx.): 101733003, Sort by: > Children,Overhead,Command,Shared Object,Symbol > Overhead Command Shared ObjectSymbol > 11.15% swapper [kernel.vmlinux] [k] > intel_idle > 3.00% firefox libxul.so[.] > 0x01298b8d > 1.74% swapper [kernel.vmlinux] [k] > update_blocked_averages > 1.69% qemu-system-x86 [kernel.vmlinux] [k] __fget > 1.18% swapper [kernel.vmlinux] [k] > update_wall_time > > > I see now duplication of info, where is the value? Can you show the usecase in > a compelling way? > > - Arnaldo Thanks for trying. The key idea is to show how does the data sort, especially the first sort key. When I use some GUI based perf tool, I can see how my data is sorted by checking the report header status. I think this is a good for browser. You are right, the info is duplicated. I got another idea that we show a '↓' at the header string and only for the first sort key. What do you think? $ perf report Samples: 405 of event 'cycles', Event count (approx.): 101733003 ↓Overhead Command Shared ObjectSymbol 11.15% swapper [kernel.vmlinux] [k] intel_idle 3.00% firefox libxul.so[.] 0x01298b8d 1.74% swapper [kernel.vmlinux] [k] update_blocked_averages Another idea I want to add is to support dynamic sorting. For me, I use perf to analysing entire system performance, and the data is very large. Then sometimes it take as long as ~10 minitues to read perf data. So I think if we can change sort w/o reload data will be good. -- Thanks, Changbin Du signature.asc Description: PGP signature
Re: [PATCH v2 0/2] kconfig/mconf: propagate jumping function in search result view
Hello, any new update need for this or new comments? :) On Wed, Feb 08, 2017 at 11:09:37AM +0800, changbin...@intel.com wrote: > From: Changbin Du <changbin...@intel.com> > > While I am searching something in menuconfig, I hope if I can jump to > interested > items directly. I even try to add this feature. When I check the code just > found > it has already been there but not documented. So why let more developers know > it? > > v2: correct spell (Jim) > > Changbin Du (2): > kconfig/mconf: add jumping tip in title of search result textbox > Documentation/kconfig: add search jump feature description > > Documentation/kbuild/kconfig.txt | 4 > scripts/kconfig/mconf.c | 8 > 2 files changed, 8 insertions(+), 4 deletions(-) > > -- > 2.7.4 > -- Thanks, Changbin Du signature.asc Description: PGP signature
Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
On Tue, Apr 04, 2017 at 12:51:03PM -0300, Arnaldo Carvalho de Melo wrote: > Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu: > > Hi Arnaldo, > > > > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo > > <a...@kernel.org> wrote: > > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin...@intel.com escreveu: > > >> From: Changbin Du <changbin...@intel.com> > > >> > > >> Some perf_hpp_fmt both registered at field and sort list. For such > > >> instance, we only can free it when removed from the both lists. This > > >> function currently only used by self-test code, but still should fix > > >> it. > > > > > > Looks sane, applying, > > > > > > Jiri, Namhyung, please holler (or ack) if needed, > > > > Did you actually see the double free problem? AFAICS the old code > > I assumed that he had seen it, in some self-test code, Changbin, can you > please show command output or further describe when this patch would be > necessary? > Arnaldo, I did observe this issue but not in self-test code. The self-test code uses that function but does not have a case that a fmt linked to two both list. I found this issue when I try to add 'dynamic sort' feature to perf, which I use this function to reset out fields. Anyway, it is clear that this is a real bug, a potential issue need to fix. > - Arnaldo > > > removed a fmt from both list before free it. In the first loop, fmt that > > was linked to both output list and sort list will be remove. And the > > second loop frees fmt that was linked only to the sort list (IOW, it > > frees fmt that was not freed in the first loop). > > This is right. It is to handle the fmts that linked to both two lists. > > Thanks, > > Namhyung > > > > > > > > > > - Arnaldo > > > -- Thanks, Changbin Du signature.asc Description: PGP signature
Re: [PATCH v2] perf sort: only insert overhead && overhead_children when no overhead* field given
On Thu, Jun 29, 2017 at 05:22:52PM +0200, Jiri Olsa wrote: > > > --- a/tools/perf/util/sort.c > > > +++ b/tools/perf/util/sort.c > > > @@ -2635,6 +2635,9 @@ static char *setup_overhead(char *keys) > > > if (sort__mode == SORT_MODE__DIFF) > > > return keys; > > > > > > + if (strstr(keys, "overhead")) > > > + return keys; > > > + > > ah ok, I think it's right.. basically you're forcing > precedence of the -s option over the --children option > > you could have your example working by running: > $ perf report -s overhead,sym --no-children > Thanks for the knowledge. > please state something like: > > /* > * User already stated overhead within -s option, > * do not mangle with that. > */ > I'll add this as comment around the change for v3. > thanks, > jirka -- Thanks, Changbin Du signature.asc Description: PGP signature
Re: [PATCH v2] perf sort: only insert overhead && overhead_children when no overhead* field given
Hi, Jiri,
what is the status of this one? I didn't get a response of v2. thanks.
On Fri, Jun 02, 2017 at 12:22:00PM +0800, changbin...@intel.com wrote:
> From: Changbin Du <changbin...@intel.com>
>
> If we always insert 'overhead' and 'overhead_children' as sort keys,
> this make it impossible to sort as overhead (which displayed as Self)
> first.Ths will be a problem if the data is collected with call-graph
> enabled. Then we never can sort the result as self-overhead on this
> data. And sometimes the data is hard to collect.
>
> > perf record -ag
> > perf report -s overhead,sym
>
> Samples: 7K of event 'cycles', Event count (approx.): 865138253
> Children Self Symbol
> + 26.41% 0.00% [k] verify_cpu
> + 26.37% 0.04% [k] cpu_startup_entry
> + 25.93% 0.27% [k] do_idle
> + 19.88% 0.00% [k] start_secondary
>
>
> I intend to sort as 'Self', but actually it sort as 'Children'.
>
> This patch fix this by only insert overhead && overhead_children
> when no overhead* field given.
>
> Signed-off-by: Changbin Du <changbin...@intel.com>
> ---
> v2: Add the example in commit message.
>
> ---
> tools/perf/util/sort.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 5762ae4..69eea3a 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -2635,6 +2635,9 @@ static char *setup_overhead(char *keys)
> if (sort__mode == SORT_MODE__DIFF)
> return keys;
>
> + if (strstr(keys, "overhead"))
> + return keys;
> +
> keys = prefix_if_not_in("overhead", keys);
>
> if (symbol_conf.cumulate_callchain)
> --
> 2.7.4
>
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [Q] What about PCI mmio access alignment?
Thank you. I have some experiment on my PC. The result is: o I always can get expected value if the unaligned access doesn't across a DWORD boundary, like readw(bar0+1). I suspect that the chipset should read a whole DWORD in behind. This may trigger unexpected behaviour on device. o If read across DWORD boundary, I get some intersting value, like readl(bar0+2). For some device, I get a cyclic shifted value of the first DWORD, while some device return all FF. So my conclusion is that no unaligned access and at least access one DWORD size. On Sat, May 27, 2017 at 06:32:48PM +0300, Andy Shevchenko wrote: > On Thu, May 25, 2017 at 1:12 PM, Du, Changbin <changbin...@intel.com> wrote: > > I have a basic quesion about the alignment when access PCI bar mmio space. > > Is > > the address accessed must be DW aligned and count must be DW aligned? > > I guess the best answer is PCI architecture specification. > Book I have nearby tells me IIDnMS that yes, you have to follow alignment. > > > As far as I know, The address field of TLB ignore lower 2 bits and the unit > > of > > length field also is DW. So does it mean above question is Yes? Else will > > CPU > > handle unaligned access for mmio space? > > Here you perhaps meant the bus, not the CPU. PCI allows it as long as > actual device allows it. > > (I recall patch series that tries to micro optimize PCI config space > access by grouping some bytes into words or even dwords, and it was > rejected). > > > I want to know wether below access illegal or not: > > - readb(bar0) > > - readb(bar0 + 1) > > - readl(bar0) > > It depends. > > -- > With Best Regards, > Andy Shevchenko -- Thanks, Changbin Du signature.asc Description: PGP signature
[Q] What about PCI mmio access alignment?
Hi, guys, I have a basic quesion about the alignment when access PCI bar mmio space. Is the address accessed must be DW aligned and count must be DW aligned? As far as I know, The address field of TLB ignore lower 2 bits and the unit of length field also is DW. So does it mean above question is Yes? Else will CPU handle unaligned access for mmio space? I want to know wether below access illegal or not: - readb(bar0) - readb(bar0 + 1) - readl(bar0) Thanks, Changbin Du signature.asc Description: PGP signature
Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field
Hi jirka, Will you send a patch to fix this issue? If not I will send my
solution in a new thread.
I have given up to add 'dynamic sort' feature since my code is not working
and I am engaged in other things. I still hope this fix can be picked up.
Thanks!
On Wed, Apr 12, 2017 at 09:48:08AM +0800, Du, Changbin wrote:
> On Tue, Apr 11, 2017 at 12:32:49PM +0200, Jiri Olsa wrote:
> > On Tue, Apr 11, 2017 at 06:13:17PM +0800, Du, Changbin wrote:
> > > > > >
> > > > > yes, this is an option. But for safety, I sugguest do not rely on
> > > > > list_del_init.
> > > > > No rule rather than create one.
> > > > >
> > > > > But anyway, both are ok for me. What's your options?
> > > >
> > > > hum, also I dont think we need to touch that bit at all
> > > > if we are going to remove it right away.. how about the
> > > > change below?
> > > >
> > > > jirka
> > > >
> > > >
> > > > ---
> > > > diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
> > > > index 5d632dca672a..0ee7db43dd7d 100644
> > > > --- a/tools/perf/ui/hist.c
> > > > +++ b/tools/perf/ui/hist.c
> > > > @@ -613,15 +613,15 @@ void perf_hpp__reset_output_field(struct
> > > > perf_hpp_list *list)
> > > >
> > > > /* reset output fields */
> > > > perf_hpp_list__for_each_format_safe(list, fmt, tmp) {
> > > > - list_del_init(>list);
> > > > - list_del_init(>sort_list);
> > > > + list_del(>list);
> > > > + /* Remove the fmt from next loop processing. */
> > > > + list_del(>sort_list);
> > > > fmt_free(fmt);
> > > What if the fmt is not linked to sort_list? I see it is possible (please
> > > checking perf_hpp__setup_output_field()). I am not sure if we really has
> > > sunch case currently, just concern :)
> >
> > if it's not linked to sort_list, then sort_list is initialized
> > and list_del should do no harm
> >
> ok, then it's fine if you insist.
>
> > jirka
>
> --
> Thanks,
> Changbin Du
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH] perf sort: only insert overhead && overhead_children when no overhead* field given
On Thu, Jun 01, 2017 at 12:21:39PM +0200, Jiri Olsa wrote:
> On Thu, Jun 01, 2017 at 05:03:21PM +0800, changbin...@intel.com wrote:
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > index 5762ae4..69eea3a 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -2635,6 +2635,9 @@ static char *setup_overhead(char *keys)
> > if (sort__mode == SORT_MODE__DIFF)
> > return keys;
> >
> > + if (strstr(keys, "overhead"))
> > + return keys;
> > +
> > keys = prefix_if_not_in("overhead", keys);
>
> hum, you basicaly do what's at begining of prefix_if_not_in function:
>
> static char *prefix_if_not_in(const char *pre, char *str)
> {
> char *n;
>
> if (!str || strstr(str, pre))
> return str;
> ...
>
Thanks, will change it.
>
> could you please provide the example described in changelog?
>
Will add example cmdline there, Thanks.
> jirka
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [PATCH] perf sort: only insert overhead && overhead_children when no overhead* field given
On Fri, Jun 02, 2017 at 10:52:24AM +0800, Du, Changbin wrote:
> On Thu, Jun 01, 2017 at 12:21:39PM +0200, Jiri Olsa wrote:
> > On Thu, Jun 01, 2017 at 05:03:21PM +0800, changbin...@intel.com wrote:
> > > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > > index 5762ae4..69eea3a 100644
> > > --- a/tools/perf/util/sort.c
> > > +++ b/tools/perf/util/sort.c
> > > @@ -2635,6 +2635,9 @@ static char *setup_overhead(char *keys)
> > > if (sort__mode == SORT_MODE__DIFF)
> > > return keys;
> > >
> > > + if (strstr(keys, "overhead"))
> > > + return keys;
> > > +
> > > keys = prefix_if_not_in("overhead", keys);
> >
> > hum, you basicaly do what's at begining of prefix_if_not_in function:
> >
> > static char *prefix_if_not_in(const char *pre, char *str)
> > {
> > char *n;
> >
> > if (!str || strstr(str, pre))
> > return str;
> > ...
> >
> Thanks, will change it.
>
Misunderstood your words. This is not equal to prefix_if_not_in.
# perf record -ag
# perf report -s overhead,sym
Samples: 7K of event 'cycles', Event count (approx.): 865138253
Children Self Symbol
+ 26.41% 0.00% [k] verify_cpu
+ 26.37% 0.04% [k] cpu_startup_entry
+ 25.93% 0.27% [k] do_idle
+ 19.88% 0.00% [k] start_secondary
'Children' should not be here.
> >
> > could you please provide the example described in changelog?
> >
> Will add example cmdline there, Thanks.
>
> > jirka
>
> --
> Thanks,
> Changbin Du
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
Re: [RESEND][PATCH] kvm: x86: Do not handle MMIO request in fast_page_fault
On Wed, Sep 13, 2017 at 04:39:56PM +0200, Radim Krčmář wrote:
> 2017-09-05 18:37+0800, changbin...@intel.com:
> > From: Changbin Du <changbin...@intel.com>
> >
> > If it is a MMIO request, it should be handled by slow path. This patch
> > actually fixed below warning when mmu debug is enabled.
> >
> > WARNING: CPU: 5 PID: 2282 at arch/x86/kvm/mmu.c:226
> > fast_page_fault+0x41b/0x520
> > CPU: 5 PID: 2282 Comm: qemu-system-x86 Not tainted 4.13.0-rc6+ #34
> > task: 9b47f5286000 task.stack: b18d03b28000
> > RIP: 0010:fast_page_fault+0x41b/0x520
> > Call Trace:
> > tdp_page_fault+0xfb/0x290
> > kvm_mmu_page_fault+0x61/0x120
> > handle_ept_misconfig+0x1ba/0x1c0
> > vmx_handle_exit+0xb8/0xd70
> > ? kvm_arch_vcpu_ioctl_run+0x9b6/0x18e0
> > kvm_arch_vcpu_ioctl_run+0xa5a/0x18e0
> > ? kvm_arch_vcpu_load+0x62/0x230
> > kvm_vcpu_ioctl+0x340/0x6c0
> > ? kvm_vcpu_ioctl+0x340/0x6c0
> > ? lock_acquire+0xf5/0x1f0
> > do_vfs_ioctl+0xa2/0x670
> > ? __fget+0x107/0x200
> > SyS_ioctl+0x79/0x90
> > entry_SYSCALL_64_fastpath+0x23/0xc2
> >
> > Signed-off-by: Changbin Du <changbin...@intel.com>
> > ---
> > arch/x86/kvm/mmu.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index 9d3f275..63c3360 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -3180,6 +3180,9 @@ static bool fast_page_fault(struct kvm_vcpu *vcpu,
> > gva_t gva, int level,
> > iterator.level < level)
> > break;
> >
> > + if (is_mmio_spte(spte))
> > + break;
>
> Hm, handle_ept_misconfig() calls kvm_mmu_page_fault with error_code =
> PFERR_RSVD_MASK. This error_code gets propagated and checked at the
> beginning of page_fault_can_be_fast(), where it should break the
> function execution.
>
> How did the execution get all the way to the loop?
>
hmm, a recent Paolo's cleanup patch already fixed it.
e08d26f ("KVM: x86: simplify ept_misconfig")
In the past, PFERR_RSVD_MASK is not set.
So this patch doesnt need any more. thanks.
> Thanks.
--
Thanks,
Changbin Du
signature.asc
Description: PGP signature
