v2 changes:
Thanks Alison, Ira for your comments, modified the v1 patches as suggested.
Dropped the patch containing tab changes in port.c
v1 cover letter:
The following patches are cleanup or fixing the styling issues found
using checkpatch
In cxl/core/mbox.c, in case of null check failure, returning errno or
-ENOMEM in this case is good enough, removing the redundant dev_err
message.
In cxl/core/region.c, the else is not required after the return
statement, cleaned it up.
Verified the build and sanity by booting the guest VM using the freshly
built components.
Raghu H (2):
cxl/mbox: Remove redundant dev_err() after failed mem alloc
cxl/region: Remove else after return statement
drivers/cxl/core/mbox.c | 4 +---
drivers/cxl/core/region.c | 8 ++++----
2 files changed, 5 insertions(+), 7 deletions(-)
--
2.39.2
Issue found with checkpatch
A return of errno should be good enough if the memory allocation fails,
the error message here is redundatant as per the coding style, removing it.
Signed-off-by: Raghu H <[email protected]>
---
drivers/cxl/core/mbox.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index f2addb457172..11ea145b4b1f 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1112,10 +1112,8 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
struct cxl_dev_state *cxlds;
cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
- if (!cxlds) {
- dev_err(dev, "No memory available\n");
+ if (!cxlds)
return ERR_PTR(-ENOMEM);
- }
mutex_init(&cxlds->mbox_mutex);
mutex_init(&cxlds->event.log_lock);
--
2.39.2
Issue found with checkpatch
The else section here is redundant after return statement, removing it.
Sanity and correctness is not affected due to this fix.
Signed-off-by: Raghu H <[email protected]>
---
drivers/cxl/core/region.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index f29028148806..a46d6ad9ef0a 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2666,11 +2666,11 @@ static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
"Bypassing cpu_cache_invalidate_memregion() for testing!\n");
clear_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
return 0;
- } else {
- dev_err(&cxlr->dev,
- "Failed to synchronize CPU cache state\n");
- return -ENXIO;
}
+
+ dev_err(&cxlr->dev,
+ "Failed to synchronize CPU cache state\n");
+ return -ENXIO;
}
cpu_cache_invalidate_memregion(IORES_DESC_CXL);
--
2.39.2
On 4/27/23 6:22 PM, Raghu H wrote:
> Issue found with checkpatch
>
> A return of errno should be good enough if the memory allocation fails,
> the error message here is redundatant as per the coding style, removing it.
>
> Signed-off-by: Raghu H <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
> drivers/cxl/core/mbox.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index f2addb457172..11ea145b4b1f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1112,10 +1112,8 @@ struct cxl_dev_state *cxl_dev_state_create(struct device *dev)
> struct cxl_dev_state *cxlds;
>
> cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
> - if (!cxlds) {
> - dev_err(dev, "No memory available\n");
> + if (!cxlds)
> return ERR_PTR(-ENOMEM);
> - }
>
> mutex_init(&cxlds->mbox_mutex);
> mutex_init(&cxlds->event.log_lock);
On 4/27/23 6:22 PM, Raghu H wrote:
> Issue found with checkpatch
>
> The else section here is redundant after return statement, removing it.
> Sanity and correctness is not affected due to this fix.
>
> Signed-off-by: Raghu H <[email protected]>
Reviewed-by: Dave Jiang <[email protected]>
> ---
> drivers/cxl/core/region.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index f29028148806..a46d6ad9ef0a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2666,11 +2666,11 @@ static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
> "Bypassing cpu_cache_invalidate_memregion() for testing!\n");
> clear_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> return 0;
> - } else {
> - dev_err(&cxlr->dev,
> - "Failed to synchronize CPU cache state\n");
> - return -ENXIO;
> }
> +
> + dev_err(&cxlr->dev,
> + "Failed to synchronize CPU cache state\n");
> + return -ENXIO;
> }
>
> cpu_cache_invalidate_memregion(IORES_DESC_CXL);
On Mon, 2023-05-01 at 07:53 -0700, Dave Jiang wrote:
>
>
> On 4/27/23 6:22 PM, Raghu H wrote:
> > Issue found with checkpatch
> >
> > The else section here is redundant after return statement, removing it.
> > Sanity and correctness is not affected due to this fix.
> >
> > Signed-off-by: Raghu H <[email protected]>
>
> Reviewed-by: Dave Jiang <[email protected]>
>
> > ---
> > drivers/cxl/core/region.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
Looks good,
Reviewed-by: Vishal Verma <[email protected]>
> >
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index f29028148806..a46d6ad9ef0a 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -2666,11 +2666,11 @@ static int cxl_region_invalidate_memregion(struct cxl_region *cxlr)
> > "Bypassing cpu_cache_invalidate_memregion() for testing!\n");
> > clear_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> > return 0;
> > - } else {
> > - dev_err(&cxlr->dev,
> > - "Failed to synchronize CPU cache state\n");
> > - return -ENXIO;
> > }
> > +
> > + dev_err(&cxlr->dev,
> > + "Failed to synchronize CPU cache state\n");
> > + return -ENXIO;
> > }
> >
> > cpu_cache_invalidate_memregion(IORES_DESC_CXL);
On Fri, 2023-04-28 at 01:22 +0000, Raghu H wrote:
> Issue found with checkpatch
>
> A return of errno should be good enough if the memory allocation
> fails,
> the error message here is redundatant as per the coding style,
> removing it.
>
> Signed-off-by: Raghu H <[email protected]>
> ---
> drivers/cxl/core/mbox.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
Looks good,
Reviewed-by: Vishal Verma <[email protected]>
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index f2addb457172..11ea145b4b1f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1112,10 +1112,8 @@ struct cxl_dev_state
> *cxl_dev_state_create(struct device *dev)
> struct cxl_dev_state *cxlds;
>
> cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
> - if (!cxlds) {
> - dev_err(dev, "No memory available\n");
> + if (!cxlds)
> return ERR_PTR(-ENOMEM);
> - }
>
> mutex_init(&cxlds->mbox_mutex);
> mutex_init(&cxlds->event.log_lock);
On venerd? 28 aprile 2023 03:22:34 CEST Raghu H wrote:
> Issue found with checkpatch
>
> A return of errno should be good enough if the memory allocation fails,
> the error message here is redundatant
Typo: it's "redundant". No problem, I think you shouldn't resend only for the
purpose to fix this. But...
> as per the coding style, removing it.
>
> Signed-off-by: Raghu H <[email protected]>
Is "Raghu H" the name you sign legal documents with?
If not, please send a new version signed-off-by your full legal name.
Otherwise... sorry for the noise.
Thanks,
Fabio
> ---
> drivers/cxl/core/mbox.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index f2addb457172..11ea145b4b1f 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1112,10 +1112,8 @@ struct cxl_dev_state *cxl_dev_state_create(struct
> device *dev) struct cxl_dev_state *cxlds;
>
> cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
> - if (!cxlds) {
> - dev_err(dev, "No memory available\n");
> + if (!cxlds)
> return ERR_PTR(-ENOMEM);
> - }
>
> mutex_init(&cxlds->mbox_mutex);
> mutex_init(&cxlds->event.log_lock);
> --
> 2.39.2
On Wed, May 03, 2023 at 08:32:37PM +0200, Fabio wrote:
> On venerd? 28 aprile 2023 03:22:34 CEST Raghu H wrote:
> > Issue found with checkpatch
> >
> > A return of errno should be good enough if the memory allocation fails,
> > the error message here is redundatant
>
> Typo: it's "redundant". No problem, I think you shouldn't resend only for the
> purpose to fix this. But...
>
Raghu,
checkpatch --codespell will catch misspellings in the commit log, when
run on the HEAD^ commit or on the formatted patch file.
> > as per the coding style, removing it.
> >
> > Signed-off-by: Raghu H <[email protected]>
>
> Is "Raghu H" the name you sign legal documents with?
>
Fabio,
Rather than asking a specific question to determine if this is a
valid SOB, let's just point folks to the documentation to figure
it out themselves. I'm aware that the 'sign legal documents' test
has been used in the past, but kernel only actually requires a
known identity.
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
https://github.com/cncf/foundation/blob/659fd32c86dc/dco-guidelines.md
> If not, please send a new version signed-off-by your full legal name.
> Otherwise... sorry for the noise.
>
> Thanks,
>
> Fabio
>
> > ---
> > drivers/cxl/core/mbox.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index f2addb457172..11ea145b4b1f 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -1112,10 +1112,8 @@ struct cxl_dev_state *cxl_dev_state_create(struct
> > device *dev) struct cxl_dev_state *cxlds;
> >
> > cxlds = devm_kzalloc(dev, sizeof(*cxlds), GFP_KERNEL);
> > - if (!cxlds) {
> > - dev_err(dev, "No memory available\n");
> > + if (!cxlds)
> > return ERR_PTR(-ENOMEM);
> > - }
> >
> > mutex_init(&cxlds->mbox_mutex);
> > mutex_init(&cxlds->event.log_lock);
> > --
> > 2.39.2
>
>
>
>
On Fri, 28 Apr 2023, Raghu H wrote:
>Issue found with checkpatch
fyi for both patches, these are not "issues" - you can remove it, or the line altogether.
Thanks,
Davidlohr
On Wed, May 03, 2023 at 03:36:50PM -0700, Davidlohr Bueso wrote:
> On Fri, 28 Apr 2023, Raghu H wrote:
>
> > Issue found with checkpatch
>
> fyi for both patches, these are not "issues" - you can remove it, or the line altogether.
>
> Thanks,
> Davidlohr
Hi Raghu,
Perhaps this patchset got you more attention than you may have expected ;)
So, this is an example of when reviewers disagree. I asked you to note
that checkpatch found these 'issues' and David disagrees.
If David had known I'd asked you to update the commit log to include the
checkpatch credit, he may have ignored it, or maybe not. I don't find
the word 'issue' to be misused. There are many flavors of this phrase used
in kernel commit logs that address checkpatch found issues.
The way the next round of reviewers knows what the first round asked
for is by looking at the changelog. (And, if they need more detail, they
pull up the previous patchset discussion.)
The changelog in the cover letter, or per patch, needs to explicitly
say what has changed.
The v2 says this:
>> v2 changes:
>> Thanks Alison, Ira for your comments, modified the v1 patches as suggested.
>> Dropped the patch containing tab changes in port.c
Next time, be more specific, like this:
v2 changes:
- Update commit logs to credit checkpatch (Alison)
- Update commit msgs to conform to subsystem style (Alison)
- Drop the patch containing tab changes in port.c (Ira)
Thanks,
Alison
On gioved? 4 maggio 2023 00:03:07 CEST Alison Schofield wrote:
> On Wed, May 03, 2023 at 08:32:37PM +0200, Fabio wrote:
> > On venerd? 28 aprile 2023 03:22:34 CEST Raghu H wrote:
[...]
> > >
> > > Signed-off-by: Raghu H <[email protected]>
> >
> > Is "Raghu H" the name you sign legal documents with?
>
> Fabio,
> Rather than asking a specific question to determine if this is a
> valid SOB, let's just point folks to the documentation to figure
> it out themselves.
> I'm aware that the 'sign legal documents' test
> has been used in the past, but kernel only actually requires a
> known identity.
>
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-you
> r-work-the-developer-s-certificate-of-origin
> https://github.com/cncf/foundation/blob/659fd32c86dc/dco-guidelines.md
Alison,
Thanks for your suggestions.
I have just a couple of questions about this issue...
1) How do we know that the "real name", which the Linux official documentation
refers to, should be interpreted in accordance to the document pointed by the
second link you provided?
I mean, how can we be sure that the official documentation should be
interpreted according to the second link, since it doesn't even cite that
document from CNCF?
Can you provide links to documents / LKML's threads that state agreement of
our Community about the "relaxed" interpretation by CNCF?
2) It looks that some maintainers (e.g., Greg K-H) still interpret "[] using
your real name (sorry, no pseudonyms or anonymous contributions.)" in a
"strict" and "common" sense.
Can you remember that Greg refused all patches from "Kloudifold" and why? If
not, please take a look at the following two questions / objections from Greg:
https://lore.kernel.org/linux-staging/[email protected]/ and
https://lore.kernel.org/linux-staging/[email protected]/.
It seems that this issue it's not yet settled.
Am I overlooking something?
Again thanks,
Fabio
> > If not, please send a new version signed-off-by your full legal name.
> > Otherwise... sorry for the noise.
> >
> > Thanks,
> >
> > Fabio
On Thu, May 04, 2023 at 12:46:37PM +0200, Fabio wrote:
> On gioved? 4 maggio 2023 00:03:07 CEST Alison Schofield wrote:
> > On Wed, May 03, 2023 at 08:32:37PM +0200, Fabio wrote:
> > > On venerd? 28 aprile 2023 03:22:34 CEST Raghu H wrote:
>
> [...]
>
> > > >
> > > > Signed-off-by: Raghu H <[email protected]>
> > >
> > > Is "Raghu H" the name you sign legal documents with?
> >
> > Fabio,
> > Rather than asking a specific question to determine if this is a
> > valid SOB, let's just point folks to the documentation to figure
> > it out themselves.
> > I'm aware that the 'sign legal documents' test
> > has been used in the past, but kernel only actually requires a
> > known identity.
> >
> > https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-you
> > r-work-the-developer-s-certificate-of-origin
> > https://github.com/cncf/foundation/blob/659fd32c86dc/dco-guidelines.md
>
> Alison,
>
> Thanks for your suggestions.
>
> I have just a couple of questions about this issue...
>
> 1) How do we know that the "real name", which the Linux official documentation
> refers to, should be interpreted in accordance to the document pointed by the
> second link you provided?
>
> I mean, how can we be sure that the official documentation should be
> interpreted according to the second link, since it doesn't even cite that
> document from CNCF?
>
> Can you provide links to documents / LKML's threads that state agreement of
> our Community about the "relaxed" interpretation by CNCF?
Citation is hidden it git history. See:
d4563201f33a ("Documentation: simplify and clarify DCO contribution example language")
>
> 2) It looks that some maintainers (e.g., Greg K-H) still interpret "[] using
> your real name (sorry, no pseudonyms or anonymous contributions.)" in a
> "strict" and "common" sense.
See the commit log above. The language was updated to say
"using a known identity (sorry, no anonymous contributions.)"
>
> Can you remember that Greg refused all patches from "Kloudifold" and why? If
> not, please take a look at the following two questions / objections from Greg:
> https://lore.kernel.org/linux-staging/[email protected]/ and
> https://lore.kernel.org/linux-staging/[email protected]/.
The second link above is Greg recognizing that known pseudonyms are
allowed.
>
> It seems that this issue it's not yet settled.
> Am I overlooking something?
Hey, I'm not meaning to jump on you for asking Raghu the question.
I realize you are being helpful to someone who is submitting their first
patch. I'm just saying to make the submitter aware of the guideline and
put the burden on them to make sure they're using a known identity.
Sometimes, what one person thinks of as 'common' is not. Let's refer to
the docs and not add out personal or historical layers of interpretation
on top of it. (The legal doc signing question may not apply to everyone.)
Alison
>
> Again thanks,
>
> Fabio
>
> > > If not, please send a new version signed-off-by your full legal name.
> > > Otherwise... sorry for the noise.
> > >
> > > Thanks,
> > >
> > > Fabio
>
>
>
Hello All,
Just checked the response to this patch, sorry for responding late here.
I will take a note on all the points raised and will follow the
guidelines in future patches, and will correct this patch too.
Regards
Raghu
On Thu, May 4, 2023 at 8:53 PM Alison Schofield
<[email protected]> wrote:
>
> On Thu, May 04, 2023 at 12:46:37PM +0200, Fabio wrote:
> > On giovedì 4 maggio 2023 00:03:07 CEST Alison Schofield wrote:
> > > On Wed, May 03, 2023 at 08:32:37PM +0200, Fabio wrote:
> > > > On venerdì 28 aprile 2023 03:22:34 CEST Raghu H wrote:
> >
> > [...]
> >
> > > > >
> > > > > Signed-off-by: Raghu H <[email protected]>
> > > >
> > > > Is "Raghu H" the name you sign legal documents with?
> > >
> > > Fabio,
> > > Rather than asking a specific question to determine if this is a
> > > valid SOB, let's just point folks to the documentation to figure
> > > it out themselves.
> > > I'm aware that the 'sign legal documents' test
> > > has been used in the past, but kernel only actually requires a
> > > known identity.
> > >
> > > https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-you
> > > r-work-the-developer-s-certificate-of-origin
> > > https://github.com/cncf/foundation/blob/659fd32c86dc/dco-guidelines.md
> >
> > Alison,
> >
> > Thanks for your suggestions.
> >
> > I have just a couple of questions about this issue...
> >
> > 1) How do we know that the "real name", which the Linux official documentation
> > refers to, should be interpreted in accordance to the document pointed by the
> > second link you provided?
> >
> > I mean, how can we be sure that the official documentation should be
> > interpreted according to the second link, since it doesn't even cite that
> > document from CNCF?
> >
> > Can you provide links to documents / LKML's threads that state agreement of
> > our Community about the "relaxed" interpretation by CNCF?
>
> Citation is hidden it git history. See:
> d4563201f33a ("Documentation: simplify and clarify DCO contribution example language")
>
> >
> > 2) It looks that some maintainers (e.g., Greg K-H) still interpret "[] using
> > your real name (sorry, no pseudonyms or anonymous contributions.)" in a
> > "strict" and "common" sense.
>
> See the commit log above. The language was updated to say
> "using a known identity (sorry, no anonymous contributions.)"
>
> >
> > Can you remember that Greg refused all patches from "Kloudifold" and why? If
> > not, please take a look at the following two questions / objections from Greg:
> > https://lore.kernel.org/linux-staging/[email protected]/ and
> > https://lore.kernel.org/linux-staging/[email protected]/.
>
> The second link above is Greg recognizing that known pseudonyms are
> allowed.
>
> >
> > It seems that this issue it's not yet settled.
> > Am I overlooking something?
>
> Hey, I'm not meaning to jump on you for asking Raghu the question.
> I realize you are being helpful to someone who is submitting their first
> patch. I'm just saying to make the submitter aware of the guideline and
> put the burden on them to make sure they're using a known identity.
>
> Sometimes, what one person thinks of as 'common' is not. Let's refer to
> the docs and not add out personal or historical layers of interpretation
> on top of it. (The legal doc signing question may not apply to everyone.)
>
> Alison
>
> >
> > Again thanks,
> >
> > Fabio
> >
> > > > If not, please send a new version signed-off-by your full legal name.
> > > > Otherwise... sorry for the noise.
> > > >
> > > > Thanks,
> > > >
> > > > Fabio
> >
> >
> >