Re: [PATCH-next 5/5] mm/vmalloc: remove an empty line

[Souptick Joarder]

On Sat, Apr 3, 2021 at 1:53 AM Uladzislau Rezki (Sony)  wrote:
>
> Signed-off-by: Uladzislau Rezki (Sony) 

How about merging it with patch [4/5] ?

> ---
>  mm/vmalloc.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 093c7e034ca2..1e643280cbcf 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1503,7 +1503,6 @@ static struct vmap_area *alloc_vmap_area(unsigned long 
> size,
> va->va_end = addr + size;
> va->vm = NULL;
>
> -
> spin_lock(_area_lock);
> insert_vmap_area(va, _area_root, _area_list);
> spin_unlock(_area_lock);
> --
> 2.20.1
>
>

Re: [PATCH] perf/core: Mark perf_pmu_snapshot_aux() as static

[Souptick Joarder]

On Sat, Jan 9, 2021 at 7:47 AM Souptick Joarder  wrote:
>
> Kernel test robot throws below warning ->
>
>  kernel/events/core.c:6535:6: warning: no previous prototype for
> 'perf_pmu_snapshot_aux' [-Wmissing-prototypes]
> 6535 | long perf_pmu_snapshot_aux(struct perf_buffer *rb,
>  |  ^
> Marking perf_pmu_snapshot_aux() as static as it is not used outside
> this file.
>
> Reported-by: kernel test robot 
> Signed-off-by: Souptick Joarder 

Any comment on this patch ?

> ---
>  kernel/events/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 55d1879..a4ba6fd 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -6532,7 +6532,7 @@ static unsigned long perf_prepare_sample_aux(struct 
> perf_event *event,
> return data->aux_size;
>  }
>
> -long perf_pmu_snapshot_aux(struct perf_buffer *rb,
> +static long perf_pmu_snapshot_aux(struct perf_buffer *rb,
>struct perf_event *event,
>struct perf_output_handle *handle,
>unsigned long size)
> --
> 1.9.1
>

[PATCH] drm/amdgpu: Mark mmhub_v1_7_setup_vm_pt_regs() as static

[Souptick Joarder]

Kernel test robot throws below warning ->

drivers/gpu/drm/amd/amdgpu/mmhub_v1_7.c:56:6: warning: no previous
prototype for 'mmhub_v1_7_setup_vm_pt_regs' [-Wmissing-prototypes]

Mark mmhub_v1_7_setup_vm_pt_regs() as static.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/gpu/drm/amd/amdgpu/mmhub_v1_7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_7.c 
b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_7.c
index 4df0b73..ae7d8a1 100644
--- a/drivers/gpu/drm/amd/amdgpu/mmhub_v1_7.c
+++ b/drivers/gpu/drm/amd/amdgpu/mmhub_v1_7.c
@@ -53,7 +53,7 @@ static u64 mmhub_v1_7_get_fb_location(struct amdgpu_device 
*adev)
return base;
 }
 
-void mmhub_v1_7_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t vmid,
+static void mmhub_v1_7_setup_vm_pt_regs(struct amdgpu_device *adev, uint32_t 
vmid,
uint64_t page_table_base)
 {
struct amdgpu_vmhub *hub = >vmhub[AMDGPU_MMHUB_0];
-- 
1.9.1

[PATCH] drm/amdgpu/display: initialize the variable 'i'

[Souptick Joarder]

>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9804:38:
>> warning: variable 'i' is uninitialized when used here
>> [-Wuninitialized]
   timing  = >detailed_timings[i];
 ^
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9714:7:
note: initialize the variable 'i' to silence this warning
   int i;
^
 = 0
   1 warning generated.

Initialize the variable 'i'.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a22a53d..e96d3d9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9717,7 +9717,7 @@ static bool parse_hdmi_amd_vsdb(struct 
amdgpu_dm_connector *aconnector,
 void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
struct edid *edid)
 {
-   int i;
+   int i = 0;
struct detailed_timing *timing;
struct detailed_non_pixel *data;
struct detailed_data_monitor_range *range;
-- 
1.9.1

Re: [PATCH] mm/hugetlb: Fix some comment typos

[Souptick Joarder]

On Thu, Jan 28, 2021 at 4:50 PM Miaohe Lin  wrote:
>
> Fix typos sasitfy to satisfy, reservtion to reservation, hugegpage to
> hugepage and uniprocesor to uniprocessor in comments.
>
> Signed-off-by: Miaohe Lin 

Reviewed-by: Souptick Joarder 

> ---
>  include/linux/hugetlb.h | 2 +-
>  mm/hugetlb.c| 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index ef5b144b8aac..95a5a239c8f2 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -37,7 +37,7 @@ struct hugepage_subpool {
> struct hstate *hstate;
> long min_hpages;/* Minimum huge pages or -1 if no minimum. */
> long rsv_hpages;/* Pages reserved against global pool to */
> -   /* sasitfy minimum size. */
> +   /* satisfy minimum size. */
>  };
>
>  struct resv_map {
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 53ea65d1c5ab..c42c61c2653e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1444,7 +1444,7 @@ static void __free_huge_page(struct page *page)
>  * reservation.  If the page was associated with a subpool, there
>  * would have been a page reserved in the subpool before allocation
>  * via hugepage_subpool_get_pages().  Since we are 'restoring' the
> -* reservtion, do not call hugepage_subpool_put_pages() as this will
> +* reservation, do not call hugepage_subpool_put_pages() as this will
>  * remove the reserved page from the subpool.
>  */
> if (!restore_reserve) {
> @@ -3715,7 +3715,7 @@ static unsigned long hugetlb_vm_op_pagesize(struct 
> vm_area_struct *vma)
>  /*
>   * We cannot handle pagefaults against hugetlb pages at all.  They cause
>   * handle_mm_fault() to try to instantiate regular-sized pages in the
> - * hugegpage VMA.  do_page_fault() is supposed to trap this, so BUG is we get
> + * hugepage VMA.  do_page_fault() is supposed to trap this, so BUG is we get
>   * this far.
>   */
>  static vm_fault_t hugetlb_vm_op_fault(struct vm_fault *vmf)
> @@ -4513,7 +4513,7 @@ u32 hugetlb_fault_mutex_hash(struct address_space 
> *mapping, pgoff_t idx)
>  }
>  #else
>  /*
> - * For uniprocesor systems we always use a single mutex, so just
> + * For uniprocessor systems we always use a single mutex, so just
>   * return 0 and avoid the hashing overhead.
>   */
>  u32 hugetlb_fault_mutex_hash(struct address_space *mapping, pgoff_t idx)
> --
> 2.19.1
>
>

Re: [PATCH] hugetlbfs: remove meaningless variable avoid_reserve

[Souptick Joarder]

On Sat, Jan 16, 2021 at 2:57 PM Miaohe Lin  wrote:
>
> The variable avoid_reserve is meaningless because we never changed its
> value and just passed it to alloc_huge_page(). So remove it to make code
> more clear that in hugetlbfs_fallocate, we never avoid reserve when alloc
> hugepage yet.
>
> Signed-off-by: Miaohe Lin 

Acked-by: Souptick Joarder 

> ---
>  fs/hugetlbfs/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 88751e35e69d..23ad6ed8b75f 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -680,7 +680,6 @@ static long hugetlbfs_fallocate(struct file *file, int 
> mode, loff_t offset,
>  */
> struct page *page;
> unsigned long addr;
> -   int avoid_reserve = 0;
>
> cond_resched();
>
> @@ -717,7 +716,7 @@ static long hugetlbfs_fallocate(struct file *file, int 
> mode, loff_t offset,
> }
>
> /* Allocate page and add to page cache */
> -   page = alloc_huge_page(_vma, addr, avoid_reserve);
> +   page = alloc_huge_page(_vma, addr, 0);
> hugetlb_drop_vma_policy(_vma);
> if (IS_ERR(page)) {
> mutex_unlock(_fault_mutex_table[hash]);
> --
> 2.19.1
>
>

Re: [PATCH] hugetlbfs: correct obsolete function name hugetlbfs_read_iter()

[Souptick Joarder]

On Sat, Jan 16, 2021 at 3:03 PM Miaohe Lin  wrote:
>
> The func do_generic_mapping_read() is killed by commit 36e789144267 ("kill
> do_generic_mapping_read"). So replace it with do_generic_mapping_read to
> keep comment uptodate.

s/func/function

replace it with generic_file_buffered_read()  ?

>
> Signed-off-by: Miaohe Lin 
> ---
>  fs/hugetlbfs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 23ad6ed8b75f..d02616513b43 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -310,7 +310,7 @@ hugetlbfs_read_actor(struct page *page, unsigned long 
> offset,
>
>  /*
>   * Support for read() - Find the page attached to f_mapping and copy out the
> - * data. Its *very* similar to do_generic_mapping_read(), we can't use that
> + * data. Its *very* similar to generic_file_buffered_read(), we can't use 
> that
>   * since it has PAGE_SIZE assumptions.
>   */
>  static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to)
> --
> 2.19.1
>
>

[PATCH] mips: cacheinfo: Remove unnecessary increment of level

[Souptick Joarder]

kernel test robot throws below warning ->

arch/mips/kernel/cacheinfo.c:112:3: warning: Variable 'level' is
modified but its new value is never used. [unreadVariable]

Remove unnecessary increment of level at the end.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 arch/mips/kernel/cacheinfo.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/mips/kernel/cacheinfo.c b/arch/mips/kernel/cacheinfo.c
index 5f9d0eb..c858ae3 100644
--- a/arch/mips/kernel/cacheinfo.c
+++ b/arch/mips/kernel/cacheinfo.c
@@ -109,7 +109,6 @@ static int __populate_cache_leaves(unsigned int cpu)
 
if (c->tcache.waysize) {
populate_cache(tcache, this_leaf, level, CACHE_TYPE_UNIFIED);
-   level++;
}
 
this_cpu_ci->cpu_map_populated = true;
-- 
1.9.1

[PATCH] drm: amdgpu: pm: Mark vangogh_clk_dpm_is_enabled() as static

[Souptick Joarder]

kernel test robot throws below warnings ->

drivers/gpu/drm/amd/amdgpu/../pm/swsmu/smu11/vangogh_ppt.c:594:6:
warning: no previous prototype for 'vangogh_clk_dpm_is_enabled'
[-Wmissing-prototypes]
drivers/gpu/drm/amd/amdgpu/../pm/swsmu/smu11/vangogh_ppt.c:594:6:
warning: no previous prototype for function 'vangogh_clk_dpm_is_enabled'
[-Wmissing-prototypes]

Mark vangogh_clk_dpm_is_enabled() as static.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c 
b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
index 75ddcad..3ffe56e 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/vangogh_ppt.c
@@ -610,7 +610,7 @@ static int vangogh_get_profiling_clk_mask(struct 
smu_context *smu,
return 0;
 }
 
-bool vangogh_clk_dpm_is_enabled(struct smu_context *smu,
+static bool vangogh_clk_dpm_is_enabled(struct smu_context *smu,
enum smu_clk_type clk_type)
 {
enum smu_feature_mask feature_id = 0;
-- 
1.9.1

[PATCH] pinctrl: ti :iodelay: Fixed inconsistent indenting

[Souptick Joarder]

Kernel test robot throws below warning ->

smatch warnings:
drivers/pinctrl/ti/pinctrl-ti-iodelay.c:708
ti_iodelay_pinconf_group_dbg_show() warn: inconsistent indenting

Fixed the inconsistent indenting.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/pinctrl/ti/pinctrl-ti-iodelay.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c 
b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
index ae91559..60a6713 100644
--- a/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
+++ b/drivers/pinctrl/ti/pinctrl-ti-iodelay.c
@@ -705,9 +705,8 @@ static void ti_iodelay_pinconf_group_dbg_show(struct 
pinctrl_dev *pctldev,
 
cfg = >cfg[i];
regmap_read(iod->regmap, cfg->offset, );
-   seq_printf(s, "\n\t0x%08x = 0x%08x (%3d, %3d)",
-  cfg->offset, reg, cfg->a_delay,
-  cfg->g_delay);
+   seq_printf(s, "\n\t0x%08x = 0x%08x (%3d, %3d)",
+   cfg->offset, reg, cfg->a_delay, cfg->g_delay);
}
 }
 #endif
-- 
1.9.1

[PATCH] ASoC: soc-pcm: return correct -ERRNO in failure path

[Souptick Joarder]

Kernel test robot throws below error ->

sound/soc/soc-pcm.c:2523 dpcm_run_update_startup() error: uninitialized
symbol 'ret'.

Initializing ret = 0 and returning correct -ERRNO in failure path.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 sound/soc/soc-pcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 481a4a2..29328ce 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2432,7 +2432,7 @@ static int dpcm_run_update_startup(struct 
snd_soc_pcm_runtime *fe, int stream)
snd_soc_dpcm_get_substream(fe, stream);
struct snd_soc_dpcm *dpcm;
enum snd_soc_dpcm_trigger trigger = fe->dai_link->trigger[stream];
-   int ret;
+   int ret = 0;
unsigned long flags;
 
dev_dbg(fe->dev, "ASoC: runtime %s open on FE %s\n",
@@ -2441,6 +2441,7 @@ static int dpcm_run_update_startup(struct 
snd_soc_pcm_runtime *fe, int stream)
/* Only start the BE if the FE is ready */
if (fe->dpcm[stream].state == SND_SOC_DPCM_STATE_HW_FREE ||
fe->dpcm[stream].state == SND_SOC_DPCM_STATE_CLOSE) {
+   ret = -EINVAL;
dev_err(fe->dev, "ASoC: FE %s is not ready %d\n",
fe->dai_link->name, fe->dpcm[stream].state);
goto disconnect;
-- 
1.9.1

[PATCH] kvm: x86: Mark __kvm_vcpu_halt() as static

[Souptick Joarder]

Kernel test robot throws below warning ->

>> arch/x86/kvm/x86.c:7979:5: warning: no previous prototype for
>> '__kvm_vcpu_halt' [-Wmissing-prototypes]
7979 | int __kvm_vcpu_halt(struct kvm_vcpu *vcpu, int state, int
reason)
 | ^~~

Marking __kvm_vcpu_halt() as static as it is used inside this file.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 61499e1..c2fdf14 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -109,6 +109,7 @@
 static void __kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
 static void store_regs(struct kvm_vcpu *vcpu);
 static int sync_regs(struct kvm_vcpu *vcpu);
+static int __kvm_vcpu_halt(struct kvm_vcpu *vcpu, int state, int reason);
 
 struct kvm_x86_ops kvm_x86_ops __read_mostly;
 EXPORT_SYMBOL_GPL(kvm_x86_ops);
@@ -7976,7 +7977,7 @@ void kvm_arch_exit(void)
kmem_cache_destroy(x86_fpu_cache);
 }
 
-int __kvm_vcpu_halt(struct kvm_vcpu *vcpu, int state, int reason)
+static int __kvm_vcpu_halt(struct kvm_vcpu *vcpu, int state, int reason)
 {
++vcpu->stat.halt_exits;
if (lapic_in_kernel(vcpu)) {
-- 
1.9.1

[PATCH] perf/core: Mark perf_pmu_snapshot_aux() as static

[Souptick Joarder]

Kernel test robot throws below warning ->

 kernel/events/core.c:6535:6: warning: no previous prototype for
'perf_pmu_snapshot_aux' [-Wmissing-prototypes]
6535 | long perf_pmu_snapshot_aux(struct perf_buffer *rb,
 |  ^
Marking perf_pmu_snapshot_aux() as static as it is not used outside
this file.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 kernel/events/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 55d1879..a4ba6fd 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -6532,7 +6532,7 @@ static unsigned long perf_prepare_sample_aux(struct 
perf_event *event,
return data->aux_size;
 }
 
-long perf_pmu_snapshot_aux(struct perf_buffer *rb,
+static long perf_pmu_snapshot_aux(struct perf_buffer *rb,
   struct perf_event *event,
   struct perf_output_handle *handle,
   unsigned long size)
-- 
1.9.1

Re: [PATCH v2] media: atomisp: Fixed error handling path

[Souptick Joarder]

On Wed, Dec 9, 2020 at 1:18 AM Souptick Joarder  wrote:
>
> On Thu, Nov 19, 2020 at 1:06 AM Souptick Joarder  wrote:
> >
> > On Wed, Nov 4, 2020 at 7:32 AM Souptick Joarder  
> > wrote:
> > >
> > > Inside alloc_user_pages() based on flag value either pin_user_pages()
> > > or get_user_pages_fast() will be called. However, these API might fail.
> > >
> > > But free_user_pages() called in error handling path doesn't bother
> > > about return value and will try to unpin bo->pgnr pages, which is
> > > incorrect.
> > >
> > > Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
> > > pages will be unpinned based on bo->mem_type. This will also take care
> > > of non error handling path.
> > >
> > > Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> > > allocation")
> > > Signed-off-by: Souptick Joarder 
> > > Reviewed-by: Dan Carpenter 
> > > Cc: John Hubbard 
> > > Cc: Ira Weiny 
> > > Cc: Dan Carpenter 
> > > ---
> > > v2:
> > > Added review tag.
> >
> > Any further comment ? If no, can we get this patch in queue for 5.11 ?
>
> Can we get this patch in the queue for 5.11 ?

Any further comment on this patch ?

>
> > >
> > >  drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 -
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c 
> > > b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > > index f13af23..0168f98 100644
> > > --- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > > @@ -857,16 +857,17 @@ static void free_private_pages(struct 
> > > hmm_buffer_object *bo,
> > > kfree(bo->page_obj);
> > >  }
> > >
> > > -static void free_user_pages(struct hmm_buffer_object *bo)
> > > +static void free_user_pages(struct hmm_buffer_object *bo,
> > > +   unsigned int page_nr)
> > >  {
> > > int i;
> > >
> > > hmm_mem_stat.usr_size -= bo->pgnr;
> > >
> > > if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
> > > -   unpin_user_pages(bo->pages, bo->pgnr);
> > > +   unpin_user_pages(bo->pages, page_nr);
> > > } else {
> > > -   for (i = 0; i < bo->pgnr; i++)
> > > +   for (i = 0; i < page_nr; i++)
> > > put_page(bo->pages[i]);
> > > }
> > > kfree(bo->pages);
> > > @@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object 
> > > *bo,
> > > dev_err(atomisp_dev,
> > > "get_user_pages err: bo->pgnr = %d, pgnr actually 
> > > pinned = %d.\n",
> > > bo->pgnr, page_nr);
> > > +   if (page_nr < 0)
> > > +   page_nr = 0;
> > > goto out_of_mem;
> > > }
> > >
> > > @@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object 
> > > *bo,
> > >
> > >  out_of_mem:
> > >
> > > -   free_user_pages(bo);
> > > +   free_user_pages(bo, page_nr);
> > >
> > > return -ENOMEM;
> > >  }
> > > @@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
> > > if (bo->type == HMM_BO_PRIVATE)
> > > free_private_pages(bo, _pool, _pool);
> > > else if (bo->type == HMM_BO_USER)
> > > -   free_user_pages(bo);
> > > +   free_user_pages(bo, bo->pgnr);
> > > else
> > > dev_err(atomisp_dev, "invalid buffer type.\n");
> > > mutex_unlock(>mutex);
> > > --
> > > 1.9.1
> > >

Re: [PATCH] input: ariel-pwrbutton.c: Remove unused variable ariel_pwrbutton_id_table[]

[Souptick Joarder]

On Tue, Dec 22, 2020 at 1:34 AM Souptick Joarder  wrote:
>
> Kernel test robot throws below warning ->
>
> >> drivers/input/misc/ariel-pwrbutton.c:152:35: warning: unused variable
> >> 'ariel_pwrbutton_id_table' [-Wunused-const-variable]
>static const struct spi_device_id ariel_pwrbutton_id_table[] = {
>  ^
>1 warning generated.
>
> Remove unused variable ariel_pwrbutton_id_table[] if no plan to use
> it further.
>
> Reported-by: kernel test robot 
> Signed-off-by: Souptick Joarder 

Any comment on this patch ?

> ---
>  drivers/input/misc/ariel-pwrbutton.c | 6 --
>  1 file changed, 6 deletions(-)
>
> diff --git a/drivers/input/misc/ariel-pwrbutton.c 
> b/drivers/input/misc/ariel-pwrbutton.c
> index eda86ab..17bbaac 100644
> --- a/drivers/input/misc/ariel-pwrbutton.c
> +++ b/drivers/input/misc/ariel-pwrbutton.c
> @@ -149,12 +149,6 @@ static int ariel_pwrbutton_probe(struct spi_device *spi)
>  };
>  MODULE_DEVICE_TABLE(of, ariel_pwrbutton_of_match);
>
> -static const struct spi_device_id ariel_pwrbutton_id_table[] = {
> -   { "wyse-ariel-ec-input", 0 },
> -   {}
> -};
> -MODULE_DEVICE_TABLE(spi, ariel_pwrbutton_id_table);
> -
>  static struct spi_driver ariel_pwrbutton_driver = {
> .driver = {
> .name = "dell-wyse-ariel-ec-input",
> --
> 1.9.1
>

[PATCH v2] mm: add prototype for __add_to_page_cache_locked()

[Souptick Joarder]

Otherwise it causes a gcc warning:

../mm/filemap.c:830:14: warning: no previous prototype for
`__add_to_page_cache_locked' [-Wmissing-prototypes]

A previous attempt to make this function static led to compilation
errors when CONFIG_DEBUG_INFO_BTF is enabled because
__add_to_page_cache_locked() is referred to by BPF code.

Adding a prototype will silence the warning.

Signed-off-by: Souptick Joarder 
Cc: Alex Shi 
Cc: Andrew Morton 
Cc: Matthew Wilcox 
---
v2:
Updated change logs and code comments as per review.
s/offset/index.

 include/linux/mm.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5299b90a..c1e9081 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -216,6 +216,13 @@ int overcommit_kbytes_handler(struct ctl_table *, int, 
void *, size_t *,
loff_t *);
 int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
loff_t *);
+/*
+ * Any attempt to mark this function as static leads to build failure
+ * when CONFIG_DEBUG_INFO_BTF is enabled because __add_to_page_cache_locked()
+ * is referred to by BPF code. This must be visible for error injection.
+ */
+int __add_to_page_cache_locked(struct page *page, struct address_space 
*mapping,
+   pgoff_t index, gfp_t gfp, void **shadowp);
 
 #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
 
-- 
1.9.1

Re: [PATCH] mm: add prototype for __add_to_page_cache_locked()

[Souptick Joarder]

On Wed, Dec 23, 2020 at 8:46 AM Souptick Joarder  wrote:
>
> Otherwise it causes a gcc warning:
>
> ../mm/filemap.c:830:14: warning: no previous prototype for
> `__add_to_page_cache_locked' [-Wmissing-prototypes]
>
> A previous attempt to make this function static led to compilation
> errors when CONFIG_DEBUG_INFO_BTF is enabled because
> __add_to_page_cache_locked() is referred to by BPF code.
>
> Adding a prototype will silence the warning.
>

Please ignore this patch. I forget to update version.

> Signed-off-by: Souptick Joarder 
> Cc: Alex Shi 
> Cc: Andrew Morton 
> Cc: Matthew Wilcox 
> ---
>  include/linux/mm.h | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 5299b90a..c1e9081 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -216,6 +216,13 @@ int overcommit_kbytes_handler(struct ctl_table *, int, 
> void *, size_t *,
> loff_t *);
>  int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
> loff_t *);
> +/*
> + * Any attempt to mark this function as static leads to build failure
> + * when CONFIG_DEBUG_INFO_BTF is enabled because __add_to_page_cache_locked()
> + * is referred to by BPF code. This must be visible for error injection.
> + */
> +int __add_to_page_cache_locked(struct page *page, struct address_space 
> *mapping,
> +   pgoff_t index, gfp_t gfp, void **shadowp);
>
>  #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
>
> --
> 1.9.1
>

[PATCH] mm: add prototype for __add_to_page_cache_locked()

[Souptick Joarder]

Otherwise it causes a gcc warning:

../mm/filemap.c:830:14: warning: no previous prototype for
`__add_to_page_cache_locked' [-Wmissing-prototypes]

A previous attempt to make this function static led to compilation
errors when CONFIG_DEBUG_INFO_BTF is enabled because
__add_to_page_cache_locked() is referred to by BPF code.

Adding a prototype will silence the warning.

Signed-off-by: Souptick Joarder 
Cc: Alex Shi 
Cc: Andrew Morton 
Cc: Matthew Wilcox 
---
 include/linux/mm.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5299b90a..c1e9081 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -216,6 +216,13 @@ int overcommit_kbytes_handler(struct ctl_table *, int, 
void *, size_t *,
loff_t *);
 int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
loff_t *);
+/*
+ * Any attempt to mark this function as static leads to build failure
+ * when CONFIG_DEBUG_INFO_BTF is enabled because __add_to_page_cache_locked()
+ * is referred to by BPF code. This must be visible for error injection.
+ */
+int __add_to_page_cache_locked(struct page *page, struct address_space 
*mapping,
+   pgoff_t index, gfp_t gfp, void **shadowp);
 
 #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
 
-- 
1.9.1

Re: [PATCH 1/1] mm: mmap: Remove unnecessary local variable

[Souptick Joarder]

On Tue, Dec 22, 2020 at 4:03 PM Adrian Huang  wrote:
>
> From: Adrian Huang 
>
> The local variable 'retval' is assigned just for once in __do_sys_brk(),
> and the function returns the value of the local variable right after
> the assignment. Remove unnecessary assignment and local variable
> declaration.
>
> Signed-off-by: Adrian Huang 

Acked-by: Souptick Joarder 

> ---
>  mm/mmap.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index dc7206032387..482c0c0bbe06 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -189,7 +189,6 @@ static int do_brk_flags(unsigned long addr, unsigned long 
> request, unsigned long
> struct list_head *uf);
>  SYSCALL_DEFINE1(brk, unsigned long, brk)
>  {
> -   unsigned long retval;
> unsigned long newbrk, oldbrk, origbrk;
> struct mm_struct *mm = current->mm;
> struct vm_area_struct *next;
> @@ -281,9 +280,8 @@ SYSCALL_DEFINE1(brk, unsigned long, brk)
> return brk;
>
>  out:
> -   retval = origbrk;
> mmap_write_unlock(mm);
> -   return retval;
> +   return origbrk;
>  }
>
>  static inline unsigned long vma_compute_gap(struct vm_area_struct *vma)
> --
> 2.17.1
>
>

Re: [PATCH 1/1] mm:improve the performance during fork

[Souptick Joarder]

On Tue, Dec 22, 2020 at 5:49 PM  wrote:
>
> From: jun qian 
>
> In our project, Many business delays come from fork, so
> we started looking for the reason why fork is time-consuming.
> I used the ftrace with function_graph to trace the fork, found
> that the vm_normal_page will be called tens of thousands and
> the execution time of this vm_normal_page function is only a
> few nanoseconds. And the vm_normal_page is not a inline function.
> So I think if the function is inline style, it maybe reduce the
> call time overhead.
>
> I did the following experiment:
>
> I have wrote the c test code, pls ignore the memory leak :)
> Before fork, I will malloc 4G bytes, then acculate the fork
> time.
>
> int main()
> {
> char *p;
> unsigned long long i=0;
> float time_use=0;
> struct timeval start;
> struct timeval end;
>
> for(i=0; i p = (char *)malloc(4096);
> if (p == NULL) {
> printf("malloc failed!\n");
> return 0;
> }
> p[0] = 0x55;
> }
> gettimeofday(,NULL);
> fork();
> gettimeofday(,NULL);
>
> time_use=(end.tv_sec * 100 + end.tv_usec) -
> (start.tv_sec * 100 + start.tv_usec);
> printf("time_use is %.10f us\n",time_use);
>
> return 0;
> }
>
> We need to compare the changes in the size of vmlinux, the time of
> fork in inline and non-inline cases, and the vm_normal_page will be
> called in many function. So we also need to compare this function's
> size. For examples, the do_wp_page will call vm_normal_page, so I
> also calculated it's size.
>
>   inline   non-inline   diff
> vmlinux size  9709248 bytes9709824 bytes-576 bytes
> fork time 23475ns  24638ns  -4.7%

Do you have time diff for both parent and child process ?

> do_wp_page size   972  743  +229
>
> According to the above test data, I think inline vm_normal_page can
> reduce fork execution time.
>
> Signed-off-by: jun qian 
> ---
>  mm/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 7d608765932b..a689bb5d3842 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -591,7 +591,7 @@ static void print_bad_pte(struct vm_area_struct *vma, 
> unsigned long addr,
>   * PFNMAP mappings in order to support COWable mappings.
>   *
>   */
> -struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
> +inline struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long 
> addr,
> pte_t pte)
>  {
> unsigned long pfn = pte_pfn(pte);
> --
> 2.18.2
>
>

[PATCH] mm: add prototype for __add_to_page_cache_locked()

[Souptick Joarder]

Otherwise it cause gcc warning:
  ^~~
../mm/filemap.c:830:14: warning: no previous prototype for
‘__add_to_page_cache_locked’ [-Wmissing-prototypes]
 noinline int __add_to_page_cache_locked(struct page *page,
  ^~

A previous attempt to make this function static leads to
compile error for few architectures.

Adding a prototype will silence the warning.

Signed-off-by: Souptick Joarder 
Cc: Alex Shi 
Cc: Andrew Morton 
---
 include/linux/mm.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5299b90a..ac07f65 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -216,6 +216,12 @@ int overcommit_kbytes_handler(struct ctl_table *, int, 
void *, size_t *,
loff_t *);
 int overcommit_policy_handler(struct ctl_table *, int, void *, size_t *,
loff_t *);
+/*
+ * Any attempt to mark this function as static leads to build failure
+ * for few architectures. Adding a prototype to silence gcc warning.
+ */
+int __add_to_page_cache_locked(struct page *page, struct address_space 
*mapping,
+   pgoff_t offset, gfp_t gfp, void **shadowp);
 
 #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n))
 
-- 
1.9.1

[PATCH] input: ariel-pwrbutton.c: Remove unused variable ariel_pwrbutton_id_table[]

[Souptick Joarder]

Kernel test robot throws below warning ->

>> drivers/input/misc/ariel-pwrbutton.c:152:35: warning: unused variable
>> 'ariel_pwrbutton_id_table' [-Wunused-const-variable]
   static const struct spi_device_id ariel_pwrbutton_id_table[] = {
 ^
   1 warning generated.

Remove unused variable ariel_pwrbutton_id_table[] if no plan to use
it further.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/input/misc/ariel-pwrbutton.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/drivers/input/misc/ariel-pwrbutton.c 
b/drivers/input/misc/ariel-pwrbutton.c
index eda86ab..17bbaac 100644
--- a/drivers/input/misc/ariel-pwrbutton.c
+++ b/drivers/input/misc/ariel-pwrbutton.c
@@ -149,12 +149,6 @@ static int ariel_pwrbutton_probe(struct spi_device *spi)
 };
 MODULE_DEVICE_TABLE(of, ariel_pwrbutton_of_match);
 
-static const struct spi_device_id ariel_pwrbutton_id_table[] = {
-   { "wyse-ariel-ec-input", 0 },
-   {}
-};
-MODULE_DEVICE_TABLE(spi, ariel_pwrbutton_id_table);
-
 static struct spi_driver ariel_pwrbutton_driver = {
.driver = {
.name = "dell-wyse-ariel-ec-input",
-- 
1.9.1

Re: [PATCH] mm/filemap: Fix warning: no previous prototype

[Souptick Joarder]

On Fri, Dec 18, 2020 at 11:27 PM Andrew Morton
 wrote:
>
> On Fri, 18 Dec 2020 09:39:30 +0530 Souptick Joarder  
> wrote:
>
> > On Thu, Dec 17, 2020 at 7:53 AM Matthew Wilcox  wrote:
> > >
> > > On Thu, Dec 17, 2020 at 10:03:11AM +0800, Xiangyang Yu wrote:
> > > > Fixed the warning when building with warnings enabled (W=1),
> > > > This function is only used in filemap.c, so mark this function
> > > > with 'static'.
> > >
> > > Good grief, no.  Look at the git history before proposing a patch.
> >
> > revert "mm/filemap: add static for function __add_to_page_cache_locked"
> > Revert commit 3351b16af494 ("mm/filemap: add static for function
> > __add_to_page_cache_locked") due to incompatibility with
> > ALLOW_ERROR_INJECTION which result in build errors.
> >
>
> How about we add a prototype for __add_to_page_cache_locked() to squash
> the warning, along with a comment explaining what's going on?
>
I think adding a prototype will silence some kernel test robot warning
and future efforts to make it static. I will post a patch.

Re: [PATCH] mm/filemap: Fix warning: no previous prototype

[Souptick Joarder]

On Thu, Dec 17, 2020 at 7:53 AM Matthew Wilcox  wrote:
>
> On Thu, Dec 17, 2020 at 10:03:11AM +0800, Xiangyang Yu wrote:
> > Fixed the warning when building with warnings enabled (W=1),
> > This function is only used in filemap.c, so mark this function
> > with 'static'.
>
> Good grief, no.  Look at the git history before proposing a patch.

revert "mm/filemap: add static for function __add_to_page_cache_locked"
Revert commit 3351b16af494 ("mm/filemap: add static for function
__add_to_page_cache_locked") due to incompatibility with
ALLOW_ERROR_INJECTION which result in build errors.

>

Re: [PATCH] mm/memory_hotplug: quieting offline operation

[Souptick Joarder]

On Fri, Dec 11, 2020 at 8:32 PM Laurent Dufour  wrote:
>
> On PowerPC, when dymically removing memory from a system we can see in the 
> console a
> lot of messages like this:
> [  186.575389] Offlined Pages 4096

Is it specific to PowerPC ?

>
> This message is displayed on each LMB (256MB) removed, which means that we
> removing 1TB of memory, this message is displayed 4096 times.
>
> Moving it to DEBUG to not flood the console.
>
> Signed-off-by: Laurent Dufour 
> ---
>  mm/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index b44d4c7ba73b..c47a53a16782 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1587,7 +1587,7 @@ int __ref offline_pages(unsigned long start_pfn, 
> unsigned long nr_pages)
>
> /* Mark all sections offline and remove free pages from the buddy. */
> __offline_isolated_pages(start_pfn, end_pfn);
> -   pr_info("Offlined Pages %ld\n", nr_pages);
> +   pr_debug("Offlined Pages %ld\n", nr_pages);
>
> /*
>  * The memory sections are marked offline, and the pageblock flags
> --
> 2.29.2
>
>

[PATCH linux-next] drm/amd/display: Adding prototype for dccg21_update_dpp_dto()

[Souptick Joarder]

Kernel test robot throws below warning ->

drivers/gpu/drm/amd/amdgpu/../display/dc/dcn21/dcn21_dccg.c:46:6:
warning: no previous prototype for 'dccg21_update_dpp_dto'
[-Wmissing-prototypes]

Adding prototype for dccg21_update_dpp_dto().

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/gpu/drm/amd/display/dc/dcn21/dcn21_dccg.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_dccg.h 
b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_dccg.h
index b7efa77..e44a374 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_dccg.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_dccg.h
@@ -32,5 +32,6 @@ struct dccg *dccg21_create(
const struct dccg_shift *dccg_shift,
const struct dccg_mask *dccg_mask);
 
+void dccg21_update_dpp_dto(struct dccg *dccg, int dpp_inst, int req_dppclk);
 
 #endif /* __DCN21_DCCG_H__ */
-- 
1.9.1

[PATCH] drm/amd/display: Fixed kernel test robot warning

[Souptick Joarder]

Kernel test robot throws below warning ->

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5349:5:
warning: no previous prototype for 'amdgpu_dm_crtc_atomic_set_property'
[-Wmissing-prototypes]
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5349:5:
warning: no previous prototype for function
'amdgpu_dm_crtc_atomic_set_property' [-Wmissing-prototypes]
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5373:5:
warning: no previous prototype for 'amdgpu_dm_crtc_atomic_get_property'
[-Wmissing-prototypes]
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:5373:5:
warning: no previous prototype for function
'amdgpu_dm_crtc_atomic_get_property' [-Wmissing-prototypes]

As these functions are only used inside amdgpu_dm.c, these can be
made static.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 313501c..e6d069d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5328,7 +5328,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc)
 }
 
 #ifdef CONFIG_DEBUG_FS
-int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc,
+static int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc *crtc,
struct drm_crtc_state *crtc_state,
struct drm_property *property,
uint64_t val)
@@ -5352,7 +5352,7 @@ int amdgpu_dm_crtc_atomic_set_property(struct drm_crtc 
*crtc,
return 0;
 }
 
-int amdgpu_dm_crtc_atomic_get_property(struct drm_crtc *crtc,
+static int amdgpu_dm_crtc_atomic_get_property(struct drm_crtc *crtc,
const struct drm_crtc_state *state,
struct drm_property *property,
uint64_t *val)
-- 
1.9.1

Re: [PATCH] mt76: Fixed kernel test robot warning

[Souptick Joarder]

On Thu, Dec 10, 2020 at 12:46 PM Kalle Valo  wrote:
>
> Souptick Joarder  writes:
>
> > Kernel test robot throws below warning ->
> >
> >drivers/net/wireless/mediatek/mt76/tx.c: In function
> > 'mt76_txq_schedule':
> >>> drivers/net/wireless/mediatek/mt76/tx.c:499:21: warning: variable 'q'
> >>> set but not used [-Wunused-but-set-variable]
> >  499 |  struct mt76_queue *q;
> >  | ^
> >
> > This patch will silence this warning.
> >
> > Reported-by: kernel test robot 
> > Signed-off-by: Souptick Joarder 
>
> I would like to take this directly to wireless-drivers-next, ok?

Ok.
>
> I'll also change the title to:
>
> mt76: remove unused variable q
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH] usb: cdns3: Fixed kernel test robot warning

[Souptick Joarder]

Kernel test robot throws below warning ->

In file included from drivers/usb/cdns3/core.c:23:
>> drivers/usb/cdns3/host-export.h:27:51: warning: 'struct usb_hcd'
>> declared inside parameter list will not be visible outside of this
>> definition or declaration
  27 | static inline int xhci_cdns3_suspend_quirk(struct usb_hcd
*hcd)
 |   ^~~

This patch will silence it.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/usb/cdns3/host-export.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/cdns3/host-export.h b/drivers/usb/cdns3/host-export.h
index fb8541b..c1259af 100644
--- a/drivers/usb/cdns3/host-export.h
+++ b/drivers/usb/cdns3/host-export.h
@@ -24,7 +24,7 @@ static inline int cdns_host_init(struct cdns *cdns)
 }
 
 static inline void cdns_host_exit(struct cdns *cdns) { }
-static inline int xhci_cdns3_suspend_quirk(struct usb_hcd *hcd)
+static int xhci_cdns3_suspend_quirk(struct usb_hcd *hcd)
 {
return 0;
 }
-- 
1.9.1

[PATCH] mt76: Fixed kernel test robot warning

[Souptick Joarder]

Kernel test robot throws below warning ->

   drivers/net/wireless/mediatek/mt76/tx.c: In function
'mt76_txq_schedule':
>> drivers/net/wireless/mediatek/mt76/tx.c:499:21: warning: variable 'q'
>> set but not used [-Wunused-but-set-variable]
 499 |  struct mt76_queue *q;
 | ^

This patch will silence this warning.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 drivers/net/wireless/mediatek/mt76/tx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/tx.c 
b/drivers/net/wireless/mediatek/mt76/tx.c
index 1e20afb..25627e7 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -504,14 +504,11 @@ void mt76_tx_complete_skb(struct mt76_dev *dev, u16 
wcid_idx, struct sk_buff *sk
 
 void mt76_txq_schedule(struct mt76_phy *phy, enum mt76_txq_id qid)
 {
-   struct mt76_queue *q;
int len;
 
if (qid >= 4)
return;
 
-   q = phy->q_tx[qid];
-
rcu_read_lock();
 
do {
-- 
1.9.1

[PATCH] riscv: Fixed kernel test robot warning

[Souptick Joarder]

Kernel test robot throws below warning -

   arch/riscv/kernel/asm-offsets.c:14:6: warning: no previous prototype
for 'asm_offsets' [-Wmissing-prototypes]
  14 | void asm_offsets(void)
 |  ^~~

This patch should fixed it.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 arch/riscv/kernel/asm-offsets.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index db20344..b79ffa3 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -11,6 +11,8 @@
 #include 
 #include 
 
+void asm_offsets(void);
+
 void asm_offsets(void)
 {
OFFSET(TASK_THREAD_RA, task_struct, thread.ra);
-- 
1.9.1

Re: [PATCH] riscv: Fixed kernel test robot warning

[Souptick Joarder]

On Wed, Dec 9, 2020 at 1:21 AM Andreas Schwab  wrote:
>
> On Dez 09 2020, Souptick Joarder wrote:
>
> > Kernel test robot throws below warning -
> >
> >arch/riscv/kernel/asm-offsets.c:14:6: warning: no previous prototype
> > for 'asm_offsets' [-Wmissing-prototypes]
> >   14 | void asm_offsets(void)
> >  |  ^~~
> >
> > This patch should fixed it.
>
> Or rename it to main, like most other asm-offsets files.

Few asm-offsets files named it as foo(). Does a rename to main() will
work straight forward ?
I don't know much about this area of code.

>
> Andreas.
>
> --
> Andreas Schwab, sch...@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."

Re: [PATCH v2] media: atomisp: Fixed error handling path

[Souptick Joarder]

On Thu, Nov 19, 2020 at 1:06 AM Souptick Joarder  wrote:
>
> On Wed, Nov 4, 2020 at 7:32 AM Souptick Joarder  wrote:
> >
> > Inside alloc_user_pages() based on flag value either pin_user_pages()
> > or get_user_pages_fast() will be called. However, these API might fail.
> >
> > But free_user_pages() called in error handling path doesn't bother
> > about return value and will try to unpin bo->pgnr pages, which is
> > incorrect.
> >
> > Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
> > pages will be unpinned based on bo->mem_type. This will also take care
> > of non error handling path.
> >
> > Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> > allocation")
> > Signed-off-by: Souptick Joarder 
> > Reviewed-by: Dan Carpenter 
> > Cc: John Hubbard 
> > Cc: Ira Weiny 
> > Cc: Dan Carpenter 
> > ---
> > v2:
> > Added review tag.
>
> Any further comment ? If no, can we get this patch in queue for 5.11 ?

Can we get this patch in the queue for 5.11 ?

> >
> >  drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 -
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c 
> > b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > index f13af23..0168f98 100644
> > --- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > @@ -857,16 +857,17 @@ static void free_private_pages(struct 
> > hmm_buffer_object *bo,
> > kfree(bo->page_obj);
> >  }
> >
> > -static void free_user_pages(struct hmm_buffer_object *bo)
> > +static void free_user_pages(struct hmm_buffer_object *bo,
> > +   unsigned int page_nr)
> >  {
> > int i;
> >
> > hmm_mem_stat.usr_size -= bo->pgnr;
> >
> > if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
> > -   unpin_user_pages(bo->pages, bo->pgnr);
> > +   unpin_user_pages(bo->pages, page_nr);
> > } else {
> > -   for (i = 0; i < bo->pgnr; i++)
> > +   for (i = 0; i < page_nr; i++)
> > put_page(bo->pages[i]);
> > }
> > kfree(bo->pages);
> > @@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object 
> > *bo,
> > dev_err(atomisp_dev,
> > "get_user_pages err: bo->pgnr = %d, pgnr actually 
> > pinned = %d.\n",
> > bo->pgnr, page_nr);
> > +   if (page_nr < 0)
> > +   page_nr = 0;
> > goto out_of_mem;
> > }
> >
> > @@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object 
> > *bo,
> >
> >  out_of_mem:
> >
> > -   free_user_pages(bo);
> > +   free_user_pages(bo, page_nr);
> >
> > return -ENOMEM;
> >  }
> > @@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
> > if (bo->type == HMM_BO_PRIVATE)
> > free_private_pages(bo, _pool, _pool);
> > else if (bo->type == HMM_BO_USER)
> > -   free_user_pages(bo);
> > +   free_user_pages(bo, bo->pgnr);
> > else
> > dev_err(atomisp_dev, "invalid buffer type.\n");
> > mutex_unlock(>mutex);
> > --
> > 1.9.1
> >

[PATCH linux-next] include/getcpu.h: Fixed kernel test robot warning

[Souptick Joarder]

Kernel test robot generates below warning ->

>> arch/s390/kernel/vdso64/getcpu.c:8:5: warning: no previous prototype
>> for function '__s390_vdso_getcpu' [-Wmissing-prototypes]
   int __s390_vdso_getcpu(unsigned *cpu, unsigned *node, struct
getcpu_cache *unused)
   ^
   arch/s390/kernel/vdso64/getcpu.c:8:1: note: declare 'static' if the
function is not intended to be used outside of this translation unit
   int __s390_vdso_getcpu(unsigned *cpu, unsigned *node, struct
getcpu_cache *unused)
   ^
   static
   1 warning generated.

vim +/__s390_vdso_getcpu +8 arch/s390/kernel/vdso64/getcpu.c

 7
   > 8  int __s390_vdso_getcpu(unsigned *cpu, unsigned *node, struct
   > getcpu_cache *unused)

It is fixed by adding a prototype.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 include/linux/getcpu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/getcpu.h b/include/linux/getcpu.h
index c304dcd..43c9208 100644
--- a/include/linux/getcpu.h
+++ b/include/linux/getcpu.h
@@ -16,4 +16,5 @@ struct getcpu_cache {
unsigned long blob[128 / sizeof(long)];
 };
 
+int __s390_vdso_getcpu(unsigned *cpu, unsigned *node, struct getcpu_cache 
*unused);
 #endif
-- 
1.9.1

[PATCH] nfsd: Fix kernel test robot warning

[Souptick Joarder]

Kernel test robot throws below warning -

>> fs/nfsd/nfs3xdr.c:299:6: warning: variable 'err' is used
>> uninitialized whenever 'if' condition is false
>> [-Wsometimes-uninitialized]
   if (!v4 || !inode->i_sb->s_export_op->fetch_iversion)
   ^~~~
   fs/nfsd/nfs3xdr.c:304:6: note: uninitialized use occurs here
   if (err) {
   ^~~
   fs/nfsd/nfs3xdr.c:299:2: note: remove the 'if' if its condition is
always true
   if (!v4 || !inode->i_sb->s_export_op->fetch_iversion)
   ^
   fs/nfsd/nfs3xdr.c:293:12: note: initialize the variable 'err' to
silence this warning
   __be32 err;
 ^
  = 0
   1 warning generated.

Initialize err = 0 to silence this warning.

Reported-by: kernel test robot 
Signed-off-by: Souptick Joarder 
---
 fs/nfsd/nfs3xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index abb1608..47aeaee 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -290,7 +290,7 @@ void fill_post_wcc(struct svc_fh *fhp)
 {
bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
struct inode *inode = d_inode(fhp->fh_dentry);
-   __be32 err;
+   __be32 err = 0;
 
if (fhp->fh_post_saved)
printk("nfsd: inode locked twice during operation.\n");
-- 
1.9.1

Re: [PATCH] mm: fix some spelling mistakes in comments

[Souptick Joarder]

On Fri, Nov 27, 2020 at 6:50 AM Haitao Shi  wrote:
>
> Fix some spelling mistakes in comments:
> udpate ==> update
> succesful ==> successful
> exmaple ==> example
> unneccessary ==> unnecessary
> stoping ==> stopping
> uknown ==> unknown
>
> Signed-off-by: Haitao Shi 

Reviewed-by: Souptick Joarder 

> ---
>  mm/filemap.c | 2 +-
>  mm/huge_memory.c | 2 +-
>  mm/khugepaged.c  | 2 +-
>  mm/memblock.c| 2 +-
>  mm/migrate.c | 2 +-
>  mm/page_ext.c| 2 +-
>  6 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 3ebbe64..8826c48 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1359,7 +1359,7 @@ static int __wait_on_page_locked_async(struct page 
> *page,
> else
> ret = PageLocked(page);
> /*
> -* If we were succesful now, we know we're still on the
> +* If we were successful now, we know we're still on the
>  * waitqueue as we're still under the lock. This means it's
>  * safe to remove and return success, we know the callback
>  * isn't going to trigger.
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ec2bb93..0fea0c2 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2356,7 +2356,7 @@ static void __split_huge_page_tail(struct page *head, 
> int tail,
>  * Clone page flags before unfreezing refcount.
>  *
>  * After successful get_page_unless_zero() might follow flags change,
> -* for exmaple lock_page() which set PG_waiters.
> +* for example lock_page() which set PG_waiters.
>  */
> page_tail->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
> page_tail->flags |= (head->flags &
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 4e3dff1..d6f7ede 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1273,7 +1273,7 @@ static int khugepaged_scan_pmd(struct mm_struct *mm,
>  * PTEs are armed with uffd write protection.
>  * Here we can also mark the new huge pmd as
>  * write protected if any of the small ones is
> -* marked but that could bring uknown
> +* marked but that could bring unknown
>  * userfault messages that falls outside of
>  * the registered range.  So, just be simple.
>  */
> diff --git a/mm/memblock.c b/mm/memblock.c
> index b68ee86..086662a 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -871,7 +871,7 @@ int __init_memblock memblock_physmem_add(phys_addr_t 
> base, phys_addr_t size)
>   * @base: base address of the region
>   * @size: size of the region
>   * @set: set or clear the flag
> - * @flag: the flag to udpate
> + * @flag: the flag to update
>   *
>   * This function isolates region [@base, @base + @size), and sets/clears flag
>   *
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5795cb8..8a3580c 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2548,7 +2548,7 @@ static bool migrate_vma_check_page(struct page *page)
>  * will bump the page reference count. Sadly there is no way 
> to
>  * differentiate a regular pin from migration wait. Hence to
>  * avoid 2 racing thread trying to migrate back to CPU to 
> enter
> -* infinite loop (one stoping migration because the other is
> +* infinite loop (one stopping migration because the other is
>  * waiting on pte migration entry). We always return true 
> here.
>  *
>  * FIXME proper solution is to rework migration_entry_wait() 
> so
> diff --git a/mm/page_ext.c b/mm/page_ext.c
> index a3616f7..cf931eb 100644
> --- a/mm/page_ext.c
> +++ b/mm/page_ext.c
> @@ -34,7 +34,7 @@
>   *
>   * The need callback is used to decide whether extended memory allocation is
>   * needed or not. Sometimes users want to deactivate some features in this
> - * boot and extra memory would be unneccessary. In this case, to avoid
> + * boot and extra memory would be unnecessary. In this case, to avoid
>   * allocating huge chunk of memory, each clients represent their need of
>   * extra memory through the need callback. If one of the need callbacks
>   * returns true, it means that someone needs extra memory so that
> --
> 2.10.1
>
>

[PATCH] remoteproc/mediatek: Fix kernel test robot warning

[Souptick Joarder]

Kernel test robot throws below warning ->

>> drivers/remoteproc/mtk_scp.c:755:37: warning: unused variable
>> 'mt8183_of_data' [-Wunused-const-variable]
   static const struct mtk_scp_of_data mt8183_of_data = {
   ^
>> drivers/remoteproc/mtk_scp.c:765:37: warning: unused variable
>> 'mt8192_of_data' [-Wunused-const-variable]
   static const struct mtk_scp_of_data mt8192_of_data = {
   ^
As suggested by Bjorn, there's no harm in just dropping the
of_match_ptr() wrapping of mtk_scp_of_match in the definition of
mtk_scp_driver and we avoid this whole problem.

Reported-by: kernel test robot 
Suggested-by: Bjorn Andersson 
Signed-off-by: Souptick Joarder 
---
 drivers/remoteproc/mtk_scp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index 7e0f1e1..5f42b9c 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -772,21 +772,19 @@ static int scp_remove(struct platform_device *pdev)
.host_to_scp_int_bit = MT8192_HOST_IPC_INT_BIT,
 };
 
-#if defined(CONFIG_OF)
 static const struct of_device_id mtk_scp_of_match[] = {
{ .compatible = "mediatek,mt8183-scp", .data = _of_data },
{ .compatible = "mediatek,mt8192-scp", .data = _of_data },
{},
 };
 MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
-#endif
 
 static struct platform_driver mtk_scp_driver = {
.probe = scp_probe,
.remove = scp_remove,
.driver = {
.name = "mtk-scp",
-   .of_match_table = of_match_ptr(mtk_scp_of_match),
+   .of_match_table = mtk_scp_of_match,
},
 };
 
-- 
1.9.1

Re: [PATCH] mm/page_owner: Record timestamp and pid

[Souptick Joarder]

On Sat, Nov 28, 2020 at 12:36 AM Vlastimil Babka  wrote:
>
> On 11/27/20 7:57 PM, Georgi Djakov wrote:
> > Hi Vlastimil,
> >
> > Thanks for the comment!
> >
> > On 11/27/20 19:52, Vlastimil Babka wrote:
> >> On 11/12/20 8:14 PM, Andrew Morton wrote:
> >>> On Thu, 12 Nov 2020 20:41:06 +0200 Georgi Djakov 
> >>> 
> >>> wrote:
> >>>
>  From: Liam Mark 
> 
>  Collect the time for each allocation recorded in page owner so that
>  allocation "surges" can be measured.
> 
>  Record the pid for each allocation recorded in page owner so that
>  the source of allocation "surges" can be better identified.
> >>>
> >>> Please provide a description of why this is considered useful.  What
> >>> has it been used for, what problems has it been used to solve?
> >>
> >> Worth noting that on x86_64 it doubles the size of struct page_owner
> >> from 16 bytes to 32, so it better be justified:
> >
> > Well, that's true. But for debug options there is almost always some 
> > penalty.
> > The timestamp and pid information is very useful for me (and others, i 
> > believe)
> > when doing memory analysis. On a crash for example, we can get this 
> > information
> > from kdump (or RAM-dump) and look into it to catch memory allocation 
> > problems
> > more easily.
>
> Right. Btw, you should add printing the info to __dump_page_owner().
>
> > If you find the above argument not strong enough, how about a separate 
> > config
> > option for this? Maybe something like CONFIG_PAGE_OWNER_EXTENDED, which 
> > could
> > be enabled in addition to CONFIG_PAGE_OWNER?
>
> It might be strong enough if it's mentioned in changelog, and also what 
> exactly
> the space tradeoff is :)

Just a thought ... putting it inside CONFIG_PAGE_OWNER_DEBUG might be
better if it is used
purely for debugging purposes.

>
> You can also mention that SLUB object tracking has also pid+timestamp.
>
> > Thanks,
> > Georgi
> >
> >>
> >> struct page_owner {
> >>  short unsigned int order;/* 0 2 */
> >>  short int  last_migrate_reason;  /* 2 2 */
> >>  gfp_t  gfp_mask; /* 4 4 */
> >>  depot_stack_handle_t   handle;   /* 8 4 */
> >>  depot_stack_handle_t   free_handle;  /*12 4 */
> >>  u64ts_nsec;  /*16 8 */
> >>  intpid;  /*24 4 */
> >>
> >>  /* size: 32, cachelines: 1, members: 7 */
> >>  /* padding: 4 */
> >>  /* last cacheline: 32 bytes */
> >> };
> >>
> >
>
>

Re: arch/powerpc/platforms/pseries/reconfig.c:394:30: error: 'ofdt_proc_ops' defined but not used

[Souptick Joarder]

On Tue, Nov 24, 2020 at 12:40 PM kernel test robot  wrote:
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
> master
> head:   d5beb3140f91b1c8a3d41b14d729aefa4dcc58bc
> commit: 97a32539b9568bb653683349e5a76d02ff3c3e2c proc: convert everything to 
> "struct proc_ops"
> date:   10 months ago
> config: powerpc-randconfig-r002-20201124 (attached as .config)
> compiler: powerpc64le-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
> wget 
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=97a32539b9568bb653683349e5a76d02ff3c3e2c
> git remote add linus 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> git fetch --no-tags linus master
> git checkout 97a32539b9568bb653683349e5a76d02ff3c3e2c
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross 
> ARCH=powerpc
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot 
>
> All errors (new ones prefixed by >>):
>
> >> arch/powerpc/platforms/pseries/reconfig.c:394:30: error: 'ofdt_proc_ops' 
> >> defined but not used [-Werror=unused-const-variable=]
>  394 | static const struct proc_ops ofdt_proc_ops = {
>  |  ^
>cc1: all warnings being treated as errors
> --
> >> arch/powerpc/platforms/pseries/lparcfg.c:701:30: error: 'lparcfg_proc_ops' 
> >> defined but not used [-Werror=unused-const-variable=]
>  701 | static const struct proc_ops lparcfg_proc_ops = {
>  |  ^~~~
>cc1: all warnings being treated as errors

Both ofdt_proc_ops & lparcfg_proc_ops are used by proc_create().
Not sure why it is throwing warnings.

>
> vim +/ofdt_proc_ops +394 arch/powerpc/platforms/pseries/reconfig.c
>
>393
>  > 394  static const struct proc_ops ofdt_proc_ops = {
>395  .proc_write = ofdt_write,
>396  .proc_lseek = noop_llseek,
>397  };
>398
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org

Re: [PATCH] mm: cma: improve pr_debug log in cma_release()

[Souptick Joarder]

On Wed, Nov 25, 2020 at 9:02 PM Charan Teja Reddy
 wrote:
>
> It is required to print 'count' of pages, along with the pages, passed
> to cma_release to debug the cases of mismatched count value passed
> between cma_alloc() and cma_release() from a code path.
>
> As an example, consider the below scenario:
> 1) CMA pool size is 4MB and
> 2) User doing the erroneous step of allocating 2 pages but freeing 1
> page in a loop from this CMA pool.
> The step 2 causes cma_alloc() to return NULL at one point of time
> because of -ENOMEM condition.
>
> And the current pr_debug logs is not giving the info about these types
> of allocation patterns because of count value not being printed in
> cma_release().
>
> We are printing the count value in the trace logs, just extend the same
> to pr_debug logs too.
>
> Signed-off-by: Charan Teja Reddy 

Reviewed-by: Souptick Joarder 

> ---
>  mm/cma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/cma.c b/mm/cma.c
> index 7f415d7..07c904b 100644
> --- a/mm/cma.c
> +++ b/mm/cma.c
> @@ -512,7 +512,7 @@ bool cma_release(struct cma *cma, const struct page 
> *pages, unsigned int count)
> if (!cma || !pages)
> return false;
>
> -   pr_debug("%s(page %p)\n", __func__, (void *)pages);
> +   pr_debug("%s(page %p, count %zu)\n", __func__, (void *)pages, count);
>
> pfn = page_to_pfn(pages);
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by The Linux Foundation
>
>

Re: [PATCH v2] media: atomisp: Fixed error handling path

[Souptick Joarder]

On Wed, Nov 4, 2020 at 7:32 AM Souptick Joarder  wrote:
>
> Inside alloc_user_pages() based on flag value either pin_user_pages()
> or get_user_pages_fast() will be called. However, these API might fail.
>
> But free_user_pages() called in error handling path doesn't bother
> about return value and will try to unpin bo->pgnr pages, which is
> incorrect.
>
> Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
> pages will be unpinned based on bo->mem_type. This will also take care
> of non error handling path.
>
> Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> allocation")
> Signed-off-by: Souptick Joarder 
> Reviewed-by: Dan Carpenter 
> Cc: John Hubbard 
> Cc: Ira Weiny 
> Cc: Dan Carpenter 
> ---
> v2:
> Added review tag.

Any further comment ? If no, can we get this patch in queue for 5.11 ?
>
>  drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c 
> b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> index f13af23..0168f98 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> @@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object 
> *bo,
> kfree(bo->page_obj);
>  }
>
> -static void free_user_pages(struct hmm_buffer_object *bo)
> +static void free_user_pages(struct hmm_buffer_object *bo,
> +   unsigned int page_nr)
>  {
> int i;
>
> hmm_mem_stat.usr_size -= bo->pgnr;
>
> if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
> -   unpin_user_pages(bo->pages, bo->pgnr);
> +   unpin_user_pages(bo->pages, page_nr);
> } else {
> -   for (i = 0; i < bo->pgnr; i++)
> +   for (i = 0; i < page_nr; i++)
> put_page(bo->pages[i]);
> }
> kfree(bo->pages);
> @@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
> dev_err(atomisp_dev,
> "get_user_pages err: bo->pgnr = %d, pgnr actually 
> pinned = %d.\n",
> bo->pgnr, page_nr);
> +   if (page_nr < 0)
> +   page_nr = 0;
> goto out_of_mem;
> }
>
> @@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
>
>  out_of_mem:
>
> -   free_user_pages(bo);
> +   free_user_pages(bo, page_nr);
>
> return -ENOMEM;
>  }
> @@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
> if (bo->type == HMM_BO_PRIVATE)
> free_private_pages(bo, _pool, _pool);
> else if (bo->type == HMM_BO_USER)
> -   free_user_pages(bo);
> +   free_user_pages(bo, bo->pgnr);
> else
> dev_err(atomisp_dev, "invalid buffer type.\n");
> mutex_unlock(>mutex);
> --
> 1.9.1
>

Re: [PATCH] mm/filemap: add static for function __add_to_page_cache_locked

[Souptick Joarder]

On Fri, Nov 6, 2020 at 4:55 PM Alex Shi  wrote:
>
> Otherwise it cause gcc warning:
>   ^~~
> ../mm/filemap.c:830:14: warning: no previous prototype for
> ‘__add_to_page_cache_locked’ [-Wmissing-prototypes]
>  noinline int __add_to_page_cache_locked(struct page *page,
>   ^~

Is CONFIG_DEBUG_INFO_BTF enabled in your .config ?

>
> Signed-off-by: Alex Shi 
> Cc: Andrew Morton 
> Cc: linux...@kvack.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  mm/filemap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index d90614f501da..249cf489f5df 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -827,7 +827,7 @@ int replace_page_cache_page(struct page *old, struct page 
> *new, gfp_t gfp_mask)
>  }
>  EXPORT_SYMBOL_GPL(replace_page_cache_page);
>
> -noinline int __add_to_page_cache_locked(struct page *page,
> +static noinline int __add_to_page_cache_locked(struct page *page,
> struct address_space *mapping,
> pgoff_t offset, gfp_t gfp,
> void **shadowp)
> --
> 1.8.3.1
>
>

Re: [PATCH 2/2] tomoyo: Fixed typo in documentation

[Souptick Joarder]

On Sat, Nov 7, 2020 at 2:27 PM John Hubbard  wrote:
>
> On 11/7/20 12:24 AM, Souptick Joarder wrote:
> > Fixed typo s/Poiner/Pointer
> >
> > Fixes: 5b636857fee6 ("TOMOYO: Allow using argv[]/envp[] of execve() as 
> > conditions.")
> > Signed-off-by: Souptick Joarder 
> > Cc: John Hubbard 
> > ---
> >   security/tomoyo/domain.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
> > index bd748be..7b2babe 100644
> > --- a/security/tomoyo/domain.c
> > +++ b/security/tomoyo/domain.c
> > @@ -891,7 +891,7 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
> >*
> >* @bprm: Pointer to "struct linux_binprm".
> >* @pos:  Location to dump.
> > - * @dump: Poiner to "struct tomoyo_page_dump".
> > + * @dump: Pointer to "struct tomoyo_page_dump".
>
> Not worth a separate patch, especially since the original comment is merely
> copying the C sources, and as such, does not add any value.
>
> I'd either a) craft a new documentation line that adds some value, or b) just
> merge this patch into the previous one, and make a note in the commit
> description to the effect that you've included a trivial typo fix as long
> as you're there.
>

John, as patch[1/2] is dropped, can we take this patch forward with some more
updates in documentations ?

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

On Sun, Nov 8, 2020 at 10:30 AM John Hubbard  wrote:
>
> On 11/7/20 8:12 PM, Tetsuo Handa wrote:
> > On 2020/11/08 11:17, John Hubbard wrote:
> >>> Excuse me, but Documentation/core-api/pin_user_pages.rst says
> >>> "CASE 5: Pinning in order to _write_ to the data within the page"
> >>> while tomoyo_dump_page() is for "_read_ the data within the page".
> >>> Do we want to convert to pin_user_pages_remote() or lock_page() ?
> >>>
> >>
> >> Sorry, I missed the direction here, was too focused on the Case 5
> >> aspect. Yes. Case 5 (which, again, I think we're about to re-document)
> >> is only about *writing* to data within the page.
> >>
> >> So in this case, where it is just reading from the page, I think it's
> >> already from a gup vs pup point of view.
> >>
> >> btw, it's not clear to me whether the current code is susceptible to any
> >> sort of problem involving something writing to the page while it
> >> is being dumped (I am curious). But changing from gup to pup wouldn't
> >> fix that, if it were a problem. It a separate question from this patch.
> >
> > The "struct page" tomoyo_dump_page() accesses is argv/envp arguments passed
> > to execve() syscall. Therefore, these pages are not visible from threads
> > except current thread, and thus there is no possibility that these pages
> > are modified by other threads while current thread is reading.
> >
>
> Perfect. So since I accidentally left out the word "correct" above (I meant
> to write, "it's already correct"), let me be extra clear: Souptick, we
> should just drop this patch.
>

Agreed. I will drop this patch.

Re: [PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

On Sun, Nov 8, 2020 at 7:47 AM John Hubbard  wrote:
>
> On 11/7/20 5:13 PM, Tetsuo Handa wrote:
> > On 2020/11/08 4:17, John Hubbard wrote:
> >> On 11/7/20 1:04 AM, John Hubbard wrote:
> >>> On 11/7/20 12:24 AM, Souptick Joarder wrote:
> >>>> In 2019, we introduced pin_user_pages*() and now we are converting
> >>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
> >>>> be referred for more information. This is case 5 as per document [1].
> >>>
> >>> It turns out that Case 5 can be implemented via a better pattern, as long
> >>> as we're just dealing with a page at a time, briefly:
> >>>
> >>> lock_page()
> >>> write to page's data
> >>> unlock_page()
> >>>
> >>> ...which neatly synchronizes with writeback and other fs activities.
> >>
> >> Ahem, I left out a key step: set_page_dirty()!
> >>
> >> lock_page()
> >> write to page's data
> >> set_page_dirty()
> >> unlock_page()
> >>
> >
> > Excuse me, but Documentation/core-api/pin_user_pages.rst says
> > "CASE 5: Pinning in order to _write_ to the data within the page"
> > while tomoyo_dump_page() is for "_read_ the data within the page".
> > Do we want to convert to pin_user_pages_remote() or lock_page() ?
> >
>
> Sorry, I missed the direction here, was too focused on the Case 5
> aspect. Yes. Case 5 (which, again, I think we're about to re-document)
> is only about *writing* to data within the page.
>
> So in this case, where it is just reading from the page, I think it's
> already from a gup vs pup point of view.
>
> btw, it's not clear to me whether the current code is susceptible to any
> sort of problem involving something writing to the page while it
> is being dumped (I am curious). But changing from gup to pup wouldn't
> fix that, if it were a problem. It a separate question from this patch.
>
> (Souptick, if you're interested, the Case 5 documentation change and
> callsite retrofit is all yours if you want it. Otherwise it's on
> my list.)

Sure John, I will take it.

[PATCH 2/2] tomoyo: Fixed typo in documentation

[Souptick Joarder]

Fixed typo s/Poiner/Pointer

Fixes: 5b636857fee6 ("TOMOYO: Allow using argv[]/envp[] of execve() as 
conditions.")
Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
---
 security/tomoyo/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index bd748be..7b2babe 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -891,7 +891,7 @@ int tomoyo_find_next_domain(struct linux_binprm *bprm)
  *
  * @bprm: Pointer to "struct linux_binprm".
  * @pos:  Location to dump.
- * @dump: Poiner to "struct tomoyo_page_dump".
+ * @dump: Pointer to "struct tomoyo_page_dump".
  *
  * Returns true on success, false otherwise.
  */
-- 
1.9.1

[PATCH 1/2] tomoyo: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
---
 security/tomoyo/domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index dc4ecc0..bd748be 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -914,7 +914,7 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned 
long pos,
 * (represented by bprm).  'current' is the process doing
 * the execve().
 */
-   if (get_user_pages_remote(bprm->mm, pos, 1,
+   if (pin_user_pages_remote(bprm->mm, pos, 1,
FOLL_FORCE, , NULL, NULL) <= 0)
return false;
 #else
@@ -936,7 +936,7 @@ bool tomoyo_dump_page(struct linux_binprm *bprm, unsigned 
long pos,
}
/* Same with put_arg_page(page) in fs/exec.c */
 #ifdef CONFIG_MMU
-   put_page(page);
+   unpin_user_page(page);
 #endif
return true;
 }
-- 
1.9.1

[PATCH v2] media: atomisp: Fixed error handling path

[Souptick Joarder]

Inside alloc_user_pages() based on flag value either pin_user_pages()
or get_user_pages_fast() will be called. However, these API might fail.

But free_user_pages() called in error handling path doesn't bother
about return value and will try to unpin bo->pgnr pages, which is
incorrect.

Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
pages will be unpinned based on bo->mem_type. This will also take care
of non error handling path.

Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
allocation")
Signed-off-by: Souptick Joarder 
Reviewed-by: Dan Carpenter 
Cc: John Hubbard 
Cc: Ira Weiny 
Cc: Dan Carpenter 
---
v2:
Added review tag.

 drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c 
b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
index f13af23..0168f98 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
@@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object 
*bo,
kfree(bo->page_obj);
 }
 
-static void free_user_pages(struct hmm_buffer_object *bo)
+static void free_user_pages(struct hmm_buffer_object *bo,
+   unsigned int page_nr)
 {
int i;
 
hmm_mem_stat.usr_size -= bo->pgnr;
 
if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
-   unpin_user_pages(bo->pages, bo->pgnr);
+   unpin_user_pages(bo->pages, page_nr);
} else {
-   for (i = 0; i < bo->pgnr; i++)
+   for (i = 0; i < page_nr; i++)
put_page(bo->pages[i]);
}
kfree(bo->pages);
@@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
dev_err(atomisp_dev,
"get_user_pages err: bo->pgnr = %d, pgnr actually 
pinned = %d.\n",
bo->pgnr, page_nr);
+   if (page_nr < 0)
+   page_nr = 0;
goto out_of_mem;
}
 
@@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 
 out_of_mem:
 
-   free_user_pages(bo);
+   free_user_pages(bo, page_nr);
 
return -ENOMEM;
 }
@@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
if (bo->type == HMM_BO_PRIVATE)
free_private_pages(bo, _pool, _pool);
else if (bo->type == HMM_BO_USER)
-   free_user_pages(bo);
+   free_user_pages(bo, bo->pgnr);
else
dev_err(atomisp_dev, "invalid buffer type.\n");
mutex_unlock(>mutex);
-- 
1.9.1

Re: kernel BUG at mm/mmap.c:LINE!

[Souptick Joarder]

On Fri, Sep 4, 2020 at 12:39 AM syzbot
 wrote:
>
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit:1127b219 Merge tag 'fallthrough-fixes-5.9-rc3' of git://gi..
> git tree:   upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=136d098e90
> kernel config:  https://syzkaller.appspot.com/x/.config?x=978db74cb30aa994
> dashboard link: https://syzkaller.appspot.com/bug?extid=721b657f8f01708b291b
> compiler:   gcc (GCC) 10.1.0-syz 20200507
>
> Unfortunately, I don't have any reproducer for this issue yet.
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+721b657f8f01708b2...@syzkaller.appspotmail.com
>
> start_brk 558decf56000 brk 558ded16 start_stack 7ffd20847a70
> arg_start 7ffd20847e89 arg_end 7ffd20847ea4 env_start 7ffd20847ea4 env_end 
> 7ffd20847fdd
> binfmt 89cdcb60 flags 200cd core_state 
> ioctx_table 
> [ cut here ]
> kernel BUG at mm/mmap.c:427!
> invalid opcode:  [#1] PREEMPT SMP KASAN
> CPU: 0 PID: 28867 Comm: systemd-udevd Not tainted 5.9.0-rc2-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS 
> Google 01/01/2011
> RIP: 0010:validate_mm+0x617/0x800 mm/mmap.c:427
> Code: cf f5 cd ff 44 89 e6 bf ff ff ff ff e8 42 f2 cd ff 41 83 fc ff 0f 85 86 
> 1b 01 00 e8 b3 f5 cd ff 48 8b 7c 24 18 e8 d9 59 fc ff <0f> 0b e8 a2 f5 cd ff 
> 48 8b 54 24 28 48 b8 00 00 00 00 00 fc ff df
> RSP: 0018:c90015d6fd98 EFLAGS: 00010286
> RAX: 038d RBX: 004f RCX: 
> RDX: 8880475ac380 RSI: 815dafc7 RDI: f52002badf52
> RBP:  R08: 038d R09: 8880ae6318e7
> R10:  R11: 0001 R12: 0001
> R13:  R14:  R15: 
> FS:  7fb1b935a8c0() GS:8880ae60() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 7ffd20959990 CR3: 000217691000 CR4: 001526f0
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
> Call Trace:
>  remove_vma_list mm/mmap.c:2616 [inline]
>  __do_munmap+0x899/0x1170 mm/mmap.c:2869
>  __vm_munmap+0xe6/0x180 mm/mmap.c:2889
>  __do_sys_munmap mm/mmap.c:2915 [inline]
>  __se_sys_munmap mm/mmap.c:2911 [inline]
>  __x64_sys_munmap+0x62/0x80 mm/mmap.c:2911
>  do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7fb1b81d66e7
> Code: c7 c0 ff ff ff ff eb 8d 48 8b 15 ac 47 2b 00 f7 d8 64 89 02 e9 5b ff ff 
> ff 66 2e 0f 1f 84 00 00 00 00 00 b8 0b 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 
> 01 c3 48 8b 0d 81 47 2b 00 f7 d8 64 89 01 48
> RSP: 002b:7ffd20846cd8 EFLAGS: 0206 ORIG_RAX: 000b
> RAX: ffda RBX: 558decf56100 RCX: 7fb1b81d66e7
> RDX: 0080 RSI: 0080ccec RDI: 7fb1b7064000
> RBP: 558debda4d18 R08: 558decf6a3c0 R09: 
> R10:  R11: 0206 R12: 558decf560e0
> R13:  R14: 0003 R15: 000e
> Modules linked in:
> ---[ end trace 76a00ebdfa09cc52 ]---
> RIP: 0010:validate_mm+0x617/0x800 mm/mmap.c:427
> Code: cf f5 cd ff 44 89 e6 bf ff ff ff ff e8 42 f2 cd ff 41 83 fc ff 0f 85 86 
> 1b 01 00 e8 b3 f5 cd ff 48 8b 7c 24 18 e8 d9 59 fc ff <0f> 0b e8 a2 f5 cd ff 
> 48 8b 54 24 28 48 b8 00 00 00 00 00 fc ff df
> RSP: 0018:c90015d6fd98 EFLAGS: 00010286
> RAX: 038d RBX: 004f RCX: 
> RDX: 8880475ac380 RSI: 815dafc7 RDI: f52002badf52
> RBP:  R08: 038d R09: 8880ae6318e7
> R10:  R11: 0001 R12: 0001
> R13:  R14:  R15: 
> FS:  7fb1b935a8c0() GS:8880ae60() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 7fad09fed000 CR3: 000217691000 CR4: 001526f0
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
>
>

CONFIG_DEBUG_VM_RB=y is set.

[ 1882.597004][T28867] mmap: backwards 1, forwards 3
[ 1882.605532][T28867] mm 88804eeff640 mmap 888094849318
seqnum 1 task_size 140737488351232

Looks like we hit panic due to below code.

static int browse_rb(struct mm_struct *mm) {
...
if (i != j) {
pr_emerg("backwards %d, forwards %d\n", j, i);
bug = 1;
}
...
}

Re: [PATCH 2/2] xen/gntdev.c: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

On Tue, Sep 29, 2020 at 6:00 PM  wrote:
>
>
> On 9/29/20 8:09 AM, Souptick Joarder wrote:
> > On Fri, Sep 11, 2020 at 8:12 PM  wrote:
> >>
> >> On 9/6/20 2:51 AM, Souptick Joarder wrote:
> >>> In 2019, we introduced pin_user_pages*() and now we are converting
> >>> get_user_pages*() to the new API as appropriate. [1] & [2] could
> >>> be referred for more information. This is case 5 as per document [1].
> >>>
> >>> [1] Documentation/core-api/pin_user_pages.rst
> >>>
> >>> [2] "Explicit pinning of user-space pages":
> >>> https://lwn.net/Articles/807108/
> >>>
> >>> Signed-off-by: Souptick Joarder 
> >>> Cc: John Hubbard 
> >>> Cc: Boris Ostrovsky 
> >>> Cc: Juergen Gross 
> >>> Cc: David Vrabel 
> >>
> >> Reviewed-by: Boris Ostrovsky 
> > Are these 2 patches queued for 5.10-rc1 ?
>
>
> Yes, I am preparing the branch. (BTW, your second patch appears to have been 
> either manually edited or not generated on top of the first patch. Please 
> don't do this next time)

I created it on top of the first one and didn't edit manually.
I was able to apply it in my local repository.
What was the error ?

Re: [PATCH 2/2] xen/gntdev.c: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

On Fri, Sep 11, 2020 at 8:12 PM  wrote:
>
>
> On 9/6/20 2:51 AM, Souptick Joarder wrote:
> > In 2019, we introduced pin_user_pages*() and now we are converting
> > get_user_pages*() to the new API as appropriate. [1] & [2] could
> > be referred for more information. This is case 5 as per document [1].
> >
> > [1] Documentation/core-api/pin_user_pages.rst
> >
> > [2] "Explicit pinning of user-space pages":
> >     https://lwn.net/Articles/807108/
> >
> > Signed-off-by: Souptick Joarder 
> > Cc: John Hubbard 
> > Cc: Boris Ostrovsky 
> > Cc: Juergen Gross 
> > Cc: David Vrabel 
>
>
> Reviewed-by: Boris Ostrovsky 

Are these 2 patches queued for 5.10-rc1 ?
>
>

Re: [PATCH] media: atomisp: Fixed error handling path

[Souptick Joarder]

Hi Dan,


On Mon, Sep 28, 2020 at 2:08 PM Dan Carpenter  wrote:
>
> On Sun, Sep 27, 2020 at 08:38:04PM +0530, Souptick Joarder wrote:
> > Inside alloc_user_pages() based on flag value either pin_user_pages()
> > or get_user_pages_fast() will be called. However, these API might fail.
> >
> > But free_user_pages() called in error handling path doesn't bother
> > about return value and will try to unpin bo->pgnr pages, which is
> > incorrect.
> >
> > Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
> > pages will be unpinned based on bo->mem_type. This will also take care
> > of non error handling path.
> >
> > Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
> > allocation")
> > Signed-off-by: Souptick Joarder 
> > Cc: John Hubbard 
> > Cc: Ira Weiny 
> > Cc: Dan Carpenter 
> > ---
> >  drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 -
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c 
> > b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > index f13af23..0168f98 100644
> > --- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
> > @@ -857,16 +857,17 @@ static void free_private_pages(struct 
> > hmm_buffer_object *bo,
> >   kfree(bo->page_obj);
> >  }
> >
> > -static void free_user_pages(struct hmm_buffer_object *bo)
> > +static void free_user_pages(struct hmm_buffer_object *bo,
> > + unsigned int page_nr)
> >  {
> >   int i;
> >
> >   hmm_mem_stat.usr_size -= bo->pgnr;
>   ^^^
> This is still a problem.  It needs to be hmm_mem_stat.usr_size -= page_nr.

In failure path, it is hmm_mem_stat.usr_size += bo->pgnr.
I think, hmm_mem_stat.usr_size -= bo->pgnr is correct as per existing code.
Do you think that needs to be changed ?

>
> regards,
> dan carpenter
>
> >
> >   if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
> > - unpin_user_pages(bo->pages, bo->pgnr);
> > + unpin_user_pages(bo->pages, page_nr);
> >   } else {
> > - for (i = 0; i < bo->pgnr; i++)
> > + for (i = 0; i < page_nr; i++)
> >   put_page(bo->pages[i]);
> >   }
> >   kfree(bo->pages);
>

[PATCH] media: atomisp: Fixed error handling path

[Souptick Joarder]

Inside alloc_user_pages() based on flag value either pin_user_pages()
or get_user_pages_fast() will be called. However, these API might fail.

But free_user_pages() called in error handling path doesn't bother
about return value and will try to unpin bo->pgnr pages, which is
incorrect.

Fix this by passing the page_nr to free_user_pages(). If page_nr > 0
pages will be unpinned based on bo->mem_type. This will also take care
of non error handling path.

Fixes: 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
allocation")
Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Ira Weiny 
Cc: Dan Carpenter 
---
 drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c 
b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
index f13af23..0168f98 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
@@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object 
*bo,
kfree(bo->page_obj);
 }
 
-static void free_user_pages(struct hmm_buffer_object *bo)
+static void free_user_pages(struct hmm_buffer_object *bo,
+   unsigned int page_nr)
 {
int i;
 
hmm_mem_stat.usr_size -= bo->pgnr;
 
if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
-   unpin_user_pages(bo->pages, bo->pgnr);
+   unpin_user_pages(bo->pages, page_nr);
} else {
-   for (i = 0; i < bo->pgnr; i++)
+   for (i = 0; i < page_nr; i++)
put_page(bo->pages[i]);
}
kfree(bo->pages);
@@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
dev_err(atomisp_dev,
"get_user_pages err: bo->pgnr = %d, pgnr actually 
pinned = %d.\n",
bo->pgnr, page_nr);
+   if (page_nr < 0)
+   page_nr = 0;
goto out_of_mem;
}
 
@@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 
 out_of_mem:
 
-   free_user_pages(bo);
+   free_user_pages(bo, page_nr);
 
return -ENOMEM;
 }
@@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
if (bo->type == HMM_BO_PRIVATE)
free_private_pages(bo, _pool, _pool);
else if (bo->type == HMM_BO_USER)
-   free_user_pages(bo);
+   free_user_pages(bo, bo->pgnr);
else
dev_err(atomisp_dev, "invalid buffer type.\n");
mutex_unlock(>mutex);
-- 
1.9.1

Re: [PATCH] mm/gup: protect unpin_user_pages() against npages==-ERRNO

[Souptick Joarder]

On Thu, Sep 17, 2020 at 1:11 PM Dan Carpenter  wrote:
>
> On Wed, Sep 16, 2020 at 11:57:06PM -0700, John Hubbard wrote:
> > As suggested by Dan Carpenter, fortify unpin_user_pages() just a bit,
> > against a typical caller mistake: check if the npages arg is really a
> > -ERRNO value, which would blow up the unpinning loop: WARN and return.
> >
> > If this new WARN_ON() fires, then the system *might* be leaking pages
> > (by leaving them pinned), but probably not. More likely, gup/pup
> > returned a hard -ERRNO error to the caller, who erroneously passed it
> > here.
> >
> > Cc: Ira Weiny 
> > Cc: Souptick Joarder 
> > Signed-off-by: Dan Carpenter 
> > Signed-off-by: John Hubbard 
> > ---
> >
> > Hi Dan,
> >
> > Is is OK to use your signed-off-by here? Since you came up with this.
> >
>
> Yeah.  That's fine.

Do we need a similar check inside unpin_user_pages_dirty_lock(),
when make_dirty set to false ?

[PATCH] misc: mic: scif: Fix error handling path

[Souptick Joarder]

Inside __scif_pin_pages(), when map_flags != SCIF_MAP_KERNEL it
will call pin_user_pages_fast() to map nr_pages. However,
pin_user_pages_fast() might fail with a return value -ERRNO.

The return value is stored in pinned_pages->nr_pages. which in
turn is passed to unpin_user_pages(), which expects
pinned_pages->nr_pages >=0, else disaster.

Fix this by assigning pinned_pages->nr_pages to 0 if
pin_user_pages_fast() returns -ERRNO.

Fixes:  ba612aa8b487 ("misc: mic: SCIF memory registration and
unregistration")
Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Ira Weiny 
Cc: Dan Carpenter 
---
 drivers/misc/mic/scif/scif_rma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/mic/scif/scif_rma.c b/drivers/misc/mic/scif/scif_rma.c
index 2da3b47..18fb9d8 100644
--- a/drivers/misc/mic/scif/scif_rma.c
+++ b/drivers/misc/mic/scif/scif_rma.c
@@ -1392,6 +1392,8 @@ int __scif_pin_pages(void *addr, size_t len, int 
*out_prot,
(prot & SCIF_PROT_WRITE) ? FOLL_WRITE : 0,
pinned_pages->pages);
if (nr_pages != pinned_pages->nr_pages) {
+   if (pinned_pages->nr_pages < 0)
+   pinned_pages->nr_pages = 0;
if (try_upgrade) {
if (ulimit)
__scif_dec_pinned_vm_lock(mm, nr_pages);
@@ -1408,7 +1410,6 @@ int __scif_pin_pages(void *addr, size_t len, int 
*out_prot,
 
if (pinned_pages->nr_pages < nr_pages) {
err = -EFAULT;
-   pinned_pages->nr_pages = nr_pages;
goto dec_pinned;
}
 
@@ -1421,7 +1422,6 @@ int __scif_pin_pages(void *addr, size_t len, int 
*out_prot,
__scif_dec_pinned_vm_lock(mm, nr_pages);
/* Something went wrong! Rollback */
 error_unmap:
-   pinned_pages->nr_pages = nr_pages;
scif_destroy_pinned_pages(pinned_pages);
*pages = NULL;
dev_dbg(scif_info.mdev.this_device,
-- 
1.9.1

Re: [linux-next PATCH] rapidio: Fix error handling path

[Souptick Joarder]

Hi Dan,

On Thu, Sep 17, 2020 at 6:10 PM Dan Carpenter  wrote:
>
> On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> > On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> > > There is an error when pin_user_pages_fast() returns -ERRNO and
> > > inside error handling path driver end up calling unpin_user_pages()
> > > with -ERRNO which is not correct.
> > >
> > > This patch will fix the problem.
> >
> > There are a few ways we could prevent bug in the future.
> >
> > 1) This could have been caught with existing static analysis tools
> >which warn about when a value is set but not used.
> >
> > 2) I've created a Smatch check which warngs about:
> >
> >   drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: 
> > unpinning negative pages 'nr_pages'
> >
> >I'll test it out tonight and see how well it works.  I don't
> >immediately see any other bugs allthough Smatch doesn't like the code
> >in siw_umem_release().  It uses "min_t(int" which suggests that
> >negative pages are okay.
> >
> >  int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> >
>
> I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
> to the mix a retest.  There were a few other false positives.  In
> reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.
>
> I sort of feel like returning partial successes is not working.  We
> could easily make a wrapper which either pins everything or it returns
> an error code.
>
> drivers/misc/mic/scif/scif_rma.c:1399 __scif_pin_pages() warn: unpinning 
> negative pages 'pinned_pages->nr_pages'
>
> drivers/misc/mic/scif/scif_rma.c
>   1355  vmalloc_addr = true;
>   1356
>   1357  for (i = 0; i < nr_pages; i++) {
>   1358  if (vmalloc_addr)
>   1359  pinned_pages->pages[i] =
>   1360  vmalloc_to_page(addr + (i * 
> PAGE_SIZE));
>   1361  else
>   1362  pinned_pages->pages[i] =
>   1363  virt_to_page(addr + (i * 
> PAGE_SIZE));
>   1364  }
>   1365  pinned_pages->nr_pages = nr_pages;
>   1366  pinned_pages->map_flags = SCIF_MAP_KERNEL;
>   1367  } else {
>   1368  /*
>   1369   * SCIF supports registration caching. If a 
> registration has
>   1370   * been requested with read only permissions, then we 
> try
>   1371   * to pin the pages with RW permissions so that a 
> subsequent
>   1372   * transfer with RW permission can hit the cache 
> instead of
>   1373   * invalidating it. If the upgrade fails with RW then 
> we
>   1374   * revert back to R permission and retry
>   1375   */
>   1376  if (prot == SCIF_PROT_READ)
>   1377  try_upgrade = true;
>   1378  prot |= SCIF_PROT_WRITE;
>   1379  retry:
>   1380  mm = current->mm;
>   1381  if (ulimit) {
>   1382  err = __scif_check_inc_pinned_vm(mm, 
> nr_pages);
>   1383  if (err) {
>   1384  pinned_pages->nr_pages = 0;
>   1385  goto error_unmap;
>   1386  }
>   1387  }
>   1388
>   1389  pinned_pages->nr_pages = pin_user_pages_fast(
>   1390  (u64)addr,
>   1391  nr_pages,
>   1392  (prot & SCIF_PROT_WRITE) ? FOLL_WRITE 
> : 0,
>   1393  pinned_pages->pages);
>   1394  if (nr_pages != pinned_pages->nr_pages) {
>   1395  if (try_upgrade) {
>   1396  if (ulimit)
>   1397  __scif_dec_pinned_vm_lock(mm, 
> nr_pages);
>   1398  /* Roll back any pinned pages */
>   1399  unpin_user_pages(pinned_pages->pages,
>   1400   
> pinned_pages->nr_pages);
>  
> ^^
> Negative.
>
>   1401  prot &= 

Re: [linux-next PATCH] rapidio: Fix error handling path

[Souptick Joarder]

On Thu, Sep 17, 2020 at 11:17 PM John Hubbard  wrote:
>
> On 9/17/20 10:34 AM, Ira Weiny wrote:
> > On Thu, Sep 17, 2020 at 03:39:51PM +0300, Dan Carpenter wrote:
> >> On Wed, Sep 16, 2020 at 01:02:32PM +0300, Dan Carpenter wrote:
> >>> On Wed, Sep 16, 2020 at 09:12:17AM +0530, Souptick Joarder wrote:
> >>>> There is an error when pin_user_pages_fast() returns -ERRNO and
> >>>> inside error handling path driver end up calling unpin_user_pages()
> >>>> with -ERRNO which is not correct.
> >>>>
> >>>> This patch will fix the problem.
> >>>
> >>> There are a few ways we could prevent bug in the future.
> >>>
> >>> 1) This could have been caught with existing static analysis tools
> >>> which warn about when a value is set but not used.
> >>>
> >>> 2) I've created a Smatch check which warngs about:
> >>>
> >>> drivers/rapidio/devices/rio_mport_cdev.c:955 rio_dma_transfer() warn: 
> >>> unpinning negative pages 'nr_pages'
> >>>
> >>> I'll test it out tonight and see how well it works.  I don't
> >>> immediately see any other bugs allthough Smatch doesn't like the code
> >>> in siw_umem_release().  It uses "min_t(int" which suggests that
> >>> negative pages are okay.
> >>>
> >>>int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
> >>>
> >>
> >> I only found one bug but I'm going to add unpin_user_pages_dirty_lock()
> >> to the mix a retest.  There were a few other false positives.  In
> >> reviewing the code, I noticed that orangefs_bufmap_map() is also buggy.
> >>
> >> I sort of feel like returning partial successes is not working.  We
> >> could easily make a wrapper which either pins everything or it returns
> >> an error code.
>
> Yes we could. And I have the same feeling about this API. It's generated a
> remarkable amount of bug fixes, several of which ended up being partial or
> wrong in themselves. And mostly this is due to the complicated tristate
> return code: instead of 0 or -ERRNO, it also can return "N pages that is
> less than what you requested", and there are no standard helpers in the kernel
> to make that easier to deal with

There was some discussion on removing return value 0 from one of the
gup variants [1].
I think it might be partially relevant to the current discussion.

[1] https://patchwork.kernel.org/patch/11529795/

>
> >
> > I guess the question is are there drivers which will keep working (or limp
> > along?) on partial pins?  A quick search of a driver I thought did this does
> > not apparently any more...  So it sounds good to me from 30,000 feet!  :-D
>
> It sounds good to me too--and from just a *few hundred feet* (having touched 
> most
> of the call sites at some point)! haha :)
>
> I think the wrapper should be short-term, though, just until all the callers
> are converted to the simpler API. Then change the core gup/pup calls to the 
> simpler
> API. There are more than enough gup/pup API entry points as it is, that's for 
> sure.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA

[linux-next PATCH] rapidio: Fix error handling path

[Souptick Joarder]

There is an error when pin_user_pages_fast() returns -ERRNO and
inside error handling path driver end up calling unpin_user_pages()
with -ERRNO which is not correct.

This patch will fix the problem.

Fixes: e8de370188d09 ("rapidio: add mport char device driver")
Signed-off-by: Souptick Joarder 
Cc: Ira Weiny 
Cc: John Hubbard 
Cc: Matthew Wilcox 
---
 drivers/rapidio/devices/rio_mport_cdev.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/rapidio/devices/rio_mport_cdev.c 
b/drivers/rapidio/devices/rio_mport_cdev.c
index a303429..163b6c72 100644
--- a/drivers/rapidio/devices/rio_mport_cdev.c
+++ b/drivers/rapidio/devices/rio_mport_cdev.c
@@ -871,15 +871,16 @@ static int do_dma_request(struct mport_dma_req *req,
rmcd_error("pin_user_pages_fast err=%ld",
   pinned);
nr_pages = 0;
-   } else
+   } else {
rmcd_error("pinned %ld out of %ld pages",
   pinned, nr_pages);
+   /*
+* Set nr_pages up to mean "how many pages to 
unpin, in
+* the error handler:
+*/
+   nr_pages = pinned;
+   }
ret = -EFAULT;
-   /*
-* Set nr_pages up to mean "how many pages to unpin, in
-* the error handler:
-*/
-   nr_pages = pinned;
goto err_pg;
}
 
-- 
1.9.1

Re: [PATCH] mm/gup.c: Handling ERR within unpin_user_pages()

[Souptick Joarder]

On Mon, Sep 14, 2020 at 7:38 PM Jason Gunthorpe  wrote:
>
> On Mon, Sep 14, 2020 at 07:20:34AM +0530, Souptick Joarder wrote:
> > On Sun, Sep 13, 2020 at 8:25 PM Matthew Wilcox  wrote:
> > >
> > > On Sun, Sep 13, 2020 at 08:02:35PM +0530, Souptick Joarder wrote:
> > > > It is possible that a buggy caller of unpin_user_pages()
> > > > (specially in error handling path) may end up calling it with
> > > > npages < 0 which is unnecessary.
> > > > @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned 
> > > > long npages)
> > > >  {
> > > >   unsigned long index;
> > > >
> > > > + if (WARN_ON_ONCE(npages < 0))
> > > > + return;
> > >
> > > But npages is unsigned long.  So it can't be less than zero.
> >
> > Sorry, I missed it.
> >
> > Then, it means if npages is assigned with -ERRNO by caller, 
> > unpin_user_pages()
> > may end up calling a big loop, which is unnecessary.
>
> How will a caller allocate memory of the right size and still manage
> to call with the wrong npages? Do you have an example of a broken caller?

These are two broken callers which might end up calling unpin_user_pages()
with -ERRNO.
drivers/rapidio/devices/rio_mport_cdev.c#L952
drivers/misc/mic/scif/scif_rma.c#L1399

They both are in error handling paths, so might not have any serious impact.
But theoretically possible.

Re: [PATCH] mm/gup.c: Handling ERR within unpin_user_pages()

[Souptick Joarder]

On Sun, Sep 13, 2020 at 8:25 PM Matthew Wilcox  wrote:
>
> On Sun, Sep 13, 2020 at 08:02:35PM +0530, Souptick Joarder wrote:
> > It is possible that a buggy caller of unpin_user_pages()
> > (specially in error handling path) may end up calling it with
> > npages < 0 which is unnecessary.
> > @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned 
> > long npages)
> >  {
> >   unsigned long index;
> >
> > + if (WARN_ON_ONCE(npages < 0))
> > + return;
>
> But npages is unsigned long.  So it can't be less than zero.

Sorry, I missed it.

Then, it means if npages is assigned with -ERRNO by caller, unpin_user_pages()
may end up calling a big loop, which is unnecessary.

Re: [PATCH] tee/tee_shm.c: Fix error handling path

[Souptick Joarder]

On Mon, Sep 14, 2020 at 1:59 AM Jens Wiklander
 wrote:
>
> On Sun, Sep 13, 2020 at 10:12:11AM +0530, Souptick Joarder wrote:
> > When shm->num_pages <= 0, we should avoid calling
> > release_registered_pages() in error handling path.
> What are we fixing?

Current code is working fine and this patch is not needed. Sorry for the noise.
>
> >
> > Signed-off-by: Souptick Joarder 
> > Cc: John Hubbard 
> > ---
> >  drivers/tee/tee_shm.c | 7 ---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> > index 00472f5..e517d9f 100644
> > --- a/drivers/tee/tee_shm.c
> > +++ b/drivers/tee/tee_shm.c
> > @@ -260,8 +260,7 @@ struct tee_shm *tee_shm_register(struct tee_context 
> > *ctx, unsigned long addr,
> >   rc = get_kernel_pages(kiov, num_pages, 0, shm->pages);
> >   kfree(kiov);
> >   }
> > - if (rc > 0)
> > - shm->num_pages = rc;
> > + shm->num_pages = rc;
> Why not avoiding assigning invalid values to shm->num_pages?
> By the way, shm->num_pages is a size_t.
>
> >   if (rc != num_pages) {
> >   if (rc >= 0)
> >   rc = -ENOMEM;
> > @@ -309,7 +308,9 @@ struct tee_shm *tee_shm_register(struct tee_context 
> > *ctx, unsigned long addr,
> >   idr_remove(>idr, shm->id);
> >   mutex_unlock(>mutex);
> >   }
> > - release_registered_pages(shm);
> > + if (shm->pages && (shm->num_pages > 0))
> > + release_registered_pages(shm);
> > +
> With this we'll leak if shm->pages has been assigned something.
>
> >   }
> >   kfree(shm);
> >   teedev_ctx_put(ctx);
> > -

[PATCH] mm/gup.c: Handling ERR within unpin_user_pages()

[Souptick Joarder]

It is possible that a buggy caller of unpin_user_pages()
(specially in error handling path) may end up calling it with
npages < 0 which is unnecessary.

This can be fixed by adding extra check inside unpin_user_pages().

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
---
 mm/gup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index 0b5c308b..2e19bd6 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long 
npages)
 {
unsigned long index;
 
+   if (WARN_ON_ONCE(npages < 0))
+   return;
+
/*
 * TODO: this can be optimized for huge pages: if a series of pages is
 * physically contiguous and part of the same compound page, then a
-- 
1.9.1

Re: [PATCH] tee/tee_shm.c: Fix error handling path

[Souptick Joarder]

On Sun, Sep 13, 2020 at 2:00 PM Markus Elfring  wrote:
>
> > When shm->num_pages <= 0, we should avoid calling
> > release_registered_pages() in error handling path.
>
> * Would an imperative wording become helpful for the change description?
>
> * I suggest to add the tag “Fixes” to the commit message.

Sure. Will address both in v2.
>
> Regards,
> Markus

[PATCH] tee/tee_shm.c: Fix error handling path

[Souptick Joarder]

When shm->num_pages <= 0, we should avoid calling
release_registered_pages() in error handling path.

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
---
 drivers/tee/tee_shm.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 00472f5..e517d9f 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -260,8 +260,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, 
unsigned long addr,
rc = get_kernel_pages(kiov, num_pages, 0, shm->pages);
kfree(kiov);
}
-   if (rc > 0)
-   shm->num_pages = rc;
+   shm->num_pages = rc;
if (rc != num_pages) {
if (rc >= 0)
rc = -ENOMEM;
@@ -309,7 +308,9 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, 
unsigned long addr,
idr_remove(>idr, shm->id);
mutex_unlock(>mutex);
}
-   release_registered_pages(shm);
+   if (shm->pages && (shm->num_pages > 0))
+   release_registered_pages(shm);
+
}
kfree(shm);
teedev_ctx_put(ctx);
-- 
1.9.1

Re: general protection fault in unlink_file_vma

[Souptick Joarder]

Hi Hiff,

On Wed, Sep 9, 2020 at 9:45 AM Hillf Danton  wrote:
>
>
> Tue, 08 Sep 2020 17:19:17 -0700
> > syzbot found the following issue on:
> >
> > HEAD commit:59126901 Merge tag 'perf-tools-fixes-for-v5.9-2020-09-03' ..
> > git tree:   upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=1166cb5d90
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=3c5f6ce8d5b68299
> > dashboard link: https://syzkaller.appspot.com/bug?extid=c5d5a51dcbb558ca0cb5
> > compiler:   gcc (GCC) 10.1.0-syz 20200507
> > syz repro:  https://syzkaller.appspot.com/x/repro.syz?x=11901e9590
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=15f5619590
> >
> > Bisection is inconclusive: the issue happens on the oldest tested release.
> >
> > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1205faed90
> > final oops: https://syzkaller.appspot.com/x/report.txt?x=1105faed90
> > console output: https://syzkaller.appspot.com/x/log.txt?x=1605faed90
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+c5d5a51dcbb558ca0...@syzkaller.appspotmail.com
> >
> > general protection fault, probably for non-canonical address 
> > 0xe00eeaee003b:  [#1] PREEMPT SMP KASAN
> > KASAN: maybe wild-memory-access in range 
> > [0x007001d8-0x007001df]
> > CPU: 1 PID: 10488 Comm: syz-executor721 Not tainted 5.9.0-rc3-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS 
> > Google 01/01/2011
> > RIP: 0010:unlink_file_vma+0x57/0xb0 mm/mmap.c:164
> > Code: 4c 8b a5 a0 00 00 00 4d 85 e4 74 4e e8 92 d7 cd ff 49 8d bc 24 d8 01 
> > 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <80> 3c 02 00 75 
> > 3d 4d 8b b4 24 d8 01 00 00 4d 8d 6e 78 4c 89 ef e8
> > RSP: 0018:c9000ac0f9b0 EFLAGS: 00010202
> > RAX: dc00 RBX: 88800010ceb0 RCX: 81592421
> > RDX: 000e003b RSI: 81a6736e RDI: 007001d8
> > RBP: 88800010ceb0 R08: 0001 R09: 88801291a50f
> > R10: ed10025234a1 R11: 0001 R12: 0070
> > R13: 7f1eea0da000 R14: 7f1eea0d9000 R15: 
> > FS:  () GS:8880ae70() knlGS:
> > CS:  0010 DS:  ES:  CR0: 80050033
> > CR2: 7f1eea11a9d0 CR3: 0007e000 CR4: 001506e0
> > DR0:  DR1:  DR2: 
> > DR3:  DR6: fffe0ff0 DR7: 0400
> > Call Trace:
> >  free_pgtables+0x1b3/0x2f0 mm/memory.c:415
> >  exit_mmap+0x2c0/0x530 mm/mmap.c:3184
> >  __mmput+0x122/0x470 kernel/fork.c:1076
> >  mmput+0x53/0x60 kernel/fork.c:1097
> >  exit_mm kernel/exit.c:483 [inline]
> >  do_exit+0xa8b/0x29f0 kernel/exit.c:793
> >  do_group_exit+0x125/0x310 kernel/exit.c:903
> >  get_signal+0x428/0x1f00 kernel/signal.c:2757
> >  arch_do_signal+0x82/0x2520 arch/x86/kernel/signal.c:811
> >  exit_to_user_mode_loop kernel/entry/common.c:136 [inline]
> >  exit_to_user_mode_prepare+0x1ae/0x200 kernel/entry/common.c:167
> >  syscall_exit_to_user_mode+0x7e/0x2e0 kernel/entry/common.c:242
> >  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Looks like d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible")
> added an extra fput.

Can you please help me to understand how do you figure out this commit ?
>
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1781,7 +1781,6 @@ unsigned long mmap_region(struct file *f
> merge = vma_merge(mm, prev, vma->vm_start, 
> vma->vm_end, vma->vm_flags,
> NULL, vma->vm_file, vma->vm_pgoff, NULL, 
> NULL_VM_UFFD_CTX);
> if (merge) {
> -   fput(file);
> vm_area_free(vma);
> vma = merge;
> /* Update vm_flags and possible addr to pick 
> up the change. We don't
>
>

[PATCH 1/2] xen/gntdev.c: Mark pages as dirty

[Souptick Joarder]

There seems to be a bug in the original code when gntdev_get_page()
is called with writeable=true then the page needs to be marked dirty
before being put.

To address this, a bool writeable is added in gnt_dev_copy_batch, set
it in gntdev_grant_copy_seg() (and drop `writeable` argument to
gntdev_get_page()) and then, based on batch->writeable, use
set_page_dirty_lock().

Fixes: a4cdb556cae0 (xen/gntdev: add ioctl for grant copy)
Suggested-by: Boris Ostrovsky 
Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: David Vrabel 
---
 drivers/xen/gntdev.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 64a9025a..5e1411b 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -720,17 +720,18 @@ struct gntdev_copy_batch {
s16 __user *status[GNTDEV_COPY_BATCH];
unsigned int nr_ops;
unsigned int nr_pages;
+   bool writeable;
 };
 
 static int gntdev_get_page(struct gntdev_copy_batch *batch, void __user *virt,
-  bool writeable, unsigned long *gfn)
+   unsigned long *gfn)
 {
unsigned long addr = (unsigned long)virt;
struct page *page;
unsigned long xen_pfn;
int ret;
 
-   ret = get_user_pages_fast(addr, 1, writeable ? FOLL_WRITE : 0, );
+   ret = get_user_pages_fast(addr, 1, batch->writeable ? FOLL_WRITE : 0, 
);
if (ret < 0)
return ret;
 
@@ -746,9 +747,13 @@ static void gntdev_put_pages(struct gntdev_copy_batch 
*batch)
 {
unsigned int i;
 
-   for (i = 0; i < batch->nr_pages; i++)
+   for (i = 0; i < batch->nr_pages; i++) {
+   if (batch->writeable && !PageDirty(batch->pages[i]))
+   set_page_dirty_lock(batch->pages[i]);
put_page(batch->pages[i]);
+   }
batch->nr_pages = 0;
+   batch->writeable = false;
 }
 
 static int gntdev_copy(struct gntdev_copy_batch *batch)
@@ -837,8 +842,9 @@ static int gntdev_grant_copy_seg(struct gntdev_copy_batch 
*batch,
virt = seg->source.virt + copied;
off = (unsigned long)virt & ~XEN_PAGE_MASK;
len = min(len, (size_t)XEN_PAGE_SIZE - off);
+   batch->writeable = false;
 
-   ret = gntdev_get_page(batch, virt, false, );
+   ret = gntdev_get_page(batch, virt, );
if (ret < 0)
return ret;
 
@@ -856,8 +862,9 @@ static int gntdev_grant_copy_seg(struct gntdev_copy_batch 
*batch,
virt = seg->dest.virt + copied;
off = (unsigned long)virt & ~XEN_PAGE_MASK;
len = min(len, (size_t)XEN_PAGE_SIZE - off);
+   batch->writeable = true;
 
-   ret = gntdev_get_page(batch, virt, true, );
+   ret = gntdev_get_page(batch, virt, );
if (ret < 0)
return ret;
 
-- 
1.9.1

[PATCH 2/2] xen/gntdev.c: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: David Vrabel 
---
 drivers/xen/gntdev.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 5e1411b..a36b712 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -731,7 +731,7 @@ static int gntdev_get_page(struct gntdev_copy_batch *batch, 
void __user *virt,
unsigned long xen_pfn;
int ret;
 
-   ret = get_user_pages_fast(addr, 1, batch->writeable ? FOLL_WRITE : 0, 
);
+   ret = pin_user_pages_fast(addr, 1, batch->writeable ? FOLL_WRITE : 0, 
);
if (ret < 0)
return ret;
 
@@ -745,13 +745,7 @@ static int gntdev_get_page(struct gntdev_copy_batch 
*batch, void __user *virt,
 
 static void gntdev_put_pages(struct gntdev_copy_batch *batch)
 {
-   unsigned int i;
-
-   for (i = 0; i < batch->nr_pages; i++) {
-   if(batch->writeable && !PageDirty(batch->pages[i]))
-   set_page_dirty_lock(batch->pages[i]);
-   put_page(batch->pages[i]);
-   }
+   unpin_user_pages_dirty_lock(batch->pages, batch->nr_pages, 
batch->writeable);
batch->nr_pages = 0;
batch->writeable = false;
 }
-- 
1.9.1

Re: [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path

[Souptick Joarder]

On Sat, Sep 5, 2020 at 6:51 AM John Hubbard  wrote:
>
> On 9/4/20 6:16 PM, Souptick Joarder wrote:
> > Hi Andrew,
> >
> > On Wed, Sep 2, 2020 at 3:00 AM John Hubbard  wrote:
> >>
> >> On 9/1/20 2:21 PM, Souptick Joarder wrote:
> >>> First, when memory allocation for sg_list_unaligned failed, there
> >>> is a bug of calling put_pages() as we haven't pinned any pages.
> >>>
> >>> Second, if get_user_pages_fast() failed we should unpin num_pinned
> >>> pages.
> >>>
> >>> This will address both.
> >>>
> >>> As part of these changes, minor update in documentation.
> >>>
> >>> Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
> >>> management driver")
> >>> Signed-off-by: Souptick Joarder 
> >>> Reviewed-by: Dan Carpenter 
> >>> Reviewed-by: John Hubbard 
> >>> ---
> >>
> >> This looks good to me.
> >
> > Can you please take this patch through the mm tree ?
> >
>
> Is there a problem with sending it through it's normal tree? It would probably
> get better testing coverage there.

scripts/get_maintainer.pl is showing only general lkml list and few
commit signer.
I didn't receive any feedback from anyone except you & Dan in more
than 12+ weeks
and mail bounced back from actual author  Timur Tabi.

As it is more than 12 weeks, I requested Andrew to take it through mm tree.

Note -

While running ./scripts/get_maintainer.pl, I observed one issue. Everytime I run
the script, list is getting changed. Is it an expected behaviour ?

Below is the details ->

jordon@jordon-HP-15-Notebook-PC:~/Documents/virt/linux-next$
./scripts/get_maintainer.pl
0001-drivers-virt-fsl_hypervisor-Fix-error-handling-path.patch
Bjorn Andersson  (commit_signer:1/1=100%)
Daniel Vetter  (commit_signer:1/1=100%)
Dan Williams  (commit_signer:1/1=100%)
"Darren Hart (VMware)"  (commit_signer:1/1=100%)
Greg Kroah-Hartman  (commit_signer:1/1=100%)
Arnd Bergmann 
(authored:1/1=100%,added_lines:1/1=100%,removed_lines:1/1=100%,blamed_fixes:1/1=100%)
Timur Tabi  (blamed_fixes:1/1=100%)
Kumar Gala  (blamed_fixes:1/1=100%)
linux-kernel@vger.kernel.org (open list)


jordon@jordon-HP-15-Notebook-PC:~/Documents/virt/linux-next$
./scripts/get_maintainer.pl
0001-drivers-virt-fsl_hypervisor-Fix-error-handling-path.patch
Bjorn Andersson  (commit_signer:1/1=100%)
Daniel Vetter  (commit_signer:1/1=100%)
Dan Williams  (commit_signer:1/1=100%)
"Darren Hart (VMware)"  (commit_signer:1/1=100%)
Greg Kroah-Hartman  (commit_signer:1/1=100%)
Arnd Bergmann 
(authored:1/1=100%,added_lines:1/1=100%,removed_lines:1/1=100%,blamed_fixes:1/1=100%)
Timur Tabi  (blamed_fixes:1/1=100%)
Kumar Gala  (blamed_fixes:1/1=100%)
linux-kernel@vger.kernel.org (open list)


jordon@jordon-HP-15-Notebook-PC:~/Documents/virt/linux-next$
./scripts/get_maintainer.pl
0001-drivers-virt-fsl_hypervisor-Fix-error-handling-path.patch
Bjorn Andersson  (commit_signer:1/1=100%)
Daniel Vetter  (commit_signer:1/1=100%)
Dan Williams  (commit_signer:1/1=100%)
"Darren Hart (VMware)"  (commit_signer:1/1=100%)
Greg Kroah-Hartman  (commit_signer:1/1=100%)
Arnd Bergmann 
(authored:1/1=100%,added_lines:1/1=100%,removed_lines:1/1=100%,blamed_fixes:1/1=100%)
Timur Tabi  (blamed_fixes:1/1=100%)
Kumar Gala  (blamed_fixes:1/1=100%)
linux-kernel@vger.kernel.org (open list)


>
>
> thanks,
> --
> John Hubbard
> NVIDIA

Re: [linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path

[Souptick Joarder]

Hi Andrew,

On Wed, Sep 2, 2020 at 3:00 AM John Hubbard  wrote:
>
> On 9/1/20 2:21 PM, Souptick Joarder wrote:
> > First, when memory allocation for sg_list_unaligned failed, there
> > is a bug of calling put_pages() as we haven't pinned any pages.
> >
> > Second, if get_user_pages_fast() failed we should unpin num_pinned
> > pages.
> >
> > This will address both.
> >
> > As part of these changes, minor update in documentation.
> >
> > Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
> > management driver")
> > Signed-off-by: Souptick Joarder 
> > Reviewed-by: Dan Carpenter 
> > Reviewed-by: John Hubbard 
> > ---
>
> This looks good to me.

Can you please take this patch through the mm tree ?

>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> > v2:
> > Added review tag.
> >
> > v3:
> > Address review comment on v2 from John.
> > Added review tag.
> >
> > v4:
> >Address another set of review comments from John.
> >
> >   drivers/virt/fsl_hypervisor.c | 17 -
> >   1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
> > index 1b0b11b..46ee0a0 100644
> > --- a/drivers/virt/fsl_hypervisor.c
> > +++ b/drivers/virt/fsl_hypervisor.c
> > @@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >
> >   unsigned int i;
> >   long ret = 0;
> > - int num_pinned; /* return value from get_user_pages() */
> > + int num_pinned = 0; /* return value from get_user_pages_fast() */
> >   phys_addr_t remote_paddr; /* The next address in the remote buffer */
> >   uint32_t count; /* The number of bytes left to copy */
> >
> > @@ -174,7 +174,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >   return -EINVAL;
> >
> >   /*
> > -  * The array of pages returned by get_user_pages() covers only
> > +  * The array of pages returned by get_user_pages_fast() covers only
> >* page-aligned memory.  Since the user buffer is probably not
> >* page-aligned, we need to handle the discrepancy.
> >*
> > @@ -224,7 +224,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >
> >   /*
> >* 'pages' is an array of struct page pointers that's initialized by
> > -  * get_user_pages().
> > +  * get_user_pages_fast().
> >*/
> >   pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
> >   if (!pages) {
> > @@ -241,7 +241,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >   if (!sg_list_unaligned) {
> >   pr_debug("fsl-hv: could not allocate S/G list\n");
> >   ret = -ENOMEM;
> > - goto exit;
> > + goto free_pages;
> >   }
> >   sg_list = PTR_ALIGN(sg_list_unaligned, sizeof(struct fh_sg_list));
> >
> > @@ -250,7 +250,6 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >   num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
> >
> >   if (num_pinned != num_pages) {
> > - /* get_user_pages() failed */
> >   pr_debug("fsl-hv: could not lock source buffer\n");
> >   ret = (num_pinned < 0) ? num_pinned : -EFAULT;
> >   goto exit;
> > @@ -292,13 +291,13 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >   virt_to_phys(sg_list), num_pages);
> >
> >   exit:
> > - if (pages) {
> > - for (i = 0; i < num_pages; i++)
> > - if (pages[i])
> > - put_page(pages[i]);
> > + if (pages && (num_pinned > 0)) {
> > + for (i = 0; i < num_pinned; i++)
> > + put_page(pages[i]);
> >   }
> >
> >   kfree(sg_list_unaligned);
> > +free_pages:
> >   kfree(pages);
> >
> >   if (!ret)
> >
>

Re: [PATCH] mm/gup: don't permit users to call get_user_pages with FOLL_LONGTERM

[Souptick Joarder]

On Fri, Sep 4, 2020 at 12:09 AM Matthew Wilcox  wrote:
>
> On Thu, Sep 03, 2020 at 12:42:44PM +0530, Souptick Joarder wrote:
> > We can use is_valid_gup_flags() inside ->
> > get_user_pages_locked(),
> > get_user_pages_unlocked(),
> > pin_user_pages_locked() as well.
> >
> > Are you planning to add it in future patches ?
>
> If you're looking for a new project, adding a foll_t or gup_t or
> something for the FOLL flags (like we have for gfp_t or vm_fault_t)
> would be helpful.  We're inconsistent with our naming here.

Sure. I will start looking into this and come up with a RFC version.

Re: [PATCH] mm/gup: don't permit users to call get_user_pages with FOLL_LONGTERM

[Souptick Joarder]

On Wed, Aug 19, 2020 at 11:45 PM John Hubbard  wrote:
>
> On 8/19/20 4:01 AM, Barry Song wrote:
> > gug prohibits users from calling get_user_pages() with FOLL_PIN. But it
>
> Maybe Andrew can fix the typo above: gug --> gup.
>
>
> > allows users to call get_user_pages() with FOLL_LONGTERM only. It seems
> > insensible.
> >
> > since FOLL_LONGTERM is a stricter case of FOLL_PIN, we should prohibit
> > users from calling get_user_pages() with FOLL_LONGTERM while not with
> > FOLL_PIN.
> >
> > mm/gup_benchmark.c used to be the only user who did this improperly.
> > But it has been fixed by moving to use pin_user_pages().
>
> For future patches, you don't have to write everything in the
> commit log. Some things are better placed in a cover letter or after
> the "---" line, because they don't need to be recorded forever.
>
> Anyway, the diffs seem fine, assuming that you've audited the call sites.

We can use is_valid_gup_flags() inside ->
get_user_pages_locked(),
get_user_pages_unlocked(),
pin_user_pages_locked() as well.

Are you planning to add it in future patches ?

>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> >
> > Cc: John Hubbard 
> > Cc: Jan Kara 
> > Cc: Jérôme Glisse 
> > Cc: "Matthew Wilcox (Oracle)" 
> > Cc: Al Viro 
> > Cc: Christoph Hellwig 
> > Cc: Dan Williams 
> > Cc: Dave Chinner 
> > Cc: Jason Gunthorpe 
> > Cc: Jonathan Corbet 
> > Cc: Michal Hocko 
> > Cc: Mike Kravetz 
> > Cc: Shuah Khan 
> > Cc: Vlastimil Babka 
> > Signed-off-by: Barry Song 
> > ---
> >   mm/gup.c | 37 ++---
> >   1 file changed, 22 insertions(+), 15 deletions(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index ae096ea7583f..4da669f79566 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1789,6 +1789,25 @@ static long __get_user_pages_remote(struct mm_struct 
> > *mm,
> >  gup_flags | FOLL_TOUCH | FOLL_REMOTE);
> >   }
> >
> > +static bool is_valid_gup_flags(unsigned int gup_flags)
> > +{
> > + /*
> > +  * FOLL_PIN must only be set internally by the pin_user_pages*() APIs,
> > +  * never directly by the caller, so enforce that with an assertion:
> > +  */
> > + if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
> > + return false;
> > + /*
> > +  * FOLL_PIN is a prerequisite to FOLL_LONGTERM. Another way of saying
> > +  * that is, FOLL_LONGTERM is a specific case, more restrictive case of
> > +  * FOLL_PIN.
> > +  */
> > + if (WARN_ON_ONCE(gup_flags & FOLL_LONGTERM))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> >   /**
> >* get_user_pages_remote() - pin user pages in memory
> >* @mm: mm_struct of target mm
> > @@ -1854,11 +1873,7 @@ long get_user_pages_remote(struct mm_struct *mm,
> >   unsigned int gup_flags, struct page **pages,
> >   struct vm_area_struct **vmas, int *locked)
> >   {
> > - /*
> > -  * FOLL_PIN must only be set internally by the pin_user_pages*() APIs,
> > -  * never directly by the caller, so enforce that with an assertion:
> > -  */
> > - if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
> > + if (!is_valid_gup_flags(gup_flags))
> >   return -EINVAL;
> >
> >   return __get_user_pages_remote(mm, start, nr_pages, gup_flags,
> > @@ -1904,11 +1919,7 @@ long get_user_pages(unsigned long start, unsigned 
> > long nr_pages,
> >   unsigned int gup_flags, struct page **pages,
> >   struct vm_area_struct **vmas)
> >   {
> > - /*
> > -  * FOLL_PIN must only be set internally by the pin_user_pages*() APIs,
> > -  * never directly by the caller, so enforce that with an assertion:
> > -  */
> > - if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
> > + if (!is_valid_gup_flags(gup_flags))
> >   return -EINVAL;
> >
> >   return __gup_longterm_locked(current->mm, start, nr_pages,
> > @@ -2810,11 +2821,7 @@ EXPORT_SYMBOL_GPL(get_user_pages_fast_only);
> >   int get_user_pages_fast(unsigned long start, int nr_pages,
> >   unsigned int gup_flags, struct page **pages)
> >   {
> > - /*
> > -  * FOLL_PIN must only be set internally by the pin_user_pages*() APIs,
> > -  * never directly by the caller, so enforce that:
> > -  */
> > - if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
> > + if (!is_valid_gup_flags(gup_flags))
> >   return -EINVAL;
> >
> >   /*
> >
>
>

[linux-next PATCH v4] drivers/virt/fsl_hypervisor: Fix error handling path

[Souptick Joarder]

First, when memory allocation for sg_list_unaligned failed, there
is a bug of calling put_pages() as we haven't pinned any pages.

Second, if get_user_pages_fast() failed we should unpin num_pinned
pages.

This will address both.

As part of these changes, minor update in documentation.

Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
management driver")
Signed-off-by: Souptick Joarder 
Reviewed-by: Dan Carpenter 
Reviewed-by: John Hubbard 
---
v2:
   Added review tag.

v3:
   Address review comment on v2 from John.
   Added review tag.

v4:
  Address another set of review comments from John.

 drivers/virt/fsl_hypervisor.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 1b0b11b..46ee0a0 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
 
unsigned int i;
long ret = 0;
-   int num_pinned; /* return value from get_user_pages() */
+   int num_pinned = 0; /* return value from get_user_pages_fast() */
phys_addr_t remote_paddr; /* The next address in the remote buffer */
uint32_t count; /* The number of bytes left to copy */
 
@@ -174,7 +174,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
return -EINVAL;
 
/*
-* The array of pages returned by get_user_pages() covers only
+* The array of pages returned by get_user_pages_fast() covers only
 * page-aligned memory.  Since the user buffer is probably not
 * page-aligned, we need to handle the discrepancy.
 *
@@ -224,7 +224,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
 
/*
 * 'pages' is an array of struct page pointers that's initialized by
-* get_user_pages().
+* get_user_pages_fast().
 */
pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
if (!pages) {
@@ -241,7 +241,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
if (!sg_list_unaligned) {
pr_debug("fsl-hv: could not allocate S/G list\n");
ret = -ENOMEM;
-   goto exit;
+   goto free_pages;
}
sg_list = PTR_ALIGN(sg_list_unaligned, sizeof(struct fh_sg_list));
 
@@ -250,7 +250,6 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
 
if (num_pinned != num_pages) {
-   /* get_user_pages() failed */
pr_debug("fsl-hv: could not lock source buffer\n");
ret = (num_pinned < 0) ? num_pinned : -EFAULT;
goto exit;
@@ -292,13 +291,13 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
__user *p)
virt_to_phys(sg_list), num_pages);
 
 exit:
-   if (pages) {
-   for (i = 0; i < num_pages; i++)
-   if (pages[i])
-   put_page(pages[i]);
+   if (pages && (num_pinned > 0)) {
+   for (i = 0; i < num_pinned; i++)
+   put_page(pages[i]);
}
 
kfree(sg_list_unaligned);
+free_pages:
kfree(pages);
 
if (!ret)
-- 
1.9.1

Re: [linux-next PATCH v3] drivers/virt/fsl_hypervisor: Fix error handling path

[Souptick Joarder]

Hi John,

On Tue, Sep 1, 2020 at 4:28 AM John Hubbard  wrote:
>
> On 8/31/20 3:07 PM, Souptick Joarder wrote:
> > First, when memory allocation for sg_list_unaligned failed, there
> > is a bug of calling put_pages() as we haven't pinned any pages.
>
> "we should unpin"

will it be "we shouldn't unpin" ? can you please clarify this ?
>
> ...
> >
> > @@ -250,7 +250,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >   num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
> >
> >   if (num_pinned != num_pages) {
> > - /* get_user_pages() failed */
> > + /* get_user_pages_fast() failed */
>
> Let's please just delete that particular comment entirely. It's of
> questionable accuracy (partial success is allowed with this API), and it
> is echoing the code too closely to be worth the line that it consumes.
>
> More importantly, though, we need to split up the cases of gup_fast
> returning a negative value, and a zero or positive value. Either here,
> or at "exit:", the negative return case should just skip any attempt to
> do any put_page() calls at all. Because it's a maintenance hazard to
> leave in a loop that depends on looping from zero, to -ERRNO, and *not*
> doing any loops--especially in the signed/unsigned soupy mess around gup
> calls.
>
>
> >   pr_debug("fsl-hv: could not lock source buffer\n");
> >   ret = (num_pinned < 0) ? num_pinned : -EFAULT;
> >   goto exit;
> > @@ -293,12 +293,12 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >
> >   exit:
> >   if (pages) {
> > - for (i = 0; i < num_pages; i++)
> > - if (pages[i])
> > - put_page(pages[i]);
> > + for (i = 0; i < num_pinned; i++)
> > + put_page(pages[i]);
>
> Looks correct. I sometimes wonder why more callers don't use
> release_pages() in situations like this, but that's beyond the scope of
> your work here.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA

Re: [linux-next PATCH v3] drivers/virt/fsl_hypervisor: Fix error handling path

[Souptick Joarder]

Hi John,

On Tue, Sep 1, 2020 at 3:38 AM Souptick Joarder  wrote:
>
> First, when memory allocation for sg_list_unaligned failed, there
> is a bug of calling put_pages() as we haven't pinned any pages.
>
> Second, if get_user_pages_fast() failed we should unpinned num_pinned
> pages instead of checking till num_pages.
>
> This will address both.
>
> As part of these changes, minor update in documentation.
>
> Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
> management driver")
> Signed-off-by: Souptick Joarder 
> Reviewed-by: Dan Carpenter 
> Reviewed-by: John Hubbard 

Few questions ->

First, there are minor updates from v2 -> v3 other than addressing your
review comments in v2. Would you like to review it again?

Second, I think, as per Documentation/core-api/pin_user_pages.rst,
this is case 5.
Shall I make the conversion as part of this series ?

> ---
> v2:
>Added review tag.
>
> v3:
>Address review comment on v2 from John.
>Added review tag.
>
>  drivers/virt/fsl_hypervisor.c | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
> index 1b0b11b..efb4e7f 100644
> --- a/drivers/virt/fsl_hypervisor.c
> +++ b/drivers/virt/fsl_hypervisor.c
> @@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> __user *p)
>
> unsigned int i;
> long ret = 0;
> -   int num_pinned; /* return value from get_user_pages() */
> +   int num_pinned = 0; /* return value from get_user_pages_fast() */
> phys_addr_t remote_paddr; /* The next address in the remote buffer */
> uint32_t count; /* The number of bytes left to copy */
>
> @@ -174,7 +174,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> __user *p)
> return -EINVAL;
>
> /*
> -* The array of pages returned by get_user_pages() covers only
> +* The array of pages returned by get_user_pages_fast() covers only
>  * page-aligned memory.  Since the user buffer is probably not
>  * page-aligned, we need to handle the discrepancy.
>  *
> @@ -224,7 +224,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> __user *p)
>
> /*
>  * 'pages' is an array of struct page pointers that's initialized by
> -* get_user_pages().
> +* get_user_pages_fast().
>  */
> pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
> if (!pages) {
> @@ -241,7 +241,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> __user *p)
> if (!sg_list_unaligned) {
> pr_debug("fsl-hv: could not allocate S/G list\n");
> ret = -ENOMEM;
> -   goto exit;
> +   goto free_pages;
> }
> sg_list = PTR_ALIGN(sg_list_unaligned, sizeof(struct fh_sg_list));
>
> @@ -250,7 +250,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> __user *p)
> num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
>
> if (num_pinned != num_pages) {
> -   /* get_user_pages() failed */
> +   /* get_user_pages_fast() failed */
> pr_debug("fsl-hv: could not lock source buffer\n");
> ret = (num_pinned < 0) ? num_pinned : -EFAULT;
> goto exit;
> @@ -293,12 +293,12 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> __user *p)
>
>  exit:
> if (pages) {
> -   for (i = 0; i < num_pages; i++)
> -   if (pages[i])
> -   put_page(pages[i]);
> +   for (i = 0; i < num_pinned; i++)
> +   put_page(pages[i]);
> }
>
> kfree(sg_list_unaligned);
> +free_pages:
> kfree(pages);
>
> if (!ret)
> --
> 1.9.1
>

[linux-next PATCH v3] drivers/virt/fsl_hypervisor: Fix error handling path

[Souptick Joarder]

First, when memory allocation for sg_list_unaligned failed, there
is a bug of calling put_pages() as we haven't pinned any pages.

Second, if get_user_pages_fast() failed we should unpinned num_pinned
pages instead of checking till num_pages.

This will address both.

As part of these changes, minor update in documentation.

Fixes: 6db7199407ca ("drivers/virt: introduce Freescale hypervisor
management driver")
Signed-off-by: Souptick Joarder 
Reviewed-by: Dan Carpenter 
Reviewed-by: John Hubbard 
---
v2:
   Added review tag.

v3:
   Address review comment on v2 from John.
   Added review tag.

 drivers/virt/fsl_hypervisor.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 1b0b11b..efb4e7f 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
 
unsigned int i;
long ret = 0;
-   int num_pinned; /* return value from get_user_pages() */
+   int num_pinned = 0; /* return value from get_user_pages_fast() */
phys_addr_t remote_paddr; /* The next address in the remote buffer */
uint32_t count; /* The number of bytes left to copy */
 
@@ -174,7 +174,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
return -EINVAL;
 
/*
-* The array of pages returned by get_user_pages() covers only
+* The array of pages returned by get_user_pages_fast() covers only
 * page-aligned memory.  Since the user buffer is probably not
 * page-aligned, we need to handle the discrepancy.
 *
@@ -224,7 +224,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
 
/*
 * 'pages' is an array of struct page pointers that's initialized by
-* get_user_pages().
+* get_user_pages_fast().
 */
pages = kcalloc(num_pages, sizeof(struct page *), GFP_KERNEL);
if (!pages) {
@@ -241,7 +241,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
if (!sg_list_unaligned) {
pr_debug("fsl-hv: could not allocate S/G list\n");
ret = -ENOMEM;
-   goto exit;
+   goto free_pages;
}
sg_list = PTR_ALIGN(sg_list_unaligned, sizeof(struct fh_sg_list));
 
@@ -250,7 +250,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
num_pages, param.source != -1 ? FOLL_WRITE : 0, pages);
 
if (num_pinned != num_pages) {
-   /* get_user_pages() failed */
+   /* get_user_pages_fast() failed */
pr_debug("fsl-hv: could not lock source buffer\n");
ret = (num_pinned < 0) ? num_pinned : -EFAULT;
goto exit;
@@ -293,12 +293,12 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
__user *p)
 
 exit:
if (pages) {
-   for (i = 0; i < num_pages; i++)
-   if (pages[i])
-   put_page(pages[i]);
+   for (i = 0; i < num_pinned; i++)
+   put_page(pages[i]);
}
 
kfree(sg_list_unaligned);
+free_pages:
kfree(pages);
 
if (!ret)
-- 
1.9.1

Re: [PATCH v2] drivers/virt/fsl_hypervisor: Correcting error handling path

[Souptick Joarder]

On Thu, Jul 30, 2020 at 6:30 AM John Hubbard  wrote:
>
> On 7/29/20 12:01 PM, Souptick Joarder wrote:
> > First, when memory allocation for sg_list_unaligned failed, there
> > is no point of calling put_pages() as we haven't pinned any pages.
> >
> > Second, if get_user_pages_fast() failed we should unpinned num_pinned
> > pages, no point of checking till num_pages.
>
> Hi Souptick,
>
> For both of the above, the wording "no point" is so overly gentle as
> to be misleading. That's because calling put_page() on any pages beyond
> num_pinned is a *bug*.
>
> So let's reword that. And let's change the patch subject from "Correcting" to
> "fix".
>
> And probably good to add a Fixes: tag, too.

Is there any scripts/ settings to fetch Fixes: tag other than using git blame ?

>
> More:
>
> >
> > This will address both.
> >
> > Signed-off-by: Souptick Joarder 
> > Reviewed-by: Dan Carpenter 
> > Cc: John Hubbard 
> > ---
> > v2:
> >   Added review tag.
> >
> >   drivers/virt/fsl_hypervisor.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
> > index 1b0b11b..ea344d7 100644
> > --- a/drivers/virt/fsl_hypervisor.c
> > +++ b/drivers/virt/fsl_hypervisor.c
> > @@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >
> >   unsigned int i;
> >   long ret = 0;
> > - int num_pinned; /* return value from get_user_pages() */
> > + int num_pinned = 0; /* return value from get_user_pages() */
> >   phys_addr_t remote_paddr; /* The next address in the remote buffer */
> >   uint32_t count; /* The number of bytes left to copy */
> >
> > @@ -293,7 +293,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy 
> > __user *p)
> >
> >   exit:
> >   if (pages) {
> > - for (i = 0; i < num_pages; i++)
> > + for (i = 0; i < num_pinned; i++)
> >   if (pages[i])
>
> I suspect that this "if" is unnecessary now.
>
> Either way, the diff itself looks good to me, so with the wording changes to
> the commit description, you can add:
>
>  Reviewed-by: John Hubbard 
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> >   put_page(pages[i]);
> >   }
> >
>

[RFC PATCH] xen/gntdev.c: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
---
 drivers/xen/gntdev.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 64a9025a..e480509 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -730,7 +730,7 @@ static int gntdev_get_page(struct gntdev_copy_batch *batch, 
void __user *virt,
unsigned long xen_pfn;
int ret;
 
-   ret = get_user_pages_fast(addr, 1, writeable ? FOLL_WRITE : 0, );
+   ret = pin_user_pages_fast(addr, 1, writeable ? FOLL_WRITE : 0, );
if (ret < 0)
return ret;
 
@@ -744,10 +744,7 @@ static int gntdev_get_page(struct gntdev_copy_batch 
*batch, void __user *virt,
 
 static void gntdev_put_pages(struct gntdev_copy_batch *batch)
 {
-   unsigned int i;
-
-   for (i = 0; i < batch->nr_pages; i++)
-   put_page(batch->pages[i]);
+   unpin_user_pages(batch->pages, batch->nr_pages);
batch->nr_pages = 0;
 }
 
-- 
1.9.1

[PATCH v2] drivers/virt/fsl_hypervisor: Correcting error handling path

[Souptick Joarder]

First, when memory allocation for sg_list_unaligned failed, there
is no point of calling put_pages() as we haven't pinned any pages.

Second, if get_user_pages_fast() failed we should unpinned num_pinned
pages, no point of checking till num_pages.

This will address both.

Signed-off-by: Souptick Joarder 
Reviewed-by: Dan Carpenter 
Cc: John Hubbard 
---
v2:
Added review tag.

 drivers/virt/fsl_hypervisor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 1b0b11b..ea344d7 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -157,7 +157,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
 
unsigned int i;
long ret = 0;
-   int num_pinned; /* return value from get_user_pages() */
+   int num_pinned = 0; /* return value from get_user_pages() */
phys_addr_t remote_paddr; /* The next address in the remote buffer */
uint32_t count; /* The number of bytes left to copy */
 
@@ -293,7 +293,7 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user 
*p)
 
 exit:
if (pages) {
-   for (i = 0; i < num_pages; i++)
+   for (i = 0; i < num_pinned; i++)
if (pages[i])
put_page(pages[i]);
}
-- 
1.9.1

Re: [PATCH v3 0/3] Few bug fixes and Convert to pin_user_pages*()

[Souptick Joarder]

Hi Boris,

On Sun, Jul 12, 2020 at 9:01 AM Souptick Joarder  wrote:
>
> This series contains few clean up, minor bug fixes and
> Convert get_user_pages() to pin_user_pages().
>
> I'm compile tested this, but unable to run-time test,
> so any testing help is much appriciated.
>
> v2:
> Addressed few review comments and compile issue.
> Patch[1/2] from v1 split into 2 in v2.
> v3:
> Address review comment. Add review tag.
>
> Cc: John Hubbard 
> Cc: Boris Ostrovsky 
> Cc: Paul Durrant 
>
> Souptick Joarder (3):
>   xen/privcmd: Corrected error handling path
>   xen/privcmd: Mark pages as dirty
>   xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

Does this series looks good to go for 5.9 ?

>
>  drivers/xen/privcmd.c | 32 ++--
>  1 file changed, 14 insertions(+), 18 deletions(-)
>
> --
> 1.9.1
>

[PATCH v3 3/3] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder 
Reviewed-by: Juergen Gross 
Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Paul Durrant 
---
 drivers/xen/privcmd.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 079d35b..63abe6c 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -593,7 +593,7 @@ static int lock_pages(
if (requested > nr_pages)
return -ENOSPC;
 
-   page_count = get_user_pages_fast(
+   page_count = pin_user_pages_fast(
(unsigned long) kbufs[i].uptr,
requested, FOLL_WRITE, pages);
if (page_count < 0)
@@ -609,13 +609,7 @@ static int lock_pages(
 
 static void unlock_pages(struct page *pages[], unsigned int nr_pages)
 {
-   unsigned int i;
-
-   for (i = 0; i < nr_pages; i++) {
-   if (!PageDirty(pages[i]))
-   set_page_dirty_lock(pages[i]);
-   put_page(pages[i]);
-   }
+   unpin_user_pages_dirty_lock(pages, nr_pages, true);
 }
 
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
-- 
1.9.1

[PATCH v3 2/3] xen/privcmd: Mark pages as dirty

[Souptick Joarder]

pages need to be marked as dirty before unpinned it in
unlock_pages() which was oversight. This is fixed now.

Signed-off-by: Souptick Joarder 
Suggested-by: John Hubbard 
Reviewed-by: Juergen Gross 
Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Paul Durrant 
---
 drivers/xen/privcmd.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index b001673..079d35b 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -611,8 +611,11 @@ static void unlock_pages(struct page *pages[], unsigned 
int nr_pages)
 {
unsigned int i;
 
-   for (i = 0; i < nr_pages; i++)
+   for (i = 0; i < nr_pages; i++) {
+   if (!PageDirty(pages[i]))
+   set_page_dirty_lock(pages[i]);
put_page(pages[i]);
+   }
 }
 
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
-- 
1.9.1

[PATCH v3 0/3] Few bug fixes and Convert to pin_user_pages*()

[Souptick Joarder]

This series contains few clean up, minor bug fixes and
Convert get_user_pages() to pin_user_pages().

I'm compile tested this, but unable to run-time test,
so any testing help is much appriciated.

v2:
Addressed few review comments and compile issue.
Patch[1/2] from v1 split into 2 in v2.
v3:
Address review comment. Add review tag.

Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Paul Durrant 

Souptick Joarder (3):
  xen/privcmd: Corrected error handling path
  xen/privcmd: Mark pages as dirty
  xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

 drivers/xen/privcmd.c | 32 ++--
 1 file changed, 14 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v3 1/3] xen/privcmd: Corrected error handling path

[Souptick Joarder]

Previously, if lock_pages() end up partially mapping pages, it used
to return -ERRNO due to which unlock_pages() have to go through
each pages[i] till *nr_pages* to validate them. This can be avoided
by passing correct number of partially mapped pages & -ERRNO separately,
while returning from lock_pages() due to error.

With this fix unlock_pages() doesn't need to validate pages[i] till
*nr_pages* for error scenario and few condition checks can be ignored.

Signed-off-by: Souptick Joarder 
Reviewed-by: Juergen Gross 
Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Paul Durrant 
---
 drivers/xen/privcmd.c | 31 +++
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 5dfc59f..b001673 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -579,13 +579,13 @@ static long privcmd_ioctl_mmap_batch(
 
 static int lock_pages(
struct privcmd_dm_op_buf kbufs[], unsigned int num,
-   struct page *pages[], unsigned int nr_pages)
+   struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
 {
unsigned int i;
 
for (i = 0; i < num; i++) {
unsigned int requested;
-   int pinned;
+   int page_count;
 
requested = DIV_ROUND_UP(
offset_in_page(kbufs[i].uptr) + kbufs[i].size,
@@ -593,14 +593,15 @@ static int lock_pages(
if (requested > nr_pages)
return -ENOSPC;
 
-   pinned = get_user_pages_fast(
+   page_count = get_user_pages_fast(
(unsigned long) kbufs[i].uptr,
requested, FOLL_WRITE, pages);
-   if (pinned < 0)
-   return pinned;
+   if (page_count < 0)
+   return page_count;
 
-   nr_pages -= pinned;
-   pages += pinned;
+   *pinned += page_count;
+   nr_pages -= page_count;
+   pages += page_count;
}
 
return 0;
@@ -610,13 +611,8 @@ static void unlock_pages(struct page *pages[], unsigned 
int nr_pages)
 {
unsigned int i;
 
-   if (!pages)
-   return;
-
-   for (i = 0; i < nr_pages; i++) {
-   if (pages[i])
-   put_page(pages[i]);
-   }
+   for (i = 0; i < nr_pages; i++)
+   put_page(pages[i]);
 }
 
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
@@ -629,6 +625,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void 
__user *udata)
struct xen_dm_op_buf *xbufs = NULL;
unsigned int i;
long rc;
+   unsigned int pinned = 0;
 
if (copy_from_user(, udata, sizeof(kdata)))
return -EFAULT;
@@ -682,9 +679,11 @@ static long privcmd_ioctl_dm_op(struct file *file, void 
__user *udata)
goto out;
}
 
-   rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
-   if (rc)
+   rc = lock_pages(kbufs, kdata.num, pages, nr_pages, );
+   if (rc < 0) {
+   nr_pages = pinned;
goto out;
+   }
 
for (i = 0; i < kdata.num; i++) {
set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
-- 
1.9.1

Re: [PATCH v2 1/3] xen/privcmd: Corrected error handling path

[Souptick Joarder]

On Tue, Jul 7, 2020 at 5:15 PM Jürgen Groß  wrote:
>
> On 07.07.20 13:40, Souptick Joarder wrote:
> > On Tue, Jul 7, 2020 at 3:05 PM Jürgen Groß  wrote:
> >>
> >> On 06.07.20 20:16, Souptick Joarder wrote:
> >>> Previously, if lock_pages() end up partially mapping pages, it used
> >>> to return -ERRNO due to which unlock_pages() have to go through
> >>> each pages[i] till *nr_pages* to validate them. This can be avoided
> >>> by passing correct number of partially mapped pages & -ERRNO separately,
> >>> while returning from lock_pages() due to error.
> >>>
> >>> With this fix unlock_pages() doesn't need to validate pages[i] till
> >>> *nr_pages* for error scenario and few condition checks can be ignored.
> >>>
> >>> Signed-off-by: Souptick Joarder 
> >>> Cc: John Hubbard 
> >>> Cc: Boris Ostrovsky 
> >>> Cc: Paul Durrant 
> >>> ---
> >>>drivers/xen/privcmd.c | 31 +++
> >>>1 file changed, 15 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> >>> index a250d11..33677ea 100644
> >>> --- a/drivers/xen/privcmd.c
> >>> +++ b/drivers/xen/privcmd.c
> >>> @@ -580,13 +580,13 @@ static long privcmd_ioctl_mmap_batch(
> >>>
> >>>static int lock_pages(
> >>>struct privcmd_dm_op_buf kbufs[], unsigned int num,
> >>> - struct page *pages[], unsigned int nr_pages)
> >>> + struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
> >>>{
> >>>unsigned int i;
> >>> + int page_count = 0;
> >>
> >> Initial value shouldn't be needed, and ...
> >>
> >>>
> >>>for (i = 0; i < num; i++) {
> >>>unsigned int requested;
> >>> - int pinned;
> >>
> >> ... you could move the declaration here.
> >>
> >> With that done you can add my
> >>
> >> Reviewed-by: Juergen Gross 
> >
> > Ok. But does it going make any difference other than limiting scope ?
>
> Dropping the initializer surely does, and in the end page_count just
> replaces the former pinned variable, so why would we want to widen the
> scope with this patch?

Agree, no reason to move it up. Will change it in v3.

>
>
> Juergen

Re: [PATCH v2 1/3] xen/privcmd: Corrected error handling path

[Souptick Joarder]

On Tue, Jul 7, 2020 at 3:05 PM Jürgen Groß  wrote:
>
> On 06.07.20 20:16, Souptick Joarder wrote:
> > Previously, if lock_pages() end up partially mapping pages, it used
> > to return -ERRNO due to which unlock_pages() have to go through
> > each pages[i] till *nr_pages* to validate them. This can be avoided
> > by passing correct number of partially mapped pages & -ERRNO separately,
> > while returning from lock_pages() due to error.
> >
> > With this fix unlock_pages() doesn't need to validate pages[i] till
> > *nr_pages* for error scenario and few condition checks can be ignored.
> >
> > Signed-off-by: Souptick Joarder 
> > Cc: John Hubbard 
> > Cc: Boris Ostrovsky 
> > Cc: Paul Durrant 
> > ---
> >   drivers/xen/privcmd.c | 31 +++
> >   1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index a250d11..33677ea 100644
> > --- a/drivers/xen/privcmd.c
> > +++ b/drivers/xen/privcmd.c
> > @@ -580,13 +580,13 @@ static long privcmd_ioctl_mmap_batch(
> >
> >   static int lock_pages(
> >   struct privcmd_dm_op_buf kbufs[], unsigned int num,
> > - struct page *pages[], unsigned int nr_pages)
> > + struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
> >   {
> >   unsigned int i;
> > + int page_count = 0;
>
> Initial value shouldn't be needed, and ...
>
> >
> >   for (i = 0; i < num; i++) {
> >   unsigned int requested;
> > - int pinned;
>
> ... you could move the declaration here.
>
> With that done you can add my
>
> Reviewed-by: Juergen Gross 

Ok. But does it going make any difference other than limiting scope ?

>
>
> Juergen

Re: [PATCH v2 2/3] xen/privcmd: Mark pages as dirty

[Souptick Joarder]

On Tue, Jul 7, 2020 at 3:08 PM Jürgen Groß  wrote:
>
> On 06.07.20 20:16, Souptick Joarder wrote:
> > pages need to be marked as dirty before unpinned it in
> > unlock_pages() which was oversight. This is fixed now.
> >
> > Signed-off-by: Souptick Joarder 
> > Suggested-by: John Hubbard 
> > Cc: John Hubbard 
> > Cc: Boris Ostrovsky 
> > Cc: Paul Durrant 
> > ---
> >   drivers/xen/privcmd.c | 5 -
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> > index 33677ea..f6c1543 100644
> > --- a/drivers/xen/privcmd.c
> > +++ b/drivers/xen/privcmd.c
> > @@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], 
> > unsigned int nr_pages)
> >   {
> >   unsigned int i;
> >
> > - for (i = 0; i < nr_pages; i++)
> > + for (i = 0; i < nr_pages; i++) {
> > + if (!PageDirty(pages[i]))
> > + set_page_dirty_lock(pages[i]);
>
> With put_page() directly following I think you should be able to use
> set_page_dirty() instead, as there is obviously a reference to the page
> existing.

Patch [3/3] will convert above codes to use unpin_user_pages_dirty_lock()
which internally do the same check. So I thought to keep linux-stable and
linux-next code in sync. John had a similar concern [1] and later agreed to keep
this check.

Shall I keep this check ?  No ?

[1] 
https://lore.kernel.org/xen-devel/a750e5e5-fd5d-663b-c5fd-261d7c939...@nvidia.com/

>
> >   put_page(pages[i]);
> > + }
> >   }
> >
> >   static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
> >
>
> Juergen

[PATCH v2 2/3] xen/privcmd: Mark pages as dirty

[Souptick Joarder]

pages need to be marked as dirty before unpinned it in
unlock_pages() which was oversight. This is fixed now.

Signed-off-by: Souptick Joarder 
Suggested-by: John Hubbard 
Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Paul Durrant 
---
 drivers/xen/privcmd.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 33677ea..f6c1543 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], unsigned 
int nr_pages)
 {
unsigned int i;
 
-   for (i = 0; i < nr_pages; i++)
+   for (i = 0; i < nr_pages; i++) {
+   if (!PageDirty(pages[i]))
+   set_page_dirty_lock(pages[i]);
put_page(pages[i]);
+   }
 }
 
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
-- 
1.9.1

[PATCH v2 3/3] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Paul Durrant 
---
 drivers/xen/privcmd.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index f6c1543..5c5cd24 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -594,7 +594,7 @@ static int lock_pages(
if (requested > nr_pages)
return -ENOSPC;
 
-   page_count = get_user_pages_fast(
+   page_count = pin_user_pages_fast(
(unsigned long) kbufs[i].uptr,
requested, FOLL_WRITE, pages);
if (page_count < 0)
@@ -610,13 +610,7 @@ static int lock_pages(
 
 static void unlock_pages(struct page *pages[], unsigned int nr_pages)
 {
-   unsigned int i;
-
-   for (i = 0; i < nr_pages; i++) {
-   if (!PageDirty(pages[i]))
-   set_page_dirty_lock(pages[i]);
-   put_page(pages[i]);
-   }
+   unpin_user_pages_dirty_lock(pages, nr_pages, true);
 }
 
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
-- 
1.9.1

[PATCH v2 1/3] xen/privcmd: Corrected error handling path

[Souptick Joarder]

Previously, if lock_pages() end up partially mapping pages, it used
to return -ERRNO due to which unlock_pages() have to go through
each pages[i] till *nr_pages* to validate them. This can be avoided
by passing correct number of partially mapped pages & -ERRNO separately,
while returning from lock_pages() due to error.

With this fix unlock_pages() doesn't need to validate pages[i] till
*nr_pages* for error scenario and few condition checks can be ignored.

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Paul Durrant 
---
 drivers/xen/privcmd.c | 31 +++
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index a250d11..33677ea 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -580,13 +580,13 @@ static long privcmd_ioctl_mmap_batch(
 
 static int lock_pages(
struct privcmd_dm_op_buf kbufs[], unsigned int num,
-   struct page *pages[], unsigned int nr_pages)
+   struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
 {
unsigned int i;
+   int page_count = 0;
 
for (i = 0; i < num; i++) {
unsigned int requested;
-   int pinned;
 
requested = DIV_ROUND_UP(
offset_in_page(kbufs[i].uptr) + kbufs[i].size,
@@ -594,14 +594,15 @@ static int lock_pages(
if (requested > nr_pages)
return -ENOSPC;
 
-   pinned = get_user_pages_fast(
+   page_count = get_user_pages_fast(
(unsigned long) kbufs[i].uptr,
requested, FOLL_WRITE, pages);
-   if (pinned < 0)
-   return pinned;
+   if (page_count < 0)
+   return page_count;
 
-   nr_pages -= pinned;
-   pages += pinned;
+   *pinned += page_count;
+   nr_pages -= page_count;
+   pages += page_count;
}
 
return 0;
@@ -611,13 +612,8 @@ static void unlock_pages(struct page *pages[], unsigned 
int nr_pages)
 {
unsigned int i;
 
-   if (!pages)
-   return;
-
-   for (i = 0; i < nr_pages; i++) {
-   if (pages[i])
-   put_page(pages[i]);
-   }
+   for (i = 0; i < nr_pages; i++)
+   put_page(pages[i]);
 }
 
 static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
@@ -630,6 +626,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void 
__user *udata)
struct xen_dm_op_buf *xbufs = NULL;
unsigned int i;
long rc;
+   unsigned int pinned = 0;
 
if (copy_from_user(, udata, sizeof(kdata)))
return -EFAULT;
@@ -683,9 +680,11 @@ static long privcmd_ioctl_dm_op(struct file *file, void 
__user *udata)
goto out;
}
 
-   rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
-   if (rc)
+   rc = lock_pages(kbufs, kdata.num, pages, nr_pages, );
+   if (rc < 0) {
+   nr_pages = pinned;
goto out;
+   }
 
for (i = 0; i < kdata.num; i++) {
set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
-- 
1.9.1

[PATCH v2 0/3] Few bug fixes and Convert to pin_user_pages*()

[Souptick Joarder]

This series contains few clean up, minor bug fixes and
Convert get_user_pages() to pin_user_pages().

I'm compile tested this, but unable to run-time test,
so any testing help is much appriciated.

v2:
Addressed few review comments and compile issue.
Patch[1/2] from v1 split into 2 in v2.

Cc: John Hubbard 
Cc: Boris Ostrovsky 
Cc: Paul Durrant 

Souptick Joarder (3):
  xen/privcmd: Corrected error handling path
  xen/privcmd: Mark pages as dirty
  xen/privcmd: Convert get_user_pages*() to  pin_user_pages*()

 drivers/xen/privcmd.c | 32 ++--
 1 file changed, 14 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v3 4/4] staging: kpc2000: kpc_dma: Remove additional goto statements

[Souptick Joarder]

As 3 goto level referring to same common code, those can be
accomodated with a single goto level and renameing it to
unpin_pages. Set the -ERRNO when returning partial mapped
pages in more appropriate place.

When dma_map_sg() failed, the previously allocated memory was
not freed properly. This is corrected now.

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Bharath Vedartham 
Cc: Dan Carpenter 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 25 +
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8cd20ad..dd716edd 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -35,7 +35,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
unsigned long iov_base, size_t iov_len)
 {
unsigned int i = 0;
-   int rv = 0;
+   int rv = 0, nr_pages = 0;
struct kpc_dma_device *ldev;
struct aio_cb_data *acd;
DECLARE_COMPLETION_ONSTACK(done);
@@ -79,22 +79,27 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, 
acd->user_pages, NULL);
mmap_read_unlock(current->mm);/*  release the semaphore */
if (rv != acd->page_count) {
+   nr_pages = rv;
+   if (rv > 0)
+   rv = -EFAULT;
+
dev_err(>ldev->pldev->dev, "Couldn't pin_user_pages 
(%d)\n", rv);
-   goto err_get_user_pages;
+   goto unpin_pages;
}
+   nr_pages = acd->page_count;
 
// Allocate and setup the sg_table (scatterlist entries)
rv = sg_alloc_table_from_pages(>sgt, acd->user_pages, 
acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
if (rv) {
dev_err(>ldev->pldev->dev, "Couldn't alloc sg_table 
(%d)\n", rv);
-   goto err_alloc_sg_table;
+   goto unpin_pages;
}
 
// Setup the DMA mapping for all the sg entries
acd->mapped_entry_count = dma_map_sg(>pldev->dev, acd->sgt.sgl, 
acd->sgt.nents, ldev->dir);
if (acd->mapped_entry_count <= 0) {
dev_err(>ldev->pldev->dev, "Couldn't dma_map_sg (%d)\n", 
acd->mapped_entry_count);
-   goto err_dma_map_sg;
+   goto free_table;
}
 
// Calculate how many descriptors are actually needed for this transfer.
@@ -186,16 +191,12 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
  err_descr_too_many:
unlock_engine(ldev);
dma_unmap_sg(>pldev->dev, acd->sgt.sgl, acd->sgt.nents, 
ldev->dir);
+ free_table:
sg_free_table(>sgt);
- err_dma_map_sg:
- err_alloc_sg_table:
-   unpin_user_pages(acd->user_pages, acd->page_count);
 
- err_get_user_pages:
-   if (rv > 0) {
-   unpin_user_pages(acd->user_pages, rv);
-   rv = -EFAULT;
-   }
+ unpin_pages:
+   if (nr_pages > 0)
+   unpin_user_pages(acd->user_pages, nr_pages);
kfree(acd->user_pages);
  err_alloc_userpages:
kfree(acd);
-- 
1.9.1

[PATCH v3 0/4] staging: kpc2000: kpc_dma: Few clean up and Convert to pin_user_pages()

[Souptick Joarder]

This series contains few clean up, minor bug fixes and
Convert get_user_pages() to pin_user_pages().

I'm compile tested this, but unable to run-time test,
so any testing help is much appriciated.

v2:
Address Dan's review comments to return -ERRNO for partially
mapped pages and changed the other patches in series accordingly.
Minor update in change logs.

v3:
Address review comment to invoke the right goto level when allocation
failed in patch[4/4].

Cc: John Hubbard 
Cc: Bharath Vedartham 
Cc: Dan Carpenter 


Souptick Joarder (4):
  staging: kpc2000: kpc_dma: Unpin partial pinned pages
  staging: kpc2000: kpc_dma: Convert set_page_dirty() --> 
set_page_dirty_lock()
  staging: kpc2000: kpc_dma: Convert get_user_pages() --> 
pin_user_pages()
  staging: kpc2000: kpc_dma: Remove additional goto statements

 drivers/staging/kpc2000/kpc_dma/fileops.c | 39 +--
 1 file changed, 21 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v3 2/4] staging: kpc2000: kpc_dma: Convert set_page_dirty() --> set_page_dirty_lock()

[Souptick Joarder]

First, convert set_page_dirty() to set_page_dirty_lock()

Second, there is an interval in there after set_page_dirty() and
before put_page(), in which the device could be running and setting
pages dirty. Moving set_page_dirty_lock() after dma_unmap_sg().

Signed-off-by: Souptick Joarder 
Suggested-by: John Hubbard 
Cc: John Hubbard 
Cc: Bharath Vedartham 
Cc: Dan Carpenter 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index becdb41..08d90a6 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -215,13 +215,13 @@ void  transfer_complete_cb(struct aio_cb_data *acd, 
size_t xfr_count, u32 flags)
BUG_ON(!acd->ldev);
BUG_ON(!acd->ldev->pldev);
 
+   dma_unmap_sg(>ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, 
acd->ldev->dir);
+
for (i = 0 ; i < acd->page_count ; i++) {
if (!PageReserved(acd->user_pages[i]))
-   set_page_dirty(acd->user_pages[i]);
+   set_page_dirty_lock(acd->user_pages[i]);
}
 
-   dma_unmap_sg(>ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, 
acd->ldev->dir);
-
for (i = 0 ; i < acd->page_count ; i++)
put_page(acd->user_pages[i]);
 
-- 
1.9.1

[PATCH v3 3/4] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 2 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Bharath Vedartham 
Cc: Dan Carpenter 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 08d90a6..8cd20ad 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -76,10 +76,10 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 
// Lock the user buffer pages in memory, and hold on to the page 
pointers (for the sglist)
mmap_read_lock(current->mm);  /*  get memory map semaphore */
-   rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE 
| FOLL_GET, acd->user_pages, NULL);
+   rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, 
acd->user_pages, NULL);
mmap_read_unlock(current->mm);/*  release the semaphore */
if (rv != acd->page_count) {
-   dev_err(>ldev->pldev->dev, "Couldn't get_user_pages 
(%d)\n", rv);
+   dev_err(>ldev->pldev->dev, "Couldn't pin_user_pages 
(%d)\n", rv);
goto err_get_user_pages;
}
 
@@ -189,13 +189,11 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
sg_free_table(>sgt);
  err_dma_map_sg:
  err_alloc_sg_table:
-   for (i = 0 ; i < acd->page_count ; i++)
-   put_page(acd->user_pages[i]);
+   unpin_user_pages(acd->user_pages, acd->page_count);
 
  err_get_user_pages:
if (rv > 0) {
-   for (i = 0; i < rv; i++)
-   put_pages(acd->user_pages[i]);
+   unpin_user_pages(acd->user_pages, rv);
rv = -EFAULT;
}
kfree(acd->user_pages);
@@ -222,8 +220,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t 
xfr_count, u32 flags)
set_page_dirty_lock(acd->user_pages[i]);
}
 
-   for (i = 0 ; i < acd->page_count ; i++)
-   put_page(acd->user_pages[i]);
+   unpin_user_pages(acd->user_pages, acd->page_count);
 
sg_free_table(>sgt);
 
-- 
1.9.1

[PATCH v3 1/4] staging: kpc2000: kpc_dma: Unpin partial pinned pages

[Souptick Joarder]

There is a bug, when get_user_pages() failed but partially pinned
pages are not unpinned and positive numbers are returned instead of
-ERRNO. Fixed it.

Also, int is more appropriate type for rv. Changed it.

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Dan Carpenter 
Cc: Bharath Vedartham 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8975346..becdb41 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -35,7 +35,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
unsigned long iov_base, size_t iov_len)
 {
unsigned int i = 0;
-   long rv = 0;
+   int rv = 0;
struct kpc_dma_device *ldev;
struct aio_cb_data *acd;
DECLARE_COMPLETION_ONSTACK(done);
@@ -79,14 +79,14 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE 
| FOLL_GET, acd->user_pages, NULL);
mmap_read_unlock(current->mm);/*  release the semaphore */
if (rv != acd->page_count) {
-   dev_err(>ldev->pldev->dev, "Couldn't get_user_pages 
(%ld)\n", rv);
+   dev_err(>ldev->pldev->dev, "Couldn't get_user_pages 
(%d)\n", rv);
goto err_get_user_pages;
}
 
// Allocate and setup the sg_table (scatterlist entries)
rv = sg_alloc_table_from_pages(>sgt, acd->user_pages, 
acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
if (rv) {
-   dev_err(>ldev->pldev->dev, "Couldn't alloc sg_table 
(%ld)\n", rv);
+   dev_err(>ldev->pldev->dev, "Couldn't alloc sg_table 
(%d)\n", rv);
goto err_alloc_sg_table;
}
 
@@ -193,10 +193,15 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
put_page(acd->user_pages[i]);
 
  err_get_user_pages:
+   if (rv > 0) {
+   for (i = 0; i < rv; i++)
+   put_pages(acd->user_pages[i]);
+   rv = -EFAULT;
+   }
kfree(acd->user_pages);
  err_alloc_userpages:
kfree(acd);
-   dev_dbg(>ldev->pldev->dev, "%s returning with error %ld\n", 
__func__, rv);
+   dev_dbg(>ldev->pldev->dev, "%s returning with error %d\n", 
__func__, rv);
return rv;
 }
 
-- 
1.9.1

[PATCH v2 3/4] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 2 as per document [1].

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Bharath Vedartham 
Cc: Dan Carpenter 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 08d90a6..8cd20ad 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -76,10 +76,10 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 
// Lock the user buffer pages in memory, and hold on to the page 
pointers (for the sglist)
mmap_read_lock(current->mm);  /*  get memory map semaphore */
-   rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE 
| FOLL_GET, acd->user_pages, NULL);
+   rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, 
acd->user_pages, NULL);
mmap_read_unlock(current->mm);/*  release the semaphore */
if (rv != acd->page_count) {
-   dev_err(>ldev->pldev->dev, "Couldn't get_user_pages 
(%d)\n", rv);
+   dev_err(>ldev->pldev->dev, "Couldn't pin_user_pages 
(%d)\n", rv);
goto err_get_user_pages;
}
 
@@ -189,13 +189,11 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
sg_free_table(>sgt);
  err_dma_map_sg:
  err_alloc_sg_table:
-   for (i = 0 ; i < acd->page_count ; i++)
-   put_page(acd->user_pages[i]);
+   unpin_user_pages(acd->user_pages, acd->page_count);
 
  err_get_user_pages:
if (rv > 0) {
-   for (i = 0; i < rv; i++)
-   put_pages(acd->user_pages[i]);
+   unpin_user_pages(acd->user_pages, rv);
rv = -EFAULT;
}
kfree(acd->user_pages);
@@ -222,8 +220,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t 
xfr_count, u32 flags)
set_page_dirty_lock(acd->user_pages[i]);
}
 
-   for (i = 0 ; i < acd->page_count ; i++)
-   put_page(acd->user_pages[i]);
+   unpin_user_pages(acd->user_pages, acd->page_count);
 
sg_free_table(>sgt);
 
-- 
1.9.1

[PATCH v2 2/4] staging: kpc2000: kpc_dma: Convert set_page_dirty() --> set_page_dirty_lock()

[Souptick Joarder]

First, convert set_page_dirty() to set_page_dirty_lock()

Second, there is an interval in there after set_page_dirty() and
before put_page(), in which the device could be running and setting
pages dirty. Moving set_page_dirty_lock() after dma_unmap_sg().

Signed-off-by: Souptick Joarder 
Suggested-by: John Hubbard 
Cc: John Hubbard 
Cc: Bharath Vedartham 
Cc: Dan Carpenter 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index becdb41..08d90a6 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -215,13 +215,13 @@ void  transfer_complete_cb(struct aio_cb_data *acd, 
size_t xfr_count, u32 flags)
BUG_ON(!acd->ldev);
BUG_ON(!acd->ldev->pldev);
 
+   dma_unmap_sg(>ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, 
acd->ldev->dir);
+
for (i = 0 ; i < acd->page_count ; i++) {
if (!PageReserved(acd->user_pages[i]))
-   set_page_dirty(acd->user_pages[i]);
+   set_page_dirty_lock(acd->user_pages[i]);
}
 
-   dma_unmap_sg(>ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, 
acd->ldev->dir);
-
for (i = 0 ; i < acd->page_count ; i++)
put_page(acd->user_pages[i]);
 
-- 
1.9.1

[PATCH v2 4/4] staging: kpc2000: kpc_dma: Remove additional goto statements

[Souptick Joarder]

As 3 goto level referring to same common code, those can be
accomodated with a single goto level and renameing it to
unpin_pages. Set the -ERRNO when returning partial mapped
pages in more appropriate place.

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Bharath Vedartham 
Cc: Dan Carpenter 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8cd20ad..d21a4fd 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -35,7 +35,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
unsigned long iov_base, size_t iov_len)
 {
unsigned int i = 0;
-   int rv = 0;
+   int rv = 0, nr_pages = 0;
struct kpc_dma_device *ldev;
struct aio_cb_data *acd;
DECLARE_COMPLETION_ONSTACK(done);
@@ -79,22 +79,27 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, 
acd->user_pages, NULL);
mmap_read_unlock(current->mm);/*  release the semaphore */
if (rv != acd->page_count) {
+   nr_pages = rv;
+   if (rv > 0)
+   rv = -EFAULT;
+
dev_err(>ldev->pldev->dev, "Couldn't pin_user_pages 
(%d)\n", rv);
-   goto err_get_user_pages;
+   goto unpin_pages;
}
+   nr_pages = acd->page_count;
 
// Allocate and setup the sg_table (scatterlist entries)
rv = sg_alloc_table_from_pages(>sgt, acd->user_pages, 
acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
if (rv) {
dev_err(>ldev->pldev->dev, "Couldn't alloc sg_table 
(%d)\n", rv);
-   goto err_alloc_sg_table;
+   goto unpin_pages;
}
 
// Setup the DMA mapping for all the sg entries
acd->mapped_entry_count = dma_map_sg(>pldev->dev, acd->sgt.sgl, 
acd->sgt.nents, ldev->dir);
if (acd->mapped_entry_count <= 0) {
dev_err(>ldev->pldev->dev, "Couldn't dma_map_sg (%d)\n", 
acd->mapped_entry_count);
-   goto err_dma_map_sg;
+   goto unpin_pages;
}
 
// Calculate how many descriptors are actually needed for this transfer.
@@ -187,15 +192,10 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
unlock_engine(ldev);
dma_unmap_sg(>pldev->dev, acd->sgt.sgl, acd->sgt.nents, 
ldev->dir);
sg_free_table(>sgt);
- err_dma_map_sg:
- err_alloc_sg_table:
-   unpin_user_pages(acd->user_pages, acd->page_count);
 
- err_get_user_pages:
-   if (rv > 0) {
-   unpin_user_pages(acd->user_pages, rv);
-   rv = -EFAULT;
-   }
+ unpin_pages:
+   if (nr_pages > 0)
+   unpin_user_pages(acd->user_pages, nr_pages);
kfree(acd->user_pages);
  err_alloc_userpages:
kfree(acd);
-- 
1.9.1

[PATCH v2 0/4] staging: kpc2000: kpc_dma: Few clean up and Convert to pin_user_pages()

[Souptick Joarder]

This series contains few clean up, minor bug fixes and
Convert get_user_pages() to pin_user_pages().

I'm compile tested this, but unable to run-time test,
so any testing help is much appriciated.

v2:
Address Dan's review comments to return -ERRNO for partially
mapped pages and changed the other patches in series accordingly.
Minor update in change logs.

Cc: John Hubbard 
Cc: Bharath Vedartham 
Cc: Dan Carpenter 


Souptick Joarder (4):
  staging: kpc2000: kpc_dma: Unpin partial pinned pages
  staging: kpc2000: kpc_dma: Convert set_page_dirty() --> 
set_page_dirty_lock()
  staging: kpc2000: kpc_dma: Convert get_user_pages() --> 
pin_user_pages()
  staging: kpc2000: kpc_dma: Remove additional goto statements

 drivers/staging/kpc2000/kpc_dma/fileops.c | 38 ---
 1 file changed, 20 insertions(+), 18 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] staging: kpc2000: kpc_dma: Unpin partial pinned pages

[Souptick Joarder]

There is a bug, when get_user_pages() failed but partially pinned
pages are not unpinned and positive numbers are returned instead of
-ERRNO. Fixed it.

Also, int is more appropriate type for rv. Changed it.

Signed-off-by: Souptick Joarder 
Cc: John Hubbard 
Cc: Dan Carpenter 
Cc: Bharath Vedartham 
---
 drivers/staging/kpc2000/kpc_dma/fileops.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8975346..becdb41 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -35,7 +35,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
unsigned long iov_base, size_t iov_len)
 {
unsigned int i = 0;
-   long rv = 0;
+   int rv = 0;
struct kpc_dma_device *ldev;
struct aio_cb_data *acd;
DECLARE_COMPLETION_ONSTACK(done);
@@ -79,14 +79,14 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE 
| FOLL_GET, acd->user_pages, NULL);
mmap_read_unlock(current->mm);/*  release the semaphore */
if (rv != acd->page_count) {
-   dev_err(>ldev->pldev->dev, "Couldn't get_user_pages 
(%ld)\n", rv);
+   dev_err(>ldev->pldev->dev, "Couldn't get_user_pages 
(%d)\n", rv);
goto err_get_user_pages;
}
 
// Allocate and setup the sg_table (scatterlist entries)
rv = sg_alloc_table_from_pages(>sgt, acd->user_pages, 
acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
if (rv) {
-   dev_err(>ldev->pldev->dev, "Couldn't alloc sg_table 
(%ld)\n", rv);
+   dev_err(>ldev->pldev->dev, "Couldn't alloc sg_table 
(%d)\n", rv);
goto err_alloc_sg_table;
}
 
@@ -193,10 +193,15 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
put_page(acd->user_pages[i]);
 
  err_get_user_pages:
+   if (rv > 0) {
+   for (i = 0; i < rv; i++)
+   put_pages(acd->user_pages[i]);
+   rv = -EFAULT;
+   }
kfree(acd->user_pages);
  err_alloc_userpages:
kfree(acd);
-   dev_dbg(>ldev->pldev->dev, "%s returning with error %ld\n", 
__func__, rv);
+   dev_dbg(>ldev->pldev->dev, "%s returning with error %d\n", 
__func__, rv);
return rv;
 }
 
-- 
1.9.1