[PATCH v2 0/7] Staging: kpc2000: kpc_dma: Clear Misc Checkpatch.pl WARNINGS.

[PATCH v2 0/7] Staging: kpc2000: kpc_dma: Clear Misc Checkpatch.pl WARNINGS.

[Sam Muhammed]

This driver has multiple WARNINGS and CHECKS,
Changes been made to match coding style and standards.
Modifications:
	- Remove Comparison to NULL.
	- Proper usage of kzalloc().
	- Reformat single statement blocks.
	- Including preferred header.
	- Using the proper SPDK comment style.
	- Using kcalloc instead of kzalloc with multiply.
	- Putting spaces around operators.

Changes in v2:
	- Adding a new patch to the series:
	    Patch #6: Use kcalloc over kzalloc.
	- Recovering alignment of code that a patch
	  messed up in the previous revision:
	    Patch #7: Use spaces around operators.

Sam Muhammed (7):
  Staging: kpc2000: kpc_dma: Remove comparison to NULL.
  Staging: kpc2000: kpc_dma: Use sizeof(*var) in kzalloc().
  Staging: kpc2000: kpc_dma: Remove unnecessary braces.
  Staging: kpc2000: kpc_dma: Include the preferred header.
  Staging: kpc2000: kpc_dma: Use the SPDK comment style.
  Staging: kpc2000: kpc_dma: Use kcalloc over kzalloc.
  Staging: kpc2000: kpc_dma: Use spaces around operators.

 drivers/staging/kpc2000/kpc_dma/dma.c         |  9 ++--
 drivers/staging/kpc2000/kpc_dma/fileops.c     | 49 +++++++++----------
 .../staging/kpc2000/kpc_dma/kpc_dma_driver.c  |  9 ++--
 .../staging/kpc2000/kpc_dma/kpc_dma_driver.h  |  4 +-
 4 files changed, 34 insertions(+), 37 deletions(-)

---
2.20.1

[PATCH v2 1/7] Staging: kpc2000: kpc_dma: Remove comparison to NULL.

[Sam Muhammed]

Comparison to NULL been used across the driver,
remove them and use (!var) instead.

Checkpatch.pl: CHECK:
Comparison to NULL could be written "!desc"... etc

Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
---
Changes in v2:
- None.

 drivers/staging/kpc2000/kpc_dma/dma.c            |  2 +-
 drivers/staging/kpc2000/kpc_dma/fileops.c        | 12 ++++++------
 drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
index 51a4dd534a0d..4bfbc717bc07 100644
--- a/drivers/staging/kpc2000/kpc_dma/dma.c
+++ b/drivers/staging/kpc2000/kpc_dma/dma.c
@@ -236,7 +236,7 @@ int  count_descriptors_available(struct kpc_dma_device *eng)
 	struct kpc_dma_descriptor *cur = eng->desc_next;

 	while (cur != eng->desc_completed) {
-		BUG_ON(cur == NULL);
+		BUG_ON(!cur);
 		count++;
 		cur = cur->Next;
 	}
diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 40525540dde6..5eb6c5f24feb 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -124,7 +124,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 		pcnt = count_parts_for_sge(sg);
 		for (p = 0 ; p < pcnt ; p++) {
 			// Fill out the descriptor
-			BUG_ON(desc == NULL);
+			BUG_ON(!desc);
 			clear_desc(desc);
 			if (p != pcnt-1) {
 				desc->DescByteCount = 0x80000;
@@ -203,11 +203,11 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
 {
 	unsigned int i;

-	BUG_ON(acd == NULL);
-	BUG_ON(acd->user_pages == NULL);
-	BUG_ON(acd->sgt.sgl == NULL);
-	BUG_ON(acd->ldev == NULL);
-	BUG_ON(acd->ldev->pldev == NULL);
+	BUG_ON(!acd);
+	BUG_ON(!acd->user_pages);
+	BUG_ON(!acd->sgt.sgl);
+	BUG_ON(!acd->ldev);
+	BUG_ON(!acd->ldev->pldev);

 	for (i = 0 ; i < acd->page_count ; i++) {
 		if (!PageReserved(acd->user_pages[i])) {
diff --git a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
index 4c8cc866b826..8b9c978257b9 100644
--- a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
+++ b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h
@@ -198,14 +198,14 @@ u32  GetEngineCompletePtr(struct kpc_dma_device *eng)
 static inline
 void  lock_engine(struct kpc_dma_device *eng)
 {
-	BUG_ON(eng == NULL);
+	BUG_ON(!eng);
 	mutex_lock(&eng->sem);
 }

 static inline
 void  unlock_engine(struct kpc_dma_device *eng)
 {
-	BUG_ON(eng == NULL);
+	BUG_ON(!eng);
 	mutex_unlock(&eng->sem);
 }

---
2.20.1

[PATCH v2 2/7] Staging: kpc2000: kpc_dma: Use sizeof(*var) in kzalloc().

[Sam Muhammed]

kzalloc(sizeof(*var), ...) was the format been used
across the driver, which is the preferred format,
but missed two instances, correct them to match the
coding standards.

Checkpatch.pl CHECK: Prefer kzalloc(sizeof(*var)...)
over kzalloc(sizeof(struct var)...)

Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
---
Changes in v2:
- Update the log message to be more descriptive.

 drivers/staging/kpc2000/kpc_dma/fileops.c        | 2 +-
 drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 5eb6c5f24feb..21ebab7dbc7b 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -253,7 +253,7 @@ int  kpc_dma_open(struct inode *inode, struct file *filp)
 		return -EBUSY; /* already open */
 	}

-	priv = kzalloc(sizeof(struct dev_private_data), GFP_KERNEL);
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;

diff --git a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
index ec79a8500caf..25ea16a1981e 100644
--- a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
+++ b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
@@ -98,7 +98,7 @@ int  kpc_dma_probe(struct platform_device *pldev)
 	int rv = 0;
 	dev_t dev;

-	struct kpc_dma_device *ldev = kzalloc(sizeof(struct kpc_dma_device), GFP_KERNEL);
+	struct kpc_dma_device *ldev = kzalloc(sizeof(*ldev), GFP_KERNEL);

 	if (!ldev) {
 		dev_err(&pldev->dev, "%s: unable to kzalloc space for kpc_dma_device\n", __func__);
---
2.20.1

[PATCH v2 3/7] Staging: kpc2000: kpc_dma: Remove unnecessary braces.

[Sam Muhammed]

Remove braces of single statement blocks,
they are not really needed.

Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
---
Changes in v2:
- Correct the log message.

 drivers/staging/kpc2000/kpc_dma/dma.c         |  5 ++---
 drivers/staging/kpc2000/kpc_dma/fileops.c     | 20 +++++++++----------
 .../staging/kpc2000/kpc_dma/kpc_dma_driver.c  |  3 +--
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
index 4bfbc717bc07..5ccc93ed2b61 100644
--- a/drivers/staging/kpc2000/kpc_dma/dma.c
+++ b/drivers/staging/kpc2000/kpc_dma/dma.c
@@ -97,11 +97,10 @@ int  setup_dma_engine(struct kpc_dma_device *eng, u32 desc_cnt)
 	if (WARN(!(caps & ENG_CAP_PRESENT), "%s() called for DMA Engine at %p which isn't present in hardware!\n", __func__, eng))
 		return -ENXIO;

-	if (caps & ENG_CAP_DIRECTION) {
+	if (caps & ENG_CAP_DIRECTION)
 		eng->dir = DMA_FROM_DEVICE;
-	} else {
+	else
 		eng->dir = DMA_TO_DEVICE;
-	}

 	eng->desc_pool_cnt = desc_cnt;
 	eng->desc_pool = dma_pool_create("KPC DMA Descriptors", &eng->pldev->dev, sizeof(struct kpc_dma_descriptor), DMA_DESC_ALIGNMENT, 4096);
diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 21ebab7dbc7b..d93097ed781c 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -126,11 +126,11 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 			// Fill out the descriptor
 			BUG_ON(!desc);
 			clear_desc(desc);
-			if (p != pcnt-1) {
+			if (p != pcnt-1)
 				desc->DescByteCount = 0x80000;
-			} else {
+			else
 				desc->DescByteCount = sg_dma_len(sg) - (p * 0x80000);
-			}
+
 			desc->DescBufferByteCount = desc->DescByteCount;

 			desc->DescControlFlags |= DMA_DESC_CTL_IRQONERR;
@@ -148,9 +148,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 			desc->DescSystemAddrMS = (dma_addr & 0xFFFFFFFF00000000UL) >> 32;

 			user_ctl = acd->priv->user_ctl;
-			if (i == acd->mapped_entry_count-1 && p == pcnt-1) {
+			if (i == acd->mapped_entry_count-1 && p == pcnt-1)
 				user_ctl = acd->priv->user_ctl_last;
-			}
+
 			desc->DescUserControlLS = (user_ctl & 0x00000000FFFFFFFFUL) >>  0;
 			desc->DescUserControlMS = (user_ctl & 0xFFFFFFFF00000000UL) >> 32;

@@ -188,9 +188,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 	sg_free_table(&acd->sgt);
  err_dma_map_sg:
  err_alloc_sg_table:
-	for (i = 0 ; i < acd->page_count ; i++) {
+	for (i = 0 ; i < acd->page_count ; i++)
 		put_page(acd->user_pages[i]);
-	}
+
  err_get_user_pages:
 	kfree(acd->user_pages);
  err_alloc_userpages:
@@ -210,16 +210,14 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
 	BUG_ON(!acd->ldev->pldev);

 	for (i = 0 ; i < acd->page_count ; i++) {
-		if (!PageReserved(acd->user_pages[i])) {
+		if (!PageReserved(acd->user_pages[i]))
 			set_page_dirty(acd->user_pages[i]);
-		}
 	}

 	dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);

-	for (i = 0 ; i < acd->page_count ; i++) {
+	for (i = 0 ; i < acd->page_count ; i++)
 		put_page(acd->user_pages[i]);
-	}

 	sg_free_table(&acd->sgt);

diff --git a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
index 25ea16a1981e..bc81afb6ade2 100644
--- a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
+++ b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
@@ -26,9 +26,8 @@ struct kpc_dma_device *kpc_dma_lookup_device(int minor)

 	mutex_lock(&kpc_dma_mtx);
 	list_for_each_entry(c, &kpc_dma_list, list) {
-		if (c->pldev->id == minor) {
+		if (c->pldev->id == minor)
 			goto out;
-		}
 	}
 	c = NULL; // not-found case
 out:
---
2.20.1

[PATCH v2 4/7] Staging: kpc2000: kpc_dma: Include the preferred header.

[Sam Muhammed]

<linux/io.h> is the preferred header to include
instead of <asm/io.h>.

Checkpatch.pl WARNING:
Use #include <linux/io.h> instead of <asm/io.h>

Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
---
Changes in v2:
- None.

 drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
index bc81afb6ade2..0bdd345cc8c8 100644
--- a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
+++ b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
@@ -2,7 +2,7 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
-#include <asm/io.h>
+#include <linux/io.h>
 #include <linux/export.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
---
2.20.1

[PATCH v2 5/7] Staging: kpc2000: kpc_dma: Use the SPDK comment style.

[Sam Muhammed]

SPDK-License-Identifier comment should have this form
// SPDX-License-Identifier: <GPL-...>

Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
---
Changes in v2:
- None.

 drivers/staging/kpc2000/kpc_dma/dma.c            | 2 +-
 drivers/staging/kpc2000/kpc_dma/fileops.c        | 2 +-
 drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
index 5ccc93ed2b61..452a3f7c835d 100644
--- a/drivers/staging/kpc2000/kpc_dma/dma.c
+++ b/drivers/staging/kpc2000/kpc_dma/dma.c
@@ -1,4 +1,4 @@
-/* SPDX-License-Identifier: GPL-2.0+ */
+// SPDX-License-Identifier: GPL-2.0+
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
index d93097ed781c..d80d894f3aac 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -1,4 +1,4 @@
-/* SPDX-License-Identifier: GPL-2.0+ */
+// SPDX-License-Identifier: GPL-2.0+
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/mm.h>
diff --git a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
index 0bdd345cc8c8..c3b30551e0ca 100644
--- a/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
+++ b/drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.c
@@ -1,4 +1,4 @@
-/* SPDX-License-Identifier: GPL-2.0+ */
+// SPDX-License-Identifier: GPL-2.0+
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
---
2.20.1

[PATCH v2 6/7] Staging: kpc2000: kpc_dma: Use kcalloc over kzalloc.

[Sam Muhammed]

Replace kzalloc(sizeof(...) * n, ...) with
kcalloc(n, sizeof(...), ...) since kcalloc is the
preferred API in case of allocating with multiply.

Checkpatch.pl: WARNING:
Prefer kcalloc over kzalloc with multiply.

Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
---
Changes in v2:
- This patch is newly added in this revision.

 drivers/staging/kpc2000/kpc_dma/fileops.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
index d80d894f3aac..8c7859ed1237 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -66,7 +66,8 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 	acd->page_count = count_pages(iov_base, iov_len);

 	// Allocate an array of page pointers
-	acd->user_pages = kzalloc(sizeof(struct page *) * acd->page_count, GFP_KERNEL);
+	acd->user_pages = kcalloc(acd->page_count, sizeof(struct page *),
+				  GFP_KERNEL);
 	if (!acd->user_pages) {
 		dev_err(&priv->ldev->pldev->dev, "Couldn't kmalloc space for for the page pointers\n");
 		rv = -ENOMEM;
---
2.20.1

[PATCH v2 7/7] Staging: kpc2000: kpc_dma: Use spaces around operators.

[Sam Muhammed]

Cleanup Checkpatch.pl CHECKs about missing
spaces around multiple operators.

Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
---
Changes in v2:
- Recover a properly aligned code in count_pages()
  that was messed up in the previous revision.

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

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8c7859ed1237..7caabdd77bbf 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -18,8 +18,8 @@
 static inline
 unsigned int  count_pages(unsigned long iov_base, size_t iov_len)
 {
-	unsigned long first = (iov_base             & PAGE_MASK) >> PAGE_SHIFT;
-	unsigned long last  = ((iov_base+iov_len-1) & PAGE_MASK) >> PAGE_SHIFT;
+	unsigned long first = (iov_base                 & PAGE_MASK) >> PAGE_SHIFT;
+	unsigned long last  = ((iov_base + iov_len - 1) & PAGE_MASK) >> PAGE_SHIFT;

 	return last - first + 1;
 }
@@ -84,7 +84,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 	}

 	// Allocate and setup the sg_table (scatterlist entries)
-	rv = sg_alloc_table_from_pages(&acd->sgt, acd->user_pages, acd->page_count, iov_base & (PAGE_SIZE-1), iov_len, GFP_KERNEL);
+	rv = sg_alloc_table_from_pages(&acd->sgt, acd->user_pages, acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
 	if (rv) {
 		dev_err(&priv->ldev->pldev->dev, "Couldn't alloc sg_table (%ld)\n", rv);
 		goto err_alloc_sg_table;
@@ -127,7 +127,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 			// Fill out the descriptor
 			BUG_ON(!desc);
 			clear_desc(desc);
-			if (p != pcnt-1)
+			if (p != pcnt - 1)
 				desc->DescByteCount = 0x80000;
 			else
 				desc->DescByteCount = sg_dma_len(sg) - (p * 0x80000);
@@ -137,7 +137,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 			desc->DescControlFlags |= DMA_DESC_CTL_IRQONERR;
 			if (i == 0 && p == 0)
 				desc->DescControlFlags |= DMA_DESC_CTL_SOP;
-			if (i == acd->mapped_entry_count-1 && p == pcnt-1)
+			if (i == acd->mapped_entry_count - 1 && p == pcnt - 1)
 				desc->DescControlFlags |= DMA_DESC_CTL_EOP | DMA_DESC_CTL_IRQONDONE;

 			desc->DescCardAddrLS = (card_addr & 0xFFFFFFFF);
@@ -149,13 +149,13 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 			desc->DescSystemAddrMS = (dma_addr & 0xFFFFFFFF00000000UL) >> 32;

 			user_ctl = acd->priv->user_ctl;
-			if (i == acd->mapped_entry_count-1 && p == pcnt-1)
+			if (i == acd->mapped_entry_count - 1 && p == pcnt - 1)
 				user_ctl = acd->priv->user_ctl_last;

 			desc->DescUserControlLS = (user_ctl & 0x00000000FFFFFFFFUL) >>  0;
 			desc->DescUserControlMS = (user_ctl & 0xFFFFFFFF00000000UL) >> 32;

-			if (i == acd->mapped_entry_count-1 && p == pcnt-1)
+			if (i == acd->mapped_entry_count - 1 && p == pcnt - 1)
 				desc->acd = acd;

 			dev_dbg(&priv->ldev->pldev->dev, "  Filled descriptor %p (acd = %p)\n", desc, desc->acd);
---
2.20.1

Re: [Outreachy kernel] [PATCH v2 1/7] Staging: kpc2000: kpc_dma: Remove comparison to NULL.

[Stefano Brivio]

On Wed, 25 Mar 2020 10:26:36 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> Comparison to NULL been used across the driver,
> remove them and use (!var) instead.
> 
> Checkpatch.pl: CHECK:
> Comparison to NULL could be written "!desc"... etc
> 
> Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> ---
> Changes in v2:
> - None.
> 
>  drivers/staging/kpc2000/kpc_dma/dma.c            |  2 +-
>  drivers/staging/kpc2000/kpc_dma/fileops.c        | 12 ++++++------
>  drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h |  4 ++--
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
> index 51a4dd534a0d..4bfbc717bc07 100644
> --- a/drivers/staging/kpc2000/kpc_dma/dma.c
> +++ b/drivers/staging/kpc2000/kpc_dma/dma.c
> @@ -236,7 +236,7 @@ int  count_descriptors_available(struct kpc_dma_device *eng)
>  	struct kpc_dma_descriptor *cur = eng->desc_next;
> 
>  	while (cur != eng->desc_completed) {
> -		BUG_ON(cur == NULL);
> +		BUG_ON(!cur);
>  		count++;
>  		cur = cur->Next;
>  	}
> diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> index 40525540dde6..5eb6c5f24feb 100644
> --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> @@ -124,7 +124,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
>  		pcnt = count_parts_for_sge(sg);
>  		for (p = 0 ; p < pcnt ; p++) {
>  			// Fill out the descriptor
> -			BUG_ON(desc == NULL);
> +			BUG_ON(!desc);
>  			clear_desc(desc);
>  			if (p != pcnt-1) {
>  				desc->DescByteCount = 0x80000;
> @@ -203,11 +203,11 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
>  {
>  	unsigned int i;
> 
> -	BUG_ON(acd == NULL);
> -	BUG_ON(acd->user_pages == NULL);
> -	BUG_ON(acd->sgt.sgl == NULL);
> -	BUG_ON(acd->ldev == NULL);
> -	BUG_ON(acd->ldev->pldev == NULL);
> +	BUG_ON(!acd);
> +	BUG_ON(!acd->user_pages);
> +	BUG_ON(!acd->sgt.sgl);
> +	BUG_ON(!acd->ldev);
> +	BUG_ON(!acd->ldev->pldev);

I've seen some tool reporting this (perhaps Coccinelle?): with
x == NULL, BUG_ON(!x) causes a kernel panic just like dereferencing x.
So the first statement in e.g.:

	BUG_ON(!acd);
	BUG_ON(!acd->user_pages);

is redundant. This change wouldn't be related to this patch, though.

-- 
Stefano

Re: [Outreachy kernel] [PATCH v2 2/7] Staging: kpc2000: kpc_dma: Use sizeof(*var) in kzalloc().

[Stefano Brivio]

On Wed, 25 Mar 2020 10:26:37 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> kzalloc(sizeof(*var), ...) was the format been used
> across the driver, which is the preferred format,
> but missed two instances, correct them to match the
> coding standards.
> 
> Checkpatch.pl CHECK: Prefer kzalloc(sizeof(*var)...)
> over kzalloc(sizeof(struct var)...)
> 
> Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> ---
> Changes in v2:
> - Update the log message to be more descriptive.

It would be nicer to refer here to the person who suggested the change,
if it's not you directly noticing the problem (you can just mention
them by name, name and surname, or explicitly say "suggested by ...",
"requested by ...", etc). There are many recent examples on this list.

-- 
Stefano

Re: [Outreachy kernel] [PATCH v2 0/7] Staging: kpc2000: kpc_dma: Clear Misc Checkpatch.pl WARNINGS.

[Stefano Brivio]

On Wed, 25 Mar 2020 10:26:35 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> This driver has multiple WARNINGS and CHECKS,
> Changes been made to match coding style and standards.
> Modifications:
> 	- Remove Comparison to NULL.
> 	- Proper usage of kzalloc().
> 	- Reformat single statement blocks.
> 	- Including preferred header.
> 	- Using the proper SPDK comment style.
> 	- Using kcalloc instead of kzalloc with multiply.
> 	- Putting spaces around operators.
> 
> Changes in v2:
> 	- Adding a new patch to the series:
> 	    Patch #6: Use kcalloc over kzalloc.
> 	- Recovering alignment of code that a patch
> 	  messed up in the previous revision:
> 	    Patch #7: Use spaces around operators.

I don't think my comment to 2/7 (which also applies to this cover
letter and other patches in the series) should block this.

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

Re: [Outreachy kernel] [PATCH v2 2/7] Staging: kpc2000: kpc_dma: Use sizeof(*var) in kzalloc().

[Sam Muhammed]

On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:
> On Wed, 25 Mar 2020 10:26:37 -0400
> Sam Muhammed <jane.pnx9@gmail.com> wrote:
> 
> > kzalloc(sizeof(*var), ...) was the format been used
> > across the driver, which is the preferred format,
> > but missed two instances, correct them to match the
> > coding standards.
> > 
> > Checkpatch.pl CHECK: Prefer kzalloc(sizeof(*var)...)
> > over kzalloc(sizeof(struct var)...)
> > 
> > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > ---
> > Changes in v2:
> > - Update the log message to be more descriptive.
> 
> It would be nicer to refer here to the person who suggested the change,
> if it's not you directly noticing the problem (you can just mention
> them by name, name and surname, or explicitly say "suggested by ...",
> "requested by ...", etc). There are many recent examples on this list.
> 
Ohh, Sorry for that, i thought this tag is more of a code related
change. since being new to the process, most of the time log messages
and subject lines will be commented on to follow the conventions. which
i thought is a different case than changes in the patch itself.

Sorry for the inconvenience,
Thank You

Sam

Re: [Outreachy kernel] [PATCH v2 1/7] Staging: kpc2000: kpc_dma: Remove comparison to NULL.

[Sam Muhammed]

On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:
> On Wed, 25 Mar 2020 10:26:36 -0400
> Sam Muhammed <jane.pnx9@gmail.com> wrote:
> 
> > Comparison to NULL been used across the driver,
> > remove them and use (!var) instead.
> > 
> > Checkpatch.pl: CHECK:
> > Comparison to NULL could be written "!desc"... etc
> > 
> > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > ---
> > Changes in v2:
> > - None.
> > 
> >  drivers/staging/kpc2000/kpc_dma/dma.c            |  2 +-
> >  drivers/staging/kpc2000/kpc_dma/fileops.c        | 12 ++++++------
> >  drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h |  4 ++--
> >  3 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
> > index 51a4dd534a0d..4bfbc717bc07 100644
> > --- a/drivers/staging/kpc2000/kpc_dma/dma.c
> > +++ b/drivers/staging/kpc2000/kpc_dma/dma.c
> > @@ -236,7 +236,7 @@ int  count_descriptors_available(struct kpc_dma_device *eng)
> >  	struct kpc_dma_descriptor *cur = eng->desc_next;
> > 
> >  	while (cur != eng->desc_completed) {
> > -		BUG_ON(cur == NULL);
> > +		BUG_ON(!cur);
> >  		count++;
> >  		cur = cur->Next;
> >  	}
> > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > index 40525540dde6..5eb6c5f24feb 100644
> > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > @@ -124,7 +124,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> >  		pcnt = count_parts_for_sge(sg);
> >  		for (p = 0 ; p < pcnt ; p++) {
> >  			// Fill out the descriptor
> > -			BUG_ON(desc == NULL);
> > +			BUG_ON(!desc);
> >  			clear_desc(desc);
> >  			if (p != pcnt-1) {
> >  				desc->DescByteCount = 0x80000;
> > @@ -203,11 +203,11 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
> >  {
> >  	unsigned int i;
> > 
> > -	BUG_ON(acd == NULL);
> > -	BUG_ON(acd->user_pages == NULL);
> > -	BUG_ON(acd->sgt.sgl == NULL);
> > -	BUG_ON(acd->ldev == NULL);
> > -	BUG_ON(acd->ldev->pldev == NULL);
> > +	BUG_ON(!acd);
> > +	BUG_ON(!acd->user_pages);
> > +	BUG_ON(!acd->sgt.sgl);
> > +	BUG_ON(!acd->ldev);
> > +	BUG_ON(!acd->ldev->pldev);
> 
> I've seen some tool reporting this (perhaps Coccinelle?): with
> x == NULL, BUG_ON(!x) causes a kernel panic just like dereferencing x.
> So the first statement in e.g.:
> 
> 	BUG_ON(!acd);
> 	BUG_ON(!acd->user_pages);
> 
> is redundant. This change wouldn't be related to this patch, though.
> 
Please correct me if i got this wrong:

Does it mean that explicitly stating BUG_ON(!x) case to cause a kernel
panic is not necessary since its already being dereferenced, and
dereferencing it is stated to cause a kernel panic already?

Thank You,

Sam

Re: [Outreachy kernel] [PATCH v2 1/7] Staging: kpc2000: kpc_dma: Remove comparison to NULL.

[Julia Lawall]

On Mon, 30 Mar 2020, Sam Muhammed wrote:

> On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:
> > On Wed, 25 Mar 2020 10:26:36 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> >
> > > Comparison to NULL been used across the driver,
> > > remove them and use (!var) instead.
> > >
> > > Checkpatch.pl: CHECK:
> > > Comparison to NULL could be written "!desc"... etc
> > >
> > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > > ---
> > > Changes in v2:
> > > - None.
> > >
> > >  drivers/staging/kpc2000/kpc_dma/dma.c            |  2 +-
> > >  drivers/staging/kpc2000/kpc_dma/fileops.c        | 12 ++++++------
> > >  drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h |  4 ++--
> > >  3 files changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
> > > index 51a4dd534a0d..4bfbc717bc07 100644
> > > --- a/drivers/staging/kpc2000/kpc_dma/dma.c
> > > +++ b/drivers/staging/kpc2000/kpc_dma/dma.c
> > > @@ -236,7 +236,7 @@ int  count_descriptors_available(struct kpc_dma_device *eng)
> > >  	struct kpc_dma_descriptor *cur = eng->desc_next;
> > >
> > >  	while (cur != eng->desc_completed) {
> > > -		BUG_ON(cur == NULL);
> > > +		BUG_ON(!cur);
> > >  		count++;
> > >  		cur = cur->Next;
> > >  	}
> > > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > index 40525540dde6..5eb6c5f24feb 100644
> > > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > @@ -124,7 +124,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > >  		pcnt = count_parts_for_sge(sg);
> > >  		for (p = 0 ; p < pcnt ; p++) {
> > >  			// Fill out the descriptor
> > > -			BUG_ON(desc == NULL);
> > > +			BUG_ON(!desc);
> > >  			clear_desc(desc);
> > >  			if (p != pcnt-1) {
> > >  				desc->DescByteCount = 0x80000;
> > > @@ -203,11 +203,11 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
> > >  {
> > >  	unsigned int i;
> > >
> > > -	BUG_ON(acd == NULL);
> > > -	BUG_ON(acd->user_pages == NULL);
> > > -	BUG_ON(acd->sgt.sgl == NULL);
> > > -	BUG_ON(acd->ldev == NULL);
> > > -	BUG_ON(acd->ldev->pldev == NULL);
> > > +	BUG_ON(!acd);
> > > +	BUG_ON(!acd->user_pages);
> > > +	BUG_ON(!acd->sgt.sgl);
> > > +	BUG_ON(!acd->ldev);
> > > +	BUG_ON(!acd->ldev->pldev);
> >
> > I've seen some tool reporting this (perhaps Coccinelle?): with
> > x == NULL, BUG_ON(!x) causes a kernel panic just like dereferencing x.
> > So the first statement in e.g.:
> >
> > 	BUG_ON(!acd);
> > 	BUG_ON(!acd->user_pages);
> >
> > is redundant. This change wouldn't be related to this patch, though.
> >
> Please correct me if i got this wrong:
>
> Does it mean that explicitly stating BUG_ON(!x) case to cause a kernel
> panic is not necessary since its already being dereferenced, and
> dereferencing it is stated to cause a kernel panic already?

Suppose acd is NULL.  Then acd->user_pages will crash with a NULL pointer
derefence.  That is the same as what you are asking the BUG_ON to do in
the first line.  So you can get the effect of both lines with only the
first line.  If BUG_ON gave some more helpful information, then you could
also have the first line.  But it doesn't.

julia

Re: [Outreachy kernel] [PATCH v2 1/7] Staging: kpc2000: kpc_dma: Remove comparison to NULL.

[Sam Muhammed]

On Mon, 2020-03-30 at 12:58 +0200, Julia Lawall wrote:
> 
> On Mon, 30 Mar 2020, Sam Muhammed wrote:
> 
> > On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:
> > > On Wed, 25 Mar 2020 10:26:36 -0400
> > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> > >
> > > > Comparison to NULL been used across the driver,
> > > > remove them and use (!var) instead.
> > > >
> > > > Checkpatch.pl: CHECK:
> > > > Comparison to NULL could be written "!desc"... etc
> > > >
> > > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > - None.
> > > >
> > > >  drivers/staging/kpc2000/kpc_dma/dma.c            |  2 +-
> > > >  drivers/staging/kpc2000/kpc_dma/fileops.c        | 12 ++++++------
> > > >  drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h |  4 ++--
> > > >  3 files changed, 9 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
> > > > index 51a4dd534a0d..4bfbc717bc07 100644
> > > > --- a/drivers/staging/kpc2000/kpc_dma/dma.c
> > > > +++ b/drivers/staging/kpc2000/kpc_dma/dma.c
> > > > @@ -236,7 +236,7 @@ int  count_descriptors_available(struct kpc_dma_device *eng)
> > > >  	struct kpc_dma_descriptor *cur = eng->desc_next;
> > > >
> > > >  	while (cur != eng->desc_completed) {
> > > > -		BUG_ON(cur == NULL);
> > > > +		BUG_ON(!cur);
> > > >  		count++;
> > > >  		cur = cur->Next;
> > > >  	}
> > > > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > > index 40525540dde6..5eb6c5f24feb 100644
> > > > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > > @@ -124,7 +124,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > > >  		pcnt = count_parts_for_sge(sg);
> > > >  		for (p = 0 ; p < pcnt ; p++) {
> > > >  			// Fill out the descriptor
> > > > -			BUG_ON(desc == NULL);
> > > > +			BUG_ON(!desc);
> > > >  			clear_desc(desc);
> > > >  			if (p != pcnt-1) {
> > > >  				desc->DescByteCount = 0x80000;
> > > > @@ -203,11 +203,11 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
> > > >  {
> > > >  	unsigned int i;
> > > >
> > > > -	BUG_ON(acd == NULL);
> > > > -	BUG_ON(acd->user_pages == NULL);
> > > > -	BUG_ON(acd->sgt.sgl == NULL);
> > > > -	BUG_ON(acd->ldev == NULL);
> > > > -	BUG_ON(acd->ldev->pldev == NULL);
> > > > +	BUG_ON(!acd);
> > > > +	BUG_ON(!acd->user_pages);
> > > > +	BUG_ON(!acd->sgt.sgl);
> > > > +	BUG_ON(!acd->ldev);
> > > > +	BUG_ON(!acd->ldev->pldev);
> > >
> > > I've seen some tool reporting this (perhaps Coccinelle?): with
> > > x == NULL, BUG_ON(!x) causes a kernel panic just like dereferencing x.
> > > So the first statement in e.g.:
> > >
> > > 	BUG_ON(!acd);
> > > 	BUG_ON(!acd->user_pages);
> > >
> > > is redundant. This change wouldn't be related to this patch, though.
> > >
> > Please correct me if i got this wrong:
> >
> > Does it mean that explicitly stating BUG_ON(!x) case to cause a kernel
> > panic is not necessary since its already being dereferenced, and
> > dereferencing it is stated to cause a kernel panic already?
> 
> Suppose acd is NULL.  Then acd->user_pages will crash with a NULL pointer
> derefence.  That is the same as what you are asking the BUG_ON to do in
> the first line.  So you can get the effect of both lines with only the
> first line.  If BUG_ON gave some more helpful information, then you could
> also have the first line.  But it doesn't.
> 

Great, Thank You

Sam

> julia

Re: [Outreachy kernel] [PATCH v2 2/7] Staging: kpc2000: kpc_dma: Use sizeof(*var) in kzalloc().

[Stefano Brivio]

On Mon, 30 Mar 2020 06:45:00 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:
> > On Wed, 25 Mar 2020 10:26:37 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> >   
> > > kzalloc(sizeof(*var), ...) was the format been used
> > > across the driver, which is the preferred format,
> > > but missed two instances, correct them to match the
> > > coding standards.
> > > 
> > > Checkpatch.pl CHECK: Prefer kzalloc(sizeof(*var)...)
> > > over kzalloc(sizeof(struct var)...)
> > > 
> > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > > ---
> > > Changes in v2:
> > > - Update the log message to be more descriptive.  
> > 
> > It would be nicer to refer here to the person who suggested the change,
> > if it's not you directly noticing the problem (you can just mention
> > them by name, name and surname, or explicitly say "suggested by ...",
> > "requested by ...", etc). There are many recent examples on this list.
> >   
> Ohh, Sorry for that, i thought this tag is more of a code related
> change. since being new to the process, most of the time log messages
> and subject lines will be commented on to follow the conventions. which
> i thought is a different case than changes in the patch itself.

Wait, I wasn't talking about tags here:

- use a Suggested-by: tag if the patch itself was suggested by somebody
  (which is not the case for this patch)

- v2 was prompted by comments you received for v1 by Julia, so it would
  be appropriate to state, in the history of log changes:
	- Update the log message to be more descriptive (suggested by
	  Julia Lawall)

-- 
Stefano

Re: [Outreachy kernel] [PATCH v2 2/7] Staging: kpc2000: kpc_dma: Use sizeof(*var) in kzalloc().

[Sam Muhammed]

On Mon, 2020-03-30 at 14:56 +0200, Stefano Brivio wrote:
> On Mon, 30 Mar 2020 06:45:00 -0400
> Sam Muhammed <jane.pnx9@gmail.com> wrote:
> 
> > On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:
> > > On Wed, 25 Mar 2020 10:26:37 -0400
> > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> > >   
> > > > kzalloc(sizeof(*var), ...) was the format been used
> > > > across the driver, which is the preferred format,
> > > > but missed two instances, correct them to match the
> > > > coding standards.
> > > > 
> > > > Checkpatch.pl CHECK: Prefer kzalloc(sizeof(*var)...)
> > > > over kzalloc(sizeof(struct var)...)
> > > > 
> > > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > - Update the log message to be more descriptive.  
> > > 
> > > It would be nicer to refer here to the person who suggested the change,
> > > if it's not you directly noticing the problem (you can just mention
> > > them by name, name and surname, or explicitly say "suggested by ...",
> > > "requested by ...", etc). There are many recent examples on this list.
> > >   
> > Ohh, Sorry for that, i thought this tag is more of a code related
> > change. since being new to the process, most of the time log messages
> > and subject lines will be commented on to follow the conventions. which
> > i thought is a different case than changes in the patch itself.
> 
> Wait, I wasn't talking about tags here:
> 
> - use a Suggested-by: tag if the patch itself was suggested by somebody
>   (which is not the case for this patch)
> 
> - v2 was prompted by comments you received for v1 by Julia, so it would
>   be appropriate to state, in the history of log changes:
> 	- Update the log message to be more descriptive (suggested by
> 	  Julia Lawall)
> 

My bad, yes.

Now is there a way to add this and your Reviewed-by tag over the series
since the changes have already been made?

Thank You.

Sam

Re: [Outreachy kernel] [PATCH v2 2/7] Staging: kpc2000: kpc_dma: Use sizeof(*var) in kzalloc().

[Stefano Brivio]

On Mon, 30 Mar 2020 09:10:27 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> On Mon, 2020-03-30 at 14:56 +0200, Stefano Brivio wrote:
> > On Mon, 30 Mar 2020 06:45:00 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> >   
> > > On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:  
> > > > On Wed, 25 Mar 2020 10:26:37 -0400
> > > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> > > >     
> > > > > kzalloc(sizeof(*var), ...) was the format been used
> > > > > across the driver, which is the preferred format,
> > > > > but missed two instances, correct them to match the
> > > > > coding standards.
> > > > > 
> > > > > Checkpatch.pl CHECK: Prefer kzalloc(sizeof(*var)...)
> > > > > over kzalloc(sizeof(struct var)...)
> > > > > 
> > > > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > > > > ---
> > > > > Changes in v2:
> > > > > - Update the log message to be more descriptive.    
> > > > 
> > > > It would be nicer to refer here to the person who suggested the change,
> > > > if it's not you directly noticing the problem (you can just mention
> > > > them by name, name and surname, or explicitly say "suggested by ...",
> > > > "requested by ...", etc). There are many recent examples on this list.
> > > >     
> > > Ohh, Sorry for that, i thought this tag is more of a code related
> > > change. since being new to the process, most of the time log messages
> > > and subject lines will be commented on to follow the conventions. which
> > > i thought is a different case than changes in the patch itself.  
> > 
> > Wait, I wasn't talking about tags here:
> > 
> > - use a Suggested-by: tag if the patch itself was suggested by somebody
> >   (which is not the case for this patch)
> > 
> > - v2 was prompted by comments you received for v1 by Julia, so it would
> >   be appropriate to state, in the history of log changes:
> > 	- Update the log message to be more descriptive (suggested by
> > 	  Julia Lawall)
> >   
> 
> My bad, yes.
> 
> Now is there a way to add this and your Reviewed-by tag over the series
> since the changes have already been made?

Ah, no. "This" doesn't end up in git anyway, so there's no need and no
way to add it. My Reviewed-by: tag was just functional to get this
merged, but I didn't notice it was merged already. Never mind then.

-- 
Stefano

Re: [Outreachy kernel] [PATCH v2 1/7] Staging: kpc2000: kpc_dma: Remove comparison to NULL.

[Stefano Brivio]

On Mon, 30 Mar 2020 06:48:21 -0400
Sam Muhammed <jane.pnx9@gmail.com> wrote:

> On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:
> > On Wed, 25 Mar 2020 10:26:36 -0400
> > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> >   
> > > Comparison to NULL been used across the driver,
> > > remove them and use (!var) instead.
> > > 
> > > Checkpatch.pl: CHECK:
> > > Comparison to NULL could be written "!desc"... etc
> > > 
> > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > > ---
> > > Changes in v2:
> > > - None.
> > > 
> > >  drivers/staging/kpc2000/kpc_dma/dma.c            |  2 +-
> > >  drivers/staging/kpc2000/kpc_dma/fileops.c        | 12 ++++++------
> > >  drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h |  4 ++--
> > >  3 files changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
> > > index 51a4dd534a0d..4bfbc717bc07 100644
> > > --- a/drivers/staging/kpc2000/kpc_dma/dma.c
> > > +++ b/drivers/staging/kpc2000/kpc_dma/dma.c
> > > @@ -236,7 +236,7 @@ int  count_descriptors_available(struct kpc_dma_device *eng)
> > >  	struct kpc_dma_descriptor *cur = eng->desc_next;
> > > 
> > >  	while (cur != eng->desc_completed) {
> > > -		BUG_ON(cur == NULL);
> > > +		BUG_ON(!cur);
> > >  		count++;
> > >  		cur = cur->Next;
> > >  	}
> > > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > index 40525540dde6..5eb6c5f24feb 100644
> > > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > @@ -124,7 +124,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > >  		pcnt = count_parts_for_sge(sg);
> > >  		for (p = 0 ; p < pcnt ; p++) {
> > >  			// Fill out the descriptor
> > > -			BUG_ON(desc == NULL);
> > > +			BUG_ON(!desc);
> > >  			clear_desc(desc);
> > >  			if (p != pcnt-1) {
> > >  				desc->DescByteCount = 0x80000;
> > > @@ -203,11 +203,11 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
> > >  {
> > >  	unsigned int i;
> > > 
> > > -	BUG_ON(acd == NULL);
> > > -	BUG_ON(acd->user_pages == NULL);
> > > -	BUG_ON(acd->sgt.sgl == NULL);
> > > -	BUG_ON(acd->ldev == NULL);
> > > -	BUG_ON(acd->ldev->pldev == NULL);
> > > +	BUG_ON(!acd);
> > > +	BUG_ON(!acd->user_pages);
> > > +	BUG_ON(!acd->sgt.sgl);
> > > +	BUG_ON(!acd->ldev);
> > > +	BUG_ON(!acd->ldev->pldev);  
> > 
> > I've seen some tool reporting this (perhaps Coccinelle?): with
> > x == NULL, BUG_ON(!x) causes a kernel panic just like dereferencing x.
> > So the first statement in e.g.:
> > 
> > 	BUG_ON(!acd);
> > 	BUG_ON(!acd->user_pages);
> > 
> > is redundant. This change wouldn't be related to this patch, though.
> >   
> Please correct me if i got this wrong:
> 
> Does it mean that explicitly stating BUG_ON(!x) case to cause a kernel
> panic is not necessary since its already being dereferenced, and
> dereferencing it is stated to cause a kernel panic already?

Yes... it took me a bit longer to understand this compared to Julia's
explanation :) but I think it's correct.

-- 
Stefano

Re: [Outreachy kernel] [PATCH v2 1/7] Staging: kpc2000: kpc_dma: Remove comparison to NULL.

[Sam Muhammed]

On Mon, 2020-03-30 at 15:55 +0200, Stefano Brivio wrote:
> On Mon, 30 Mar 2020 06:48:21 -0400
> Sam Muhammed <jane.pnx9@gmail.com> wrote:
> 
> > On Mon, 2020-03-30 at 03:04 +0200, Stefano Brivio wrote:
> > > On Wed, 25 Mar 2020 10:26:36 -0400
> > > Sam Muhammed <jane.pnx9@gmail.com> wrote:
> > >   
> > > > Comparison to NULL been used across the driver,
> > > > remove them and use (!var) instead.
> > > > 
> > > > Checkpatch.pl: CHECK:
> > > > Comparison to NULL could be written "!desc"... etc
> > > > 
> > > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > - None.
> > > > 
> > > >  drivers/staging/kpc2000/kpc_dma/dma.c            |  2 +-
> > > >  drivers/staging/kpc2000/kpc_dma/fileops.c        | 12 ++++++------
> > > >  drivers/staging/kpc2000/kpc_dma/kpc_dma_driver.h |  4 ++--
> > > >  3 files changed, 9 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/kpc2000/kpc_dma/dma.c b/drivers/staging/kpc2000/kpc_dma/dma.c
> > > > index 51a4dd534a0d..4bfbc717bc07 100644
> > > > --- a/drivers/staging/kpc2000/kpc_dma/dma.c
> > > > +++ b/drivers/staging/kpc2000/kpc_dma/dma.c
> > > > @@ -236,7 +236,7 @@ int  count_descriptors_available(struct kpc_dma_device *eng)
> > > >  	struct kpc_dma_descriptor *cur = eng->desc_next;
> > > > 
> > > >  	while (cur != eng->desc_completed) {
> > > > -		BUG_ON(cur == NULL);
> > > > +		BUG_ON(!cur);
> > > >  		count++;
> > > >  		cur = cur->Next;
> > > >  	}
> > > > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > > index 40525540dde6..5eb6c5f24feb 100644
> > > > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > > > @@ -124,7 +124,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > > >  		pcnt = count_parts_for_sge(sg);
> > > >  		for (p = 0 ; p < pcnt ; p++) {
> > > >  			// Fill out the descriptor
> > > > -			BUG_ON(desc == NULL);
> > > > +			BUG_ON(!desc);
> > > >  			clear_desc(desc);
> > > >  			if (p != pcnt-1) {
> > > >  				desc->DescByteCount = 0x80000;
> > > > @@ -203,11 +203,11 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
> > > >  {
> > > >  	unsigned int i;
> > > > 
> > > > -	BUG_ON(acd == NULL);
> > > > -	BUG_ON(acd->user_pages == NULL);
> > > > -	BUG_ON(acd->sgt.sgl == NULL);
> > > > -	BUG_ON(acd->ldev == NULL);
> > > > -	BUG_ON(acd->ldev->pldev == NULL);
> > > > +	BUG_ON(!acd);
> > > > +	BUG_ON(!acd->user_pages);
> > > > +	BUG_ON(!acd->sgt.sgl);
> > > > +	BUG_ON(!acd->ldev);
> > > > +	BUG_ON(!acd->ldev->pldev);  
> > > 
> > > I've seen some tool reporting this (perhaps Coccinelle?): with
> > > x == NULL, BUG_ON(!x) causes a kernel panic just like dereferencing x.
> > > So the first statement in e.g.:
> > > 
> > > 	BUG_ON(!acd);
> > > 	BUG_ON(!acd->user_pages);
> > > 
> > > is redundant. This change wouldn't be related to this patch, though.
> > >   
> > Please correct me if i got this wrong:
> > 
> > Does it mean that explicitly stating BUG_ON(!x) case to cause a kernel
> > panic is not necessary since its already being dereferenced, and
> > dereferencing it is stated to cause a kernel panic already?
> 
> Yes... it took me a bit longer to understand this compared to Julia's

I totally agree :D

> explanation :) but I think it's correct.
> 

Thank You.

Sam