On Fri, Nov 10, 2023 at 02:51:57AM +0530, Abhinav Singh wrote:
> Sparse static analysis tools generate a warning with this message
> "Using plain integer as NULL pointer". In this case this warning is
> being shown because we are trying to initialize pointer to NULL using
> integer value 0.
And that is a problem because?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/27/23 22:44, Borislav Petkov wrote:
> On Fri, Nov 10, 2023 at 02:51:57AM +0530, Abhinav Singh wrote:
>> Sparse static analysis tools generate a warning with this message
>> "Using plain integer as NULL pointer". In this case this warning is
>> being shown because we are trying to initialize pointer to NULL using
>> integer value 0.
>
> And that is a problem because?
>
Hello, thanks for reviewing this. As of now this is only a warning issue
in kernel. I saw this post by linus
https://www.spinics.net/lists/linux-sparse/msg10066.html and thought of
submitting a patch. Also a similar patch of mine got accepted
https://www.mail-archive.com/[email protected]/msg2560740.html,
so thought about opening this one as well.
Thank You,
Abhinav Singh
On Mon, Nov 27, 2023 at 11:53:02PM +0530, Abhinav Singh wrote:
> Hello, thanks for reviewing this. As of now this is only a warning issue in
> kernel. I saw this post by linus
> https://www.spinics.net/lists/linux-sparse/msg10066.html and thought of
> submitting a patch. Also a similar patch of mine got accepted
> https://www.mail-archive.com/[email protected]/msg2560740.html,
> so thought about opening this one as well.
Lemme try to understand what you're saying: just because someone
accepted a patch of yours, then others should not ask you to improve
your commit message so that it explains *why* a change should be done.
How about you put the gist of what Linus is saying in your commit
message? Don't you think it would be a much better commit message then?
Especially if it explains why, even if it is the case that 0 == NULL, we
don't want those in the kernel.
Hmmm?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/28/23 00:09, Borislav Petkov wrote:
> On Mon, Nov 27, 2023 at 11:53:02PM +0530, Abhinav Singh wrote:
>> Hello, thanks for reviewing this. As of now this is only a warning issue in
>> kernel. I saw this post by linus
>> https://www.spinics.net/lists/linux-sparse/msg10066.html and thought of
>> submitting a patch. Also a similar patch of mine got accepted
>> https://www.mail-archive.com/[email protected]/msg2560740.html,
>> so thought about opening this one as well.
>
> Lemme try to understand what you're saying: just because someone
> accepted a patch of yours, then others should not ask you to improve
> your commit message so that it explains *why* a change should be done.
>
> How about you put the gist of what Linus is saying in your commit
> message? Don't you think it would be a much better commit message then?
>
> Especially if it explains why, even if it is the case that 0 == NULL, we
> don't want those in the kernel.
>
> Hmmm?
>
Hello, my sincere apologies, I wrongly interpreted that you were asking
for a reason in reply rather than in the commit message itself. Yeah I
agree putting a reason in the commit message will make more sense. Just
to be correct this time, I need to put a reason why this needs to be
fixed, in the patch itself, right?
Thank You,
Abhinav Singh
On Tue, Nov 28, 2023 at 12:23:54AM +0530, Abhinav Singh wrote:
> Just to be correct this time, I need to put a reason why this needs to
> be fixed, in the patch itself, right?
No, the commit message is perfectly fine for that.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/28/23 01:35, Borislav Petkov wrote:
> On Tue, Nov 28, 2023 at 12:23:54AM +0530, Abhinav Singh wrote:
>> Just to be correct this time, I need to put a reason why this needs to
>> be fixed, in the patch itself, right?
>
> No, the commit message is perfectly fine for that.
>
Okay, so I guess as of now there is no change needed, so I need not send
a v2 patch, right?
Thank You,
Abhinav Singh
On Tue, Nov 28, 2023 at 01:49:06AM +0530, Abhinav Singh wrote:a
> Okay, so I guess as of now there is no change needed, so I need not
> send a v2 patch, right?
As of now, you should go back and reread the whole thread.
I'm going to give you one last example and then stop:
Imagine one fine day you're doing git archeology, you find the place in
the code about which you want to find out why it was changed the way it
is now.
You do git annotate <filename> ... find the line, see the commit id and
you do:
git show <commit id>
You read the commit message and there's just gibberish and nothing's
explaining *why* that change was done. And you start scratching your
head, trying to figure out why. Because the damn commit message is worth
sh*t.
This happens to us maintainers at least once a week. Well, I don't want
that to happen in my tree anymore.
You catch my drift? :)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Sparse static analysis tools generate a warning with this message
"Using plain integer as NULL pointer". In this case this warning is
being shown because we are trying to initialize pointer to NULL using
integer value 0.
The reason for this change is that use of numeric 0 for a null pointer is
unacceptable. See this link for the long description why:
Link: https://www.spinics.net/lists/linux-sparse/msg10066.html
Signed-off-by: Abhinav Singh <[email protected]>
---
v1 -> v2: 1. Fixed the comment section descrbing the current code.
2. Added a reason for why this change is required.
drivers/edac/i7core_edac.c | 4 ++--
drivers/edac/sb_edac.c | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
index 08bf20c60111..4c76d0f180ec 100644
--- a/drivers/edac/i7core_edac.c
+++ b/drivers/edac/i7core_edac.c
@@ -376,7 +376,7 @@ static const struct pci_id_table pci_dev_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_nehalem),
PCI_ID_TABLE_ENTRY(pci_dev_descr_lynnfield),
PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_westmere),
- {NULL,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
/*
@@ -385,7 +385,7 @@ static const struct pci_id_table pci_dev_table[] = {
static const struct pci_device_id i7core_pci_tbl[] = {
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_X58_HUB_MGMT)},
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LYNNFIELD_QPI_LINK0)},
- {0,} /* 0 terminated list. */
+ {0,} /* NULL terminated list. */
};
/****************************************************************************
diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index a3f50a66de33..5d9a2963dc54 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -439,7 +439,7 @@ static const struct pci_id_descr pci_dev_descr_sbridge[] = {
static const struct pci_id_table pci_dev_descr_sbridge_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_sbridge, ARRAY_SIZE(pci_dev_descr_sbridge), 1, SANDY_BRIDGE),
- {NULL,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
/* This changes depending if 1HA or 2HA:
@@ -576,7 +576,7 @@ static const struct pci_id_descr pci_dev_descr_haswell[] = {
static const struct pci_id_table pci_dev_descr_haswell_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_haswell, 13, 2, HASWELL),
- {NULL,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
/* Knight's Landing Support */
@@ -686,7 +686,7 @@ static const struct pci_id_descr pci_dev_descr_broadwell[] = {
static const struct pci_id_table pci_dev_descr_broadwell_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_broadwell, 10, 2, BROADWELL),
- {NULL,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
--
2.39.2
On 11/28/23 03:11, Abhinav Singh wrote:
> Sparse static analysis tools generate a warning with this message
> "Using plain integer as NULL pointer". In this case this warning is
> being shown because we are trying to initialize pointer to NULL using
> integer value 0.
>
> The reason for this change is that use of numeric 0 for a null pointer is
> unacceptable. See this link for the long description why:
> Link: https://www.spinics.net/lists/linux-sparse/msg10066.html
>
> Signed-off-by: Abhinav Singh <[email protected]>
> ---
> v1 -> v2: 1. Fixed the comment section descrbing the current code.
> 2. Added a reason for why this change is required.
>
> drivers/edac/i7core_edac.c | 4 ++--
> drivers/edac/sb_edac.c | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
> index 08bf20c60111..4c76d0f180ec 100644
> --- a/drivers/edac/i7core_edac.c
> +++ b/drivers/edac/i7core_edac.c
> @@ -376,7 +376,7 @@ static const struct pci_id_table pci_dev_table[] = {
> PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_nehalem),
> PCI_ID_TABLE_ENTRY(pci_dev_descr_lynnfield),
> PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_westmere),
> - {NULL,} /* 0 terminated list. */
> + {NULL,} /* NULL terminated list. */
> };
>
> /*
> @@ -385,7 +385,7 @@ static const struct pci_id_table pci_dev_table[] = {
> static const struct pci_device_id i7core_pci_tbl[] = {
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_X58_HUB_MGMT)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LYNNFIELD_QPI_LINK0)},
> - {0,} /* 0 terminated list. */
> + {0,} /* NULL terminated list. */
> };
>
> /****************************************************************************
> diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
> index a3f50a66de33..5d9a2963dc54 100644
> --- a/drivers/edac/sb_edac.c
> +++ b/drivers/edac/sb_edac.c
> @@ -439,7 +439,7 @@ static const struct pci_id_descr pci_dev_descr_sbridge[] = {
>
> static const struct pci_id_table pci_dev_descr_sbridge_table[] = {
> PCI_ID_TABLE_ENTRY(pci_dev_descr_sbridge, ARRAY_SIZE(pci_dev_descr_sbridge), 1, SANDY_BRIDGE),
> - {NULL,} /* 0 terminated list. */
> + {NULL,} /* NULL terminated list. */
> };
>
> /* This changes depending if 1HA or 2HA:
> @@ -576,7 +576,7 @@ static const struct pci_id_descr pci_dev_descr_haswell[] = {
>
> static const struct pci_id_table pci_dev_descr_haswell_table[] = {
> PCI_ID_TABLE_ENTRY(pci_dev_descr_haswell, 13, 2, HASWELL),
> - {NULL,} /* 0 terminated list. */
> + {NULL,} /* NULL terminated list. */
> };
>
> /* Knight's Landing Support */
> @@ -686,7 +686,7 @@ static const struct pci_id_descr pci_dev_descr_broadwell[] = {
>
> static const struct pci_id_table pci_dev_descr_broadwell_table[] = {
> PCI_ID_TABLE_ENTRY(pci_dev_descr_broadwell, 10, 2, BROADWELL),
> - {NULL,} /* 0 terminated list. */
> + {NULL,} /* NULL terminated list. */
> };
>
>
Thank You, for explaining me what needs to be done. Yeah without a
proper reason a change is never really clear which can be confusing to
anyone looking. I will make sure I add a reason in every patch from now on.
Thank You,
Abhinav Singh
> From: Abhinav Singh <[email protected]>
> ...
> diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c index
> 08bf20c60111..4c76d0f180ec 100644
> --- a/drivers/edac/i7core_edac.c
> +++ b/drivers/edac/i7core_edac.c
> @@ -376,7 +376,7 @@ static const struct pci_id_table pci_dev_table[] = {
> PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_nehalem),
> PCI_ID_TABLE_ENTRY(pci_dev_descr_lynnfield),
> PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_westmere),
> - {NULL,} /* 0 terminated list. */
> + {NULL,} /* NULL terminated list. */
> };
>
> /*
> @@ -385,7 +385,7 @@ static const struct pci_id_table pci_dev_table[] =
> { static const struct pci_device_id i7core_pci_tbl[] = {
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_X58_HUB_MGMT)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> PCI_DEVICE_ID_INTEL_LYNNFIELD_QPI_LINK0)},
> - {0,} /* 0 terminated list. */
> + {0,} /* NULL terminated list. */
> };
The updated comment (NULL pointer) mismatches the code (zero value).
Sparse static analysis tools generate a warning with this message
"Using plain integer as NULL pointer". In this case this warning is
being shown because we are trying to initialize pointer to NULL using
integer value 0.
The reason for this change is that use of numeric 0 for a null pointer is
unacceptable. See this link for the long description why:
Link: https://www.spinics.net/lists/linux-sparse/msg10066.html
Signed-off-by: Abhinav Singh <[email protected]>
---
v1 -> v2: 1. Fixed the comment section descrbing the current code.
2. Added a reason for why this change is required.
v2 -> v3: 1. Reversed change made in comments by mistake.
drivers/edac/i7core_edac.c | 2 +-
drivers/edac/sb_edac.c | 10 +++++-----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
index 23d25724bae4..1177da186eea 100644
--- a/drivers/edac/i7core_edac.c
+++ b/drivers/edac/i7core_edac.c
@@ -376,7 +376,7 @@ static const struct pci_id_table pci_dev_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_nehalem),
PCI_ID_TABLE_ENTRY(pci_dev_descr_lynnfield),
PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_westmere),
- {0,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
/*
diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 0c779a0326b6..24ee6f28cfbe 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -439,7 +439,7 @@ static const struct pci_id_descr pci_dev_descr_sbridge[] = {
static const struct pci_id_table pci_dev_descr_sbridge_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_sbridge, ARRAY_SIZE(pci_dev_descr_sbridge), 1, SANDY_BRIDGE),
- {0,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
/* This changes depending if 1HA or 2HA:
@@ -505,7 +505,7 @@ static const struct pci_id_descr pci_dev_descr_ibridge[] = {
static const struct pci_id_table pci_dev_descr_ibridge_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_ibridge, 12, 2, IVY_BRIDGE),
- {0,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
/* Haswell support */
@@ -576,7 +576,7 @@ static const struct pci_id_descr pci_dev_descr_haswell[] = {
static const struct pci_id_table pci_dev_descr_haswell_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_haswell, 13, 2, HASWELL),
- {0,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
/* Knight's Landing Support */
@@ -620,7 +620,7 @@ static const struct pci_id_descr pci_dev_descr_knl[] = {
static const struct pci_id_table pci_dev_descr_knl_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_knl, ARRAY_SIZE(pci_dev_descr_knl), 1, KNIGHTS_LANDING),
- {0,}
+ {NULL,}
};
/*
@@ -686,7 +686,7 @@ static const struct pci_id_descr pci_dev_descr_broadwell[] = {
static const struct pci_id_table pci_dev_descr_broadwell_table[] = {
PCI_ID_TABLE_ENTRY(pci_dev_descr_broadwell, 10, 2, BROADWELL),
- {0,} /* 0 terminated list. */
+ {NULL,} /* NULL terminated list. */
};
--
2.39.2
On 11/28/23 20:13, Borislav Petkov wrote:
> On Tue, Nov 28, 2023 at 07:47:03PM +0530, Abhinav Singh wrote:
>> Sparse static analysis tools generate a warning with this message
>> "Using plain integer as NULL pointer". In this case this warning is
>> being shown because we are trying to initialize pointer to NULL using
>> integer value 0.
>
> Applied after massaging, thanks.
>
Thanks a lot for reviewing and merging the patch maintainers.
Thank You,
Abhinav Singh
On Tue, Nov 28, 2023 at 07:47:03PM +0530, Abhinav Singh wrote:
> Sparse static analysis tools generate a warning with this message
> "Using plain integer as NULL pointer". In this case this warning is
> being shown because we are trying to initialize pointer to NULL using
> integer value 0.
Applied after massaging, thanks.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette