Re: [PATCH v3 05/11] mm, fsdax: Refactor memory-failure handler for dax mapping

[zhong jiang]

l *tk;
  
@@ -342,9 +348,12 @@ static void add_to_kill(struct task_struct *tsk, struct page *p,

}
  
  	tk->addr = page_address_in_vma(p, vma);

-   if (is_zone_device_page(p))
-   tk->size_shift = dev_pagemap_mapping_shift(p, vma);
-   else
+   if (is_zone_device_page(p)) {
+   if (is_device_fsdax_page(p))
+   tk->addr = vma->vm_start +
+   ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
+   tk->size_shift = dev_pagemap_mapping_shift(vma, tk->addr);
+   } else
tk->size_shift = page_shift(compound_head(p));
  
  	/*

@@ -492,7 +501,7 @@ static void collect_procs_anon(struct page *page, struct 
list_head *to_kill,
if (!page_mapped_in_vma(page, vma))
continue;
if (vma->vm_mm == t->mm)
-   add_to_kill(t, page, vma, to_kill);
+   add_to_kill(t, page, 0, vma, to_kill);
}
}
read_unlock(_lock);
@@ -502,24 +511,19 @@ static void collect_procs_anon(struct page *page, struct 
list_head *to_kill,
  /*
   * Collect processes when the error hit a file mapped page.
   */
-static void collect_procs_file(struct page *page, struct list_head *to_kill,
-   int force_early)
+static void collect_procs_file(struct page *page, struct address_space 
*mapping,
+   pgoff_t pgoff, struct list_head *to_kill, int force_early)
  {
struct vm_area_struct *vma;
struct task_struct *tsk;
-   struct address_space *mapping = page->mapping;
-   pgoff_t pgoff;
  
  	i_mmap_lock_read(mapping);

read_lock(_lock);
-   pgoff = page_to_pgoff(page);
for_each_process(tsk) {
struct task_struct *t = task_early_kill(tsk, force_early);
-
if (!t)
continue;
-   vma_interval_tree_foreach(vma, >i_mmap, pgoff,
- pgoff) {
+   vma_interval_tree_foreach(vma, >i_mmap, pgoff, pgoff) {
/*
 * Send early kill signal to tasks where a vma covers
 * the page but the corrupted page is not necessarily
@@ -528,7 +532,7 @@ static void collect_procs_file(struct page *page, struct 
list_head *to_kill,
 * to be informed of all such data corruptions.
 */
if (vma->vm_mm == t->mm)
-   add_to_kill(t, page, vma, to_kill);
+   add_to_kill(t, page, pgoff, vma, to_kill);
}
}
read_unlock(_lock);
@@ -547,7 +551,8 @@ static void collect_procs(struct page *page, struct 
list_head *tokill,
if (PageAnon(page))
collect_procs_anon(page, tokill, force_early);
else
-   collect_procs_file(page, tokill, force_early);
+   collect_procs_file(page, page_mapping(page), 
page_to_pgoff(page),
+  tokill, force_early);
  }
  
  static const char *action_name[] = {

@@ -1214,6 +1219,50 @@ static int try_to_split_thp_page(struct page *page, 
const char *msg)
return 0;
  }
  
+int mf_dax_mapping_kill_procs(struct address_space *mapping, pgoff_t index, int flags)

+{
+   const bool unmap_success = true;
+   unsigned long pfn, size = 0;
+   struct to_kill *tk;
+   LIST_HEAD(to_kill);
+   int rc = -EBUSY;
+   loff_t start;
+
+   /* load the pfn of the dax mapping file */
+   pfn = dax_load_pfn(mapping, index);
+   if (!pfn)
+   return rc;
+   /*
+* Unlike System-RAM there is no possibility to swap in a
+* different physical page at a given virtual address, so all
+* userspace consumption of ZONE_DEVICE memory necessitates
+* SIGBUS (i.e. MF_MUST_KILL)
+*/
+   flags |= MF_ACTION_REQUIRED | MF_MUST_KILL;


MF_ACTION_REQUIRED only kill the current execution context. A page can be shared
when reflink file be mapped by different process. We can not kill all process
shared the page.  Other process still can access the posioned page ?

Thanks,
zhong jiang


+   collect_procs_file(pfn_to_page(pfn), mapping, index, _kill,
+  flags & MF_ACTION_REQUIRED);
+
+   list_for_each_entry(tk, _kill, nd)
+   if (tk->size_shift)
+   size = max(size, 1UL << tk->size_shift);
+   if (size) {
+   /*
+* Unmap the largest mapping to avoid breaking up
+* device-dax mappings which are constant size. The
+* actual size of the mapping being torn down is
+* communicated in siginfo, see kill_proc()
+*/
+   start

Re: [PATCH 04/10] mm, fsdax: Refactor memory-failure handler for dax mapping

[zhong jiang]

On 2021/1/14 11:52 上午, Ruan Shiyang wrote:



On 2021/1/14 上午11:26, zhong jiang wrote:


On 2021/1/14 9:44 上午, Ruan Shiyang wrote:



On 2021/1/13 下午6:04, zhong jiang wrote:


On 2021/1/12 10:55 上午, Ruan Shiyang wrote:



On 2021/1/6 下午11:41, Jan Kara wrote:

On Thu 31-12-20 00:55:55, Shiyang Ruan wrote:
The current memory_failure_dev_pagemap() can only handle 
single-mapped
dax page for fsdax mode.  The dax page could be mapped by 
multiple files
and offsets if we let reflink feature & fsdax mode work 
together. So,
we refactor current implementation to support handle memory 
failure on

each file and offset.

Signed-off-by: Shiyang Ruan 


Overall this looks OK to me, a few comments below.


---
  fs/dax.c    | 21 +++
  include/linux/dax.h |  1 +
  include/linux/mm.h  |  9 +
  mm/memory-failure.c | 91 
++---

  4 files changed, 100 insertions(+), 22 deletions(-)


...

  @@ -345,9 +348,12 @@ static void add_to_kill(struct 
task_struct *tsk, struct page *p,

  }
    tk->addr = page_address_in_vma(p, vma);
-    if (is_zone_device_page(p))
-    tk->size_shift = dev_pagemap_mapping_shift(p, vma);
-    else
+    if (is_zone_device_page(p)) {
+    if (is_device_fsdax_page(p))
+    tk->addr = vma->vm_start +
+    ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);


It seems strange to use 'pgoff' for dax pages and not for any 
other page.
Why? I'd rather pass correct pgoff from all callers of 
add_to_kill() and

avoid this special casing...


Because one fsdax page can be shared by multiple pgoffs. I have to 
pass each pgoff in each iteration to calculate the address in vma 
(for tk->addr).  Other kinds of pages don't need this. They can 
get their unique address by calling "page_address_in_vma()".


IMO,   an fsdax page can be shared by multiple files rather than 
multiple pgoffs if fs query support reflink.   Because an page only 
located in an mapping(page->mapping is exclusive), hence it  only 
has an pgoff or index pointing at the node.


  or  I miss something for the feature ?  thanks,


Yes, a fsdax page is shared by multiple files because of reflink. I 
think my description of 'pgoff' here is not correct.  This 'pgoff' 
means the offset within the a file. (We use rmap to find out all the 
sharing files and their offsets.)  So, I said that "can be shared by 
multiple pgoffs".  It's my bad.


I think I should name it another word to avoid misunderstandings.

IMO,  All the sharing files should be the same offset to share the 
fsdax page.  why not that ? 


The dedupe operation can let different files share their same data 
extent, though offsets are not same.  So, files can share one fsdax 
page at different offset.

Ok,  Get it.


As you has said,  a shared fadax page should be inserted to different 
mapping files.  but page->index and page->mapping is exclusive.  
hence an page only should be placed in an mapping tree.


We can't use page->mapping and page->index here for reflink & fsdax. 
And that's this patchset aims to solve.  I introduced a series of 
->corrupted_range(), from mm to pmem driver to block device and 
finally to filesystem, to use rmap feature of filesystem to find out 
all files sharing same data extent (fsdax page).


From this patch,  each file has mapping tree,  the shared page will be 
inserted into multiple file mapping tree.  then filesystem use file and 
offset to get the killed process.   Is it correct?


Thanks,




--
Thanks,
Ruan Shiyang.



And In the current patch,  we failed to found out that all process 
use the fsdax page shared by multiple files and kill them.



Thanks,


--
Thanks,
Ruan Shiyang.



So, I added this fsdax case here. This patchset only implemented 
the fsdax case, other cases also need to be added here if to be 
implemented.



--
Thanks,
Ruan Shiyang.



+    tk->size_shift = dev_pagemap_mapping_shift(p, vma, 
tk->addr);

+    } else
  tk->size_shift = page_shift(compound_head(p));
    /*
@@ -495,7 +501,7 @@ static void collect_procs_anon(struct page 
*page, struct list_head *to_kill,

  if (!page_mapped_in_vma(page, vma))
  continue;
  if (vma->vm_mm == t->mm)
-    add_to_kill(t, page, vma, to_kill);
+    add_to_kill(t, page, NULL, 0, vma, to_kill);
  }
  }
  read_unlock(_lock);
@@ -505,24 +511,19 @@ static void collect_procs_anon(struct page 
*page, struct list_head *to_kill,

  /*
   * Collect processes when the error hit a file mapped page.
   */
-static void collect_procs_file(struct page *page, struct 
list_head *to_kill,

-    int force_early)
+static void collect_procs_file(struct page *page, struct 
address_space *mapping,

+    pgoff_t pgoff, struct list_head *to_kill, int force_early)
  {
  struct vm_area_struct *vma;
  struct tas

Re: [PATCH 04/10] mm, fsdax: Refactor memory-failure handler for dax mapping

[zhong jiang]

On 2021/1/14 9:44 上午, Ruan Shiyang wrote:



On 2021/1/13 下午6:04, zhong jiang wrote:


On 2021/1/12 10:55 上午, Ruan Shiyang wrote:



On 2021/1/6 下午11:41, Jan Kara wrote:

On Thu 31-12-20 00:55:55, Shiyang Ruan wrote:
The current memory_failure_dev_pagemap() can only handle 
single-mapped
dax page for fsdax mode.  The dax page could be mapped by multiple 
files
and offsets if we let reflink feature & fsdax mode work together.  
So,
we refactor current implementation to support handle memory 
failure on

each file and offset.

Signed-off-by: Shiyang Ruan 


Overall this looks OK to me, a few comments below.


---
  fs/dax.c    | 21 +++
  include/linux/dax.h |  1 +
  include/linux/mm.h  |  9 +
  mm/memory-failure.c | 91 
++---

  4 files changed, 100 insertions(+), 22 deletions(-)


...

  @@ -345,9 +348,12 @@ static void add_to_kill(struct task_struct 
*tsk, struct page *p,

  }
    tk->addr = page_address_in_vma(p, vma);
-    if (is_zone_device_page(p))
-    tk->size_shift = dev_pagemap_mapping_shift(p, vma);
-    else
+    if (is_zone_device_page(p)) {
+    if (is_device_fsdax_page(p))
+    tk->addr = vma->vm_start +
+    ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);


It seems strange to use 'pgoff' for dax pages and not for any other 
page.
Why? I'd rather pass correct pgoff from all callers of 
add_to_kill() and

avoid this special casing...


Because one fsdax page can be shared by multiple pgoffs.  I have to 
pass each pgoff in each iteration to calculate the address in vma 
(for tk->addr).  Other kinds of pages don't need this. They can get 
their unique address by calling "page_address_in_vma()".


IMO,   an fsdax page can be shared by multiple files rather than 
multiple pgoffs if fs query support reflink.   Because an page only 
located in an mapping(page->mapping is exclusive), hence it  only has 
an pgoff or index pointing at the node.


  or  I miss something for the feature ?  thanks,


Yes, a fsdax page is shared by multiple files because of reflink. I 
think my description of 'pgoff' here is not correct.  This 'pgoff' 
means the offset within the a file.  (We use rmap to find out all the 
sharing files and their offsets.)  So, I said that "can be shared by 
multiple pgoffs".  It's my bad.


I think I should name it another word to avoid misunderstandings.

IMO,  All the sharing files should be the same offset to share the fsdax 
page.  why not that ?  As you has said,  a shared fadax page should be 
inserted to different mapping files.  but page->index and page->mapping 
is exclusive.  hence an page only should be placed in an mapping tree.


And In the current patch,  we failed to found out that all process use 
the fsdax page shared by multiple files and kill them.



Thanks,


--
Thanks,
Ruan Shiyang.



So, I added this fsdax case here.  This patchset only implemented 
the fsdax case, other cases also need to be added here if to be 
implemented.



--
Thanks,
Ruan Shiyang.



+    tk->size_shift = dev_pagemap_mapping_shift(p, vma, 
tk->addr);

+    } else
  tk->size_shift = page_shift(compound_head(p));
    /*
@@ -495,7 +501,7 @@ static void collect_procs_anon(struct page 
*page, struct list_head *to_kill,

  if (!page_mapped_in_vma(page, vma))
  continue;
  if (vma->vm_mm == t->mm)
-    add_to_kill(t, page, vma, to_kill);
+    add_to_kill(t, page, NULL, 0, vma, to_kill);
  }
  }
  read_unlock(_lock);
@@ -505,24 +511,19 @@ static void collect_procs_anon(struct page 
*page, struct list_head *to_kill,

  /*
   * Collect processes when the error hit a file mapped page.
   */
-static void collect_procs_file(struct page *page, struct 
list_head *to_kill,

-    int force_early)
+static void collect_procs_file(struct page *page, struct 
address_space *mapping,

+    pgoff_t pgoff, struct list_head *to_kill, int force_early)
  {
  struct vm_area_struct *vma;
  struct task_struct *tsk;
-    struct address_space *mapping = page->mapping;
-    pgoff_t pgoff;
    i_mmap_lock_read(mapping);
  read_lock(_lock);
-    pgoff = page_to_pgoff(page);
  for_each_process(tsk) {
  struct task_struct *t = task_early_kill(tsk, force_early);
-
  if (!t)
  continue;
-    vma_interval_tree_foreach(vma, >i_mmap, pgoff,
-  pgoff) {
+    vma_interval_tree_foreach(vma, >i_mmap, pgoff, 
pgoff) {

  /*
   * Send early kill signal to tasks where a vma covers
   * the page but the corrupted page is not necessarily
@@ -531,7 +532,7 @@ static void collect_procs_file(struct page 
*page, struct list_head *to_kill,

   * to be informed of all such data corruptions.
   */

Re: [PATCH 04/10] mm, fsdax: Refactor memory-failure handler for dax mapping

[zhong jiang]

On 2021/1/12 10:55 上午, Ruan Shiyang wrote:



On 2021/1/6 下午11:41, Jan Kara wrote:

On Thu 31-12-20 00:55:55, Shiyang Ruan wrote:

The current memory_failure_dev_pagemap() can only handle single-mapped
dax page for fsdax mode.  The dax page could be mapped by multiple 
files

and offsets if we let reflink feature & fsdax mode work together.  So,
we refactor current implementation to support handle memory failure on
each file and offset.

Signed-off-by: Shiyang Ruan 


Overall this looks OK to me, a few comments below.


---
  fs/dax.c    | 21 +++
  include/linux/dax.h |  1 +
  include/linux/mm.h  |  9 +
  mm/memory-failure.c | 91 
++---

  4 files changed, 100 insertions(+), 22 deletions(-)


...

  @@ -345,9 +348,12 @@ static void add_to_kill(struct task_struct 
*tsk, struct page *p,

  }
    tk->addr = page_address_in_vma(p, vma);
-    if (is_zone_device_page(p))
-    tk->size_shift = dev_pagemap_mapping_shift(p, vma);
-    else
+    if (is_zone_device_page(p)) {
+    if (is_device_fsdax_page(p))
+    tk->addr = vma->vm_start +
+    ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);


It seems strange to use 'pgoff' for dax pages and not for any other 
page.

Why? I'd rather pass correct pgoff from all callers of add_to_kill() and
avoid this special casing...


Because one fsdax page can be shared by multiple pgoffs.  I have to 
pass each pgoff in each iteration to calculate the address in vma (for 
tk->addr).  Other kinds of pages don't need this. They can get their 
unique address by calling "page_address_in_vma()".


IMO,   an fsdax page can be shared by multiple files rather than 
multiple pgoffs if fs query support reflink.   Because an page only 
located in an mapping(page->mapping is exclusive),  hence it  only has 
an pgoff or index pointing at the node.


 or  I miss something for the feature ?  thanks,

So, I added this fsdax case here.  This patchset only implemented the 
fsdax case, other cases also need to be added here if to be implemented.



--
Thanks,
Ruan Shiyang.




+    tk->size_shift = dev_pagemap_mapping_shift(p, vma, tk->addr);
+    } else
  tk->size_shift = page_shift(compound_head(p));
    /*
@@ -495,7 +501,7 @@ static void collect_procs_anon(struct page 
*page, struct list_head *to_kill,

  if (!page_mapped_in_vma(page, vma))
  continue;
  if (vma->vm_mm == t->mm)
-    add_to_kill(t, page, vma, to_kill);
+    add_to_kill(t, page, NULL, 0, vma, to_kill);
  }
  }
  read_unlock(_lock);
@@ -505,24 +511,19 @@ static void collect_procs_anon(struct page 
*page, struct list_head *to_kill,

  /*
   * Collect processes when the error hit a file mapped page.
   */
-static void collect_procs_file(struct page *page, struct list_head 
*to_kill,

-    int force_early)
+static void collect_procs_file(struct page *page, struct 
address_space *mapping,

+    pgoff_t pgoff, struct list_head *to_kill, int force_early)
  {
  struct vm_area_struct *vma;
  struct task_struct *tsk;
-    struct address_space *mapping = page->mapping;
-    pgoff_t pgoff;
    i_mmap_lock_read(mapping);
  read_lock(_lock);
-    pgoff = page_to_pgoff(page);
  for_each_process(tsk) {
  struct task_struct *t = task_early_kill(tsk, force_early);
-
  if (!t)
  continue;
-    vma_interval_tree_foreach(vma, >i_mmap, pgoff,
-  pgoff) {
+    vma_interval_tree_foreach(vma, >i_mmap, pgoff, 
pgoff) {

  /*
   * Send early kill signal to tasks where a vma covers
   * the page but the corrupted page is not necessarily
@@ -531,7 +532,7 @@ static void collect_procs_file(struct page 
*page, struct list_head *to_kill,

   * to be informed of all such data corruptions.
   */
  if (vma->vm_mm == t->mm)
-    add_to_kill(t, page, vma, to_kill);
+    add_to_kill(t, page, mapping, pgoff, vma, to_kill);
  }
  }
  read_unlock(_lock);
@@ -550,7 +551,8 @@ static void collect_procs(struct page *page, 
struct list_head *tokill,

  if (PageAnon(page))
  collect_procs_anon(page, tokill, force_early);
  else
-    collect_procs_file(page, tokill, force_early);
+    collect_procs_file(page, page->mapping, page_to_pgoff(page),


Why not use page_mapping() helper here? It would be safer for THPs if 
they

ever get here...

    Honza

[PATCH] power: supply: cpcap-charger: Make cpcap_charger_voltage_to_regval static

[zhong jiang]

The GCC complains the following case when compiling kernel.

drivers/power/supply/cpcap-charger.c:563:5: warning: symbol 
'cpcap_charger_voltage_to_regval' was not declared. Should it be static?

Signed-off-by: zhong jiang 
---
 drivers/power/supply/cpcap-charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/cpcap-charger.c 
b/drivers/power/supply/cpcap-charger.c
index 40d96b8..c0d452e 100644
--- a/drivers/power/supply/cpcap-charger.c
+++ b/drivers/power/supply/cpcap-charger.c
@@ -560,7 +560,7 @@ static void cpcap_charger_update_state(struct 
cpcap_charger_ddata *ddata,
dev_dbg(ddata->dev, "state: %s\n", status);
 }
 
-int cpcap_charger_voltage_to_regval(int voltage)
+static int cpcap_charger_voltage_to_regval(int voltage)
 {
int offset;
 
-- 
1.7.12.4

Re: [PATCH RESEND v2] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/14 18:06, Jerome Pouiller wrote:
> On Monday 14 October 2019 11:53:19 CEST Jérôme Pouiller wrote:
> [...]
>> Hello Zhong,
>>
>> Now, I see the problem. It happens when CONFIG_MMC=m and CONFIG_WFX=y
>> (if CONFIG_WFX=m, it works).
>>
>> I think the easiest way to solve problem is to disallow CONFIG_WFX=y if 
>> CONFIG_MMC=m.
>>
>> This solution impacts users who want to use SPI bus with configuration:
>> CONFIG_WFX=y + CONFIG_SPI=y + CONFIG_MMC=m. However, I think this is a
>> twisted case. So, I think it won't be missed.
>>
>> I think that patch below do the right thing:
>>
>> -8<--8<--8<-
>>
>> diff --git i/drivers/staging/wfx/Kconfig w/drivers/staging/wfx/Kconfig
>> index 9b8a1c7a9e90..833f3b05b6b4 100644
>> --- i/drivers/staging/wfx/Kconfig
>> +++ w/drivers/staging/wfx/Kconfig
>> @@ -1,7 +1,7 @@
>>  config WFX
>> tristate "Silicon Labs wireless chips WF200 and further"
>> depends on MAC80211
>> -   depends on (SPI || MMC)
>> +   depends on (MMC=m && m) || MMC=y || (SPI && MMC!=m)
>> help
>>   This is a driver for Silicons Labs WFxxx series (WF200 and further)
>>   chipsets. This chip can be found on SPI or SDIO buses.
>>
>>
>>
> An alternative (more understandable?):
>
> diff --git i/drivers/staging/wfx/Kconfig w/drivers/staging/wfx/Kconfig
> index 9b8a1c7a9e90..83ee4d0ca8c6 100644
> --- i/drivers/staging/wfx/Kconfig
> +++ w/drivers/staging/wfx/Kconfig
> @@ -1,6 +1,7 @@
>  config WFX
> tristate "Silicon Labs wireless chips WF200 and further"
> depends on MAC80211
> +   depends on MMC || !MMC # do not allow WFX=y if MMC=m
> depends on (SPI || MMC)
> help
>   This is a driver for Silicons Labs WFxxx series (WF200 and further)
>
>
Hi,  Jerome

It's better to understandable.
Could you send the patch or want to repost by me?

Thanks,
zhong jiang

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/10/17 8:49, Andrew Morton wrote:
> On Wed, 16 Oct 2019 17:07:44 +0800 zhong jiang  wrote:
>
>>>> --- a/mm/gup.c~a
>>>> +++ a/mm/gup.c
>>>> @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages(
>>>>   bool drain_allow = true;
>>>>   bool migrate_allow = true;
>>>>   LIST_HEAD(cma_page_list);
>>>> +long ret;
>>>> check_again:
>>>>   for (i = 0; i < nr_pages;) {
>>>> @@ -1511,17 +1512,18 @@ check_again:
>>>>* again migrating any new CMA pages which we failed to isolate
>>>>* earlier.
>>>>*/
>>>> -nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages,
>>>> +ret = __get_user_pages_locked(tsk, mm, start, nr_pages,
>>>>  pages, vmas, NULL,
>>>>  gup_flags);
>>>>   -if ((nr_pages > 0) && migrate_allow) {
>>>> +nr_pages = ret;
>>>> +if (ret > 0 && migrate_allow) {
>>>>   drain_allow = true;
>>>>   goto check_again;
>>>>   }
>>>>   }
>>>>   -return nr_pages;
>>>> +return ret;
>>>>   }
>>>>   #else
>>>>   static long check_and_migrate_cma_pages(struct task_struct *tsk,
>>>>
>>> +1 for this approach, please.
>>>
>>>
>>> thanks,
>> Hi,  Andrew
>>
>> I didn't see the fix for the issue in the upstream. Your proposal should be
>> appiled to upstream. Could you appiled the patch or  repost by me ?
> Forgotten about it ;)  Please send a patch sometime?
>
> .
>
I will  repost the patch as your suggestion.  Thanks,

Sincerely,
zhong jiang

[PATCH] staging: rtl8723bs: remove an redundant null check before kfree()

[zhong jiang]

kfree() has taken null pointer into account. hence it is safe to remove
the unnecessary check.

Signed-off-by: zhong jiang 
---
 drivers/staging/rtl8723bs/core/rtw_xmit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_xmit.c 
b/drivers/staging/rtl8723bs/core/rtw_xmit.c
index 7011c2a..4597f4f 100644
--- a/drivers/staging/rtl8723bs/core/rtw_xmit.c
+++ b/drivers/staging/rtl8723bs/core/rtw_xmit.c
@@ -2210,8 +2210,7 @@ void rtw_free_hwxmits(struct adapter *padapter)
struct xmit_priv *pxmitpriv = >xmitpriv;
 
hwxmits = pxmitpriv->hwxmits;
-   if (hwxmits)
-   kfree(hwxmits);
+   kfree(hwxmits);
 }
 
 void rtw_init_hwxmits(struct hw_xmit *phwxmit, sint entry)
-- 
1.7.12.4

Re: [PATCH RESEND v2] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/14 18:06, Jerome Pouiller wrote:
> On Monday 14 October 2019 11:53:19 CEST Jérôme Pouiller wrote:
> [...]
>> Hello Zhong,
>>
>> Now, I see the problem. It happens when CONFIG_MMC=m and CONFIG_WFX=y
>> (if CONFIG_WFX=m, it works).
>>
>> I think the easiest way to solve problem is to disallow CONFIG_WFX=y if 
>> CONFIG_MMC=m.
>>
>> This solution impacts users who want to use SPI bus with configuration:
>> CONFIG_WFX=y + CONFIG_SPI=y + CONFIG_MMC=m. However, I think this is a
>> twisted case. So, I think it won't be missed.
>>
>> I think that patch below do the right thing:
>>
>> -8<--8<--8<-
>>
>> diff --git i/drivers/staging/wfx/Kconfig w/drivers/staging/wfx/Kconfig
>> index 9b8a1c7a9e90..833f3b05b6b4 100644
>> --- i/drivers/staging/wfx/Kconfig
>> +++ w/drivers/staging/wfx/Kconfig
>> @@ -1,7 +1,7 @@
>>  config WFX
>> tristate "Silicon Labs wireless chips WF200 and further"
>> depends on MAC80211
>> -   depends on (SPI || MMC)
>> +   depends on (MMC=m && m) || MMC=y || (SPI && MMC!=m)
>> help
>>   This is a driver for Silicons Labs WFxxx series (WF200 and further)
>>   chipsets. This chip can be found on SPI or SDIO buses.
>>
>>
>>
> An alternative (more understandable?):
>
> diff --git i/drivers/staging/wfx/Kconfig w/drivers/staging/wfx/Kconfig
> index 9b8a1c7a9e90..83ee4d0ca8c6 100644
> --- i/drivers/staging/wfx/Kconfig
> +++ w/drivers/staging/wfx/Kconfig
> @@ -1,6 +1,7 @@
>  config WFX
> tristate "Silicon Labs wireless chips WF200 and further"
>     depends on MAC80211
> +   depends on MMC || !MMC # do not allow WFX=y if MMC=m
> depends on (SPI || MMC)
> help
>   This is a driver for Silicons Labs WFxxx series (WF200 and further)
>
>
It's better and clear.  Thanks

sincerely,
zhong jiang

Re: [PATCH v3] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/12 23:32, Greg KH wrote:
> On Sat, Oct 12, 2019 at 06:54:53PM +0800, zhong jiang wrote:
>> I hit the following error when compile the kernel.
>>
>> drivers/staging/wfx/main.o: In function `wfx_core_init':
>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: 
>> undefined reference to `sdio_register_driver'
>> drivers/staging/wfx/main.o: In function `wfx_core_exit':
>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: 
>> undefined reference to `sdio_unregister_driver'
>> drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
>> `sdio_register_driver'
>> drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
>> `sdio_unregister_driver'
>>
>> Signed-off-by: zhong jiang 
>> ---
>> v2->v3:
>> We'd better not use #ifdef in .c file to use IS_ENABLED instead.
>>
>> v1->v2:
>> We should prefer to current dependencies rather than force to enable. 
>>
>>  drivers/staging/wfx/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/wfx/Makefile b/drivers/staging/wfx/Makefile
>> index 0d9c1ed..77d68b7 100644
>> --- a/drivers/staging/wfx/Makefile
>> +++ b/drivers/staging/wfx/Makefile
>> @@ -19,6 +19,6 @@ wfx-y := \
>>  sta.o \
>>  debug.o
>>  wfx-$(CONFIG_SPI) += bus_spi.o
>> -wfx-$(subst m,y,$(CONFIG_MMC)) += bus_sdio.o
>> +wfx-$(CONFIG_MMC) += bus_sdio.o
>>  
>>  obj-$(CONFIG_WFX) += wfx.o
> How does this change any of the existing logic?  What does this really
> change to solve the issue?  I thought you were going to fix this up as I
> suggested in my last email?
Yep,  It's my stupid fault.  I disable the CONFIG_SPI to test the issue.  :-(

Thanks,
zhong jiang
> thanks,
>
> greg k-h
>
> .
>

[PATCH v3] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

I hit the following error when compile the kernel.

drivers/staging/wfx/main.o: In function `wfx_core_init':
/home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: undefined 
reference to `sdio_register_driver'
drivers/staging/wfx/main.o: In function `wfx_core_exit':
/home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: undefined 
reference to `sdio_unregister_driver'
drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
`sdio_register_driver'
drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
`sdio_unregister_driver'

Signed-off-by: zhong jiang 
---
v2->v3:
We'd better not use #ifdef in .c file to use IS_ENABLED instead.

v1->v2:
We should prefer to current dependencies rather than force to enable. 

 drivers/staging/wfx/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/Makefile b/drivers/staging/wfx/Makefile
index 0d9c1ed..77d68b7 100644
--- a/drivers/staging/wfx/Makefile
+++ b/drivers/staging/wfx/Makefile
@@ -19,6 +19,6 @@ wfx-y := \
sta.o \
debug.o
 wfx-$(CONFIG_SPI) += bus_spi.o
-wfx-$(subst m,y,$(CONFIG_MMC)) += bus_sdio.o
+wfx-$(CONFIG_MMC) += bus_sdio.o
 
 obj-$(CONFIG_WFX) += wfx.o
-- 
1.7.12.4

Re: [PATCH RESEND v2] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/12 1:03, Greg KH wrote:
> On Sat, Oct 12, 2019 at 12:34:07AM +0800, zhong jiang wrote:
>> I hit the following error when compile the kernel.
>>
>> drivers/staging/wfx/main.o: In function `wfx_core_init':
>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: 
>> undefined reference to `sdio_register_driver'
>> drivers/staging/wfx/main.o: In function `wfx_core_exit':
>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: 
>> undefined reference to `sdio_unregister_driver'
>> drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
>> `sdio_register_driver'
>> drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
>> `sdio_unregister_driver'
>>
>> Signed-off-by: zhong jiang 
>> ---
>>  drivers/staging/wfx/Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> What changed from v1?  Always put that below the --- line.
>
> v3 please?
Fine,  I will repost in v3.  Thanks,

Sincerely,
zhong jiang
> thanks,
>
> greg k-h
>
> .
>

[PATCH RESEND v2] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

I hit the following error when compile the kernel.

drivers/staging/wfx/main.o: In function `wfx_core_init':
/home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: undefined 
reference to `sdio_register_driver'
drivers/staging/wfx/main.o: In function `wfx_core_exit':
/home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: undefined 
reference to `sdio_unregister_driver'
drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
`sdio_register_driver'
drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
`sdio_unregister_driver'

Signed-off-by: zhong jiang 
---
 drivers/staging/wfx/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/Makefile b/drivers/staging/wfx/Makefile
index 0d9c1ed..77d68b7 100644
--- a/drivers/staging/wfx/Makefile
+++ b/drivers/staging/wfx/Makefile
@@ -19,6 +19,6 @@ wfx-y := \
sta.o \
debug.o
 wfx-$(CONFIG_SPI) += bus_spi.o
-wfx-$(subst m,y,$(CONFIG_MMC)) += bus_sdio.o
+wfx-$(CONFIG_MMC) += bus_sdio.o
 
 obj-$(CONFIG_WFX) += wfx.o
-- 
1.7.12.4

Re: [PATCH] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/12 0:16, Greg KH wrote:
> On Fri, Oct 11, 2019 at 11:51:16PM +0800, zhong jiang wrote:
>> On 2019/10/11 17:02, Greg KH wrote:
>>> On Fri, Oct 11, 2019 at 08:40:08AM +, Jerome Pouiller wrote:
>>>> On Friday 11 October 2019 06:26:16 CEST Greg KH wrote:
>>>>> CAUTION: This email originated from outside of the organization. Do not 
>>>> click links or open attachments unless you recognize the sender and know 
>>>> the 
>>>> content is safe.
>>>>> On Fri, Oct 11, 2019 at 11:02:19AM +0800, zhong jiang wrote:
>>>>>> I hit the following error when compile the kernel.
>>>>>>
>>>>>> drivers/staging/wfx/main.o: In function `wfx_core_init':
>>>>>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: 
>>>> undefined reference to `sdio_register_driver'
>>>>>> drivers/staging/wfx/main.o: In function `wfx_core_exit':
>>>>>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: 
>>>> undefined reference to `sdio_unregister_driver'
>>>>>> drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
>>>> `sdio_register_driver'
>>>>>> drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
>>>> `sdio_unregister_driver'
>>>>>> Signed-off-by: zhong jiang 
>>>>>> ---
>>>>>>  drivers/staging/wfx/Kconfig  | 3 ++-
>>>>>>  drivers/staging/wfx/Makefile | 5 +++--
>>>>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/wfx/Kconfig b/drivers/staging/wfx/Kconfig
>>>>>> index 9b8a1c7..4d045513 100644
>>>>>> --- a/drivers/staging/wfx/Kconfig
>>>>>> +++ b/drivers/staging/wfx/Kconfig
>>>>>> @@ -1,7 +1,8 @@
>>>>>>  config WFX
>>>>>>   tristate "Silicon Labs wireless chips WF200 and further"
>>>>>>   depends on MAC80211
>>>>>> - depends on (SPI || MMC)
>>>>>> + depends on SPI
>>>>>> + select MMC
>>>>> How about:
>>>>> depends on (SPI && MMC)
>>>> I dislike to force user to enable both buses while only one of them is 
>>>> sufficient. I would prefer to keep current dependencies and to add
>>>> #ifdef around problematic functions.
>>> Yes, that's the better thing to do here overall.
>>>
>>> zhong, can you work on that?
>> How about the following patch ?
>>
>> diff --git a/drivers/staging/wfx/Makefile b/drivers/staging/wfx/Makefile
>> index 0d9c1ed..77d68b7 100644
>> --- a/drivers/staging/wfx/Makefile
>> +++ b/drivers/staging/wfx/Makefile
>> @@ -19,6 +19,6 @@ wfx-y := \
>> sta.o \
>> debug.o
>>  wfx-$(CONFIG_SPI) += bus_spi.o
>> -wfx-$(subst m,y,$(CONFIG_MMC)) += bus_sdio.o
>> +wfx-$(CONFIG_MMC) += bus_sdio.o
>>
>>  obj-$(CONFIG_WFX) += wfx.o
>> diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
>> index d2508bc..26720a4 100644
>> --- a/drivers/staging/wfx/main.c
>> +++ b/drivers/staging/wfx/main.c
>> @@ -484,16 +484,19 @@ static int __init wfx_core_init(void)
>>
>> if (IS_ENABLED(CONFIG_SPI))
>> ret = spi_register_driver(_spi_driver);
>> -   if (IS_ENABLED(CONFIG_MMC) && !ret)
>> +#ifdef CONFIG_MMC
>> +   if (!ret)
>> ret = sdio_register_driver(_sdio_driver);
> Put this in an inline function in the .h file so that you just call:
>   wfx_register_sdio_driver()
> and it does what is needed to be done depending on if CONFIG_MMC is
> enabled or not (note, your check here isn't quite correct, I think you
> need to do IS_ENABLED())
>
> Same for spi.
>
> We really do not like #ifdefs in .c files.
You're right.  I should not use #ifdef here.

Thanks,
zhong jiang
> thanks,
>
> greg k-h
>
> .
>

Re: [PATCH] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/11 23:55, Jerome Pouiller wrote:
> On Friday 11 October 2019 17:51:29 CEST zhong jiang wrote:
> [...]
>> How about the following patch ?
>>
>> diff --git a/drivers/staging/wfx/Makefile b/drivers/staging/wfx/Makefile
>> index 0d9c1ed..77d68b7 100644
>> --- a/drivers/staging/wfx/Makefile
>> +++ b/drivers/staging/wfx/Makefile
>> @@ -19,6 +19,6 @@ wfx-y := \
>> sta.o \
>> debug.o
>>  wfx-$(CONFIG_SPI) += bus_spi.o
>> -wfx-$(subst m,y,$(CONFIG_MMC)) += bus_sdio.o
>> +wfx-$(CONFIG_MMC) += bus_sdio.o
>>
>>  obj-$(CONFIG_WFX) += wfx.o
>> diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
>> index d2508bc..26720a4 100644
>> --- a/drivers/staging/wfx/main.c
>> +++ b/drivers/staging/wfx/main.c
>> @@ -484,16 +484,19 @@ static int __init wfx_core_init(void)
>>
>> if (IS_ENABLED(CONFIG_SPI))
>> ret = spi_register_driver(_spi_driver);
>> -   if (IS_ENABLED(CONFIG_MMC) && !ret)
>> +#ifdef CONFIG_MMC
>> +   if (!ret)
>> ret = sdio_register_driver(_sdio_driver);
>> +#endif
>> return ret
>>  }
>>  module_init(wfx_core_init);
>>
>>  static void __exit wfx_core_exit(void)
>>  {
>> -   if (IS_ENABLED(CONFIG_MMC))
>> -   sdio_unregister_driver(_sdio_driver);
>> +#ifdef CONFIG_MMC
>> +   sdio_unregister_driver(_sdio_driver);
>> +#endif
>> if (IS_ENABLED(CONFIG_SPI))
>> spi_unregister_driver(_spi_driver);
>>  }
> Hello zhong,
>
> Can you also check the case where CONFIG_SPI is not set?
I have tested the case and it works well when CONFIG_SPI is not set.

Thanks,
zhong jiang
> Thank you,
>

Re: [PATCH] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/11 17:02, Greg KH wrote:
> On Fri, Oct 11, 2019 at 08:40:08AM +, Jerome Pouiller wrote:
>> On Friday 11 October 2019 06:26:16 CEST Greg KH wrote:
>>> CAUTION: This email originated from outside of the organization. Do not 
>> click links or open attachments unless you recognize the sender and know the 
>> content is safe.
>>>
>>> On Fri, Oct 11, 2019 at 11:02:19AM +0800, zhong jiang wrote:
>>>> I hit the following error when compile the kernel.
>>>>
>>>> drivers/staging/wfx/main.o: In function `wfx_core_init':
>>>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: 
>> undefined reference to `sdio_register_driver'
>>>> drivers/staging/wfx/main.o: In function `wfx_core_exit':
>>>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: 
>> undefined reference to `sdio_unregister_driver'
>>>> drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
>> `sdio_register_driver'
>>>> drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
>> `sdio_unregister_driver'
>>>> Signed-off-by: zhong jiang 
>>>> ---
>>>>  drivers/staging/wfx/Kconfig  | 3 ++-
>>>>  drivers/staging/wfx/Makefile | 5 +++--
>>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/wfx/Kconfig b/drivers/staging/wfx/Kconfig
>>>> index 9b8a1c7..4d045513 100644
>>>> --- a/drivers/staging/wfx/Kconfig
>>>> +++ b/drivers/staging/wfx/Kconfig
>>>> @@ -1,7 +1,8 @@
>>>>  config WFX
>>>>   tristate "Silicon Labs wireless chips WF200 and further"
>>>>   depends on MAC80211
>>>> - depends on (SPI || MMC)
>>>> + depends on SPI
>>>> + select MMC
>>> How about:
>>> depends on (SPI && MMC)
>> I dislike to force user to enable both buses while only one of them is 
>> sufficient. I would prefer to keep current dependencies and to add
>> #ifdef around problematic functions.
> Yes, that's the better thing to do here overall.
>
> zhong, can you work on that?
How about the following patch ?

diff --git a/drivers/staging/wfx/Makefile b/drivers/staging/wfx/Makefile
index 0d9c1ed..77d68b7 100644
--- a/drivers/staging/wfx/Makefile
+++ b/drivers/staging/wfx/Makefile
@@ -19,6 +19,6 @@ wfx-y := \
sta.o \
debug.o
 wfx-$(CONFIG_SPI) += bus_spi.o
-wfx-$(subst m,y,$(CONFIG_MMC)) += bus_sdio.o
+wfx-$(CONFIG_MMC) += bus_sdio.o

 obj-$(CONFIG_WFX) += wfx.o
diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index d2508bc..26720a4 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -484,16 +484,19 @@ static int __init wfx_core_init(void)

if (IS_ENABLED(CONFIG_SPI))
ret = spi_register_driver(_spi_driver);
-   if (IS_ENABLED(CONFIG_MMC) && !ret)
+#ifdef CONFIG_MMC
+   if (!ret)
ret = sdio_register_driver(_sdio_driver);
+#endif
return ret;
 }
 module_init(wfx_core_init);

 static void __exit wfx_core_exit(void)
 {
-   if (IS_ENABLED(CONFIG_MMC))
-   sdio_unregister_driver(_sdio_driver);
+#ifdef CONFIG_MMC
+   sdio_unregister_driver(_sdio_driver);
+#endif
if (IS_ENABLED(CONFIG_SPI))
spi_unregister_driver(_spi_driver);
 }
--

Thanks,
zhong jiang
> thanks,
>
> greg k-h
>
> .
>

Re: [PATCH] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/11 17:02, Greg KH wrote:
> On Fri, Oct 11, 2019 at 08:40:08AM +, Jerome Pouiller wrote:
>> On Friday 11 October 2019 06:26:16 CEST Greg KH wrote:
>>> CAUTION: This email originated from outside of the organization. Do not 
>> click links or open attachments unless you recognize the sender and know the 
>> content is safe.
>>>
>>> On Fri, Oct 11, 2019 at 11:02:19AM +0800, zhong jiang wrote:
>>>> I hit the following error when compile the kernel.
>>>>
>>>> drivers/staging/wfx/main.o: In function `wfx_core_init':
>>>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: 
>> undefined reference to `sdio_register_driver'
>>>> drivers/staging/wfx/main.o: In function `wfx_core_exit':
>>>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: 
>> undefined reference to `sdio_unregister_driver'
>>>> drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
>> `sdio_register_driver'
>>>> drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
>> `sdio_unregister_driver'
>>>> Signed-off-by: zhong jiang 
>>>> ---
>>>>  drivers/staging/wfx/Kconfig  | 3 ++-
>>>>  drivers/staging/wfx/Makefile | 5 +++--
>>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/wfx/Kconfig b/drivers/staging/wfx/Kconfig
>>>> index 9b8a1c7..4d045513 100644
>>>> --- a/drivers/staging/wfx/Kconfig
>>>> +++ b/drivers/staging/wfx/Kconfig
>>>> @@ -1,7 +1,8 @@
>>>>  config WFX
>>>>   tristate "Silicon Labs wireless chips WF200 and further"
>>>>   depends on MAC80211
>>>> - depends on (SPI || MMC)
>>>> + depends on SPI
>>>> + select MMC
>>> How about:
>>> depends on (SPI && MMC)
>> I dislike to force user to enable both buses while only one of them is 
>> sufficient. I would prefer to keep current dependencies and to add
>> #ifdef around problematic functions.
> Yes, that's the better thing to do here overall.
>
> zhong, can you work on that?
I will repost as Jerome said.  Thanks

Sincerely,
zhong jiang
> thanks,
>
> greg k-h
>
> .
>

[PATCH v2] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

drivers/staging/wfx/main.o: In function `wfx_core_init':
/home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: undefined 
reference to `sdio_register_driver'
drivers/staging/wfx/main.o: In function `wfx_core_exit':
/home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: undefined 
reference to `sdio_unregister_driver'
drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
`sdio_register_driver'
drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
`sdio_unregister_driver'

Signed-off-by: zhong jiang 
---
 drivers/staging/wfx/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/wfx/Kconfig b/drivers/staging/wfx/Kconfig
index 9b8a1c7..2d73722 100644
--- a/drivers/staging/wfx/Kconfig
+++ b/drivers/staging/wfx/Kconfig
@@ -1,7 +1,7 @@
 config WFX
tristate "Silicon Labs wireless chips WF200 and further"
depends on MAC80211
-   depends on (SPI || MMC)
+   depends on (SPI && MMC)
help
  This is a driver for Silicons Labs WFxxx series (WF200 and further)
  chipsets. This chip can be found on SPI or SDIO buses.
-- 
1.7.12.4

Re: [PATCH] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

On 2019/10/11 12:26, Greg KH wrote:
> On Fri, Oct 11, 2019 at 11:02:19AM +0800, zhong jiang wrote:
>> I hit the following error when compile the kernel.
>>
>> drivers/staging/wfx/main.o: In function `wfx_core_init':
>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: 
>> undefined reference to `sdio_register_driver'
>> drivers/staging/wfx/main.o: In function `wfx_core_exit':
>> /home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: 
>> undefined reference to `sdio_unregister_driver'
>> drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
>> `sdio_register_driver'
>> drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
>> `sdio_unregister_driver'
>>
>> Signed-off-by: zhong jiang 
>> ---
>>  drivers/staging/wfx/Kconfig  | 3 ++-
>>  drivers/staging/wfx/Makefile | 5 +++--
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/wfx/Kconfig b/drivers/staging/wfx/Kconfig
>> index 9b8a1c7..4d045513 100644
>> --- a/drivers/staging/wfx/Kconfig
>> +++ b/drivers/staging/wfx/Kconfig
>> @@ -1,7 +1,8 @@
>>  config WFX
>>  tristate "Silicon Labs wireless chips WF200 and further"
>>  depends on MAC80211
>> -depends on (SPI || MMC)
>> +depends on SPI
>> +select MMC
> How about:
>   depends on (SPI && MMC)
>
> instead?
Yep,  That is better.  Will repost in v2.  Thanks,

Sincerely,
zhong jiang
> thanks,
>
> greg k-h
>
> .
>

[PATCH] staging: wfx: fix an undefined reference error when CONFIG_MMC=m

[zhong jiang]

I hit the following error when compile the kernel.

drivers/staging/wfx/main.o: In function `wfx_core_init':
/home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:488: undefined 
reference to `sdio_register_driver'
drivers/staging/wfx/main.o: In function `wfx_core_exit':
/home/z00352263/linux-next/linux-next/drivers/staging/wfx/main.c:496: undefined 
reference to `sdio_unregister_driver'
drivers/staging/wfx/main.o:(.debug_addr+0x1a8): undefined reference to 
`sdio_register_driver'
drivers/staging/wfx/main.o:(.debug_addr+0x6f0): undefined reference to 
`sdio_unregister_driver'

Signed-off-by: zhong jiang 
---
 drivers/staging/wfx/Kconfig  | 3 ++-
 drivers/staging/wfx/Makefile | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/wfx/Kconfig b/drivers/staging/wfx/Kconfig
index 9b8a1c7..4d045513 100644
--- a/drivers/staging/wfx/Kconfig
+++ b/drivers/staging/wfx/Kconfig
@@ -1,7 +1,8 @@
 config WFX
tristate "Silicon Labs wireless chips WF200 and further"
depends on MAC80211
-   depends on (SPI || MMC)
+   depends on SPI
+   select MMC
help
  This is a driver for Silicons Labs WFxxx series (WF200 and further)
  chipsets. This chip can be found on SPI or SDIO buses.
diff --git a/drivers/staging/wfx/Makefile b/drivers/staging/wfx/Makefile
index 0d9c1ed..fc30b49 100644
--- a/drivers/staging/wfx/Makefile
+++ b/drivers/staging/wfx/Makefile
@@ -17,8 +17,9 @@ wfx-y := \
key.o \
main.o \
sta.o \
-   debug.o
+   debug.o \
+   bus_sdio.o
+
 wfx-$(CONFIG_SPI) += bus_spi.o
-wfx-$(subst m,y,$(CONFIG_MMC)) += bus_sdio.o
 
 obj-$(CONFIG_WFX) += wfx.o
-- 
1.7.12.4

[PATCH v2 4/4] media: v4l2-dv-timings: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/media/v4l2-core/v4l2-dv-timings.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-dv-timings.c 
b/drivers/media/v4l2-core/v4l2-dv-timings.c
index 4f23e93..2b399c0 100644
--- a/drivers/media/v4l2-core/v4l2-dv-timings.c
+++ b/drivers/media/v4l2-core/v4l2-dv-timings.c
@@ -757,7 +757,7 @@ bool v4l2_detect_gtf(unsigned frame_height,
pix_clk = pix_clk / GTF_PXL_CLK_GRAN * GTF_PXL_CLK_GRAN;
 
hsync = (frame_width * 8 + 50) / 100;
-   hsync = ((hsync + GTF_CELL_GRAN / 2) / GTF_CELL_GRAN) * GTF_CELL_GRAN;
+   hsync = DIV_ROUND_CLOSEST(hsync, GTF_CELL_GRAN) * GTF_CELL_GRAN;
 
h_fp = h_blank / 2 - hsync;
 
-- 
1.7.12.4

[PATCH v2 3/4] media: uvcvideo: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/media/usb/uvc/uvc_ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index e399b9f..9f3697161 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1604,8 +1604,8 @@ int uvc_ctrl_set(struct uvc_fh *handle,
if (step == 0)
step = 1;
 
-   xctrl->value = min + ((u32)(xctrl->value - min) + step / 2)
-/ step * step;
+   xctrl->value = min + DIV_ROUND_CLOSEST((u32)(xctrl->value - 
min),
+   step) * step;
if (mapping->data_type == UVC_CTRL_DATA_TYPE_SIGNED)
xctrl->value = clamp(xctrl->value, min, max);
else
-- 
1.7.12.4

[PATCH v2 0/4] media: Use DIV_ROUND_CLOSEST directly

[zhong jiang]

With the help of Coccinelle. I find some place that DIV_ROUND_CLOSEST
can replace it directly.

v1->v2:
   patch 1: remove mt312_div() to use the DIV_ROUND_CLOSEST directly.

zhong jiang (4):
  media: dvb-frontends: Use DIV_ROUND_CLOSEST directly to make it
readable
  media: tuners/qm1d1c0042: Use DIV_ROUND_CLOSEST directly to make it
readable
  media: uvcvideo: Use DIV_ROUND_CLOSEST directly to make it readable
  media: v4l2-dv-timings: Use DIV_ROUND_CLOSEST directly to make it
readable

 drivers/media/dvb-frontends/mt312.c   | 14 +-
 drivers/media/tuners/qm1d1c0042.c |  2 +-
 drivers/media/usb/uvc/uvc_ctrl.c  |  4 ++--
 drivers/media/v4l2-core/v4l2-dv-timings.c |  2 +-
 4 files changed, 9 insertions(+), 13 deletions(-)

-- 
1.7.12.4

[PATCH v2 2/4] media: tuners/qm1d1c0042: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/media/tuners/qm1d1c0042.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/tuners/qm1d1c0042.c 
b/drivers/media/tuners/qm1d1c0042.c
index 83ca5dc..0e26d22 100644
--- a/drivers/media/tuners/qm1d1c0042.c
+++ b/drivers/media/tuners/qm1d1c0042.c
@@ -206,7 +206,7 @@ static int qm1d1c0042_set_params(struct dvb_frontend *fe)
if (ret < 0)
return ret;
 
-   a = (freq + state->cfg.xtal_freq / 2) / state->cfg.xtal_freq;
+   a = DIV_ROUND_CLOSEST(freq, state->cfg.xtal_freq);
 
state->regs[0x06] &= 0x40;
state->regs[0x06] |= (a - 12) / 4;
-- 
1.7.12.4

[PATCH v2 1/4] media: dvb-frontends: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/media/dvb-frontends/mt312.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/media/dvb-frontends/mt312.c 
b/drivers/media/dvb-frontends/mt312.c
index 7cae7d6..5c4090c 100644
--- a/drivers/media/dvb-frontends/mt312.c
+++ b/drivers/media/dvb-frontends/mt312.c
@@ -135,11 +135,6 @@ static inline int mt312_writereg(struct mt312_state *state,
return mt312_write(state, reg, , 1);
 }
 
-static inline u32 mt312_div(u32 a, u32 b)
-{
-   return (a + (b / 2)) / b;
-}
-
 static int mt312_reset(struct mt312_state *state, const u8 full)
 {
return mt312_writereg(state, RESET, full ? 0x80 : 0x40);
@@ -187,7 +182,7 @@ static int mt312_get_symbol_rate(struct mt312_state *state, 
u32 *sr)
monitor = (buf[0] << 8) | buf[1];
 
dprintk("sr(auto) = %u\n",
-  mt312_div(monitor * 15625, 4));
+   DIV_ROUND_CLOSEST(monitor * 15625, 4));
} else {
ret = mt312_writereg(state, MON_CTRL, 0x05);
if (ret < 0)
@@ -291,10 +286,11 @@ static int mt312_initfe(struct dvb_frontend *fe)
}
 
/* SYS_CLK */
-   buf[0] = mt312_div(state->xtal * state->freq_mult * 2, 100);
+   buf[0] = DIV_ROUND_CLOSEST(state->xtal *
+   state->freq_mult * 2, 100);
 
/* DISEQC_RATIO */
-   buf[1] = mt312_div(state->xtal, 22000 * 4);
+   buf[1] = DIV_ROUND_CLOSEST(state->xtal, 22000 * 4);
 
ret = mt312_write(state, SYS_CLK, buf, sizeof(buf));
if (ret < 0)
@@ -610,7 +606,7 @@ static int mt312_set_frontend(struct dvb_frontend *fe)
}
 
/* sr = (u16)(sr * 256.0 / 100.0) */
-   sr = mt312_div(p->symbol_rate * 4, 15625);
+   sr = DIV_ROUND_CLOSEST(p->symbol_rate * 4, 15625);
 
/* SYM_RATE */
buf[0] = (sr >> 8) & 0x3f;
-- 
1.7.12.4

Re: [PATCH 1/4] media: dvb-frontends: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

On 2019/10/1 19:15, Sean Young wrote:
> Hi,
>
> On Fri, Sep 06, 2019 at 12:14:49AM +0800, zhong jiang wrote:
>> The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
>> but is perhaps more readable.
>>
>> Signed-off-by: zhong jiang 
>> ---
>>  drivers/media/dvb-frontends/mt312.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/dvb-frontends/mt312.c 
>> b/drivers/media/dvb-frontends/mt312.c
>> index 7cae7d6..251ff41 100644
>> --- a/drivers/media/dvb-frontends/mt312.c
>> +++ b/drivers/media/dvb-frontends/mt312.c
>> @@ -137,7 +137,7 @@ static inline int mt312_writereg(struct mt312_state 
>> *state,
>>  
>>  static inline u32 mt312_div(u32 a, u32 b)
>>  {
>> -return (a + (b / 2)) / b;
>> +return DIV_ROUND_CLOSEST(a, b);
> Well spotted, that is absolutely correct. However now mt312_div() is just
> a wrapper for DIV_ROUND_CLOSEST() -- and even marked inline. Really all
> the callers to mt312_div() should be replaced with DIV_ROUND_CLOSEST().
Thanks for your suggestion.   I will use DIV_ROUND_CLOSEST directly in v2.

Sincerely,
zhong jiang
> Thanks,
>
> Sean
>
> .
>

[PATCH] broadcom: Fix an compile warning in nvram_init

[zhong jiang]

I hit the following error when compile the kernel.

drivers/firmware/broadcom/bcm47xx_nvram.c: In function ‘nvram_init’:
./include/linux/kern_levels.h:5:18: warning: format ‘%zu’ expects argument of 
type ‘size_t’, but argument 2 has type ‘u32 {aka unsigned int}’ [-Wformat=]
 #define KERN_SOH "\001"  /* ASCII Start Of Header */
  ^
./include/linux/kern_levels.h:11:18: note: in expansion of macro ‘KERN_SOH’
 #define KERN_ERR KERN_SOH "3" /* error conditions */
  ^~~~
./include/linux/printk.h:304:9: note: in expansion of macro ‘KERN_ERR’
  printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
 ^~~~
drivers/firmware/broadcom/bcm47xx_nvram.c:151:4: note: in expansion of macro 
‘pr_err’
pr_err("nvram on flash (%zu bytes) is bigger than the reserved space in 
memory, will just copy the first %i bytes\n",
^~
  CC  sound/soc/sof/debug.o
drivers/firmware/broadcom/bcm47xx_nvram.c:151:30: note: format string is 
defined here
pr_err("nvram on flash (%zu bytes) is bigger than the reserved space in 
memory, will just copy the first %i bytes\n",

Signed-off-by: zhong jiang 
---
 drivers/firmware/broadcom/bcm47xx_nvram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c 
b/drivers/firmware/broadcom/bcm47xx_nvram.c
index 6d2820f..75a3240 100644
--- a/drivers/firmware/broadcom/bcm47xx_nvram.c
+++ b/drivers/firmware/broadcom/bcm47xx_nvram.c
@@ -148,7 +148,7 @@ static int nvram_init(void)
header.len > sizeof(header)) {
nvram_len = header.len;
if (nvram_len >= NVRAM_SPACE) {
-   pr_err("nvram on flash (%zu bytes) is bigger than the 
reserved space in memory, will just copy the first %i bytes\n",
+   pr_err("nvram on flash (%u bytes) is bigger than the 
reserved space in memory, will just copy the first %i bytes\n",
header.len, NVRAM_SPACE);
nvram_len = NVRAM_SPACE - 1;
}
-- 
1.7.12.4

[PATCH] smack: fix an compile error in smack_post_notification

[zhong jiang]

I hit the following error when compile the kernel.

security/smack/smack_lsm.c: In function smack_post_notification:
security/smack/smack_lsm.c:4383:7: error: dereferencing pointer to incomplete 
type struct watch_notification
  if (n->type == WATCH_TYPE_META)
   ^~
security/smack/smack_lsm.c:4383:17: error: WATCH_TYPE_META undeclared (first 
use in this function); did you mean TCA_PIE_BETA?
  if (n->type == WATCH_TYPE_META)
 ^~~
 TCA_PIE_BETA
security/smack/smack_lsm.c:4383:17: note: each undeclared identifier is 
reported only once for each function it appears in

Signed-off-by: zhong jiang 
---
 security/smack/smack.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 62529f3..02b05a2 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * Use IPv6 port labeling if IPv6 is enabled and secmarks
-- 
1.7.12.4

[PATCH] iio: Fix an undefied reference error in noa1305_probe

[zhong jiang]

I hit the following error when compile the kernel.

drivers/iio/light/noa1305.o: In function `noa1305_probe':
noa1305.c:(.text+0x65): undefined reference to `__devm_regmap_init_i2c'
make: *** [vmlinux] Error 1

Signed-off-by: zhong jiang 
---
 drivers/iio/light/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 08d7e1e..4a1a883 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -314,6 +314,7 @@ config MAX44009
 config NOA1305
tristate "ON Semiconductor NOA1305 ambient light sensor"
depends on I2C
+   select REGMAP_I2C
help
 Say Y here if you want to build support for the ON Semiconductor
 NOA1305 ambient light sensor.
-- 
1.7.12.4

Re: [PATCH net-next] ixgbe: Use memzero_explicit directly in crypto cases

[zhong jiang]

On 2019/9/18 10:36, zhong jiang wrote:
> In general, Use kzfree() to replace memset() + kfree() is feasible and
> resonable.  But It's btter to use memzero_explicit() to replace memset()
> in crypto cases.
s/btter/better/,  will repost.   sorry for that.

Thanks,
zhong jiang
> Signed-off-by: zhong jiang 
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> index 113f608..7e4f32f 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
> @@ -960,9 +960,11 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, 
> u32 *msgbuf, u32 vf)
>   return 0;
>  
>  err_aead:
> - kzfree(xs->aead);
> + memzero_explicit(xs->aead, sizeof(*xs->aead));
> + kfree(xs->aead);
>  err_xs:
> - kzfree(xs);
> + memzero_explicit(xs, sizeof(*xs));
> + kfree(xs);
>  err_out:
>   msgbuf[1] = err;
>   return err;
> @@ -1047,7 +1049,8 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter 
> *adapter, u32 *msgbuf, u32 vf)
>   ixgbe_ipsec_del_sa(xs);
>  
>   /* remove the xs that was made-up in the add request */
> - kzfree(xs);
> + memzero_explicit(xs, sizeof(*xs));
> + kfree(xs);
>  
>   return 0;
>  }

[RESENT PATCH net-next] ixgbe: Use memzero_explicit directly in crypto cases

[zhong jiang]

In general, Use kzfree() to replace memset() + kfree() is feasible and
resonable.  But It's better to use memzero_explicit() to replace memset()
in crypto cases.

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 113f608..7e4f32f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -960,9 +960,11 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
return 0;
 
 err_aead:
-   kzfree(xs->aead);
+   memzero_explicit(xs->aead, sizeof(*xs->aead));
+   kfree(xs->aead);
 err_xs:
-   kzfree(xs);
+   memzero_explicit(xs, sizeof(*xs));
+   kfree(xs);
 err_out:
msgbuf[1] = err;
return err;
@@ -1047,7 +1049,8 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
ixgbe_ipsec_del_sa(xs);
 
/* remove the xs that was made-up in the add request */
-   kzfree(xs);
+   memzero_explicit(xs, sizeof(*xs));
+   kfree(xs);
 
return 0;
 }
-- 
1.7.12.4

[PATCH net-next] ixgbe: Use memzero_explicit directly in crypto cases

[zhong jiang]

In general, Use kzfree() to replace memset() + kfree() is feasible and
resonable.  But It's btter to use memzero_explicit() to replace memset()
in crypto cases.

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 113f608..7e4f32f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -960,9 +960,11 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
return 0;
 
 err_aead:
-   kzfree(xs->aead);
+   memzero_explicit(xs->aead, sizeof(*xs->aead));
+   kfree(xs->aead);
 err_xs:
-   kzfree(xs);
+   memzero_explicit(xs, sizeof(*xs));
+   kfree(xs);
 err_out:
msgbuf[1] = err;
return err;
@@ -1047,7 +1049,8 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
ixgbe_ipsec_del_sa(xs);
 
/* remove the xs that was made-up in the add request */
-   kzfree(xs);
+   memzero_explicit(xs, sizeof(*xs));
+   kfree(xs);
 
return 0;
 }
-- 
1.7.12.4

Re: [RESENT PATCH v2] ixgbe: Use memzero_explicit directly in crypto cases

[zhong jiang]

On 2019/9/18 2:11, Jakub Kicinski wrote:
> On Tue, 17 Sep 2019 22:44:22 +0800, zhong jiang wrote:
>> It's better to use memzero_explicit() to replace memset() in crypto cases.
>>
>> Signed-off-by: zhong jiang 
> Thank you for the follow up! Your previous patch to use kzfree() 
> has been applied on its own merit, could you rebase this one on top 
> of current net-next/master?
I will do that.

Thanks,
zhong jiang
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> index 31629fc..7e4f32f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -960,10 +960,10 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter 
>> *adapter, u32 *msgbuf, u32 vf)
>>  return 0;
>>  
>>  err_aead:
>> -memset(xs->aead, 0, sizeof(*xs->aead));
>> +memzero_explicit(xs->aead, sizeof(*xs->aead));
>>  kfree(xs->aead);
>>  err_xs:
>> -memset(xs, 0, sizeof(*xs));
>> +memzero_explicit(xs, sizeof(*xs));
>>  kfree(xs);
>>  err_out:
>>  msgbuf[1] = err;
>> @@ -1049,7 +1049,7 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter 
>> *adapter, u32 *msgbuf, u32 vf)
>>  ixgbe_ipsec_del_sa(xs);
>>  
>>  /* remove the xs that was made-up in the add request */
>> -memset(xs, 0, sizeof(*xs));
>> +memzero_explicit(xs, sizeof(*xs));
>>  kfree(xs);
>>  
>>  return 0;
>
> .
>

[RESENT PATCH v2] ixgbe: Use memzero_explicit directly in crypto cases

[zhong jiang]

It's better to use memzero_explicit() to replace memset() in crypto cases.

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 31629fc..7e4f32f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -960,10 +960,10 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
return 0;
 
 err_aead:
-   memset(xs->aead, 0, sizeof(*xs->aead));
+   memzero_explicit(xs->aead, sizeof(*xs->aead));
kfree(xs->aead);
 err_xs:
-   memset(xs, 0, sizeof(*xs));
+   memzero_explicit(xs, sizeof(*xs));
kfree(xs);
 err_out:
msgbuf[1] = err;
@@ -1049,7 +1049,7 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
ixgbe_ipsec_del_sa(xs);
 
/* remove the xs that was made-up in the add request */
-   memset(xs, 0, sizeof(*xs));
+   memzero_explicit(xs, sizeof(*xs));
kfree(xs);
 
return 0;
-- 
1.7.12.4

Re: [PATCH v2] ixgbe: Use memset_explicit directly in crypto cases

[zhong jiang]

On 2019/9/17 17:59, Sergei Shtylyov wrote:
> Hello!
>
> On 17.09.2019 6:45, zhong jiang wrote:
>
>> It's better to use memset_explicit() to replace memset() in crypto cases.
>
>But you're using memzero_explicit() below?
Sorry, stupid Oops. I will repost.  Thank for your reminder.

Sincerely,
zhong jiang
>
>> Signed-off-by: zhong jiang 
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> index 31629fc..7e4f32f 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -960,10 +960,10 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter 
>> *adapter, u32 *msgbuf, u32 vf)
>>   return 0;
>> err_aead:
>> -memset(xs->aead, 0, sizeof(*xs->aead));
>> +memzero_explicit(xs->aead, sizeof(*xs->aead));
>>   kfree(xs->aead);
>>   err_xs:
>> -memset(xs, 0, sizeof(*xs));
>> +memzero_explicit(xs, sizeof(*xs));
>>   kfree(xs);
>>   err_out:
>>   msgbuf[1] = err;
>> @@ -1049,7 +1049,7 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter 
>> *adapter, u32 *msgbuf, u32 vf)
>>   ixgbe_ipsec_del_sa(xs);
>> /* remove the xs that was made-up in the add request */
>> -memset(xs, 0, sizeof(*xs));
>> +memzero_explicit(xs, sizeof(*xs));
>>   kfree(xs);
>> return 0;
>
> MBR, Sergei
>
> .
>

[PATCH v2] ixgbe: Use memset_explicit directly in crypto cases

[zhong jiang]

It's better to use memset_explicit() to replace memset() in crypto cases.

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 31629fc..7e4f32f 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -960,10 +960,10 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
return 0;
 
 err_aead:
-   memset(xs->aead, 0, sizeof(*xs->aead));
+   memzero_explicit(xs->aead, sizeof(*xs->aead));
kfree(xs->aead);
 err_xs:
-   memset(xs, 0, sizeof(*xs));
+   memzero_explicit(xs, sizeof(*xs));
kfree(xs);
 err_out:
msgbuf[1] = err;
@@ -1049,7 +1049,7 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
ixgbe_ipsec_del_sa(xs);
 
/* remove the xs that was made-up in the add request */
-   memset(xs, 0, sizeof(*xs));
+   memzero_explicit(xs, sizeof(*xs));
kfree(xs);
 
return 0;
-- 
1.7.12.4

Re: [PATCH 1/3] ixgbe: Use kzfree() rather than its implementation.

[zhong jiang]

On 2019/9/17 10:43, Jakub Kicinski wrote:
> On Wed, 4 Sep 2019 10:39:10 +0800, zhong jiang wrote:
>> Use kzfree() instead of memset() + kfree().
>>
>> Signed-off-by: zhong jiang 
>> ---
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 +++--
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> index 31629fc..113f608 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>> @@ -960,11 +960,9 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter 
>> *adapter, u32 *msgbuf, u32 vf)
>>  return 0;
>>  
>>  err_aead:
>> -memset(xs->aead, 0, sizeof(*xs->aead));
>> -kfree(xs->aead);
>> +kzfree(xs->aead);
>>  err_xs:
>> -memset(xs, 0, sizeof(*xs));
>> -kfree(xs);
>> +kzfree(xs);
>>  err_out:
>>  msgbuf[1] = err;
>>  return err;
>> @@ -1049,8 +1047,7 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter 
>> *adapter, u32 *msgbuf, u32 vf)
>>  ixgbe_ipsec_del_sa(xs);
>>  
>>  /* remove the xs that was made-up in the add request */
>> -memset(xs, 0, sizeof(*xs));
>> -kfree(xs);
>> +kzfree(xs);
>>  
>>  return 0;
>>  }
> All the crypto cases should really be converted to memzero_explicit().
It's better to do that.  I will repost it in v2.

Thanks,
zhong jiang

Re: [PATCH 2/3] nfp: Drop unnecessary continue in nfp_net_pf_alloc_vnics

[zhong jiang]

On 2019/9/17 10:45, Jakub Kicinski wrote:
> On Wed, 4 Sep 2019 11:46:23 +0800, zhong jiang wrote:
>> Continue is not needed at the bottom of a loop.
>>
>> Signed-off-by: zhong jiang 
>> ---
>>  drivers/net/ethernet/netronome/nfp/nfp_net_main.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c 
>> b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> index 986464d..68db47d 100644
>> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> @@ -205,10 +205,8 @@ static void nfp_net_pf_free_vnics(struct nfp_pf *pf)
>>  ctrl_bar += NFP_PF_CSR_SLICE_SIZE;
>>  
>>  /* Kill the vNIC if app init marked it as invalid */
>> -if (nn->port && nn->port->type == NFP_PORT_INVALID) {
>> +if (nn->port && nn->port->type == NFP_PORT_INVALID)
>>  nfp_net_pf_free_vnic(pf, nn);
>> -continue;
>> -}
> Ugh, I already nack at least one patch like this, this continue makes
> the _intent_ of the code more clear, the compiler will ignore it anyway.
Thanks,   I miss that information you object to above modification.  

Sincerely,
zhong jiang
> I guess there's no use in fighting the bots..
>
>>  }
>>  
>>  if (list_empty(>vnics))
>
> .
>

Re: [PATCH 2/3] wlegacy: Remove unneeded variable and make function to be void

[zhong jiang]

On 2019/9/13 1:45, Kalle Valo wrote:
> zhong jiang  writes:
>
>> il4965_set_tkip_dynamic_key_info  do not need return value to
>> cope with different ases. And change functon return type to void.
>>
>> Signed-off-by: zhong jiang 
>> ---
>>  drivers/net/wireless/intel/iwlegacy/4965-mac.c | 8 ++--
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c 
>> b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
>> index ffb705b..a7bbfe2 100644
>> --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
>> +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
>> @@ -3326,12 +3326,11 @@ struct il_mod_params il4965_mod_params = {
>>  return il_send_add_sta(il, _cmd, CMD_SYNC);
>>  }
>>  
>> -static int
>> +static void
>>  il4965_set_tkip_dynamic_key_info(struct il_priv *il,
>>   struct ieee80211_key_conf *keyconf, u8 sta_id)
>>  {
>>  unsigned long flags;
>> -int ret = 0;
>>  __le16 key_flags = 0;
>>  
>>  key_flags |= (STA_KEY_FLG_TKIP | STA_KEY_FLG_MAP_KEY_MSK);
>> @@ -3367,8 +3366,6 @@ struct il_mod_params il4965_mod_params = {
>>  memcpy(il->stations[sta_id].sta.key.key, keyconf->key, 16);
>>  
>>  spin_unlock_irqrestore(>sta_lock, flags);
>> -
>> -return ret;
>>  }
>>  
>>  void
>> @@ -3483,8 +3480,7 @@ struct il_mod_params il4965_mod_params = {
>>  il4965_set_ccmp_dynamic_key_info(il, keyconf, sta_id);
>>  break;
>>  case WLAN_CIPHER_SUITE_TKIP:
>> -ret =
>> -il4965_set_tkip_dynamic_key_info(il, keyconf, sta_id);
>> +il4965_set_tkip_dynamic_key_info(il, keyconf, sta_id);
>>  break;
>>  case WLAN_CIPHER_SUITE_WEP40:
>>  case WLAN_CIPHER_SUITE_WEP104:
> To me this looks inconsistent with the rest of the cases in the switch
> statement. And won't we then return the ret variable uninitalised?
Yep,  I miss that.   please ignore the patch.  Thanks,

Sincerely,
zhong jiang

[PATCH] misc: rtsx: Remove unneeded variable in rts5260_card_power_on

[zhong jiang]

rts5260_card_power_on do not need local variable to store different value,
Hence just remove it.

Signed-off-by: zhong jiang 
---
 drivers/misc/cardreader/rts5260.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/misc/cardreader/rts5260.c 
b/drivers/misc/cardreader/rts5260.c
index 40a6d19..4214f02 100644
--- a/drivers/misc/cardreader/rts5260.c
+++ b/drivers/misc/cardreader/rts5260.c
@@ -191,7 +191,6 @@ static int sd_set_sample_push_timing_sd30(struct rtsx_pcr 
*pcr)
 
 static int rts5260_card_power_on(struct rtsx_pcr *pcr, int card)
 {
-   int err = 0;
struct rtsx_cr_option *option = >option;
 
if (option->ocp_en)
@@ -231,7 +230,7 @@ static int rts5260_card_power_on(struct rtsx_pcr *pcr, int 
card)
 
rtsx_pci_write_register(pcr, REG_PRE_RW_MODE, EN_INFINITE_MODE, 0);
 
-   return err;
+   return 0;
 }
 
 static int rts5260_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
-- 
1.7.12.4

[PATCH] drivers/misc: Remove unneeded variable in st_tty_open

[zhong jiang]

st_tty_open do not need local variable to store different value,
Hence just remove it.

Signed-off-by: zhong jiang 
---
 drivers/misc/ti-st/st_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
index 7d9e23a..2ae9948 100644
--- a/drivers/misc/ti-st/st_core.c
+++ b/drivers/misc/ti-st/st_core.c
@@ -708,7 +708,6 @@ long st_write(struct sk_buff *skb)
  */
 static int st_tty_open(struct tty_struct *tty)
 {
-   int err = 0;
struct st_data_s *st_gdata;
pr_info("%s ", __func__);
 
@@ -731,7 +730,8 @@ static int st_tty_open(struct tty_struct *tty)
 */
st_kim_complete(st_gdata->kim_data);
pr_debug("done %s", __func__);
-   return err;
+
+   return 0;
 }
 
 static void st_tty_close(struct tty_struct *tty)
-- 
1.7.12.4

[PATCH] net/mlx5: Remove unneeded variable in mlx5_unload_one

[zhong jiang]

mlx5_unload_one do not need local variable to store different value,
Hence just remove it.

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/mellanox/mlx5/core/main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c 
b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 9648c22..c39bb37 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1228,8 +1228,6 @@ static int mlx5_load_one(struct mlx5_core_dev *dev, bool 
boot)
 
 static int mlx5_unload_one(struct mlx5_core_dev *dev, bool cleanup)
 {
-   int err = 0;
-
if (cleanup) {
mlx5_unregister_device(dev);
mlx5_drain_health_wq(dev);
@@ -1257,7 +1255,7 @@ static int mlx5_unload_one(struct mlx5_core_dev *dev, 
bool cleanup)
mlx5_function_teardown(dev, cleanup);
 out:
mutex_unlock(>intf_state_mutex);
-   return err;
+   return 0;
 }
 
 static int mlx5_mdev_init(struct mlx5_core_dev *dev, int profile_idx)
-- 
1.7.12.4

[PATCH 0/3] wireless: Remove unneeded variable

[zhong jiang]

With the help of Coccinelle, I find some place to use redundant
variable to store the return value, It is unnecessary. Just remove
it and make the funtion to be void.

zhong jiang (3):
  brcmsmac: Remove unneeded variable and make function to be void
  wlegacy: Remove unneeded variable and make function to be void
  libertas: Remove unneeded variable and make function to be void

 drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c | 5 +
 drivers/net/wireless/intel/iwlegacy/4965-mac.c  | 8 ++--
 drivers/net/wireless/marvell/libertas/cmd.h | 2 +-
 drivers/net/wireless/marvell/libertas/cmdresp.c | 5 +
 4 files changed, 5 insertions(+), 15 deletions(-)

-- 
1.7.12.4

[PATCH 1/3] brcmsmac: Remove unneeded variable and make function to be void

[zhong jiang]

brcms_c_set_mac  do not need return value to cope with different
cases. And change functon return type to void.

Signed-off-by: zhong jiang 
---
 drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
index 080e829..ddc4d47 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/main.c
@@ -3776,17 +3776,14 @@ static void brcms_c_set_ps_ctrl(struct brcms_c_info 
*wlc)
  * Write this BSS config's MAC address to core.
  * Updates RXE match engine.
  */
-static int brcms_c_set_mac(struct brcms_bss_cfg *bsscfg)
+static void brcms_c_set_mac(struct brcms_bss_cfg *bsscfg)
 {
-   int err = 0;
struct brcms_c_info *wlc = bsscfg->wlc;
 
/* enter the MAC addr into the RXE match registers */
brcms_c_set_addrmatch(wlc, RCM_MAC_OFFSET, wlc->pub->cur_etheraddr);
 
brcms_c_ampdu_macaddr_upd(wlc);
-
-   return err;
 }
 
 /* Write the BSS config's BSSID address to core (set_bssid in d11procs.tcl).
-- 
1.7.12.4

[PATCH 2/3] wlegacy: Remove unneeded variable and make function to be void

[zhong jiang]

il4965_set_tkip_dynamic_key_info  do not need return value to
cope with different ases. And change functon return type to void.

Signed-off-by: zhong jiang 
---
 drivers/net/wireless/intel/iwlegacy/4965-mac.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c 
b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index ffb705b..a7bbfe2 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -3326,12 +3326,11 @@ struct il_mod_params il4965_mod_params = {
return il_send_add_sta(il, _cmd, CMD_SYNC);
 }
 
-static int
+static void
 il4965_set_tkip_dynamic_key_info(struct il_priv *il,
 struct ieee80211_key_conf *keyconf, u8 sta_id)
 {
unsigned long flags;
-   int ret = 0;
__le16 key_flags = 0;
 
key_flags |= (STA_KEY_FLG_TKIP | STA_KEY_FLG_MAP_KEY_MSK);
@@ -3367,8 +3366,6 @@ struct il_mod_params il4965_mod_params = {
memcpy(il->stations[sta_id].sta.key.key, keyconf->key, 16);
 
spin_unlock_irqrestore(>sta_lock, flags);
-
-   return ret;
 }
 
 void
@@ -3483,8 +3480,7 @@ struct il_mod_params il4965_mod_params = {
il4965_set_ccmp_dynamic_key_info(il, keyconf, sta_id);
break;
case WLAN_CIPHER_SUITE_TKIP:
-   ret =
-   il4965_set_tkip_dynamic_key_info(il, keyconf, sta_id);
+   il4965_set_tkip_dynamic_key_info(il, keyconf, sta_id);
break;
case WLAN_CIPHER_SUITE_WEP40:
case WLAN_CIPHER_SUITE_WEP104:
-- 
1.7.12.4

[PATCH 3/3] libertas: Remove unneeded variable and make function to be void

[zhong jiang]

lbs_process_event  do not need return value to cope with different
cases. And change functon return type to void.

Signed-off-by: zhong jiang 
---
 drivers/net/wireless/marvell/libertas/cmd.h | 2 +-
 drivers/net/wireless/marvell/libertas/cmdresp.c | 5 +
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/marvell/libertas/cmd.h 
b/drivers/net/wireless/marvell/libertas/cmd.h
index 8087856..3c19307 100644
--- a/drivers/net/wireless/marvell/libertas/cmd.h
+++ b/drivers/net/wireless/marvell/libertas/cmd.h
@@ -76,7 +76,7 @@ void lbs_mac_event_disconnected(struct lbs_private *priv,
 
 /* Events */
 
-int lbs_process_event(struct lbs_private *priv, u32 event);
+void lbs_process_event(struct lbs_private *priv, u32 event);
 
 
 /* Actual commands */
diff --git a/drivers/net/wireless/marvell/libertas/cmdresp.c 
b/drivers/net/wireless/marvell/libertas/cmdresp.c
index b73d083..cb515c5 100644
--- a/drivers/net/wireless/marvell/libertas/cmdresp.c
+++ b/drivers/net/wireless/marvell/libertas/cmdresp.c
@@ -220,9 +220,8 @@ int lbs_process_command_response(struct lbs_private *priv, 
u8 *data, u32 len)
return ret;
 }
 
-int lbs_process_event(struct lbs_private *priv, u32 event)
+void lbs_process_event(struct lbs_private *priv, u32 event)
 {
-   int ret = 0;
struct cmd_header cmd;
 
switch (event) {
@@ -351,6 +350,4 @@ int lbs_process_event(struct lbs_private *priv, u32 event)
netdev_alert(priv->dev, "EVENT: unknown event id %d\n", event);
break;
}
-
-   return ret;
 }
-- 
1.7.12.4

Re: [PATCH] ethernet: micrel: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

On 2019/9/7 3:40, Andrew Lunn wrote:
> On Thu, Sep 05, 2019 at 11:53:48PM +0800, zhong jiang wrote:
>> The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
>> but is perhaps more readable.
> Hi Zhong
>
> Did you find this by hand, or did you use a tool. If a tool is used,
> it is normal to give some credit to the tool.
With the following help of Coccinelle. 
-(((x) + ((__divisor) / 2)) / (__divisor))
+ DIV_ROUND_CLOSEST(x,__divisor)

Sometimes, I will add the information in the description. Sometimes, I desn't 
do that.

I will certainly add the description when I send an series of patches to modify 
the case.

Thanks,
zhong jiang

> Thanks
>   Andrew
>
> .
>

[PATCH] brcmsmac: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 .../net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c   | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
index 07f61d6..3bf152d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_n.c
@@ -17748,7 +17748,7 @@ static void wlc_phy_txpwrctrl_pwr_setup_nphy(struct 
brcms_phy *pi)
num = 8 *
  (16 * b0[tbl_id - 26] + b1[tbl_id - 26] * idx);
den = 32768 + a1[tbl_id - 26] * idx;
-   pwr_est = max(((4 * num + den / 2) / den), -8);
+   pwr_est = max(DIV_ROUND_CLOSEST(4 * num, den), -8);
if (NREV_LT(pi->pubpi.phy_rev, 3)) {
if (idx <=
(uint) (31 - idle_tssi[tbl_id - 26] + 1))
@@ -26990,8 +26990,8 @@ static void wlc_phy_rxcal_phycleanup_nphy(struct 
brcms_phy *pi, u8 rx_core)
 NPHY_RXCAL_TONEAMP, 0, cal_type, false);
 
wlc_phy_rx_iq_est_nphy(pi, est, num_samps, 32, 0);
-   i_pwr = (est[rx_core].i_pwr + num_samps / 2) / num_samps;
-   q_pwr = (est[rx_core].q_pwr + num_samps / 2) / num_samps;
+   i_pwr = DIV_ROUND_CLOSEST(est[rx_core].i_pwr, num_samps);
+   q_pwr = DIV_ROUND_CLOSEST(est[rx_core].q_pwr, num_samps);
curr_pwr = i_pwr + q_pwr;
 
switch (gainctrl_dirn) {
@@ -27673,10 +27673,10 @@ static int wlc_phy_cal_rxiq_nphy_rev3(struct 
brcms_phy *pi,
wlc_phy_rx_iq_est_nphy(pi, est,
   num_samps, 32,
   0);
-   i_pwr = (est[rx_core].i_pwr +
-num_samps / 2) / num_samps;
-   q_pwr = (est[rx_core].q_pwr +
-num_samps / 2) / num_samps;
+   i_pwr = 
DIV_ROUND_CLOSEST(est[rx_core].i_pwr,
+
num_samps);
+   q_pwr = 
DIV_ROUND_CLOSEST(est[rx_core].q_pwr,
+
num_samps);
tot_pwr[gain_pass] = i_pwr + q_pwr;
} else {
 
-- 
1.7.12.4

[PATCH 1/4] media: dvb-frontends: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/media/dvb-frontends/mt312.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb-frontends/mt312.c 
b/drivers/media/dvb-frontends/mt312.c
index 7cae7d6..251ff41 100644
--- a/drivers/media/dvb-frontends/mt312.c
+++ b/drivers/media/dvb-frontends/mt312.c
@@ -137,7 +137,7 @@ static inline int mt312_writereg(struct mt312_state *state,
 
 static inline u32 mt312_div(u32 a, u32 b)
 {
-   return (a + (b / 2)) / b;
+   return DIV_ROUND_CLOSEST(a, b);
 }
 
 static int mt312_reset(struct mt312_state *state, const u8 full)
-- 
1.7.12.4

[PATCH 2/4] media: tuners/qm1d1c0042: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/media/tuners/qm1d1c0042.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/tuners/qm1d1c0042.c 
b/drivers/media/tuners/qm1d1c0042.c
index 83ca5dc..0e26d22 100644
--- a/drivers/media/tuners/qm1d1c0042.c
+++ b/drivers/media/tuners/qm1d1c0042.c
@@ -206,7 +206,7 @@ static int qm1d1c0042_set_params(struct dvb_frontend *fe)
if (ret < 0)
return ret;
 
-   a = (freq + state->cfg.xtal_freq / 2) / state->cfg.xtal_freq;
+   a = DIV_ROUND_CLOSEST(freq, state->cfg.xtal_freq);
 
state->regs[0x06] &= 0x40;
state->regs[0x06] |= (a - 12) / 4;
-- 
1.7.12.4

[PATCH 0/4] media: Use DIV_ROUND_CLOSEST directly

[zhong jiang]

With the following help of Coccinelle. I find out some place can be replaced.

-(((x) + ((__divisor) / 2)) / (__divisor))
+ DIV_ROUND_CLOSEST(x,__divisor)

zhong jiang (4):
  media: dvb-frontends: Use DIV_ROUND_CLOSEST directly to make it
readable
  media: tuners/qm1d1c0042: Use DIV_ROUND_CLOSEST directly to make it
readable
  media: uvcvideo: Use DIV_ROUND_CLOSEST directly to make it readable
  media: v4l2-dv-timings: Use DIV_ROUND_CLOSEST directly to make it
readable

 drivers/media/dvb-frontends/mt312.c   | 2 +-
 drivers/media/tuners/qm1d1c0042.c | 2 +-
 drivers/media/usb/uvc/uvc_ctrl.c  | 4 ++--
 drivers/media/v4l2-core/v4l2-dv-timings.c | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

-- 
1.7.12.4

[PATCH 3/4] media: uvcvideo: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/media/usb/uvc/uvc_ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index e399b9f..9f3697161 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1604,8 +1604,8 @@ int uvc_ctrl_set(struct uvc_fh *handle,
if (step == 0)
step = 1;
 
-   xctrl->value = min + ((u32)(xctrl->value - min) + step / 2)
-/ step * step;
+   xctrl->value = min + DIV_ROUND_CLOSEST((u32)(xctrl->value - 
min),
+   step) * step;
if (mapping->data_type == UVC_CTRL_DATA_TYPE_SIGNED)
xctrl->value = clamp(xctrl->value, min, max);
else
-- 
1.7.12.4

[PATCH 4/4] media: v4l2-dv-timings: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/media/v4l2-core/v4l2-dv-timings.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-dv-timings.c 
b/drivers/media/v4l2-core/v4l2-dv-timings.c
index 4f23e93..2b399c0 100644
--- a/drivers/media/v4l2-core/v4l2-dv-timings.c
+++ b/drivers/media/v4l2-core/v4l2-dv-timings.c
@@ -757,7 +757,7 @@ bool v4l2_detect_gtf(unsigned frame_height,
pix_clk = pix_clk / GTF_PXL_CLK_GRAN * GTF_PXL_CLK_GRAN;
 
hsync = (frame_width * 8 + 50) / 100;
-   hsync = ((hsync + GTF_CELL_GRAN / 2) / GTF_CELL_GRAN) * GTF_CELL_GRAN;
+   hsync = DIV_ROUND_CLOSEST(hsync, GTF_CELL_GRAN) * GTF_CELL_GRAN;
 
h_fp = h_blank / 2 - hsync;
 
-- 
1.7.12.4

[PATCH] ethernet: micrel: Use DIV_ROUND_CLOSEST directly to make it readable

[zhong jiang]

The kernel.h macro DIV_ROUND_CLOSEST performs the computation (x + d/2)/d
but is perhaps more readable.

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/micrel/ksz884x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/micrel/ksz884x.c 
b/drivers/net/ethernet/micrel/ksz884x.c
index 3103446..e102e15 100644
--- a/drivers/net/ethernet/micrel/ksz884x.c
+++ b/drivers/net/ethernet/micrel/ksz884x.c
@@ -2166,7 +2166,7 @@ static void sw_get_broad_storm(struct ksz_hw *hw, u8 
*percent)
num = (data & BROADCAST_STORM_RATE_HI);
num <<= 8;
num |= (data & BROADCAST_STORM_RATE_LO) >> 8;
-   num = (num * 100 + BROADCAST_STORM_VALUE / 2) / BROADCAST_STORM_VALUE;
+   num = DIV_ROUND_CLOSEST(num * 100, BROADCAST_STORM_VALUE);
*percent = (u8) num;
 }
 
-- 
1.7.12.4

Re: [PATCH] hostap: remove set but not used variable 'copied' in prism2_io_debug_proc_read

[zhong jiang]

On 2019/9/5 21:45, Kalle Valo wrote:
> zhong jiang  writes:
>
>> Please ignore the patch.  Because  the hostap_proc.c is marked as 'obsolete'.
> You mean marked in the MAINTAINERS file? I don't see that as a problem,
> I can (and should) still apply any patches submitted to hostap driver.
>
I  hit the following issue when checking the patch by checkpatch.pl

WARNING: drivers/net/wireless/intersil/hostap/hostap_proc.c is marked as 
'obsolete' in the MAINTAINERS hierarchy.
No unnecessary modifications please.

I certainly hope it can be appiled to upstream if the above check doesn't 
matter.

Thanks,
zhong jiang

[PATCH 0/3] replace code with FIELD_SIZEOF

[zhong jiang]

FIELD_SIZEOF() has implemented sizeof().  Hence use FIELD_SIZEOF
directly.

zhong jiang (3):
  batman-adv: Use FIELD_SIZEOF directly
  media: v4l2: Use FIELD_SIZEOF directly
  IB/mlx5: Use FIELD_SIZEOF directly

 drivers/infiniband/hw/mlx5/mlx5_ib.h   | 2 +-
 drivers/media/v4l2-core/v4l2-ioctl.c   | 2 +-
 net/batman-adv/distributed-arp-table.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

-- 
1.7.12.4

[PATCH 3/3] IB/mlx5: Use FIELD_SIZEOF directly

[zhong jiang]

It's more clear to use FIELD_SIZEOF instead of its implementation.

Signed-off-by: zhong jiang 
---
 drivers/infiniband/hw/mlx5/mlx5_ib.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h 
b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index c3ea299..e5681f7 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -65,7 +65,7 @@
 __LINE__, current->pid, ##arg)
 
 #define field_avail(type, fld, sz) (offsetof(type, fld) +  \
-   sizeof(((type *)0)->fld) <= (sz))
+   FIELD_SIZEOF(type, fld) <= (sz))
 #define MLX5_IB_DEFAULT_UIDX 0xff
 #define MLX5_USER_ASSIGNED_UIDX_MASK __mlx5_mask(qpc, user_index)
 
-- 
1.7.12.4

[PATCH 1/3] batman-adv: Use FIELD_SIZEOF directly

[zhong jiang]

It's more clear to use FIELD_SIZEOF instead of its implementation.

Signed-off-by: zhong jiang 
---
 net/batman-adv/distributed-arp-table.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/batman-adv/distributed-arp-table.c 
b/net/batman-adv/distributed-arp-table.c
index b0af3a1..c79fdf8 100644
--- a/net/batman-adv/distributed-arp-table.c
+++ b/net/batman-adv/distributed-arp-table.c
@@ -91,8 +91,8 @@ struct batadv_dhcp_packet {
__u8 options[0];
 };
 
-#define BATADV_DHCP_YIADDR_LEN sizeof(((struct batadv_dhcp_packet *)0)->yiaddr)
-#define BATADV_DHCP_CHADDR_LEN sizeof(((struct batadv_dhcp_packet *)0)->chaddr)
+#define BATADV_DHCP_YIADDR_LEN FIELD_SIZEOF(struct batadv_dhcp_packet, yiaddr)
+#define BATADV_DHCP_CHADDR_LEN FIELD_SIZEOF(struct batadv_dhcp_packet, chaddr)
 
 static void batadv_dat_purge(struct work_struct *work);
 
-- 
1.7.12.4

[PATCH 2/3] media: v4l2: Use FIELD_SIZEOF directly

[zhong jiang]

It's more clear  to use FIELD_SIZEOF instead of its implementation.

Signed-off-by: zhong jiang 
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c 
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 51b9127..eebea91 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2637,7 +2637,7 @@ struct v4l2_ioctl_info {
 /* Zero struct from after the field to the end */
 #define INFO_FL_CLEAR(v4l2_struct, field)  \
((offsetof(struct v4l2_struct, field) + \
- sizeof(((struct v4l2_struct *)0)->field)) << 16)
+ FIELD_SIZEOF(struct v4l2_struct, field)) << 16)
 #define INFO_FL_CLEAR_MASK (_IOC_SIZEMASK << 16)
 
 #define DEFINE_V4L_STUB_FUNC(_vidioc)  \
-- 
1.7.12.4

Re: [PATCH 4/4] rtc: ds1347: Use PTR_ERR_OR_ZERO rather than its implementation

[zhong jiang]

On 2019/9/5 15:39, Alexandre Belloni wrote:
> On 05/09/2019 14:43:15+0800, zhong jiang wrote:
>> PTR_ERR_OR_ZERO contains if(IS_ERR(...)) + PTR_ERR. It is better to
>> use it directly. hence just replace it.
>>
> Unless you have a more significant contribution to this driver, I'm not
> going to apply this patch, especially since it will have to be reverted
> as soon as the probe function changes.
Anyway,  Thanks,

Sincerely,
zhong jiang
>> Signed-off-by: zhong jiang 
>> ---
>>  drivers/rtc/rtc-ds1347.c | 5 +
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-ds1347.c b/drivers/rtc/rtc-ds1347.c
>> index d392a7b..5a64eea 100644
>> --- a/drivers/rtc/rtc-ds1347.c
>> +++ b/drivers/rtc/rtc-ds1347.c
>> @@ -151,10 +151,7 @@ static int ds1347_probe(struct spi_device *spi)
>>  rtc = devm_rtc_device_register(>dev, "ds1347",
>>  _rtc_ops, THIS_MODULE);
>>  
>> -if (IS_ERR(rtc))
>> -return PTR_ERR(rtc);
>> -
>> -return 0;
>> +return PTR_ERR_OR_ZERO(rtc);
>>  }
>>  
>>  static struct spi_driver ds1347_driver = {
>> -- 
>> 1.7.12.4
>>

[PATCH 3/4] phy: tegra: Use PTR_ERR_OR_ZERO rather than its implementation

[zhong jiang]

PTR_ERR_OR_ZERO contains if(IS_ERR(...)) + PTR_ERR. It is better to
use it directly. hence just replace it.

Signed-off-by: zhong jiang 
---
 drivers/phy/tegra/phy-tegra194-p2u.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/phy/tegra/phy-tegra194-p2u.c 
b/drivers/phy/tegra/phy-tegra194-p2u.c
index 7042bed..40bc550 100644
--- a/drivers/phy/tegra/phy-tegra194-p2u.c
+++ b/drivers/phy/tegra/phy-tegra194-p2u.c
@@ -92,10 +92,8 @@ static int tegra_p2u_probe(struct platform_device *pdev)
phy_set_drvdata(generic_phy, phy);
 
phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
-   if (IS_ERR(phy_provider))
-   return PTR_ERR(phy_provider);
 
-   return 0;
+   return PTR_ERR_OR_ZERO(phy_provider);
 }
 
 static const struct of_device_id tegra_p2u_id_table[] = {
-- 
1.7.12.4

[PATCH 4/4] rtc: ds1347: Use PTR_ERR_OR_ZERO rather than its implementation

[zhong jiang]

PTR_ERR_OR_ZERO contains if(IS_ERR(...)) + PTR_ERR. It is better to
use it directly. hence just replace it.

Signed-off-by: zhong jiang 
---
 drivers/rtc/rtc-ds1347.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-ds1347.c b/drivers/rtc/rtc-ds1347.c
index d392a7b..5a64eea 100644
--- a/drivers/rtc/rtc-ds1347.c
+++ b/drivers/rtc/rtc-ds1347.c
@@ -151,10 +151,7 @@ static int ds1347_probe(struct spi_device *spi)
rtc = devm_rtc_device_register(>dev, "ds1347",
_rtc_ops, THIS_MODULE);
 
-   if (IS_ERR(rtc))
-   return PTR_ERR(rtc);
-
-   return 0;
+   return PTR_ERR_OR_ZERO(rtc);
 }
 
 static struct spi_driver ds1347_driver = {
-- 
1.7.12.4

[PATCH 0/4] Use PTR_ERR_OR_ZERO directly

[zhong jiang]

With the help of ptr_ret.cocci, I find some place to use
PTR_ERR_OR_ZERO directly.

zhong jiang (4):
  bus: ti-sysc: Use PTR_ERR_OR_ZERO rather than its implementation
  misc: mic: Use PTR_ERR_OR_ZERO rather than its implementation
  phy: tegra: Use PTR_ERR_OR_ZERO rather than its implementation
  rtc: ds1347: Use PTR_ERR_OR_ZERO rather than its implementation

 drivers/bus/ti-sysc.c| 4 +---
 drivers/misc/mic/scif/scif_epd.h | 5 ++---
 drivers/phy/tegra/phy-tegra194-p2u.c | 4 +---
 drivers/rtc/rtc-ds1347.c | 5 +
 4 files changed, 5 insertions(+), 13 deletions(-)

-- 
1.7.12.4

[PATCH 2/4] misc: mic: Use PTR_ERR_OR_ZERO rather than its implementation

[zhong jiang]

PTR_ERR_OR_ZERO contains if(IS_ERR(...)) + PTR_ERR. It is better to
use it directly. hence just replace it.

Signed-off-by: zhong jiang 
---
 drivers/misc/mic/scif/scif_epd.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/mic/scif/scif_epd.h b/drivers/misc/mic/scif/scif_epd.h
index d3837f8..0b9dfe1 100644
--- a/drivers/misc/mic/scif/scif_epd.h
+++ b/drivers/misc/mic/scif/scif_epd.h
@@ -156,9 +156,8 @@ static inline int scif_verify_epd(struct scif_endpt *ep)
 static inline int scif_anon_inode_getfile(scif_epd_t epd)
 {
epd->anon = anon_inode_getfile("scif", _anon_fops, NULL, 0);
-   if (IS_ERR(epd->anon))
-   return PTR_ERR(epd->anon);
-   return 0;
+
+   return PTR_ERR_OR_ZERO(epd->anon);
 }
 
 static inline void scif_anon_inode_fput(scif_epd_t epd)
-- 
1.7.12.4

[PATCH 1/4] bus: ti-sysc: Use PTR_ERR_OR_ZERO rather than its implementation

[zhong jiang]

PTR_ERR_OR_ZERO contains if(IS_ERR(...)) + PTR_ERR. It is better to
use it directly. hence just replace it.

Signed-off-by: zhong jiang 
---
 drivers/bus/ti-sysc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
index 9207ac2..6f4c102 100644
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -504,10 +504,8 @@ static int sysc_init_resets(struct sysc *ddata)
 {
ddata->rsts =
devm_reset_control_get_optional_shared(ddata->dev, "rstctrl");
-   if (IS_ERR(ddata->rsts))
-   return PTR_ERR(ddata->rsts);
 
-   return 0;
+   return PTR_ERR_OR_ZERO(ddata->rsts);
 }
 
 /**
-- 
1.7.12.4

Re: [PATCH v2] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/9/5 11:12, Matthew Wilcox wrote:
> On Thu, Sep 05, 2019 at 10:17:51AM +0800, zhong jiang wrote:
>> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages'
>> compare with zero. And __gup_longterm_locked pass an long local variant
>> 'rc' to check_and_migrate_cma_pages. Hence it is nicer to change the
>> parameter to long to fix the issue.
> I think this patch is right, but I have concerns about this cocci grep.
>
> The code says:
>
> if ((nr_pages > 0) && migrate_allow) {
>
> There's nothing wrong with this (... other than the fact that nr_pages might
> happen to be a negative errno).  nr_pages might be 0, and this would be
> exactly the right test for that situation.  I suppose some might argue
> that this should be != 0 instead of > 0, but it depends on the situation
> which one would read better.
>
> So please don't blindly make these changes; you're right this time.
Thanks for your affirmation.  but Andrew come up with anther fix,  using an 
local long variant
to store the nr_pages.  which one do you prefer ?

Thanks,
zhong jiang

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/9/5 2:48, Andrew Morton wrote:
> On Wed, 4 Sep 2019 13:24:58 +0200 Vlastimil Babka  wrote:
>
>> On 9/4/19 12:26 PM, zhong jiang wrote:
>>> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"'
>>> compare with zero. And __get_user_pages_locked will return an long value.
>>> Hence, Convert the long to compare with zero is feasible.
>> It would be nicer if the parameter nr_pages was long again instead of 
>> unsigned
>> long (note there are two variants of the function, so both should be 
>> changed).
> nr_pages should be unsigned - it's a count of pages!
>
> The bug is that __get_user_pages_locked() returns a signed long which
> can be a -ve errno.
>
> I think it's best if __get_user_pages_locked() is to get itself a new
> local with the same type as its return value.  Something like:
>
> --- a/mm/gup.c~a
> +++ a/mm/gup.c
> @@ -1450,6 +1450,7 @@ static long check_and_migrate_cma_pages(
>   bool drain_allow = true;
>   bool migrate_allow = true;
>   LIST_HEAD(cma_page_list);
> + long ret;
>  
>  check_again:
>   for (i = 0; i < nr_pages;) {
> @@ -1511,17 +1512,18 @@ check_again:
>* again migrating any new CMA pages which we failed to isolate
>* earlier.
>*/
> - nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages,
> + ret = __get_user_pages_locked(tsk, mm, start, nr_pages,
>  pages, vmas, NULL,
>  gup_flags);
>  
> - if ((nr_pages > 0) && migrate_allow) {
> + nr_pages = ret;
> + if (ret > 0 && migrate_allow) {
>   drain_allow = true;
>   goto check_again;
>   }
>   }
>  
> - return nr_pages;
> + return ret;
>  }
>  #else
>  static long check_and_migrate_cma_pages(struct task_struct *tsk,
Firstly,  I consider the some modified method as you has writen down above.  It 
seems to work well.
According to Vlastimil's feedback,   I repost the patch in v2,   changing the 
parameter to long to fix
the issue.  which one do you prefer?

Thanks,
zhong jiang

[PATCH v2] drm/amdgpu: Remove two redundant null pointer checks

[zhong jiang]

The functions "debugfs_remove" and "kfree" tolerate the passing
of null pointers. Hence it is unnecessary to check such arguments
around the calls. Thus remove the extra condition check at two places.

Signed-off-by: zhong jiang 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 5652cc7..cb94627 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1077,8 +1077,7 @@ static int amdgpu_debugfs_ib_preempt(void *data, u64 val)
 
ttm_bo_unlock_delayed_workqueue(>mman.bdev, resched);
 
-   if (fences)
-   kfree(fences);
+   kfree(fences);
 
return 0;
 }
@@ -1103,8 +1102,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 
 void amdgpu_debugfs_preempt_cleanup(struct amdgpu_device *adev)
 {
-   if (adev->debugfs_preempt)
-   debugfs_remove(adev->debugfs_preempt);
+   debugfs_remove(adev->debugfs_preempt);
 }
 
 #else
-- 
1.7.12.4

Re: drm/amdgpu: remove the redundant null check

[zhong jiang]

On 2019/9/5 1:50, Markus Elfring wrote:
>> debugfs_remove and kfree has taken the null check in account.
>> hence it is unnecessary to check it. Just remove the condition.
> How do you think about a wording like the following?
>
>   The functions “debugfs_remove” and “kfree” tolerate the passing
>   of null pointers. Hence it is unnecessary to check such arguments
>   around the calls. Thus remove the extra condition check at two places.
>
It's better, Thanks
>> No functional change.
> I find this information questionable while it is partly reasonable
> according to the shown software refactoring.
>
> Can a subject like “[PATCH] drm/amdgpu: Remove two redundant
> null pointer checks” be nicer here?
>
It's more clearer, thanks,  Will repost using above description in v2.
> Were any source code analysis tools involved for finding
> these update candidates?
With the help of Coccinelle. You can find out some example in 
scripts/coccinelle/.

Sincerely,
zhong jiang
> Regards,
> Markus
>
> .
>

[PATCH] cfg80211: Do not compare with boolean in nl80211_common_reg_change_event

[zhong jiang]

With the help of boolinit.cocci, we use !nl80211_reg_change_event_fill
instead of (nl80211_reg_change_event_fill == false). Meanwhile, Clean
up the code.

Signed-off-by: zhong jiang 
---
 net/wireless/nl80211.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 3e30e18..0c7fa60 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -14997,12 +14997,10 @@ void nl80211_common_reg_change_event(enum 
nl80211_commands cmd_id,
return;
 
hdr = nl80211hdr_put(msg, 0, 0, 0, cmd_id);
-   if (!hdr) {
-   nlmsg_free(msg);
-   return;
-   }
+   if (!hdr)
+   goto nla_put_failure;
 
-   if (nl80211_reg_change_event_fill(msg, request) == false)
+   if (!nl80211_reg_change_event_fill(msg, request))
goto nla_put_failure;
 
genlmsg_end(msg, hdr);
-- 
1.7.12.4

Re: NFS: remove the redundant check when kfree an object in nfs_netns_client_release

[zhong jiang]

On 2019/9/4 23:11, Markus Elfring wrote:
>> kfree has taken the null check in account.
> I suggest to take another look at a similar patch.
How to fast find out the similar patch. Search the key word doesn't work well.

Thanks,
zhong jiang
> NFS: fix ifnullfree.cocci warnings
> https://lkml.org/lkml/2019/7/7/73
> https://lore.kernel.org/patchwork/patch/1098005/
> https://lore.kernel.org/r/alpine.DEB.2.21.1907071844310.2521@hadrien/
>
> Regards,
> Markus
>
> .
>

[PATCH v2] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages'
compare with zero. And __gup_longterm_locked pass an long local variant
'rc' to check_and_migrate_cma_pages. Hence it is nicer to change the
parameter to long to fix the issue.

Fixes: 932f4a630a69 ("mm/gup: replace get_user_pages_longterm() with 
FOLL_LONGTERM")
Signed-off-by: zhong jiang 
---
 mm/gup.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 23a9f9c..ee0b71f 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1433,13 +1433,13 @@ static struct page *new_non_cma_page(struct page *page, 
unsigned long private)
 static long check_and_migrate_cma_pages(struct task_struct *tsk,
struct mm_struct *mm,
unsigned long start,
-   unsigned long nr_pages,
+   long nr_pages,
struct page **pages,
struct vm_area_struct **vmas,
unsigned int gup_flags)
 {
-   unsigned long i;
-   unsigned long step;
+   long i;
+   long step;
bool drain_allow = true;
bool migrate_allow = true;
LIST_HEAD(cma_page_list);
@@ -1520,7 +1520,7 @@ static long check_and_migrate_cma_pages(struct 
task_struct *tsk,
 static long check_and_migrate_cma_pages(struct task_struct *tsk,
struct mm_struct *mm,
unsigned long start,
-   unsigned long nr_pages,
+   long nr_pages,
struct page **pages,
struct vm_area_struct **vmas,
unsigned int gup_flags)
-- 
1.7.12.4

Re: [PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

On 2019/9/4 19:24, Vlastimil Babka wrote:
> On 9/4/19 12:26 PM, zhong jiang wrote:
>> With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"'
>> compare with zero. And __get_user_pages_locked will return an long value.
>> Hence, Convert the long to compare with zero is feasible.
> It would be nicer if the parameter nr_pages was long again instead of unsigned
> long (note there are two variants of the function, so both should be changed).
Yep, the parameter 'nr_pages' was changed to long. and the variants ‘i、step’ 
should
be changed accordingly.
>> Signed-off-by: zhong jiang 
> Fixes: 932f4a630a69 ("mm/gup: replace get_user_pages_longterm() with 
> FOLL_LONGTERM")
>
> (which changed long to unsigned long)
>
> AFAICS... stable shouldn't be needed as the only "risk" is that we goto
> check_again even when we fail, which should be harmless.
Agreed, Thanks.

Sincerely,
zhong jiang
> Vlastimil
>
>> ---
>>  mm/gup.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 23a9f9c..956d5a1 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1508,7 +1508,7 @@ static long check_and_migrate_cma_pages(struct 
>> task_struct *tsk,
>> pages, vmas, NULL,
>> gup_flags);
>>  
>> -if ((nr_pages > 0) && migrate_allow) {
>> +if (((long)nr_pages > 0) && migrate_allow) {
>>  drain_allow = true;
>>  goto check_again;
>>  }
>>
>
> .
>

Re: net: hsr: remove a redundant null check before kfree_skb

[zhong jiang]

On 2019/9/4 19:55, Markus Elfring wrote:
>> kfree_skb has taken the null pointer into account.
> I suggest to take another look also at information around
> a similar update suggestion.
>
> net-hsr: Delete unnecessary checks before the function call "kfree_skb"
> https://lkml.org/lkml/2015/11/14/120
> https://lore.kernel.org/patchwork/patch/617878/
> https://lore.kernel.org/r/5647a77e.6040...@users.sourceforge.net/
>
> https://lkml.org/lkml/2015/11/24/433
> https://lore.kernel.org/r/56546951.9080...@alten.se/
Thanks you for explaination. I miss the similar patch before sending it.

Sincerely,
zhong jiang
> Regards,
> Markus
>
> .
>

[PATCH] mm: Unsigned 'nr_pages' always larger than zero

[zhong jiang]

With the help of unsigned_lesser_than_zero.cocci. Unsigned 'nr_pages"'
compare with zero. And __get_user_pages_locked will return an long value.
Hence, Convert the long to compare with zero is feasible.

Signed-off-by: zhong jiang 
---
 mm/gup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c
index 23a9f9c..956d5a1 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1508,7 +1508,7 @@ static long check_and_migrate_cma_pages(struct 
task_struct *tsk,
   pages, vmas, NULL,
   gup_flags);
 
-   if ((nr_pages > 0) && migrate_allow) {
+   if (((long)nr_pages > 0) && migrate_allow) {
drain_allow = true;
goto check_again;
}
-- 
1.7.12.4

[PATCH] staging: exfat: remove the redundant check when kfree an object in exfat_destroy_inode

[zhong jiang]

kfree has taken the null check in account. hence it is unnecessary to add the
null check before kfree the object. Just remove it.

Signed-off-by: zhong jiang 
---
 drivers/staging/exfat/exfat_super.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/exfat/exfat_super.c 
b/drivers/staging/exfat/exfat_super.c
index 5b5c2ca..87f858b 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -3487,8 +3487,7 @@ static struct inode *exfat_alloc_inode(struct super_block 
*sb)
 
 static void exfat_destroy_inode(struct inode *inode)
 {
-   if (EXFAT_I(inode)->target)
-   kfree(EXFAT_I(inode)->target);
+   kfree(EXFAT_I(inode)->target);
EXFAT_I(inode)->target = NULL;
 
kmem_cache_free(exfat_inode_cachep, EXFAT_I(inode));
-- 
1.7.12.4

[PATCH] ath9k: Remove unneeded variable to store return value

[zhong jiang]

ath9k_reg_rmw_single do not need return value to cope with different
cases. And change functon return type to void.

Signed-off-by: zhong jiang 
---
 drivers/net/wireless/ath/ath9k/htc_drv_init.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c 
b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index 214c682..d961095 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -463,7 +463,7 @@ static void ath9k_enable_rmw_buffer(void *hw_priv)
atomic_inc(>wmi->m_rmw_cnt);
 }
 
-static u32 ath9k_reg_rmw_single(void *hw_priv,
+static void ath9k_reg_rmw_single(void *hw_priv,
 u32 reg_offset, u32 set, u32 clr)
 {
struct ath_hw *ah = hw_priv;
@@ -471,7 +471,6 @@ static u32 ath9k_reg_rmw_single(void *hw_priv,
struct ath9k_htc_priv *priv = (struct ath9k_htc_priv *) common->priv;
struct register_rmw buf, buf_ret;
int ret;
-   u32 val = 0;
 
buf.reg = cpu_to_be32(reg_offset);
buf.set = cpu_to_be32(set);
@@ -485,7 +484,6 @@ static u32 ath9k_reg_rmw_single(void *hw_priv,
ath_dbg(common, WMI, "REGISTER RMW FAILED:(0x%04x, %d)\n",
reg_offset, ret);
}
-   return val;
 }
 
 static u32 ath9k_reg_rmw(void *hw_priv, u32 reg_offset, u32 set, u32 clr)
-- 
1.7.12.4

[PATCH 1/3] rtlwifi: Remove an unnecessary continue in _rtl8723be_phy_config_bb_with_pgheaderfile

[zhong jiang]

Continue is not needed at the bottom of a loop. Hence just drop it.

Signed-off-by: zhong jiang 
---
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c 
b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c
index aa8a095..4805b84 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c
@@ -732,7 +732,6 @@ static bool 
_rtl8723be_phy_config_bb_with_pgheaderfile(struct ieee80211_hw *hw,
else
_rtl8723be_store_tx_power_by_rate(hw,
v1, v2, v3, v4, v5, v6);
-   continue;
}
}
} else {
-- 
1.7.12.4

[PATCH 2/3] nfp: Drop unnecessary continue in nfp_net_pf_alloc_vnics

[zhong jiang]

Continue is not needed at the bottom of a loop.

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/netronome/nfp/nfp_net_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c 
b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
index 986464d..68db47d 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
@@ -205,10 +205,8 @@ static void nfp_net_pf_free_vnics(struct nfp_pf *pf)
ctrl_bar += NFP_PF_CSR_SLICE_SIZE;
 
/* Kill the vNIC if app init marked it as invalid */
-   if (nn->port && nn->port->type == NFP_PORT_INVALID) {
+   if (nn->port && nn->port->type == NFP_PORT_INVALID)
nfp_net_pf_free_vnic(pf, nn);
-   continue;
-   }
}
 
if (list_empty(>vnics))
-- 
1.7.12.4

[PATCH 3/3] ath10k: Drop unnecessary continue in ath10k_mac_update_vif_chan

[zhong jiang]

Continue is not needed at the bottom of a loop. Hence just remove it.

Signed-off-by: zhong jiang 
---
 drivers/net/wireless/ath/ath10k/mac.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index 12dad65..91e4635 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -7804,11 +7804,9 @@ static int ath10k_ampdu_action(struct ieee80211_hw *hw,
continue;
 
ret = ath10k_wmi_vdev_down(ar, arvif->vdev_id);
-   if (ret) {
+   if (ret)
ath10k_warn(ar, "failed to down vdev %d: %d\n",
arvif->vdev_id, ret);
-   continue;
-   }
}
 
/* All relevant vdevs are downed and associated channel resources
-- 
1.7.12.4

[PATCH 0/3] net: remove an redundant continue

[zhong jiang]

With the help of Coccinelle. we find some place to replace.

@@

for (...;...;...) {
   ...
   if (...) {
 ...
-   continue;
   }
}


zhong jiang (3):
  rtlwifi: Remove an unnecessary continue in
_rtl8723be_phy_config_bb_with_pgheaderfile
  nfp: Drop unnecessary continue in nfp_net_pf_alloc_vnics
  ath10k: Drop unnecessary continue in ath10k_mac_update_vif_chan

 drivers/net/ethernet/netronome/nfp/nfp_net_main.c| 4 +---
 drivers/net/wireless/ath/ath10k/mac.c| 4 +---
 drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c | 1 -
 3 files changed, 2 insertions(+), 7 deletions(-)

-- 
1.7.12.4

[PATCH] scsi: qedi: remove an redundant null check before kfree_skb

[zhong jiang]

kfree_skb has taken null pointer into account. Hence it is unnecessary
to check it before kfree_skb. Just remove the condition.

Signed-off-by: zhong jiang 
---
 drivers/scsi/qedi/qedi_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/qedi/qedi_main.c b/drivers/scsi/qedi/qedi_main.c
index acb930b..dabf425 100644
--- a/drivers/scsi/qedi/qedi_main.c
+++ b/drivers/scsi/qedi/qedi_main.c
@@ -789,8 +789,7 @@ static void qedi_ll2_free_skbs(struct qedi_ctx *qedi)
spin_lock_bh(>ll2_lock);
list_for_each_entry_safe(work, work_tmp, >ll2_skb_list, list) {
list_del(>list);
-   if (work->skb)
-   kfree_skb(work->skb);
+   kfree_skb(work->skb);
kfree(work);
}
spin_unlock_bh(>ll2_lock);
-- 
1.7.12.4

[PATCH] net: hsr: remove an redundant null check before kfree_skb

[zhong jiang]

kfree_skb has taken the null pointer into account. Hence just remove
the null check before kfree_skb.

Signed-off-by: zhong jiang 
---
 net/hsr/hsr_forward.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index ddd9605..0c9e5b0 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -367,10 +367,8 @@ void hsr_forward_skb(struct sk_buff *skb, struct hsr_port 
*port)
port->dev->stats.tx_bytes += skb->len;
}
 
-   if (frame.skb_hsr)
-   kfree_skb(frame.skb_hsr);
-   if (frame.skb_std)
-   kfree_skb(frame.skb_std);
+   kfree_skb(frame.skb_hsr);
+   kfree_skb(frame.skb_std);
return;
 
 out_drop:
-- 
1.7.12.4

[PATCH 2/3] sunrpc: Use kzfree rather than its implementation.

[zhong jiang]

Use kzfree instead of memset() + kfree().

Signed-off-by: zhong jiang 
---
 net/sunrpc/auth_gss/gss_krb5_keys.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/auth_gss/gss_krb5_keys.c 
b/net/sunrpc/auth_gss/gss_krb5_keys.c
index 550fdf1..3b7f721 100644
--- a/net/sunrpc/auth_gss/gss_krb5_keys.c
+++ b/net/sunrpc/auth_gss/gss_krb5_keys.c
@@ -228,14 +228,11 @@ u32 krb5_derive_key(const struct gss_krb5_enctype *gk5e,
ret = 0;
 
 err_free_raw:
-   memset(rawkey, 0, keybytes);
-   kfree(rawkey);
+   kzfree(rawkey);
 err_free_out:
-   memset(outblockdata, 0, blocksize);
-   kfree(outblockdata);
+   kzfree(outblockdata);
 err_free_in:
-   memset(inblockdata, 0, blocksize);
-   kfree(inblockdata);
+   kzfree(inblockdata);
 err_free_cipher:
crypto_free_sync_skcipher(cipher);
 err_return:
-- 
1.7.12.4

[PATCH 1/3] ixgbe: Use kzfree() rather than its implementation.

[zhong jiang]

Use kzfree() instead of memset() + kfree().

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c 
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
index 31629fc..113f608 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
@@ -960,11 +960,9 @@ int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
return 0;
 
 err_aead:
-   memset(xs->aead, 0, sizeof(*xs->aead));
-   kfree(xs->aead);
+   kzfree(xs->aead);
 err_xs:
-   memset(xs, 0, sizeof(*xs));
-   kfree(xs);
+   kzfree(xs);
 err_out:
msgbuf[1] = err;
return err;
@@ -1049,8 +1047,7 @@ int ixgbe_ipsec_vf_del_sa(struct ixgbe_adapter *adapter, 
u32 *msgbuf, u32 vf)
ixgbe_ipsec_del_sa(xs);
 
/* remove the xs that was made-up in the add request */
-   memset(xs, 0, sizeof(*xs));
-   kfree(xs);
+   kzfree(xs);
 
return 0;
 }
-- 
1.7.12.4

[PATCH 0/3] net: Use kzfree() directly

[zhong jiang]

With the help of Coccinelle. We find some place to replace.

@@
expression M, S;
@@

- memset(M, 0, S);
- kfree(M);
+ kzfree(M); 

zhong jiang (3):
  ixgbe: Use kzfree() rather than its implementation.
  sunrpc: Use kzfree rather than its implementation.
  net: mpoa: Use kzfree rather than its implementation.

 drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 9 +++--
 net/atm/mpoa_caches.c  | 6 ++
 net/sunrpc/auth_gss/gss_krb5_keys.c| 9 +++--
 3 files changed, 8 insertions(+), 16 deletions(-)

-- 
1.7.12.4

[PATCH 3/3] net: mpoa: Use kzfree rather than its implementation.

[zhong jiang]

Use kzfree instead of memset() + kfree().

Signed-off-by: zhong jiang 
---
 net/atm/mpoa_caches.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/atm/mpoa_caches.c b/net/atm/mpoa_caches.c
index 4bb4183..3286f9d 100644
--- a/net/atm/mpoa_caches.c
+++ b/net/atm/mpoa_caches.c
@@ -180,8 +180,7 @@ static int cache_hit(in_cache_entry *entry, struct 
mpoa_client *mpc)
 static void in_cache_put(in_cache_entry *entry)
 {
if (refcount_dec_and_test(>use)) {
-   memset(entry, 0, sizeof(in_cache_entry));
-   kfree(entry);
+   kzfree(entry);
}
 }
 
@@ -416,8 +415,7 @@ static eg_cache_entry *eg_cache_get_by_src_ip(__be32 ipaddr,
 static void eg_cache_put(eg_cache_entry *entry)
 {
if (refcount_dec_and_test(>use)) {
-   memset(entry, 0, sizeof(eg_cache_entry));
-   kfree(entry);
+   kzfree(entry);
}
 }
 
-- 
1.7.12.4

Re: [PATCH] fs: omfs: Use kmemdup rather than duplicating its implementation in omfs_get_imap

[zhong jiang]

On 2019/9/3 21:25, Bob Copeland wrote:
> On Tue, Sep 03, 2019 at 02:39:44PM +0800, zhong jiang wrote:
>> kmemdup contains the kmalloc + memcpy. hence it is better to use kmemdup
>> directly. Just replace it.
>>
>> Signed-off-by: zhong jiang 
> This same patch was already sent to me by someone else and I acked it:
>
> https://lore.kernel.org/lkml/20190703163158.937-1-huangfq.dax...@gmail.com/
>
I miss the patch.  Thanks,

Sincerely,
zhong jiang

Re: [PATCH] hostap: remove set but not used variable 'copied' in prism2_io_debug_proc_read

[zhong jiang]

Please ignore the patch.  Because  the hostap_proc.c is marked as 'obsolete'.

Thanks,
zhong jiang
On 2019/9/3 15:57, zhong jiang wrote:
> Obviously, variable 'copied' is initialized to zero. But it is not used.
> hence just remove it.
>
> Signed-off-by: zhong jiang 
> ---
>  drivers/net/wireless/intersil/hostap/hostap_proc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/intersil/hostap/hostap_proc.c 
> b/drivers/net/wireless/intersil/hostap/hostap_proc.c
> index 703d74c..6151d8d 100644
> --- a/drivers/net/wireless/intersil/hostap/hostap_proc.c
> +++ b/drivers/net/wireless/intersil/hostap/hostap_proc.c
> @@ -234,7 +234,7 @@ static int prism2_io_debug_proc_read(char *page, char 
> **start, off_t off,
>  {
>   local_info_t *local = (local_info_t *) data;
>   int head = local->io_debug_head;
> - int start_bytes, left, copy, copied;
> + int start_bytes, left, copy;
>  
>   if (off + count > PRISM2_IO_DEBUG_SIZE * 4) {
>   *eof = 1;
> @@ -243,7 +243,6 @@ static int prism2_io_debug_proc_read(char *page, char 
> **start, off_t off,
>   count = PRISM2_IO_DEBUG_SIZE * 4 - off;
>   }
>  
> - copied = 0;
>   start_bytes = (PRISM2_IO_DEBUG_SIZE - head) * 4;
>   left = count;
>  

[PATCH] hostap: remove set but not used variable 'copied' in prism2_io_debug_proc_read

[zhong jiang]

Obviously, variable 'copied' is initialized to zero. But it is not used.
hence just remove it.

Signed-off-by: zhong jiang 
---
 drivers/net/wireless/intersil/hostap/hostap_proc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intersil/hostap/hostap_proc.c 
b/drivers/net/wireless/intersil/hostap/hostap_proc.c
index 703d74c..6151d8d 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_proc.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_proc.c
@@ -234,7 +234,7 @@ static int prism2_io_debug_proc_read(char *page, char 
**start, off_t off,
 {
local_info_t *local = (local_info_t *) data;
int head = local->io_debug_head;
-   int start_bytes, left, copy, copied;
+   int start_bytes, left, copy;
 
if (off + count > PRISM2_IO_DEBUG_SIZE * 4) {
*eof = 1;
@@ -243,7 +243,6 @@ static int prism2_io_debug_proc_read(char *page, char 
**start, off_t off,
count = PRISM2_IO_DEBUG_SIZE * 4 - off;
}
 
-   copied = 0;
start_bytes = (PRISM2_IO_DEBUG_SIZE - head) * 4;
left = count;
 
-- 
1.7.12.4

[PATCH] net/mlx5: Use PTR_ERR_OR_ZERO rather than its implementation

[zhong jiang]

PTR_ERR_OR_ZERO contains if(IS_ERR(...)) + PTR_ERR. It is better
to use it directly. hence just replace it.

Signed-off-by: zhong jiang 
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c 
b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 5581a80..2e0b467 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -989,10 +989,7 @@ static void mlx5e_hairpin_flow_del(struct mlx5e_priv *priv,
_act, dest, dest_ix);
mutex_unlock(>fs.tc.t_lock);
 
-   if (IS_ERR(flow->rule[0]))
-   return PTR_ERR(flow->rule[0]);
-
-   return 0;
+   return PTR_ERR_OR_ZERO(flow->rule[0]);
 }
 
 static void mlx5e_tc_del_nic_flow(struct mlx5e_priv *priv,
-- 
1.7.12.4

[PATCH] fs: omfs: Use kmemdup rather than duplicating its implementation in omfs_get_imap

[zhong jiang]

kmemdup contains the kmalloc + memcpy. hence it is better to use kmemdup
directly. Just replace it.

Signed-off-by: zhong jiang 
---
 fs/omfs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/omfs/inode.c b/fs/omfs/inode.c
index b76ec6b..8867cef 100644
--- a/fs/omfs/inode.c
+++ b/fs/omfs/inode.c
@@ -363,12 +363,11 @@ static int omfs_get_imap(struct super_block *sb)
bh = sb_bread(sb, block++);
if (!bh)
goto nomem_free;
-   *ptr = kmalloc(sb->s_blocksize, GFP_KERNEL);
+   *ptr = kmemdup(bh->b_data, sb->s_blocksize, GFP_KERNEL);
if (!*ptr) {
brelse(bh);
goto nomem_free;
}
-   memcpy(*ptr, bh->b_data, sb->s_blocksize);
if (count < sb->s_blocksize)
memset((void *)*ptr + count, 0xff,
sb->s_blocksize - count);
-- 
1.7.12.4

[PATCH] NFS: remove the redundant check when kfree an object in nfs_netns_client_release

[zhong jiang]

kfree has taken the null check in account. hence it is unnecessary to add the
null check before kfree the object. Just remove it.

Signed-off-by: zhong jiang 
---
 fs/nfs/sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/nfs/sysfs.c b/fs/nfs/sysfs.c
index 4f3390b..c489496 100644
--- a/fs/nfs/sysfs.c
+++ b/fs/nfs/sysfs.c
@@ -121,8 +121,7 @@ static void nfs_netns_client_release(struct kobject *kobj)
struct nfs_netns_client,
kobject);
 
-   if (c->identifier)
-   kfree(c->identifier);
+   kfree(c->identifier);
kfree(c);
 }
 
-- 
1.7.12.4

Re: [PATCH] mm/mempolicy: Fix an incorrect rebind node in mpol_rebind_nodemask

[zhong jiang]

On 2019/5/27 20:23, Vlastimil Babka wrote:
> On 5/25/19 8:28 PM, Andrew Morton wrote:
>> (Cc Vlastimil)
> Oh dear, 2 years and I forgot all the details about how this works.
>
>> On Sat, 25 May 2019 15:07:23 +0800 zhong jiang  wrote:
>>
>>> We bind an different node to different vma, Unluckily,
>>> it will bind different vma to same node by checking the 
>>> /proc/pid/numa_maps.   
>>> Commit 213980c0f23b ("mm, mempolicy: simplify rebinding mempolicies when 
>>> updating cpusets")
>>> has introduced the issue.  when we change memory policy by seting 
>>> cpuset.mems,
>>> A process will rebind the specified policy more than one times. 
>>> if the cpuset_mems_allowed is not equal to user specified nodes. hence the 
>>> issue will trigger.
>>> Maybe result in the out of memory which allocating memory from same node.
> I have a hard time understanding what the problem is. Could you please
> write it as a (pseudo) reproducer? I.e. an example of the process/admin
> mempolicy/cpuset actions that have some wrong observed results vs the
> correct expected result.
Sorry, I havn't an testcase to reproduce the issue. At first, It was 
disappeared by
my colleague to configure the xml to start an vm.  To his suprise, The bind 
mempolicy
doesn't work.

Thanks,
zhong jiang
>>> --- a/mm/mempolicy.c
>>> +++ b/mm/mempolicy.c
>>> @@ -345,7 +345,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, 
>>> const nodemask_t *nodes)
>>> else {
>>> nodes_remap(tmp, pol->v.nodes,pol->w.cpuset_mems_allowed,
>>> *nodes);
>>> -   pol->w.cpuset_mems_allowed = tmp;
>>> +   pol->w.cpuset_mems_allowed = *nodes;
> Looks like a mechanical error on my side when removing the code for
> step1+step2 rebinding. Before my commit there was
>
> pol->w.cpuset_mems_allowed = step ? tmp : *nodes;
>
> Since 'step' was removed and thus 0, I should have used *nodes indeed.
> Thanks for catching that.
>
>>> }
>>>  
>>> if (nodes_empty(tmp))
>> hm, I'm not surprised the code broke.  What the heck is going on in
>> there?  It used to have a perfunctory comment, but Vlastimil deleted
>> it.
> Yeah the comment was specific for the case that was being removed.
>
>> Could someone please propose a comment for the above code block
>> explaining why we're doing what we do?
> I'll have to relearn this first...
>
>

[PATCH] mm/mempolicy: Fix an incorrect rebind node in mpol_rebind_nodemask

[zhong jiang]

We bind an different node to different vma, Unluckily,
it will bind different vma to same node by checking the /proc/pid/numa_maps.   
Commit 213980c0f23b ("mm, mempolicy: simplify rebinding mempolicies when 
updating cpusets")
has introduced the issue.  when we change memory policy by seting cpuset.mems,
A process will rebind the specified policy more than one times. 
if the cpuset_mems_allowed is not equal to user specified nodes. hence the 
issue will trigger.
Maybe result in the out of memory which allocating memory from same node.

Fixes: 213980c0f23b ("mm, mempolicy: simplify rebinding mempolicies when 
updating cpusets") 
Signed-off-by: zhong jiang 
---
 mm/mempolicy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index e3ab1d9..a60a3be 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -345,7 +345,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, 
const nodemask_t *nodes)
else {
nodes_remap(tmp, pol->v.nodes,pol->w.cpuset_mems_allowed,
*nodes);
-   pol->w.cpuset_mems_allowed = tmp;
+   pol->w.cpuset_mems_allowed = *nodes;
}
 
if (nodes_empty(tmp))
-- 
1.7.12.4

Re: [PATCH] mm/memory_hotplug: Do not unlock when fails to take the device_hotplug_lock

[zhong jiang]

I am sorry,  It is incorrect.  please ignore the patch.  I will resent it.

Thanks,
zhong jiang
On 2019/4/8 12:00, zhong jiang wrote:
> When adding the memory by probing memory block in sysfs interface, there is an
> obvious issue that we will unlock the device_hotplug_lock when fails to takes 
> it.
>
> That issue was introduced in Commit 8df1d0e4a265
> ("mm/memory_hotplug: make add_memory() take the device_hotplug_lock")
>
> We should drop out in time when fails to take the device_hotplug_lock.
>
> Fixes: 8df1d0e4a265 ("mm/memory_hotplug: make add_memory() take the 
> device_hotplug_lock")
> Reported-by: Yang yingliang 
> Signed-off-by: zhong jiang 
> ---
>  drivers/base/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index d9ebb89..8b0cec7 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -507,7 +507,7 @@ static ssize_t probe_store(struct device *dev, struct 
> device_attribute *attr,
>  
>   ret = lock_device_hotplug_sysfs();
>   if (ret)
> - goto out;
> + goto ret;
>  
>   nid = memory_add_physaddr_to_nid(phys_addr);
>   ret = __add_memory(nid, phys_addr,

[RESENT PATCH] mm/memory_hotplug: Do not unlock when fails to take the device_hotplug_lock

[zhong jiang]

When adding the memory by probing memory block in sysfs interface, there is an
obvious issue that we will unlock the device_hotplug_lock when fails to takes 
it.

That issue was introduced in Commit 8df1d0e4a265
("mm/memory_hotplug: make add_memory() take the device_hotplug_lock")

We should drop out in time when fails to take the device_hotplug_lock.

Fixes: 8df1d0e4a265 ("mm/memory_hotplug: make add_memory() take the 
device_hotplug_lock")
Reported-by: Yang yingliang 
Signed-off-by: zhong jiang 
---
 drivers/base/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index d9ebb89..0c9e22f 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -507,7 +507,7 @@ static ssize_t probe_store(struct device *dev, struct 
device_attribute *attr,
 
ret = lock_device_hotplug_sysfs();
if (ret)
-   goto out;
+   return ret;
 
nid = memory_add_physaddr_to_nid(phys_addr);
ret = __add_memory(nid, phys_addr,
-- 
1.7.12.4

[PATCH] mm/memory_hotplug: Do not unlock when fails to take the device_hotplug_lock

[zhong jiang]

When adding the memory by probing memory block in sysfs interface, there is an
obvious issue that we will unlock the device_hotplug_lock when fails to takes 
it.

That issue was introduced in Commit 8df1d0e4a265
("mm/memory_hotplug: make add_memory() take the device_hotplug_lock")

We should drop out in time when fails to take the device_hotplug_lock.

Fixes: 8df1d0e4a265 ("mm/memory_hotplug: make add_memory() take the 
device_hotplug_lock")
Reported-by: Yang yingliang 
Signed-off-by: zhong jiang 
---
 drivers/base/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index d9ebb89..8b0cec7 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -507,7 +507,7 @@ static ssize_t probe_store(struct device *dev, struct 
device_attribute *attr,
 
ret = lock_device_hotplug_sysfs();
if (ret)
-   goto out;
+   goto ret;
 
nid = memory_add_physaddr_to_nid(phys_addr);
ret = __add_memory(nid, phys_addr,
-- 
1.7.12.4

Re: KASAN: use-after-free Read in get_mem_cgroup_from_mm

[zhong jiang]

On 2019/3/17 3:42, Andrea Arcangeli wrote:
> On Sat, Mar 16, 2019 at 05:38:54PM +0800, zhong jiang wrote:
>> On 2019/3/16 5:39, Andrea Arcangeli wrote:
>>> On Fri, Mar 08, 2019 at 03:10:08PM +0800, zhong jiang wrote:
>>>> I can reproduce the issue in arm64 qemu machine.  The issue will leave 
>>>> after applying the
>>>> patch.
>>>>
>>>> Tested-by: zhong jiang 
>>> Thanks a lot for the quick testing!
>>>
>>>> Meanwhile,  I just has a little doubt whether it is necessary to use RCU 
>>>> to free the task struct or not.
>>>> I think that mm->owner alway be NULL after failing to create to process. 
>>>> Because we call mm_clear_owner.
>>> I wish it was enough, but the problem is that the other CPU may be in
>>> the middle of get_mem_cgroup_from_mm() while this runs, and it would
>>> dereference mm->owner while it is been freed without the call_rcu
>>> affter we clear mm->owner. What prevents this race is the
>> As you had said, It would dereference mm->owner after we clear mm->owner.
>>
>> But after we clear mm->owner,  mm->owner should be NULL.  Is it right?
>>
>> And mem_cgroup_from_task will check the parameter. 
>> you mean that it is possible after checking the parameter to  clear the 
>> owner .
>> and the NULL pointer will trigger. :-(
> Dereference mm->owner didn't mean reading the value of the mm->owner
> pointer, it really means to dereference the value of the pointer. It's
> like below:
>
> get_mem_cgroup_from_mm()  failing fork()
>   ---
> task = mm->owner
>   mm->owner = NULL;
>   free(mm->owner)
> *task /* use after free */
>
> We didn't set mm->owner to NULL before, so the window for the race was
> larger, but setting mm->owner to NULL only hides the problem and it
> can still happen (albeit with a smaller window).
>
> If get_mem_cgroup_from_mm() can see at any time mm->owner not NULL,
> then the free of the task struct must be delayed until after
> rcu_read_unlock has returned in get_mem_cgroup_from_mm(). This is
> the standard RCU model, the freeing must be delayed until after the
> next quiescent point.

Thank you for your explaination patiently.  The patch should go to upstream 
too.  I think you
should send a formal patch to the mainline.  Maybe other people suffer from
the issue.  :-)

Thanks,
zhong jiang
> BTW, both mm_update_next_owner() and mm_clear_owner() should have used
> WRITE_ONCE when they write to mm->owner, I can update that too but
> it's just to not to make assumptions that gcc does the right thing
> (and we still rely on gcc to do the right thing in other places) so
> that is just an orthogonal cleanup.
>
> Thanks,
> Andrea
>
> .
>

Re: KASAN: use-after-free Read in get_mem_cgroup_from_mm

[zhong jiang]

On 2019/3/16 5:39, Andrea Arcangeli wrote:
> On Fri, Mar 08, 2019 at 03:10:08PM +0800, zhong jiang wrote:
>> I can reproduce the issue in arm64 qemu machine.  The issue will leave after 
>> applying the
>> patch.
>>
>> Tested-by: zhong jiang 
> Thanks a lot for the quick testing!
>
>> Meanwhile,  I just has a little doubt whether it is necessary to use RCU to 
>> free the task struct or not.
>> I think that mm->owner alway be NULL after failing to create to process. 
>> Because we call mm_clear_owner.
> I wish it was enough, but the problem is that the other CPU may be in
> the middle of get_mem_cgroup_from_mm() while this runs, and it would
> dereference mm->owner while it is been freed without the call_rcu
> affter we clear mm->owner. What prevents this race is the
As you had said, It would dereference mm->owner after we clear mm->owner.

But after we clear mm->owner,  mm->owner should be NULL.  Is it right?

And mem_cgroup_from_task will check the parameter. 
you mean that it is possible after checking the parameter to  clear the owner .
and the NULL pointer will trigger. :-(

Thanks,
zhong jiang
> rcu_read_lock() in get_mem_cgroup_from_mm() and the corresponding
> call_rcu to free the task struct in the fork failure path (again only
> if CONFIG_MEMCG=y is defined). Considering you can reproduce this tiny
> race on arm64 qemu (perhaps tcg JIT timing variantions helps?), you
> might also in theory be able to still reproduce the race condition if
> you remove the call_rcu from delayed_free_task and you replace it with
> free_task.
>
> .
>