Re: [PATCH v2 1/2] staging: wilc1000: fix always return 0 error

[Souptick Joarder]

HI Glen,

On Thu, Dec 24, 2015 at 11:32 AM, Glen Lee  wrote:
> This patch fixes a bug that return always 0 so it fails every time.
>
> Fixes: c1af9db78950 ("staging: wilc1000: call linux_sdio_init instead of 
> io_init")
> Signed-off-by: Glen Lee 
> ---
> Changes in v2: separate v1 patch into two patches.
> ---
>  drivers/staging/wilc1000/wilc_sdio.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_sdio.c 
> b/drivers/staging/wilc1000/wilc_sdio.c
> index e961b50..464d27d 100644
> --- a/drivers/staging/wilc1000/wilc_sdio.c
> +++ b/drivers/staging/wilc1000/wilc_sdio.c
> @@ -614,8 +614,6 @@ static int sdio_init(struct wilc *wilc)
> if (!wilc_sdio_init()) {
> dev_err(>dev, "Failed io init bus...\n");
> return 0;
> -   } else {
> -   return 0;
> }

I think it's better to handle the error case properly when
wilc_sdio_init() call fails.

>
> /**
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-Souptick
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 17/20] staging: wilc1000: fix return type of host_int_get_ipaddress

[Souptick Joarder]

Hi Lim,

On Wed, Dec 30, 2015 at 5:45 PM, Chaehyun Lim  wrote:
> This patch changes return type of host_int_get_ipaddress from s32 to
> int. The result variable gets return value from wilc_mq_send that has
> data type of int. It should be changed return type of this function as
> well as data type of result variable.
>
> Signed-off-by: Chaehyun Lim 
> ---
>  drivers/staging/wilc1000/host_interface.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/host_interface.c 
> b/drivers/staging/wilc1000/host_interface.c
> index 90fdbdd..a203647 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -362,7 +362,7 @@ static s32 handle_set_operation_mode(struct wilc_vif *vif,
> return result;
>  }
>
> -static s32 host_int_get_ipaddress(struct wilc_vif *vif,
> +static int host_int_get_ipaddress(struct wilc_vif *vif,
>   struct host_if_drv *hif_drv,
>   u8 *u16ipadd, u8 idx);
>
s32 and int both are same. isn't it ?

> @@ -4664,11 +4664,11 @@ int wilc_setup_ipaddress(struct wilc_vif *vif, u8 
> *ip_addr, u8 idx)
> return result;
>  }
>
> -static s32 host_int_get_ipaddress(struct wilc_vif *vif,
> +static int host_int_get_ipaddress(struct wilc_vif *vif,
>   struct host_if_drv *hif_drv,
>   u8 *u16ipadd, u8 idx)
>  {
> -   s32 result = 0;
> +   int result = 0;
> struct host_if_msg msg;
>
> if (!hif_drv) {
> --
> 2.6.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-Souptick
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] rtlwifi: Change long delays to sleeps

[Souptick Joarder]

On Tue, Feb 16, 2016 at 3:42 AM, Larry Finger  wrote:
> Routine rtl_addr_delay() uses delay statements in code that can
> sleep. To improve system responsiveness, the various delay statements
> are changed.
>
> In addition, routines rtl_rfreg_delay() and rtl_bb_delay() are
> rewritten to use the code in rtl_addr_delay() for most of their
> input values.
>
> Suggested-by: Byeoungwook Kim 
> Signed-off-by: Larry Finger 
> ---
>
> Kalle,
>
> This patch will interfere with a set of 3 patches submitted by Byeoungwook Kim
>  on Feb. 3, 2016 that are currently in the deferred list
> at patchwork.
>
> Larry
>
>  drivers/net/wireless/realtek/rtlwifi/core.c | 44 
> -
>  1 file changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c 
> b/drivers/net/wireless/realtek/rtlwifi/core.c
> index 02eba0e..16ad0d6 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
> @@ -54,59 +54,39 @@ EXPORT_SYMBOL(channel5g_80m);
>  void rtl_addr_delay(u32 addr)
>  {
> if (addr == 0xfe)
> -   mdelay(50);
> +   msleep(50);
> else if (addr == 0xfd)
> -   mdelay(5);
> +   msleep(5);
> else if (addr == 0xfc)
> -   mdelay(1);
> +   msleep(1);
> else if (addr == 0xfb)
> -   udelay(50);
> +   usleep_range(50, 100);
> else if (addr == 0xfa)
> -   udelay(5);
> +   usleep_range(5, 10);
> else if (addr == 0xf9)
> -   udelay(1);
> +   usleep_range(1, 2);

 why udelay is replaced by usleep_range?
>  }
>  EXPORT_SYMBOL(rtl_addr_delay);
>
>  void rtl_rfreg_delay(struct ieee80211_hw *hw, enum radio_path rfpath, u32 
> addr,
>  u32 mask, u32 data)
>  {
> -   if (addr == 0xfe) {
> -   mdelay(50);
> -   } else if (addr == 0xfd) {
> -   mdelay(5);
> -   } else if (addr == 0xfc) {
> -   mdelay(1);
> -   } else if (addr == 0xfb) {
> -   udelay(50);
> -   } else if (addr == 0xfa) {
> -   udelay(5);
> -   } else if (addr == 0xf9) {
> -   udelay(1);
> +   if (addr >= 0xf9 && addr <= 0xfe) {
> +   rtl_addr_delay(addr);
> } else {
> rtl_set_rfreg(hw, rfpath, addr, mask, data);
> -   udelay(1);
> +   usleep_range(1, 2);
> }
>  }
>  EXPORT_SYMBOL(rtl_rfreg_delay);
>
>  void rtl_bb_delay(struct ieee80211_hw *hw, u32 addr, u32 data)
>  {
> -   if (addr == 0xfe) {
> -   mdelay(50);
> -   } else if (addr == 0xfd) {
> -   mdelay(5);
> -   } else if (addr == 0xfc) {
> -   mdelay(1);
> -   } else if (addr == 0xfb) {
> -   udelay(50);
> -   } else if (addr == 0xfa) {
> -   udelay(5);
> -   } else if (addr == 0xf9) {
> -   udelay(1);
> +   if (addr >= 0xf9 && addr <= 0xfe) {
> +   rtl_addr_delay(addr);
> } else {
> rtl_set_bbreg(hw, addr, MASKDWORD, data);
> -   udelay(1);
> +   usleep_range(1, 2);
> }
>  }
>  EXPORT_SYMBOL(rtl_bb_delay);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-Souptick
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging :rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

There are few functions where we need to free previously allocated memory
when kmalloc fails. Else it may lead to memory leakage.
In  _init_cmd_priv() and _r8712_init_xmit_priv(),in few places we are not
freeing previously allocated memory  when kmalloc fails.
This patch will address it.

Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
---
 drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
 drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
b/drivers/staging/rtl8712/rtl871x_cmd.c
index b7ee5e6..04638f1 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -72,8 +72,11 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
((addr_t)(pcmdpriv->cmd_allocated_buf) &
(CMDBUFF_ALIGN_SZ - 1));
pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
-   if (!pcmdpriv->rsp_allocated_buf)
+   if (!pcmdpriv->rsp_allocated_buf) {
+   kfree(pcmdpriv->cmd_allocated_buf);
return _FAIL;
+   }
pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
pcmdpriv->cmd_issued_cnt = 0;
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
b/drivers/staging/rtl8712/rtl871x_xmit.c
index be38364..484d2f2 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -128,8 +128,11 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
_init_queue(>pending_xmitbuf_queue);
pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
xmit_buf) + 4,
GFP_ATOMIC);
-   if (!pxmitpriv->pallocated_xmitbuf)
+   if (!pxmitpriv->pallocated_xmitbuf) {
+   kfree(pxmitpriv->pallocated_frame_buf);
return _FAIL;
+   }
pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
  ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

There are few functions where we need to free previously allocated memory
when kmalloc fails.Else it may lead to memory leakage.In  _init_cmd_priv()
and _r8712_init_xmit_priv(),in few places we are not freeing previously
allocated memory when kmalloc fails.

Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
---
Changes in v2:
 -Make the commit message more cleaner.

 drivers/staging/rtl8712/rtl871x_cmd.c  | 4 +++-
 drivers/staging/rtl8712/rtl871x_xmit.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
b/drivers/staging/rtl8712/rtl871x_cmd.c
index b7ee5e6..1f284ae 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -72,8 +72,10 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
((addr_t)(pcmdpriv->cmd_allocated_buf) &
(CMDBUFF_ALIGN_SZ - 1));
pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
-   if (!pcmdpriv->rsp_allocated_buf)
+   if (!pcmdpriv->rsp_allocated_buf) {
+   kfree(pcmdpriv->cmd_allocated_buf);
+   pcmdpriv->cmd_allocated_buf = NULL;
return _FAIL;
+   }
pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
pcmdpriv->cmd_issued_cnt = 0;
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
b/drivers/staging/rtl8712/rtl871x_xmit.c
index be38364..f335161 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -128,8 +128,10 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
_init_queue(>pending_xmitbuf_queue);
pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
xmit_buf) + 4,
GFP_ATOMIC);
-   if (!pxmitpriv->pallocated_xmitbuf)
+   if (!pxmitpriv->pallocated_xmitbuf) {
+   kfree(pxmitpriv->pallocated_frame_buf);
+   pxmitpriv->pallocated_frame_buf = NULL;
return _FAIL;
+   }
pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
  ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging :rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

On Thu, Oct 27, 2016 at 11:20 AM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Thu, Oct 27, 2016 at 11:10:09AM +0530, Souptick Joarder wrote:
>> There are few functions where we need to free previously allocated memory
>> when kmalloc fails. Else it may lead to memory leakage.
>> In  _init_cmd_priv() and _r8712_init_xmit_priv(),in few places we are not
>> freeing previously allocated memory  when kmalloc fails.
>
> Odd use of spaces, can you fix that up?
>
>> This patch will address it.
>
> That last sentance is not needed.

Ok, I will do those changes.
>
> And isn't this a new version of a patch, or am I mistaken?  If so,
> please properly version it as SubmittingPatches describes to do.

This is the new version of previous patch.
>
>>
>> Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
>> ---
>>  drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
>>  drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
>> b/drivers/staging/rtl8712/rtl871x_cmd.c
>> index b7ee5e6..04638f1 100644
>> --- a/drivers/staging/rtl8712/rtl871x_cmd.c
>> +++ b/drivers/staging/rtl8712/rtl871x_cmd.c
>> @@ -72,8 +72,11 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
>>   ((addr_t)(pcmdpriv->cmd_allocated_buf) &
>>   (CMDBUFF_ALIGN_SZ - 1));
>>   pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
>> - if (!pcmdpriv->rsp_allocated_buf)
>> + if (!pcmdpriv->rsp_allocated_buf) {
>> + kfree(pcmdpriv->cmd_allocated_buf);
>>   return _FAIL;
>> + }
>
> If you need to set the buffer to NULL, please do so, last time I thought
> you said it was required, is it really not?

This NULL is required. But I assume, last time you are not agree with it :)
So I remove it in new version.

Anyway I will  properly version  it.

>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

On Thu, Oct 27, 2016 at 6:40 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Thu, Oct 27, 2016 at 01:46:13PM +0530, Souptick Joarder wrote:
>> There are few functions where we need to free previously allocated memory
>> when kmalloc fails.Else it may lead to memory leakage.In  _init_cmd_priv()
>> and _r8712_init_xmit_priv(),in few places we are not freeing previously
>> allocated memory when kmalloc fails.
>>
>> Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
>> ---
>> Changes in v2:
>>  -Make the commit message more cleaner.
>
> I'll be pedantic here, but please always put a ' ' after a '.' or a ','

I didn't get which part exactly you are referring.
Are you referring to the patch description and asking me to put  '  '
after a '.'  or  ','

> It's the copy-editor in me coming out...
>
> You couldn't do something like that on a term-paper and expect to not
> get it marked down, right?

My apology here.
>
> another try?
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Free memory and return failure when kmalloc fails

[Souptick Joarder]

Hi Larry, Greg,

On Thu, Oct 20, 2016 at 12:29 PM, Souptick Joarder <jrdr.li...@gmail.com> wrote:
> This patch is added to free memory and return failure when kmalloc fails
>
> Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
> ---
>  drivers/staging/rtl8712/os_intfs.c | 3 ++-
>  drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
>  drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/os_intfs.c 
> b/drivers/staging/rtl8712/os_intfs.c
> index cbe4de0..aab3141 100644
> --- a/drivers/staging/rtl8712/os_intfs.c
> +++ b/drivers/staging/rtl8712/os_intfs.c
> @@ -313,7 +313,8 @@ u8 r8712_init_drv_sw(struct _adapter *padapter)
> return _FAIL;
> if (r8712_init_mlme_priv(padapter) == _FAIL)
> return _FAIL;
> -   _r8712_init_xmit_priv(>xmitpriv, padapter);
> +   if ((_r8712_init_xmit_priv(>xmitpriv, padapter)) != 
> _SUCCESS)
> +   return _FAIL;
> _r8712_init_recv_priv(>recvpriv, padapter);
> memset((unsigned char *)>securitypriv, 0,
>sizeof(struct security_priv));
> diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
> b/drivers/staging/rtl8712/rtl871x_cmd.c
> index b7ee5e6..04638f1 100644
> --- a/drivers/staging/rtl8712/rtl871x_cmd.c
> +++ b/drivers/staging/rtl8712/rtl871x_cmd.c
> @@ -72,8 +72,11 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
> ((addr_t)(pcmdpriv->cmd_allocated_buf) &
> (CMDBUFF_ALIGN_SZ - 1));
> pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
> -   if (!pcmdpriv->rsp_allocated_buf)
> +   if (!pcmdpriv->rsp_allocated_buf) {
> +   kfree(pcmdpriv->cmd_allocated_buf);
> +   pcmdpriv->cmd_allocated_buf = NULL;
> return _FAIL;
> +   }
> pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
> ((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
> pcmdpriv->cmd_issued_cnt = 0;
> diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
> b/drivers/staging/rtl8712/rtl871x_xmit.c
> index be38364..484d2f2 100644
> --- a/drivers/staging/rtl8712/rtl871x_xmit.c
> +++ b/drivers/staging/rtl8712/rtl871x_xmit.c
> @@ -128,8 +128,11 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
> _init_queue(>pending_xmitbuf_queue);
> pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
> xmit_buf) + 4,
> GFP_ATOMIC);
> -   if (!pxmitpriv->pallocated_xmitbuf)
> +   if (!pxmitpriv->pallocated_xmitbuf) {
> +   kfree(pxmitpriv->pallocated_frame_buf);
> +   pxmitpriv->pallocated_frame_buf = NULL;
> return _FAIL;
> +   }
> pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
>   ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
> pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
> --

Any Comment for this patch ?
> 1.9.1
>

Regards
Souptick
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Free memory and return failure when kmalloc fails

[Souptick Joarder]

Hi Greg,


On Tue, Oct 25, 2016 at 2:33 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Thu, Oct 20, 2016 at 12:29:33PM +0530, Souptick Joarder wrote:
>> This patch is added to free memory and return failure when kmalloc fails
>
> I'm sorry, but I can not parse that sentance.  Can you rephrase this a
> bit better?  What exactly are you doing here?

  There are few functions where we need to free previously allocated memory
  when kmalloc fails. Else it may lead to memory leakage.
  In  _init_cmd_priv() and _r8712_init_xmit_priv() , few places we are
not freeing
  previously allocated memory  when kmalloc fails.This patch will address it.

  shall I resend the patch?

>
>>
>> Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
>> ---
>>  drivers/staging/rtl8712/os_intfs.c | 3 ++-
>>  drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
>>  drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
>>  3 files changed, 10 insertions(+), 3 deletions(-)
>
> Any reason why you didn't cc: the driverdevel mailing list?  I doubt
> linux-wireless cares about staging drivers :(

My Apology. I will add driverdevel mailing list in cc.

>
>>
>> diff --git a/drivers/staging/rtl8712/os_intfs.c 
>> b/drivers/staging/rtl8712/os_intfs.c
>> index cbe4de0..aab3141 100644
>> --- a/drivers/staging/rtl8712/os_intfs.c
>> +++ b/drivers/staging/rtl8712/os_intfs.c
>> @@ -313,7 +313,8 @@ u8 r8712_init_drv_sw(struct _adapter *padapter)
>>   return _FAIL;
>>   if (r8712_init_mlme_priv(padapter) == _FAIL)
>>   return _FAIL;
>> - _r8712_init_xmit_priv(>xmitpriv, padapter);
>> + if ((_r8712_init_xmit_priv(>xmitpriv, padapter)) != _SUCCESS)
>> + return _FAIL;
>
> You don't have to unwind anything that r8712_init_mlme_priv() did?

  I didn't get your question?

  r8712_init_drv_sw() is getting called during initialization.
  if _r8712_init_xmit_priv() fails is it required to continue driver
initialization
  or return _FAIL similar like  previous function r8712_init_mlme_priv() ?


>
>>   _r8712_init_recv_priv(>recvpriv, padapter);
>>   memset((unsigned char *)>securitypriv, 0,
>>  sizeof(struct security_priv));
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

There are few functions where we need to free previously allocated memory
when kmalloc fails. Else it may lead to memory leakage. In  _init_cmd_priv()
and _r8712_init_xmit_priv(), in few places we are not freeing previously
allocated memory when kmalloc fails.

Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
---
v2:
  - Make the commit message more descriptive
v3:
  - Modified the patch description by adding space in required places

 drivers/staging/rtl8712/rtl871x_cmd.c  | 4 +++-
 drivers/staging/rtl8712/rtl871x_xmit.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
b/drivers/staging/rtl8712/rtl871x_cmd.c
index b7ee5e6..1f284ae 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -72,8 +72,10 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
((addr_t)(pcmdpriv->cmd_allocated_buf) &
(CMDBUFF_ALIGN_SZ - 1));
pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
-   if (!pcmdpriv->rsp_allocated_buf)
+   if (!pcmdpriv->rsp_allocated_buf) {
+   kfree(pcmdpriv->cmd_allocated_buf);
+   pcmdpriv->cmd_allocated_buf = NULL;
return _FAIL;
+   }
pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
pcmdpriv->cmd_issued_cnt = 0;
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
b/drivers/staging/rtl8712/rtl871x_xmit.c
index be38364..f335161 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -128,8 +128,10 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
_init_queue(>pending_xmitbuf_queue);
pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
xmit_buf) + 4,
GFP_ATOMIC);
-   if (!pxmitpriv->pallocated_xmitbuf)
+   if (!pxmitpriv->pallocated_xmitbuf) {
+   kfree(pxmitpriv->pallocated_frame_buf);
+   pxmitpriv->pallocated_frame_buf = NULL;
return _FAIL;
+   }
pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
  ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Free memory and return failure when kmalloc fails

[Souptick Joarder]

On Tue, Oct 25, 2016 at 11:41 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Tue, Oct 25, 2016 at 10:06:48PM +0530, Souptick Joarder wrote:
>> Hi Greg,
>>
>>
>> On Tue, Oct 25, 2016 at 2:33 PM, Greg KH <gre...@linuxfoundation.org> wrote:
>> > On Thu, Oct 20, 2016 at 12:29:33PM +0530, Souptick Joarder wrote:
>> >> This patch is added to free memory and return failure when kmalloc fails
>> >
>> > I'm sorry, but I can not parse that sentance.  Can you rephrase this a
>> > bit better?  What exactly are you doing here?
>>
>>   There are few functions where we need to free previously allocated memory
>>   when kmalloc fails. Else it may lead to memory leakage.
>>   In  _init_cmd_priv() and _r8712_init_xmit_priv() , few places we are
>> not freeing
>>   previously allocated memory  when kmalloc fails.This patch will address it.
>>
>>   shall I resend the patch?
>
> Please do, it is long-gone from my queue, and put more text, like you
> write here, in the changelog area.
>
>> >> diff --git a/drivers/staging/rtl8712/os_intfs.c 
>> >> b/drivers/staging/rtl8712/os_intfs.c
>> >> index cbe4de0..aab3141 100644
>> >> --- a/drivers/staging/rtl8712/os_intfs.c
>> >> +++ b/drivers/staging/rtl8712/os_intfs.c
>> >> @@ -313,7 +313,8 @@ u8 r8712_init_drv_sw(struct _adapter *padapter)
>> >>   return _FAIL;
>> >>   if (r8712_init_mlme_priv(padapter) == _FAIL)
>> >>   return _FAIL;
>> >> - _r8712_init_xmit_priv(>xmitpriv, padapter);
>> >> + if ((_r8712_init_xmit_priv(>xmitpriv, padapter)) != 
>> >> _SUCCESS)
>> >> + return _FAIL;
>> >
>> > You don't have to unwind anything that r8712_init_mlme_priv() did?
>>
>>   I didn't get your question?
>>
>>   r8712_init_drv_sw() is getting called during initialization.
>>   if _r8712_init_xmit_priv() fails is it required to continue driver
>> initialization
>>   or return _FAIL similar like  previous function r8712_init_mlme_priv() ?
>
> Are you sure that r8712_init_mlme_priv() does not allocate anything that
> you need to now free?

  Yes you are right. We are not freeing few memories allocated in
r8712_init_mlme_priv()
  when _r8712_init_xmit_priv() return _FAIL. So we can't simply send
_FAIL without freeing
  those memories.

  I will resend the modified patch in a new mail.

>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

There are few functions where we need to free previously allocated memory
when kmalloc fails. Else it may lead to memory leakage.
In  _init_cmd_priv() and _r8712_init_xmit_priv(),in few places we are not
freeing previously allocated memory  when kmalloc fails.
This patch will address it.

Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
---
 drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
 drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
b/drivers/staging/rtl8712/rtl871x_cmd.c
index b7ee5e6..04638f1 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -72,8 +72,11 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
((addr_t)(pcmdpriv->cmd_allocated_buf) &
(CMDBUFF_ALIGN_SZ - 1));
pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
-   if (!pcmdpriv->rsp_allocated_buf)
+   if (!pcmdpriv->rsp_allocated_buf) {
+   kfree(pcmdpriv->cmd_allocated_buf);
+   pcmdpriv->cmd_allocated_buf = NULL;
return _FAIL;
+   }
pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
pcmdpriv->cmd_issued_cnt = 0;
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
b/drivers/staging/rtl8712/rtl871x_xmit.c
index be38364..484d2f2 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -128,8 +128,11 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
_init_queue(>pending_xmitbuf_queue);
pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
xmit_buf) + 4,
GFP_ATOMIC);
-   if (!pxmitpriv->pallocated_xmitbuf)
+   if (!pxmitpriv->pallocated_xmitbuf) {
+   kfree(pxmitpriv->pallocated_frame_buf);
+   pxmitpriv->pallocated_frame_buf = NULL;
return _FAIL;
+   }
pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
  ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

On Wed, Oct 26, 2016 at 12:39 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Wed, Oct 26, 2016 at 12:30:26PM +0530, Souptick Joarder wrote:
>> There are few functions where we need to free previously allocated memory
>> when kmalloc fails. Else it may lead to memory leakage.
>> In  _init_cmd_priv() and _r8712_init_xmit_priv(),in few places we are not
>> freeing previously allocated memory  when kmalloc fails.
>> This patch will address it.
>>
>> Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
>> ---
>>  drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
>>  drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
>>  2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
>> b/drivers/staging/rtl8712/rtl871x_cmd.c
>> index b7ee5e6..04638f1 100644
>> --- a/drivers/staging/rtl8712/rtl871x_cmd.c
>> +++ b/drivers/staging/rtl8712/rtl871x_cmd.c
>> @@ -72,8 +72,11 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
>>   ((addr_t)(pcmdpriv->cmd_allocated_buf) &
>>   (CMDBUFF_ALIGN_SZ - 1));
>>   pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
>> - if (!pcmdpriv->rsp_allocated_buf)
>> + if (!pcmdpriv->rsp_allocated_buf) {
>> + kfree(pcmdpriv->cmd_allocated_buf);
>> + pcmdpriv->cmd_allocated_buf = NULL;
>
> Why do you have to set this to NULL?

When _init_cmd_priv() fails  r8712_usb_dvobj_deinit() will be called
to during deinit of driver.
 r8712_usb_dvobj_deinit() is not yet implemented.

 pcmdpriv->cmd_allocated_buf is set to NULL when freed. Else after
free pcmdpriv->cmd_allocated_buf still hold some invalid address.
 So during deinit if anyone try to free again, it may lead to  stability issue.

 Correct me if I am wrong.

 Do I need to remove pcmdpriv->cmd_allocated_buf = NULL ?

>
>>   return _FAIL;
>> + }
>>   pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
>>   ((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
>>   pcmdpriv->cmd_issued_cnt = 0;
>> diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
>> b/drivers/staging/rtl8712/rtl871x_xmit.c
>> index be38364..484d2f2 100644
>> --- a/drivers/staging/rtl8712/rtl871x_xmit.c
>> +++ b/drivers/staging/rtl8712/rtl871x_xmit.c
>> @@ -128,8 +128,11 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
>>   _init_queue(>pending_xmitbuf_queue);
>>   pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
>> xmit_buf) + 4,
>>   GFP_ATOMIC);
>> - if (!pxmitpriv->pallocated_xmitbuf)
>> + if (!pxmitpriv->pallocated_xmitbuf) {
>> + kfree(pxmitpriv->pallocated_frame_buf);
>> + pxmitpriv->pallocated_frame_buf = NULL;
>
> Same here, why set to NULL?  What code relies on this?
>
same here.

Do I need to remove pxmitpriv->pallocated_frame_buf = NULL ?

> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

There are few functions where we need to free previously allocated
memory when kmalloc fails. Else it may lead to memory leakage. In
_init_cmd_priv() and _r8712_init_xmit_priv(), in few places we are
not freeing previously allocated memory when kmalloc fails.

Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
---
v2:
  - Make the commit message more descriptive
v3:
  - Modified the patch description by adding space in required places

 drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
 drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
b/drivers/staging/rtl8712/rtl871x_cmd.c
index b7ee5e6..04638f1 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -72,8 +72,11 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
((addr_t)(pcmdpriv->cmd_allocated_buf) &
(CMDBUFF_ALIGN_SZ - 1));
pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
-   if (!pcmdpriv->rsp_allocated_buf)
+   if (!pcmdpriv->rsp_allocated_buf) {
+   kfree(pcmdpriv->cmd_allocated_buf);
+   pcmdpriv->cmd_allocated_buf = NULL;
return _FAIL;
+   }
pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
pcmdpriv->cmd_issued_cnt = 0;
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
b/drivers/staging/rtl8712/rtl871x_xmit.c
index be38364..484d2f2 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -128,8 +128,11 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
_init_queue(>pending_xmitbuf_queue);
pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
xmit_buf) + 4,
GFP_ATOMIC);
-   if (!pxmitpriv->pallocated_xmitbuf)
+   if (!pxmitpriv->pallocated_xmitbuf) {
+   kfree(pxmitpriv->pallocated_frame_buf);
+   pxmitpriv->pallocated_frame_buf = NULL;
return _FAIL;
+   }
pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
  ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

On Sun, Oct 30, 2016 at 8:41 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Fri, Oct 28, 2016 at 10:37:53AM +0530, Souptick Joarder wrote:
>> There are few functions where we need to free previously allocated memory
>> when kmalloc fails. Else it may lead to memory leakage. In  _init_cmd_priv()
>> and _r8712_init_xmit_priv(), in few places we are not freeing previously
>> allocated memory when kmalloc fails.
>>
>> Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
>> ---
>> v2:
>>   - Make the commit message more descriptive
>> v3:
>>   - Modified the patch description by adding space in required places
>>
>>  drivers/staging/rtl8712/rtl871x_cmd.c  | 4 +++-
>>  drivers/staging/rtl8712/rtl871x_xmit.c | 4 +++-
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
>> b/drivers/staging/rtl8712/rtl871x_cmd.c
>> index b7ee5e6..1f284ae 100644
>> --- a/drivers/staging/rtl8712/rtl871x_cmd.c
>> +++ b/drivers/staging/rtl8712/rtl871x_cmd.c
>> @@ -72,8 +72,10 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
>>   ((addr_t)(pcmdpriv->cmd_allocated_buf) &
>>   (CMDBUFF_ALIGN_SZ - 1));
>>   pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
>> - if (!pcmdpriv->rsp_allocated_buf)
>> + if (!pcmdpriv->rsp_allocated_buf) {
>> + kfree(pcmdpriv->cmd_allocated_buf);
>> + pcmdpriv->cmd_allocated_buf = NULL;
>>   return _FAIL;
>> + }
>>   pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
>>   ((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
>>   pcmdpriv->cmd_issued_cnt = 0;
>> diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
>> b/drivers/staging/rtl8712/rtl871x_xmit.c
>> index be38364..f335161 100644
>> --- a/drivers/staging/rtl8712/rtl871x_xmit.c
>> +++ b/drivers/staging/rtl8712/rtl871x_xmit.c
>> @@ -128,8 +128,10 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
>>   _init_queue(>pending_xmitbuf_queue);
>>   pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
>> xmit_buf) + 4,
>>   GFP_ATOMIC);
>> - if (!pxmitpriv->pallocated_xmitbuf)
>> + if (!pxmitpriv->pallocated_xmitbuf) {
>> + kfree(pxmitpriv->pallocated_frame_buf);
>> + pxmitpriv->pallocated_frame_buf = NULL;
>>   return _FAIL;
>> + }
>>   pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
>> ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
>>   pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
>> --
>> 1.9.1
>
> This patch is totally corrupted, how did you generate it?

I edited the patch file.

> You can't hand-edit a patch file, unless you know how to do it...

I am not aware of it. Is there any guidelines to hand-edit a patch file?
>
> Please fix up and resend in a format I can apply it in.

I will fix and send it to you.
>
> thanks,
>
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v4] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

There are few functions where we need to free previously allocated
memory when kmalloc fails. Else it may lead to memory leakage. In
_init_cmd_priv() and _r8712_init_xmit_priv(), in few places we are
not freeing previously allocated memory when kmalloc fails.

Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
---
v2:
  - Make the commit message more descriptive
v3:
  - Modified the patch description by adding space in required places
v4:
  - patch v3 was broken due to hand-edit in patch file. Corrected it.

 drivers/staging/rtl8712/rtl871x_cmd.c  | 5 -
 drivers/staging/rtl8712/rtl871x_xmit.c | 5 -
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c 
b/drivers/staging/rtl8712/rtl871x_cmd.c
index b7ee5e6..04638f1 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.c
+++ b/drivers/staging/rtl8712/rtl871x_cmd.c
@@ -72,8 +72,11 @@ static sint _init_cmd_priv(struct cmd_priv *pcmdpriv)
((addr_t)(pcmdpriv->cmd_allocated_buf) &
(CMDBUFF_ALIGN_SZ - 1));
pcmdpriv->rsp_allocated_buf = kmalloc(MAX_RSPSZ + 4, GFP_ATOMIC);
-   if (!pcmdpriv->rsp_allocated_buf)
+   if (!pcmdpriv->rsp_allocated_buf) {
+   kfree(pcmdpriv->cmd_allocated_buf);
+   pcmdpriv->cmd_allocated_buf = NULL;
return _FAIL;
+   }
pcmdpriv->rsp_buf = pcmdpriv->rsp_allocated_buf  +  4 -
((addr_t)(pcmdpriv->rsp_allocated_buf) & 3);
pcmdpriv->cmd_issued_cnt = 0;
diff --git a/drivers/staging/rtl8712/rtl871x_xmit.c 
b/drivers/staging/rtl8712/rtl871x_xmit.c
index be38364..484d2f2 100644
--- a/drivers/staging/rtl8712/rtl871x_xmit.c
+++ b/drivers/staging/rtl8712/rtl871x_xmit.c
@@ -128,8 +128,11 @@ sint _r8712_init_xmit_priv(struct xmit_priv *pxmitpriv,
_init_queue(>pending_xmitbuf_queue);
pxmitpriv->pallocated_xmitbuf = kmalloc(NR_XMITBUFF * sizeof(struct 
xmit_buf) + 4,
GFP_ATOMIC);
-   if (!pxmitpriv->pallocated_xmitbuf)
+   if (!pxmitpriv->pallocated_xmitbuf) {
+   kfree(pxmitpriv->pallocated_frame_buf);
+   pxmitpriv->pallocated_frame_buf = NULL;
return _FAIL;
+   }
pxmitpriv->pxmitbuf = pxmitpriv->pallocated_xmitbuf + 4 -
  ((addr_t)(pxmitpriv->pallocated_xmitbuf) & 3);
pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmitbuf;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging : rtl8712: Free memory when kmalloc fails

[Souptick Joarder]

H Greg,

On Mon, Oct 31, 2016 at 3:37 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Mon, Oct 31, 2016 at 02:32:39PM +0530, Souptick Joarder wrote:
>> There are few functions where we need to free previously allocated
>> memory when kmalloc fails. Else it may lead to memory leakage. In
>> _init_cmd_priv() and _r8712_init_xmit_priv(), in few places we are
>> not freeing previously allocated memory when kmalloc fails.
>>
>> Signed-off-by: Souptick joarder <jrdr.li...@gmail.com>
>> ---
>> v2:
>>   - Make the commit message more descriptive
>> v3:
>>   - Modified the patch description by adding space in required places
>
> Why did you resend the broken v3 patch?

I am getting confused here. Do I need to update the change log again to v4?
The last patch which I send today is the latest patch file from my side.
I tried to apply it locally and it's working fine.

I am not clear what is the mistake I am doing ?
It will be helpful if you explain a bit that I will not repeat the
same mistake again.
>
> This is getting really annoying, what would you do in my situation?...

I am extremely sorry for causing this unnecessary overhead.
It was not intentional.  Sorry once again.

As this is really getting worse, Shall I drop this patch , rewrite
this patch and summit it as a completely new patch ?

Regards
Souptick
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Souptick Joarder]

On Wed, Nov 29, 2017 at 4:00 PM, Souptick Joarder <jrdr.li...@gmail.com> wrote:
> On Wed, Nov 29, 2017 at 6:25 AM, David Daney <david.da...@cavium.com> wrote:
>> From: Carlos Munoz <cmu...@cavium.com>
>>
>> The Cavium OCTEON cn78xx and cn73xx SoCs have network packet I/O
>> hardware that is significantly different from previous generations of
>> the family.

>> diff --git a/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c 
>> b/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c
>> new file mode 100644
>> index ..4dad35fa4270
>> --- /dev/null
>> +++ b/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c
>> @@ -0,0 +1,2033 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2017 Cavium, Inc.
>> + *
>> + * This file is subject to the terms and conditions of the GNU General 
>> Public
>> + * License.  See the file "COPYING" in the main directory of this archive
>> + * for more details.
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +

>> +static void bgx_port_sgmii_set_link_down(struct bgx_port_priv *priv)
>> +{
>> +   u64 data;

>> +   data = oct_csr_read(BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, 
>> priv->index));
>> +   data |= BIT(11);
>> +   oct_csr_write(data, BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, 
>> priv->index));
>> +   data = oct_csr_read(BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx, 
>> priv->index));
>
> Any particular reason to read immediately after write ?



>> +static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv, struct 
>> port_status status)
>> +{
>> +   u64 data;
>> +   u64 prtx;
>> +   u64 miscx;
>> +   int timeout;
>> +

>> +
>> +   switch (status.speed) {
>> +   case 10:
>
> In my opinion, instead of hard coding the value, is it fine to use ENUM ?
   Similar comments applicable in other places where hard coded values are used.



>> +static int bgx_port_gser_27882(struct bgx_port_priv *priv)
>> +{
>> +   u64 data;
>> +   u64 addr;
>
>> +   int timeout = 200;
>> +
>> +   //timeout = 200;
Better to initialize the timeout value


>> +static int bgx_port_qlm_rx_equalization(struct bgx_port_priv *priv, int 
>> qlm, int lane)
>> +{
>> +   lmode = oct_csr_read(GSER_LANE_MODE(priv->node, qlm));
>> +   lmode &= 0xf;
>> +   addr = GSER_LANE_P_MODE_1(priv->node, qlm, lmode);
>> +   data = oct_csr_read(addr);
>> +   /* Don't complete rx equalization if in VMA manual mode */
>> +   if (data & BIT(14))
>> +   return 0;
>> +
>> +   /* Apply rx equalization for speed > 6250 */
>> +   if (bgx_port_get_qlm_speed(priv, qlm) < 6250)
>> +   return 0;
>> +
>> +   /* Wait until rx data is valid (CDRLOCK) */
>> +   timeout = 500;
>
> 500 us is the min required value or it can be further reduced ?


>> +static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
>> +{

>> +
>> +   if (use_ber) {
>> +   timeout = 1;
>> +   do {
>> +   data =
>> +   oct_csr_read(BGX_SPU_BR_STATUS1(priv->node, 
>> priv->bgx, priv->index));
>> +   if (data & BIT(0))
>> +   break;
>> +   timeout--;
>> +   udelay(1);
>> +   } while (timeout);
>
> In my opinion, it's better to implement similar kind of loops inside macros.
>
>> +   if (!timeout) {
>> +   pr_debug("BGX%d:%d:%d: BLK_LOCK timeout\n",
>> +priv->bgx, priv->index, priv->node);
>> +   return -1;
>> +   }
>> +   } else {
>> +   timeout = 1;
>> +   do {
>> +   data =
>> +   oct_csr_read(BGX_SPU_BX_STATUS(priv->node, 
>> priv->bgx, priv->index));
>> +   if (data & BIT(12))
>> +   break;
>> +   timeout--;
>> +   udelay(1);
>> +   } while (timeout);
> same here
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 7/8] netdev: octeon-ethernet: Add Cavium Octeon III support.

[Souptick Joarder]

Hi David, Dan,


On Thu, Nov 30, 2017 at 12:50 AM, David Daney <dda...@caviumnetworks.com> wrote:
> On 11/29/2017 08:07 AM, Souptick Joarder wrote:
>>
>> On Wed, Nov 29, 2017 at 4:00 PM, Souptick Joarder <jrdr.li...@gmail.com>
>> wrote:
>>>
>>> On Wed, Nov 29, 2017 at 6:25 AM, David Daney <david.da...@cavium.com>
>>> wrote:
>>>>
>>>> From: Carlos Munoz <cmu...@cavium.com>
>>>>
>>>> The Cavium OCTEON cn78xx and cn73xx SoCs have network packet I/O
>>>> hardware that is significantly different from previous generations of
>>>> the family.
>>
>>
>>>> diff --git a/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c
>>>> b/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c
>>>> new file mode 100644
>>>> index ..4dad35fa4270
>>>> --- /dev/null
>>>> +++ b/drivers/net/ethernet/cavium/octeon/octeon3-bgx-port.c
>>>> @@ -0,0 +1,2033 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/* Copyright (c) 2017 Cavium, Inc.
>>>> + *
>>>> + * This file is subject to the terms and conditions of the GNU General
>>>> Public
>>>> + * License.  See the file "COPYING" in the main directory of this
>>>> archive
>>>> + * for more details.
>>>> + */
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +
>>
>>
>>>> +static void bgx_port_sgmii_set_link_down(struct bgx_port_priv *priv)
>>>> +{
>>>> +   u64 data;
>>
>>
>>>> +   data = oct_csr_read(BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx,
>>>> priv->index));
>>>> +   data |= BIT(11);
>>>> +   oct_csr_write(data, BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx,
>>>> priv->index));
>>>> +   data = oct_csr_read(BGX_GMP_PCS_MISC_CTL(priv->node, priv->bgx,
>>>> priv->index));
>>>
>>>
>>> Any particular reason to read immediately after write ?
>>
>>
>
> Yes, to ensure the write is committed to hardware before the next step.
>
>>
>>
>>>> +static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv,
>>>> struct port_status status)
>>>> +{
>>>> +   u64 data;
>>>> +   u64 prtx;
>>>> +   u64 miscx;
>>>> +   int timeout;
>>>> +
>>
>>
>>>> +
>>>> +   switch (status.speed) {
>>>> +   case 10:
>>>
>>>
>>> In my opinion, instead of hard coding the value, is it fine to use ENUM ?
>>
>> Similar comments applicable in other places where hard coded values
>> are used.
>>
>
> There is nothing to be gained by interposing an extra layer of abstraction
> in this case.  The code is more clear with the raw numbers in this
> particular case.

   As mentioned by Andrew,  macros defined in uapi/linux/ethtool.h may
be useful here.
   Otherwise it's fine to me :)
>
>
>>
>>
>>>> +static int bgx_port_gser_27882(struct bgx_port_priv *priv)
>>>> +{
>>>> +   u64 data;
>>>> +   u64 addr;
>>>
>>>
>>>> +   int timeout = 200;
>>>> +
>>>> +   //timeout = 200;
>>
>> Better to initialize the timeout value

>
>
> What are you talking about?  It is properly initialized using valid C code.

  I mean, instead of writing

   int timeout;
   timeout = 200;

  write,

   int timeout = 200;

Anyway both are correct and there is nothing wrong in your code.
Please ignore my comment here.

>
>
>>
>>
>>>> +static int bgx_port_qlm_rx_equalization(struct bgx_port_priv *priv, int
>>>> qlm, int lane)
>>>> +{
>>>> +   lmode = oct_csr_read(GSER_LANE_MODE(priv->node, qlm));
>>>> +   lmode &= 0xf;
>>>> +   addr = GSER_LANE_P_MODE_1(priv->node, qlm, lmode);
>>>> +   data = oct_csr_read(addr);
>>>> +   /* Don't complete rx equalization if in VMA manual mode */
>>>> +   if (data & BIT(14))
>>>> +   return 0;
>>>> +
>>>> +   /* Apply rx equ

[PATCH v2] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.

Commit 1c8f422059ae ("mm: change return type to vm_fault_t")

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
v2: updated the change log

 drivers/staging/lustre/lustre/llite/llite_mmap.c | 35 
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_mmap.c 
b/drivers/staging/lustre/lustre/llite/llite_mmap.c
index c0533bd..5b8fd10 100644
--- a/drivers/staging/lustre/lustre/llite/llite_mmap.c
+++ b/drivers/staging/lustre/lustre/llite/llite_mmap.c
@@ -231,7 +231,7 @@ static int ll_page_mkwrite0(struct vm_area_struct *vma, 
struct page *vmpage,
return result;
 }
 
-static inline int to_fault_error(int result)
+static inline vm_fault_t to_fault_error(int result)
 {
switch (result) {
case 0:
@@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
  * \retval VM_FAULT_ERROR on general error
  * \retval NOPAGE_OOM not have memory for allocate new page
  */
-static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
+static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
struct lu_env  *env;
struct cl_io*io;
@@ -269,16 +269,16 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
vm_fault *vmf)
struct page  *vmpage;
unsigned long   ra_flags;
int   result = 0;
-   int   fault_ret = 0;
+   vm_fault_t  fault_ret = 0;
u16 refcheck;
 
env = cl_env_get();
if (IS_ERR(env))
-   return PTR_ERR(env);
+   return VM_FAULT_ERROR;
 
io = ll_fault_io_init(env, vma, vmf->pgoff, _flags);
if (IS_ERR(io)) {
-   result = to_fault_error(PTR_ERR(io));
+   fault_ret = to_fault_error(PTR_ERR(io));
goto out;
}
 
@@ -319,15 +319,15 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
vm_fault *vmf)
if (result != 0 && !(fault_ret & VM_FAULT_RETRY))
fault_ret |= to_fault_error(result);
 
-   CDEBUG(D_MMAP, "%s fault %d/%d\n", current->comm, fault_ret, result);
+   CDEBUG(D_MMAP, "%s fault %x/%d\n", current->comm, fault_ret, result);
return fault_ret;
 }
 
-static int ll_fault(struct vm_fault *vmf)
+static vm_fault_t ll_fault(struct vm_fault *vmf)
 {
int count = 0;
bool printed = false;
-   int result;
+   vm_fault_t result;
sigset_t set;
 
/* Only SIGKILL and SIGTERM are allowed for fault/nopage/mkwrite
@@ -364,18 +364,19 @@ static int ll_fault(struct vm_fault *vmf)
return result;
 }
 
-static int ll_page_mkwrite(struct vm_fault *vmf)
+static vm_fault_t ll_page_mkwrite(struct vm_fault *vmf)
 {
struct vm_area_struct *vma = vmf->vma;
int count = 0;
bool printed = false;
bool retry;
-   int result;
+   int err;
+   vm_fault_t ret;
 
file_update_time(vma->vm_file);
do {
retry = false;
-   result = ll_page_mkwrite0(vma, vmf->page, );
+   err = ll_page_mkwrite0(vma, vmf->page, );
 
if (!printed && ++count > 16) {
const struct dentry *de = vma->vm_file->f_path.dentry;
@@ -387,25 +388,25 @@ static int ll_page_mkwrite(struct vm_fault *vmf)
}
} while (retry);
 
-   switch (result) {
+   switch (err) {
case 0:
LASSERT(PageLocked(vmf->page));
-   result = VM_FAULT_LOCKED;
+   ret = VM_FAULT_LOCKED;
break;
case -ENODATA:
case -EAGAIN:
case -EFAULT:
-   result = VM_FAULT_NOPAGE;
+   ret = VM_FAULT_NOPAGE;
break;
case -ENOMEM:
-   result = VM_FAULT_OOM;
+   ret = VM_FAULT_OOM;
break;
default:
-   result = VM_FAULT_SIGBUS;
+   ret = VM_FAULT_SIGBUS;
break;
}
 
-   return result;
+   return ret;
 }
 
 /**
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

On Sun, May 20, 2018 at 6:12 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Thu, May 17, 2018 at 12:27:11AM +0530, Souptick Joarder wrote:
>> Use new return type vm_fault_t for fault handler. For
>> now, this is just documenting that the function returns
>> a VM_FAULT value rather than an errno. Once all instances
>> are converted, vm_fault_t will become a distinct type.
>>
>> Commit 1c8f422059ae ("mm: change return type to vm_fault_t")
>>
>
> What does that line mean?  Please be more descriptive.
>
> confused,
>
> greg k-h

Sure, will send v3 with more description.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v3] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.

Ref-> commit 1c8f422059ae ("mm: change return type to
vm_fault_t") was added in 4.17-rc1 to introduce the new
typedef vm_fault_t. Currently we are making change to all
drivers to return vm_fault_t for page fault handlers. As
part of that lustre driver is also getting changed to
return vm_fault_t type. 

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
v2: updated the change log

v3: updated the change log

 drivers/staging/lustre/lustre/llite/llite_mmap.c | 35 
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_mmap.c 
b/drivers/staging/lustre/lustre/llite/llite_mmap.c
index c0533bd..5b8fd10 100644
--- a/drivers/staging/lustre/lustre/llite/llite_mmap.c
+++ b/drivers/staging/lustre/lustre/llite/llite_mmap.c
@@ -231,7 +231,7 @@ static int ll_page_mkwrite0(struct vm_area_struct *vma, 
struct page *vmpage,
return result;
 }
 
-static inline int to_fault_error(int result)
+static inline vm_fault_t to_fault_error(int result)
 {
switch (result) {
case 0:
@@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
  * \retval VM_FAULT_ERROR on general error
  * \retval NOPAGE_OOM not have memory for allocate new page
  */
-static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
+static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
struct lu_env  *env;
struct cl_io*io;
@@ -269,16 +269,16 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
vm_fault *vmf)
struct page  *vmpage;
unsigned long   ra_flags;
int   result = 0;
-   int   fault_ret = 0;
+   vm_fault_t  fault_ret = 0;
u16 refcheck;
 
env = cl_env_get();
if (IS_ERR(env))
-   return PTR_ERR(env);
+   return VM_FAULT_ERROR;
 
io = ll_fault_io_init(env, vma, vmf->pgoff, _flags);
if (IS_ERR(io)) {
-   result = to_fault_error(PTR_ERR(io));
+   fault_ret = to_fault_error(PTR_ERR(io));
goto out;
}
 
@@ -319,15 +319,15 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
vm_fault *vmf)
if (result != 0 && !(fault_ret & VM_FAULT_RETRY))
fault_ret |= to_fault_error(result);
 
-   CDEBUG(D_MMAP, "%s fault %d/%d\n", current->comm, fault_ret, result);
+   CDEBUG(D_MMAP, "%s fault %x/%d\n", current->comm, fault_ret, result);
return fault_ret;
 }
 
-static int ll_fault(struct vm_fault *vmf)
+static vm_fault_t ll_fault(struct vm_fault *vmf)
 {
int count = 0;
bool printed = false;
-   int result;
+   vm_fault_t result;
sigset_t set;
 
/* Only SIGKILL and SIGTERM are allowed for fault/nopage/mkwrite
@@ -364,18 +364,19 @@ static int ll_fault(struct vm_fault *vmf)
return result;
 }
 
-static int ll_page_mkwrite(struct vm_fault *vmf)
+static vm_fault_t ll_page_mkwrite(struct vm_fault *vmf)
 {
struct vm_area_struct *vma = vmf->vma;
int count = 0;
bool printed = false;
bool retry;
-   int result;
+   int err;
+   vm_fault_t ret;
 
file_update_time(vma->vm_file);
do {
retry = false;
-   result = ll_page_mkwrite0(vma, vmf->page, );
+   err = ll_page_mkwrite0(vma, vmf->page, );
 
if (!printed && ++count > 16) {
const struct dentry *de = vma->vm_file->f_path.dentry;
@@ -387,25 +388,25 @@ static int ll_page_mkwrite(struct vm_fault *vmf)
}
} while (retry);
 
-   switch (result) {
+   switch (err) {
case 0:
LASSERT(PageLocked(vmf->page));
-   result = VM_FAULT_LOCKED;
+   ret = VM_FAULT_LOCKED;
break;
case -ENODATA:
case -EAGAIN:
case -EFAULT:
-   result = VM_FAULT_NOPAGE;
+   ret = VM_FAULT_NOPAGE;
break;
case -ENOMEM:
-   result = VM_FAULT_OOM;
+   ret = VM_FAULT_OOM;
break;
default:
-   result = VM_FAULT_SIGBUS;
+   ret = VM_FAULT_SIGBUS;
break;
}
 
-   return result;
+   return ret;
 }
 
 /**
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [lustre-devel] [PATCH v3] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

On Thu, Jun 14, 2018 at 12:59 PM, NeilBrown  wrote:
> On Tue, Jun 12 2018, Souptick Joarder wrote:
>
>> On 12-Jun-2018 2:21 AM, "Greg KH"  wrote:
>>>
>>> On Tue, Jun 12, 2018 at 02:00:47AM +0530, Souptick Joarder wrote:
>>> > On Mon, May 21, 2018 at 11:39 PM, Souptick Joarder 
>>> >
>>> > If no further comment, we would like to get this patch in 4.18-rc-X.
>>>
>>> Why?  Is it a regression fix?  That's all that is allowed after -rc1.
>>
>> No, this is not regression fix. We need to get this into 4.18-rc-1.  But
>> mostly it can't make into linus tree in rc-1 :)
>>>
>>> And have you tried applying it to Linus's current tree?  :)
>>
>> Last tested on 4.17-rc-6 and it worked fine. Let me verify in current tree.
>>
>
> As you have undoubtedly noticed, lustre is no longer in Linus' tree.
> I'm experimenting with maintaining a branch which retains the code
> (lustre/* in github.com/neilbrown/linux) so we can get it ready for
> merging properly.
> I've added you patch to my tree.

You need to add this patch in your tree as well.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20180614=1c8f422059ae5da07db7406ab916203f9417e396

This patch appears to be missing in your github branch.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

On Tue, Jun 12, 2018 at 2:37 AM, Greg KH  wrote:
> On Tue, Jun 12, 2018 at 02:30:27AM +0530, Souptick Joarder wrote:
>> > >
>> > > If no further comment, we would like to get this patch in 4.18-rc-X.
>> >
>> > Why?  Is it a regression fix?  That's all that is allowed after -rc1.
>>
>> No, this is not regression fix. We need to get this into 4.18-rc-1.  But
>> mostly it can't make into linus tree in rc-1 :)
>
> Why does it _have_ to get into 4.18-rc1?  My tree is long-closed and
> Linus already has all of my patches in his tree for the staging section
> of the kernel.
>
>> > And have you tried applying it to Linus's current tree?  :)
>>
>> Last tested on 4.17-rc-6 and it worked fine. Let me verify in current tree.
>
> Try it, you might be surprised :)

Yes, got the surprise :)
Sorry for making noise, I will drop this patch as it is no more valid
in current Linus's tree.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

On Mon, May 21, 2018 at 11:39 PM, Souptick Joarder  wrote:
> Use new return type vm_fault_t for fault handler. For
> now, this is just documenting that the function returns
> a VM_FAULT value rather than an errno. Once all instances
> are converted, vm_fault_t will become a distinct type.
>
> Ref-> commit 1c8f422059ae ("mm: change return type to
> vm_fault_t") was added in 4.17-rc1 to introduce the new
> typedef vm_fault_t. Currently we are making change to all
> drivers to return vm_fault_t for page fault handlers. As
> part of that lustre driver is also getting changed to
> return vm_fault_t type.
>
> Signed-off-by: Souptick Joarder 
> ---
> v2: updated the change log
>
> v3: updated the change log
>
>  drivers/staging/lustre/lustre/llite/llite_mmap.c | 35 
> 
>  1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/llite_mmap.c 
> b/drivers/staging/lustre/lustre/llite/llite_mmap.c
> index c0533bd..5b8fd10 100644
> --- a/drivers/staging/lustre/lustre/llite/llite_mmap.c
> +++ b/drivers/staging/lustre/lustre/llite/llite_mmap.c
> @@ -231,7 +231,7 @@ static int ll_page_mkwrite0(struct vm_area_struct *vma, 
> struct page *vmpage,
> return result;
>  }
>
> -static inline int to_fault_error(int result)
> +static inline vm_fault_t to_fault_error(int result)
>  {
> switch (result) {
> case 0:
> @@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
>   * \retval VM_FAULT_ERROR on general error
>   * \retval NOPAGE_OOM not have memory for allocate new page
>   */
> -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
> +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>  {
> struct lu_env  *env;
> struct cl_io*io;
> @@ -269,16 +269,16 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
> vm_fault *vmf)
> struct page  *vmpage;
> unsigned long   ra_flags;
> int   result = 0;
> -   int   fault_ret = 0;
> +   vm_fault_t  fault_ret = 0;
> u16 refcheck;
>
> env = cl_env_get();
> if (IS_ERR(env))
> -   return PTR_ERR(env);
> +   return VM_FAULT_ERROR;
>
> io = ll_fault_io_init(env, vma, vmf->pgoff, _flags);
> if (IS_ERR(io)) {
> -   result = to_fault_error(PTR_ERR(io));
> +   fault_ret = to_fault_error(PTR_ERR(io));
> goto out;
> }
>
> @@ -319,15 +319,15 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
> vm_fault *vmf)
> if (result != 0 && !(fault_ret & VM_FAULT_RETRY))
> fault_ret |= to_fault_error(result);
>
> -   CDEBUG(D_MMAP, "%s fault %d/%d\n", current->comm, fault_ret, result);
> +   CDEBUG(D_MMAP, "%s fault %x/%d\n", current->comm, fault_ret, result);
> return fault_ret;
>  }
>
> -static int ll_fault(struct vm_fault *vmf)
> +static vm_fault_t ll_fault(struct vm_fault *vmf)
>  {
> int count = 0;
> bool printed = false;
> -   int result;
> +   vm_fault_t result;
> sigset_t set;
>
> /* Only SIGKILL and SIGTERM are allowed for fault/nopage/mkwrite
> @@ -364,18 +364,19 @@ static int ll_fault(struct vm_fault *vmf)
> return result;
>  }
>
> -static int ll_page_mkwrite(struct vm_fault *vmf)
> +static vm_fault_t ll_page_mkwrite(struct vm_fault *vmf)
>  {
> struct vm_area_struct *vma = vmf->vma;
> int count = 0;
> bool printed = false;
> bool retry;
> -   int result;
> +   int err;
> +   vm_fault_t ret;
>
> file_update_time(vma->vm_file);
> do {
> retry = false;
> -   result = ll_page_mkwrite0(vma, vmf->page, );
> +   err = ll_page_mkwrite0(vma, vmf->page, );
>
> if (!printed && ++count > 16) {
> const struct dentry *de = vma->vm_file->f_path.dentry;
> @@ -387,25 +388,25 @@ static int ll_page_mkwrite(struct vm_fault *vmf)
> }
> } while (retry);
>
> -   switch (result) {
> +   switch (err) {
> case 0:
> LASSERT(PageLocked(vmf->page));
> -   result = VM_FAULT_LOCKED;
> +   ret = VM_FAULT_LOCKED;
> break;
> case -ENODATA:
> case -EAGAIN:
> case -EFAULT:
> -   result = VM_FAULT_NOPAGE;
> +  

[PATCH] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for fault handler. For
now, this is just documenting that the function returns
a VM_FAULT value rather than an errno. Once all instances
are converted, vm_fault_t will become a distinct type.

Commit 1c8f422059ae ("mm: change return type to vm_fault_t")

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
 drivers/staging/lustre/lustre/llite/llite_mmap.c | 29 
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/llite_mmap.c 
b/drivers/staging/lustre/lustre/llite/llite_mmap.c
index c0533bd..6aafe7c 100644
--- a/drivers/staging/lustre/lustre/llite/llite_mmap.c
+++ b/drivers/staging/lustre/lustre/llite/llite_mmap.c
@@ -231,7 +231,7 @@ static int ll_page_mkwrite0(struct vm_area_struct *vma, 
struct page *vmpage,
return result;
 }
 
-static inline int to_fault_error(int result)
+static inline vm_fault_t to_fault_error(int result)
 {
switch (result) {
case 0:
@@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
  * \retval VM_FAULT_ERROR on general error
  * \retval NOPAGE_OOM not have memory for allocate new page
  */
-static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
+static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
struct lu_env  *env;
struct cl_io*io;
@@ -269,7 +269,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
vm_fault *vmf)
struct page  *vmpage;
unsigned long   ra_flags;
int   result = 0;
-   int   fault_ret = 0;
+   vm_fault_t  fault_ret = 0;
u16 refcheck;
 
env = cl_env_get();
@@ -278,7 +278,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
vm_fault *vmf)
 
io = ll_fault_io_init(env, vma, vmf->pgoff, _flags);
if (IS_ERR(io)) {
-   result = to_fault_error(PTR_ERR(io));
+   fault_ret = to_fault_error(PTR_ERR(io));
goto out;
}
 
@@ -319,7 +319,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
vm_fault *vmf)
if (result != 0 && !(fault_ret & VM_FAULT_RETRY))
fault_ret |= to_fault_error(result);
 
-   CDEBUG(D_MMAP, "%s fault %d/%d\n", current->comm, fault_ret, result);
+   CDEBUG(D_MMAP, "%s fault %x/%d\n", current->comm, fault_ret, result);
return fault_ret;
 }
 
@@ -327,7 +327,7 @@ static int ll_fault(struct vm_fault *vmf)
 {
int count = 0;
bool printed = false;
-   int result;
+   vm_fault_t result;
sigset_t set;
 
/* Only SIGKILL and SIGTERM are allowed for fault/nopage/mkwrite
@@ -370,12 +370,13 @@ static int ll_page_mkwrite(struct vm_fault *vmf)
int count = 0;
bool printed = false;
bool retry;
-   int result;
+   int err;
+   vm_fault_t ret;
 
file_update_time(vma->vm_file);
do {
retry = false;
-   result = ll_page_mkwrite0(vma, vmf->page, );
+   err = ll_page_mkwrite0(vma, vmf->page, );
 
if (!printed && ++count > 16) {
const struct dentry *de = vma->vm_file->f_path.dentry;
@@ -387,25 +388,25 @@ static int ll_page_mkwrite(struct vm_fault *vmf)
}
} while (retry);
 
-   switch (result) {
+   switch (err) {
case 0:
LASSERT(PageLocked(vmf->page));
-   result = VM_FAULT_LOCKED;
+   ret = VM_FAULT_LOCKED;
break;
case -ENODATA:
case -EAGAIN:
case -EFAULT:
-   result = VM_FAULT_NOPAGE;
+   ret = VM_FAULT_NOPAGE;
break;
case -ENOMEM:
-   result = VM_FAULT_OOM;
+   ret = VM_FAULT_OOM;
break;
default:
-   result = VM_FAULT_SIGBUS;
+   ret = VM_FAULT_SIGBUS;
break;
}
 
-   return result;
+   return ret;
 }
 
 /**
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] android: binder: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for fault handler
in struct vm_operations_struct. For now, this is
just documenting that the function returns a 
VM_FAULT value rather than an errno.  Once all
instances are converted, vm_fault_t will become
a distinct type.

Reference commit id->
1c8f422059ae5da07db7406ab916203f9417e396

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
 drivers/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 15e3d3c..65be4ec 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4671,7 +4671,7 @@ static void binder_vma_close(struct vm_area_struct *vma)
binder_defer_work(proc, BINDER_DEFERRED_PUT_FILES);
 }

-static int binder_vm_fault(struct vm_fault *vmf)
+static vm_fault_t binder_vm_fault(struct vm_fault *vmf)
 {
return VM_FAULT_SIGBUS;
 }
--
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] android: binder: Change return type to vm_fault_t

[Souptick Joarder]

On Mon, Apr 23, 2018 at 2:48 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> On Mon, Apr 16, 2018 at 08:41:21PM +0530, Souptick Joarder wrote:
>> Use new return type vm_fault_t for fault handler
>> in struct vm_operations_struct. For now, this is
>> just documenting that the function returns a
>> VM_FAULT value rather than an errno.  Once all
>> instances are converted, vm_fault_t will become
>> a distinct type.
>>
>> Reference commit id->
>> 1c8f422059ae5da07db7406ab916203f9417e396
>
> As you know, that is not how you reference git commit ids.  Please fix
> up and resend.

Sure, I will send v2.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] android: binder: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for fault handler in
struct vm_operations_struct. For now, this is just
documenting that the function returns a VM_FAULT
value rather than an errno.  Once all instances are
converted, vm_fault_t will become a distinct type.

Reference id -> 1c8f422059ae ("mm: change return type
to vm_fault_t")

Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
---
 drivers/android/binder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 15e3d3c..65be4ec 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4671,7 +4671,7 @@ static void binder_vma_close(struct vm_area_struct *vma)
binder_defer_work(proc, BINDER_DEFERRED_PUT_FILES);
 }

-static int binder_vm_fault(struct vm_fault *vmf)
+static vm_fault_t binder_vm_fault(struct vm_fault *vmf)
 {
return VM_FAULT_SIGBUS;
 }
--
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

On Sun, Apr 22, 2018 at 8:50 AM, Matthew Wilcox <wi...@infradead.org> wrote:
> On Sun, Apr 22, 2018 at 03:47:24AM +0530, Souptick Joarder wrote:
>> @@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
>>   * \retval VM_FAULT_ERROR on general error
>>   * \retval NOPAGE_OOM not have memory for allocate new page
>>   */
>> -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>> +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault 
>> *vmf)
>>  {
>>   struct lu_env  *env;
>>   struct cl_io*io;
>
> Did you compile-test this with the sparse changes?  Because I can see
> a problem here:

Yes, compile-tested. Sparse didn't throw any warning/error.

>
> env = cl_env_get();
> if (IS_ERR(env))
> return PTR_ERR(env);
>
>> @@ -269,7 +269,7 @@ static int ll_fault0(struct vm_area_struct *vma, struct 
>> vm_fault *vmf)
>>   struct page  *vmpage;
>>   unsigned long   ra_flags;
>>   int   result = 0;
>> - int   fault_ret = 0;
>> + vm_fault_t  fault_ret = 0;
>
> What odd indentation ...

Sorry, will correct it.
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: lustre: Change return type to vm_fault_t

[Souptick Joarder]

On Mon, Apr 23, 2018 at 11:05 PM, Matthew Wilcox <wi...@infradead.org> wrote:
> On Mon, Apr 23, 2018 at 10:12:30PM +0530, Souptick Joarder wrote:
>> On Sun, Apr 22, 2018 at 8:50 AM, Matthew Wilcox <wi...@infradead.org> wrote:
>> > On Sun, Apr 22, 2018 at 03:47:24AM +0530, Souptick Joarder wrote:
>> >> @@ -261,7 +261,7 @@ static inline int to_fault_error(int result)
>> >>   * \retval VM_FAULT_ERROR on general error
>> >>   * \retval NOPAGE_OOM not have memory for allocate new page
>> >>   */
>> >> -static int ll_fault0(struct vm_area_struct *vma, struct vm_fault *vmf)
>> >> +static vm_fault_t ll_fault0(struct vm_area_struct *vma, struct vm_fault 
>> >> *vmf)
>> >>  {
>> >>   struct lu_env  *env;
>> >>   struct cl_io*io;
>> >
>> > Did you compile-test this with the sparse changes?  Because I can see
>> > a problem here:
>>
>> Yes, compile-tested. Sparse didn't throw any warning/error.
>
> I think you need to check your setup ... I get this:
>
> drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31: warning: incorrect 
> type in return expression (different base types)
> drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:expected 
> restricted vm_fault_t
> drivers/staging/lustre/lustre/llite/llite_mmap.c:277:31:got long

What I am trying is -
after applying the patch in 4.17-rc1, enable the configuration
for this driver and do "make c=1/2 -j8"

Let me verify again.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] drivers/android/binder.c: Remove duplicate header

[Souptick Joarder]

On Fri, Nov 9, 2018 at 8:17 PM Greg KH  wrote:
>
> On Fri, Nov 09, 2018 at 10:40:14PM +0800, kbuild test robot wrote:
> > Hi Brajeswar,
> >
> > Thank you for the patch! Yet something to improve:
> >
> > [auto build test ERROR on staging/staging-testing]
> > [also build test ERROR on v4.20-rc1 next-20181109]
> > [if your patch is applied to the wrong git tree, please drop us a note to 
> > help improve the system]
> >
> > url:
> > https://github.com/0day-ci/linux/commits/Brajeswar-Ghosh/drivers-android-binder-c-Remove-duplicate-header/20181109-154717
> > config: i386-allmodconfig (attached as .config)
> > compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> > reproduce:
> > # save the attached .config to linux build tree
> > make ARCH=i386
> >
> > All errors (new ones prefixed by >>):
>
> 
>
> Yeah, I was right :(
>
> Always test-build your patches.

Sorry about it. It was a mistake from our side.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] android/binder: Replace vm_insert_page with vmf_insert_page

[Souptick Joarder]

There is a plan to replace vm_insert_page with new API
vmf_insert_page. As part of it, converting vm_insert_page
to use vmf_insert_page.

Signed-off-by: Souptick Joarder 
---
 drivers/android/binder_alloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 64fd96e..17368ef 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -238,6 +238,7 @@ static int binder_update_page_range(struct binder_alloc 
*alloc, int allocate,
int ret;
bool on_lru;
size_t index;
+   vm_fault_t vmf;
 
index = (page_addr - alloc->buffer) / PAGE_SIZE;
page = >pages[index];
@@ -279,8 +280,8 @@ static int binder_update_page_range(struct binder_alloc 
*alloc, int allocate,
}
user_page_addr =
(uintptr_t)page_addr + alloc->user_buffer_offset;
-   ret = vm_insert_page(vma, user_page_addr, page[0].page_ptr);
-   if (ret) {
+   vmf = vmf_insert_page(vma, user_page_addr, page[0].page_ptr);
+   if (vmf != VM_FAULT_NOPAGE) {
pr_err("%d: binder_alloc_buf failed to map page at %lx 
in userspace\n",
   alloc->pid, user_page_addr);
goto err_vm_insert_page_failed;
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging/android/vsoc: Remove duplicate header

[Souptick Joarder]

On Wed, Jan 9, 2019 at 8:56 PM Brajeswar Ghosh
 wrote:
>
> Remove linux/mutex.h.h which is included more than once
>
> Signed-off-by: Brajeswar Ghosh 

Acked-by: Souptick Joarder 

> ---
>  drivers/staging/android/vsoc.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/android/vsoc.c b/drivers/staging/android/vsoc.c
> index 22571abcaa4e..8a75bd27c413 100644
> --- a/drivers/staging/android/vsoc.c
> +++ b/drivers/staging/android/vsoc.c
> @@ -29,7 +29,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include "uapi/vsoc_shm.h"
> --
> 2.17.1
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RESEND PATCH 3/7] mm/gup: Change GUP fast to use flags rather than a write 'bool'

[Souptick Joarder]

Hi Ira,

On Wed, Feb 20, 2019 at 11:01 AM  wrote:
>
> From: Ira Weiny 
>
> To facilitate additional options to get_user_pages_fast() change the
> singular write parameter to be gup_flags.
>
> This patch does not change any functionality.  New functionality will
> follow in subsequent patches.
>
> Some of the get_user_pages_fast() call sites were unchanged because they
> already passed FOLL_WRITE or 0 for the write parameter.
>
> Signed-off-by: Ira Weiny 
> ---
>  arch/mips/mm/gup.c | 11 ++-
>  arch/powerpc/kvm/book3s_64_mmu_hv.c|  4 ++--
>  arch/powerpc/kvm/e500_mmu.c|  2 +-
>  arch/powerpc/mm/mmu_context_iommu.c|  4 ++--
>  arch/s390/kvm/interrupt.c  |  2 +-
>  arch/s390/mm/gup.c | 12 ++--
>  arch/sh/mm/gup.c   | 11 ++-
>  arch/sparc/mm/gup.c|  9 +
>  arch/x86/kvm/paging_tmpl.h |  2 +-
>  arch/x86/kvm/svm.c |  2 +-
>  drivers/fpga/dfl-afu-dma-region.c  |  2 +-
>  drivers/gpu/drm/via/via_dmablit.c  |  3 ++-
>  drivers/infiniband/hw/hfi1/user_pages.c|  3 ++-
>  drivers/misc/genwqe/card_utils.c   |  2 +-
>  drivers/misc/vmw_vmci/vmci_host.c  |  2 +-
>  drivers/misc/vmw_vmci/vmci_queue_pair.c|  6 --
>  drivers/platform/goldfish/goldfish_pipe.c  |  3 ++-
>  drivers/rapidio/devices/rio_mport_cdev.c   |  4 +++-
>  drivers/sbus/char/oradax.c |  2 +-
>  drivers/scsi/st.c  |  3 ++-
>  drivers/staging/gasket/gasket_page_table.c |  4 ++--
>  drivers/tee/tee_shm.c  |  2 +-
>  drivers/vfio/vfio_iommu_spapr_tce.c|  3 ++-
>  drivers/vhost/vhost.c  |  2 +-
>  drivers/video/fbdev/pvr2fb.c   |  2 +-
>  drivers/virt/fsl_hypervisor.c  |  2 +-
>  drivers/xen/gntdev.c   |  2 +-
>  fs/orangefs/orangefs-bufmap.c  |  2 +-
>  include/linux/mm.h |  4 ++--
>  kernel/futex.c |  2 +-
>  lib/iov_iter.c |  7 +--
>  mm/gup.c   | 10 +-
>  mm/util.c  |  8 
>  net/ceph/pagevec.c |  2 +-
>  net/rds/info.c |  2 +-
>  net/rds/rdma.c |  3 ++-
>  36 files changed, 81 insertions(+), 65 deletions(-)
>
> diff --git a/arch/mips/mm/gup.c b/arch/mips/mm/gup.c
> index 0d14e0d8eacf..4c2b4483683c 100644
> --- a/arch/mips/mm/gup.c
> +++ b/arch/mips/mm/gup.c
> @@ -235,7 +235,7 @@ int __get_user_pages_fast(unsigned long start, int 
> nr_pages, int write,
>   * get_user_pages_fast() - pin user pages in memory
>   * @start: starting user address
>   * @nr_pages:  number of pages from start to pin
> - * @write: whether pages will be written to
> + * @gup_flags: flags modifying pin behaviour
>   * @pages: array that receives pointers to the pages pinned.
>   * Should be at least nr_pages long.
>   *
> @@ -247,8 +247,8 @@ int __get_user_pages_fast(unsigned long start, int 
> nr_pages, int write,
>   * requested. If nr_pages is 0 or negative, returns 0. If no pages
>   * were pinned, returns -errno.
>   */
> -int get_user_pages_fast(unsigned long start, int nr_pages, int write,
> -   struct page **pages)
> +int get_user_pages_fast(unsigned long start, int nr_pages,
> +   unsigned int gup_flags, struct page **pages)
>  {
> struct mm_struct *mm = current->mm;
> unsigned long addr, len, end;
> @@ -273,7 +273,8 @@ int get_user_pages_fast(unsigned long start, int 
> nr_pages, int write,
> next = pgd_addr_end(addr, end);
> if (pgd_none(pgd))
> goto slow;
> -   if (!gup_pud_range(pgd, addr, next, write, pages, ))
> +   if (!gup_pud_range(pgd, addr, next, gup_flags & FOLL_WRITE,
> +  pages, ))
> goto slow;
> } while (pgdp++, addr = next, addr != end);
> local_irq_enable();
> @@ -289,7 +290,7 @@ int get_user_pages_fast(unsigned long start, int 
> nr_pages, int write,
> pages += nr;
>
> ret = get_user_pages_unlocked(start, (end - start) >> PAGE_SHIFT,
> - pages, write ? FOLL_WRITE : 0);
> + pages, gup_flags);
>
> /* Have to be a bit careful with return values */
> if (nr > 0) {
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c 
> b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index bd2dcfbf00cd..8fcb0a921e46 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -582,7 +582,7 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, 
> struct kvm_vcpu *vcpu,
> /* 

Re: [PATCH] staging: android: vsoc: Remove VSOC_WAIT_FOR_INCOMING_INTERRUPT

[Souptick Joarder]

On Tue, Feb 12, 2019 at 12:28 AM Alistair Strachan  wrote:
>
> On Mon, Feb 11, 2019 at 9:22 AM Todd Kjos  wrote:
> >
> > +Alistair Strachan
> >
> > On Mon, Feb 11, 2019 at 9:11 AM Greg KH  wrote:
> > >
> > > On Mon, Feb 11, 2019 at 10:15:18PM +0530, Souptick Joarder wrote:
> > > > On Mon, Feb 11, 2019 at 9:27 PM Greg KH  
> > > > wrote:
> > > > >
> > > > > On Mon, Feb 11, 2019 at 09:21:19PM +0530, Souptick Joarder wrote:
> > > > > > On Mon, Feb 11, 2019 at 9:10 PM Greg KH 
> > > > > >  wrote:
> > > > > > >
> > > > > > > On Mon, Feb 11, 2019 at 08:56:02PM +0530, Souptick Joarder wrote:
> > > > > > > > As mentioned in TODO list, Removed 
> > > > > > > > VSOC_WAIT_FOR_INCOMING_INTERRUPT
> > > > > > > > ioctl. This functionality has been superseded by the futex and 
> > > > > > > > is
> > > > > > > > there for legacy reasons.
> > > > > > > >
> > > > > > > > Signed-off-by: Souptick Joarder 
> > > > > > > > ---
> > > > > > > >  drivers/staging/android/uapi/vsoc_shm.h | 7 ---
> > > > > > > >  drivers/staging/android/vsoc.c  | 5 -
> > > > > > > >  2 files changed, 12 deletions(-)
> > > > > > >
> > > > > > > So userspace is all fixed up now and this ioctl can be dropped?  
> > > > > > > Any
> > > > > > > pointers to the userspace commit that did this?
>
> The ioctl is still being used and cannot be removed.

I think, it's good to add this info in TODO file. I can edit it if you are ok.

>
> > > > > >
> > > > > > I am not sure about user space part.
> > > > >
> > > > > Then we can not just delete the ioctl :)
> > > >
> > > > Agree, but where to verify the user space commit ?
> > > > Any pointer to the source code path ?
>
> The userspace code using the Linux 'vsoc' staging driver can be cloned
> from here:
>
> https://android.googlesource.com/device/google/cuttlefish_common

Thanks.
>
> > >
> > > Please work with the android developers to solve this.  It should be in
> > > AOSP "somewhere" :(
>
> I'm working on documenting this better on source.android.com. Stay tuned.
>
> > >
> > > good luck,
> > >
> > > greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: vsoc: Remove VSOC_WAIT_FOR_INCOMING_INTERRUPT

[Souptick Joarder]

On Mon, Feb 11, 2019 at 9:27 PM Greg KH  wrote:
>
> On Mon, Feb 11, 2019 at 09:21:19PM +0530, Souptick Joarder wrote:
> > On Mon, Feb 11, 2019 at 9:10 PM Greg KH  wrote:
> > >
> > > On Mon, Feb 11, 2019 at 08:56:02PM +0530, Souptick Joarder wrote:
> > > > As mentioned in TODO list, Removed VSOC_WAIT_FOR_INCOMING_INTERRUPT
> > > > ioctl. This functionality has been superseded by the futex and is
> > > > there for legacy reasons.
> > > >
> > > > Signed-off-by: Souptick Joarder 
> > > > ---
> > > >  drivers/staging/android/uapi/vsoc_shm.h | 7 ---
> > > >  drivers/staging/android/vsoc.c  | 5 -
> > > >  2 files changed, 12 deletions(-)
> > >
> > > So userspace is all fixed up now and this ioctl can be dropped?  Any
> > > pointers to the userspace commit that did this?
> >
> > I am not sure about user space part.
>
> Then we can not just delete the ioctl :)

Agree, but where to verify the user space commit ?
Any pointer to the source code path ?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: android: vsoc: Remove VSOC_WAIT_FOR_INCOMING_INTERRUPT

[Souptick Joarder]

As mentioned in TODO list, Removed VSOC_WAIT_FOR_INCOMING_INTERRUPT
ioctl. This functionality has been superseded by the futex and is
there for legacy reasons.

Signed-off-by: Souptick Joarder 
---
 drivers/staging/android/uapi/vsoc_shm.h | 7 ---
 drivers/staging/android/vsoc.c  | 5 -
 2 files changed, 12 deletions(-)

diff --git a/drivers/staging/android/uapi/vsoc_shm.h 
b/drivers/staging/android/uapi/vsoc_shm.h
index 6291fb2..69090cc 100644
--- a/drivers/staging/android/uapi/vsoc_shm.h
+++ b/drivers/staging/android/uapi/vsoc_shm.h
@@ -232,13 +232,6 @@ struct vsoc_shm_layout_descriptor {
 #define VSOC_MAYBE_SEND_INTERRUPT_TO_HOST _IO(0xF5, 2)
 
 /*
- * When this returns the guest will scan host_to_guest_signal_table to
- * check for new futexes to wake.
- */
-/* TODO(ghartman): Consider moving this to the bottom half */
-#define VSOC_WAIT_FOR_INCOMING_INTERRUPT _IO(0xF5, 3)
-
-/*
  * Guest HALs will use this to retrieve the region description after
  * opening their device node.
  */
diff --git a/drivers/staging/android/vsoc.c b/drivers/staging/android/vsoc.c
index 22571ab..a842ff7 100644
--- a/drivers/staging/android/vsoc.c
+++ b/drivers/staging/android/vsoc.c
@@ -592,11 +592,6 @@ static long vsoc_ioctl(struct file *filp, unsigned int 
cmd, unsigned long arg)
case VSOC_SEND_INTERRUPT_TO_HOST:
writel(reg_num, vsoc_dev.regs + DOORBELL);
return 0;
-   case VSOC_WAIT_FOR_INCOMING_INTERRUPT:
-   wait_event_interruptible
-   (reg_data->interrupt_wait_queue,
-(atomic_read(reg_data->incoming_signalled) != 0));
-   break;
 
case VSOC_DESCRIBE_REGION:
return do_vsoc_describe_region
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: android: vsoc: Remove VSOC_WAIT_FOR_INCOMING_INTERRUPT

[Souptick Joarder]

On Mon, Feb 11, 2019 at 9:10 PM Greg KH  wrote:
>
> On Mon, Feb 11, 2019 at 08:56:02PM +0530, Souptick Joarder wrote:
> > As mentioned in TODO list, Removed VSOC_WAIT_FOR_INCOMING_INTERRUPT
> > ioctl. This functionality has been superseded by the futex and is
> > there for legacy reasons.
> >
> > Signed-off-by: Souptick Joarder 
> > ---
> >  drivers/staging/android/uapi/vsoc_shm.h | 7 ---
> >  drivers/staging/android/vsoc.c  | 5 -
> >  2 files changed, 12 deletions(-)
>
> So userspace is all fixed up now and this ioctl can be dropped?  Any
> pointers to the userspace commit that did this?

I am not sure about user space part.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

Currently {get|pin}_user_pages_fast() have 3 return value 0, -errno
and no of pinned pages. The only case where these two functions will
return 0, is for nr_pages <= 0, which doesn't find a valid use case.
But if at all any, then a -ERRNO will be returned instead of 0, which
means {get|pin}_user_pages_fast() will have 2 return values -errno &
no of pinned pages.

Update all the callers which deals with return value 0 accordingly.

Signed-off-by: Souptick Joarder 
---
 arch/ia64/kernel/err_inject.c  | 2 +-
 drivers/platform/goldfish/goldfish_pipe.c  | 2 +-
 drivers/staging/gasket/gasket_page_table.c | 4 ++--
 drivers/tee/tee_shm.c  | 2 +-
 mm/gup.c   | 6 +++---
 net/rds/rdma.c | 2 +-
 6 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/ia64/kernel/err_inject.c b/arch/ia64/kernel/err_inject.c
index 8b5b8e6b..fd72218 100644
--- a/arch/ia64/kernel/err_inject.c
+++ b/arch/ia64/kernel/err_inject.c
@@ -143,7 +143,7 @@ static DEVICE_ATTR(name, 0644, show_##name, store_##name)
int ret;
 
ret = get_user_pages_fast(virt_addr, 1, FOLL_WRITE, NULL);
-   if (ret<=0) {
+   if (ret < 0) {
 #ifdef ERR_INJ_DEBUG
printk("Virtual address %lx is not existing.\n",virt_addr);
 #endif
diff --git a/drivers/platform/goldfish/goldfish_pipe.c 
b/drivers/platform/goldfish/goldfish_pipe.c
index 1ab207e..831449d 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -277,7 +277,7 @@ static int goldfish_pin_pages(unsigned long first_page,
ret = pin_user_pages_fast(first_page, requested_pages,
  !is_write ? FOLL_WRITE : 0,
  pages);
-   if (ret <= 0)
+   if (ret < 0)
return -EFAULT;
if (ret < requested_pages)
*iter_last_page_size = PAGE_SIZE;
diff --git a/drivers/staging/gasket/gasket_page_table.c 
b/drivers/staging/gasket/gasket_page_table.c
index f6d7157..1d08e1d 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -489,11 +489,11 @@ static int gasket_perform_mapping(struct 
gasket_page_table *pg_tbl,
ret = get_user_pages_fast(page_addr - offset, 1,
  FOLL_WRITE, );
 
-   if (ret <= 0) {
+   if (ret < 0) {
dev_err(pg_tbl->device,
"get user pages failed for addr=0x%lx, 
offset=0x%lx [ret=%d]\n",
page_addr, offset, ret);
-   return ret ? ret : -ENOMEM;
+   return ret;
}
++pg_tbl->num_active_pages;
 
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index bd679b7..2706a1f 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -230,7 +230,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, 
unsigned long addr,
if (rc > 0)
shm->num_pages = rc;
if (rc != num_pages) {
-   if (rc >= 0)
+   if (rc > 0)
rc = -ENOMEM;
ret = ERR_PTR(rc);
goto err;
diff --git a/mm/gup.c b/mm/gup.c
index 50681f0..8d293ed 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2760,7 +2760,7 @@ static int internal_get_user_pages_fast(unsigned long 
start, int nr_pages,
end = start + len;
 
if (end <= start)
-   return 0;
+   return -EINVAL;
if (unlikely(!access_ok((void __user *)start, len)))
return -EFAULT;
 
@@ -2805,8 +2805,8 @@ static int internal_get_user_pages_fast(unsigned long 
start, int nr_pages,
  * calling get_user_pages().
  *
  * Returns number of pages pinned. This may be fewer than the number requested.
- * If nr_pages is 0 or negative, returns 0. If no pages were pinned, returns
- * -errno.
+ * If nr_pages is 0 or negative, returns -errno. If no pages were pinned,
+ * returns -errno.
  */
 int get_user_pages_fast(unsigned long start, int nr_pages,
unsigned int gup_flags, struct page **pages)
diff --git a/net/rds/rdma.c b/net/rds/rdma.c
index a7ae118..44b96e6 100644
--- a/net/rds/rdma.c
+++ b/net/rds/rdma.c
@@ -161,7 +161,7 @@ static int rds_pin_pages(unsigned long user_addr, unsigned 
int nr_pages,
gup_flags |= FOLL_WRITE;
 
ret = pin_user_pages_fast(user_addr, nr_pages, gup_flags, pages);
-   if (ret >= 0 && ret < nr_pages) {
+   if (ret > 0 && ret < nr_pages) {
unpin_user_pages(pages, ret);
ret = -EFAULT;
}
-- 
1.9.1

___
devel mailing lis

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

On Wed, May 6, 2020 at 1:08 AM John Hubbard  wrote:
>
> On 2020-05-05 12:14, Souptick Joarder wrote:
> > Currently {get|pin}_user_pages_fast() have 3 return value 0, -errno
> > and no of pinned pages. The only case where these two functions will
> > return 0, is for nr_pages <= 0, which doesn't find a valid use case.
> > But if at all any, then a -ERRNO will be returned instead of 0, which
> > means {get|pin}_user_pages_fast() will have 2 return values -errno &
> > no of pinned pages.
> >
> > Update all the callers which deals with return value 0 accordingly.
>
> Hmmm, seems a little shaky. In order to do this safely, I'd recommend
> first changing gup_fast/pup_fast so so that they return -EINVAL if
> the caller specified nr_pages==0, and of course auditing all callers,
> to ensure that this won't cause problems.

While auditing it was figured out, there are 5 callers which cares for
return value
0 of gup_fast/pup_fast. What problem it might cause if we change
gup_fast/pup_fast
to return -EINVAL and update all the callers in a single commit ?

>
> The gup.c documentation would also need updating in a couple of comment
> blocks, above get_user_pages_remote(), and __get_user_pages(), because
> those talk about a zero return value.

OK.

>
> This might be practical without slowing down the existing code, because
> there is already a check in place, so just tweaking it like this (untested)
> won't change performance at all:
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 11fda538c9d9..708eed79ae29 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2787,7 +2787,7 @@ static int internal_get_user_pages_fast(unsigned long 
> start,
> int nr_pages,
>  end = start + len;
>
>  if (end <= start)
> -   return 0;
> +   return -EINVAL;
>  if (unlikely(!access_ok((void __user *)start, len)))
>  return -EFAULT;
>
> ...although I might be missing some other things that need a similar change,
> so you should look carefully for yourself.

Do you refer to other gup APIs similar to gup_fast/pup_fast ?

>
>
> Once that change (and anything I missed) is in place, then you could go
> ahead and stop handling ret==0 cases at the call sites.
>
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> >
> > Signed-off-by: Souptick Joarder 
> > ---
> >   arch/ia64/kernel/err_inject.c  | 2 +-
> >   drivers/platform/goldfish/goldfish_pipe.c  | 2 +-
> >   drivers/staging/gasket/gasket_page_table.c | 4 ++--
> >   drivers/tee/tee_shm.c  | 2 +-
> >   mm/gup.c   | 6 +++---
> >   net/rds/rdma.c | 2 +-
> >   6 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/ia64/kernel/err_inject.c b/arch/ia64/kernel/err_inject.c
> > index 8b5b8e6b..fd72218 100644
> > --- a/arch/ia64/kernel/err_inject.c
> > +++ b/arch/ia64/kernel/err_inject.c
> > @@ -143,7 +143,7 @@ static DEVICE_ATTR(name, 0644, show_##name, 
> > store_##name)
> >   int ret;
> >
> >   ret = get_user_pages_fast(virt_addr, 1, FOLL_WRITE, NULL);
> > - if (ret<=0) {
> > + if (ret < 0) {
> >   #ifdef ERR_INJ_DEBUG
> >   printk("Virtual address %lx is not existing.\n",virt_addr);
> >   #endif
> > diff --git a/drivers/platform/goldfish/goldfish_pipe.c 
> > b/drivers/platform/goldfish/goldfish_pipe.c
> > index 1ab207e..831449d 100644
> > --- a/drivers/platform/goldfish/goldfish_pipe.c
> > +++ b/drivers/platform/goldfish/goldfish_pipe.c
> > @@ -277,7 +277,7 @@ static int goldfish_pin_pages(unsigned long first_page,
> >   ret = pin_user_pages_fast(first_page, requested_pages,
> > !is_write ? FOLL_WRITE : 0,
> > pages);
> > - if (ret <= 0)
> > + if (ret < 0)
> >   return -EFAULT;
> >   if (ret < requested_pages)
> >   *iter_last_page_size = PAGE_SIZE;
> > diff --git a/drivers/staging/gasket/gasket_page_table.c 
> > b/drivers/staging/gasket/gasket_page_table.c
> > index f6d7157..1d08e1d 100644
> > --- a/drivers/staging/gasket/gasket_page_table.c
> > +++ b/drivers/staging/gasket/gasket_page_table.c
> > @@ -489,11 +489,11 @@ static int gasket_perform_mapping(struct 
> > gasket_page_table *pg_tbl,
> >   ret = get_user_pages_fast(page_addr - offset, 1,
> > FOLL_WRITE, );
> >
> > - if (ret <= 0) {
> > +   

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

On Thu, May 7, 2020 at 3:43 PM Jan Kara  wrote:
>
> On Wed 06-05-20 21:38:40, Souptick Joarder wrote:
> > On Wed, May 6, 2020 at 6:29 PM Jan Kara  wrote:
> > >
> > > On Wed 06-05-20 17:51:39, Souptick Joarder wrote:
> > > > On Wed, May 6, 2020 at 3:36 PM Jan Kara  wrote:
> > > > >
> > > > > On Wed 06-05-20 02:06:56, Souptick Joarder wrote:
> > > > > > On Wed, May 6, 2020 at 1:08 AM John Hubbard  
> > > > > > wrote:
> > > > > > >
> > > > > > > On 2020-05-05 12:14, Souptick Joarder wrote:
> > > > > > > > Currently {get|pin}_user_pages_fast() have 3 return value 0, 
> > > > > > > > -errno
> > > > > > > > and no of pinned pages. The only case where these two functions 
> > > > > > > > will
> > > > > > > > return 0, is for nr_pages <= 0, which doesn't find a valid use 
> > > > > > > > case.
> > > > > > > > But if at all any, then a -ERRNO will be returned instead of 0, 
> > > > > > > > which
> > > > > > > > means {get|pin}_user_pages_fast() will have 2 return values 
> > > > > > > > -errno &
> > > > > > > > no of pinned pages.
> > > > > > > >
> > > > > > > > Update all the callers which deals with return value 0 
> > > > > > > > accordingly.
> > > > > > >
> > > > > > > Hmmm, seems a little shaky. In order to do this safely, I'd 
> > > > > > > recommend
> > > > > > > first changing gup_fast/pup_fast so so that they return -EINVAL if
> > > > > > > the caller specified nr_pages==0, and of course auditing all 
> > > > > > > callers,
> > > > > > > to ensure that this won't cause problems.
> > > > > >
> > > > > > While auditing it was figured out, there are 5 callers which cares 
> > > > > > for
> > > > > > return value
> > > > > > 0 of gup_fast/pup_fast. What problem it might cause if we change
> > > > > > gup_fast/pup_fast
> > > > > > to return -EINVAL and update all the callers in a single commit ?
> > > > >
> > > > > Well, first I'd ask a different question: Why do you want to change 
> > > > > the
> > > > > current behavior? It's not like the current behavior is confusing.  
> > > > > Callers
> > > > > that pass >0 pages can happily rely on the simple behavior of < 0 
> > > > > return on
> > > > > error or > 0 return if we mapped some pages. Callers that can 
> > > > > possibly ask
> > > > > to map 0 pages can get 0 pages back - kind of expected - and I don't 
> > > > > see
> > > > > any benefit in trying to rewrite these callers to handle -EINVAL 
> > > > > instead...
> > > >
> > > > Callers with a request to map 0 pages doesn't have a valid use case. 
> > > > But if any
> > > > caller end up doing it mistakenly, -errno should be returned to caller
> > > > rather than 0
> > > > which will indicate more precisely that map 0 pages is not a valid
> > > > request from caller.
> > >
> > > Well, I believe this depends on the point of view. Similarly as reading 0
> > > bytes is successful, we could consider mapping 0 pages successful as well.
> > > And there can be valid cases where number of pages to map is computed from
> > > some input and when 0 pages should be mapped, it is not a problem and your
> > > change would force such callers to special case this with explicitely
> > > checking for 0 pages to map and not calling GUP in that case at all.
> > >
> > > I'm not saying what you propose is necessarily bad, I just say I don't 
> > > find
> > > it any better than the current behavior and so IMO it's not worth the
> > > churn. Now if you can come up with some examples of current in-kernel 
> > > users
> > > who indeed do get the handling of the return value wrong, I could be
> > > convinced otherwise.
> >
> > There are 5 callers of {get|pin}_user_pages_fast().
>
> Oh, there are *much* more callers that 5. It's more like 70. Just grep the
> source... And then you have all other {get|pin}_user_pages() variants that
> need 

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

On Wed, May 6, 2020 at 3:36 PM Jan Kara  wrote:
>
> On Wed 06-05-20 02:06:56, Souptick Joarder wrote:
> > On Wed, May 6, 2020 at 1:08 AM John Hubbard  wrote:
> > >
> > > On 2020-05-05 12:14, Souptick Joarder wrote:
> > > > Currently {get|pin}_user_pages_fast() have 3 return value 0, -errno
> > > > and no of pinned pages. The only case where these two functions will
> > > > return 0, is for nr_pages <= 0, which doesn't find a valid use case.
> > > > But if at all any, then a -ERRNO will be returned instead of 0, which
> > > > means {get|pin}_user_pages_fast() will have 2 return values -errno &
> > > > no of pinned pages.
> > > >
> > > > Update all the callers which deals with return value 0 accordingly.
> > >
> > > Hmmm, seems a little shaky. In order to do this safely, I'd recommend
> > > first changing gup_fast/pup_fast so so that they return -EINVAL if
> > > the caller specified nr_pages==0, and of course auditing all callers,
> > > to ensure that this won't cause problems.
> >
> > While auditing it was figured out, there are 5 callers which cares for
> > return value
> > 0 of gup_fast/pup_fast. What problem it might cause if we change
> > gup_fast/pup_fast
> > to return -EINVAL and update all the callers in a single commit ?
>
> Well, first I'd ask a different question: Why do you want to change the
> current behavior? It's not like the current behavior is confusing.  Callers
> that pass >0 pages can happily rely on the simple behavior of < 0 return on
> error or > 0 return if we mapped some pages. Callers that can possibly ask
> to map 0 pages can get 0 pages back - kind of expected - and I don't see
> any benefit in trying to rewrite these callers to handle -EINVAL instead...

Callers with a request to map 0 pages doesn't have a valid use case. But if any
caller end up doing it mistakenly, -errno should be returned to caller
rather than 0
which will indicate more precisely that map 0 pages is not a valid
request from caller.
With these, 3rd return value 0, is no more needed.

That was the thought behind this proposal.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast()

[Souptick Joarder]

On Wed, May 6, 2020 at 6:29 PM Jan Kara  wrote:
>
> On Wed 06-05-20 17:51:39, Souptick Joarder wrote:
> > On Wed, May 6, 2020 at 3:36 PM Jan Kara  wrote:
> > >
> > > On Wed 06-05-20 02:06:56, Souptick Joarder wrote:
> > > > On Wed, May 6, 2020 at 1:08 AM John Hubbard  wrote:
> > > > >
> > > > > On 2020-05-05 12:14, Souptick Joarder wrote:
> > > > > > Currently {get|pin}_user_pages_fast() have 3 return value 0, -errno
> > > > > > and no of pinned pages. The only case where these two functions will
> > > > > > return 0, is for nr_pages <= 0, which doesn't find a valid use case.
> > > > > > But if at all any, then a -ERRNO will be returned instead of 0, 
> > > > > > which
> > > > > > means {get|pin}_user_pages_fast() will have 2 return values -errno &
> > > > > > no of pinned pages.
> > > > > >
> > > > > > Update all the callers which deals with return value 0 accordingly.
> > > > >
> > > > > Hmmm, seems a little shaky. In order to do this safely, I'd recommend
> > > > > first changing gup_fast/pup_fast so so that they return -EINVAL if
> > > > > the caller specified nr_pages==0, and of course auditing all callers,
> > > > > to ensure that this won't cause problems.
> > > >
> > > > While auditing it was figured out, there are 5 callers which cares for
> > > > return value
> > > > 0 of gup_fast/pup_fast. What problem it might cause if we change
> > > > gup_fast/pup_fast
> > > > to return -EINVAL and update all the callers in a single commit ?
> > >
> > > Well, first I'd ask a different question: Why do you want to change the
> > > current behavior? It's not like the current behavior is confusing.  
> > > Callers
> > > that pass >0 pages can happily rely on the simple behavior of < 0 return 
> > > on
> > > error or > 0 return if we mapped some pages. Callers that can possibly ask
> > > to map 0 pages can get 0 pages back - kind of expected - and I don't see
> > > any benefit in trying to rewrite these callers to handle -EINVAL 
> > > instead...
> >
> > Callers with a request to map 0 pages doesn't have a valid use case. But if 
> > any
> > caller end up doing it mistakenly, -errno should be returned to caller
> > rather than 0
> > which will indicate more precisely that map 0 pages is not a valid
> > request from caller.
>
> Well, I believe this depends on the point of view. Similarly as reading 0
> bytes is successful, we could consider mapping 0 pages successful as well.
> And there can be valid cases where number of pages to map is computed from
> some input and when 0 pages should be mapped, it is not a problem and your
> change would force such callers to special case this with explicitely
> checking for 0 pages to map and not calling GUP in that case at all.
>
> I'm not saying what you propose is necessarily bad, I just say I don't find
> it any better than the current behavior and so IMO it's not worth the
> churn. Now if you can come up with some examples of current in-kernel users
> who indeed do get the handling of the return value wrong, I could be
> convinced otherwise.

There are 5 callers of {get|pin}_user_pages_fast().

arch/ia64/kernel/err_inject.c#L145
staging/gasket/gasket_page_table.c#L489

Checking return value 0 doesn't make sense for above 2.

drivers/platform/goldfish/goldfish_pipe.c#L277
net/rds/rdma.c#L165
drivers/tee/tee_shm.c#L262

These 3 callers have calculated the no of pages value before passing it to
{get|pin}_user_pages_fast(). But if they end up passing nr_pages <= 0, a return
value of either 0 or -EINVAL doesn't going to harm any existing
behavior of callers.

IMO, it is safe to return -errno for nr_pages <= 0, for
{get|pin}_user_pages_fast().
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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);
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()

[Souptick Joarder]

This code was using get_user_pages_fast(), in a "Case 2" scenario
(DMA/RDMA), using the categorization from [1]. That means that it's
time to convert the get_user_pages_fast() + put_page() calls to
pin_user_pages_fast() + unpin_user_page() calls.

There is some helpful background in [2]: basically, this is a small
part of fixing a long-standing disconnect between pinning pages, and
file systems' use of those pages.

[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 

Hi,

I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.
---
 drivers/staging/gasket/gasket_page_table.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c 
b/drivers/staging/gasket/gasket_page_table.c
index f6d7157..d712ad4 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -449,7 +449,7 @@ static bool gasket_release_page(struct page *page)
 
if (!PageReserved(page))
SetPageDirty(page);
-   put_page(page);
+   unpin_user_page(page);
 
return true;
 }
@@ -486,12 +486,12 @@ static int gasket_perform_mapping(struct 
gasket_page_table *pg_tbl,
ptes[i].dma_addr = pg_tbl->coherent_pages[0].paddr +
   off + i * PAGE_SIZE;
} else {
-   ret = get_user_pages_fast(page_addr - offset, 1,
+   ret = pin_user_pages_fast(page_addr - offset, 1,
  FOLL_WRITE, );
 
if (ret <= 0) {
dev_err(pg_tbl->device,
-   "get user pages failed for addr=0x%lx, 
offset=0x%lx [ret=%d]\n",
+   "pin user pages failed for addr=0x%lx, 
offset=0x%lx [ret=%d]\n",
page_addr, offset, ret);
return ret ? ret : -ENOMEM;
}
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()

[Souptick Joarder]

On Thu, May 28, 2020 at 4:34 PM Dan Carpenter  wrote:
>
> On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
> > This code was using get_user_pages_fast(), in a "Case 2" scenario
> > (DMA/RDMA), using the categorization from [1]. That means that it's
> > time to convert the get_user_pages_fast() + put_page() calls to
> > pin_user_pages_fast() + unpin_user_page() calls.
>
> You are saying that the page is used for DIO and not DMA, but it sure
> looks to me like it is used for DMA.

No, I was referring to "Case 2" scenario in change log which means  it is
used for DMA, not DIO.

>
>503  /* Map the page into DMA space. */
>504  ptes[i].dma_addr =
>505  dma_map_page(pg_tbl->device, page, 0, 
> PAGE_SIZE,
>506   DMA_BIDIRECTIONAL);
>
> To be honest, that starting paragraph was confusing.  At first I thought
> you were saying gasket was an RDMA driver. :P  I shouldn't have to read
> a different document to understand the commit message.  It should be
> summarized enough and the other documentation is supplemental.
>
> "In 2019 we introduced pin_user_pages() and now we are converting
> get_user_pages() to the new API as appropriate".

As all other similar conversion have similar change logs, so I was trying
to maintain the same. John might have a different opinion on this.

John, Any further opinion ??

>
> >
> > There is some helpful background in [2]: basically, this is a small
> > part of fixing a long-standing disconnect between pinning pages, and
> > file systems' use of those pages.
>
> What is the impact of this patch on runtime?

I don't have the hardware to validate the runtime impact and will
wait if someone is going to validate it for runtime impact.

>
> >
> > [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 
> >
> > Hi,
> >
> > I'm compile tested this, but unable to run-time test, so any testing
> > help is much appriciated.
> > ---
>
> The "Hi" part of patch should have been under the "---" cut off line so
> this will definitely need to be resent.

Sorry about it.
Will wait for feedback from John before resend it :)

>
> regards,
> dan carpenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: gasket: Convert get_user_pages*() --> pin_user_pages*()

[Souptick Joarder]

On Fri, May 29, 2020 at 11:46 AM Souptick Joarder  wrote:
>
> On Thu, May 28, 2020 at 4:34 PM Dan Carpenter  
> wrote:
> >
> > On Thu, May 28, 2020 at 02:32:42AM +0530, Souptick Joarder wrote:
> > > This code was using get_user_pages_fast(), in a "Case 2" scenario
> > > (DMA/RDMA), using the categorization from [1]. That means that it's
> > > time to convert the get_user_pages_fast() + put_page() calls to
> > > pin_user_pages_fast() + unpin_user_page() calls.
> >
> > You are saying that the page is used for DIO and not DMA, but it sure
> > looks to me like it is used for DMA.
>
> No, I was referring to "Case 2" scenario in change log which means  it is
> used for DMA, not DIO.
>
> >
> >503  /* Map the page into DMA space. */
> >504  ptes[i].dma_addr =
> >505  dma_map_page(pg_tbl->device, page, 
> > 0, PAGE_SIZE,
> >506   DMA_BIDIRECTIONAL);
> >
> > To be honest, that starting paragraph was confusing.  At first I thought
> > you were saying gasket was an RDMA driver. :P  I shouldn't have to read
> > a different document to understand the commit message.  It should be
> > summarized enough and the other documentation is supplemental.
> >
> > "In 2019 we introduced pin_user_pages() and now we are converting
> > get_user_pages() to the new API as appropriate".
>
> As all other similar conversion have similar change logs, so I was trying
> to maintain the same. John might have a different opinion on this.

For example, I was referring to few recent similar commits for change logs.

http://lkml.kernel.org/r/20200519002124.2025955-5-jhubb...@nvidia.com
https://lore.kernel.org/r/20200518015237.1568940-1-jhubb...@nvidia.com


>
> John, Any further opinion ??
>
> >
> > >
> > > There is some helpful background in [2]: basically, this is a small
> > > part of fixing a long-standing disconnect between pinning pages, and
> > > file systems' use of those pages.
> >
> > What is the impact of this patch on runtime?
>
> I don't have the hardware to validate the runtime impact and will
> wait if someone is going to validate it for runtime impact.
>
> >
> > >
> > > [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 
> > >
> > > Hi,
> > >
> > > I'm compile tested this, but unable to run-time test, so any testing
> > > help is much appriciated.
> > > ---
> >
> > The "Hi" part of patch should have been under the "---" cut off line so
> > this will definitely need to be resent.
>
> Sorry about it.
> Will wait for feedback from John before resend it :)
>
> >
> > regards,
> > dan carpenter
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: gasket: 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.

[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: Dan Carpenter 

---
Hi,

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

 drivers/staging/gasket/gasket_page_table.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c 
b/drivers/staging/gasket/gasket_page_table.c
index f6d7157..d712ad4 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -449,7 +449,7 @@ static bool gasket_release_page(struct page *page)
 
if (!PageReserved(page))
SetPageDirty(page);
-   put_page(page);
+   unpin_user_page(page);
 
return true;
 }
@@ -486,12 +486,12 @@ static int gasket_perform_mapping(struct 
gasket_page_table *pg_tbl,
ptes[i].dma_addr = pg_tbl->coherent_pages[0].paddr +
   off + i * PAGE_SIZE;
} else {
-   ret = get_user_pages_fast(page_addr - offset, 1,
+   ret = pin_user_pages_fast(page_addr - offset, 1,
  FOLL_WRITE, );
 
if (ret <= 0) {
dev_err(pg_tbl->device,
-   "get user pages failed for addr=0x%lx, 
offset=0x%lx [ret=%d]\n",
+   "pin user pages failed for addr=0x%lx, 
offset=0x%lx [ret=%d]\n",
page_addr, offset, ret);
return ret ? ret : -ENOMEM;
}
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] 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.

When pin_user_pages() returns numbers of partially mapped pages,
those pages were not unpinned as part of error handling. Fixed
it as part of this patch.

[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 
---
Hi,

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

 drivers/staging/kpc2000/kpc_dma/fileops.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8975346..29bab13 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -48,6 +48,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
u64 card_addr;
u64 dma_addr;
u64 user_ctl;
+   int nr_pages = 0;
 
ldev = priv->ldev;
 
@@ -76,13 +77,15 @@ 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 
(%ld)\n", rv);
+   dev_err(>ldev->pldev->dev, "Couldn't pin_user_pages 
(%ld)\n", rv);
+   nr_pages = rv;
goto err_get_user_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) {
@@ -189,10 +192,9 @@ 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]);
-
  err_get_user_pages:
+   if (nr_pages > 0)
+   unpin_user_pages(acd->user_pages, nr_pages);
kfree(acd->user_pages);
  err_alloc_userpages:
kfree(acd);
@@ -217,8 +219,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t 
xfr_count, u32 flags)
 
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]);
+   unpin_user_pages(acd->user_pages, acd->page_count);
 
sg_free_table(>sgt);
 
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: vc04_services: 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.

[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 
---
Hi,

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

 .../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c   | 16 +---
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c 
b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index 38a13e4..4616013 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -287,12 +287,8 @@ int vchiq_dump_platform_state(void *dump_context)
 pagelistinfo->num_pages, pagelistinfo->dma_dir);
}
 
-   if (pagelistinfo->pages_need_release) {
-   unsigned int i;
-
-   for (i = 0; i < pagelistinfo->num_pages; i++)
-   put_page(pagelistinfo->pages[i]);
-   }
+   if (pagelistinfo->pages_need_release)
+   unpin_user_pages(pagelistinfo->pages, pagelistinfo->num_pages);
 
dma_free_coherent(g_dev, pagelistinfo->pagelist_buffer_size,
  pagelistinfo->pagelist, pagelistinfo->dma_addr);
@@ -395,7 +391,7 @@ int vchiq_dump_platform_state(void *dump_context)
}
/* do not try and release vmalloc pages */
} else {
-   actual_pages = get_user_pages_fast(
+   actual_pages = pin_user_pages_fast(
  (unsigned long)buf & PAGE_MASK,
  num_pages,
  type == PAGELIST_READ,
@@ -407,10 +403,8 @@ int vchiq_dump_platform_state(void *dump_context)
   __func__, actual_pages, num_pages);
 
/* This is probably due to the process being killed */
-   while (actual_pages > 0) {
-   actual_pages--;
-   put_page(pages[actual_pages]);
-   }
+   if (actual_pages > 0)
+   unpin_user_pages(pages, actual_pages);
cleanup_pagelistinfo(pagelistinfo);
return NULL;
}
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/4] staging: kpc2000: kpc_dma: Remove excess goto statement

[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_user_pages.

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

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 1f5ab81..b0fcde5 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,23 @@ 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;
dev_err(>ldev->pldev->dev, "Couldn't pin_user_pages 
(%ld)\n", rv);
-   goto err_get_user_pages;
+   goto unpin_user_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 
(%ld)\n", rv);
-   goto err_alloc_sg_table;
+   goto unpin_user_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_user_pages;
}
 
// Calculate how many descriptors are actually needed for this transfer.
@@ -187,13 +188,9 @@ 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);
+ unpin_user_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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 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.

[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 | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index bcce86c..1f5ab81 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 
(%ld)\n", rv);
+   dev_err(>ldev->pldev->dev, "Couldn't pin_user_pages 
(%ld)\n", rv);
goto err_get_user_pages;
}
 
@@ -189,14 +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])
-   }
+   if (rv > 0)
+   unpin_user_pages(acd->user_pages, rv);
kfree(acd->user_pages);
  err_alloc_userpages:
kfree(acd);
@@ -221,8 +218,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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 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: Dan Carpenter 
Cc: Bharath Vedartham 
---
 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 b136353..bcce86c 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -214,13 +214,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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: gasket: 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.

[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 
Acked-by: Dan Carpenter 
Cc: John Hubbard 
Cc: Dan Carpenter 

---
Hi,

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

v2: 
Added review tag.

 drivers/staging/gasket/gasket_page_table.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/gasket/gasket_page_table.c 
b/drivers/staging/gasket/gasket_page_table.c
index f6d7157..d712ad4 100644
--- a/drivers/staging/gasket/gasket_page_table.c
+++ b/drivers/staging/gasket/gasket_page_table.c
@@ -449,7 +449,7 @@ static bool gasket_release_page(struct page *page)
 
if (!PageReserved(page))
SetPageDirty(page);
-   put_page(page);
+   unpin_user_page(page);
 
return true;
 }
@@ -486,12 +486,12 @@ static int gasket_perform_mapping(struct 
gasket_page_table *pg_tbl,
ptes[i].dma_addr = pg_tbl->coherent_pages[0].paddr +
   off + i * PAGE_SIZE;
} else {
-   ret = get_user_pages_fast(page_addr - offset, 1,
+   ret = pin_user_pages_fast(page_addr - offset, 1,
  FOLL_WRITE, );
 
if (ret <= 0) {
dev_err(pg_tbl->device,
-   "get user pages failed for addr=0x%lx, 
offset=0x%lx [ret=%d]\n",
+   "pin user pages failed for addr=0x%lx, 
offset=0x%lx [ret=%d]\n",
page_addr, offset, ret);
return ret ? ret : -ENOMEM;
}
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Souptick Joarder]

There is a bug, when get_user_pages() failed but partially pinned
pages are not unpinned. Fixed it.

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

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

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8975346..b136353 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);
@@ -193,6 +193,10 @@ 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])
+   }
kfree(acd->user_pages);
  err_alloc_userpages:
kfree(acd);
-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 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.

Souptick Joarder (4):
  staging: kpc2000: 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 excess goto statement

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

-- 
1.9.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Souptick Joarder]

On Wed, Jun 17, 2020 at 4:43 PM Dan Carpenter  wrote:
>
> On Wed, Jun 17, 2020 at 07:57:20AM +0530, Souptick Joarder wrote:
> > There is a bug, when get_user_pages() failed but partially pinned
> > pages are not unpinned. Fixed it.
> >
> > Also, int is more appropriate type for rv. Changed it.
> >
> > Signed-off-by: Souptick Joarder 
> > Cc: John Hubbard 
> > Cc: Bharath Vedartham 
> > Cc: Dan Carpenter 
> > ---
> >  drivers/staging/kpc2000/kpc_dma/fileops.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
> > b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > index 8975346..b136353 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);
> > @@ -193,6 +193,10 @@ 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])
> 
>
> > + }
>
> This isn't a complete fix.  "rv" is the negative error code but here we
> are returning a positive value on this path.

In case of error of get_user_pages(), it will return -errno, 0 and 3rd one is
(rv > 0 && rv != acd->page_count). When rv is -errno or 0 there is no need
to call put_pages() in error path. But for 3rd case partially mapped pages
need to unpin.

Correct me if I am missing anything.

>Also it's ugly to have
> same put_page() loop repeated twice.

Yes, I agree, but these are intermediate steps. Patch [4/4] of this series
finally did the same.

>
> It would be better to write it like this:
>
> 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 < 0)
> goto free_pages;
> if (rv != acd->page_count) {
> acd->page_count = rv;
> rv = -EFAULT;
> goto put_pages;
> }
>
> ...
>
> put_pages:
> for (i = 0 ; i < acd->page_count ; i++)
> put_pages(acd->user_pages[i]);
> free_pages:
> kfree(acd->user_pages);
>
> regards,
> dan carpenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Souptick Joarder]

On Wed, Jun 17, 2020 at 11:29 PM Dan Carpenter  wrote:
>
> On Wed, Jun 17, 2020 at 11:13:32PM +0530, Souptick Joarder wrote:
> > On Wed, Jun 17, 2020 at 4:43 PM Dan Carpenter  
> > wrote:
> > >
> > > On Wed, Jun 17, 2020 at 07:57:20AM +0530, Souptick Joarder wrote:
> > > > There is a bug, when get_user_pages() failed but partially pinned
> > > > pages are not unpinned. Fixed it.
> > > >
> > > > Also, int is more appropriate type for rv. Changed it.
> > > >
> > > > Signed-off-by: Souptick Joarder 
> > > > Cc: John Hubbard 
> > > > Cc: Bharath Vedartham 
> > > > Cc: Dan Carpenter 
> > > > ---
> > > >  drivers/staging/kpc2000/kpc_dma/fileops.c | 6 +-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
> > > > b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > > index 8975346..b136353 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);
> > > > @@ -193,6 +193,10 @@ 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])
> > > 
> > >
> > > > + }
> > >
> > > This isn't a complete fix.  "rv" is the negative error code but here we
> > > are returning a positive value on this path.
> >
> > In case of error of get_user_pages(), it will return -errno, 0 and 3rd one 
> > is
> > (rv > 0 && rv != acd->page_count). When rv is -errno or 0 there is no need
> > to call put_pages() in error path. But for 3rd case partially mapped pages
> > need to unpin.
> >
> > Correct me if I am missing anything.
> >
>
>182  kfree(acd);
>183  }
>184  return rv;
>185
>186   err_descr_too_many:
>187  unlock_engine(ldev);
>188  dma_unmap_sg(>pldev->dev, acd->sgt.sgl, acd->sgt.nents, 
> ldev->dir);
>189  sg_free_table(>sgt);
>190   err_dma_map_sg:
>191   err_alloc_sg_table:
>192  for (i = 0 ; i < acd->page_count ; i++)
>193  put_page(acd->user_pages[i]);
>194
>195   err_get_user_pages:
>196  if (rv > 0) {
> ^^
> "rv" is positive.
>
>197  for (i = 0; i < rv; i++)
>198  put_pages(acd->user_pages[i])
>199  }
>200  kfree(acd->user_pages);
>201   err_alloc_userpages:
>202  kfree(acd);
>203  dev_dbg(>ldev->pldev->dev, "%s returning with error 
> %ld\n", __func__, rv);
>204  return rv;
>^^
> "rv" is still positive but it should be -EFAULT.
>

Ahh,  my mistake. Will correct it in v2.
Do other patches in the series looks good ?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Souptick Joarder]

On Wed, Jun 17, 2020 at 7:50 AM Souptick Joarder  wrote:
>
> 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: Dan Carpenter 
> Cc: Bharath Vedartham 
> ---
>  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 b136353..bcce86c 100644
> --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> @@ -214,13 +214,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]))

Question -> is PageReserved() used with specific purpose not PageDirty() ??

> -   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
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

On Mon, Jun 1, 2020 at 7:15 AM John Hubbard  wrote:
>
> On 2020-05-31 10:51, 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.
> >
> > When pin_user_pages() returns numbers of partially mapped pages,
> > those pages were not unpinned as part of error handling. Fixed
> > it as part of this patch.
> >
>
> Hi Souptick,
>
> btw, Bharath (+cc) attempted to do the "put" side of this, last year.
> That got as far as a v4 patch [1], and then I asked him to let me put
> it into my tree. But then it didn't directly apply anymore after the
> whole design moved to pin+unpin, and so here we are now.
>
>
> If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
> tag (see 
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).

Sure, will add him as *Co-Developed-by*
>
> Anyway, I'd recommend splitting the bug fix(es) into it at least one
> separate patch. That's a "best practice", and I don't see any reason
> not to do it here, even though the bugs are not huge.
>
> Also I think there may be more than one bug to fix, because I just
> noticed that the pre-existing code is doing set_page_dirty(), when
> it should be doing set_page_dirty_lock(). See below.
>
>
> > [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 
> > ---
> > Hi,
> >
> > I'm compile tested this, but unable to run-time test, so any testing
> > help is much appriciated.
> >
> >   drivers/staging/kpc2000/kpc_dma/fileops.c | 15 ---
> >   1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c 
> > b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > index 8975346..29bab13 100644
> > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > @@ -48,6 +48,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> >   u64 card_addr;
> >   u64 dma_addr;
> >   u64 user_ctl;
> > + int nr_pages = 0;
>
> Probably best to correct the "rv" type as well: it should be an int, rather
> than a long.

Noted.

>
> >
> >   ldev = priv->ldev;
> >
> > @@ -76,13 +77,15 @@ 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 
> > (%ld)\n", rv);
> > + dev_err(>ldev->pldev->dev, "Couldn't pin_user_pages 
> > (%ld)\n", rv);
> > + nr_pages = rv;
> >   goto err_get_user_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) {
> > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data 
> > *priv,
> >   sg_free_table(>sgt);
> >err_dma_map_sg:
> >err_alloc_sg_table:
>
> So now we end up with two unnecessary labels. Probably best to delete two of 
> these
> three and name the remaining one appropriately:

Hmm, I thought about it. But later decided to wait for review comments
on the same in v1.
I will remove it now.

>
>   err_dma_map_sg:
>   err_alloc_sg_table:
>   err_get_user_pages:
>
> > - for (i = 0 ; i < acd->page_count ; i++)
> > - put_page(acd->user_pages[i]);
> > -
> >err_get_user_pages:
> > + if (nr_pages > 0)
> > + unpin_user_pages(acd->user_pages, nr_pages);
> >   kfree(acd->user_pages

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

On Tue, Jun 9, 2020 at 12:47 AM Dan Carpenter  wrote:
>
> On Tue, Jun 09, 2020 at 12:31:42AM +0530, Souptick Joarder wrote:
> > > > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct 
> > > > dev_private_data *priv,
> > > >   sg_free_table(>sgt);
> > > >err_dma_map_sg:
> > > >err_alloc_sg_table:
> > >
> > > So now we end up with two unnecessary labels. Probably best to delete two 
> > > of these
> > > three and name the remaining one appropriately:
> >
> > Hmm, I thought about it. But later decided to wait for review comments
> > on the same in v1.
> > I will remove it now.
>
> These are all unrelated to pin_user_pages().  Please don't do it in the
> same patch. Staging code is there because it's ugly...  If you don't
> want to do unrelated changes to label names then you don't have to.

What I am planning is to put this changes in a series. One patch will take care
of pin_user_pages() related changes, 2nd patch will take care of minor bug
fix in error path + level correction and 3rd patch
will take care of set_page_dirty() -> set_page_dirty_lock().

Does it make sense ?

>
> Also on a personal note.  The label name should say what the goto does
> just like a function name says what the function does.  "goto put_pages;"
> Or "goto free_foo;".
>
> Don't do this:
>
> p = kmalloc();
> if (!p)
> goto kmalloc_failed;
>
> This is a come-from label name and does not provide any information that
> is not there on the line above...
>
> If you send a patch which uses your own personal style of label names,
> I won't say anything about unless there is a bug.  But just know in your
> heart that you are wrong and I have silently reviewed your patch to
> drivers/staging.
>
> regards,
> dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
> > >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel