[PATCH 1/2] tpm: replace kmalloc() + memcpy() with kmemdup()

[Ji-Hun Kim]

Use kmemdup rather than duplicating its implementation.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
 drivers/char/tpm/eventlog/of.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index b7cac47..bba5fba 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -69,14 +69,12 @@ int tpm_read_log_of(struct tpm_chip *chip)
return -EIO;
}
 
-   log->bios_event_log = kmalloc(size, GFP_KERNEL);
+   log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL);
if (!log->bios_event_log)
return -ENOMEM;
 
log->bios_event_log_end = log->bios_event_log + size;
 
-   memcpy(log->bios_event_log, __va(base), size);
-
if (chip->flags & TPM_CHIP_FLAG_TPM2)
return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
-- 
1.9.1

[PATCH 1/2] tpm: replace kmalloc() + memcpy() with kmemdup()

[Ji-Hun Kim]

Use kmemdup rather than duplicating its implementation.

Signed-off-by: Ji-Hun Kim 
---
 drivers/char/tpm/eventlog/of.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index b7cac47..bba5fba 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -69,14 +69,12 @@ int tpm_read_log_of(struct tpm_chip *chip)
return -EIO;
}
 
-   log->bios_event_log = kmalloc(size, GFP_KERNEL);
+   log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL);
if (!log->bios_event_log)
return -ENOMEM;
 
log->bios_event_log_end = log->bios_event_log + size;
 
-   memcpy(log->bios_event_log, __va(base), size);
-
if (chip->flags & TPM_CHIP_FLAG_TPM2)
return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
-- 
1.9.1

[PATCH 2/2] tpm: replace kmalloc() + memcpy() with kmemdup()

[Ji-Hun Kim]

Use kmemdup rather than duplicating its implementation.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
 drivers/char/tpm/eventlog/efi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 68f1e7e..3e673ab 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -51,10 +51,9 @@ int tpm_read_log_efi(struct tpm_chip *chip)
}
 
/* malloc EventLog space */
-   log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
+   log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
if (!log->bios_event_log)
goto err_memunmap;
-   memcpy(log->bios_event_log, log_tbl->log, log_size);
log->bios_event_log_end = log->bios_event_log + log_size;
 
tpm_log_version = log_tbl->version;
-- 
1.9.1

[PATCH 2/2] tpm: replace kmalloc() + memcpy() with kmemdup()

[Ji-Hun Kim]

Use kmemdup rather than duplicating its implementation.

Signed-off-by: Ji-Hun Kim 
---
 drivers/char/tpm/eventlog/efi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 68f1e7e..3e673ab 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -51,10 +51,9 @@ int tpm_read_log_efi(struct tpm_chip *chip)
}
 
/* malloc EventLog space */
-   log->bios_event_log = kmalloc(log_size, GFP_KERNEL);
+   log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
if (!log->bios_event_log)
goto err_memunmap;
-   memcpy(log->bios_event_log, log_tbl->log, log_size);
log->bios_event_log_end = log->bios_event_log + log_size;
 
tpm_log_version = log_tbl->version;
-- 
1.9.1

[PATCH] staging: ks7010: replace kmalloc() + memcpy() with kmemdup()

[Ji-Hun Kim]

Use kmemdup rather than duplicating its implementation.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
 drivers/staging/ks7010/ks7010_sdio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index b8f55a1..c8eb55b 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -589,11 +589,10 @@ static int ks7010_sdio_update_index(struct 
ks_wlan_private *priv, u32 index)
int ret;
unsigned char *data_buf;
 
-   data_buf = kmalloc(sizeof(u32), GFP_KERNEL);
+   data_buf = kmemdup(, sizeof(u32), GFP_KERNEL);
if (!data_buf)
return -ENOMEM;
 
-   memcpy(data_buf, , sizeof(index));
ret = ks7010_sdio_write(priv, WRITE_INDEX, data_buf, sizeof(index));
if (ret)
goto err_free_data_buf;
-- 
1.9.1

[PATCH] staging: ks7010: replace kmalloc() + memcpy() with kmemdup()

[Ji-Hun Kim]

Use kmemdup rather than duplicating its implementation.

Signed-off-by: Ji-Hun Kim 
---
 drivers/staging/ks7010/ks7010_sdio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/ks7010/ks7010_sdio.c 
b/drivers/staging/ks7010/ks7010_sdio.c
index b8f55a1..c8eb55b 100644
--- a/drivers/staging/ks7010/ks7010_sdio.c
+++ b/drivers/staging/ks7010/ks7010_sdio.c
@@ -589,11 +589,10 @@ static int ks7010_sdio_update_index(struct 
ks_wlan_private *priv, u32 index)
int ret;
unsigned char *data_buf;
 
-   data_buf = kmalloc(sizeof(u32), GFP_KERNEL);
+   data_buf = kmemdup(, sizeof(u32), GFP_KERNEL);
if (!data_buf)
return -ENOMEM;
 
-   memcpy(data_buf, , sizeof(index));
ret = ks7010_sdio_write(priv, WRITE_INDEX, data_buf, sizeof(index));
if (ret)
goto err_free_data_buf;
-- 
1.9.1

[PATCH v5 2/2] staging: vt6655: add handling memory leak on vnt_start()

[Ji-Hun Kim]

There was no code for handling memory leaks of device_init_rings() and
request_irq(). It needs to free allocated memory in the device_init_rings()
, when request_irq() would be failed. Add freeing sequences of irq and
device init rings.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
It's additional memory leak handling patch from the
[PATCH v5 1/2] staging: vt6655: check for memory allocation failures

Changes v2:
- Change label names following coding-style conventions.
- Remove uneccessary goto, just return error number like original code.

 drivers/staging/vt6655/device_main.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index 700c03c..1ab0e85 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -1237,13 +1237,13 @@ static int vnt_start(struct ieee80211_hw *hw)
  IRQF_SHARED, "vt6655", priv);
if (ret) {
dev_dbg(>pcid->dev, "failed to start irq\n");
-   return ret;
+   goto err_free_rings;
}
 
dev_dbg(>pcid->dev, "call device init rd0 ring\n");
ret = device_init_rd0_ring(priv);
if (ret)
-   return ret;
+   goto err_free_irq;
ret = device_init_rd1_ring(priv);
if (ret)
goto err_free_rd0_ring;
@@ -1269,6 +1269,10 @@ static int vnt_start(struct ieee80211_hw *hw)
device_free_rd1_ring(priv);
 err_free_rd0_ring:
device_free_rd0_ring(priv);
+err_free_irq:
+   free_irq(priv->pcid->irq, priv);
+err_free_rings:
+   device_free_rings(priv);
return ret;
 }
 
-- 
1.9.1

[PATCH v5 2/2] staging: vt6655: add handling memory leak on vnt_start()

[Ji-Hun Kim]

There was no code for handling memory leaks of device_init_rings() and
request_irq(). It needs to free allocated memory in the device_init_rings()
, when request_irq() would be failed. Add freeing sequences of irq and
device init rings.

Signed-off-by: Ji-Hun Kim 
---
It's additional memory leak handling patch from the
[PATCH v5 1/2] staging: vt6655: check for memory allocation failures

Changes v2:
- Change label names following coding-style conventions.
- Remove uneccessary goto, just return error number like original code.

 drivers/staging/vt6655/device_main.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index 700c03c..1ab0e85 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -1237,13 +1237,13 @@ static int vnt_start(struct ieee80211_hw *hw)
  IRQF_SHARED, "vt6655", priv);
if (ret) {
dev_dbg(>pcid->dev, "failed to start irq\n");
-   return ret;
+   goto err_free_rings;
}
 
dev_dbg(>pcid->dev, "call device init rd0 ring\n");
ret = device_init_rd0_ring(priv);
if (ret)
-   return ret;
+   goto err_free_irq;
ret = device_init_rd1_ring(priv);
if (ret)
goto err_free_rd0_ring;
@@ -1269,6 +1269,10 @@ static int vnt_start(struct ieee80211_hw *hw)
device_free_rd1_ring(priv);
 err_free_rd0_ring:
device_free_rd0_ring(priv);
+err_free_irq:
+   free_irq(priv->pcid->irq, priv);
+err_free_rings:
+   device_free_rings(priv);
return ret;
 }
 
-- 
1.9.1

[PATCH v5 1/2] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
Changes v5:
- Add error handling case for device_alloc_rx_buf() failures.
- Add device_free_rx_buf() which is corresponding free function of
  device_allocated_rx_buf(). And change duplicated codes by this function.
- Modify error handling code about freeing allocated value in the loops to
  more proper ways.
- Change goto label names following coding-style conventions.

Changes v4:
- Fix potential memory leaks from error handling code from device init
  functions in vnt_start().

Changes v3:
- Modify return type of device_init_rd*, device_init_td*. Then add returns
  error code at those functions and vnt_start as well.

Changes v2:
- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
  values.

 drivers/staging/vt6655/device_main.c | 144 ---
 1 file changed, 118 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..700c03c 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -19,6 +19,7 @@
  *   device_print_info - print out resource
  *   device_rx_srv - rx service function
  *   device_alloc_rx_buf - rx buffer pre-allocated function
+ *   device_free_rx_buf - free rx buffer function
  *   device_free_tx_buf - free tx buffer function
  *   device_init_rd0_ring- initial rd dma0 ring
  *   device_init_rd1_ring- initial rd dma1 ring
@@ -124,14 +125,15 @@
 static void device_free_info(struct vnt_private *priv);
 static void device_print_info(struct vnt_private *priv);
 
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
 
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
 static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rx_desc *);
+static void device_free_rx_buf(struct vnt_private *priv, struct vnt_rx_desc 
*rd);
 static void device_init_registers(struct vnt_private *priv);
 static void device_free_tx_buf(struct vnt_private *, struct vnt_tx_desc *);
 static void device_free_td0_ring(struct vnt_private *priv);
@@ -528,20 +530,28 @@ static void device_free_rings(struct vnt_private *priv)
  priv->tx0_bufs, priv->tx_bufs_dma0);
 }
 
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd0_pool_dma;
struct vnt_rx_desc *desc;
+   int ret;
 
/* Init the RD0 ring entries */
for (i = 0; i < priv->opts.rx_descs0;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto err_free_desc;
+   }
 
-   if (!device_alloc_rx_buf(priv, desc))
+   if (!device_alloc_rx_buf(priv, desc)) {
dev_err(>pcid->dev, "can not alloc rx bufs\n");
+   ret = -ENOMEM;
+   goto err_free_rd;
+   }
 
desc->next = >aRD0Ring[(i + 1) % priv->opts.rx_descs0];
desc->next_desc = cpu_to_le32(curr + sizeof(struct 
vnt_rx_desc));
@@ -550,22 +560,44 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = >aRD0Ring[0];
+
+   return 0;
+
+err_free_rd:
+   kfree(desc->rd_info);
+
+err_free_desc:
+   while (--i) {
+   desc = >aRD0Ring[i];
+   device_free_rx_buf(priv, desc);
+   kfree(desc->rd_info);
+   }
+
+   return ret;
 }
 
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv

[PATCH v5 1/2] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.

Signed-off-by: Ji-Hun Kim 
---
Changes v5:
- Add error handling case for device_alloc_rx_buf() failures.
- Add device_free_rx_buf() which is corresponding free function of
  device_allocated_rx_buf(). And change duplicated codes by this function.
- Modify error handling code about freeing allocated value in the loops to
  more proper ways.
- Change goto label names following coding-style conventions.

Changes v4:
- Fix potential memory leaks from error handling code from device init
  functions in vnt_start().

Changes v3:
- Modify return type of device_init_rd*, device_init_td*. Then add returns
  error code at those functions and vnt_start as well.

Changes v2:
- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
  values.

 drivers/staging/vt6655/device_main.c | 144 ---
 1 file changed, 118 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..700c03c 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -19,6 +19,7 @@
  *   device_print_info - print out resource
  *   device_rx_srv - rx service function
  *   device_alloc_rx_buf - rx buffer pre-allocated function
+ *   device_free_rx_buf - free rx buffer function
  *   device_free_tx_buf - free tx buffer function
  *   device_init_rd0_ring- initial rd dma0 ring
  *   device_init_rd1_ring- initial rd dma1 ring
@@ -124,14 +125,15 @@
 static void device_free_info(struct vnt_private *priv);
 static void device_print_info(struct vnt_private *priv);
 
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
 
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
 static bool device_alloc_rx_buf(struct vnt_private *, struct vnt_rx_desc *);
+static void device_free_rx_buf(struct vnt_private *priv, struct vnt_rx_desc 
*rd);
 static void device_init_registers(struct vnt_private *priv);
 static void device_free_tx_buf(struct vnt_private *, struct vnt_tx_desc *);
 static void device_free_td0_ring(struct vnt_private *priv);
@@ -528,20 +530,28 @@ static void device_free_rings(struct vnt_private *priv)
  priv->tx0_bufs, priv->tx_bufs_dma0);
 }
 
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd0_pool_dma;
struct vnt_rx_desc *desc;
+   int ret;
 
/* Init the RD0 ring entries */
for (i = 0; i < priv->opts.rx_descs0;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto err_free_desc;
+   }
 
-   if (!device_alloc_rx_buf(priv, desc))
+   if (!device_alloc_rx_buf(priv, desc)) {
dev_err(>pcid->dev, "can not alloc rx bufs\n");
+   ret = -ENOMEM;
+   goto err_free_rd;
+   }
 
desc->next = >aRD0Ring[(i + 1) % priv->opts.rx_descs0];
desc->next_desc = cpu_to_le32(curr + sizeof(struct 
vnt_rx_desc));
@@ -550,22 +560,44 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = >aRD0Ring[0];
+
+   return 0;
+
+err_free_rd:
+   kfree(desc->rd_info);
+
+err_free_desc:
+   while (--i) {
+   desc = >aRD0Ring[i];
+   device_free_rx_buf(priv, desc);
+   kfree(desc->rd_info);
+   }
+
+   return ret;
 }
 
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv)
 {
int i;
dma_addr_

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Tue, Apr 03, 2018 at 01:40:52PM +0300, Dan Carpenter wrote:
> > desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> > -
> > +   if (!desc->rd_info) {
> > +   ret = -ENOMEM;
> > +   goto error;
> > +   }
> > if (!device_alloc_rx_buf(priv, desc))
> > dev_err(>pcid->dev, "can not alloc rx bufs\n");
> >  
> 
> We need to handle the case where device_alloc_rx_buf() fails as well...

Hi Dan, thanks for your comments! I will write a patch v5 including this fix.

> Some years back, I wrote a post about error handling that might be
> helpful:
> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

This post is very helpful to me, Thank you.

> You are using "one err" and "do nothing" style error handling which are
> described in the post.

Sorry, I didn't fully understand "one err" and "do nothing" style error
handling of this code. The reason using goto instead of returns directly
was that for freeing previously allocated "rd_info"s in the for loop.
Please see below comment.


> > @@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private 
> > *priv)
> > if (i > 0)
> > priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
> > priv->pCurrRD[0] = >aRD0Ring[0];
> > +
> > +   return 0;
> > +error:
> > +   device_free_rd0_ring(priv);
> > +   return ret;
> >  }
> 
> Of course, Jia-Ju Bai is correct to say that this is a layering
> violation.  Each function should only clean up after its self.
> 
> Also, this is a very typical "one err" style bug which I explain about
> in my g+ post.  The rule that applies here is that you should only free
> things which have been allocated.  Since we only partially allocated the
> rd0 ring, device_free_rd0_ring() will crash when we do:
> 
>   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
>priv->rx_buf_sz, DMA_FROM_DEVICE);
> 
> "rd_info" is NULL so rd_info->skb_dma is a NULL dereference.

For dealing with this problem, I added below code on patch v3. I think it
would not make Null dereferencing issues, because it will not enter 
dma_unmap_single(), if "rd_info" is Null.

+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   priv->rx_buf_sz, DMA_FROM_DEVICE);
+   dev_kfree_skb(rd_info->skb);
+   kfree(desc->rd_info);
+   }

I would appreciate for your opinions what would be better way for freeing
allocated "rd_info"s in the loop previously.

Best regards,
Ji-Hun

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Tue, Apr 03, 2018 at 01:40:52PM +0300, Dan Carpenter wrote:
> > desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
> > -
> > +   if (!desc->rd_info) {
> > +   ret = -ENOMEM;
> > +   goto error;
> > +   }
> > if (!device_alloc_rx_buf(priv, desc))
> > dev_err(>pcid->dev, "can not alloc rx bufs\n");
> >  
> 
> We need to handle the case where device_alloc_rx_buf() fails as well...

Hi Dan, thanks for your comments! I will write a patch v5 including this fix.

> Some years back, I wrote a post about error handling that might be
> helpful:
> https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ

This post is very helpful to me, Thank you.

> You are using "one err" and "do nothing" style error handling which are
> described in the post.

Sorry, I didn't fully understand "one err" and "do nothing" style error
handling of this code. The reason using goto instead of returns directly
was that for freeing previously allocated "rd_info"s in the for loop.
Please see below comment.


> > @@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private 
> > *priv)
> > if (i > 0)
> > priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
> > priv->pCurrRD[0] = >aRD0Ring[0];
> > +
> > +   return 0;
> > +error:
> > +   device_free_rd0_ring(priv);
> > +   return ret;
> >  }
> 
> Of course, Jia-Ju Bai is correct to say that this is a layering
> violation.  Each function should only clean up after its self.
> 
> Also, this is a very typical "one err" style bug which I explain about
> in my g+ post.  The rule that applies here is that you should only free
> things which have been allocated.  Since we only partially allocated the
> rd0 ring, device_free_rd0_ring() will crash when we do:
> 
>   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
>priv->rx_buf_sz, DMA_FROM_DEVICE);
> 
> "rd_info" is NULL so rd_info->skb_dma is a NULL dereference.

For dealing with this problem, I added below code on patch v3. I think it
would not make Null dereferencing issues, because it will not enter 
dma_unmap_single(), if "rd_info" is Null.

+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   priv->rx_buf_sz, DMA_FROM_DEVICE);
+   dev_kfree_skb(rd_info->skb);
+   kfree(desc->rd_info);
+   }

I would appreciate for your opinions what would be better way for freeing
allocated "rd_info"s in the loop previously.

Best regards,
Ji-Hun

[PATCH v4 2/2] staging: vt6655: add handling memory leak on vnt_start()

[Ji-Hun Kim]

There was no code for handling memory leaks of device_init_rings() and
request_irq(). It needs to free allocated memory in the device_init_rings()
, when request_irq() is failed. Add freeing sequences of irq and device
init rings.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
It's additional memory leak handling patch from
[PATCH v4 1/2] staging: vt6655: check for memory allocation failures

 drivers/staging/vt6655/device_main.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index c9752df..3604f2d 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -1194,14 +1194,17 @@ static int vnt_start(struct ieee80211_hw *hw)
int ret;
 
priv->rx_buf_sz = PKT_BUF_SZ;
-   if (!device_init_rings(priv))
-   return -ENOMEM;
+   ret = (int)device_init_rings(priv);
+   if (!ret) {
+   ret = -ENOMEM;
+   goto err_init_rings;
+   }
 
ret = request_irq(priv->pcid->irq, vnt_interrupt,
  IRQF_SHARED, "vt6655", priv);
if (ret) {
dev_dbg(>pcid->dev, "failed to start irq\n");
-   return ret;
+   goto err_irq;
}
 
dev_dbg(>pcid->dev, "call device init rd0 ring\n");
@@ -1234,6 +1237,10 @@ static int vnt_start(struct ieee80211_hw *hw)
 err_init_rd1_ring:
device_free_rd0_ring(priv);
 err_init_rd0_ring:
+   free_irq(priv->pcid->irq, priv);
+err_irq:
+   device_free_rings(priv);
+err_init_rings:
return ret;
 }
 
-- 
1.9.1

[PATCH v4 2/2] staging: vt6655: add handling memory leak on vnt_start()

[Ji-Hun Kim]

There was no code for handling memory leaks of device_init_rings() and
request_irq(). It needs to free allocated memory in the device_init_rings()
, when request_irq() is failed. Add freeing sequences of irq and device
init rings.

Signed-off-by: Ji-Hun Kim 
---
It's additional memory leak handling patch from
[PATCH v4 1/2] staging: vt6655: check for memory allocation failures

 drivers/staging/vt6655/device_main.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index c9752df..3604f2d 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -1194,14 +1194,17 @@ static int vnt_start(struct ieee80211_hw *hw)
int ret;
 
priv->rx_buf_sz = PKT_BUF_SZ;
-   if (!device_init_rings(priv))
-   return -ENOMEM;
+   ret = (int)device_init_rings(priv);
+   if (!ret) {
+   ret = -ENOMEM;
+   goto err_init_rings;
+   }
 
ret = request_irq(priv->pcid->irq, vnt_interrupt,
  IRQF_SHARED, "vt6655", priv);
if (ret) {
dev_dbg(>pcid->dev, "failed to start irq\n");
-   return ret;
+   goto err_irq;
}
 
dev_dbg(>pcid->dev, "call device init rd0 ring\n");
@@ -1234,6 +1237,10 @@ static int vnt_start(struct ieee80211_hw *hw)
 err_init_rd1_ring:
device_free_rd0_ring(priv);
 err_init_rd0_ring:
+   free_irq(priv->pcid->irq, priv);
+err_irq:
+   device_free_rings(priv);
+err_init_rings:
return ret;
 }
 
-- 
1.9.1

[PATCH v4 1/2] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
Changes v2:
- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
  values.

Changes v3:
- Modify return type of device_init_rd*, device_init_td*. Then add returns
  error code at those functions and vnt_start as well.

Changes v4:
- Fix potential memory leaks from error handling code of device init
  functions in vnt_start().

 drivers/staging/vt6655/device_main.c | 121 ++-
 1 file changed, 89 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..c9752df 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -124,10 +124,10 @@
 static void device_free_info(struct vnt_private *priv);
 static void device_print_info(struct vnt_private *priv);
 
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
 
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
@@ -528,18 +528,22 @@ static void device_free_rings(struct vnt_private *priv)
  priv->tx0_bufs, priv->tx_bufs_dma0);
 }
 
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd0_pool_dma;
struct vnt_rx_desc *desc;
+   int ret = 0;
 
/* Init the RD0 ring entries */
for (i = 0; i < priv->opts.rx_descs0;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto error;
+   }
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = >aRD0Ring[0];
+
+   return 0;
+error:
+   device_free_rd0_ring(priv);
+   return ret;
 }
 
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd1_pool_dma;
struct vnt_rx_desc *desc;
+   int ret = 0;
 
/* Init the RD1 ring entries */
for (i = 0; i < priv->opts.rx_descs1;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto error;
+   }
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -574,6 +587,11 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = >aRD1Ring[0];
+
+   return 0;
+error:
+   device_free_rd1_ring(priv);
+   return ret;
 }
 
 static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +602,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = >aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
 
-   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-   dev_kfree_skb(rd_info->skb);
-
-   kfree(desc->rd_info);
+   if (rd_info) {
+   dma_unmap_single(>pcid->de

[PATCH v4 1/2] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.

Signed-off-by: Ji-Hun Kim 
---
Changes v2:
- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
  values.

Changes v3:
- Modify return type of device_init_rd*, device_init_td*. Then add returns
  error code at those functions and vnt_start as well.

Changes v4:
- Fix potential memory leaks from error handling code of device init
  functions in vnt_start().

 drivers/staging/vt6655/device_main.c | 121 ++-
 1 file changed, 89 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..c9752df 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -124,10 +124,10 @@
 static void device_free_info(struct vnt_private *priv);
 static void device_print_info(struct vnt_private *priv);
 
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
 
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
@@ -528,18 +528,22 @@ static void device_free_rings(struct vnt_private *priv)
  priv->tx0_bufs, priv->tx_bufs_dma0);
 }
 
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd0_pool_dma;
struct vnt_rx_desc *desc;
+   int ret = 0;
 
/* Init the RD0 ring entries */
for (i = 0; i < priv->opts.rx_descs0;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto error;
+   }
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = >aRD0Ring[0];
+
+   return 0;
+error:
+   device_free_rd0_ring(priv);
+   return ret;
 }
 
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd1_pool_dma;
struct vnt_rx_desc *desc;
+   int ret = 0;
 
/* Init the RD1 ring entries */
for (i = 0; i < priv->opts.rx_descs1;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto error;
+   }
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -574,6 +587,11 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = >aRD1Ring[0];
+
+   return 0;
+error:
+   device_free_rd1_ring(priv);
+   return ret;
 }
 
 static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +602,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = >aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
 
-   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-   dev_kfree_skb(rd_info->skb);
-
-   kfree(desc->rd_info);
+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote:
> 
> 
> On 2018/3/30 10:44, Ji-Hun Kim wrote:
> >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
> > }
> > dev_dbg(>pcid->dev, "call device init rd0 ring\n");
> >-device_init_rd0_ring(priv);
> >-device_init_rd1_ring(priv);
> >-device_init_td0_ring(priv);
> >-device_init_td1_ring(priv);
> >+ret = device_init_rd0_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_rd1_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_td0_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_td1_ring(priv);
> >+if (ret)
> >+goto error;
> > device_init_registers(priv);
> >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
> > ieee80211_wake_queues(hw);
> > return 0;
> >+error:
> >+return ret;
> >  }
> 
> This code will lead to memory leaks when device_init_rd1_ring()
> fails, because the memory allocated by device_init_rd0_ring() is not
> freed.
> 
> I think this one will be better:
> ret = device_init_rd0_ring(priv);
> if (ret)
> goto error_init_rd0_ring;
> ret = device_init_rd1_ring(priv);
> if (ret)
> goto error_init_rd1_ring;
> ret = device_init_td0_ring(priv);
> if (ret)
> goto error_init_td0_ring;
> ret = device_init_td1_ring(priv);
> if (ret)
> goto error_init_td1_ring;
> ..
> error_init_td1_ring:
> device_free_td0_ring(priv);
> error_init_td0_ring:
> device_free_rd1_ring(priv);
> error_init_rd1_ring:
> device_free_rd0_ring(priv);
> error_init_rd0_ring:
> return ret;
> 
> 
> Best wishes,
> Jia-Ju Bai
> 
> 
Oh, sorry, I got what you said. Yes, you are right. I am going to make patch 
v4. Thanks!

Best regards,
Ji-Hun

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote:
> 
> 
> On 2018/3/30 10:44, Ji-Hun Kim wrote:
> >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
> > }
> > dev_dbg(>pcid->dev, "call device init rd0 ring\n");
> >-device_init_rd0_ring(priv);
> >-device_init_rd1_ring(priv);
> >-device_init_td0_ring(priv);
> >-device_init_td1_ring(priv);
> >+ret = device_init_rd0_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_rd1_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_td0_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_td1_ring(priv);
> >+if (ret)
> >+goto error;
> > device_init_registers(priv);
> >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
> > ieee80211_wake_queues(hw);
> > return 0;
> >+error:
> >+return ret;
> >  }
> 
> This code will lead to memory leaks when device_init_rd1_ring()
> fails, because the memory allocated by device_init_rd0_ring() is not
> freed.
> 
> I think this one will be better:
> ret = device_init_rd0_ring(priv);
> if (ret)
> goto error_init_rd0_ring;
> ret = device_init_rd1_ring(priv);
> if (ret)
> goto error_init_rd1_ring;
> ret = device_init_td0_ring(priv);
> if (ret)
> goto error_init_td0_ring;
> ret = device_init_td1_ring(priv);
> if (ret)
> goto error_init_td1_ring;
> ..
> error_init_td1_ring:
> device_free_td0_ring(priv);
> error_init_td0_ring:
> device_free_rd1_ring(priv);
> error_init_rd1_ring:
> device_free_rd0_ring(priv);
> error_init_rd0_ring:
> return ret;
> 
> 
> Best wishes,
> Jia-Ju Bai
> 
> 
Oh, sorry, I got what you said. Yes, you are right. I am going to make patch 
v4. Thanks!

Best regards,
Ji-Hun

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote:
> 
> 
> On 2018/3/30 10:44, Ji-Hun Kim wrote:
> >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
> > }
> > dev_dbg(>pcid->dev, "call device init rd0 ring\n");
> >-device_init_rd0_ring(priv);
> >-device_init_rd1_ring(priv);
> >-device_init_td0_ring(priv);
> >-device_init_td1_ring(priv);
> >+ret = device_init_rd0_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_rd1_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_td0_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_td1_ring(priv);
> >+if (ret)
> >+goto error;
> > device_init_registers(priv);
> >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
> > ieee80211_wake_queues(hw);
> > return 0;
> >+error:
> >+return ret;
> >  }
> 
> This code will lead to memory leaks when device_init_rd1_ring()
> fails, because the memory allocated by device_init_rd0_ring() is not
> freed.
> 
> I think this one will be better:
> ret = device_init_rd0_ring(priv);
> if (ret)
> goto error_init_rd0_ring;
> ret = device_init_rd1_ring(priv);
> if (ret)
> goto error_init_rd1_ring;
> ret = device_init_td0_ring(priv);
> if (ret)
> goto error_init_td0_ring;
> ret = device_init_td1_ring(priv);
> if (ret)
> goto error_init_td1_ring;
> ..
> error_init_td1_ring:
> device_free_td0_ring(priv);
> error_init_td0_ring:
> device_free_rd1_ring(priv);
> error_init_rd1_ring:
> device_free_rd0_ring(priv);
> error_init_rd0_ring:
> return ret;
> 
> 
> Best wishes,
> Jia-Ju Bai
> 
> 
But, those freeing function are already placed in the each device_init
functions for allocation fail like below. 

@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
+   return 0;
+error:
+   device_free_rd0_ring(priv);
+   return ret;

Would freeing in the vnt_start() be better instead of freeing in
device_init_rd0_ring function?

Best regards,
Ji-Hun

Re: [PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Fri, Mar 30, 2018 at 11:15:03AM +0800, Jia-Ju Bai wrote:
> 
> 
> On 2018/3/30 10:44, Ji-Hun Kim wrote:
> >@@ -1165,10 +1205,18 @@ static int vnt_start(struct ieee80211_hw *hw)
> > }
> > dev_dbg(>pcid->dev, "call device init rd0 ring\n");
> >-device_init_rd0_ring(priv);
> >-device_init_rd1_ring(priv);
> >-device_init_td0_ring(priv);
> >-device_init_td1_ring(priv);
> >+ret = device_init_rd0_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_rd1_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_td0_ring(priv);
> >+if (ret)
> >+goto error;
> >+ret = device_init_td1_ring(priv);
> >+if (ret)
> >+goto error;
> > device_init_registers(priv);
> >@@ -1178,6 +1226,8 @@ static int vnt_start(struct ieee80211_hw *hw)
> > ieee80211_wake_queues(hw);
> > return 0;
> >+error:
> >+return ret;
> >  }
> 
> This code will lead to memory leaks when device_init_rd1_ring()
> fails, because the memory allocated by device_init_rd0_ring() is not
> freed.
> 
> I think this one will be better:
> ret = device_init_rd0_ring(priv);
> if (ret)
> goto error_init_rd0_ring;
> ret = device_init_rd1_ring(priv);
> if (ret)
> goto error_init_rd1_ring;
> ret = device_init_td0_ring(priv);
> if (ret)
> goto error_init_td0_ring;
> ret = device_init_td1_ring(priv);
> if (ret)
> goto error_init_td1_ring;
> ..
> error_init_td1_ring:
> device_free_td0_ring(priv);
> error_init_td0_ring:
> device_free_rd1_ring(priv);
> error_init_rd1_ring:
> device_free_rd0_ring(priv);
> error_init_rd0_ring:
> return ret;
> 
> 
> Best wishes,
> Jia-Ju Bai
> 
> 
But, those freeing function are already placed in the each device_init
functions for allocation fail like below. 

@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
+   return 0;
+error:
+   device_free_rd0_ring(priv);
+   return ret;

Would freeing in the vnt_start() be better instead of freeing in
device_init_rd0_ring function?

Best regards,
Ji-Hun

[PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
Changes v2:

- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
  values.

Changes v3:

- Modify return type of device_init_rd*, device_init_td*. Then add returns
  error code at those functions and vnt_start as well.

 drivers/staging/vt6655/device_main.c | 114 +--
 1 file changed, 82 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..0d55f34 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -124,10 +124,10 @@
 static void device_free_info(struct vnt_private *priv);
 static void device_print_info(struct vnt_private *priv);
 
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
 
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
@@ -528,18 +528,22 @@ static void device_free_rings(struct vnt_private *priv)
  priv->tx0_bufs, priv->tx_bufs_dma0);
 }
 
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd0_pool_dma;
struct vnt_rx_desc *desc;
+   int ret = 0;
 
/* Init the RD0 ring entries */
for (i = 0; i < priv->opts.rx_descs0;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto error;
+   }
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = >aRD0Ring[0];
+
+   return 0;
+error:
+   device_free_rd0_ring(priv);
+   return ret;
 }
 
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd1_pool_dma;
struct vnt_rx_desc *desc;
+   int ret = 0;
 
/* Init the RD1 ring entries */
for (i = 0; i < priv->opts.rx_descs1;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto error;
+   }
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -574,6 +587,11 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = >aRD1Ring[0];
+
+   return 0;
+error:
+   device_free_rd1_ring(priv);
+   return ret;
 }
 
 static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +602,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = >aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
 
-   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-   dev_kfree_skb(rd_info->skb);
-
-   kfree(desc->rd_info);
+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   priv->rx_buf_sz, DMA_FROM_DEVICE);
+

[PATCH v3] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Implement error handling code on device_init_rd*, device_init_td*
and vnt_start for the allocation failures.

Signed-off-by: Ji-Hun Kim 
---
Changes v2:

- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, add if statement for freeing to only allocated
  values.

Changes v3:

- Modify return type of device_init_rd*, device_init_td*. Then add returns
  error code at those functions and vnt_start as well.

 drivers/staging/vt6655/device_main.c | 114 +--
 1 file changed, 82 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..0d55f34 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -124,10 +124,10 @@
 static void device_free_info(struct vnt_private *priv);
 static void device_print_info(struct vnt_private *priv);
 
-static void device_init_rd0_ring(struct vnt_private *priv);
-static void device_init_rd1_ring(struct vnt_private *priv);
-static void device_init_td0_ring(struct vnt_private *priv);
-static void device_init_td1_ring(struct vnt_private *priv);
+static int device_init_rd0_ring(struct vnt_private *priv);
+static int device_init_rd1_ring(struct vnt_private *priv);
+static int device_init_td0_ring(struct vnt_private *priv);
+static int device_init_td1_ring(struct vnt_private *priv);
 
 static int  device_rx_srv(struct vnt_private *priv, unsigned int idx);
 static int  device_tx_srv(struct vnt_private *priv, unsigned int idx);
@@ -528,18 +528,22 @@ static void device_free_rings(struct vnt_private *priv)
  priv->tx0_bufs, priv->tx_bufs_dma0);
 }
 
-static void device_init_rd0_ring(struct vnt_private *priv)
+static int device_init_rd0_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd0_pool_dma;
struct vnt_rx_desc *desc;
+   int ret = 0;
 
/* Init the RD0 ring entries */
for (i = 0; i < priv->opts.rx_descs0;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto error;
+   }
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -550,20 +554,29 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = >aRD0Ring[0];
+
+   return 0;
+error:
+   device_free_rd0_ring(priv);
+   return ret;
 }
 
-static void device_init_rd1_ring(struct vnt_private *priv)
+static int device_init_rd1_ring(struct vnt_private *priv)
 {
int i;
dma_addr_t  curr = priv->rd1_pool_dma;
struct vnt_rx_desc *desc;
+   int ret = 0;
 
/* Init the RD1 ring entries */
for (i = 0; i < priv->opts.rx_descs1;
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info) {
+   ret = -ENOMEM;
+   goto error;
+   }
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -574,6 +587,11 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = >aRD1Ring[0];
+
+   return 0;
+error:
+   device_free_rd1_ring(priv);
+   return ret;
 }
 
 static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +602,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = >aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
 
-   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-   dev_kfree_skb(rd_info->skb);
-
-   kfree(desc->rd_info);
+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   priv->rx_buf_sz, DMA_FROM_DEVICE);
+   dev_kfree_skb(rd_info->skb);
+  

Re: [PATCH v2] staging: vt6655: check for memory allocation failures

[ji-hun Kim]

2018-03-29 17:00 GMT+09:00 Jia-Ju Bai <baijiaju1...@gmail.com>:
>
>
>
> On 2018/3/29 15:22, Ji-Hun Kim wrote:
>>
>> There are no null pointer checking on rd_info and td_info values which
>> are allocated by kzalloc. It has potential null pointer dereferencing
>> issues. Add return when allocation is failed.
>>
>> Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
>> ---
>>
>> Change: since v1:
>>
>> - Delete WARN_ON which can makes crashes on some machines.
>> - Instead of return directly, goto freeing function for freeing previously
>>allocated memory in the for loop after kzalloc() failed.
>> - In the freeing function, if td_info and rd_info are not allocated, no
>>needs to free.
>>
>>   drivers/staging/vt6655/device_main.c | 64 
>> +---
>>   1 file changed, 44 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/staging/vt6655/device_main.c 
>> b/drivers/staging/vt6655/device_main.c
>> index fbc4bc6..ecbba43 100644
>> --- a/drivers/staging/vt6655/device_main.c
>> +++ b/drivers/staging/vt6655/device_main.c
>> @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private 
>> *priv)
>>  i ++, curr += sizeof(struct vnt_rx_desc)) {
>> desc = >aRD0Ring[i];
>> desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
>> -
>> +   if (!desc->rd_info)
>> +   goto error;
>> if (!device_alloc_rx_buf(priv, desc))
>> dev_err(>pcid->dev, "can not alloc rx bufs\n");
>>   @@ -550,6 +551,10 @@ static void device_init_rd0_ring(struct vnt_private 
>> *priv)
>> if (i > 0)
>> priv->aRD0Ring[i-1].next_desc = 
>> cpu_to_le32(priv->rd0_pool_dma);
>> priv->pCurrRD[0] = >aRD0Ring[0];
>> +
>> +   return;
>> +error:
>> +   device_free_rd0_ring(priv);
>>   }
>>
>
>
> I think you should return an error number here, because 
> device_init_rd0_ring() is called by vnt_start().
> You should also implement error handling code in vnt_start(), and let 
> vnt_start() returns an error number too.
> The same for device_init_rd1_ring(), device_init_td0_ring() and 
> device_init_td1_ring().
>

Hi Jia-Ju, Thanks for the feedback. All right, those function looks
that needs returns
an error number. I will implement error handling code and send a patch
v3 tomorrow

Best regards,
Ji-Hun

Re: [PATCH v2] staging: vt6655: check for memory allocation failures

[ji-hun Kim]

2018-03-29 17:00 GMT+09:00 Jia-Ju Bai :
>
>
>
> On 2018/3/29 15:22, Ji-Hun Kim wrote:
>>
>> There are no null pointer checking on rd_info and td_info values which
>> are allocated by kzalloc. It has potential null pointer dereferencing
>> issues. Add return when allocation is failed.
>>
>> Signed-off-by: Ji-Hun Kim 
>> ---
>>
>> Change: since v1:
>>
>> - Delete WARN_ON which can makes crashes on some machines.
>> - Instead of return directly, goto freeing function for freeing previously
>>allocated memory in the for loop after kzalloc() failed.
>> - In the freeing function, if td_info and rd_info are not allocated, no
>>needs to free.
>>
>>   drivers/staging/vt6655/device_main.c | 64 
>> +---
>>   1 file changed, 44 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/staging/vt6655/device_main.c 
>> b/drivers/staging/vt6655/device_main.c
>> index fbc4bc6..ecbba43 100644
>> --- a/drivers/staging/vt6655/device_main.c
>> +++ b/drivers/staging/vt6655/device_main.c
>> @@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private 
>> *priv)
>>  i ++, curr += sizeof(struct vnt_rx_desc)) {
>> desc = >aRD0Ring[i];
>> desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
>> -
>> +   if (!desc->rd_info)
>> +   goto error;
>> if (!device_alloc_rx_buf(priv, desc))
>> dev_err(>pcid->dev, "can not alloc rx bufs\n");
>>   @@ -550,6 +551,10 @@ static void device_init_rd0_ring(struct vnt_private 
>> *priv)
>> if (i > 0)
>> priv->aRD0Ring[i-1].next_desc = 
>> cpu_to_le32(priv->rd0_pool_dma);
>> priv->pCurrRD[0] = >aRD0Ring[0];
>> +
>> +   return;
>> +error:
>> +   device_free_rd0_ring(priv);
>>   }
>>
>
>
> I think you should return an error number here, because 
> device_init_rd0_ring() is called by vnt_start().
> You should also implement error handling code in vnt_start(), and let 
> vnt_start() returns an error number too.
> The same for device_init_rd1_ring(), device_init_td0_ring() and 
> device_init_td1_ring().
>

Hi Jia-Ju, Thanks for the feedback. All right, those function looks
that needs returns
an error number. I will implement error handling code and send a patch
v3 tomorrow

Best regards,
Ji-Hun

[PATCH v2] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Add return when allocation is failed.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---

Change: since v1:

- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, if td_info and rd_info are not allocated, no
  needs to free.

 drivers/staging/vt6655/device_main.c | 64 +---
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..ecbba43 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info)
+   goto error;
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -550,6 +551,10 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = >aRD0Ring[0];
+
+   return;
+error:
+   device_free_rd0_ring(priv);
 }
 
 static void device_init_rd1_ring(struct vnt_private *priv)
@@ -563,7 +568,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info)
+   goto error;
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -574,6 +580,10 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = >aRD1Ring[0];
+
+   return;
+error:
+   device_free_rd1_ring(priv);
 }
 
 static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +594,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = >aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
 
-   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-   dev_kfree_skb(rd_info->skb);
-
-   kfree(desc->rd_info);
+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   priv->rx_buf_sz, DMA_FROM_DEVICE);
+   dev_kfree_skb(rd_info->skb);
+   kfree(desc->rd_info);
+   }
}
 }
 
@@ -601,12 +611,12 @@ static void device_free_rd1_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = >aRD1Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
 
-   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-   dev_kfree_skb(rd_info->skb);
-
-   kfree(desc->rd_info);
+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   priv->rx_buf_sz, DMA_FROM_DEVICE);
+   dev_kfree_skb(rd_info->skb);
+   kfree(desc->rd_info);
+   }
}
 }
 
@@ -621,7 +631,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
 i++, curr += sizeof(struct vnt_tx_desc)) {
desc = >apTD0Rings[i];
desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+   if (!desc->td_info)
+   goto error;
desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
 
@@ -632,6 +643,10 @@ static void device_init_td0_ring(struct vnt_private *priv)
if (i > 0)
priv->apTD0Rings[i-1].next_desc = 
cpu_to_le32(priv->td0_pool_dma);
priv->apTailTD[0] = priv->apCurrTD[0] = >apTD0Rings[0];
+
+   return;
+error:
+   device_free_td0_ring(priv);
 }
 
 static void device_init_td1_ring(struc

[PATCH v2] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Add return when allocation is failed.

Signed-off-by: Ji-Hun Kim 
---

Change: since v1:

- Delete WARN_ON which can makes crashes on some machines.
- Instead of return directly, goto freeing function for freeing previously
  allocated memory in the for loop after kzalloc() failed.
- In the freeing function, if td_info and rd_info are not allocated, no
  needs to free.

 drivers/staging/vt6655/device_main.c | 64 +---
 1 file changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..ecbba43 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info)
+   goto error;
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -550,6 +551,10 @@ static void device_init_rd0_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD0Ring[i-1].next_desc = cpu_to_le32(priv->rd0_pool_dma);
priv->pCurrRD[0] = >aRD0Ring[0];
+
+   return;
+error:
+   device_free_rd0_ring(priv);
 }
 
 static void device_init_rd1_ring(struct vnt_private *priv)
@@ -563,7 +568,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (!desc->rd_info)
+   goto error;
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -574,6 +580,10 @@ static void device_init_rd1_ring(struct vnt_private *priv)
if (i > 0)
priv->aRD1Ring[i-1].next_desc = cpu_to_le32(priv->rd1_pool_dma);
priv->pCurrRD[1] = >aRD1Ring[0];
+
+   return;
+error:
+   device_free_rd1_ring(priv);
 }
 
 static void device_free_rd0_ring(struct vnt_private *priv)
@@ -584,12 +594,12 @@ static void device_free_rd0_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = >aRD0Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
 
-   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-   dev_kfree_skb(rd_info->skb);
-
-   kfree(desc->rd_info);
+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   priv->rx_buf_sz, DMA_FROM_DEVICE);
+   dev_kfree_skb(rd_info->skb);
+   kfree(desc->rd_info);
+   }
}
 }
 
@@ -601,12 +611,12 @@ static void device_free_rd1_ring(struct vnt_private *priv)
struct vnt_rx_desc *desc = >aRD1Ring[i];
struct vnt_rd_info *rd_info = desc->rd_info;
 
-   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
-priv->rx_buf_sz, DMA_FROM_DEVICE);
-
-   dev_kfree_skb(rd_info->skb);
-
-   kfree(desc->rd_info);
+   if (rd_info) {
+   dma_unmap_single(>pcid->dev, rd_info->skb_dma,
+   priv->rx_buf_sz, DMA_FROM_DEVICE);
+   dev_kfree_skb(rd_info->skb);
+   kfree(desc->rd_info);
+   }
}
 }
 
@@ -621,7 +631,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
 i++, curr += sizeof(struct vnt_tx_desc)) {
desc = >apTD0Rings[i];
desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+   if (!desc->td_info)
+   goto error;
desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
 
@@ -632,6 +643,10 @@ static void device_init_td0_ring(struct vnt_private *priv)
if (i > 0)
priv->apTD0Rings[i-1].next_desc = 
cpu_to_le32(priv->td0_pool_dma);
priv->apTailTD[0] = priv->apCurrTD[0] = >apTD0Rings[0];
+
+   return;
+error:
+   device_free_td0_ring(priv);
 }
 
 static void device_init_td1_ring(struct vnt_private *priv)
@@ -646,

Re: [PATCH] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Wed, Mar 28, 2018 at 05:55:57PM +0800, Jia-Ju Bai wrote:
> >@@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private 
> >*priv)
> >  i++, curr += sizeof(struct vnt_tx_desc)) {
> > desc = >apTD1Rings[i];
> > desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
> >-
> >+if (WARN_ON(!desc->td_info))
> >+return;
> > desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
> > desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
> 
> I think the bugs you found are right.
> But your patch is not correct, because it is dangerous to return directly.
> I think you should return an error and then implement error handling
> code for these functions.
> 
Yes, it needs to free previous allocated values in the for loop. Directly
return could make memory leaks. I am going to make patch v2. 

- Delete WARN_ON which could make crashes on some machines.
- Add freeing sequences for previously allocated memory when kzalloc()
  failed instead of returning directly.

Does these changes would be fine on this bug?

Re: [PATCH] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

On Wed, Mar 28, 2018 at 05:55:57PM +0800, Jia-Ju Bai wrote:
> >@@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private 
> >*priv)
> >  i++, curr += sizeof(struct vnt_tx_desc)) {
> > desc = >apTD1Rings[i];
> > desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
> >-
> >+if (WARN_ON(!desc->td_info))
> >+return;
> > desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
> > desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
> 
> I think the bugs you found are right.
> But your patch is not correct, because it is dangerous to return directly.
> I think you should return an error and then implement error handling
> code for these functions.
> 
Yes, it needs to free previous allocated values in the for loop. Directly
return could make memory leaks. I am going to make patch v2. 

- Delete WARN_ON which could make crashes on some machines.
- Add freeing sequences for previously allocated memory when kzalloc()
  failed instead of returning directly.

Does these changes would be fine on this bug?

[PATCH] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Add return when allocation is failed.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
 drivers/staging/vt6655/device_main.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..5d0ba94 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (WARN_ON(!desc->rd_info))
+   return;
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -563,7 +564,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (WARN_ON(!desc->rd_info))
+   return;
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -621,7 +623,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
 i++, curr += sizeof(struct vnt_tx_desc)) {
desc = >apTD0Rings[i];
desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+   if (WARN_ON(!desc->td_info))
+   return;
desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
 
@@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
 i++, curr += sizeof(struct vnt_tx_desc)) {
desc = >apTD1Rings[i];
desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+   if (WARN_ON(!desc->td_info))
+   return;
desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
 
-- 
1.9.1

[PATCH] staging: vt6655: check for memory allocation failures

[Ji-Hun Kim]

There are no null pointer checking on rd_info and td_info values which
are allocated by kzalloc. It has potential null pointer dereferencing
issues. Add return when allocation is failed.

Signed-off-by: Ji-Hun Kim 
---
 drivers/staging/vt6655/device_main.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6655/device_main.c 
b/drivers/staging/vt6655/device_main.c
index fbc4bc6..5d0ba94 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -539,7 +539,8 @@ static void device_init_rd0_ring(struct vnt_private *priv)
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD0Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (WARN_ON(!desc->rd_info))
+   return;
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -563,7 +564,8 @@ static void device_init_rd1_ring(struct vnt_private *priv)
 i ++, curr += sizeof(struct vnt_rx_desc)) {
desc = >aRD1Ring[i];
desc->rd_info = kzalloc(sizeof(*desc->rd_info), GFP_KERNEL);
-
+   if (WARN_ON(!desc->rd_info))
+   return;
if (!device_alloc_rx_buf(priv, desc))
dev_err(>pcid->dev, "can not alloc rx bufs\n");
 
@@ -621,7 +623,8 @@ static void device_init_td0_ring(struct vnt_private *priv)
 i++, curr += sizeof(struct vnt_tx_desc)) {
desc = >apTD0Rings[i];
desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+   if (WARN_ON(!desc->td_info))
+   return;
desc->td_info->buf = priv->tx0_bufs + i * PKT_BUF_SZ;
desc->td_info->buf_dma = priv->tx_bufs_dma0 + i * PKT_BUF_SZ;
 
@@ -646,7 +649,8 @@ static void device_init_td1_ring(struct vnt_private *priv)
 i++, curr += sizeof(struct vnt_tx_desc)) {
desc = >apTD1Rings[i];
desc->td_info = kzalloc(sizeof(*desc->td_info), GFP_KERNEL);
-
+   if (WARN_ON(!desc->td_info))
+   return;
desc->td_info->buf = priv->tx1_bufs + i * PKT_BUF_SZ;
desc->td_info->buf_dma = priv->tx_bufs_dma1 + i * PKT_BUF_SZ;
 
-- 
1.9.1

Re: [PATCH v3 1/2] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

On Wed, Mar 21, 2018 at 01:39:09PM +0900, Ji-Hun Kim wrote:
> There is no failure checking on the param value which will be allocated
> memory by kmalloc. Add a null pointer checking statement. Then goto error:
> and return -ENOMEM error code when kmalloc is failed.
> 
> Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
> ---
> Changes since v1:
>   - Return with -ENOMEM directly, instead of goto error: then return.
>   - [Patch v3 1/2] is same with [patch v2]
> 
>  drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
> b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> index 6a3434c..ffcd86d 100644
> --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> @@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, 
> struct vpfe_ipipe_config *cfg)
>  
>   params = kmalloc(sizeof(struct ipipe_module_params),
>GFP_KERNEL);
> + if (!params)
> + return -ENOMEM;
> +
>   to = (void *)params + module_if->param_offset;
>   size = module_if->param_size;
>  
> @@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, 
> struct vpfe_ipipe_config *cfg)
>  
>   params =  kmalloc(sizeof(struct ipipe_module_params),
>   GFP_KERNEL);
> + if (!params)
> + return -ENOMEM;
> +
>   from = (void *)params + module_if->param_offset;
>   size = module_if->param_size;
>  
> -- 
> 1.9.1
> 
> 

Are there any opinions? I'd like to know how this patch is going.

Best regards,
Ji-Hun

Re: [PATCH v3 1/2] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

On Wed, Mar 21, 2018 at 01:39:09PM +0900, Ji-Hun Kim wrote:
> There is no failure checking on the param value which will be allocated
> memory by kmalloc. Add a null pointer checking statement. Then goto error:
> and return -ENOMEM error code when kmalloc is failed.
> 
> Signed-off-by: Ji-Hun Kim 
> ---
> Changes since v1:
>   - Return with -ENOMEM directly, instead of goto error: then return.
>   - [Patch v3 1/2] is same with [patch v2]
> 
>  drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
> b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> index 6a3434c..ffcd86d 100644
> --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> @@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, 
> struct vpfe_ipipe_config *cfg)
>  
>   params = kmalloc(sizeof(struct ipipe_module_params),
>GFP_KERNEL);
> + if (!params)
> + return -ENOMEM;
> +
>   to = (void *)params + module_if->param_offset;
>   size = module_if->param_size;
>  
> @@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, 
> struct vpfe_ipipe_config *cfg)
>  
>   params =  kmalloc(sizeof(struct ipipe_module_params),
>   GFP_KERNEL);
> + if (!params)
> + return -ENOMEM;
> +
>   from = (void *)params + module_if->param_offset;
>   size = module_if->param_size;
>  
> -- 
> 1.9.1
> 
> 

Are there any opinions? I'd like to know how this patch is going.

Best regards,
Ji-Hun

[PATCH v3 2/2] staging: media: davinci_vpfe: add kfree() on goto err statement

[Ji-Hun Kim]

It needs to free of allocated params value in the goto error statement.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
Changes since v2:
  - add kfree(params) on the error case of the function
  - rename unclear goto statement name
  - declare the params value at start of the function, so it can be free
end of the function

 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index ffcd86d..735d8b5 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1263,6 +1263,7 @@ static int ipipe_get_cgs_params(struct vpfe_ipipe_device 
*ipipe, void *param)
 static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config 
*cfg)
 {
struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
+   struct ipipe_module_params *params;
unsigned int i;
int rval = 0;
 
@@ -1272,7 +1273,6 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
if (cfg->flag & bit) {
const struct ipipe_module_if *module_if =
_modules[i];
-   struct ipipe_module_params *params;
void __user *from = *(void * __user *)
((void *)cfg + module_if->config_offset);
size_t size;
@@ -1289,26 +1289,30 @@ static int ipipe_s_config(struct v4l2_subdev *sd, 
struct vpfe_ipipe_config *cfg)
if (to && from && size) {
if (copy_from_user(to, from, size)) {
rval = -EFAULT;
-   break;
+   goto err_free_params;
}
rval = module_if->set(ipipe, to);
if (rval)
-   goto error;
+   goto err_free_params;
} else if (to && !from && size) {
rval = module_if->set(ipipe, NULL);
if (rval)
-   goto error;
+   goto err_free_params;
}
kfree(params);
}
}
-error:
+   return 0;
+
+err_free_params:
+   kfree(params);
return rval;
 }
 
 static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config 
*cfg)
 {
struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
+   struct ipipe_module_params *params;
unsigned int i;
int rval = 0;
 
@@ -1318,7 +1322,6 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
if (cfg->flag & bit) {
const struct ipipe_module_if *module_if =
_modules[i];
-   struct ipipe_module_params *params;
void __user *to = *(void * __user *)
((void *)cfg + module_if->config_offset);
size_t size;
@@ -1335,16 +1338,19 @@ static int ipipe_g_config(struct v4l2_subdev *sd, 
struct vpfe_ipipe_config *cfg)
if (to && from && size) {
rval = module_if->get(ipipe, from);
if (rval)
-   goto error;
+   goto err_free_params;
if (copy_to_user(to, from, size)) {
rval = -EFAULT;
-   break;
+   goto err_free_params;
}
}
kfree(params);
}
}
-error:
+   return 0;
+
+err_free_params:
+   kfree(params);
return rval;
 }
 
-- 
1.9.1

[PATCH v3 2/2] staging: media: davinci_vpfe: add kfree() on goto err statement

[Ji-Hun Kim]

It needs to free of allocated params value in the goto error statement.

Signed-off-by: Ji-Hun Kim 
---
Changes since v2:
  - add kfree(params) on the error case of the function
  - rename unclear goto statement name
  - declare the params value at start of the function, so it can be free
end of the function

 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index ffcd86d..735d8b5 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1263,6 +1263,7 @@ static int ipipe_get_cgs_params(struct vpfe_ipipe_device 
*ipipe, void *param)
 static int ipipe_s_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config 
*cfg)
 {
struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
+   struct ipipe_module_params *params;
unsigned int i;
int rval = 0;
 
@@ -1272,7 +1273,6 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
if (cfg->flag & bit) {
const struct ipipe_module_if *module_if =
_modules[i];
-   struct ipipe_module_params *params;
void __user *from = *(void * __user *)
((void *)cfg + module_if->config_offset);
size_t size;
@@ -1289,26 +1289,30 @@ static int ipipe_s_config(struct v4l2_subdev *sd, 
struct vpfe_ipipe_config *cfg)
if (to && from && size) {
if (copy_from_user(to, from, size)) {
rval = -EFAULT;
-   break;
+   goto err_free_params;
}
rval = module_if->set(ipipe, to);
if (rval)
-   goto error;
+   goto err_free_params;
} else if (to && !from && size) {
rval = module_if->set(ipipe, NULL);
if (rval)
-   goto error;
+   goto err_free_params;
}
kfree(params);
}
}
-error:
+   return 0;
+
+err_free_params:
+   kfree(params);
return rval;
 }
 
 static int ipipe_g_config(struct v4l2_subdev *sd, struct vpfe_ipipe_config 
*cfg)
 {
struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
+   struct ipipe_module_params *params;
unsigned int i;
int rval = 0;
 
@@ -1318,7 +1322,6 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
if (cfg->flag & bit) {
const struct ipipe_module_if *module_if =
_modules[i];
-   struct ipipe_module_params *params;
void __user *to = *(void * __user *)
((void *)cfg + module_if->config_offset);
size_t size;
@@ -1335,16 +1338,19 @@ static int ipipe_g_config(struct v4l2_subdev *sd, 
struct vpfe_ipipe_config *cfg)
if (to && from && size) {
rval = module_if->get(ipipe, from);
if (rval)
-   goto error;
+   goto err_free_params;
if (copy_to_user(to, from, size)) {
rval = -EFAULT;
-   break;
+   goto err_free_params;
}
}
kfree(params);
}
}
-error:
+   return 0;
+
+err_free_params:
+   kfree(params);
return rval;
 }
 
-- 
1.9.1

[PATCH v3 1/2] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

There is no failure checking on the param value which will be allocated
memory by kmalloc. Add a null pointer checking statement. Then goto error:
and return -ENOMEM error code when kmalloc is failed.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
Changes since v1:
  - Return with -ENOMEM directly, instead of goto error: then return.
  - [Patch v3 1/2] is same with [patch v2]

 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 6a3434c..ffcd86d 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params = kmalloc(sizeof(struct ipipe_module_params),
 GFP_KERNEL);
+   if (!params)
+   return -ENOMEM;
+
to = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
@@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params =  kmalloc(sizeof(struct ipipe_module_params),
GFP_KERNEL);
+   if (!params)
+   return -ENOMEM;
+
from = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
-- 
1.9.1

[PATCH v3 1/2] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

There is no failure checking on the param value which will be allocated
memory by kmalloc. Add a null pointer checking statement. Then goto error:
and return -ENOMEM error code when kmalloc is failed.

Signed-off-by: Ji-Hun Kim 
---
Changes since v1:
  - Return with -ENOMEM directly, instead of goto error: then return.
  - [Patch v3 1/2] is same with [patch v2]

 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 6a3434c..ffcd86d 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params = kmalloc(sizeof(struct ipipe_module_params),
 GFP_KERNEL);
+   if (!params)
+   return -ENOMEM;
+
to = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
@@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params =  kmalloc(sizeof(struct ipipe_module_params),
GFP_KERNEL);
+   if (!params)
+   return -ENOMEM;
+
from = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
-- 
1.9.1

[PATCH v2] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

There is no failure checking on the param value which will be allocated
memory by kmalloc. Add a null pointer checking statement. Then goto error:
and return -ENOMEM error code when kmalloc is failed.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
Changes since v1:
  - Return with -ENOMEM directly, instead of goto error: then return.

 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 6a3434c..ffcd86d 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params = kmalloc(sizeof(struct ipipe_module_params),
 GFP_KERNEL);
+   if (!params)
+   return -ENOMEM;
+
to = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
@@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params =  kmalloc(sizeof(struct ipipe_module_params),
GFP_KERNEL);
+   if (!params)
+   return -ENOMEM;
+
from = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
-- 
1.9.1

[PATCH v2] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

There is no failure checking on the param value which will be allocated
memory by kmalloc. Add a null pointer checking statement. Then goto error:
and return -ENOMEM error code when kmalloc is failed.

Signed-off-by: Ji-Hun Kim 
---
Changes since v1:
  - Return with -ENOMEM directly, instead of goto error: then return.

 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 6a3434c..ffcd86d 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1280,6 +1280,9 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params = kmalloc(sizeof(struct ipipe_module_params),
 GFP_KERNEL);
+   if (!params)
+   return -ENOMEM;
+
to = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
@@ -1323,6 +1326,9 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params =  kmalloc(sizeof(struct ipipe_module_params),
GFP_KERNEL);
+   if (!params)
+   return -ENOMEM;
+
from = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
-- 
1.9.1

Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

On Fri, Mar 16, 2018 at 11:32:34AM +0300, Dan Carpenter wrote:
> On Fri, Mar 16, 2018 at 01:58:23PM +0900, Ji-Hun Kim wrote:
> > There is no failure checking on the param value which will be allocated
> > memory by kmalloc. Add a null pointer checking statement. Then goto error:
> > and return -ENOMEM error code when kmalloc is failed.
> > 
> > Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
> > ---
> >  drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
> > b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> > index 6a3434c..55a922c 100644
> > --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> > +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> > @@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, 
> > struct vpfe_ipipe_config *cfg)
> >  
> > params = kmalloc(sizeof(struct ipipe_module_params),
> >  GFP_KERNEL);
> > +   if (!params) {
> > +   rval = -ENOMEM;
> > +   goto error;
> ^^
> 
> What does "goto error" do, do you think?  It's not clear from the name.
> When you have an unclear goto like this it often means the error
> handling is going to be buggy.
> 
> In this case, it does nothing so a direct "return -ENOMEM;" would be
> more clear.  But the rest of the error handling is buggy.
Hi Dan,
I appreciate for your specific feedbacks. It looks more clear. And I'd
like you to see my question below. I will send the patch v2.

> 
>   1263  static int ipipe_s_config(struct v4l2_subdev *sd, struct 
> vpfe_ipipe_config *cfg)
>   1264  {
>   1265  struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
>   1266  unsigned int i;
>   1267  int rval = 0;
>   1268  
>   1269  for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) {
>   1270  unsigned int bit = 1 << i;
>   1271  
>   1272  if (cfg->flag & bit) {
>   1273  const struct ipipe_module_if *module_if =
>   1274  _modules[i];
>   1275  struct ipipe_module_params *params;
>   1276  void __user *from = *(void * __user *)
>   1277  ((void *)cfg + 
> module_if->config_offset);
>   1278  size_t size;
>   1279  void *to;
>   1280  
>   1281  params = kmalloc(sizeof(struct 
> ipipe_module_params),
>   1282   GFP_KERNEL);
> 
> Do a direct return:
> 
>   if (!params)
>   return -ENOMEM;
> 
>   1283  to = (void *)params + module_if->param_offset;
>   1284  size = module_if->param_size;
>   1285  
>   1286  if (to && from && size) {
>   1287  if (copy_from_user(to, from, size)) {
>   1288  rval = -EFAULT;
>   1289  break;
> 
> The most recent thing we allocated is "params" so lets do a
> "goto free_params;".  We'll have to declare "params" at the start of the
> function instead inside this block.
> 
>   1290  }
>   1291  rval = module_if->set(ipipe, to);
>   1292  if (rval)
>   1293  goto error;
> 
> goto free_params again since params is still the most recent thing we
> allocated.
> 
>   1294  } else if (to && !from && size) {
>   1295  rval = module_if->set(ipipe, NULL);
>   1296  if (rval)
>   1297  goto error;
> 
> And here again goto free_params.
> 
>   1298  }
>   1299  kfree(params);
>   1300  }
>   1301  }
>   1302  error:
>   1303  return rval;
> 
> 
> Change this to:
> 
>   return 0;
Instead of returning rval, returning 0 would be fine? It looks that should
return rval in normal case.

> 
> free_params:
>   kfree(params);
>   return rval;
> 
>   1304  }
> 
> regards,
> dan carpenter
> 
> 
Thanks,
Ji-Hun

Re: Re: [PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

On Fri, Mar 16, 2018 at 11:32:34AM +0300, Dan Carpenter wrote:
> On Fri, Mar 16, 2018 at 01:58:23PM +0900, Ji-Hun Kim wrote:
> > There is no failure checking on the param value which will be allocated
> > memory by kmalloc. Add a null pointer checking statement. Then goto error:
> > and return -ENOMEM error code when kmalloc is failed.
> > 
> > Signed-off-by: Ji-Hun Kim 
> > ---
> >  drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
> > b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> > index 6a3434c..55a922c 100644
> > --- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> > +++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
> > @@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, 
> > struct vpfe_ipipe_config *cfg)
> >  
> > params = kmalloc(sizeof(struct ipipe_module_params),
> >  GFP_KERNEL);
> > +   if (!params) {
> > +   rval = -ENOMEM;
> > +   goto error;
> ^^
> 
> What does "goto error" do, do you think?  It's not clear from the name.
> When you have an unclear goto like this it often means the error
> handling is going to be buggy.
> 
> In this case, it does nothing so a direct "return -ENOMEM;" would be
> more clear.  But the rest of the error handling is buggy.
Hi Dan,
I appreciate for your specific feedbacks. It looks more clear. And I'd
like you to see my question below. I will send the patch v2.

> 
>   1263  static int ipipe_s_config(struct v4l2_subdev *sd, struct 
> vpfe_ipipe_config *cfg)
>   1264  {
>   1265  struct vpfe_ipipe_device *ipipe = v4l2_get_subdevdata(sd);
>   1266  unsigned int i;
>   1267  int rval = 0;
>   1268  
>   1269  for (i = 0; i < ARRAY_SIZE(ipipe_modules); i++) {
>   1270  unsigned int bit = 1 << i;
>   1271  
>   1272  if (cfg->flag & bit) {
>   1273  const struct ipipe_module_if *module_if =
>   1274  _modules[i];
>   1275  struct ipipe_module_params *params;
>   1276  void __user *from = *(void * __user *)
>   1277  ((void *)cfg + 
> module_if->config_offset);
>   1278  size_t size;
>   1279  void *to;
>   1280  
>   1281  params = kmalloc(sizeof(struct 
> ipipe_module_params),
>   1282   GFP_KERNEL);
> 
> Do a direct return:
> 
>   if (!params)
>   return -ENOMEM;
> 
>   1283  to = (void *)params + module_if->param_offset;
>   1284  size = module_if->param_size;
>   1285  
>   1286  if (to && from && size) {
>   1287  if (copy_from_user(to, from, size)) {
>   1288  rval = -EFAULT;
>   1289  break;
> 
> The most recent thing we allocated is "params" so lets do a
> "goto free_params;".  We'll have to declare "params" at the start of the
> function instead inside this block.
> 
>   1290  }
>   1291  rval = module_if->set(ipipe, to);
>   1292  if (rval)
>   1293  goto error;
> 
> goto free_params again since params is still the most recent thing we
> allocated.
> 
>   1294  } else if (to && !from && size) {
>   1295  rval = module_if->set(ipipe, NULL);
>   1296  if (rval)
>   1297  goto error;
> 
> And here again goto free_params.
> 
>   1298  }
>   1299  kfree(params);
>   1300  }
>   1301  }
>   1302  error:
>   1303  return rval;
> 
> 
> Change this to:
> 
>   return 0;
Instead of returning rval, returning 0 would be fine? It looks that should
return rval in normal case.

> 
> free_params:
>   kfree(params);
>   return rval;
> 
>   1304  }
> 
> regards,
> dan carpenter
> 
> 
Thanks,
Ji-Hun

[PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

There is no failure checking on the param value which will be allocated
memory by kmalloc. Add a null pointer checking statement. Then goto error:
and return -ENOMEM error code when kmalloc is failed.

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 6a3434c..55a922c 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params = kmalloc(sizeof(struct ipipe_module_params),
 GFP_KERNEL);
+   if (!params) {
+   rval = -ENOMEM;
+   goto error;
+   }
to = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
@@ -1323,6 +1327,10 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params =  kmalloc(sizeof(struct ipipe_module_params),
GFP_KERNEL);
+   if (!params) {
+   rval = -ENOMEM;
+   goto error;
+   }
from = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
-- 
1.9.1

[PATCH] staging: media: davinci_vpfe: add error handling on kmalloc failure

[Ji-Hun Kim]

There is no failure checking on the param value which will be allocated
memory by kmalloc. Add a null pointer checking statement. Then goto error:
and return -ENOMEM error code when kmalloc is failed.

Signed-off-by: Ji-Hun Kim 
---
 drivers/staging/media/davinci_vpfe/dm365_ipipe.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c 
b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
index 6a3434c..55a922c 100644
--- a/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
+++ b/drivers/staging/media/davinci_vpfe/dm365_ipipe.c
@@ -1280,6 +1280,10 @@ static int ipipe_s_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params = kmalloc(sizeof(struct ipipe_module_params),
 GFP_KERNEL);
+   if (!params) {
+   rval = -ENOMEM;
+   goto error;
+   }
to = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
@@ -1323,6 +1327,10 @@ static int ipipe_g_config(struct v4l2_subdev *sd, struct 
vpfe_ipipe_config *cfg)
 
params =  kmalloc(sizeof(struct ipipe_module_params),
GFP_KERNEL);
+   if (!params) {
+   rval = -ENOMEM;
+   goto error;
+   }
from = (void *)params + module_if->param_offset;
size = module_if->param_size;
 
-- 
1.9.1

[PATCH] staging: rtl8723bs: core: rtw_cmd: remove unnecessary initialization

[Ji-Hun Kim]

Clean up checkpatch error:
ERROR: do not initialise globals to 0

Signed-off-by: Ji-Hun Kim <ji_hun@samsung.com>
---
 drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index af0a9e0..9e132f9 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -1742,7 +1742,7 @@ u8 rtw_ps_cmd(struct adapter *padapter)
return res;
 }
 
-u32 g_wait_hiq_empty = 0;
+u32 g_wait_hiq_empty;
 
 static void rtw_chk_hi_queue_hdl(struct adapter *padapter)
 {
-- 
1.9.1

[PATCH] staging: rtl8723bs: core: rtw_cmd: remove unnecessary initialization

[Ji-Hun Kim]

Clean up checkpatch error:
ERROR: do not initialise globals to 0

Signed-off-by: Ji-Hun Kim 
---
 drivers/staging/rtl8723bs/core/rtw_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_cmd.c 
b/drivers/staging/rtl8723bs/core/rtw_cmd.c
index af0a9e0..9e132f9 100644
--- a/drivers/staging/rtl8723bs/core/rtw_cmd.c
+++ b/drivers/staging/rtl8723bs/core/rtw_cmd.c
@@ -1742,7 +1742,7 @@ u8 rtw_ps_cmd(struct adapter *padapter)
return res;
 }
 
-u32 g_wait_hiq_empty = 0;
+u32 g_wait_hiq_empty;
 
 static void rtw_chk_hi_queue_hdl(struct adapter *padapter)
 {
-- 
1.9.1

[PATCH 2/2] staging: iio: add spaces around '-' operator

[Ji-Hun Kim]

Clean up checkpatch warning:
CHECK: spaces preferred around that '-' (ctx:VxV)

Signed-off-by: Ji-Hun Kim <jihuu...@gmail.com>
---
 drivers/staging/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index f015955..c4eff71 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -340,7 +340,7 @@ ad7192_show_scale_available(struct device *dev,
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
-in_voltage-voltage_scale_available,
+in_voltage - voltage_scale_available,
 0444, ad7192_show_scale_available, NULL, 0);
 
 static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
-- 
2.10.1 (Apple Git-78)

[PATCH 2/2] staging: iio: add spaces around '-' operator

[Ji-Hun Kim]

Clean up checkpatch warning:
CHECK: spaces preferred around that '-' (ctx:VxV)

Signed-off-by: Ji-Hun Kim 
---
 drivers/staging/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index f015955..c4eff71 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -340,7 +340,7 @@ ad7192_show_scale_available(struct device *dev,
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
-in_voltage-voltage_scale_available,
+in_voltage - voltage_scale_available,
 0444, ad7192_show_scale_available, NULL, 0);
 
 static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
-- 
2.10.1 (Apple Git-78)

[PATCH 1/2] staging: iio: remove unnecessary parentheses

[Ji-Hun Kim]

Clean up checkpatch warning:
CHECK: Unnecessary parentheses around 'st->devid != ID_AD7195'

Signed-off-by: Ji-Hun Kim <jihuu...@gmail.com>
---
 drivers/staging/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index cadfb96..f015955 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -271,7 +271,7 @@ static int ad7192_setup(struct ad7192_state *st,
if (pdata->sinc3_en)
st->mode |= AD7192_MODE_SINC3;
 
-   if (pdata->refin2_en && (st->devid != ID_AD7195))
+   if (pdata->refin2_en && st->devid != ID_AD7195)
st->conf |= AD7192_CONF_REFSEL;
 
if (pdata->chop_en) {
-- 
2.10.1 (Apple Git-78)

[PATCH 1/2] staging: iio: remove unnecessary parentheses

[Ji-Hun Kim]

Clean up checkpatch warning:
CHECK: Unnecessary parentheses around 'st->devid != ID_AD7195'

Signed-off-by: Ji-Hun Kim 
---
 drivers/staging/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index cadfb96..f015955 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -271,7 +271,7 @@ static int ad7192_setup(struct ad7192_state *st,
if (pdata->sinc3_en)
st->mode |= AD7192_MODE_SINC3;
 
-   if (pdata->refin2_en && (st->devid != ID_AD7195))
+   if (pdata->refin2_en && st->devid != ID_AD7195)
st->conf |= AD7192_CONF_REFSEL;
 
if (pdata->chop_en) {
-- 
2.10.1 (Apple Git-78)

[PATCH 3/3] staging: irda: separate multiple assignments

[JI-HUN KIM]

Clean up checkpatch warning:
CHECK: multiple assignments should be avoided

Signed-off-by: JI-HUN KIM <jihuu...@gmail.com>
---
 drivers/staging/irda/drivers/esi-sir.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/irda/drivers/esi-sir.c 
b/drivers/staging/irda/drivers/esi-sir.c
index 00866a3..01097f1 100644
--- a/drivers/staging/irda/drivers/esi-sir.c
+++ b/drivers/staging/irda/drivers/esi-sir.c
@@ -104,7 +104,8 @@ static int esi_change_speed(struct sir_dev *dev, unsigned 
int speed)
rts = FALSE;
break;
case 115200:
-   dtr = rts = TRUE;
+   dtr = TRUE;
+   rts = TRUE;
break;
default:
ret = -EINVAL;
-- 
2.10.1 (Apple Git-78)

[PATCH 3/3] staging: irda: separate multiple assignments

[JI-HUN KIM]

Clean up checkpatch warning:
CHECK: multiple assignments should be avoided

Signed-off-by: JI-HUN KIM 
---
 drivers/staging/irda/drivers/esi-sir.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/irda/drivers/esi-sir.c 
b/drivers/staging/irda/drivers/esi-sir.c
index 00866a3..01097f1 100644
--- a/drivers/staging/irda/drivers/esi-sir.c
+++ b/drivers/staging/irda/drivers/esi-sir.c
@@ -104,7 +104,8 @@ static int esi_change_speed(struct sir_dev *dev, unsigned 
int speed)
rts = FALSE;
break;
case 115200:
-   dtr = rts = TRUE;
+   dtr = TRUE;
+   rts = TRUE;
break;
default:
ret = -EINVAL;
-- 
2.10.1 (Apple Git-78)

[PATCH 2/3] staging: irda: add spaces around '|' operator

[JI-HUN KIM]

Clean up checkpatch warning:
CHECK: spaces preferred around that '|' (ctx:VxV)

Signed-off-by: JI-HUN KIM <jihuu...@gmail.com>
---
 drivers/staging/irda/drivers/esi-sir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/irda/drivers/esi-sir.c 
b/drivers/staging/irda/drivers/esi-sir.c
index a12cf55..00866a3 100644
--- a/drivers/staging/irda/drivers/esi-sir.c
+++ b/drivers/staging/irda/drivers/esi-sir.c
@@ -69,7 +69,7 @@ static int esi_open(struct sir_dev *dev)
/* Power up and set dongle to 9600 baud */
sirdev_set_dtr_rts(dev, FALSE, TRUE);
 
-   qos->baud_rate.bits &= IR_9600|IR_19200|IR_115200;
+   qos->baud_rate.bits &= IR_9600 | IR_19200 | IR_115200;
qos->min_turn_time.bits = 0x01; /* Needs at least 10 ms */
irda_qos_bits_to_value(qos);
 
-- 
2.10.1 (Apple Git-78)

[PATCH 2/3] staging: irda: add spaces around '|' operator

[JI-HUN KIM]

Clean up checkpatch warning:
CHECK: spaces preferred around that '|' (ctx:VxV)

Signed-off-by: JI-HUN KIM 
---
 drivers/staging/irda/drivers/esi-sir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/irda/drivers/esi-sir.c 
b/drivers/staging/irda/drivers/esi-sir.c
index a12cf55..00866a3 100644
--- a/drivers/staging/irda/drivers/esi-sir.c
+++ b/drivers/staging/irda/drivers/esi-sir.c
@@ -69,7 +69,7 @@ static int esi_open(struct sir_dev *dev)
/* Power up and set dongle to 9600 baud */
sirdev_set_dtr_rts(dev, FALSE, TRUE);
 
-   qos->baud_rate.bits &= IR_9600|IR_19200|IR_115200;
+   qos->baud_rate.bits &= IR_9600 | IR_19200 | IR_115200;
qos->min_turn_time.bits = 0x01; /* Needs at least 10 ms */
irda_qos_bits_to_value(qos);
 
-- 
2.10.1 (Apple Git-78)

[PATCH 1/3] staging: irda: fix type from "unsigned" to "unsigned int"

[JI-HUN KIM]

Clean up checkpatch warning:
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'

Signed-off-by: JI-HUN KIM <jihuu...@gmail.com>
---
 drivers/staging/irda/drivers/esi-sir.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/irda/drivers/esi-sir.c 
b/drivers/staging/irda/drivers/esi-sir.c
index eb7aa64..a12cf55 100644
--- a/drivers/staging/irda/drivers/esi-sir.c
+++ b/drivers/staging/irda/drivers/esi-sir.c
@@ -39,7 +39,7 @@
 
 static int esi_open(struct sir_dev *);
 static int esi_close(struct sir_dev *);
-static int esi_change_speed(struct sir_dev *, unsigned);
+static int esi_change_speed(struct sir_dev *, unsigned int);
 static int esi_reset(struct sir_dev *);
 
 static struct dongle_driver esi = {
@@ -93,7 +93,7 @@ static int esi_close(struct sir_dev *dev)
  * Apparently (see old esi-driver) no delays are needed here...
  *
  */
-static int esi_change_speed(struct sir_dev *dev, unsigned speed)
+static int esi_change_speed(struct sir_dev *dev, unsigned int speed)
 {
int ret = 0;
int dtr, rts;
-- 
2.10.1 (Apple Git-78)

[PATCH 1/3] staging: irda: fix type from "unsigned" to "unsigned int"

[JI-HUN KIM]

Clean up checkpatch warning:
WARNING: Prefer 'unsigned int' to bare use of 'unsigned'

Signed-off-by: JI-HUN KIM 
---
 drivers/staging/irda/drivers/esi-sir.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/irda/drivers/esi-sir.c 
b/drivers/staging/irda/drivers/esi-sir.c
index eb7aa64..a12cf55 100644
--- a/drivers/staging/irda/drivers/esi-sir.c
+++ b/drivers/staging/irda/drivers/esi-sir.c
@@ -39,7 +39,7 @@
 
 static int esi_open(struct sir_dev *);
 static int esi_close(struct sir_dev *);
-static int esi_change_speed(struct sir_dev *, unsigned);
+static int esi_change_speed(struct sir_dev *, unsigned int);
 static int esi_reset(struct sir_dev *);
 
 static struct dongle_driver esi = {
@@ -93,7 +93,7 @@ static int esi_close(struct sir_dev *dev)
  * Apparently (see old esi-driver) no delays are needed here...
  *
  */
-static int esi_change_speed(struct sir_dev *dev, unsigned speed)
+static int esi_change_speed(struct sir_dev *dev, unsigned int speed)
 {
int ret = 0;
int dtr, rts;
-- 
2.10.1 (Apple Git-78)