[RFC PATCH] [media] rc: filter out not allowed protocols when decoding

[Du, Changbin]

From: "Du, Changbin" 

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 
---
 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/

[PATCH] dynamic_debug: add wildcard support to filter files/functions/modules

[Du, Changbin]

From: "Du, Changbin" 

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" > /dynamic_debug/control
2.  open debug logs for usb xhci code
echo "file *xhci* +p" > /dynamic_debug/control

Signed-off-by: Du, Changbin 
---
 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/

[Resend PATCH v2] usb: gadget: s3c-hsotg: fix core reset timeout failure

[Du, Changbin]

From: "Du, Changbin" 

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 
---
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/

Re: [Resend PATCH] media: rc: ati_remote.c: code style and compile warning fixing

[Du Changbin]

> > 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 
> >*Added support for the "Lola" remote contributed by:
> >*Seth Cohn 
> > + *  Jul 2012: Du, Changbin 
> > + *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

[Du, Changbin]

From: "Du, Changbin" 

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 
---
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",
 dat

Re: Does perf-annotate work correctly?

[Du, Changbin]

On Mon, Oct 16, 2017 at 11:28:53AM +0200, Jiri Olsa wrote:
> On Fri, Oct 13, 2017 at 06:15:00PM +0800, Du, Changbin wrote:
> > Hi Jiri,
> > Sorry, missed you (but get_maintainer.pl doesn't list you). Here is ealier 
> > email.
> > https://lkml.org/lkml/2017/9/12/158
> > 
> > Do you think if this is a real issue?
> 
> SNIP
> 
> > >   0.02 │  test   %esi,%esi
> > >▒
> > >│↓ js 25   
> > >▒
> > >  99.98 │← retq
> > >▒
> > >│25:   push   %rbp 
> > >▒
> > >│  mov$0x440a,%ecx 
> > >▒
> > >│  mov$0x440c,%edx 
> > >▒
> > >│vmx_complete_interrupts():
> > >◆
> > >│break;
> > >▒
> > >│} 
> > >▒
> > >│} 
> > >▒
> > >│  
> > >▒
> > >│static void vmx_complete_interrupts(struct vcpu_vmx *vmx) 
> > >▒
> > >│{ 
> > >▒
> > >│  mov%rsp,%rbp
> > >▒
> > >│→ callq  __vmx_complete_interrupts.part.64
> > >▒
> > >│__vmx_complete_interrupts(&vmx->vcpu, 
> > > vmx->idt_vectoring_info,   ▒
> > >│  pop%rbp 
> > >▒
> > >│← retq
> > >▒
> 
> hi,
> there's 'o' key to togle the instruction address or you
> can use the perf annotate --stdio to get it.. should be
> easier to tell if that's the same instruction
>

Thanks for replying. I know the reason now, the instructions are shown in pc
address order, and some C statments are split into chunks. It gives me a
illusion. Thanks.

> jirka
> 

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

Re: Does perf-annotate work correctly?

[Du, Changbin]

On Mon, Oct 16, 2017 at 11:30:51AM +0200, Jiri Olsa wrote:
> On Fri, Oct 13, 2017 at 06:15:00PM +0800, Du, Changbin wrote:
> > Hi Jiri,
> > Sorry, missed you (but get_maintainer.pl doesn't list you). Here is ealier 
> > email.
> > https://lkml.org/lkml/2017/9/12/158
> > 
> > Do you think if this is a real issue?
> > 
> > btw, is their a dedicated mailist for perf? Thanks.
> 
> there's linux-perf-us...@vger.kernel.org for perf specific issues
> 

Good to know it. Can we add it to MAINTAINERS?

> jirka

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

Re: [PATCH 1/2] mm, thp: introduce dedicated transparent huge page allocation interfaces

[Du, Changbin]

Hi Khandual,
Thanks for your review.

On Tue, Oct 17, 2017 at 01:38:07PM +0530, Anshuman Khandual wrote:
> On 10/16/2017 02:49 PM, changbin...@intel.com wrote:
> > From: Changbin Du 
> > 
> > This patch introduced 4 new interfaces to allocate a prepared
> > transparent huge page.
> >   - alloc_transhuge_page_vma
> >   - alloc_transhuge_page_nodemask
> >   - alloc_transhuge_page_node
> >   - alloc_transhuge_page
> > 
> 
> If we are trying to match HugeTLB helpers, then it should have
> format something like alloc_transhugepage_xxx instead of
> alloc_transhuge_page_XXX. But I think its okay.
>
HugeTLB helpers are something like alloc_huge_page, so I think
alloc_transhuge_page match it. And existing names already have
*transhuge_page* style.

> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 14bc21c..1dd2c33 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -130,9 +130,20 @@ extern unsigned long thp_get_unmapped_area(struct file 
> > *filp,
> > unsigned long addr, unsigned long len, unsigned long pgoff,
> > unsigned long flags);
> >  
> > -extern void prep_transhuge_page(struct page *page);
> >  extern void free_transhuge_page(struct page *page);
> >  
> > +struct page *alloc_transhuge_page_vma(gfp_t gfp_mask,
> > +   struct vm_area_struct *vma, unsigned long addr);
> > +struct page *alloc_transhuge_page_nodemask(gfp_t gfp_mask,
> > +   int preferred_nid, nodemask_t *nmask);
> 
> Would not they require 'extern' here ?
>
Need or not, function declaration are implicitly 'extern'. I will add it to
align with existing code.

> > +
> > +static inline struct page *alloc_transhuge_page_node(int nid, gfp_t 
> > gfp_mask)
> > +{
> > +   return alloc_transhuge_page_nodemask(gfp_mask, nid, NULL);
> > +}
> > +
> > +struct page *alloc_transhuge_page(gfp_t gfp_mask);
> > +
> >  bool can_split_huge_page(struct page *page, int *pextra_pins);
> >  int split_huge_page_to_list(struct page *page, struct list_head *list);
> >  static inline int split_huge_page(struct page *page)
> > diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> > index 643c7ae..70a00f3 100644
> > --- a/include/linux/migrate.h
> > +++ b/include/linux/migrate.h
> > @@ -42,19 +42,15 @@ static inline struct page *new_page_nodemask(struct 
> > page *page,
> > return 
> > alloc_huge_page_nodemask(page_hstate(compound_head(page)),
> > preferred_nid, nodemask);
> >  
> > -   if (thp_migration_supported() && PageTransHuge(page)) {
> > -   order = HPAGE_PMD_ORDER;
> > -   gfp_mask |= GFP_TRANSHUGE;
> > -   }
> > -
> > if (PageHighMem(page) || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
> > gfp_mask |= __GFP_HIGHMEM;
> >  
> > -   new_page = __alloc_pages_nodemask(gfp_mask, order,
> > +   if (thp_migration_supported() && PageTransHuge(page))
> > +   return alloc_transhuge_page_nodemask(gfp_mask | GFP_TRANSHUGE,
> > +   preferred_nid, nodemask);
> > +   else
> > +   return __alloc_pages_nodemask(gfp_mask, order,
> > preferred_nid, nodemask);
> > -
> > -   if (new_page && PageTransHuge(page))
> > -   prep_transhuge_page(new_page);
> 
> This makes sense, calling prep_transhuge_page() inside the
> function alloc_transhuge_page_nodemask() is better I guess.
> 
> >  
> > return new_page;
> >  }
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 269b5df..e267488 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -490,7 +490,7 @@ static inline struct list_head 
> > *page_deferred_list(struct page *page)
> > return (struct list_head *)&page[2].mapping;
> >  }
> >  
> > -void prep_transhuge_page(struct page *page)
> > +static void prep_transhuge_page(struct page *page)
> 
> Right. It wont be used outside huge page allocation context and
> you have already mentioned about it.
> 
> >  {
> > /*
> >  * we use page->mapping and page->indexlru in second tail page
> > @@ -501,6 +501,45 @@ void prep_transhuge_page(struct page *page)
> > set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
> >  }
> >  
> > +struct page *alloc_transhuge_page_vma(gfp_t gfp_mask,
> > +   struct vm_area_struct *vma, unsigned long addr)
> > +{
> > +   struct page *page;
> > +
> > +   page = alloc_pages_vma(gfp_mask | __GFP_COMP, HPAGE_PMD_ORDER,
> > +  vma, addr, numa_node_id(), true);
> > +   if (unlikely(!page))
> > +   return NULL;
> > +   prep_transhuge_page(page);
> > +   return page;
> > +}
> 
> __GFP_COMP and HPAGE_PMD_ORDER are the minimum flags which will be used
> for huge page allocation and preparation. Any thing else depending upon
> the context will be passed by the caller. Makes sense.
> 
yes, thanks.

> > +
> > +struct page *alloc_transhuge_page_nodemask(gfp_t gfp_mask,
> > +   int preferred_nid, nodemask_t *nmask)
> > +{
> > +   struct page *

Re: [PATCH 2/2] mm: rename page dtor functions to {compound,huge,transhuge}_page__dtor

[Du, Changbin]

Hi Khandual,
> > long freed);
> >  bool isolate_huge_page(struct page *page, struct list_head *list);
> >  void putback_active_hugepage(struct page *page);
> > -void free_huge_page(struct page *page);
> > +void huge_page_dtor(struct page *page);
> >  void hugetlb_fix_reserve_counts(struct inode *inode);
> >  extern struct mutex *hugetlb_fault_mutex_table;
> >  u32 hugetlb_fault_mutex_hash(struct hstate *h, struct mm_struct *mm,
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 065d99d..adfa906 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -616,7 +616,7 @@ void split_page(struct page *page, unsigned int order);
> >   * prototype for that function and accessor functions.
> >   * These are _only_ valid on the head of a compound page.
> >   */
> > -typedef void compound_page_dtor(struct page *);
> > +typedef void compound_page_dtor_t(struct page *);
> 
> Why changing this ? I understand _t kind of specifies it more
> like a type def but this patch is just to rename the compound
> page destructor functions. Not sure we should change datatype
> here as well in this patch.
>
It is because of name conflict. I think you already get it per below comments.
I will describe it in commit message.

> >  
> >  /* Keep the enum in sync with compound_page_dtors array in mm/page_alloc.c 
> > */
> >  enum compound_dtor_id {
> > @@ -630,7 +630,7 @@ enum compound_dtor_id {
> >  #endif
> > NR_COMPOUND_DTORS,
> >  };
> > -extern compound_page_dtor * const compound_page_dtors[];
> > +extern compound_page_dtor_t * const compound_page_dtors[];
> >  
> >  static inline void set_compound_page_dtor(struct page *page,
> > enum compound_dtor_id compound_dtor)
> > @@ -639,7 +639,7 @@ static inline void set_compound_page_dtor(struct page 
> > *page,
> > page[1].compound_dtor = compound_dtor;
> >  }
> >  
> > -static inline compound_page_dtor *get_compound_page_dtor(struct page *page)
> > +static inline compound_page_dtor_t *get_compound_page_dtor(struct page 
> > *page)
> 
> Which is adding these kind of changes to the patch without
> having a corresponding description in the commit message.
> 
> >  {
> > VM_BUG_ON_PAGE(page[1].compound_dtor >= NR_COMPOUND_DTORS, page);
> > return compound_page_dtors[page[1].compound_dtor];
> > @@ -657,7 +657,7 @@ static inline void set_compound_order(struct page 
> > *page, unsigned int order)
> > page[1].compound_order = order;
> >  }
> >  
> > -void free_compound_page(struct page *page);
> > +void compound_page_dtor(struct page *page);
> >  
> >  #ifdef CONFIG_MMU
> >  /*
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index e267488..a01125b 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2717,7 +2717,7 @@ fail: if (mapping)
> > return ret;
> >  }
> >  
> > -void free_transhuge_page(struct page *page)
> > +void transhuge_page_dtor(struct page *page)
> >  {
> > struct pglist_data *pgdata = NODE_DATA(page_to_nid(page));
> > unsigned long flags;
> > @@ -2728,7 +2728,7 @@ void free_transhuge_page(struct page *page)
> > list_del(page_deferred_list(page));
> > }
> > spin_unlock_irqrestore(&pgdata->split_queue_lock, flags);
> > -   free_compound_page(page);
> > +   compound_page_dtor(page);
> >  }
> >  
> >  void deferred_split_huge_page(struct page *page)
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 424b0ef..1af2c4e7 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1250,7 +1250,7 @@ static void clear_page_huge_active(struct page *page)
> > ClearPagePrivate(&page[1]);
> >  }
> >  
> > -void free_huge_page(struct page *page)
> > +void huge_page_dtor(struct page *page)
> >  {
> > /*
> >  * Can't pass hstate in here because it is called from the
> > @@ -1363,7 +1363,7 @@ int PageHeadHuge(struct page *page_head)
> > if (!PageHead(page_head))
> > return 0;
> >  
> > -   return get_compound_page_dtor(page_head) == free_huge_page;
> > +   return get_compound_page_dtor(page_head) == huge_page_dtor;
> >  }
> >  
> >  pgoff_t __basepage_index(struct page *page)
> > @@ -1932,11 +1932,11 @@ static long vma_add_reservation(struct hstate *h,
> >   * specific error paths, a huge page was allocated (via alloc_huge_page)
> >   * and is about to be freed.  If a reservation for the page existed,
> >   * alloc_huge_page would have consumed the reservation and set PagePrivate
> > - * in the newly allocated page.  When the page is freed via free_huge_page,
> > + * in the newly allocated page.  When the page is freed via huge_page_dtor,
> >   * the global reservation count will be incremented if PagePrivate is set.
> > - * However, free_huge_page can not adjust the reserve map.  Adjust the
> > + * However, huge_page_dtor can not adjust the reserve map.  Adjust the
> >   * reserve map here to be consistent with global reserve count adjustments
> > - * to be made by free_huge_page.
> > + * to be made

Re: [Q] What about PCI mmio access alignment?

[Du, Changbin]

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  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

Re: [PATCH v2] tracing: Allocate mask_str buffer dynamically

[Du, Changbin]

Hi Rostedt,
On Tue, Oct 31, 2017 at 12:19:58PM -0400, Steven Rostedt wrote:
> On Thu, 26 Oct 2017 00:20:28 +0800
> changbin...@intel.com wrote:
> 
> > From: Changbin Du 
> > 
> > The default NR_CPUS can be very large, but actual possible nr_cpu_ids
> > usually is very small. For my x86 distribution, the NR_CPUS is 8192 and
> > nr_cpu_ids is 4. About 2 pages are wasted.
> > 
> > Most machines don't have so many CPUs, so define a array with NR_CPUS
> > just wastes memory. So let's allocate the buffer dynamically when need.
> > 
> > The exact buffer size should be:
> >   DIV_ROUND_UP(nr_cpu_ids, 4) + nr_cpu_ids/32 + 2;
> > 
> > Example output:
> >   ff,
> > 
> > Signed-off-by: Changbin Du 
> > 
> > ---
> > v2:
> >   - remove 'static' declaration.
> >   - fix buffer size.
> > ---
> >  kernel/trace/trace.c | 18 ++
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 752e5da..6b70648 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -4184,31 +4184,33 @@ static const struct file_operations 
> > show_traces_fops = {
> >   */
> >  static DEFINE_MUTEX(tracing_cpumask_update_lock);
> 
> The above mutex was used to protect mask_str.
> 
> >  
> > -/*
> > - * Temporary storage for the character representation of the
> > - * CPU bitmask (and one more byte for the newline):
> > - */
> > -static char mask_str[NR_CPUS + 1];
> > -
> >  static ssize_t
> >  tracing_cpumask_read(struct file *filp, char __user *ubuf,
> >  size_t count, loff_t *ppos)
> >  {
> > struct trace_array *tr = file_inode(filp)->i_private;
> > +   char *mask_str;
> > int len;
> >  
> > +   /* Bitmap, ',' and two more bytes for the newline and '\0'. */
> > +   len = DIV_ROUND_UP(nr_cpu_ids, 4) + nr_cpu_ids/32 + 2;
> > +   mask_str = kmalloc(len, GFP_KERNEL);
> > +   if (!mask_str)
> > +   return -ENOMEM;
> > +
> > mutex_lock(&tracing_cpumask_update_lock);
> 
> This patch can remove the mutex as well, since there's no sharing of
> the mask anymore.
> 
> -- Steve
>
ok, let me remove it in v3.

> >  
> > -   len = snprintf(mask_str, count, "%*pb\n",
> > +   len = snprintf(mask_str, len, "%*pb\n",
> >cpumask_pr_args(tr->tracing_cpumask));
> > if (len >= count) {
> > count = -EINVAL;
> > goto out_err;
> > }
> > -   count = simple_read_from_buffer(ubuf, count, ppos, mask_str, NR_CPUS+1);
> > +   count = simple_read_from_buffer(ubuf, count, ppos, mask_str, len);
> >  
> >  out_err:
> > mutex_unlock(&tracing_cpumask_update_lock);
> > +   kfree(mask_str);
> >  
> > return count;
> >  }
> 

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

Re: [PATCH] x86, build: Improve the isolinux searching of isoimage generation

[Du, Changbin]

Hi Ingo and Yamada,
Thanks for your suggestions. I'll have a try though I am not familiar with 
kbuild system.

On Wed, Nov 01, 2017 at 12:17:50PM +0900, Masahiro Yamada wrote:
> 2017-10-31 18:39 GMT+09:00 Ingo Molnar :
> >
> > * changbin...@intel.com  wrote:
> >
> >> From: Changbin Du 
> >>
> >> Recently I failed to build isoimage target, because the path of 
> >> isolinux.bin
> >> changed to /usr/xxx/ISOLINUX/isolinux.bin, as well as ldlinux.c32 which
> >> changed to /usr/xxx/syslinux/modules/bios/ldlinux.c32.
> >>
> >> This patch has a improvement of the file search:
> >>   - Don't print the raw shell commands. It doesn't make sense to show the
> >> entire big block.
> >>   - Show a error message instead of silent fail.
> >>   - Add above new paths.
> >>
> >> Now it becomes:
> >> Kernel: arch/x86/boot/bzImage is ready  (#62)
> >> rm -rf arch/x86/boot/isoimage
> >> mkdir arch/x86/boot/isoimage
> >> Using /usr/lib/ISOLINUX/isolinux.bin
> >> Using /usr/lib/syslinux/modules/bios/ldlinux.c32
> >> cp arch/x86/boot/bzImage arch/x86/boot/isoimage/linux
> >> ...
> >>
> >> Before:
> >> Kernel: arch/x86/boot/bzImage is ready  (#63)
> >> rm -rf arch/x86/boot/isoimage
> >> mkdir arch/x86/boot/isoimage
> >> for i in lib lib64 share end ; do \
> >>   if [ -f /usr/$i/syslinux/isolinux.bin ] ; then \
> >>   cp /usr/$i/syslinux/isolinux.bin arch/x86/boot/isoimage ; \
> >>   if [ -f /usr/$i/syslinux/ldlinux.c32 ]; then \
> >>   cp /usr/$i/syslinux/ldlinux.c32 
> >> arch/x86/boot/isoimage ; \
> >>   fi ; \
> >>   break ; \
> >>   fi ; \
> >>   if [ $i = end ] ; then exit 1 ; fi ; \
> >> done
> >> arch/x86/boot/Makefile:161: recipe for target 'isoimage' failed
> >> make[1]: *** [isoimage] Error 1
> >
> > I like these changes. Could we please further improve it: for example the 
> > boot
> > image build messages are still pretty unstructured, while regular build 
> > system
> > messages come in the following format:
> >
> >   CC  arch/x86/events/msr.o
> >   RELOCS  arch/x86/realmode/rm/realmode.relocs
> >   OBJCOPY arch/x86/realmode/rm/realmode.bin
> >   CC  arch/x86/kernel/signal.o
> >   AS  arch/x86/realmode/rmpiggy.o
> >   CC  ipc/msg.o
> >   AR  arch/x86/ia32/built-in.o
> >   CC  arch/x86/events/amd/iommu.o
> >   CC  init/do_mounts.o
> >   AR  arch/x86/realmode/built-in.o
> >
> > So instead of:
> >
> >> Kernel: arch/x86/boot/bzImage is ready  (#62)
> >> rm -rf arch/x86/boot/isoimage
> >> mkdir arch/x86/boot/isoimage
> >> Using /usr/lib/ISOLINUX/isolinux.bin
> >> Using /usr/lib/syslinux/modules/bios/ldlinux.c32
> >> cp arch/x86/boot/bzImage arch/x86/boot/isoimage/linux
> >
> > Could we make it something more streamlined and similar to the rest of the 
> > build
> > as well, like:
> >
> >   GEN arch/x86/boot/bzImage
> >   GEN arch/x86/boot/isoimage
> >   GEN arch/x86/boot/isoimage/linux
> >
> > I.e. only mention the new files built, with an appropriate prefix.
> >
> > I've Cc:-ed the kbuild maintainers, maybe they have a better suggestion 
> > instead of
> > the 'GEN' abbreviation?
> >
> 
> Generally, the abbreviation is the tool that has processed the target,
> but if you do not find an appropriate one, 'GEN' is fine.
> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

Re: [PATCH] tracing: Allocate mask_str buffer dynamically

[Du, Changbin]

On Wed, Oct 25, 2017 at 07:24:36PM +0800, changbin...@intel.com wrote:
> From: Changbin Du 
> 
> The default NR_CPUS can be very large, but actual possible nr_cpu_ids
> usually is very small. For my x86 distribution, the NR_CPUS is 8192 and
> nr_cpu_ids is 4. About 2 pages are wasted.
> 
> Most machines don't have so many CPUs, so define a array with NR_CPUS
> just wastes memory. So let's allocate the buffer dynamically when need.
> 
> Signed-off-by: Changbin Du 
> ---
>  kernel/trace/trace.c | 15 ---
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 752e5da..d1b3f11 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4184,19 +4184,18 @@ static const struct file_operations show_traces_fops 
> = {
>   */
>  static DEFINE_MUTEX(tracing_cpumask_update_lock);
>  
> -/*
> - * Temporary storage for the character representation of the
> - * CPU bitmask (and one more byte for the newline):
> - */
> -static char mask_str[NR_CPUS + 1];
> -
>  static ssize_t
>  tracing_cpumask_read(struct file *filp, char __user *ubuf,
>size_t count, loff_t *ppos)
>  {
>   struct trace_array *tr = file_inode(filp)->i_private;
> + static char *mask_str;
ah, need remove 'static'.

>   int len;
>  
> + mask_str = kmalloc(nr_cpu_ids + 1, GFP_KERNEL);
> + if (!mask_str)
> + return -ENOMEM;
> +
>   mutex_lock(&tracing_cpumask_update_lock);
>  
>   len = snprintf(mask_str, count, "%*pb\n",
> @@ -4205,10 +4204,12 @@ tracing_cpumask_read(struct file *filp, char __user 
> *ubuf,
>   count = -EINVAL;
>   goto out_err;
>   }
> - count = simple_read_from_buffer(ubuf, count, ppos, mask_str, NR_CPUS+1);
> + count = simple_read_from_buffer(ubuf, count, ppos,
> + mask_str, nr_cpu_ids+1);
>  
>  out_err:
>   mutex_unlock(&tracing_cpumask_update_lock);
> + kfree(mask_str);
>  
>   return count;
>  }
> -- 
> 2.7.4
> 

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

Re: [PATCH 1/4] x86, build: Fact out fdimage/isoimage generation commands to standalone script

[Du, Changbin]

On Sun, Nov 05, 2017 at 10:32:08AM +0100, Ingo Molnar wrote:
> 
> A few spelling fixes:
> 
> in the title:
> 
> s/Fact out
>  /Factor out
> 
> * changbin...@intel.com  wrote:
> 
> > From: Changbin Du 
> > 
> > The build message for fdimage/isoimage are pretty unstructured. The raw
> > shell command blocks are printed. We can improve them as regular build
> > system messages. Besides, writing commands in shell script is much more
> > easier than in a Makefile.
> 
> s/much more easier
>  /much more easy
> 
> > 
> > See Ingo's suggestion here https://lkml.org/lkml/2017/10/31/124.
> > 
> > This patch fact out the commands used for fdimage/isoimage generation from
> > arch/x86/boot/Makefile to new script arch/x86/boot/genimage.sh. Then add a
> > new kbuild command 'genimage' which invokes the new script. All
> > fdimages/isoimage now is generated by call to 'genimage' with different
> > parameters.
> 
> s/fact out
>  /factors out
> 
> s/to new script
>   to a new script
> 
> s/Then add
>  /Then it adds
> 
> s/a new kbuild command 'genimage'
>  /the new 'genimage' kbuild command
> 
> s/All fdimages/isoimage now is generated by call to
>  /All fdimage/isoimage files are now generated by a call to
> 
Sorry for these grammar errors. I alwyas forgot to write complete sentences in
English. :)

> > +#   $3 - kernel bzImage file
> > +#   $4 - mtool configuration file
> > +#   $5 - kernel cmdline
> > +#   $6 - inird image file
> > +#
> 
> The new script is much easier to read!
> 
> Thanks,
> 
>   Ingo

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

Re: [PATCH 2/4] x86, build: Add new paths for isolinux.bin and ldlinux.c32

[Du, Changbin]

Hi Ingo,
On Sun, Nov 05, 2017 at 10:33:53AM +0100, Ingo Molnar wrote:
> 
> * changbin...@intel.com  wrote:
> 
> > From: Changbin Du 
> > 
> > Recently I failed to build isoimage target, because the path of isolinux.bin
> > changed to /usr/xxx/ISOLINUX/isolinux.bin, as well as ldlinux.c32 which
> > changed to /usr/xxx/syslinux/modules/bios/ldlinux.c32.
> > 
> > This patch has a improvement of the file search:
> >   - Show a error message instead of silent fail.
> >   - Add above new paths.
> 
> How about:
> 
>   This patch improves the file search logic:
> - Show an error message instead of failing silently
> - Add the new paths listed above.
> 
> 
> > +   if [ $i = end -a -z "$isolinux" ] ; then
> > +   echo 'Need isolinux.bin, please install 
> > syslinux/isolinux'
> 
> How about:
> 
>   echo 'Need an isolinux.bin file, please install 
> syslinux/isolinux'
>
The new description is much better, will update. Thanks.

> Thanks,
> 
>   Ingo

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

Re: [PATCH 1/2] mm, thp: introduce dedicated transparent huge page allocation interfaces

[Du, Changbin]

Hi Hocko,

On Tue, Oct 17, 2017 at 12:20:52PM +0200, Michal Hocko wrote:
> [CC Kirill]
> 
> On Mon 16-10-17 17:19:16, changbin...@intel.com wrote:
> > From: Changbin Du 
> > 
> > This patch introduced 4 new interfaces to allocate a prepared
> > transparent huge page.
> >   - alloc_transhuge_page_vma
> >   - alloc_transhuge_page_nodemask
> >   - alloc_transhuge_page_node
> >   - alloc_transhuge_page
> > 
> > The aim is to remove duplicated code and simplify transparent
> > huge page allocation. These are similar to alloc_hugepage_xxx
> > which are for hugetlbfs pages. This patch does below changes:
> >   - define alloc_transhuge_page_xxx interfaces
> >   - apply them to all existing code
> >   - declare prep_transhuge_page as static since no others use it
> >   - remove alloc_hugepage_vma definition since it no longer has users
> 
> So what exactly is the advantage of the new API? The diffstat doesn't
> sound very convincing to me.
>
The caller only need one step to allocate thp. Several LOCs removed for all the
caller side with this change. So it's little more convinent.

> > Signed-off-by: Changbin Du 
> > ---
> >  include/linux/gfp.h |  4 
> >  include/linux/huge_mm.h | 13 -
> >  include/linux/migrate.h | 14 +-
> >  mm/huge_memory.c| 50 
> > ++---
> >  mm/khugepaged.c | 11 ++-
> >  mm/mempolicy.c  | 10 +++---
> >  mm/migrate.c| 12 
> >  mm/shmem.c  |  6 ++
> >  8 files changed, 71 insertions(+), 49 deletions(-)
> > 
> > diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> > index f780718..855c72e 100644
> > --- a/include/linux/gfp.h
> > +++ b/include/linux/gfp.h
> > @@ -507,15 +507,11 @@ alloc_pages(gfp_t gfp_mask, unsigned int order)
> >  extern struct page *alloc_pages_vma(gfp_t gfp_mask, int order,
> > struct vm_area_struct *vma, unsigned long addr,
> > int node, bool hugepage);
> > -#define alloc_hugepage_vma(gfp_mask, vma, addr, order) \
> > -   alloc_pages_vma(gfp_mask, order, vma, addr, numa_node_id(), true)
> >  #else
> >  #define alloc_pages(gfp_mask, order) \
> > alloc_pages_node(numa_node_id(), gfp_mask, order)
> >  #define alloc_pages_vma(gfp_mask, order, vma, addr, node, false)\
> > alloc_pages(gfp_mask, order)
> > -#define alloc_hugepage_vma(gfp_mask, vma, addr, order) \
> > -   alloc_pages(gfp_mask, order)
> >  #endif
> >  #define alloc_page(gfp_mask) alloc_pages(gfp_mask, 0)
> >  #define alloc_page_vma(gfp_mask, vma, addr)\
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 14bc21c..1dd2c33 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -130,9 +130,20 @@ extern unsigned long thp_get_unmapped_area(struct file 
> > *filp,
> > unsigned long addr, unsigned long len, unsigned long pgoff,
> > unsigned long flags);
> >  
> > -extern void prep_transhuge_page(struct page *page);
> >  extern void free_transhuge_page(struct page *page);
> >  
> > +struct page *alloc_transhuge_page_vma(gfp_t gfp_mask,
> > +   struct vm_area_struct *vma, unsigned long addr);
> > +struct page *alloc_transhuge_page_nodemask(gfp_t gfp_mask,
> > +   int preferred_nid, nodemask_t *nmask);
> > +
> > +static inline struct page *alloc_transhuge_page_node(int nid, gfp_t 
> > gfp_mask)
> > +{
> > +   return alloc_transhuge_page_nodemask(gfp_mask, nid, NULL);
> > +}
> > +
> > +struct page *alloc_transhuge_page(gfp_t gfp_mask);
> > +
> >  bool can_split_huge_page(struct page *page, int *pextra_pins);
> >  int split_huge_page_to_list(struct page *page, struct list_head *list);
> >  static inline int split_huge_page(struct page *page)
> > diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> > index 643c7ae..70a00f3 100644
> > --- a/include/linux/migrate.h
> > +++ b/include/linux/migrate.h
> > @@ -42,19 +42,15 @@ static inline struct page *new_page_nodemask(struct 
> > page *page,
> > return 
> > alloc_huge_page_nodemask(page_hstate(compound_head(page)),
> > preferred_nid, nodemask);
> >  
> > -   if (thp_migration_supported() && PageTransHuge(page)) {
> > -   order = HPAGE_PMD_ORDER;
> > -   gfp_mask |= GFP_TRANSHUGE;
> > -   }
> > -
> > if (PageHighMem(page) || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
> > gfp_mask |= __GFP_HIGHMEM;
> >  
> > -   new_page = __alloc_pages_nodemask(gfp_mask, order,
> > +   if (thp_migration_supported() && PageTransHuge(page))
> > +   return alloc_transhuge_page_nodemask(gfp_mask | GFP_TRANSHUGE,
> > +   preferred_nid, nodemask);
> > +   else
> > +   return __alloc_pages_nodemask(gfp_mask, order,
> > preferred_nid, nodemask);
> > -
> > -   if (new_page && PageTransHuge(page))
> > -   prep_transhuge_pag

Re: [PATCH 1/2] mm, thp: introduce dedicated transparent huge page allocation interfaces

[Du, Changbin]

On Tue, Oct 17, 2017 at 02:12:46PM +0300, Kirill A. Shutemov wrote:
> On Mon, Oct 16, 2017 at 05:19:16PM +0800, changbin...@intel.com wrote:
> > @@ -501,6 +501,45 @@ void prep_transhuge_page(struct page *page)
> > set_compound_page_dtor(page, TRANSHUGE_PAGE_DTOR);
> >  }
> >  
> > +struct page *alloc_transhuge_page_vma(gfp_t gfp_mask,
> > +   struct vm_area_struct *vma, unsigned long addr)
> > +{
> > +   struct page *page;
> > +
> > +   page = alloc_pages_vma(gfp_mask | __GFP_COMP, HPAGE_PMD_ORDER,
> > +  vma, addr, numa_node_id(), true);
> > +   if (unlikely(!page))
> > +   return NULL;
> > +   prep_transhuge_page(page);
> > +   return page;
> > +}
> > +
> > +struct page *alloc_transhuge_page_nodemask(gfp_t gfp_mask,
> > +   int preferred_nid, nodemask_t *nmask)
> > +{
> > +   struct page *page;
> > +
> > +   page = __alloc_pages_nodemask(gfp_mask | __GFP_COMP, HPAGE_PMD_ORDER,
> > + preferred_nid, nmask);
> > +   if (unlikely(!page))
> > +   return NULL;
> > +   prep_transhuge_page(page);
> > +   return page;
> > +}
> > +
> > +struct page *alloc_transhuge_page(gfp_t gfp_mask)
> > +{
> > +   struct page *page;
> > +
> > +   VM_BUG_ON(!(gfp_mask & __GFP_COMP));
> 
> Why do you check for __GFP_COMP only in this helper?
> 
> > +   page = alloc_pages(gfp_mask | __GFP_COMP, HPAGE_PMD_ORDER);
> 
> And still apply __GFP_COMP anyway?
>
This is a mistake, will removed. Thanks.

> > +   if (unlikely(!page))
> > +   return NULL;
> > +   prep_transhuge_page(page);
> > +   return page;
> > +}
> > +
> >  unsigned long __thp_get_unmapped_area(struct file *filp, unsigned long len,
> > loff_t off, unsigned long flags, unsigned long size)
> >  {
> 
> -- 
>  Kirill A. Shutemov

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

Re: [PATCH 0/2] mm, thp: introduce dedicated transparent huge page allocation interfaces

[Du, Changbin]

Hi Morton,
On Tue, Oct 17, 2017 at 04:28:16PM -0700, Andrew Morton wrote:
> On Mon, 16 Oct 2017 17:19:15 +0800 changbin...@intel.com wrote:
> 
> > The first one introduce new interfaces, the second one kills naming 
> > confusion.
> > The aim is to remove duplicated code and simplify transparent huge page
> > allocation.
> 
> These introduce various allnoconfig build errors.
Thanks, I will fix and have more test.

-- 
Thanks,
Changbin Du


signature.asc
Description: PGP signature

[RESEND PATCH v2] usb/gadget: make composite gadget meet usb compliance for vbus draw

[Du, Changbin]

>From 08df419517694c4dd9ff328f5644b46a99c2999e Mon Sep 17 00:00:00 2001
From: "Du, Changbin" 
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 
---
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 -

RE: [RESEND PATCH v2] usb/gadget: make composite gadget meet usb compliance for vbus draw

[Du, Changbin]

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" 
> > 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 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Du, Changbin]

Hi Rostedt,
On Tue, Jan 09, 2018 at 11:19:36PM -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2018 11:18:23 +0800
> "Du, Changbin"  wrote:
> 
> > write(3, "abcdefg", 7)  
> > > 
> > > From my point of view, the above isn't done writing the function name
> > > yet and we SHOULD continue waiting for more input.
> > >   
> > hmm, thanks for the background. Your above case is a postive use case. So by
> > this design, instead of write(3, "abcdefg", 7), it should be
> > write(3, "abcdefg\0", 8), right?
> 
> BTW, gcc would translate the above string to 'abcdefg\0\0'. When
> defining strings with "", gcc (and all C compilers) append a '\0' to
> the end.
> 
I should clarify the expression here first. :) All the strings here is to 
express
all the content of a string buffer, including the compiler appended '\0'. (Just 
like
the output of 'strace').
If this description is still not clear, please let me know!

> But I replied to the first patch, saying that allowing \0 as whitespace
> may be OK, given the usecase I showed.
> 
> > 
> > If true, it means kernel expect userspace write every string terminated with
> > '\0'. So to fix this issue:
> > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > write(3, " \0", 2)  = -1 EINVAL (Invalid argument)
> > 
> > Fix would be:
> > write(3, "\0", 1)?
> > 
> > So far, I am still confused. Some of the tracing debugfs entry accept '\0'
> > while some not. AFIK, 'echo xxx > ' always has a '\0'
> > terminated.
> 
> I don't believe that's true.
> 
>  $ echo -n abc > /tmp/abc
>  $ wc /tmp/abc
>  0 1 3 /tmp/abc
> 
> Echo writes only the characters you put on the line, nothing more.
> 
Sorry, I misundertood it. The extra character is '\n'.
  $ echo abc > set_ftrace_filter
0.000 probe:ftrace_filter_write_line0:(a7b8db80) ubuf=0xc77408 
cnt=0x4)
  $ echo -n abc > set_ftrace_filter
8889.832 probe:ftrace_filter_write_line0:(a7b8db80) ubuf=0xc77408 
cnt=0x3)

> Note, when the file descriptor is closed, the code also executes on
> what was written but not terminated. That is:
> 
>   write(fd, "abc", 3);
>   close(fd);
> 
> Will keep the "abc" in the continue buffer, but the closing of the file
> descriptor will flush it, and execute it.
> 
Thanks, so now I unstand why below corner case. The userspace try to set the
filter with a unrecognized symbole name (e.g "abcdefg").
open("/sys/kernel/debug/tracing/set_ftrace_filter", O_WRONLY|O_TRUNC) = 3
write(3, "abcdefg", 7)

Since "abcdefg" is not in the symbole list, so we would expect the write return
-EINVAL, right? As below:
# echo abcdefg > set_ftrace_filter
bash: echo: write error: Invalid argument

But the above mechanism hide the error. It return success actually no filter is
apllied at all.
# echo -n abcdefg > set_ftrace_filter

I think in this case kernel may request the userspace append a '\0' or space to 
the
string buffer so everything can work.

Also there is another corner case. Below write dosn't work.
open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
write(3, " \0", 2)  = -1 EINVAL (Invalid argument)

While these works:
# echo "" > set_ftrace_pid
# echo " " > set_ftrace_pid
# echo -n " " > set_ftrace_pid

These is the reason why I think '\0' should be recognized by the parser.

> -- Steve

-- 
Thanks,
Changbin Du

Re: [PATCH] perf ftrace: Fix the buffer size in __write_tracing_file

[Du, Changbin]

Hi Olsa,
What about this fix now? Thanks!

On Tue, Dec 26, 2017 at 05:26:56PM +0800, changbin...@intel.com wrote:
> From: Changbin Du 
> 
> The terminal character '\0' should take into account as size of the string
> buffer. Without this fix, the '--graph-funcs', '--nograph-funcs' and
> '--trace-funcs' options didn't work as expected when the  doesn't
> exist.
> 
> I didn't dive into kernel ftrace fops, but strace shows that if usersapce
> writes a non-terminated string, the kernel side will return success but
> no filter applied. After this fix in userspace, the kernel will return an
> error.
> 
> $ sudo ./perf ftrace -a --graph-depth 1 --graph-funcs abcdefg
>  0)   0.140 us|  rcu_all_qs();
>  3)   0.304 us|  mutex_unlock();
>  0)   0.153 us|  find_vma();
>  3)   0.088 us|  __fsnotify_parent();
>  0)   6.145 us|  handle_mm_fault();
>  3)   0.089 us|  fsnotify();
>  3)   0.161 us|  __sb_end_write();
>  3)   0.710 us|  SyS_close();
>  3)   7.848 us|  exit_to_usermode_loop();
> 
> On above example, I specified function filter 'abcdefg' but all functions
> are enabled.
> 
> Signed-off-by: Changbin Du 
> ---
>  tools/perf/builtin-ftrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index 25a42ac..2604a64 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> @@ -69,7 +69,7 @@ static int __write_tracing_file(const char *name, const 
> char *val, bool append)
>  {
>   char *file;
>   int fd, ret = -1;
> - ssize_t size = strlen(val);
> + ssize_t size = strlen(val) + 1;
>   int flags = O_WRONLY;
>   char errbuf[512];
>  
> -- 
> 2.7.4
> 

-- 
Thanks,
Changbin Du

Re: [PATCH v3 0/3] tracing: Fix the parser when processing strings w/ or w/o terminated '\0'

[Du, Changbin]

On Tue, Jan 16, 2018 at 12:42:26PM -0500, Steven Rostedt wrote:
> On Tue, 16 Jan 2018 17:02:27 +0800
> changbin...@intel.com wrote:
> 
> > From: Changbin Du 
> > 
> > I found there are some problems in the tracing parser when I investiage the 
> > root
> > cause of issues mentioned in below patch.
> > https://patchwork.kernel.org/patch/10132953/
> 
> I pulled in your patches and tweaked the change logs of the other two
> patches as well. You can see my temporary git tree here, but it may
> rebase.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> 
>  ftrace/core
> 
> -- Steve
Got it. Thank you!

Hi Olsa, so the perf patch 'perf ftrace: Fix the buffer size 
in__write_tracing_file'
is still needed. I will resend you at appropriate time.

-- 
Thanks,
Changbin Du

Re: [PATCH v3 0/3] tracing: Fix the parser when processing strings w/ or w/o terminated '\0'

[Du, Changbin]

On Wed, Jan 17, 2018 at 02:45:24PM +0900, Namhyung Kim wrote:
> Hello,
> 
> On Wed, Jan 17, 2018 at 12:54:34PM +0800, Du, Changbin wrote:
> > On Tue, Jan 16, 2018 at 12:42:26PM -0500, Steven Rostedt wrote:
> > > On Tue, 16 Jan 2018 17:02:27 +0800
> > > changbin...@intel.com wrote:
> > > 
> > > > From: Changbin Du 
> > > > 
> > > > I found there are some problems in the tracing parser when I investiage 
> > > > the root
> > > > cause of issues mentioned in below patch.
> > > > https://patchwork.kernel.org/patch/10132953/
> > > 
> > > I pulled in your patches and tweaked the change logs of the other two
> > > patches as well. You can see my temporary git tree here, but it may
> > > rebase.
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> > > 
> > >  ftrace/core
> > > 
> > > -- Steve
> > Got it. Thank you!
> > 
> > Hi Olsa, so the perf patch 'perf ftrace: Fix the buffer size 
> > in__write_tracing_file'
> > is still needed. I will resend you at appropriate time.
> 
> But it will work on the future kernels only, right?  For tools to be
> compatible with old kernels, you'd better writing a whitespace after
> the function name IMHO.
> 
Yes, it needs to write a space if it doesn't want possible error hidden.

> Thanks,
> Namhyung

-- 
Thanks,
Changbin Du

Re: [PATCH] perf ftrace: Fix the buffer size in __write_tracing_file

[Du, Changbin]

On Mon, Jan 08, 2018 at 03:34:57PM +0100, Jiri Olsa wrote:
> On Mon, Jan 08, 2018 at 11:05:12AM +0800, Du, Changbin wrote:
> > Hi Olsa,
> > What about this fix now? Thanks!
> > 
> > On Tue, Dec 26, 2017 at 05:26:56PM +0800, changbin...@intel.com wrote:
> > > From: Changbin Du 
> > > 
> > > The terminal character '\0' should take into account as size of the string
> > > buffer. Without this fix, the '--graph-funcs', '--nograph-funcs' and
> > > '--trace-funcs' options didn't work as expected when the  doesn't
> > > exist.
> > > 
> > > I didn't dive into kernel ftrace fops, but strace shows that if usersapce
> > > writes a non-terminated string, the kernel side will return success but
> > > no filter applied. After this fix in userspace, the kernel will return an
> > > error.
> > > 
> > > $ sudo ./perf ftrace -a --graph-depth 1 --graph-funcs abcdefg
> > >  0)   0.140 us|  rcu_all_qs();
> > >  3)   0.304 us|  mutex_unlock();
> > >  0)   0.153 us|  find_vma();
> > >  3)   0.088 us|  __fsnotify_parent();
> > >  0)   6.145 us|  handle_mm_fault();
> > >  3)   0.089 us|  fsnotify();
> > >  3)   0.161 us|  __sb_end_write();
> > >  3)   0.710 us|  SyS_close();
> > >  3)   7.848 us|  exit_to_usermode_loop();
> > > 
> > > On above example, I specified function filter 'abcdefg' but all functions
> > > are enabled.
> 
> hum, haven't checked, but looks like the filter is not working at all now:
> 
> [root@krava perf]# ./perf ftrace -vv -a --graph-depth 1 --graph-funcs 
> proc_sys_read
> write ' ' to tracing/set_ftrace_pid failed: Invalid argument
> [root@krava perf]# ./perf ftrace -vv -a --graph-depth 1 --graph-funcs SyS_read
> write ' ' to tracing/set_ftrace_pid failed: Invalid argument
> [root@krava perf]# ./perf ftrace -vv -a --graph-depth 1 --graph-funcs fsnotify
> write ' ' to tracing/set_ftrace_pid failed: Invalid argument
>
Thanks for your test. I forgot to test normal case and thought the err is 
expected...

This time I dived into kernel side, and found 3 issues (if I am all right) at 
the
kernel function trace_get_user(). This function has problems to process both 
complete
C string or not.

I will send the kernel patches and Cc you guys. And I still think it is better
let perf write a complete C string.

Thanks!
Changbin Du

> jirka
>
[...] 

Re: [PATCH 1/3] tracing: detect the string termination character when parsing user input string

[Du, Changbin]

hi Rostedt,

On Tue, Jan 09, 2018 at 05:54:34PM -0500, Steven Rostedt wrote:
> On Tue,  9 Jan 2018 17:55:46 +0800
> changbin...@intel.com wrote:
> 
> > From: Changbin Du 
> > 
> > The usersapce can give a '\0' terminated C string or even has '\0' at the
> > middle of input buffer. We need handle both these two cases correctly.
> 
> What do you define as correctly. Because I'm not seeing it.
>
Soory I don't fully understand your question. What I meant is want to get clear 
that
how will tracing parser below strings.
  "", "  ",  "\0", " \0 ", "aa\0bb"
The parser may only recognize certain formats, but whatever the behaviour should
be clear and coherent for all tracing interfaces.

> > 
> > Before this change, trace_get_user() will return a parsed string "\0" in
> > below case. It is not expected (expects it skip all inputs) and cause the
> > caller failed.
> > 
> > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > write(3, " \0", 2)  = -1 EINVAL (Invalid argument)
> 
> That looks more like a feature and not a bug.
> 
I point this out because I think the parser should take this as an emptry string
per the comments of trace_get_user().
/*
 * trace_get_user - reads the user input string separated by  space
 * (matched by isspace(ch))
 *
 * For each string found the 'struct trace_parser' is updated,
 * and the function returns.
 *
 * Returns number of bytes read.
 *
 * See kernel/trace/trace.h for 'struct trace_parser' details.
 */

> > 
> > This patch try to make the parser '\0' aware to fix such issue.
> 
> Why?
> 

> > 
> > Since the caller expects trace_get_user() to parse whole input buffer, so
> > this patch treat '\0' as a separator as whitespace.
> 
> It looks more like we are trying to fix a userspace bug via the kernel.
> 

> I'm not liking this. So NACK.
> 
> -- Steve
> 
> > 
> > Signed-off-by: Changbin Du 
> > ---
> >  kernel/trace/trace.c | 17 +++--
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 2a8d8a2..18526a1 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -1194,9 +1194,14 @@ void trace_parser_put(struct trace_parser *parser)
> > parser->buffer = NULL;
> >  }
> >  
> > +static inline bool is_space_or_zero(char ch)
> > +{
> > +   return isspace(ch) || !ch;
> > +}
> > +
> >  /*
> > - * trace_get_user - reads the user input string separated by  space
> > - * (matched by isspace(ch))
> > + * trace_get_user - reads the user input string separated by space or '\0'
> > + * (matched by is_space_or_zero(ch))
> >   *
> >   * For each string found the 'struct trace_parser' is updated,
> >   * and the function returns.
> > @@ -1228,7 +1233,7 @@ int trace_get_user(struct trace_parser *parser, const 
> > char __user *ubuf,
> >  */
> > if (!parser->cont) {
> > /* skip white space */
> > -   while (cnt && isspace(ch)) {
> > +   while (cnt && is_space_or_zero(ch)) {
> > ret = get_user(ch, ubuf++);
> > if (ret)
> > goto out;
> > @@ -1237,7 +1242,7 @@ int trace_get_user(struct trace_parser *parser, const 
> > char __user *ubuf,
> > }
> >  
> > /* only spaces were written */
> > -   if (isspace(ch)) {
> > +   if (is_space_or_zero(ch)) {
> > *ppos += read;
> > ret = read;
> > goto out;
> > @@ -1247,7 +1252,7 @@ int trace_get_user(struct trace_parser *parser, const 
> > char __user *ubuf,
> > }
> >  
> > /* read the non-space input */
> > -   while (cnt && !isspace(ch)) {
> > +   while (cnt && !is_space_or_zero(ch)) {
> > if (parser->idx < parser->size - 1)
> > parser->buffer[parser->idx++] = ch;
> > else {
> > @@ -1262,7 +1267,7 @@ int trace_get_user(struct trace_parser *parser, const 
> > char __user *ubuf,
> > }
> >  
> > /* We either got finished input or we have to wait for another call. */
> > -   if (isspace(ch)) {
> > +   if (is_space_or_zero(ch)) {
> > parser->buffer[parser->idx] = 0;
> > parser->cont = false;
> > } else if (parser->idx < parser->size - 1) {
> 

-- 
Thanks,
Changbin Du

Re: [PATCH 2/3] tracing: make sure the parsed string always terminates with '\0'

[Du, Changbin]

On Tue, Jan 09, 2018 at 06:02:58PM -0500, Steven Rostedt wrote:
> On Tue,  9 Jan 2018 17:55:47 +0800
> changbin...@intel.com wrote:
> 
> > From: Changbin Du 
> > 
> > The parser parse every string into parser.buffer. And some of the callers
> > assume that parser.buffer contains a C string. So it is dangerous that the
> > parser returns a unterminated string. The userspace can leverage this to
> > attack the kernel.
> 
> Is this only a bug if we apply your first patch?
>
I don't think so. Seems it is there already.
 
> -- Steve
> 

-- 
Thanks,
Changbin Du

Re: [PATCH 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Du, Changbin]

On Tue, Jan 09, 2018 at 06:12:41PM -0500, Steven Rostedt wrote:
> On Tue,  9 Jan 2018 17:55:48 +0800
> changbin...@intel.com wrote:
> 
> > From: Changbin Du 
> > 
> > We should not set parser->cont if it has reached the end of input buffer.
> > And since some callers (like ftrace_graph_write()) treat it as an error
> > condition if trace_parser_cont() returns true.
> 
> This will break existing use cases. In fact you are removing the entire
> point of this code. It NEEDS to continue if it reached the end of the
> input buffer.
> 
> I do things like:
> 
>  # cat file > set_ftrace_filter
> 
> where the file has a list of function names. It writes in blocks, and
> it could very well have a function name split between two writes where
> the write is at the end of the buffer but not finished writing the
> function name.
>
> > 
> > For example, if userspace set 'set_ftrace_filter' by writing:
> > write(3, "abcdefg", 7)
> 
> From my point of view, the above isn't done writing the function name
> yet and we SHOULD continue waiting for more input.
> 
hmm, thanks for the background. Your above case is a postive use case. So by
this design, instead of write(3, "abcdefg", 7), it should be
write(3, "abcdefg\0", 8), right?

If true, it means kernel expect userspace write every string terminated with
'\0'. So to fix this issue:
open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
write(3, " \0", 2)  = -1 EINVAL (Invalid argument)

Fix would be:
write(3, "\0", 1)?

So far, I am still confused. Some of the tracing debugfs entry accept '\0'
while some not. AFIK, 'echo xxx > ' always has a '\0'
terminated.

> BIG NACK on this patch. Sorry.
> 
> I'm guessing you have some program that writes only the strlen() of
> these strings. That's wrong, you need to write "strlen()+1". Write some
> real white space between calls, it will work. Add a "write(fd, " ", 1)"
> between calls if you need to. Please don't change the kernel to fix
> some bad use case. Especially when your fix will break existing use
> cases.
> 
> -- Steve
> 
> > 
> > Then in the kernel function ftrace_regex_write(), ftrace_process_regex()
> > will not be executed. The result is that the given filter will not be
> > applied at all.
> > 
> > ftrace_regex_write() {
> > ...
> > read = trace_get_user(parser, ubuf, cnt, ppos);
> > if (read >= 0 && trace_parser_loaded(parser) &&
> > !trace_parser_cont(parser)) {
> > ret = ftrace_process_regex(iter, parser->buffer,
> >parser->idx, enable);
> > ...
> > }
> > ...
> > }
> > 
> > Signed-off-by: Changbin Du 

-- 
Thanks,
Changbin Du

Re: [PATCH 3/3] tracing: don't set parser->cont if it has reached the end of input buffer

[Du, Changbin]

On Fri, Jan 12, 2018 at 10:31:08AM -0500, Steven Rostedt wrote:
[...]
> > Thanks, so now I unstand why below corner case. The userspace try to set the
> > filter with a unrecognized symbole name (e.g "abcdefg").
> > open("/sys/kernel/debug/tracing/set_ftrace_filter", O_WRONLY|O_TRUNC) = 3
> > write(3, "abcdefg", 7)
> > 
> > Since "abcdefg" is not in the symbole list, so we would expect the write 
> > return
> > -EINVAL, right? As below:
> > # echo abcdefg > set_ftrace_filter
> > bash: echo: write error: Invalid argument
> 
> The write itself doesn't finish the operation. There may be another
> write. In other words:
> 
>   write(3, "do_", 3);
>   write(3, "IRQ\n", 4);
> 
> Should both return success, even though it only enabled do_IRQ.
> 
> > 
> > But the above mechanism hide the error. It return success actually no 
> > filter is
> > apllied at all.
> > # echo -n abcdefg > set_ftrace_filter
> > 
> > I think in this case kernel may request the userspace append a '\0' or 
> > space to the
> > string buffer so everything can work.
> > 
> > Also there is another corner case. Below write dosn't work.
> > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > write(3, " \0", 2)  = -1 EINVAL (Invalid argument)
> > 
> > While these works:
> > # echo "" > set_ftrace_pid
> > # echo " " > set_ftrace_pid
> > # echo -n " " > set_ftrace_pid
> > 
> > These is the reason why I think '\0' should be recognized by the parser.
> 
> Hmm, thinking about this more, I do partially agree with you. We should
> accept '\0' but I disagree that it should be treated as a space. I
> don't want hidden code.
> 
> It should be treated as a terminator. And carefully as well.
> 
>   write(3, "do_IRQ", 7);
> 
> Which will send to the kernel 'd' 'o' '_' 'I' 'R' 'Q' '\0' when the
> kernel sees the '\0', and the write has not sent anything else, it
> should go ahead and execute 'do_IRQ'
> 
> This will allow for this to work:
> 
>   char *funcs[] = { "do_IRQ", "schedule", NULL };
> 
>   for (i = 0; funcs[i]; i++) {
>   ret = write(3, funcs[i], strlen(funcs[i]) + 1);
>   if (ret < 0)
>   exit(-1);
>   }
> 
> 
> Now if someone were to write:
> 
>   write(3, "do_IRQ\0schedule", 16);
> 
> That should return an error.
> 
> Why?
> 
> Because these are strings, and most tools treat '\0' as a nul
> terminator to a string. If we allow for tools to send data after that
> nul terminator, we are opening up a way for those interacting with
> these tools to sneak in strings that are not visible.
> 
> Say we have some admin tools that is doing tracing, and takes input.
> And all the input is logged. And say the tool does something like:
> 
> 
>   r = read(0, buf, sizeof(buf));
>   if (r < 0 || r > sizeof(buf) - 1)
>   return -1;
>   log("Adding to output %s\n", buf);
>   write(3, buf, r);
> 
> The "Adding to output" would only show up to the '\0', but if we allow
> that write to process after the '\0' then we just allowed the user to
> circumvent the log.
> 
> -- Steve
I agree on your concern. So I will revise this serias and drop the last patch.

-- 
Thanks,
Changbin Du

Re: [PATCH 2/3] tracing: make sure the parsed string always terminates with '\0'

[Du, Changbin]

On Tue, Jan 09, 2018 at 11:10:22PM -0500, Steven Rostedt wrote:
> On Wed, 10 Jan 2018 11:02:06 +0800
> "Du, Changbin"  wrote:
> 
> > On Tue, Jan 09, 2018 at 06:02:58PM -0500, Steven Rostedt wrote:
> > > On Tue,  9 Jan 2018 17:55:47 +0800
> > > changbin...@intel.com wrote:
> > >   
> > > > From: Changbin Du 
> > > > 
> > > > The parser parse every string into parser.buffer. And some of the 
> > > > callers
> > > > assume that parser.buffer contains a C string. So it is dangerous that 
> > > > the
> > > > parser returns a unterminated string. The userspace can leverage this to
> > > > attack the kernel.  
> > > 
> > > Is this only a bug if we apply your first patch?
> > >  
> > I don't think so. Seems it is there already.
> >  
> 
> OK. I'll have to take a deeper look into this so that I completely
> understand the problem and your solution. I'm currently traveling and
> may not get to do that this week. Please ping me next week if you don't
> hear back from me on this issue.
> 
> Thanks!
> 
> -- Steve

I checked every trace_get_user() clients again and found it is not an issue in
current kernel. The client has checked trace_parser_cont() before using parsed
string or append '\0'.

I still want to make the parser returns a '\0' terminated string. Then we don't
require the clients append it. I think this would be better since we are dealing
with strings.

-- 
Thanks,
Changbin Du

Re: [PATCH v2 1/3] tracing: detect the string termination character when parsing user input string

[Du, Changbin]

Hi Rostedt,
Thanks for your polish, let me update commit msg with your words.

On Mon, Jan 15, 2018 at 06:20:00PM -0500, Steven Rostedt wrote:
> On Mon, 15 Jan 2018 19:41:12 +0800
> changbin...@intel.com wrote:
> 
> > From: Changbin Du 
> > 
> > The usersapce can give a '\0' terminated C string in the input buffer.
> > Before this change, trace_get_user() will return a parsed string "\0" in
> > below case which is not expected (expects it skip all inputs) and cause the
> > caller failed.
> 
> The above totally does not parse (no pun intended).
> 
> Are you trying to say:
> 
> "User space can pass in a C nul character '\0' along with its input.
> The function trace_get_user() will try to process it as a normal
> character, and that will fail to parse.
> 
> > 
> > open("/sys/kernel/debug/tracing//set_ftrace_pid", O_WRONLY|O_TRUNC) = 3
> > write(3, " \0", 2)  = -1 EINVAL (Invalid argument)
> > 
> > while parse can handle spaces, so below works.
> > 
> > $ echo "" > set_ftrace_pid
> > $ echo " " > set_ftrace_pid
> > $ echo -n " " > set_ftrace_pid
> > 
> > This patch try to make the parser '\0' aware to fix such issue. When parser
> > sees a '\0' it stops further parsing. With this change, write(3, " \0", 2)
> > will work.
> 
> The above should be something like:
> 
> "Have the parser stop on '\0' and cease any further parsing. Only
> process the characters up to the nul '\0' character and do not process
> it."
> 
> -- Steve
> 
> 
> > 
> > Signed-off-by: Changbin Du 
> > 
> > ---
> >   v2: Stop parsing when '\0' found.
> > ---
> >  kernel/trace/trace.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index 2a8d8a2..144d08e 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -1237,7 +1237,7 @@ int trace_get_user(struct trace_parser *parser, const 
> > char __user *ubuf,
> > }
> >  
> > /* only spaces were written */
> > -   if (isspace(ch)) {
> > +   if (isspace(ch) || !ch) {
> > *ppos += read;
> > ret = read;
> > goto out;
> > @@ -1247,7 +1247,7 @@ int trace_get_user(struct trace_parser *parser, const 
> > char __user *ubuf,
> > }
> >  
> > /* read the non-space input */
> > -   while (cnt && !isspace(ch)) {
> > +   while (cnt && !isspace(ch) && ch) {
> > if (parser->idx < parser->size - 1)
> > parser->buffer[parser->idx++] = ch;
> > else {
> > @@ -1262,7 +1262,7 @@ int trace_get_user(struct trace_parser *parser, const 
> > char __user *ubuf,
> > }
> >  
> > /* We either got finished input or we have to wait for another call. */
> > -   if (isspace(ch)) {
> > +   if (isspace(ch) || !ch) {
> > parser->buffer[parser->idx] = 0;
> > parser->cont = false;
> > } else if (parser->idx < parser->size - 1) {
> 

-- 
Thanks,
Changbin Du

Re: [RESEND PATCH] perf sched map: re-annotate shortname if thread comm changed

[Du, Changbin]

I just done final version, please check v2. Thanks for your comments!

On Mon, Mar 05, 2018 at 11:37:54PM +0100, Jiri Olsa wrote:
> On Mon, Mar 05, 2018 at 03:11:36PM +0800, Du, Changbin wrote:
> 
> SNIP
> 
> > > > on the other hand it's simple enough and looks
> > > > like generic solution would be more tricky
> > > 
> > > What about adding perf_sched__process_comm() to set it in the
> > > thread::priv?
> > >
> > I can be done, then thread->comm_changed moves to 
> > thread_runtime->comm_changed.
> > Draft code as below. It is also a little tricky.
> > 
> > +int perf_sched__process_comm(struct perf_tool *tool __maybe_unused,
> > +union perf_event *event,
> > +struct perf_sample *sample,
> > +struct machine *machine)
> > +{
> > +   struct thread *thread;
> > +   struct thread_runtime *r;
> > +
> > +   perf_event__process_comm(tool, event, sample, machine);
> > +
> > +   thread = machine__findnew_thread(machine, pid, tid);
> 
> should you use machine__find_thread in here?
> 
> > +   if (thread) {
> > +   r = thread__priv(thread);
> > +   if (r)
> > +   r->comm_changed = true;
> > +   thread__put(thread);
> > +   }
> > +}
> > +
> >  static int perf_sched__read_events(struct perf_sched *sched)
> >  {
> > const struct perf_evsel_str_handler handlers[] = {
> > @@ -3291,7 +3311,7 @@ int cmd_sched(int argc, const char **argv)
> > struct perf_sched sched = {
> > .tool = {
> > .sample  = 
> > perf_sched__process_tracepoint_sample,
> > -   .comm= perf_event__process_comm,
> > +   .comm= perf_sched__process_comm,
> > 
> > 
> > But I'd keep 'comm_changed' where 'shortname' is defined. I think they 
> > should appears
> > togother. And 'shortname' is only used by sched command, too.
> 
> they can both go to struct thread_runtime then
> 
> > 
> > So I still prefer my privous simpler change. Thanks!
> 
> I was wrong thinking that the amount of code
> making it sched specific would be biger
> 
> we're trying to keep the core structs generic,
> so this one fits better 
> 
> thanks,
> jirka

-- 
Thanks,
Changbin Du

Re: [PATCH v2 0/2] perf sched map: re-annotate shortname if thread comm changed

[Du, Changbin]

On Tue, Mar 06, 2018 at 11:17:07AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 06, 2018 at 08:53:02AM +0100, Jiri Olsa escreveu:
> > On Tue, Mar 06, 2018 at 11:37:35AM +0800, changbin...@intel.com wrote:
> > > From: Changbin Du 
> > > 
> > > v2:
> > >   o add a patch to move thread::shortname to thread_runtime
> > >   o add function perf_sched__process_comm() to process PERF_RECORD_COMM 
> > > event.
> > > 
> > > Changbin Du (2):
> > >   perf sched: move thread::shortname to thread_runtime
> > >   perf sched map: re-annotate shortname if thread comm changed
> > 
> > Acked-by: Jiri Olsa 
> 
> Thanks, applied both, the final layout for 'struct thread_runtime':
> 
> [root@jouet perf]# pahole -C thread_runtime ~/bin/perf
> struct thread_runtime {
>   u64last_time;/* 0 8 */
>   u64dt_run;   /* 8 8 */
>   u64dt_sleep; /*16 8 */
>   u64dt_iowait;/*24 8 */
>   u64dt_preempt;   /*32 8 */
>   u64dt_delay; /*40 8 */
>   u64ready_to_run; /*48 8 */
>   struct stats   run_stats;/*5640 */
>   /* --- cacheline 1 boundary (64 bytes) was 32 bytes ago --- */
>   u64total_run_time;   /*96 8 */
>   u64total_sleep_time; /*   104 8 */
>   u64total_iowait_time;/*   112 8 */
>   u64total_preempt_time;   /*   120 8 */
>   /* --- cacheline 2 boundary (128 bytes) --- */
>   u64total_delay_time; /*   128 8 */
>   intlast_state;   /*   136 4 */
>   char   shortname[3]; /*   140 3 */
>   _Bool  comm_changed; /*   143 1 */
>   u64migrations;   /*   144 8 */
> 
>   /* size: 152, cachelines: 3, members: 17 */
>   /* last cacheline: 24 bytes */
> };
> [root@jouet perf]#

Hi Arnaldo, thanks for your patient optimization for this!

-- 
Thanks,
Changbin Du

Re: [PATCH 4/4] selftests/bpf: fix compiling errors

[Du, Changbin]

On Mon, Mar 26, 2018 at 07:55:13AM -0700, Alexei Starovoitov wrote:
> On Mon, Mar 26, 2018 at 05:23:28PM +0800, changbin...@intel.com wrote:
> > Signed-off-by: Changbin Du 
> > ---
> >  tools/testing/selftests/bpf/Makefile | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/Makefile 
> > b/tools/testing/selftests/bpf/Makefile
> > index 5c43c18..dc0fdc8 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -10,7 +10,8 @@ ifneq ($(wildcard $(GENHDR)),)
> >GENFLAGS := -DHAVE_GENHDR
> >  endif
> >  
> > -CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) 
> > -I../../../include
> > +CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) \
> > + -I../../../include -I../../../../usr/include
> >  LDLIBS += -lcap -lelf -lrt -lpthread
> >  
> >  # Order correspond to 'make run_tests' order
> > @@ -62,7 +63,7 @@ else
> >CPU ?= generic
> >  endif
> >  
> > -CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> > +CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi 
> > -I../../../../usr/include \
> >   -Wno-compare-distinct-pointer-types
> 
> Nack.
> I suspect that will break the build for everyone else who's doing it in the 
> directory
> itself instead of the outer one.
>

This one? But I didn't see any problem.

changbin@gvt-dell-host:~/work/linux/tools/testing/selftests/bpf$ make
make -C ../../../lib/bpf 
OUTPUT=/home/changbin/work/linux/tools/testing/selftests/bpf/
make[1]: Entering directory '/home/changbin/work/linux/tools/lib/bpf'
  HOSTCC   /home/changbin/work/linux/tools/testing/selftests/bpf/fixdep.o
  HOSTLD   /home/changbin/work/linux/tools/testing/selftests/bpf/fixdep-in.o
  LINK /home/changbin/work/linux/tools/testing/selftests/bpf/fixdep
  CC   /home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.o
  CC   /home/changbin/work/linux/tools/testing/selftests/bpf/bpf.o
  CC   /home/changbin/work/linux/tools/testing/selftests/bpf/nlattr.o
  LD   /home/changbin/work/linux/tools/testing/selftests/bpf/libbpf-in.o
  LINK /home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.a
  LINK /home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.so
make[1]: Leaving directory '/home/changbin/work/linux/tools/lib/bpf'
make -C ../../../lib/bpf 
OUTPUT=/home/changbin/work/linux/tools/testing/selftests/bpf/
make[1]: Entering directory '/home/changbin/work/linux/tools/lib/bpf'
make[1]: Leaving directory '/home/changbin/work/linux/tools/lib/bpf'
gcc -Wall -O2 -I../../../include/uapi -I../../../lib 
-I../../../../include/generated -DHAVE_GENHDR -I../../../include 
-I../../../../usr/includetest_verifier.c 
/home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.a cgroup_helpers.c 
-lcap -lelf -lrt -lpthread -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_verifier
gcc -Wall -O2 -I../../../include/uapi -I../../../lib 
-I../../../../include/generated -DHAVE_GENHDR -I../../../include 
-I../../../../usr/includetest_tag.c 
/home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.a cgroup_helpers.c 
-lcap -lelf -lrt -lpthread -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_tag
gcc -Wall -O2 -I../../../include/uapi -I../../../lib 
-I../../../../include/generated -DHAVE_GENHDR -I../../../include 
-I../../../../usr/includetest_maps.c 
/home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.a cgroup_helpers.c 
-lcap -lelf -lrt -lpthread -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_maps
gcc -Wall -O2 -I../../../include/uapi -I../../../lib 
-I../../../../include/generated -DHAVE_GENHDR -I../../../include 
-I../../../../usr/includetest_lru_map.c 
/home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.a cgroup_helpers.c 
-lcap -lelf -lrt -lpthread -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_lru_map
gcc -Wall -O2 -I../../../include/uapi -I../../../lib 
-I../../../../include/generated -DHAVE_GENHDR -I../../../include 
-I../../../../usr/includetest_lpm_map.c 
/home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.a cgroup_helpers.c 
-lcap -lelf -lrt -lpthread -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_lpm_map
gcc -Wall -O2 -I../../../include/uapi -I../../../lib 
-I../../../../include/generated -DHAVE_GENHDR -I../../../include 
-I../../../../usr/includetest_progs.c 
/home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.a cgroup_helpers.c 
-lcap -lelf -lrt -lpthread -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_progs
gcc -Wall -O2 -I../../../include/uapi -I../../../lib 
-I../../../../include/generated -DHAVE_GENHDR -I../../../include 
-I../../../../usr/includetest_align.c 
/home/changbin/work/linux/tools/testing/selftests/bpf/libbpf.a cgroup_helpers.c 
-lcap -lelf -lrt -lpthread -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_align
gcc -Wall -O2 

Re: [PATCH 4/4] selftests/bpf: fix compiling errors

[Du, Changbin]

Hi Starovoitov,

This one does have the issue you mentioned.
[PATCH 2/4] selftests/gpio: fix paths in Makefile

And can be fixed by:

--- a/tools/testing/selftests/gpio/Makefile
+++ b/tools/testing/selftests/gpio/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0

+OUTPUT ?= $(shell pwd)
 TEST_PROGS := gpio-mockup.sh
 TEST_FILES := gpio-mockup-sysfs.sh $(BINARIES)
 BINARIES := gpio-mockup-chardev
@@ -24,7 +25,7 @@ LDLIBS += -lmount -I/usr/include/libmount
 $(BINARIES): gpio-utils.o ../../../../usr/include/linux/gpio.h

 gpio-utils.o:
-   make ARCH=$(ARCH) CROSS_COMPILE=$(CROSS_COMPILE) -C ../../../gpio
+   make ARCH=$(ARCH) CROSS_COMPILE=$(CROSS_COMPILE) OUTPUT=$(OUTPUT)/ -C 
../../../gpio

 ../../../../usr/include/linux/gpio.h:


I will update it later.

On Mon, Mar 26, 2018 at 07:55:13AM -0700, Alexei Starovoitov wrote:
> On Mon, Mar 26, 2018 at 05:23:28PM +0800, changbin...@intel.com wrote:
> > From: Changbin Du 
> > 
> > This patch fixed below errors of missing head files.
> > 
> > tools/testing/selftests$ make
> > ...
> > clang -I. -I./include/uapi -I../../../include/uapi 
> > -Wno-compare-distinct-pointer-types \
> >  -O2 -target bpf -emit-llvm -c test_pkt_access.c -o - |  \
> > llc -march=bpf -mcpu=generic -filetype=obj -o 
> > /home/changbin/work/linux/tools/testing/selftests/bpf//test_pkt_access.o
> > In file included from test_pkt_access.c:9:
> > In file included from ../../../include/uapi/linux/bpf.h:11:
> > In file included from ./include/uapi/linux/types.h:5:
> > /usr/include/asm-generic/int-ll64.h:11:10: fatal error: 'asm/bitsperlong.h' 
> > file not found
> >  #include 
> >  ^
> > 1 error generated.
> > clang -I. -I./include/uapi -I../../../include/uapi 
> > -Wno-compare-distinct-pointer-types \
> >  -O2 -target bpf -emit-llvm -c test_xdp.c -o - |  \
> > llc -march=bpf -mcpu=generic -filetype=obj -o 
> > /home/changbin/work/linux/tools/testing/selftests/bpf//test_xdp.o
> > In file included from test_xdp.c:9:
> > In file included from ../../../include/uapi/linux/bpf.h:11:
> > In file included from ./include/uapi/linux/types.h:5:
> > /usr/include/asm-generic/int-ll64.h:11:10: fatal error: 'asm/bitsperlong.h' 
> > file not found
> >  #include 
> >  ^
> > 1 error generated.
> > clang -I. -I./include/uapi -I../../../include/uapi 
> > -Wno-compare-distinct-pointer-types \
> >  -O2 -target bpf -emit-llvm -c test_l4lb.c -o - |  \
> > llc -march=bpf -mcpu=generic -filetype=obj -o 
> > /home/changbin/work/linux/tools/testing/selftests/bpf//test_l4lb.o
> > In file included from test_l4lb.c:10:
> > In file included from /usr/include/linux/pkt_cls.h:4:
> > In file included from ./include/uapi/linux/types.h:5:
> > /usr/include/asm-generic/int-ll64.h:11:10: fatal error: 'asm/bitsperlong.h' 
> > file not found
> >  #include 
> >  ^
> > 1 error generated.
> > clang -I. -I./include/uapi -I../../../include/uapi 
> > -Wno-compare-distinct-pointer-types \
> >  -O2 -target bpf -emit-llvm -c test_tcp_estats.c -o - |  \
> > llc -march=bpf -mcpu=generic -filetype=obj -o 
> > /home/changbin/work/linux/tools/testing/selftests/bpf//test_tcp_estats.o
> > In file included from test_tcp_estats.c:35:
> > In file included from ../../../include/uapi/linux/bpf.h:11:
> > In file included from ./include/uapi/linux/types.h:5:
> > /usr/include/asm-generic/int-ll64.h:11:10: fatal error: 'asm/bitsperlong.h' 
> > file not found
> >  #include 
> > ...
> > 
> > Signed-off-by: Changbin Du 
> > ---
> >  tools/testing/selftests/bpf/Makefile | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/Makefile 
> > b/tools/testing/selftests/bpf/Makefile
> > index 5c43c18..dc0fdc8 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -10,7 +10,8 @@ ifneq ($(wildcard $(GENHDR)),)
> >GENFLAGS := -DHAVE_GENHDR
> >  endif
> >  
> > -CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) 
> > -I../../../include
> > +CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) \
> > + -I../../../include -I../../../../usr/include
> >  LDLIBS += -lcap -lelf -lrt -lpthread
> >  
> >  # Order correspond to 'make run_tests' order
> > @@ -62,7 +63,7 @@ else
> >CPU ?= generic
> >  endif
> >  
> > -CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> > +CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi 
> > -I../../../../usr/include \
> >   -Wno-compare-distinct-pointer-types
> 
> Nack.
> I suspect that will break the build for everyone else who's doing it in the 
> directory
> itself instead of the outer one.
> 

-- 
Thanks,
Changbin Du

Re: [PATCH 4/4] selftests/bpf: fix compiling errors

[Du, Changbin]

On Mon, Mar 26, 2018 at 08:02:30PM -0700, Alexei Starovoitov wrote:
> On Tue, Mar 27, 2018 at 10:20:10AM +0800, Du, Changbin wrote:
> > On Mon, Mar 26, 2018 at 07:55:13AM -0700, Alexei Starovoitov wrote:
> > > On Mon, Mar 26, 2018 at 05:23:28PM +0800, changbin...@intel.com wrote:
> > > > Signed-off-by: Changbin Du 
> > > > ---
> > > >  tools/testing/selftests/bpf/Makefile | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/bpf/Makefile 
> > > > b/tools/testing/selftests/bpf/Makefile
> > > > index 5c43c18..dc0fdc8 100644
> > > > --- a/tools/testing/selftests/bpf/Makefile
> > > > +++ b/tools/testing/selftests/bpf/Makefile
> > > > @@ -10,7 +10,8 @@ ifneq ($(wildcard $(GENHDR)),)
> > > >GENFLAGS := -DHAVE_GENHDR
> > > >  endif
> > > >  
> > > > -CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) 
> > > > -I../../../include
> > > > +CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) \
> > > > + -I../../../include -I../../../../usr/include
> > > >  LDLIBS += -lcap -lelf -lrt -lpthread
> > > >  
> > > >  # Order correspond to 'make run_tests' order
> > > > @@ -62,7 +63,7 @@ else
> > > >CPU ?= generic
> > > >  endif
> > > >  
> > > > -CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> > > > +CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi 
> > > > -I../../../../usr/include \
> > > >   -Wno-compare-distinct-pointer-types
> > > 
> > > Nack.
> > > I suspect that will break the build for everyone else who's doing it in 
> > > the directory
> > > itself instead of the outer one.
> > >
> > 
> > This one? But I didn't see any problem.
> 
> because the build was lucky and additional path ../../../../usr/include 
> didn't point
> to a valid dir?
I am sorry but I don't understand why you mean *lucky*. Of cause, the path is 
valid.

> Please test with in-source and out-of-source builds.
> 
agree.

-- 
Thanks,
Changbin Du

Re: [PATCH 4/4] selftests/bpf: fix compiling errors

[Du, Changbin]

On Tue, Mar 27, 2018 at 10:52:57AM +0200, Daniel Borkmann wrote:
> On 03/27/2018 05:06 AM, Du, Changbin wrote:
> > On Mon, Mar 26, 2018 at 08:02:30PM -0700, Alexei Starovoitov wrote:
> >> On Tue, Mar 27, 2018 at 10:20:10AM +0800, Du, Changbin wrote:
> >>> On Mon, Mar 26, 2018 at 07:55:13AM -0700, Alexei Starovoitov wrote:
> >>>> On Mon, Mar 26, 2018 at 05:23:28PM +0800, changbin...@intel.com wrote:
> >>>>> Signed-off-by: Changbin Du 
> >>>>> ---
> >>>>>  tools/testing/selftests/bpf/Makefile | 5 +++--
> >>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/tools/testing/selftests/bpf/Makefile 
> >>>>> b/tools/testing/selftests/bpf/Makefile
> >>>>> index 5c43c18..dc0fdc8 100644
> >>>>> --- a/tools/testing/selftests/bpf/Makefile
> >>>>> +++ b/tools/testing/selftests/bpf/Makefile
> >>>>> @@ -10,7 +10,8 @@ ifneq ($(wildcard $(GENHDR)),)
> >>>>>GENFLAGS := -DHAVE_GENHDR
> >>>>>  endif
> >>>>>  
> >>>>> -CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) 
> >>>>> -I../../../include
> >>>>> +CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) \
> >>>>> + -I../../../include -I../../../../usr/include
> >>>>>  LDLIBS += -lcap -lelf -lrt -lpthread
> >>>>>  
> >>>>>  # Order correspond to 'make run_tests' order
> >>>>> @@ -62,7 +63,7 @@ else
> >>>>>CPU ?= generic
> >>>>>  endif
> >>>>>  
> >>>>> -CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> >>>>> +CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi 
> >>>>> -I../../../../usr/include \
> >>>>>   -Wno-compare-distinct-pointer-types
> >>>>
> >>>> Nack.
> >>>> I suspect that will break the build for everyone else who's doing it in 
> >>>> the directory
> >>>> itself instead of the outer one.
> >>>
> >>> This one? But I didn't see any problem.
> >>
> >> because the build was lucky and additional path ../../../../usr/include 
> >> didn't point
> >> to a valid dir?
> 
> Agree.
> 
> > I am sorry but I don't understand why you mean *lucky*. Of cause, the path 
> > is valid.
> 
> The problem is that this suddenly requires users to do a 'make 
> headers_install' in
> order to populate usr/include/ directory in the first place. While it's 
> annoying
> enough for BPF samples where this is needed, I absolutely don't want to 
> introduce
> this for BPF kselftests. It's the wrong approach. Besides, in tools infra, 
> there is
> a tools/arch/*/include/uapi/asm/bitsperlong.h header copy already, so we 
> really need
> to use that instead. Please adapt your patch accordingly and respin. Please 
> also Cc
> us and net...@vger.kernel.org for BPF kselftests changes.
> 
> Thanks,
> Daniel
Thanks for the explanation. So we expect that tools/arch/*/include is in the 
searching list, right?
The corrent makefile seems not. How do you get this built?

changbin@gvt-dell-host:~/work/linux/tools/testing/selftests/bpf$ make -p
[...]
clang -I. -I./include/uapi -I../../../include/uapi 
-Wno-compare-distinct-pointer-types \
 -O2 -target bpf -emit-llvm -c test_pkt_access.c -o - |  \
llc -march=bpf -mcpu=generic -filetype=obj -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_pkt_access.o
In file included from test_pkt_access.c:9:
In file included from ../../../include/uapi/linux/bpf.h:11:
In file included from ./include/uapi/linux/types.h:5:
/usr/include/asm-generic/int-ll64.h:11:10: fatal error: 'asm/bitsperlong.h' 
file not found
#include 


-- 
Thanks,
Changbin Du

Re: [PATCH 4/4] selftests/bpf: fix compiling errors

[Du, Changbin]

On Tue, Mar 27, 2018 at 11:52:27AM +0200, Daniel Borkmann wrote:
> On 03/27/2018 11:00 AM, Du, Changbin wrote:
> > On Tue, Mar 27, 2018 at 10:52:57AM +0200, Daniel Borkmann wrote:
> >> On 03/27/2018 05:06 AM, Du, Changbin wrote:
> >>> On Mon, Mar 26, 2018 at 08:02:30PM -0700, Alexei Starovoitov wrote:
> >>>> On Tue, Mar 27, 2018 at 10:20:10AM +0800, Du, Changbin wrote:
> >>>>> On Mon, Mar 26, 2018 at 07:55:13AM -0700, Alexei Starovoitov wrote:
> >>>>>> On Mon, Mar 26, 2018 at 05:23:28PM +0800, changbin...@intel.com wrote:
> >>>>>>> Signed-off-by: Changbin Du 
> >>>>>>> ---
> >>>>>>>  tools/testing/selftests/bpf/Makefile | 5 +++--
> >>>>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/tools/testing/selftests/bpf/Makefile 
> >>>>>>> b/tools/testing/selftests/bpf/Makefile
> >>>>>>> index 5c43c18..dc0fdc8 100644
> >>>>>>> --- a/tools/testing/selftests/bpf/Makefile
> >>>>>>> +++ b/tools/testing/selftests/bpf/Makefile
> >>>>>>> @@ -10,7 +10,8 @@ ifneq ($(wildcard $(GENHDR)),)
> >>>>>>>GENFLAGS := -DHAVE_GENHDR
> >>>>>>>  endif
> >>>>>>>  
> >>>>>>> -CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) 
> >>>>>>> -I../../../include
> >>>>>>> +CFLAGS += -Wall -O2 -I$(APIDIR) -I$(LIBDIR) -I$(GENDIR) $(GENFLAGS) \
> >>>>>>> +   -I../../../include -I../../../../usr/include
> >>>>>>>  LDLIBS += -lcap -lelf -lrt -lpthread
> >>>>>>>  
> >>>>>>>  # Order correspond to 'make run_tests' order
> >>>>>>> @@ -62,7 +63,7 @@ else
> >>>>>>>CPU ?= generic
> >>>>>>>  endif
> >>>>>>>  
> >>>>>>> -CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> >>>>>>> +CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi 
> >>>>>>> -I../../../../usr/include \
> >>>>>>> -Wno-compare-distinct-pointer-types
> >>>>>>
> >>>>>> Nack.
> >>>>>> I suspect that will break the build for everyone else who's doing it 
> >>>>>> in the directory
> >>>>>> itself instead of the outer one.
> >>>>>
> >>>>> This one? But I didn't see any problem.
> >>>>
> >>>> because the build was lucky and additional path ../../../../usr/include 
> >>>> didn't point
> >>>> to a valid dir?
> >>
> >> Agree.
> >>
> >>> I am sorry but I don't understand why you mean *lucky*. Of cause, the 
> >>> path is valid.
> >>
> >> The problem is that this suddenly requires users to do a 'make 
> >> headers_install' in
> >> order to populate usr/include/ directory in the first place. While it's 
> >> annoying
> >> enough for BPF samples where this is needed, I absolutely don't want to 
> >> introduce
> >> this for BPF kselftests. It's the wrong approach. Besides, in tools infra, 
> >> there is
> >> a tools/arch/*/include/uapi/asm/bitsperlong.h header copy already, so we 
> >> really need
> >> to use that instead. Please adapt your patch accordingly and respin. 
> >> Please also Cc
> >> us and net...@vger.kernel.org for BPF kselftests changes.
> >>
> > Thanks for the explanation. So we expect that tools/arch/*/include is in 
> > the searching list, right?
> > The corrent makefile seems not. How do you get this built?
> 
> E.g. take a look at tools/include/asm/barrier.h or 
> tools/include/uapi/asm/bpf_perf_event.h
> just to name two examples. We'd need something similar to this which then 
> points to the
> arch specific includes.
> 
> Thanks,
> Daniel
>
ok, I see. But I am going to skip this fix this time. Because one get fixed, 
another appears.
IMHO, It doesn't sound like a good idea to sync all these files manually. We 
should have
better solution I think.

clang -I. -I./include/uapi -I../../../include/uapi 
-Wno-compare-distinct-pointer-types \
 -O2 -target bpf -emit-llvm -c test_pkt_access.c -o - |  \
llc -march=bpf -mcpu=generic -filetype=obj -o 
/home/changbin/work/linux/tools/testing/selftests/bpf/test_pkt_access.o
In file included from test_pkt_access.c:12:
/usr/include/linux/ip.h:20:10: fatal error: 'asm/byteorder.h' file not found
#include 

 
> > changbin@gvt-dell-host:~/work/linux/tools/testing/selftests/bpf$ make -p
> > [...]
> > clang -I. -I./include/uapi -I../../../include/uapi 
> > -Wno-compare-distinct-pointer-types \
> >  -O2 -target bpf -emit-llvm -c test_pkt_access.c -o - |  \
> > llc -march=bpf -mcpu=generic -filetype=obj -o 
> > /home/changbin/work/linux/tools/testing/selftests/bpf/test_pkt_access.o
> > In file included from test_pkt_access.c:9:
> > In file included from ../../../include/uapi/linux/bpf.h:11:
> > In file included from ./include/uapi/linux/types.h:5:
> > /usr/include/asm-generic/int-ll64.h:11:10: fatal error: 'asm/bitsperlong.h' 
> > file not found
> > #include 
> > 
> > 
> 

-- 
Thanks,
Changbin Du

Re: [PATCH] Documentation: fix reST markup error in driver-api/usb/typec.rst

[Du, Changbin]

On Sun, Apr 08, 2018 at 09:19:58AM +0200, Greg KH wrote:
> On Sun, Apr 08, 2018 at 10:47:12AM +0800, changbin...@intel.com wrote:
> > From: Changbin Du 
> > 
> > There is an format error in driver-api/usb/typec.rst that breaks sphinx
> > docs building.
> > 
> > reST markup error:
> > /home/changbin/work/linux/Documentation/driver-api/usb/typec.rst:215: 
> > (SEVERE/4) Unexpected section title or transition.
> > 
> > 
> > Documentation/Makefile:68: recipe for target 'htmldocs' failed
> > make[1]: *** [htmldocs] Error 1
> > Makefile:1527: recipe for target 'htmldocs' failed
> > make: *** [htmldocs] Error 2
> > 
> > Signed-off-by: Changbin Du 
> > ---
> >  Documentation/driver-api/usb/typec.rst | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Thanks, someone else already sent this, sorry.  I'll be sending it
> onward after 4.17-rc1 is out.
>
No problem. Thanks for your quick checking!

> greg k-h

-- 
Thanks,
Changbin Du

Re: [PATCH 00/17] Include linux trace docs to Sphinx TOC tree

[Du, Changbin]

On Wed, Mar 07, 2018 at 10:46:49AM -0700, Jonathan Corbet wrote:
> On Tue, 27 Feb 2018 17:43:37 -0500
> Steven Rostedt  wrote:
> 
> > On Tue, 27 Feb 2018 17:34:22 +0800
> > "Du, Changbin"  wrote:
> >
> > > Ten days past, will you accept this serias? Thank you!
> > 
> > Currently I'm very overloaded with other code that needs to get done
> > ASAP, and I need to balance what is critical and what is not. I don't
> > have time to review this, as this isn't critical, and can wait.
> > 
> > If Jon can review it to make sure that it doesn't change the
> > readability of the text, then I'll trust his judgment. 
> 
> So I've spent a while working through the patches.  I think it's a
> well-done RST conversion carried out with a light hand; I do not believe
> there are any readability issues with the resulting text files.
> 
> I will note that the series adds some new build warnings:
> 
> > Documentation/trace/events.rst:45: WARNING: Inline emphasis start-string 
> > without end-string.
> > Documentation/trace/events.rst:49: WARNING: Inline emphasis start-string 
> > without end-string.
> > Documentation/trace/events.rst:193: WARNING: Inline emphasis start-string 
> > without end-string.
> > Documentation/trace/events.rst:114: WARNING: Unknown target name: "common".
> > Documentation/trace/ftrace.rst:2620: WARNING: Inline emphasis start-string 
> > without end-string.
> 
> These point to places where the resulting formatted docs are, in fact,
> incorrect.  I had to append the attached patch to the series to make those
> problems go away.  The warnings are there for a purpose!
> 
> Anyway, with that, the patch series is applied.  Thanks for helping to
> improve the docs, and my apologies for taking so long to get to this.
> 
> jon
> 
I am also appriciated for your review. And I am glade to see these docs can 
appear
in the new beautiful html documentation! Thnak you.

- changbin

Re: [PATCH] scripts/faddr2line: show the code context

[Du, Changbin]

On Wed, May 30, 2018 at 08:01:48AM +1000, NeilBrown wrote:
> On Tue, May 29 2018, Peter Zijlstra wrote:
> 
> > On Tue, May 29, 2018 at 12:07:10PM -0500, Josh Poimboeuf wrote:
> >> Yeah, this change really should have been an optional arg.  It hurt the
> >> readability and compactness of the output.  The above looks good to me.
> >> 
> >> Care to send a proper patch?  If you send it to Linus he might apply it
> >> directly as he did with my original patches.
> >
> > ---
> > From: Peter Zijlstra (Intel) 
> >
> > Commit 6870c0165feaa5 ("scripts/faddr2line: show the code context")
> > radically altered the output format of the faddr2line tool. And while
> > the new list output format might have merrit it broke my vim usage and
> > was hard to read.
> >
> > Make the new format optional; using a '--list' argument and attempt to
> > make the output slightly easier to read by adding a little whitespace to
> > separate the different files and explicitly mark the line in question.
> 
> Not commenting on your code but on the original patch.
> I've recently noticed that ADDR2LINE sometimes outputs
>   (discriminator 2)
> or similar at the end of the line.  This messes up the parsing.
> 
> I hacked it to work so I could keep debugging with
> 
> - local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; 
> $dir_prefix\(\./\)*; ;")
> + local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed -e 
> "s; $dir_prefix\(\./\)*; ;" -e "s/(discriminator [0-9]*)//")
> 
> but someone should probably find out exactly what sort of messages
> ADDR2LINE produces, and make sure they are all parsed correctly.
> (maybe that someone should be me, but not today).
> 
Hi, I have fixed it by commit 78eb0c6356 ("scripts/faddr2line: fix error when
addr2line output contains discriminator") and it is already in the mainline now.
Thank you!

> Thanks,
> NeilBrown
> 
> 
> >
> > Cc: Changbin Du 
> > Acked-by: Josh Poimboeuf 
> > Fixes: 6870c0165feaa5 ("scripts/faddr2line: show the code context")
> > Signed-off-by: Peter Zijlstra (Intel) 
> > ---
> >  scripts/faddr2line | 18 --
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/faddr2line b/scripts/faddr2line
> > index 1876a741087c..a0149db00be7 100755
> > --- a/scripts/faddr2line
> > +++ b/scripts/faddr2line
> > @@ -56,7 +56,7 @@ command -v ${SIZE} >/dev/null 2>&1 || die "size isn't 
> > installed"
> >  command -v ${NM} >/dev/null 2>&1 || die "nm isn't installed"
> >  
> >  usage() {
> > -   echo "usage: faddr2line   ..." 
> > >&2
> > +   echo "usage: faddr2line [--list]   
> > ..." >&2
> > exit 1
> >  }
> >  
> > @@ -166,15 +166,25 @@ __faddr2line() {
> > local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; 
> > $dir_prefix\(\./\)*; ;")
> > [[ -z $file_lines ]] && return
> >  
> > +   if [[ $LIST = 0 ]]; then
> > +   echo "$file_lines" | while read -r line
> > +   do
> > +   echo $line
> > +   done
> > +   DONE=1;
> > +   return
> > +   fi
> > +
> > # show each line with context
> > echo "$file_lines" | while read -r line
> > do
> > +   echo
> > echo $line
> > n=$(echo $line | sed 's/.*:\([0-9]\+\).*/\1/g')
> > n1=$[$n-5]
> > n2=$[$n+5]
> > f=$(echo $line | sed 's/.*at \(.\+\):.*/\1/g')
> > -   awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") 
> > {printf("%d\t%s\n", NR, $0)}' $f
> > +   awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") { 
> > if (NR=='$n') printf(">%d<", NR); else printf(" %d ", NR); printf("\t%s\n", 
> > $0)}' $f
> > done
> >  
> > DONE=1
> > @@ -185,6 +195,10 @@ __faddr2line() {
> >  [[ $# -lt 2 ]] && usage
> >  
> >  objfile=$1
> > +
> > +LIST=0
> > +[[ "$objfile" == "--list" ]] && LIST=1 && shift && objfile=$1
> > +
> >  [[ ! -f $objfile ]] && die "can't find objfile $objfile"
> >  shift
> >  



-- 
Thanks,
Changbin Du

Re: [PATCH] scripts/faddr2line: show the code context

[Du, Changbin]

On Tue, May 29, 2018 at 06:03:32PM +0200, Peter Zijlstra wrote:
> On Mon, Mar 19, 2018 at 03:23:25PM +0800, changbin...@intel.com wrote:
> > From: Changbin Du 
> > 
> > Inspired by gdb command 'list', show the code context of target lines.
> > Here is a example:
> > 
> > $ scripts/faddr2line vmlinux native_write_msr+0x6
> > native_write_msr+0x6/0x20:
> > arch_static_branch at arch/x86/include/asm/msr.h:105
> > 100 return EAX_EDX_VAL(val, low, high);
> > 101 }
> > 102
> > 103 static inline void notrace __wrmsr(unsigned int msr, u32 low, u32 
> > high)
> > 104 {
> > 105 asm volatile("1: wrmsr\n"
> > 106  "2:\n"
> > 107  _ASM_EXTABLE_HANDLE(1b, 2b, 
> > ex_handler_wrmsr_unsafe)
> > 108  : : "c" (msr), "a"(low), "d" (high) : 
> > "memory");
> > 109 }
> > 110
> > (inlined by) static_key_false at include/linux/jump_label.h:142
> > 137 #define JUMP_TYPE_LINKED2UL
> > 138 #define JUMP_TYPE_MASK  3UL
> > 139
> > 140 static __always_inline bool static_key_false(struct static_key *key)
> > 141 {
> > 142 return arch_static_branch(key, false);
> > 143 }
> > 144
> > 145 static __always_inline bool static_key_true(struct static_key *key)
> > 146 {
> > 147 return !arch_static_branch(key, true);
> > (inlined by) native_write_msr at arch/x86/include/asm/msr.h:150
> > 145 static inline void notrace
> > 146 native_write_msr(unsigned int msr, u32 low, u32 high)
> > 147 {
> > 148 __wrmsr(msr, low, high);
> > 149
> > 150 if (msr_tracepoint_active(__tracepoint_write_msr))
> > 151 do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
> > 152 }
> > 153
> > 154 /* Can be uninlined because referenced by paravirt */
> > 155 static inline int notrace
> 
> Not a fan of this :-/ And you didn't even make it optional. Nor did you
> Cc the original author of the tool.
Yeah, understand your compatibility concern, and thanks for your improvment.
I only added people from 'scripts/get_maintainer.pl'.


-- 
Thanks,
Changbin Du

Re: kernel BUG at arch/x86/kvm/x86.c:LINE! (2)

[Du Changbin]

try/entry_64.S:993
RIP: 0010:kvm_spurious_fault+0x9/0x10 arch/x86/kvm/x86.c:353
Code: 45 10 50 e8 e9 44 7c 00 58 5a 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f
5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 e8 97 03 73 00 <0f> 0b 0f 1f 44 00
00 55 48 89 e5 41 57 41 56 41 55 41 89 fd 41 54
RSP: 0018:88018b397448 EFLAGS: 00010212
RAX: 0004 RBX: 110031672e8d RCX: c9000628e000
RDX: 0417 RSI: 810bd1f9 RDI: 88018b397488
RBP: 88018b397448 R08: 8801cc2f2180 R09: 8801c308d000
R10: ed0038611bff R11: 8801c308dfff R12: 88018b3974c8
R13: dc00 R14: 8801c308d000 R15: 88018b397488
 kvm_fastop_exception+0x50b/0x5455
 loaded_vmcs_init arch/x86/kvm/vmx.c:2129 [inline]
 alloc_loaded_vmcs+0x7f/0x280 arch/x86/kvm/vmx.c:4766
 vmx_create_vcpu+0x20e/0x25e0 arch/x86/kvm/vmx.c:11025
 kvm_arch_vcpu_create+0xe5/0x220 arch/x86/kvm/x86.c:8471
 kvm_vm_ioctl_create_vcpu arch/x86/kvm/../../../virt/kvm/kvm_main.c:2476
[inline]
 kvm_vm_ioctl+0x470/0x1d40 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2977
 kvm_vm_compat_ioctl+0x143/0x430
arch/x86/kvm/../../../virt/kvm/kvm_main.c:3170
 __do_compat_sys_ioctl fs/compat_ioctl.c:1419 [inline]
 __se_compat_sys_ioctl fs/compat_ioctl.c:1365 [inline]
 __ia32_compat_sys_ioctl+0x20e/0x630 fs/compat_ioctl.c:1365
 do_syscall_32_irqs_on arch/x86/entry/common.c:326 [inline]
 do_fast_syscall_32+0x34d/0xfb2 arch/x86/entry/common.c:397
 entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139
RIP: 0023:0xf7fecca9
Code: 85 d2 74 02 89 0a 5b 5d c3 8b 04 24 c3 8b 0c 24 c3 8b 1c 24 c3 90 90
90 90 90 90 90 90 90 90 90 90 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90
90 90 eb 0d 90 90 90 90 90 90 90 90 90 90 90 90
RSP: 002b:f5fa60cc EFLAGS: 0296 ORIG_RAX: 0036
RAX: ffda RBX: 000c RCX: ae41
RDX:  RSI:  RDI: 
RBP:  R08:  R09: 
R10:  R11:  R12: 
R13:  R14:  R15: 
Modules linked in:
---[ end trace 1c8fec48833612c0 ]---
RIP: 0010:kvm_spurious_fault+0x9/0x10 arch/x86/kvm/x86.c:353
Code: 45 10 50 e8 e9 44 7c 00 58 5a 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f
5d c3 0f 1f 84 00 00 00 00 00 55 48 89 e5 e8 97 03 73 00 <0f> 0b 0f 1f 44 00
00 55 48 89 e5 41 57 41 56 41 55 41 89 fd 41 54
RSP: 0018:8801dae07bd8 EFLAGS: 00010006
RAX: 8801cc2f2180 RBX: 11003b5c0f7f RCX: 81385bcc
RDX: 0001 RSI: 810bd1f9 RDI: 8801dae07c18
RBP: 8801dae07bd8 R08: 8801cc2f2180 R09: ed003b5c5ba0
R10: ed003b5c5ba0 R11: 8801dae2dd07 R12: 8801dae07c58
R13: dc00 R14: 8801beccc000 R15: 8801dae07c18
FS:  () GS:8801dae0(0063) knlGS:f5fa6b40
CS:  0010 DS: 002b ES: 002b CR0: 80050033
CR2: 8801dae07c18 CR3: 0001cc8d1000 CR4: 001426f0
DR0:  DR1:  DR2: 
DR3:  DR6: fffe0ff0 DR7: 0400


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkal...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.


--
Thanks,
Du Changbin

[PATCH] scripts/gdb: fix lx-version for gdb 7.3-

[Du Changbin]

For gdb version less than 7.3, lx-version only one character.
(gdb) lx-version
L(gdb)

This can be fixed by casting 'linux_banner' as (char *).
(gdb) lx-version
Linux version 4.19.0-rc1+ (changbin@acer) (gcc version 7.3.0 (Ubuntu 
7.3.0-16ubuntu3)) #21 SMP Sat Sep 1 21:43:30 CST 2018

gdb 7.4 seems to be no such issue.

Signed-off-by: Du Changbin 
---
 scripts/gdb/linux/proc.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/gdb/linux/proc.py b/scripts/gdb/linux/proc.py
index 086d27223c0c..0aebd7565b03 100644
--- a/scripts/gdb/linux/proc.py
+++ b/scripts/gdb/linux/proc.py
@@ -41,7 +41,7 @@ class LxVersion(gdb.Command):
 
 def invoke(self, arg, from_tty):
 # linux_banner should contain a newline
-gdb.write(gdb.parse_and_eval("linux_banner").string())
+gdb.write(gdb.parse_and_eval("(char *)linux_banner").string())
 
 LxVersion()
 
-- 
2.17.1

[PATCH v2] scripts/gdb: fix lx-version for gdb 7.3-

[Du Changbin]

For gdb version less than 7.3, lx-version only prints one character.
(gdb) lx-version
L(gdb)

This can be fixed by casting 'linux_banner' as (char *).
(gdb) lx-version
Linux version 4.19.0-rc1+ (changbin@acer) (gcc version 7.3.0 (Ubuntu 
7.3.0-16ubuntu3)) #21 SMP Sat Sep 1 21:43:30 CST 2018

gdb 7.4 seems to be no such issue.

Signed-off-by: Du Changbin 
---
 scripts/gdb/linux/proc.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/gdb/linux/proc.py b/scripts/gdb/linux/proc.py
index 086d27223c0c..0aebd7565b03 100644
--- a/scripts/gdb/linux/proc.py
+++ b/scripts/gdb/linux/proc.py
@@ -41,7 +41,7 @@ class LxVersion(gdb.Command):
 
 def invoke(self, arg, from_tty):
 # linux_banner should contain a newline
-gdb.write(gdb.parse_and_eval("linux_banner").string())
+gdb.write(gdb.parse_and_eval("(char *)linux_banner").string())
 
 LxVersion()
 
-- 
2.17.1

Re: [PATCH] PCI: make pci_size() return real size

[Du Changbin]

Hi Bjorn. Have you checked this little improvment? The idea here is that
this is not a hotspot, so readbility matters than trick. Thanks!

On Sat, Oct 13, 2018 at 08:49:19AM +0800, changbin...@gmail.com wrote:
> From: Du Changbin 
> 
> Currently, the pci_size() function actually return 'size-1'.
> Make it return real size to avoid confusing.
> 
> Signed-off-by: Du Changbin 
> ---
>  drivers/pci/probe.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 201f9e5ff55c..8ff2b1413865 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -121,13 +121,13 @@ static u64 pci_size(u64 base, u64 maxbase, u64 mask)
>* Get the lowest of them to find the decode size, and from that
>* the extent.
>*/
> - size = (size & ~(size-1)) - 1;
> + size = size & ~(size-1);
>  
>   /*
>* base == maxbase can be valid only if the BAR has already been
>* programmed with all 1s.
>*/
> - if (base == maxbase && ((base | size) & mask) != mask)
> + if (base == maxbase && ((base | (size - 1)) & mask) != mask)
>   return 0;
>  
>   return size;
> @@ -278,7 +278,7 @@ int __pci_read_base(struct pci_dev *dev, enum 
> pci_bar_type type,
>   /* Above 32-bit boundary; try to reallocate */
>   res->flags |= IORESOURCE_UNSET;
>   res->start = 0;
> - res->end = sz64;
> + res->end = sz64 - 1;
>   pci_info(dev, "reg 0x%x: can't handle BAR above 4GB 
> (bus address %#010llx)\n",
>pos, (unsigned long long)l64);
>   goto out;
> @@ -286,7 +286,7 @@ int __pci_read_base(struct pci_dev *dev, enum 
> pci_bar_type type,
>   }
>  
>   region.start = l64;
> - region.end = l64 + sz64;
> + region.end = l64 + sz64 - 1;
>  
>   pcibios_bus_to_resource(dev->bus, res, ®ion);
>   pcibios_resource_to_bus(dev->bus, &inverted_region, res);
> -- 
> 2.17.1
> 

-- 
Thanks,
Du Changbin

[PATCH v3] scripts/gdb: fix lx-version

[Du Changbin]

For current gdb version (has tested with 7.3 and 8.1), 'lx-version'
only prints one character.
(gdb) lx-version
L(gdb)

This can be fixed by casting 'linux_banner' as (char *).
(gdb) lx-version
Linux version 4.19.0-rc1+ (changbin@acer) (gcc version 7.3.0 (Ubuntu 
7.3.0-16ubuntu3)) #21 SMP Sat Sep 1 21:43:30 CST 2018

Signed-off-by: Du Changbin 
---
 scripts/gdb/linux/proc.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/gdb/linux/proc.py b/scripts/gdb/linux/proc.py
index 086d27223c0c..0aebd7565b03 100644
--- a/scripts/gdb/linux/proc.py
+++ b/scripts/gdb/linux/proc.py
@@ -41,7 +41,7 @@ class LxVersion(gdb.Command):
 
 def invoke(self, arg, from_tty):
 # linux_banner should contain a newline
-gdb.write(gdb.parse_and_eval("linux_banner").string())
+gdb.write(gdb.parse_and_eval("(char *)linux_banner").string())
 
 LxVersion()
 
-- 
2.17.1

[PATCH 1/4] x86/mm: surround level4_kernel_pgt with #ifdef CONFIG_X86_5LEVEL...#endif

[Du Changbin]

From: Changbin Du 

The level4_kernel_pgt is only defined when X86_5LEVEL is enabled. So
surround level4_kernel_pgt with #ifdef CONFIG_X86_5LEVEL...#endif to
make code correct.

Signed-off-by: Changbin Du 
Acked-by: Steven Rostedt (VMware) 
---
 arch/x86/include/asm/pgtable_64.h |  2 ++
 arch/x86/kernel/head64.c  | 13 ++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_64.h 
b/arch/x86/include/asm/pgtable_64.h
index 9c85b54bf03c..9333f7fa5bdb 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -16,7 +16,9 @@
 #include 
 #include 
 
+#ifdef CONFIG_X86_5LEVEL
 extern p4d_t level4_kernel_pgt[512];
+#endif
 extern p4d_t level4_ident_pgt[512];
 extern pud_t level3_kernel_pgt[512];
 extern pud_t level3_ident_pgt[512];
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index ddee1f0870c4..4a59ef93c258 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -151,16 +151,15 @@ unsigned long __head __startup_64(unsigned long physaddr,
 
pgd = fixup_pointer(&early_top_pgt, physaddr);
p = pgd + pgd_index(__START_KERNEL_map);
-   if (la57)
-   *p = (unsigned long)level4_kernel_pgt;
-   else
-   *p = (unsigned long)level3_kernel_pgt;
-   *p += _PAGE_TABLE_NOENC - __START_KERNEL_map + load_delta;
-
+#ifdef CONFIG_X86_5LEVEL
if (la57) {
+   *p = (unsigned long)level4_kernel_pgt;
p4d = fixup_pointer(&level4_kernel_pgt, physaddr);
p4d[511] += load_delta;
-   }
+   } else
+#endif
+   *p = (unsigned long)level3_kernel_pgt;
+   *p += _PAGE_TABLE_NOENC - __START_KERNEL_map + load_delta;
 
pud = fixup_pointer(&level3_kernel_pgt, physaddr);
pud[510] += load_delta;
-- 
2.17.1

[PATCH 2/4] kernel hacking: new config NO_AUTO_INLINE to disable compiler auto-inline optimizations

[Du Changbin]

From: Changbin Du 

This patch add a new kernel hacking option NO_AUTO_INLINE. Selecting
this option will prevent the compiler from optimizing the kernel by
auto-inlining functions not marked with the inline keyword.

With this option, only functions explicitly marked with "inline" will
be inlined. This will allow the function tracer to trace more functions
because it only traces functions that the compiler has not inlined.

Signed-off-by: Changbin Du 
Acked-by: Steven Rostedt (VMware) 
---
 Makefile  |  6 ++
 lib/Kconfig.debug | 25 +
 2 files changed, 31 insertions(+)

diff --git a/Makefile b/Makefile
index e8b599b4dcde..757d6507cb5c 100644
--- a/Makefile
+++ b/Makefile
@@ -749,6 +749,12 @@ KBUILD_CFLAGS  += $(call cc-option, 
-femit-struct-debug-baseonly) \
   $(call cc-option,-fno-var-tracking)
 endif
 
+ifdef CONFIG_NO_AUTO_INLINE
+KBUILD_CFLAGS   += $(call cc-option, -fno-inline-functions) \
+  $(call cc-option, -fno-inline-small-functions) \
+  $(call cc-option, -fno-inline-functions-called-once)
+endif
+
 ifdef CONFIG_FUNCTION_TRACER
 ifdef CONFIG_FTRACE_MCOUNT_RECORD
   # gcc 5 supports generating the mcount tables directly
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4966c4fbe7f7..0f9b4fa78b1c 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -211,6 +211,31 @@ config GDB_SCRIPTS
  instance. See Documentation/dev-tools/gdb-kernel-debugging.rst
  for further details.
 
+config NO_AUTO_INLINE
+   bool "Disable compiler auto-inline optimizations"
+   help
+ This will prevent the compiler from optimizing the kernel by
+ auto-inlining functions not marked with the inline keyword.
+ With this option, only functions explicitly marked with
+ "inline" will be inlined. This will allow the function tracer
+ to trace more functions because it only traces functions that
+ the compiler has not inlined.
+
+ Enabling this function can help debugging a kernel if using
+ the function tracer. But it can also change how the kernel
+ works, because inlining functions may change the timing,
+ which could make it difficult while debugging race conditions.
+
+ If unsure, select N.
+
+config ENABLE_WARN_DEPRECATED
+   bool "Enable __deprecated logic"
+   default y
+   help
+ Enable the __deprecated logic in the kernel build.
+ Disable this to suppress the "warning: 'foo' is deprecated
+ (declared at kernel/power/somefile.c:1234)" messages.
+
 config ENABLE_MUST_CHECK
bool "Enable __must_check logic"
default y
-- 
2.17.1

[PATCH 0/4] kernel hacking: GCC optimization for better debug experience (-Og)

[Du Changbin]

Hi all,
I have posted this series several months ago but interrupted by personal
affairs. Now I get time to complete this task. Thanks for all of the
reviewers.

I know some kernel developers was searching for a method to dissable GCC
optimizations, probably they want to apply GCC '-O0' option. But since Linux
kernel replys on GCC optimization to remove some dead code, so '-O0' just
breaks the build. They do need this because they want to debug kernel with
qemu, simics, kgtp or kgdb.

Thanks for the GCC '-Og' optimization level introduced in GCC 4.8, which
offers a reasonable level of optimization while maintaining fast compilation
and a good debugging experience. It is similar to '-O1' while perferring to
keep debug ability over runtime speed. With '-Og', we can build a kernel with
better debug ability and little performance drop after some simple change.

In this series, firstly introduce a new config CONFIG_NO_AUTO_INLINE after two
fixes for this new option. With this option, only functions explicitly marked
with "inline" will  be inlined. This will allow the function tracer to trace
more functions because it only traces functions that the compiler has not
inlined.

Then introduce new config CC_OPTIMIZE_FOR_DEBUGGING which apply '-Og'
optimization level for whole kernel, with a simple fix in fix_to_virt().
Currently I have only tested this option on x86 and ARM platform. Other
platforms should also work but probably need some compiling fixes as what
having done in this series. I leave that to who want to try this debug
option.

Comparison of vmlinux size: a bit smaller.

w/o CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ size vmlinux
   textdata bss dec hex filename
22665554   9709674  2920908 3529613621a9388 vmlinux

w/ CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ size vmlinux
   textdata bss dec hex filename
21499032   10102758 2920908 3452269820ec64a vmlinux


Comparison of system performance: a bit drop (~6%).
This benchmark of kernel compilation is suggested by Ingo Molnar.
https://lkml.org/lkml/2018/5/2/74

Preparation: Set cpufreq to 'performance'.
for ((cpu=0; cpu<120; cpu++)); do
  G=/sys/devices/system/cpu/cpu$cpu/cpufreq/scaling_governor
  [ -f $G ] && echo performance > $G
done

w/o CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ perf stat --repeat 5 --null --pre '\
cp -a kernel ../kernel.copy.$(date +%s); \
rm -rf *;\
git checkout .;  \
echo 1 > /proc/sys/vm/drop_caches;   \
find ../kernel* -type f | xargs cat >/dev/null;  \
make -j kernel >/dev/null;   \
make clean >/dev/null 2>&1;  \
sync'\
 \
make -j8 >/dev/null

 Performance counter stats for 'make -j8' (5 runs):

219.764246652 seconds time elapsed   ( +-  0.78% )

w/ CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ perf stat --repeat 5 --null --pre '\
cp -a kernel ../kernel.copy.$(date +%s); \
rm -rf *;\
git checkout .;  \
echo 1 > /proc/sys/vm/drop_caches;   \
find ../kernel* -type f | xargs cat >/dev/null;  \
make -j kernel >/dev/null;   \
make clean >/dev/null 2>&1;  \
sync'\
 \
make -j8 >/dev/null

Performance counter stats for 'make -j8' (5 runs):

 233.574187771 seconds time elapsed  ( +-  0.19% )


Changbin Du (4):
  x86/mm: surround level4_kernel_pgt with #ifdef
CONFIG_X86_5LEVEL...#endif
  kernel hacking: new config NO_AUTO_INLINE to disable compiler
auto-inline optimizations
  ARM: mm: fix build error in fix_to_virt with
CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
  kernel hacking: new config CC_OPTIMIZE_FOR_DEBUGGING to apply GCC -Og
optimization

 Makefile  | 11 +++
 arch/arm/mm/mmu.c |  2 +-
 arch/x86/include/asm/pgtable_64.h |  2 ++
 arch/x86/kernel/head64.c  | 13 ++---
 include/linux/compiler-gcc.h  |  2 +-
 include/linux/compiler.h  |  2 +-
 init/Kconfig  | 19 +++
 lib/Kconfig.debug | 25 +
 8 files changed, 66 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH 4/4] kernel hacking: new config CC_OPTIMIZE_FOR_DEBUGGING to apply GCC -Og optimization

[Du Changbin]

From: Changbin Du 

This will apply GCC '-Og' optimization level which is supported
since GCC 4.8. This optimization level offers a reasonable level
of optimization while maintaining fast compilation and a good
debugging experience. It is similar to '-O1' while perferring
to keep debug ability over runtime speed.

If enabling this option breaks your kernel, you should either
disable this or find a fix (mostly in the arch code). Currently
this option has only been tested on x86_64 and arm platform.

This option can satisfy people who was searching for a method
to disable compiler optimizations so to achieve better kernel
debugging experience with kgdb or qemu.

The main problem of '-Og' is we must not use __attribute__((error(msg))).
The compiler will report error though the call to error function
still can be optimize out. So we must fallback to array tricky.

Comparison of vmlinux size: a bit smaller.

w/o CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ size vmlinux
   textdata bss dec hex filename
22665554   9709674  2920908 3529613621a9388 vmlinux

w/ CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ size vmlinux
   textdata bss dec hex filename
21499032   10102758 2920908 3452269820ec64a vmlinux

Comparison of system performance: a bit drop (~6%).
This benchmark of kernel compilation is suggested by Ingo Molnar.
https://lkml.org/lkml/2018/5/2/74

Preparation: Set cpufreq to 'performance'.
for ((cpu=0; cpu<120; cpu++)); do
  G=/sys/devices/system/cpu/cpu$cpu/cpufreq/scaling_governor
  [ -f $G ] && echo performance > $G
done

w/o CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ perf stat --repeat 5 --null --pre '\
cp -a kernel ../kernel.copy.$(date +%s); \
rm -rf *;\
git checkout .;  \
echo 1 > /proc/sys/vm/drop_caches;   \
find ../kernel* -type f | xargs cat >/dev/null;  \
make -j kernel >/dev/null;   \
make clean >/dev/null 2>&1;  \
sync'\
 \
make -j8 >/dev/null

Performance counter stats for 'make -j8' (5 runs):

219.764246652 seconds time elapsed   ( +-  0.78% )

w/ CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ perf stat --repeat 5 --null --pre '\
cp -a kernel ../kernel.copy.$(date +%s); \
rm -rf *;\
git checkout .;  \
echo 1 > /proc/sys/vm/drop_caches;   \
find ../kernel* -type f | xargs cat >/dev/null;  \
make -j kernel >/dev/null;   \
make clean >/dev/null 2>&1;  \
sync'\
 \
make -j8 >/dev/null

Performance counter stats for 'make -j8' (5 runs):

 233.574187771 seconds time elapsed  ( +-  0.19% )

Signed-off-by: Changbin Du 
Acked-by: Steven Rostedt (VMware) 
---
 Makefile |  5 +
 include/linux/compiler-gcc.h |  2 +-
 include/linux/compiler.h |  2 +-
 init/Kconfig | 19 +++
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 757d6507cb5c..0e747fafb53b 100644
--- a/Makefile
+++ b/Makefile
@@ -657,6 +657,10 @@ KBUILD_CFLAGS  += $(call cc-disable-warning, 
format-truncation)
 KBUILD_CFLAGS  += $(call cc-disable-warning, format-overflow)
 KBUILD_CFLAGS  += $(call cc-disable-warning, int-in-bool-context)
 
+ifdef CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
+KBUILD_CFLAGS  += $(call cc-option, -Og)
+KBUILD_CFLAGS   += $(call cc-disable-warning,maybe-uninitialized,)
+else
 ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
 KBUILD_CFLAGS  += $(call cc-option,-Oz,-Os)
 KBUILD_CFLAGS  += $(call cc-disable-warning,maybe-uninitialized,)
@@ -667,6 +671,7 @@ else
 KBUILD_CFLAGS   += -O2
 endif
 endif
+endif
 
 KBUILD_CFLAGS += $(call cc-ifversion, -lt, 0409, \
$(call cc-disable-warning,maybe-uninitialized,))
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 4d36b27214fd..2a76f7c64b54 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -85,7 +85,7 @@
 
 #define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 
-#ifndef __CHECKER__
+#if !defined(__CHECKER__) && !defined(CONFIG_CC_OPTIMIZE_FOR_DEBUGGING)
 #define __compiletime_warning(message) __attribute__((warning(message)))
 #define __compiletime_error(message) __attribute__((error(message)))
 
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 681d866efb1e..9385c62e9f00 100644
--- a/include/linux/compiler.h
+++ b/include/l

[PATCH 3/4] ARM: mm: fix build error in fix_to_virt with CONFIG_CC_OPTIMIZE_FOR_DEBUGGING

[Du Changbin]

From: Changbin Du 

With '-Og' optimization level, GCC would not optimize a count for a loop
as a constant value. But BUILD_BUG_ON() only accept compile-time constant
values. Let's use __fix_to_virt() to avoid the error.

arch/arm/mm/mmu.o: In function `fix_to_virt':
/home/changbin/work/linux/./include/asm-generic/fixmap.h:31: undefined 
reference to `__compiletime_assert_31'
Makefile:1051: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1

Signed-off-by: Changbin Du 
Acked-by: Steven Rostedt (VMware) 
---
 arch/arm/mm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e46a6a446cdd..c08d74e76714 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1599,7 +1599,7 @@ static void __init early_fixmap_shutdown(void)
pte_t *pte;
struct map_desc map;
 
-   map.virtual = fix_to_virt(i);
+   map.virtual = __fix_to_virt(i);
pte = pte_offset_early_fixmap(pmd_off_k(map.virtual), 
map.virtual);
 
/* Only i/o device mappings are supported ATM */
-- 
2.17.1

Re: [PATCH 2/4] kernel hacking: new config NO_AUTO_INLINE to disable compiler auto-inline optimizations

[Du Changbin]

On Thu, Oct 18, 2018 at 05:57:56PM +0100, Robin Murphy wrote:
> On 18/10/18 17:25, Du Changbin wrote:
> > From: Changbin Du 
> > 
> > This patch add a new kernel hacking option NO_AUTO_INLINE. Selecting
> > this option will prevent the compiler from optimizing the kernel by
> > auto-inlining functions not marked with the inline keyword.
> > 
> > With this option, only functions explicitly marked with "inline" will
> > be inlined. This will allow the function tracer to trace more functions
> > because it only traces functions that the compiler has not inlined.
> > 
> > Signed-off-by: Changbin Du 
> > Acked-by: Steven Rostedt (VMware) 
> > ---
> >   Makefile  |  6 ++
> >   lib/Kconfig.debug | 25 +
> >   2 files changed, 31 insertions(+)
> > 
> > diff --git a/Makefile b/Makefile
> > index e8b599b4dcde..757d6507cb5c 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -749,6 +749,12 @@ KBUILD_CFLAGS  += $(call cc-option, 
> > -femit-struct-debug-baseonly) \
> >$(call cc-option,-fno-var-tracking)
> >   endif
> > +ifdef CONFIG_NO_AUTO_INLINE
> > +KBUILD_CFLAGS   += $(call cc-option, -fno-inline-functions) \
> > +  $(call cc-option, -fno-inline-small-functions) \
> > +  $(call cc-option, -fno-inline-functions-called-once)
> > +endif
> > +
> >   ifdef CONFIG_FUNCTION_TRACER
> >   ifdef CONFIG_FTRACE_MCOUNT_RECORD
> > # gcc 5 supports generating the mcount tables directly
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 4966c4fbe7f7..0f9b4fa78b1c 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -211,6 +211,31 @@ config GDB_SCRIPTS
> >   instance. See Documentation/dev-tools/gdb-kernel-debugging.rst
> >   for further details.
> > +config NO_AUTO_INLINE
> > +   bool "Disable compiler auto-inline optimizations"
> > +   help
> > + This will prevent the compiler from optimizing the kernel by
> > + auto-inlining functions not marked with the inline keyword.
> > + With this option, only functions explicitly marked with
> > + "inline" will be inlined. This will allow the function tracer
> > + to trace more functions because it only traces functions that
> > + the compiler has not inlined.
> > +
> > + Enabling this function can help debugging a kernel if using
> > + the function tracer. But it can also change how the kernel
> > + works, because inlining functions may change the timing,
> > + which could make it difficult while debugging race conditions.
> > +
> > + If unsure, select N.
> > +
> > +config ENABLE_WARN_DEPRECATED
> 
> This part doesn't look like it belongs in this patch, and judging by the
> commit message in 771c035372a0 wouldn't be welcome back anyway.
> 
opps, this is a rebasing mistake. Let me update it. Thanks.

> Robin.
> 
> > +   bool "Enable __deprecated logic"
> > +   default y
> > +   help
> > + Enable the __deprecated logic in the kernel build.
> > + Disable this to suppress the "warning: 'foo' is deprecated
> > + (declared at kernel/power/somefile.c:1234)" messages.
> > +
> >   config ENABLE_MUST_CHECK
> > bool "Enable __must_check logic"
> > default y
> > 

-- 
Thanks,
Du Changbin

Re: [PATCH 2/4] kernel hacking: new config NO_AUTO_INLINE to disable compiler auto-inline optimizations

[Du Changbin]

On Thu, Oct 18, 2018 at 12:59:48PM -0400, Steven Rostedt wrote:
> On Thu, 18 Oct 2018 16:25:46 +
> Du Changbin  wrote:
> 
> > From: Changbin Du 
> > 
> > This patch add a new kernel hacking option NO_AUTO_INLINE. Selecting
> > this option will prevent the compiler from optimizing the kernel by
> > auto-inlining functions not marked with the inline keyword.
> > 
> > With this option, only functions explicitly marked with "inline" will
> > be inlined. This will allow the function tracer to trace more functions
> > because it only traces functions that the compiler has not inlined.
> > 
> > Signed-off-by: Changbin Du 
> > Acked-by: Steven Rostedt (VMware) 
> 
> I have acked patch this before, but this particular patch has extra
> changes that I have not acked.
>
Steven, no extra changes made. I just wronly rebased it on top of mainline.
:)

> 
> 
> > +config ENABLE_WARN_DEPRECATED
> > +   bool "Enable __deprecated logic"
> > +   default y
> > +   help
> > + Enable the __deprecated logic in the kernel build.
> > + Disable this to suppress the "warning: 'foo' is deprecated
> > + (declared at kernel/power/somefile.c:1234)" messages.
> > +
> 
> What is that?
> 
> -- Steve

-- 
Thanks,
Du Changbin

[PATCH v2 0/4] kernel hacking: GCC optimization for better debug experience (-Og)

[Du Changbin]

Hi all,
I have posted this series several months ago but interrupted by personal
affairs. Now I get time to complete this task. Thanks for all of the
reviewers.

I know some kernel developers was searching for a method to dissable GCC
optimizations, probably they want to apply GCC '-O0' option. But since Linux
kernel relies on GCC optimization to remove some dead code, so '-O0' just
breaks the build. They do need this because they want to debug kernel with
qemu, simics, kgtp or kgdb.

Thanks for the GCC '-Og' optimization level introduced in GCC 4.8, which
offers a reasonable level of optimization while maintaining fast compilation
and a good debugging experience. It is similar to '-O1' while perferring to
keep debug ability over runtime speed. With '-Og', we can build a kernel with
better debug ability and little performance drop after some simple change.

In this series, firstly introduce a new config CONFIG_NO_AUTO_INLINE after two
fixes for this new option. With this option, only functions explicitly marked
with "inline" will  be inlined. This will allow the function tracer to trace
more functions because it only traces functions that the compiler has not
inlined.

Then introduce new config CC_OPTIMIZE_FOR_DEBUGGING which apply '-Og'
optimization level for whole kernel, with a simple fix in fix_to_virt().
Currently I have only tested this option on x86 and ARM platform. Other
platforms should also work but probably need some compiling fixes as what
having done in this series. I leave that to who want to try this debug
option.

Comparison of vmlinux size: a bit smaller.

w/o CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ size vmlinux
   textdata bss dec hex filename
22665554   9709674  2920908 3529613621a9388 vmlinux

w/ CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ size vmlinux
   textdata bss dec hex filename
21499032   10102758 2920908 3452269820ec64a vmlinux


Comparison of system performance: a bit drop (~6%).
This benchmark of kernel compilation is suggested by Ingo Molnar.
https://lkml.org/lkml/2018/5/2/74

Preparation: Set cpufreq to 'performance'.
for ((cpu=0; cpu<120; cpu++)); do
  G=/sys/devices/system/cpu/cpu$cpu/cpufreq/scaling_governor
  [ -f $G ] && echo performance > $G
done

w/o CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ perf stat --repeat 5 --null --pre '\
cp -a kernel ../kernel.copy.$(date +%s); \
rm -rf *;\
git checkout .;  \
echo 1 > /proc/sys/vm/drop_caches;   \
find ../kernel* -type f | xargs cat >/dev/null;  \
make -j kernel >/dev/null;   \
make clean >/dev/null 2>&1;  \
sync'\
 \
make -j8 >/dev/null

 Performance counter stats for 'make -j8' (5 runs):

219.764246652 seconds time elapsed   ( +-  0.78% )

w/ CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ perf stat --repeat 5 --null --pre '\
cp -a kernel ../kernel.copy.$(date +%s); \
rm -rf *;\
git checkout .;  \
echo 1 > /proc/sys/vm/drop_caches;   \
find ../kernel* -type f | xargs cat >/dev/null;  \
make -j kernel >/dev/null;   \
make clean >/dev/null 2>&1;  \
sync'\
 \
make -j8 >/dev/null

Performance counter stats for 'make -j8' (5 runs):

 233.574187771 seconds time elapsed  ( +-  0.19% )


Du Changbin (4):
  x86/mm: surround level4_kernel_pgt with #ifdef
CONFIG_X86_5LEVEL...#endif
  kernel hacking: new config NO_AUTO_INLINE to disable compiler
auto-inline optimizations
  ARM: mm: fix build error in fix_to_virt with
CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
  kernel hacking: new config CC_OPTIMIZE_FOR_DEBUGGING to apply GCC -Og
optimization

 Makefile  | 11 +++
 arch/arm/mm/mmu.c |  2 +-
 arch/x86/include/asm/pgtable_64.h |  2 ++
 arch/x86/kernel/head64.c  | 13 ++---
 include/linux/compiler-gcc.h  |  2 +-
 include/linux/compiler.h  |  2 +-
 init/Kconfig  | 19 +++
 lib/Kconfig.debug | 17 +
 8 files changed, 58 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] x86/mm: surround level4_kernel_pgt with #ifdef CONFIG_X86_5LEVEL...#endif

[Du Changbin]

The level4_kernel_pgt is only defined when X86_5LEVEL is enabled. So
surround level4_kernel_pgt with #ifdef CONFIG_X86_5LEVEL...#endif to
make code correct.

Signed-off-by: Du Changbin 
Acked-by: Steven Rostedt (VMware) 
---
 arch/x86/include/asm/pgtable_64.h |  2 ++
 arch/x86/kernel/head64.c  | 13 ++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/pgtable_64.h 
b/arch/x86/include/asm/pgtable_64.h
index 9c85b54bf03c..9333f7fa5bdb 100644
--- a/arch/x86/include/asm/pgtable_64.h
+++ b/arch/x86/include/asm/pgtable_64.h
@@ -16,7 +16,9 @@
 #include 
 #include 
 
+#ifdef CONFIG_X86_5LEVEL
 extern p4d_t level4_kernel_pgt[512];
+#endif
 extern p4d_t level4_ident_pgt[512];
 extern pud_t level3_kernel_pgt[512];
 extern pud_t level3_ident_pgt[512];
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index ddee1f0870c4..4a59ef93c258 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -151,16 +151,15 @@ unsigned long __head __startup_64(unsigned long physaddr,
 
pgd = fixup_pointer(&early_top_pgt, physaddr);
p = pgd + pgd_index(__START_KERNEL_map);
-   if (la57)
-   *p = (unsigned long)level4_kernel_pgt;
-   else
-   *p = (unsigned long)level3_kernel_pgt;
-   *p += _PAGE_TABLE_NOENC - __START_KERNEL_map + load_delta;
-
+#ifdef CONFIG_X86_5LEVEL
if (la57) {
+   *p = (unsigned long)level4_kernel_pgt;
p4d = fixup_pointer(&level4_kernel_pgt, physaddr);
p4d[511] += load_delta;
-   }
+   } else
+#endif
+   *p = (unsigned long)level3_kernel_pgt;
+   *p += _PAGE_TABLE_NOENC - __START_KERNEL_map + load_delta;
 
pud = fixup_pointer(&level3_kernel_pgt, physaddr);
pud[510] += load_delta;
-- 
2.17.1

[PATCH v2 2/4] kernel hacking: new config NO_AUTO_INLINE to disable compiler auto-inline optimizations

[Du Changbin]

This patch add a new kernel hacking option NO_AUTO_INLINE. Selecting
this option will prevent the compiler from optimizing the kernel by
auto-inlining functions not marked with the inline keyword.

With this option, only functions explicitly marked with "inline" will
be inlined. This will allow the function tracer to trace more functions
because it only traces functions that the compiler has not inlined.

Signed-off-by: Du Changbin 
Acked-by: Steven Rostedt (VMware) 
---
 Makefile  |  6 ++
 lib/Kconfig.debug | 17 +
 2 files changed, 23 insertions(+)

diff --git a/Makefile b/Makefile
index e8b599b4dcde..757d6507cb5c 100644
--- a/Makefile
+++ b/Makefile
@@ -749,6 +749,12 @@ KBUILD_CFLAGS  += $(call cc-option, 
-femit-struct-debug-baseonly) \
   $(call cc-option,-fno-var-tracking)
 endif
 
+ifdef CONFIG_NO_AUTO_INLINE
+KBUILD_CFLAGS   += $(call cc-option, -fno-inline-functions) \
+  $(call cc-option, -fno-inline-small-functions) \
+  $(call cc-option, -fno-inline-functions-called-once)
+endif
+
 ifdef CONFIG_FUNCTION_TRACER
 ifdef CONFIG_FTRACE_MCOUNT_RECORD
   # gcc 5 supports generating the mcount tables directly
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4966c4fbe7f7..c7c28ee01dfc 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -211,6 +211,23 @@ config GDB_SCRIPTS
  instance. See Documentation/dev-tools/gdb-kernel-debugging.rst
  for further details.
 
+config NO_AUTO_INLINE
+   bool "Disable compiler auto-inline optimizations"
+   help
+ This will prevent the compiler from optimizing the kernel by
+ auto-inlining functions not marked with the inline keyword.
+ With this option, only functions explicitly marked with
+ "inline" will be inlined. This will allow the function tracer
+ to trace more functions because it only traces functions that
+ the compiler has not inlined.
+
+ Enabling this function can help debugging a kernel if using
+ the function tracer. But it can also change how the kernel
+ works, because inlining functions may change the timing,
+ which could make it difficult while debugging race conditions.
+
+ If unsure, select N.
+
 config ENABLE_MUST_CHECK
bool "Enable __must_check logic"
default y
-- 
2.17.1

[PATCH v2 3/4] ARM: mm: fix build error in fix_to_virt with CONFIG_CC_OPTIMIZE_FOR_DEBUGGING

[Du Changbin]

With '-Og' optimization level, GCC would not optimize a count for a loop
as a constant value. But BUILD_BUG_ON() only accept compile-time constant
values. Let's use __fix_to_virt() to avoid the error.

arch/arm/mm/mmu.o: In function `fix_to_virt':
/home/changbin/work/linux/./include/asm-generic/fixmap.h:31: undefined 
reference to `__compiletime_assert_31'
Makefile:1051: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1

Signed-off-by: Du Changbin 
Acked-by: Steven Rostedt (VMware) 
---
 arch/arm/mm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e46a6a446cdd..c08d74e76714 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1599,7 +1599,7 @@ static void __init early_fixmap_shutdown(void)
pte_t *pte;
struct map_desc map;
 
-   map.virtual = fix_to_virt(i);
+   map.virtual = __fix_to_virt(i);
pte = pte_offset_early_fixmap(pmd_off_k(map.virtual), 
map.virtual);
 
/* Only i/o device mappings are supported ATM */
-- 
2.17.1

[PATCH v2 4/4] kernel hacking: new config CC_OPTIMIZE_FOR_DEBUGGING to apply GCC -Og optimization

[Du Changbin]

This will apply GCC '-Og' optimization level which is supported
since GCC 4.8. This optimization level offers a reasonable level
of optimization while maintaining fast compilation and a good
debugging experience. It is similar to '-O1' while perferring
to keep debug ability over runtime speed.

If enabling this option breaks your kernel, you should either
disable this or find a fix (mostly in the arch code). Currently
this option has only been tested on x86_64 and arm platform.

This option can satisfy people who was searching for a method
to disable compiler optimizations so to achieve better kernel
debugging experience with kgdb or qemu.

The main problem of '-Og' is we must not use __attribute__((error(msg))).
The compiler will report error though the call to error function
still can be optimize out. So we must fallback to array tricky.

Comparison of vmlinux size: a bit smaller.

w/o CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ size vmlinux
   textdata bss dec hex filename
22665554   9709674  2920908 3529613621a9388 vmlinux

w/ CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ size vmlinux
   textdata bss dec hex filename
21499032   10102758 2920908 3452269820ec64a vmlinux

Comparison of system performance: a bit drop (~6%).
This benchmark of kernel compilation is suggested by Ingo Molnar.
https://lkml.org/lkml/2018/5/2/74

Preparation: Set cpufreq to 'performance'.
for ((cpu=0; cpu<120; cpu++)); do
  G=/sys/devices/system/cpu/cpu$cpu/cpufreq/scaling_governor
  [ -f $G ] && echo performance > $G
done

w/o CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ perf stat --repeat 5 --null --pre '\
cp -a kernel ../kernel.copy.$(date +%s); \
rm -rf *;\
git checkout .;  \
echo 1 > /proc/sys/vm/drop_caches;   \
find ../kernel* -type f | xargs cat >/dev/null;  \
make -j kernel >/dev/null;   \
make clean >/dev/null 2>&1;  \
sync'\
 \
make -j8 >/dev/null

Performance counter stats for 'make -j8' (5 runs):

219.764246652 seconds time elapsed   ( +-  0.78% )

w/ CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
$ perf stat --repeat 5 --null --pre '\
cp -a kernel ../kernel.copy.$(date +%s); \
rm -rf *;\
git checkout .;  \
echo 1 > /proc/sys/vm/drop_caches;   \
find ../kernel* -type f | xargs cat >/dev/null;  \
make -j kernel >/dev/null;   \
make clean >/dev/null 2>&1;  \
sync'\
 \
make -j8 >/dev/null

Performance counter stats for 'make -j8' (5 runs):

 233.574187771 seconds time elapsed  ( +-  0.19% )

Signed-off-by: Du Changbin 
Acked-by: Steven Rostedt (VMware) 
---
 Makefile |  5 +
 include/linux/compiler-gcc.h |  2 +-
 include/linux/compiler.h |  2 +-
 init/Kconfig | 19 +++
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 757d6507cb5c..ea908cfe8594 100644
--- a/Makefile
+++ b/Makefile
@@ -657,6 +657,10 @@ KBUILD_CFLAGS  += $(call cc-disable-warning, 
format-truncation)
 KBUILD_CFLAGS  += $(call cc-disable-warning, format-overflow)
 KBUILD_CFLAGS  += $(call cc-disable-warning, int-in-bool-context)
 
+ifdef CONFIG_CC_OPTIMIZE_FOR_DEBUGGING
+KBUILD_CFLAGS  += $(call cc-option, -Og)
+KBUILD_CFLAGS  += $(call cc-disable-warning,maybe-uninitialized,)
+else
 ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
 KBUILD_CFLAGS  += $(call cc-option,-Oz,-Os)
 KBUILD_CFLAGS  += $(call cc-disable-warning,maybe-uninitialized,)
@@ -667,6 +671,7 @@ else
 KBUILD_CFLAGS   += -O2
 endif
 endif
+endif
 
 KBUILD_CFLAGS += $(call cc-ifversion, -lt, 0409, \
$(call cc-disable-warning,maybe-uninitialized,))
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 4d36b27214fd..2a76f7c64b54 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -85,7 +85,7 @@
 
 #define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
 
-#ifndef __CHECKER__
+#if !defined(__CHECKER__) && !defined(CONFIG_CC_OPTIMIZE_FOR_DEBUGGING)
 #define __compiletime_warning(message) __attribute__((warning(message)))
 #define __compiletime_error(message) __attribute__((error(message)))
 
diff --git a/include

[PATCH] usb: gadget: composite: enable BESL support

[Du, Changbin]

>From a6615937bcd9234e6d6bb817c3701fce44d0a84d Mon Sep 17 00:00:00 2001
From: Felipe Balbi 
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:  # 3.14
Signed-off-by: Felipe Balbi 
Signed-off-by: Du, Changbin 
---
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/

RE: [PATCH] usb: gadget: composite: enable BESL support

[Du, Changbin]

> > From: Felipe Balbi 
> > 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:  # 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 
> > Signed-off-by: Du, Changbin 
> > ---
> > 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 v2] usb: gadget: composite: enable BESL support

[Du, Changbin]

>From a6615937bcd9234e6d6bb817c3701fce44d0a84d Mon Sep 17 00:00:00 2001
From: Felipe Balbi 
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:  # 3.1+: a661593: usb: enable BESL support
Signed-off-by: Felipe Balbi 
Signed-off-by: Du, Changbin 
---
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 v3] usb: gadget: composite: enable BESL support

[Du, Changbin]

>From a6615937bcd9234e6d6bb817c3701fce44d0a84d Mon Sep 17 00:00:00 2001
From: Felipe Balbi 
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 
Signed-off-by: Du, Changbin 
---
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

[Du, Changbin]

>From a6615937bcd9234e6d6bb817c3701fce44d0a84d Mon Sep 17 00:00:00 2001
From: Felipe Balbi 
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:  # 3.1+
Signed-off-by: Felipe Balbi 
Signed-off-by: Du, Changbin 
---
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 v2] tracing/power: Polish the tracepoints cpu_idle and cpu_frequency

[Du, Changbin]

On Wed, Feb 28, 2018 at 10:14:41AM +0100, Rafael J. Wysocki wrote:
> On 2/28/2018 3:45 AM, Du, Changbin wrote:
> > On Tue, Feb 27, 2018 at 05:39:38PM -0500, Steven Rostedt wrote:
> > > On Tue, 27 Feb 2018 17:35:27 +0800
> > > "Du, Changbin"  wrote:
> > > 
> > > > >  From the tracing perspective:
> > > > > 
> > > > > Acked-by: Steven Rostedt (VMware) 
> > > > > 
> > > > > -- Steve
> > > > Hi Steve, will you pick this or someoneelse?
> > > I maintain the tracing infrastructure, but the tracing use cases are
> > > maintained by the maintainers of the users of the trace events. That
> > > is, who added these trace events? They are the ones most affected by
> > > these changes.
> > > 
> > > For example, it looks like Rafael J. Wysocki, is the one that added
> > > trace_cpu_frequency(). He's the one that is affected by this change,
> > > and is the one that you need to have take it.
> > > 
> > Got it, thanks!
> > 
> > Hi Wysocki, could you take a look?
> 
> Please send the patch(es) to linux...@vger.kernel.org with a CC to me and I
> will take care of them.
> 
sure~

> Thanks,
> Rafael
> 

-- 
Thanks,
Changbin Du

Re: [PATCH] perf trace: remove redundant ')'

[Du, Changbin]

On Tue, Apr 03, 2018 at 04:19:07PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 28, 2018 at 03:26:31PM +0800, Du, Changbin escreveu:
> > Hi Arnaldo,
> > Just a kind reminder. Hope you didn't forget this.
> 
> Ok, applied.
> 
> - Arnaldo
>  
Got it, thanks!

Re: [PATCH v2 1/4] selftests/Makefile: append a slash to env variable OUTPUT

[Du, Changbin]

On Tue, Mar 27, 2018 at 03:19:26PM -0600, Shuah Khan wrote:
> On 03/26/2018 09:11 PM, changbin...@intel.com wrote:
> > From: Changbin Du 
> > 
> > The tools/build/Makefile.build use 'OUTPUT' variable as below example:
> > objprefix:= $(subst ./,,$(OUTPUT)$(dir)/)
> > 
> > So it requires the 'OUTPUT' already has a slash at the end.
> > 
> > This patch can kill below odd paths:
> > make[3]: Entering directory '/home/changbin/work/linux/tools/gpio'
> >   CC   /home/changbin/work/linux/tools/testing/selftests/gpiolsgpio.o
> >   CC   
> > /home/changbin/work/linux/tools/testing/selftests/gpiogpio-utils.o
> >   LD   /home/changbin/work/linux/tools/testing/selftests/gpiolsgpio-in.o
> > 
> > A correct path should be:
> > /home/changbin/work/linux/tools/testing/selftests/gpio/lsgpio.o
> > 
> > Signed-off-by: Changbin Du 
> 
> Are you seeing this when you run "make kselftest" - if gpio is the
> only test compile that fails, it should be fixed in gpio/Makefile,
> not is the common Makefile.
>
I only saw error in gpio, but I also saw some kselftest Makefiles having string 
concatenation
as '$(OUTPUT)$(dir)'. So the rule is not aligned all over. They just didn't 
produce any errors
so far.

By the way, is there a basic test for kselftest infrastructure? It seems it was 
always
reporting error when building it :(

> thanks,
> -- Shuah

-- 
Thanks,
Changbin Du

Re: [PATCH] perf trace: remove redundant ')'

[Du, Changbin]

Hi Arnaldo,
Just a kind reminder. Hope you didn't forget this.

On Fri, Mar 16, 2018 at 09:50:45AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Mar 16, 2018 at 03:51:09PM +0800, Du, Changbin escreveu:
> > Hi Arnaldo, How about this simple one? Thanks.
> > 
> > On Tue, Mar 13, 2018 at 06:40:01PM +0800, changbin...@intel.com wrote:
> > > From: Changbin Du 
> > > 
> > > There is a redundant ')' at the tail of each event. So remove it.
> > > $ sudo perf trace --no-syscalls -e 'kmem:*' -a
> > >899.342 kmem:kfree:(vfs_writev+0xb9) call_site=9c453979 
> > > ptr=(nil))
> > >899.344 kmem:kfree:(___sys_recvmsg+0x188) call_site=9c9b8b88 
> > > ptr=(nil))
> > > 
> > > Signed-off-by: Changbin Du 
> > > ---
> > >  tools/perf/builtin-trace.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > > index e7f1b18..7273f5f 100644
> > > --- a/tools/perf/builtin-trace.c
> > > +++ b/tools/perf/builtin-trace.c
> > > @@ -1959,7 +1959,7 @@ static int trace__event_handler(struct trace 
> > > *trace, struct perf_evsel *evsel,
> > > trace->output);
> > >   }
> > >  
> > > - fprintf(trace->output, ")\n");
> > > + fprintf(trace->output, "\n");
> 
> It looks simple on the surface, but I couldn't quickly recall why this
> ')' was put there in the first place... So I left for later to do a 'git
> blame' on this file, etc.
> 
> - Arnaldo
> 
> > >   if (callchain_ret > 0)
> > >   trace__fprintf_callchain(trace, sample);
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Thanks,
> > Changbin Du

-- 
Thanks,
Changbin Du

Re: [PATCH 0/5] kernel hacking: GCC optimization for debug experience (-Og)

[Du, Changbin]

On Wed, May 02, 2018 at 09:33:15AM +0200, Ingo Molnar wrote:
> 
> * changbin...@intel.com  wrote:
> 
> > Comparison of system performance: a bit drop.
> > 
> > w/o CONFIG_DEBUG_EXPERIENCE
> > $ time make -j4
> > real6m43.619s
> > user19m5.160s
> > sys 2m20.287s
> > 
> > w/ CONFIG_DEBUG_EXPERIENCE
> > $ time make -j4
> > real6m55.054s
> > user19m11.129s
> > sys 2m36.345s
> 
> Sorry, that's not a proper kbuild performance measurement - there's no noise 
> estimation at all.
> 
> Below is a description that should produce more reliable numbers.
> 
> Thanks,
> 
>   Ingo
>
Thanks for your suggestion, I will try your tips to eliminate noise. Since it is
tested in KVM guest, so I just reboot the guest before testing. But in host side
I still need to consider these noises.

> 
> =>
> 
> So here's a pretty reliable way to measure kernel build time, which tries to 
> avoid 
> the various pitfalls of caching.
> 
> First I make sure that cpufreq is set to 'performance':
> 
>   for ((cpu=0; cpu<120; cpu++)); do
> G=/sys/devices/system/cpu/cpu$cpu/cpufreq/scaling_governor
> [ -f $G ] && echo performance > $G
>   done
> 
> [ ... because it can be *really* annoying to discover that an ostensible 
>   performance regression was a cpufreq artifact ... again. ;-) ]
> 
> Then I copy a kernel tree to /tmp (ramfs) as root:
> 
>   cd /tmp
>   rm -rf linux
>   git clone ~/linux linux
>   cd linux
>   make defconfig >/dev/null
>   
> ... and then we can build the kernel in such a loop (as root again):
> 
>   perf stat --repeat 10 --null --pre  '\
>   cp -a kernel ../kernel.copy.$(date +%s); \
>   rm -rf *;\
>   git checkout .;  \
>   echo 1 > /proc/sys/vm/drop_caches;   \
>   find ../kernel* -type f | xargs cat >/dev/null;  \
>   make -j kernel >/dev/null;   \
>   make clean >/dev/null 2>&1;  \
>   sync'\
>\
>   make -j16 >/dev/null
> 
> ( I have tested these by pasting them into a terminal. Adjust the ~/linux 
> source 
>   git tree and the '-j16' to your system. )
> 
> Notes:
> 
>  - the 'pre' script portion is not timed by 'perf stat', only the raw build 
> times
> 
>  - we flush all caches via drop_caches and re-establish everything again, but:
> 
>  - we also introduce an intentional memory leak by slowly filling up ramfs 
> with 
>copies of 'kernel/', thus continously changing the layout of free memory, 
>cached data such as compiler binaries and the source code hierarchy. (Note 
>that the leak is about 8MB per iteration, so it isn't massive.)
> 
> With 10 iterations this is the statistical stability I get this on a big box:
> 
>  Performance counter stats for 'make -j128 kernel' (10 runs):
> 
>   26.346436425 seconds time elapsed(+- 0.19%)
> 
> ... which, despite a high iteration count of 10, is still surprisingly noisy, 
> right?
> 
> A 0.2% stddev is probably not enough to call a 0.7% regression with good 
> confidence, so I had to use *30* iterations to make measurement noise to be 
> about 
> an order of magnitude lower than the effect I'm trying to measure:
> 
>  Performance counter stats for 'make -j128' (30 runs):
> 
>   26.334767571 seconds time elapsed(+- 0.09% )
> 
> i.e. "26.334 +- 0.023" seconds is a number we can have pretty high confidence 
> in, 
> on this system.
> 
> And just to demonstrate that it's all real, I repeated the whole 30-iteration 
> measurement again:
> 
>  Performance counter stats for 'make -j128' (30 runs):
> 
>   26.311166142 seconds time elapsed(+- 0.07%)
> 

-- 
Thanks,
Changbin Du

Re: [PATCH 2/5] regulator: add dummy of_find_regulator_by_node

[Du, Changbin]

On Wed, May 02, 2018 at 05:40:36AM +0900, Mark Brown wrote:
> On Tue, May 01, 2018 at 09:00:11PM +0800, changbin...@intel.com wrote:
> > From: Changbin Du 
> > 
> > If device tree is not enabled, of_find_regulator_by_node() should have
> > a dummy function since the function call is still there.
> > 
> > Signed-off-by: Changbin Du 
> 
> This appears to have no obvious connection with the cover letter for the
> series...  The first question here is if this is something best fixed
> with a stub or by fixing the users - is the lack of a stub pointing out
> some bugs in them?  I'm a bit worried about how we've been managing to
> avoid any build test issues here though, surely the various builders
> would have spotted a problem?

This is to fix build error after NO_AUTO_INLINE is introduced. If this option
is enabled, GCC will not auto-inline functions that are not explicitly marked
as inline.

In this case (no CONFIG_OF), the copmiler will report error in 
regulator_dev_lookup().
W/o NO_AUTO_INLINE, function of_get_regulator() is auto-inlined and then the 
call
to of_find_regulator_by_node() is optimized out since of_get_regulator() always
return NULL. W/ NO_AUTO_INLINE, the return value of of_get_regulator() is a 
variable
so the call to of_find_regulator_by_node() cannot be optimized out.

static struct regulator_dev *regulator_dev_lookup(struct device *dev,
  const char *supply)
{
struct regulator_dev *r = NULL;
struct device_node *node;
struct regulator_map *map;
const char *devname = NULL;

regulator_supply_alias(&dev, &supply);

/* first do a dt based lookup */
if (dev && dev->of_node) {
node = of_get_regulator(dev, supply);
if (node) {
r = of_find_regulator_by_node(node);
if (r)
return r;


It is safe we just provide a stub of_find_regulator_by_node() if no CONFIG_OF.

-- 
Thanks,
Changbin Du

Re: [PATCH 4/5] kernel hacking: new config DEBUG_EXPERIENCE to apply GCC -Og optimization

[Du, Changbin]

On Tue, May 01, 2018 at 08:25:27AM -0700, Randy Dunlap wrote:
> Good morning.
> 
> On 05/01/2018 06:00 AM, changbin...@intel.com wrote:
> > From: Changbin Du 
> > 
> > 
> > Signed-off-by: Changbin Du 
> > ---
> >  Makefile |  4 
> >  include/linux/compiler-gcc.h |  2 +-
> >  include/linux/compiler.h |  2 +-
> >  lib/Kconfig.debug| 21 +
> >  4 files changed, 27 insertions(+), 2 deletions(-)
> > 
> 
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 90f35ad..2432e77d 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -211,6 +211,27 @@ config NO_AUTO_INLINE
> >  
> >   Use only if you want to debug the kernel.
> >  
> > +config DEBUG_EXPERIENCE
> > +   bool "Optimize for better debugging experience (-Og)"
> > +   default n
> > +   select NO_AUTO_INLINE
> > +   depends on !CC_OPTIMIZE_FOR_SIZE
> > +   help
> > + This will apply GCC '-Og' optimization level get supported from
> 
>  which is supported since
> 
> > + GCC 4.8. This optimization level offers a reasonable level of
> > + optimization while maintaining fast compilation and a good
> > + debugging experience. It is similar to '-O1' while perfer keeping
> 
>  while preferring to keep
> 
> > + debug ability over runtime speed. The overall performance will
> > + drop a bit.
> > +
> > + If enabling this option break your kernel, you should either
> 
> breaks
> 
> > + disable this or find a fix (mostly in the arch code). Currently
> > + this option has only be tested in qemu x86_64 guest.
> > +
> > + Use only if you want to debug the kernel, especially if you want
> > + to have better kernel debugging experience with gdb facilities
> > + like kgdb and qemu.
> > +
> >  config ENABLE_WARN_DEPRECATED
> > bool "Enable __deprecated logic"
> > default y
> > 
> 
> thanks,
> -- 
> ~Randy

Thanks for your correction, I will update.

-- 
Thanks,
Changbin Du

Re: [PATCH 3/5] kernel hacking: new config NO_AUTO_INLINE to disable compiler atuo-inline optimizations

[Du, Changbin]

On Tue, May 01, 2018 at 10:54:20AM -0400, Steven Rostedt wrote:
> On Tue,  1 May 2018 21:00:12 +0800
> changbin...@intel.com wrote:
> 
> > From: Changbin Du 
> > 
> > This patch add a new kernel hacking option NO_AUTO_INLINE. Selecting
> > this option will make compiler not auto-inline kernel functions. By
> > enabling this option, all the kernel functions (including static ones)
> > will not be optimized out except those marked as inline or always_inline.
> > This is useful when you are using ftrace to understand the control flow
> > of kernel code or tracing some static functions.
> 
> I'm not against this patch, but it's up to others if this gets included
> or not.
> 
> > 
> > Signed-off-by: Changbin Du 
> > Cc: Steven Rostedt 
> > ---
> >  Makefile  |  6 ++
> >  lib/Kconfig.debug | 13 +
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/Makefile b/Makefile
> > index 619a85a..eb694f6 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -775,6 +775,12 @@ KBUILD_CFLAGS  += $(call cc-option, 
> > -femit-struct-debug-baseonly) \
> >$(call cc-option,-fno-var-tracking)
> >  endif
> >  
> > +ifdef CONFIG_NO_AUTO_INLINE
> > +KBUILD_CFLAGS   += $(call cc-option, -fno-inline-functions) \
> > +  $(call cc-option, -fno-inline-small-functions) \
> > +  $(call cc-option, -fno-inline-functions-called-once)
> > +endif
> > +
> >  ifdef CONFIG_FUNCTION_TRACER
> >  ifndef CC_FLAGS_FTRACE
> >  CC_FLAGS_FTRACE := -pg
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index c40c7b7..90f35ad 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -198,6 +198,19 @@ config GDB_SCRIPTS
> >   instance. See Documentation/dev-tools/gdb-kernel-debugging.rst
> >   for further details.
> >  
> > +config NO_AUTO_INLINE
> > +   bool "Disable compiler atuo-inline optimizations"
> 
> typo: s/atuo/auto/
> 
> > +   default n
> > +   help
> > + This will make compiler not auto-inline kernel functions for
> > + optimization. By enabling this option, all the kernel functions
> > + (including static ones) will not be optimized out except those
> > + marked as inline or always_inline. This is useful when you are
> > + using ftrace to understand the control flow of kernel code or
> > + tracing some static functions.
> 
> Some grammar updates:
> 
> This will prevent the compiler from optimizing the kernel by
> auto-inlining functions not marked with the inline keyword.
> With this option, only functions explicitly marked with
> "inline" will be inlined. This will allow the function tracer
> to trace more functions because it only traces functions that
> the compiler has not inlined.
> 
> Enabling this function can help debugging a kernel if using
> the function tracer. But it can also change how the kernel
> works, because inlining functions may change the timing,
> which could make it difficult while debugging race conditions.
> 

Thanks for your kind grammar updates. I will update them. :)

> > +
> > + Use only if you want to debug the kernel.
> 
> The proper way to say the above is:
> 
> If unsure, select N
>
Agree.

> -- Steve
> 
> > +
> >  config ENABLE_WARN_DEPRECATED
> > bool "Enable __deprecated logic"
> > default y
> 


-- 
Thanks,
Changbin Du

Re: [PATCH v5 2/4] kernel hacking: new config NO_AUTO_INLINE to disable compiler auto-inline optimizations

[Du, Changbin]

Hi,
On Thu, Jun 07, 2018 at 09:47:18AM +0530, Viresh Kumar wrote:
> +Greg/Alex,
> 
> @Fegguang/build-bot: I do see mention of Greg and /me in your initial email's
> body saying TO: Viresh, CC: Greg, but I don't see any of us getting cc'd in 
> your
> email. Bug ?
> 
> On 06-06-18, 14:26, Steven Rostedt wrote:
> > On Wed, 6 Jun 2018 16:26:00 +0200
> > Johan Hovold  wrote:
> > 
> > > Looks like the greybus code above is working as intended by checking for
> > > unterminated string after the strncpy, even if this does now triggers
> > > the truncation warning.
> 
> So why exactly are we generating a warning here ? Is it because it is possible
> that the first n bytes of src may not have the null terminating byte and the
> dest may not be null terminated eventually ?
> 
> Maybe I should just use memcpy here then ?
> 
I think if the destination is not a null terminated string (If I understand your
description below), memcpy can be used to get rid of such warning. The warning
makes sense in general as explained in mannual. Thanks!

> But AFAIR, I used strncpy() specifically because it also sets all the 
> remaining
> bytes after the null terminating byte with the null terminating byte. And so 
> it
> is pretty easy for me to check if the final string is null terminated by
> checking [max - 1] byte against '\0', which the code is doing right now.
> 
> I am not sure what would the best way to get around this incorrect-warning.
> 
> And I am wondering on why buildbot reported the warning only for two instances
> in that file, while I have done the same thing at 4 places.
> 
> > Ah, yes I now see that. Thanks for pointing it out. But perhaps it
> > should also add the "- 1" to the strncpy() so that gcc doesn't think
> > it's a mistake.
> 
> The src string is passed on from a firmware entity and we need to make sure 
> the
> protocol (greybus) is implemented properly by the other end. For example, in 
> the
> current case if the firmware sends "HELLOWORLD", its an error as it should 
> have
> sent "HELLWORLD\0". But with what you are saying we will forcefully make dest 
> as
> "HELLWORLD\0", which wouldn't be the right thing to do as we will miss the bug
> present in firmware.
> 
> -- 
> viresh

-- 
Thanks,
Changbin Du

Re: [PATCH] iommu/vt-d: fix shift-out-of-bounds in bug checking

[Du, Changbin]

Hello, any reviewer? Thanks!

On Fri, Apr 20, 2018 at 01:29:55PM +0800, changbin...@intel.com wrote:
> From: Changbin Du 
> 
> It allows to flush more than 4GB of device TLBs. So the mask should be
> 64bit wide. UBSAN captured this fault as below.
> 
> [3.760024] 
> 
> [3.768440] UBSAN: Undefined behaviour in drivers/iommu/dmar.c:1348:3
> [3.774864] shift exponent 64 is too large for 32-bit type 'int'
> [3.780853] CPU: 2 PID: 0 Comm: swapper/2 Tainted: G U
> 4.17.0-rc1+ #89
> [3.788661] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.2.8 
> 01/26/2016
> [3.796034] Call Trace:
> [3.798472]  
> [3.800479]  dump_stack+0x90/0xfb
> [3.803787]  ubsan_epilogue+0x9/0x40
> [3.807353]  __ubsan_handle_shift_out_of_bounds+0x10e/0x170
> [3.812916]  ? qi_flush_dev_iotlb+0x124/0x180
> [3.817261]  qi_flush_dev_iotlb+0x124/0x180
> [3.821437]  iommu_flush_dev_iotlb+0x94/0xf0
> [3.825698]  iommu_flush_iova+0x10b/0x1c0
> [3.829699]  ? fq_ring_free+0x1d0/0x1d0
> [3.833527]  iova_domain_flush+0x25/0x40
> [3.837448]  fq_flush_timeout+0x55/0x160
> [3.841368]  ? fq_ring_free+0x1d0/0x1d0
> [3.845200]  ? fq_ring_free+0x1d0/0x1d0
> [3.849034]  call_timer_fn+0xbe/0x310
> [3.852696]  ? fq_ring_free+0x1d0/0x1d0
> [3.856530]  run_timer_softirq+0x223/0x6e0
> [3.860625]  ? sched_clock+0x5/0x10
> [3.864108]  ? sched_clock+0x5/0x10
> [3.867594]  __do_softirq+0x1b5/0x6f5
> [3.871250]  irq_exit+0xd4/0x130
> [3.874470]  smp_apic_timer_interrupt+0xb8/0x2f0
> [3.879075]  apic_timer_interrupt+0xf/0x20
> [3.883159]  
> [3.885255] RIP: 0010:poll_idle+0x60/0xe7
> [3.889252] RSP: 0018:b1b201943e30 EFLAGS: 0246 ORIG_RAX: 
> ff13
> [3.896802] RAX: 8020 RBX: 008e RCX: 
> 001f
> [3.903918] RDX:  RSI: 2819aa06 RDI: 
> 
> [3.911031] RBP: 9e93c6b33280 R08: 0010f717d567 R09: 
> 0010d205
> [3.918146] R10: b1b201943df8 R11: 0001 R12: 
> e01b169d
> [3.925260] R13:  R14: b12aa400 R15: 
> 
> [3.932382]  cpuidle_enter_state+0xb4/0x470
> [3.936558]  do_idle+0x222/0x310
> [3.939779]  cpu_startup_entry+0x78/0x90
> [3.943693]  start_secondary+0x205/0x2e0
> [3.947607]  secondary_startup_64+0xa5/0xb0
> [3.951783] 
> 
> 
> Signed-off-by: Changbin Du 
> ---
>  drivers/iommu/dmar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index accf5838..e4ae600 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -1345,7 +1345,7 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 
> sid, u16 qdep,
>   struct qi_desc desc;
>  
>   if (mask) {
> - BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
> + BUG_ON(addr & ((1ULL << (VTD_PAGE_SHIFT + mask)) - 1));
>   addr |= (1ULL << (VTD_PAGE_SHIFT + mask - 1)) - 1;
>   desc.high = QI_DEV_IOTLB_ADDR(addr) | QI_DEV_IOTLB_SIZE;
>   } else
> -- 
> 2.7.4
> 

-- 
Thanks,
Changbin Du

[PATCH v2] dynamic_debug: add wildcard support to filter files/functions/modules

[Du, Changbin]

From: "Du, Changbin" 

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" > /dynamic_debug/control
2.  open debug logs for usb xhci code
echo "file *xhci* +p" > /dynamic_debug/control

Signed-off-by: Du, Changbin 
---
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/

[PATCH v3] dynamic_debug: add wildcard support to filter files/functions/modules

[Du, Changbin]

From: "Du, Changbin" 

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" > /dynamic_debug/control
2.  open debug logs for usb xhci code
echo "file *xhci* +p" > /dynamic_debug/control

Signed-off-by: Du, Changbin 
---
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

[Du, Changbin]

From: "Du, Changbin" 

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" > /dynamic_debug/control
2.  open debug logs for usb xhci code
echo "file *xhci* +p" > /dynamic_debug/control

Signed-off-by: Du, Changbin 
---
 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 
  * Copyright (c) 2008 Silicon Graphics Inc.  All Rights Reserved.
  * Copyright (C) 2011 Bart Van Assche.  All Rights Reserved.
+ * Copyright (C) 2013 Du, Changbin 
  */
 
 #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

[Du, Changbin]

From: "Du, Changbin" 

Add the usage of using new feature wildcard support.

Signed-off-by: Du, Changbin 
---
 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 > /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" > /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' >
/dynamic_debug/control
 
+// enable messages in files of which the pathes include string "usb"
+nullarbor:~ # echo -n '*usb* +p' > /dynamic_debug/control
+
 // enable all messages
 nullarbor:~ # echo -n '+p' > /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 v5 0/3] add wildcard support for dynamic debug

[Du, Changbin]

From: "Du, Changbin" 

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" > /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

[Du, Changbin]

From: "Du, Changbin" 

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 
---
 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

[Du, Changbin]

From: "Du, Changbin" 

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" > /dynamic_debug/control
2.  open debug logs for usb xhci code
echo "file *xhci* +p" > /dynamic_debug/control

Signed-off-by: Du, Changbin 
---
 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 
  * Copyright (c) 2008 Silicon Graphics Inc.  All Rights Reserved.
  * Copyright (C) 2011 Bart Van Assche.  All Rights Reserved.
+ * Copyright (C) 2013 Du, Changbin 
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
@@ -24,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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

[Du, Changbin]

From: "Du, Changbin" 

Add the usage of using new feature wildcard support.

Signed-off-by: Du, Changbin 
---
 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 > /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" > /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' >
/dynamic_debug/control
 
+// enable messages in files of which the pathes include string "usb"
+nullarbor:~ # echo -n '*usb* +p' > /dynamic_debug/control
+
 // enable all messages
 nullarbor:~ # echo -n '+p' > /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] usb/gadget: make composite gadget meet usb compliance for vbus draw

[Du, Changbin]

>From 0a8e0d63a9887735c6782d7b0c15c2c1fdf1952a Mon Sep 17 00:00:00 2001
From: "Du, Changbin" 
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 
---
 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

[Du, Changbin]

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

[Du, Changbin]

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/

RE: [PATCH] usb/gadget: make composite gadget meet usb compliance for vbus draw

[Du, Changbin]

> > 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 v2] usb/gadget: make composite gadget meet usb compliance for vbus draw

[Du, Changbin]

>From 08df419517694c4dd9ff328f5644b46a99c2999e Mon Sep 17 00:00:00 2001
From: "Du, Changbin" 
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 
---
 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/includ

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

Hello, Sergei,
> > From: "Du, Changbin" 
> >
> > 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 
> > ---
> > 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,
> &dwc3_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, &dwc3_mode_fops);
> 
> MBR, Sergei

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

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 
> Cc: gre...@linuxfoundation.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Du, Changbin 
> 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" 
> >
> > 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 
> > ---
> > 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(&dwc->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
> to make sure we're allowed to start a transfer ;-)
> 
> I&

RE: [PATCH v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

> > 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 v4 2/2] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

> Hi,
> 
> "Du, Changbin"  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 1/2] usb: configfs: allow UDC binding rule configured as binding to *any* UDC

[Du, Changbin]

> >>> 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&D Institute Poland
> Samsung Electronics

Best Regards,
Du, Changbin

RE: [PATCH] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks

[Du, Changbin]

> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Can you please fix your mail client. Every mail you send has:
> 
> Cc: .....
>     "Du, Changbin" ,
> 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(&db->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(&o, "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] debugobjects: insulate non-fixup logic related to static obj from fixup callbacks

[Du, Changbin]

> 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(&db->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] usb: hub: fix panic caused by NULL bos pointer during reset device

[Du, Changbin]

> > 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

[Du, Changbin]

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&D Institute Poland
> Samsung Electronics

Thanks,
Du, Changbin

RE: [PATCH] usb: gadget: f_fs: report error if excess data received

[Du, Changbin]

> Hi,
> 
> changbin...@intel.com writes:
> > From: "Du, Changbin" 
> >
> > 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

[Du, Changbin]

> 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

[Du, Changbin]

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

[Du, Changbin]

> > 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