From: Markus Elfring <[email protected]>
Date: Mon, 8 Jan 2018 10:32:23 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (3):
Delete an error message for a failed memory allocation in genwqe_user_vmap()
Fix a typo in two comments
Adjust 12 checks for null pointers
drivers/misc/genwqe/card_utils.c | 34 +++++++++++++++-------------------
1 file changed, 15 insertions(+), 19 deletions(-)
--
2.15.1
From: Markus Elfring <[email protected]>
Date: Mon, 8 Jan 2018 09:37:23 +0100
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/misc/genwqe/card_utils.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/misc/genwqe/card_utils.c b/drivers/misc/genwqe/card_utils.c
index 8f2e6442d88b..55c389a9e7d7 100644
--- a/drivers/misc/genwqe/card_utils.c
+++ b/drivers/misc/genwqe/card_utils.c
@@ -593,7 +593,6 @@ int genwqe_user_vmap(struct genwqe_dev *cd, struct dma_mapping *m, void *uaddr,
sizeof(struct page *) + sizeof(dma_addr_t),
GFP_KERNEL);
if (!m->page_list) {
- dev_err(&pci_dev->dev, "err: alloc page_list failed\n");
m->nr_pages = 0;
m->u_vaddr = NULL;
m->size = 0; /* mark unused and not added */
--
2.15.1
From: Markus Elfring <[email protected]>
Date: Mon, 8 Jan 2018 09:57:10 +0100
Add a missing character in two words of these descriptions.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/misc/genwqe/card_utils.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/genwqe/card_utils.c b/drivers/misc/genwqe/card_utils.c
index 55c389a9e7d7..0b466664c908 100644
--- a/drivers/misc/genwqe/card_utils.c
+++ b/drivers/misc/genwqe/card_utils.c
@@ -453,7 +453,7 @@ int genwqe_setup_sgl(struct genwqe_dev *cd, struct genwqe_sgl *sgl,
s += 8; /* continue 8 elements further */
}
fixup:
- if (j == 1) { /* combining happend on last entry! */
+ if (j == 1) { /* combining happened on last entry! */
s -= 8; /* full shift needed on previous sgl block */
j = 7; /* shift all elements */
}
@@ -471,7 +471,7 @@ int genwqe_setup_sgl(struct genwqe_dev *cd, struct genwqe_sgl *sgl,
* genwqe_free_sync_sgl() - Free memory for sgl and overlapping pages
*
* After the DMA transfer has been completed we free the memory for
- * the sgl and the cached pages. Data is being transfered from cached
+ * the sgl and the cached pages. Data is being transferred from cached
* pages into user-space buffers.
*/
int genwqe_free_sync_sgl(struct genwqe_dev *cd, struct genwqe_sgl *sgl)
--
2.15.1
From: Markus Elfring <[email protected]>
Date: Mon, 8 Jan 2018 10:21:25 +0100
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The script “checkpatch.pl” pointed information out like the following.
Comparison to NULL could be written …
Thus fix the affected source code places.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/misc/genwqe/card_utils.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/drivers/misc/genwqe/card_utils.c b/drivers/misc/genwqe/card_utils.c
index 0b466664c908..84408dc69020 100644
--- a/drivers/misc/genwqe/card_utils.c
+++ b/drivers/misc/genwqe/card_utils.c
@@ -58,7 +58,7 @@ int __genwqe_writeq(struct genwqe_dev *cd, u64 byte_offs, u64 val)
if (cd->err_inject & GENWQE_INJECT_HARDWARE_FAILURE)
return -EIO;
- if (cd->mmio == NULL)
+ if (!cd->mmio)
return -EIO;
if (pci_channel_offline(pci_dev))
@@ -88,7 +88,7 @@ u64 __genwqe_readq(struct genwqe_dev *cd, u64 byte_offs)
(byte_offs == IO_SLC_CFGREG_GFIR))
return 0x00000000ffff0000ull;
- if (cd->mmio == NULL)
+ if (!cd->mmio)
return 0xffffffffffffffffull;
return be64_to_cpu((__force __be64)__raw_readq(cd->mmio + byte_offs));
@@ -109,7 +109,7 @@ int __genwqe_writel(struct genwqe_dev *cd, u64 byte_offs, u32 val)
if (cd->err_inject & GENWQE_INJECT_HARDWARE_FAILURE)
return -EIO;
- if (cd->mmio == NULL)
+ if (!cd->mmio)
return -EIO;
if (pci_channel_offline(pci_dev))
@@ -131,7 +131,7 @@ u32 __genwqe_readl(struct genwqe_dev *cd, u64 byte_offs)
if (cd->err_inject & GENWQE_INJECT_HARDWARE_FAILURE)
return 0xffffffff;
- if (cd->mmio == NULL)
+ if (!cd->mmio)
return 0xffffffff;
return be32_to_cpu((__force __be32)__raw_readl(cd->mmio + byte_offs));
@@ -227,7 +227,7 @@ void *__genwqe_alloc_consistent(struct genwqe_dev *cd, size_t size,
void __genwqe_free_consistent(struct genwqe_dev *cd, size_t size,
void *vaddr, dma_addr_t dma_handle)
{
- if (vaddr == NULL)
+ if (!vaddr)
return;
dma_free_coherent(&cd->pci_dev->dev, size, vaddr, dma_handle);
@@ -323,7 +323,7 @@ int genwqe_alloc_sync_sgl(struct genwqe_dev *cd, struct genwqe_sgl *sgl,
sgl->sgl = __genwqe_alloc_consistent(cd, sgl->sgl_size,
&sgl->sgl_dma_addr);
- if (sgl->sgl == NULL) {
+ if (!sgl->sgl) {
dev_err(&pci_dev->dev,
"[%s] err: no memory available!\n", __func__);
return -ENOMEM;
@@ -333,7 +333,7 @@ int genwqe_alloc_sync_sgl(struct genwqe_dev *cd, struct genwqe_sgl *sgl,
if ((sgl->fpage_size != 0) && (sgl->fpage_size != PAGE_SIZE)) {
sgl->fpage = __genwqe_alloc_consistent(cd, PAGE_SIZE,
&sgl->fpage_dma_addr);
- if (sgl->fpage == NULL)
+ if (!sgl->fpage)
goto err_out;
/* Sync with user memory */
@@ -346,7 +346,7 @@ int genwqe_alloc_sync_sgl(struct genwqe_dev *cd, struct genwqe_sgl *sgl,
if (sgl->lpage_size != 0) {
sgl->lpage = __genwqe_alloc_consistent(cd, PAGE_SIZE,
&sgl->lpage_dma_addr);
- if (sgl->lpage == NULL)
+ if (!sgl->lpage)
goto err_out1;
/* Sync with user memory */
@@ -406,15 +406,12 @@ int genwqe_setup_sgl(struct genwqe_dev *cd, struct genwqe_sgl *sgl,
/* DMA mapping for requested page, offs, size */
size_to_map = min(size, PAGE_SIZE - map_offs);
- if ((p == 0) && (sgl->fpage != NULL)) {
+ if (p == 0 && sgl->fpage)
daddr = sgl->fpage_dma_addr + map_offs;
-
- } else if ((p == sgl->nr_pages - 1) &&
- (sgl->lpage != NULL)) {
+ else if ((p == sgl->nr_pages - 1) && sgl->lpage)
daddr = sgl->lpage_dma_addr;
- } else {
+ else
daddr = dma_list[p] + map_offs;
- }
size -= size_to_map;
map_offs = 0;
@@ -538,7 +535,7 @@ static int genwqe_free_user_pages(struct page **page_list,
unsigned int i;
for (i = 0; i < nr_pages; i++) {
- if (page_list[i] != NULL) {
+ if (page_list[i]) {
if (dirty)
set_page_dirty_lock(page_list[i]);
put_page(page_list[i]);
@@ -577,7 +574,7 @@ int genwqe_user_vmap(struct genwqe_dev *cd, struct dma_mapping *m, void *uaddr,
unsigned long data, offs;
struct pci_dev *pci_dev = cd->pci_dev;
- if ((uaddr == NULL) || (size == 0)) {
+ if (!uaddr || size == 0) {
m->size = 0; /* mark unused and not added */
return -EINVAL;
}
--
2.15.1
Hi Markus,
On 2018-01-08 10:41, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 8 Jan 2018 09:37:23 +0100
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/misc/genwqe/card_utils.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/misc/genwqe/card_utils.c
> b/drivers/misc/genwqe/card_utils.c
> index 8f2e6442d88b..55c389a9e7d7 100644
> --- a/drivers/misc/genwqe/card_utils.c
> +++ b/drivers/misc/genwqe/card_utils.c
> @@ -593,7 +593,6 @@ int genwqe_user_vmap(struct genwqe_dev *cd, struct
> dma_mapping *m, void *uaddr,
> sizeof(struct page *) + sizeof(dma_addr_t),
> GFP_KERNEL);
> if (!m->page_list) {
> - dev_err(&pci_dev->dev, "err: alloc page_list failed\n");
Are there different printouts which cover this? I mean the debug
printout is not
appearing all the time, it must be enabled, if I remember correctly. So
why do
you suggest to remove it?
> m->nr_pages = 0;
> m->u_vaddr = NULL;
> m->size = 0; /* mark unused and not added */
Regards
Frank
Hi Markus,
On 2018-01-08 10:42, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 8 Jan 2018 09:57:10 +0100
>
> Add a missing character in two words of these descriptions.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/misc/genwqe/card_utils.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/genwqe/card_utils.c
> b/drivers/misc/genwqe/card_utils.c
> index 55c389a9e7d7..0b466664c908 100644
> --- a/drivers/misc/genwqe/card_utils.c
> +++ b/drivers/misc/genwqe/card_utils.c
> @@ -453,7 +453,7 @@ int genwqe_setup_sgl(struct genwqe_dev *cd, struct
> genwqe_sgl *sgl,
> s += 8; /* continue 8 elements further */
> }
> fixup:
> - if (j == 1) { /* combining happend on last entry! */
> + if (j == 1) { /* combining happened on last entry! */
> s -= 8; /* full shift needed on previous sgl block */
> j = 7; /* shift all elements */
> }
> @@ -471,7 +471,7 @@ int genwqe_setup_sgl(struct genwqe_dev *cd, struct
> genwqe_sgl *sgl,
> * genwqe_free_sync_sgl() - Free memory for sgl and overlapping pages
> *
> * After the DMA transfer has been completed we free the memory for
> - * the sgl and the cached pages. Data is being transfered from cached
> + * the sgl and the cached pages. Data is being transferred from cached
> * pages into user-space buffers.
> */
> int genwqe_free_sync_sgl(struct genwqe_dev *cd, struct genwqe_sgl
> *sgl)
Looks good to me. Thanks.
Frank
Acked-by: Frank Haverkamp <[email protected]>
Hi Markus,
On 2018-01-08 10:43, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 8 Jan 2018 10:21:25 +0100
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The script “checkpatch.pl” pointed information out like the
> following.
>
> Comparison to NULL could be written …
>
> Thus fix the affected source code places.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/misc/genwqe/card_utils.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/misc/genwqe/card_utils.c
> b/drivers/misc/genwqe/card_utils.c
> index 0b466664c908..84408dc69020 100644
> --- a/drivers/misc/genwqe/card_utils.c
> +++ b/drivers/misc/genwqe/card_utils.c
> @@ -58,7 +58,7 @@ int __genwqe_writeq(struct genwqe_dev *cd, u64
> byte_offs, u64 val)
> if (cd->err_inject & GENWQE_INJECT_HARDWARE_FAILURE)
> return -EIO;
>
> - if (cd->mmio == NULL)
> + if (!cd->mmio)
> return -EIO;
>
> if (pci_channel_offline(pci_dev))
> @@ -88,7 +88,7 @@ u64 __genwqe_readq(struct genwqe_dev *cd, u64
> byte_offs)
> (byte_offs == IO_SLC_CFGREG_GFIR))
> return 0x00000000ffff0000ull;
>
> - if (cd->mmio == NULL)
> + if (!cd->mmio)
> return 0xffffffffffffffffull;
>
> return be64_to_cpu((__force __be64)__raw_readq(cd->mmio +
> byte_offs));
> @@ -109,7 +109,7 @@ int __genwqe_writel(struct genwqe_dev *cd, u64
> byte_offs, u32 val)
> if (cd->err_inject & GENWQE_INJECT_HARDWARE_FAILURE)
> return -EIO;
>
> - if (cd->mmio == NULL)
> + if (!cd->mmio)
> return -EIO;
>
> if (pci_channel_offline(pci_dev))
> @@ -131,7 +131,7 @@ u32 __genwqe_readl(struct genwqe_dev *cd, u64
> byte_offs)
> if (cd->err_inject & GENWQE_INJECT_HARDWARE_FAILURE)
> return 0xffffffff;
>
> - if (cd->mmio == NULL)
> + if (!cd->mmio)
> return 0xffffffff;
>
> return be32_to_cpu((__force __be32)__raw_readl(cd->mmio +
> byte_offs));
> @@ -227,7 +227,7 @@ void *__genwqe_alloc_consistent(struct genwqe_dev
> *cd, size_t size,
> void __genwqe_free_consistent(struct genwqe_dev *cd, size_t size,
> void *vaddr, dma_addr_t dma_handle)
> {
> - if (vaddr == NULL)
> + if (!vaddr)
> return;
>
> dma_free_coherent(&cd->pci_dev->dev, size, vaddr, dma_handle);
> @@ -323,7 +323,7 @@ int genwqe_alloc_sync_sgl(struct genwqe_dev *cd,
> struct genwqe_sgl *sgl,
>
> sgl->sgl = __genwqe_alloc_consistent(cd, sgl->sgl_size,
> &sgl->sgl_dma_addr);
> - if (sgl->sgl == NULL) {
> + if (!sgl->sgl) {
> dev_err(&pci_dev->dev,
> "[%s] err: no memory available!\n", __func__);
> return -ENOMEM;
> @@ -333,7 +333,7 @@ int genwqe_alloc_sync_sgl(struct genwqe_dev *cd,
> struct genwqe_sgl *sgl,
> if ((sgl->fpage_size != 0) && (sgl->fpage_size != PAGE_SIZE)) {
> sgl->fpage = __genwqe_alloc_consistent(cd, PAGE_SIZE,
> &sgl->fpage_dma_addr);
> - if (sgl->fpage == NULL)
> + if (!sgl->fpage)
> goto err_out;
>
> /* Sync with user memory */
> @@ -346,7 +346,7 @@ int genwqe_alloc_sync_sgl(struct genwqe_dev *cd,
> struct genwqe_sgl *sgl,
> if (sgl->lpage_size != 0) {
> sgl->lpage = __genwqe_alloc_consistent(cd, PAGE_SIZE,
> &sgl->lpage_dma_addr);
> - if (sgl->lpage == NULL)
> + if (!sgl->lpage)
> goto err_out1;
>
> /* Sync with user memory */
> @@ -406,15 +406,12 @@ int genwqe_setup_sgl(struct genwqe_dev *cd,
> struct genwqe_sgl *sgl,
> /* DMA mapping for requested page, offs, size */
> size_to_map = min(size, PAGE_SIZE - map_offs);
>
> - if ((p == 0) && (sgl->fpage != NULL)) {
> + if (p == 0 && sgl->fpage)
> daddr = sgl->fpage_dma_addr + map_offs;
> -
> - } else if ((p == sgl->nr_pages - 1) &&
> - (sgl->lpage != NULL)) {
> + else if ((p == sgl->nr_pages - 1) && sgl->lpage)
> daddr = sgl->lpage_dma_addr;
> - } else {
> + else
> daddr = dma_list[p] + map_offs;
> - }
>
> size -= size_to_map;
> map_offs = 0;
> @@ -538,7 +535,7 @@ static int genwqe_free_user_pages(struct page
> **page_list,
> unsigned int i;
>
> for (i = 0; i < nr_pages; i++) {
> - if (page_list[i] != NULL) {
> + if (page_list[i]) {
> if (dirty)
> set_page_dirty_lock(page_list[i]);
> put_page(page_list[i]);
> @@ -577,7 +574,7 @@ int genwqe_user_vmap(struct genwqe_dev *cd, struct
> dma_mapping *m, void *uaddr,
> unsigned long data, offs;
> struct pci_dev *pci_dev = cd->pci_dev;
>
> - if ((uaddr == NULL) || (size == 0)) {
> + if (!uaddr || size == 0) {
> m->size = 0; /* mark unused and not added */
> return -EINVAL;
> }
I personally like the explicit compare (ptr != NULL) more than the !ptr
notation.
When was the checkpatch.pl script modified to suggest the latter
notation?
Is there any advantage other than the shorter notation?
Regards
Frank
On Mon, Jan 08, 2018 at 01:45:02PM +0100, haver wrote:
> Hi Markus,
>
> On 2018-01-08 10:41, SF Markus Elfring wrote:
> > From: Markus Elfring <[email protected]>
> > Date: Mon, 8 Jan 2018 09:37:23 +0100
> >
> > Omit an extra message for a memory allocation failure in this function.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <[email protected]>
> > ---
> > drivers/misc/genwqe/card_utils.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/misc/genwqe/card_utils.c
> > b/drivers/misc/genwqe/card_utils.c
> > index 8f2e6442d88b..55c389a9e7d7 100644
> > --- a/drivers/misc/genwqe/card_utils.c
> > +++ b/drivers/misc/genwqe/card_utils.c
> > @@ -593,7 +593,6 @@ int genwqe_user_vmap(struct genwqe_dev *cd, struct
> > dma_mapping *m, void *uaddr,
> > sizeof(struct page *) + sizeof(dma_addr_t),
> > GFP_KERNEL);
> > if (!m->page_list) {
> > - dev_err(&pci_dev->dev, "err: alloc page_list failed\n");
>
> Are there different printouts which cover this? I mean the debug printout is
> not
> appearing all the time, it must be enabled, if I remember correctly.
The kmalloc() error messages are enabled by default. See warn_alloc().
The stack trace from this specific call site is going to be very clear
what went wrong.
> So why
> do
> you suggest to remove it?
>
It's a checkpatch warning and a small memory savings.
regards,
dan carpenter
>> @@ -593,7 +593,6 @@ int genwqe_user_vmap(struct genwqe_dev *cd, struct
>> dma_mapping *m, void *uaddr,
>> sizeof(struct page *) + sizeof(dma_addr_t),
>> GFP_KERNEL);
>> if (!m->page_list) {
>> - dev_err(&pci_dev->dev, "err: alloc page_list failed\n");
>
> Are there different printouts which cover this?
Is this error message redundant?
> I mean the debug printout is not appearing all the time,
> it must be enabled, if I remember correctly.
Would you like to clarify corresponding configuration possibilities any more?
> So why do you suggest to remove it?
Can the Linux allocation failure report be sufficient for this use case already?
Regards,
Markus
Hi Dan,
On 2018-01-08 14:23, Dan Carpenter wrote:
> On Mon, Jan 08, 2018 at 01:45:02PM +0100, haver wrote:
>> Hi Markus,
>>
>> On 2018-01-08 10:41, SF Markus Elfring wrote:
>> > From: Markus Elfring <[email protected]>
>> > Date: Mon, 8 Jan 2018 09:37:23 +0100
>> >
>> > Omit an extra message for a memory allocation failure in this function.
>> >
>> > This issue was detected by using the Coccinelle software.
>> >
>> > Signed-off-by: Markus Elfring <[email protected]>
>> > ---
>> > drivers/misc/genwqe/card_utils.c | 1 -
>> > 1 file changed, 1 deletion(-)
>> >
>> > diff --git a/drivers/misc/genwqe/card_utils.c
>> > b/drivers/misc/genwqe/card_utils.c
>> > index 8f2e6442d88b..55c389a9e7d7 100644
>> > --- a/drivers/misc/genwqe/card_utils.c
>> > +++ b/drivers/misc/genwqe/card_utils.c
>> > @@ -593,7 +593,6 @@ int genwqe_user_vmap(struct genwqe_dev *cd, struct
>> > dma_mapping *m, void *uaddr,
>> > sizeof(struct page *) + sizeof(dma_addr_t),
>> > GFP_KERNEL);
>> > if (!m->page_list) {
>> > - dev_err(&pci_dev->dev, "err: alloc page_list failed\n");
>>
>> Are there different printouts which cover this? I mean the debug
>> printout is
>> not
>> appearing all the time, it must be enabled, if I remember correctly.
>
> The kmalloc() error messages are enabled by default. See warn_alloc().
> The stack trace from this specific call site is going to be very clear
> what went wrong.
>
So it appears it is ok to be removed. Let's remove the redundant
printout as suggested.
>
>> So why
>> do
>> you suggest to remove it?
>>
>
> It's a checkpatch warning and a small memory savings.
>
> regards,
> dan carpenter
Acked-by: Frank Haverkamp <[email protected]>
On 2018-01-08 14:24, SF Markus Elfring wrote:
>>> @@ -593,7 +593,6 @@ int genwqe_user_vmap(struct genwqe_dev *cd,
>>> struct
>>> dma_mapping *m, void *uaddr,
>>> sizeof(struct page *) +
>>> sizeof(dma_addr_t),
>>> GFP_KERNEL);
>>> if (!m->page_list) {
>>> - dev_err(&pci_dev->dev, "err: alloc page_list
>>> failed\n");
>>
>> Are there different printouts which cover this?
>
> Is this error message redundant?
>
>
>> I mean the debug printout is not appearing all the time,
>> it must be enabled, if I remember correctly.
>
> Would you like to clarify corresponding configuration possibilities any
> more?
>
>
>> So why do you suggest to remove it?
>
> Can the Linux allocation failure report be sufficient for this use case
> already?
>
> Regards,
> Markus
According to Dans message in parallel, it should be redundant and I am
fine with removing it.
Acked-by: Frank Haverkamp <[email protected]>
> I personally like the explicit compare (ptr != NULL) more than the !ptr notation.
Coding style aspects can evolve, can't they?
> When was the checkpatch.pl script modified to suggest the latter notation?
Would you like to take another look at the software update “checkpatch: add
--strict "pointer comparison to NULL" test” from 2014-12-10?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b75ac618df751b927469ddbca63cf151a62f0f9d
> Is there any advantage other than the shorter notation?
* Do you eventually care for an influence on the run time characteristics
for the compilation of this software module?
* How do you think about to reduce the dependency on a special preprocessor symbol?
Regards,
Markus