| Message ID | 20230428012235.119333-2-raghuhack78@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixing check patch styling issues | expand |
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 <raghuhack78@gmail.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > 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 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 <raghuhack78@gmail.com> > --- > drivers/cxl/core/mbox.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Looks good, Reviewed-by: Vishal Verma <vishal.l.verma@intel.com> > > 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 <raghuhack78@gmail.com> 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 <raghuhack78@gmail.com> > > 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 <raghuhack78@gmail.com> > > > > 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/ZCQkPr6t8IOvF6bk@kroah.com/ and https://lore.kernel.org/linux-staging/ZBCjK2BXhfiFooeO@kroah.com/. 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 <raghuhack78@gmail.com> > > > > > > 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/ZCQkPr6t8IOvF6bk@kroah.com/ and > https://lore.kernel.org/linux-staging/ZBCjK2BXhfiFooeO@kroah.com/. 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 <alison.schofield@intel.com> 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 <raghuhack78@gmail.com> > > > > > > > > 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/ZCQkPr6t8IOvF6bk@kroah.com/ and > > https://lore.kernel.org/linux-staging/ZBCjK2BXhfiFooeO@kroah.com/. > > 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 > > > > > >
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);
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 <raghuhack78@gmail.com> --- drivers/cxl/core/mbox.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)