Re: [PATCH] drm/nouveau/nvif: Avoid build error due to potential integer overflows

[Joe Perches]

On Sat, 2024-05-18 at 11:23 -0700, Guenter Roeck wrote:
> On 5/18/24 10:32, Kees Cook wrote:
> 
[]
> > I think the INT_MAX test is actually better in this case because
> > nvif_object_ioctl()'s size argument is u32:
> > 
> > ret = nvif_object_ioctl(object, args, sizeof(*args) + size, NULL);
> >
> > 
> > So that could wrap around, even though the allocation may not.
> > 
> > Better yet, since "sizeof(*args) + size" is repeated 3 times in the
> > function, I'd recommend:
> > 
> > ...
> > u32 args_size;
> > 
> > if (check_add_overflow(sizeof(*args), size, _size))
> > return -ENOMEM;
> > if (args_size > sizeof(stack)) {
> > if (!(args = kmalloc(args_size, GFP_KERNEL)))

trivia:

More typical kernel style would use separate alloc and test

args = kmalloc(args_size, GFP_KERNEL);
if (!args)

> > return -ENOMEM;
> >  } else {
> >  args = (void *)stack;
> >  }
> > ...
> >  ret = nvif_object_ioctl(object, args, args_size, NULL);
> > 
> > This will catch the u32 overflow to nvif_object_ioctl(), catch an
> > allocation underflow on 32-bits systems, and make the code more
> > readable. :)
> > 
> 
> Makes sense. I'll change that and send v2.
> 
> Thanks,
> Guenter
> 
> 

Re: [PATCH] drm/nouveau/nvif: Avoid build error due to potential integer overflows

[Joe Perches]

On Sat, 2024-05-18 at 11:23 -0700, Guenter Roeck wrote:
> On 5/18/24 10:32, Kees Cook wrote:
> 
[]
> > I think the INT_MAX test is actually better in this case because
> > nvif_object_ioctl()'s size argument is u32:
> > 
> > ret = nvif_object_ioctl(object, args, sizeof(*args) + size, NULL);
> >
> > 
> > So that could wrap around, even though the allocation may not.
> > 
> > Better yet, since "sizeof(*args) + size" is repeated 3 times in the
> > function, I'd recommend:
> > 
> > ...
> > u32 args_size;
> > 
> > if (check_add_overflow(sizeof(*args), size, _size))
> > return -ENOMEM;
> > if (args_size > sizeof(stack)) {
> > if (!(args = kmalloc(args_size, GFP_KERNEL)))

trivia:

More typical kernel style would use separate alloc and test

args = kmalloc(args_size, GFP_KERNEL);
if (!args)

> > return -ENOMEM;
> >  } else {
> >  args = (void *)stack;
> >  }
> > ...
> >  ret = nvif_object_ioctl(object, args, args_size, NULL);
> > 
> > This will catch the u32 overflow to nvif_object_ioctl(), catch an
> > allocation underflow on 32-bits systems, and make the code more
> > readable. :)
> > 
> 
> Makes sense. I'll change that and send v2.
> 
> Thanks,
> Guenter
> 
> 

Re: [PATCH v6] checkpatch: add check for snprintf to scnprintf

[Joe Perches]

On Mon, 2024-04-29 at 12:49 -0700, Kees Cook wrote:
> On Mon, Apr 29, 2024 at 06:39:28PM +, Justin Stitt wrote:
> > I am going to quote Lee Jones who has been doing some snprintf ->
> > scnprintf refactorings:
> > 
> > "There is a general misunderstanding amongst engineers that
> > {v}snprintf() returns the length of the data *actually* encoded into the
> > destination array.  However, as per the C99 standard {v}snprintf()
> > really returns the length of the data that *would have been* written if
> > there were enough space for it.  This misunderstanding has led to
> > buffer-overruns in the past.  It's generally considered safer to use the
> > {v}scnprintf() variants in their place (or even sprintf() in simple
> > cases).  So let's do that."
> > 
> > To help prevent new instances of snprintf() from popping up, let's add a
> > check to checkpatch.pl.
> > 
> > Suggested-by: Finn Thain 
> > Signed-off-by: Justin Stitt 
> 
> Thanks!
> 
> Reviewed-by: Kees Cook 
> 

$ git grep -P '\b((v|)snprintf)\s*\(' | wc -l
7745
$ git grep -P '(?:return\s+|=\s*)\b((v|)snprintf)\s*\(' | wc -l
1626

Given there are ~5000 uses of these that don't care
whether or not it's snprintf or scnprintf, I think this
is not great.

I'd much rather make sure the return value of the call
is used before suggesting an alternative.

$ git grep  -P '\b((v|)snprintf)\s*\(.*PAGE_SIZE' | wc -l
515

And about 1/3 of these snprintf calls are for sysfs style
output that ideally would be converted to sysfs_emit or
sysfs_emit_at instead.

Re: [PATCH] PCI/AER: Print error message as per the TODO

[Joe Perches]

On Mon, 2024-04-15 at 16:10 +, Abhinav Jain wrote:
> Add a pr_err() to print the add device error in find_device_iter()
[]
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
[]
> @@ -885,7 +885,8 @@ static int find_device_iter(struct pci_dev *dev, void 
> *data)
>   /* List this device */
>   if (add_error_device(e_info, dev)) {
>   /* We cannot handle more... Stop iteration */
> - /* TODO: Should print error message here? */
> + pr_err("find_device_iter: Cannot handle more devices.
> + Stopping iteration");

You are adding unnecessary whitespace after the period.
String concatenation keeps _all_ the whitespace.

The format is fine on a single line too.

Something like:

pr_notice("%s: Cannot handle more devices - iteration 
stopped\n",
  __func__);

Re: [PATCH v4] checkpatch: add check for snprintf to scnprintf

[Joe Perches]

On Thu, 2024-04-11 at 22:01 +0200, Christophe JAILLET wrote:
> Le 08/04/2024 à 22:53, Justin Stitt a écrit :
> > I am going to quote Lee Jones who has been doing some snprintf ->
> > scnprintf refactorings:
> > 
> > "There is a general misunderstanding amongst engineers that
> > {v}snprintf() returns the length of the data *actually* encoded into the
> > destination array.  However, as per the C99 standard {v}snprintf()
> > really returns the length of the data that *would have been* written if
> > there were enough space for it.  This misunderstanding has led to
> > buffer-overruns in the past.  It's generally considered safer to use the
> > {v}scnprintf() variants in their place (or even sprintf() in simple
> > cases).  So let's do that."
> > 
> > To help prevent new instances of snprintf() from popping up, let's add a
> > check to checkpatch.pl.
> > 
> > Suggested-by: Finn Thain 
> > Signed-off-by: Justin Stitt 
> > ---
> > Changes in v4:
> > - also check for vsnprintf variant (thanks Bill)
> > - Link to v3: 
> > https://lore.kernel.org/r/20240315-snprintf-checkpatch-v3-1-a451e7664...@google.com
> > 
> > Changes in v3:
> > - fix indentation
> > - add reference link (https://github.com/KSPP/linux/issues/105) (thanks Joe)
> > - Link to v2: 
> > https://lore.kernel.org/r/20240221-snprintf-checkpatch-v2-1-9baeb59da...@google.com
> > 
> > Changes in v2:
> > - Had a vim moment and deleted a character before sending the patch.
> > - Replaced the character :)
> > - Link to v1: 
> > https://lore.kernel.org/r/20240221-snprintf-checkpatch-v1-1-3ac5025b5...@google.com
> > ---
> >  From a discussion here [1].
> > 
> > [1]: 
> > https://lore.kernel.org/all/0f9c95f9-2c14-eee6-7faf-635880edc...@linux-m68k.org/
> > ---
> >   scripts/checkpatch.pl | 6 ++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 9c4c4a61bc83..a0fd681ea837 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -7012,6 +7012,12 @@ sub process {
> >  "Prefer strscpy, strscpy_pad, or __nonstring over 
> > strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
> > }
> >   
> > +# {v}snprintf uses that should likely be {v}scnprintf
> > +   if ($line =~ /\b(v|)snprintf\s*\(\s*/) {
> 
> Hi,
> 
> for my understanding, what is the purpose of the 2nd "\s*"?
> IMHO, it could be just removed.

It could.

# {v}snprintf uses that should likely be {v}scnprintf
if ($line =~ /\b((v?)snprintf)\s*\(/) {
WARN("SNPRINTF",
 "Prefer ${2}scnprintf over $1 - see: 
https://github.com/KSPP/linux/issues/105\n; . $herecurr);
}



Though I also think it's better to use lore rather than github

Re: [PATCH 1/4] gpu/drm: Add SPDX-license-Identifier tag

[Joe Perches]

On Tue, 2024-04-02 at 22:43 +, Nicolas Devos wrote:
> This commit fixes following checkpatch warning:
> WARNING: Missing or malformed SPDX-License-Identifier tag

NAK without specific agreement from Intel.

The existing license in the file is neither GPL nor MIT

> 
> Signed-off-by: Nicolas Devos 
> ---
>  drivers/gpu/drm/drm_connector.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b0516505f7ae..40350256b1f6 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
>  /*
>   * Copyright (c) 2016 Intel Corporation
>   *

Re: [PATCH v2] checkpatch: add check for snprintf to scnprintf

[Joe Perches]

On Fri, 2024-02-23 at 10:38 +, Lee Jones wrote:
> On Wed, 21 Feb 2024, Joe Perches wrote:
> 
> > On Wed, 2024-02-21 at 22:11 +, Justin Stitt wrote:
> > > I am going to quote Lee Jones who has been doing some snprintf ->
> > > scnprintf refactorings:
> > > 
> > > "There is a general misunderstanding amongst engineers that
> > > {v}snprintf() returns the length of the data *actually* encoded into the
> > > destination array.  However, as per the C99 standard {v}snprintf()
> > > really returns the length of the data that *would have been* written if
> > > there were enough space for it.  This misunderstanding has led to
> > > buffer-overruns in the past.  It's generally considered safer to use the
> > > {v}scnprintf() variants in their place (or even sprintf() in simple
> > > cases).  So let's do that."
> > > 
> > > To help prevent new instances of snprintf() from popping up, let's add a
> > > check to checkpatch.pl.
> > > 
> > > Suggested-by: Finn Thain 
> > > Signed-off-by: Justin Stitt 
> > > ---
> > > Changes in v2:
> > > - Had a vim moment and deleted a character before sending the patch.
> > > - Replaced the character :)
> > > - Link to v1: 
> > > https://lore.kernel.org/r/20240221-snprintf-checkpatch-v1-1-3ac5025b5...@google.com
> > > ---
> > > From a discussion here [1].
> > > 
> > > [1]: 
> > > https://lore.kernel.org/all/0f9c95f9-2c14-eee6-7faf-635880edc...@linux-m68k.org/
> > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -7012,6 +7012,12 @@ sub process {
> > >"Prefer strscpy, strscpy_pad, or __nonstring over 
> > > strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
> > >   }
> > >  
> > > +# snprintf uses that should likely be {v}scnprintf
> > > + if ($line =~ /\bsnprintf\s*\(\s*/) {
> > > + WARN("SNPRINTF",
> > > +  "Prefer scnprintf over snprintf\n" . 
> > > $herecurr);
> > 
> > There really should be some sort of reference link here
> > similar to the one above this.
> > 
> > Also, I rather doubt _all_ of these should be changed just
> > for churn's sake.
> 
> This is for new implementations only.
> 
> Kees is planning on changing all of the current instances kernel-wide.

I saw that.  I also saw pushback.
Not just my own.

Creating a cocci script is easy.
Getting Linus and others to run it isn't.

Re: [PATCH v2] checkpatch: add check for snprintf to scnprintf

[Joe Perches]

On Wed, 2024-02-21 at 22:11 +, Justin Stitt wrote:
> I am going to quote Lee Jones who has been doing some snprintf ->
> scnprintf refactorings:
> 
> "There is a general misunderstanding amongst engineers that
> {v}snprintf() returns the length of the data *actually* encoded into the
> destination array.  However, as per the C99 standard {v}snprintf()
> really returns the length of the data that *would have been* written if
> there were enough space for it.  This misunderstanding has led to
> buffer-overruns in the past.  It's generally considered safer to use the
> {v}scnprintf() variants in their place (or even sprintf() in simple
> cases).  So let's do that."
> 
> To help prevent new instances of snprintf() from popping up, let's add a
> check to checkpatch.pl.
> 
> Suggested-by: Finn Thain 
> Signed-off-by: Justin Stitt 
> ---
> Changes in v2:
> - Had a vim moment and deleted a character before sending the patch.
> - Replaced the character :)
> - Link to v1: 
> https://lore.kernel.org/r/20240221-snprintf-checkpatch-v1-1-3ac5025b5...@google.com
> ---
> From a discussion here [1].
> 
> [1]: 
> https://lore.kernel.org/all/0f9c95f9-2c14-eee6-7faf-635880edc...@linux-m68k.org/

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7012,6 +7012,12 @@ sub process {
>"Prefer strscpy, strscpy_pad, or __nonstring over 
> strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
>   }
>  
> +# snprintf uses that should likely be {v}scnprintf
> + if ($line =~ /\bsnprintf\s*\(\s*/) {
> + WARN("SNPRINTF",
> +  "Prefer scnprintf over snprintf\n" . 
> $herecurr);

There really should be some sort of reference link here
similar to the one above this.

Also, I rather doubt _all_ of these should be changed just
for churn's sake.

Maybe add a test for some return value use like

if (defined($stat) &&
$stat =~ /$Lval\s*=\s*snprintf\s*\(/) {
etc...

Maybe offer to --fix it too.

Re: .mailmap support for removals (was Re: [PATCH 00/10] scsi: Replace {v}snprintf() variants with safer alternatives)

[Joe Perches]

On Fri, 2024-02-09 at 22:56 -0800, Kees Cook wrote:
> On Thu, Feb 08, 2024 at 05:49:12PM +, Lee Jones wrote:
> > On Thu, 08 Feb 2024, Bart Van Assche wrote:
> > 
> > > On 2/8/24 00:44, Lee Jones wrote:
> > > > Cc: Andre Hedrick 
> > > 
> > > Please take a look at https://lwn.net/Articles/508222/.
> > 
> > get_maintainer.pl pulled it from here:
> > 
> > https://github.com/torvalds/linux/blob/master/drivers/scsi/3w-.c#L11
> 
> Oh. Hm. It seems "git check-mailmap" (and get_maintainers.pl) don't
> support a way to remove an email address -- only redirect it.
> 
> It seems we may want to support "don't use this email address" for more
> than just the currently observed rationale. I don't have any good
> suggestions for what the format should look like? Perhaps:
> 
> "" 

/dev/null ?

Re: [PATCH v7 3/9] drm/plane: Add drm_for_each_primary_visible_plane macro

[Joe Perches]

On Mon, 2024-01-08 at 11:24 +0100, Jocelyn Falempe wrote:
> Hi checkpatch maintainers,
> 
> This patch gives me the following checkpatch error:
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #30: FILE: include/drm/drm_plane.h:959:
> +#define drm_for_each_primary_visible_plane(plane, dev) \
> + list_for_each_entry((plane), &(dev)->mode_config.plane_list, head) \
> + for_each_if((plane)->type == DRM_PLANE_TYPE_PRIMARY && \
> + (plane)->state && \
> + (plane)->state->fb && \
> + (plane)->state->visible)
> 
> total: 1 errors, 0 warnings, 21 lines checked
> 
> I think this requirement cannot work when you use list_for_each kind of 
> macros.
> Do you have any suggestion ?
> 

checkpatch is a brainless regex script.
Ignore it when it's stupid.

Re: [PATCH v3] iio: sx9324: avoid copying property strings

[Joe Perches]

On Sun, 2023-12-17 at 13:24 +, Jonathan Cameron wrote:
> On Mon, 11 Dec 2023 22:30:12 -0800
> Joe Perches  wrote:
> 
> > On Tue, 2023-12-12 at 00:42 +, Justin Stitt wrote:
> > > We're doing some needless string copies when trying to assign the proper
> > > `prop` string. We can make `prop` a const char* and simply assign to
> > > string literals.  
> > 
> > trivia:
> > 
> > I would have updated it like this moving the
> > various declarations into the case blocks
> > where they are used and removing a few unused
> > #defines
> 
> I'd definitely like to see those defines gone.
> Arguably an unrelated change as I guess they are left from a previous refactor
> of this code.
> 
> Why prop to type renaming?

random, no specific need, though I prefer not reusing
identifiers with different types in separate local scopes.

>   It's getting passed into calls where the parameter
> is propname so I'd understand renaming to that, but type just seems a bit 
> random
> to me.  I do wonder if we are better off having some long lines and getting 
> rid
> of the property naming local variables completely by just duplicating
> the device_property_read_u32() call and passing them in directly.

maybe, give it a try and see what you think.

Re: [PATCH v3] iio: sx9324: avoid copying property strings

[Joe Perches]

On Tue, 2023-12-12 at 00:42 +, Justin Stitt wrote:
> We're doing some needless string copies when trying to assign the proper
> `prop` string. We can make `prop` a const char* and simply assign to
> string literals.

trivia:

I would have updated it like this moving the
various declarations into the case blocks
where they are used and removing a few unused
#defines

---
 drivers/iio/proximity/sx9324.c | 69 +-
 1 file changed, 41 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index ac2ed2da21ccc..c50c1108a69cc 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -877,17 +877,8 @@ static const struct sx_common_reg_default *
 sx9324_get_default_reg(struct device *dev, int idx,
   struct sx_common_reg_default *reg_def)
 {
-   static const char * const sx9324_rints[] = { "lowest", "low", "high",
-   "highest" };
-   static const char * const sx9324_csidle[] = { "hi-z", "hi-z", "gnd",
-   "vdd" };
-#define SX9324_PIN_DEF "semtech,ph0-pin"
-#define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
-#define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
-   unsigned int pin_defs[SX9324_NUM_PINS];
-   char prop[] = SX9324_PROXRAW_DEF;
-   u32 start = 0, raw = 0, pos = 0;
-   int ret, count, ph, pin;
+   u32 raw = 0;
+   int ret;
 
memcpy(reg_def, _default_regs[idx], sizeof(*reg_def));
 
@@ -896,7 +887,13 @@ sx9324_get_default_reg(struct device *dev, int idx,
case SX9324_REG_AFE_PH0:
case SX9324_REG_AFE_PH1:
case SX9324_REG_AFE_PH2:
-   case SX9324_REG_AFE_PH3:
+   case SX9324_REG_AFE_PH3: {
+   unsigned int pin_defs[SX9324_NUM_PINS];
+   int count;
+   int pin;
+   int ph;
+   char prop[32];
+
ph = reg_def->reg - SX9324_REG_AFE_PH0;
snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
 
@@ -913,7 +910,15 @@ sx9324_get_default_reg(struct device *dev, int idx,
   SX9324_REG_AFE_PH0_PIN_MASK(pin);
reg_def->def = raw;
break;
-   case SX9324_REG_AFE_CTRL0:
+   }
+   case SX9324_REG_AFE_CTRL0: {
+   static const char * const sx9324_csidle[] = {
+   "hi-z", "hi-z", "gnd", "vdd"
+   };
+   static const char * const sx9324_rints[] = {
+   "lowest", "low", "high", "highest"
+   };
+
ret = device_property_match_property_string(dev, 
"semtech,cs-idle-sleep",
sx9324_csidle,

ARRAY_SIZE(sx9324_csidle));
@@ -930,16 +935,17 @@ sx9324_get_default_reg(struct device *dev, int idx,
reg_def->def |= ret << SX9324_REG_AFE_CTRL0_RINT_SHIFT;
}
break;
+   }
case SX9324_REG_AFE_CTRL4:
-   case SX9324_REG_AFE_CTRL7:
+   case SX9324_REG_AFE_CTRL7: {
+   const char *type;
+
if (reg_def->reg == SX9324_REG_AFE_CTRL4)
-   strncpy(prop, "semtech,ph01-resolution",
-   ARRAY_SIZE(prop));
+   type = "semtech,ph01-resolution";
else
-   strncpy(prop, "semtech,ph23-resolution",
-   ARRAY_SIZE(prop));
+   type = "semtech,ph23-resolution";
 
-   ret = device_property_read_u32(dev, prop, );
+   ret = device_property_read_u32(dev, type, );
if (ret)
break;
 
@@ -949,6 +955,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
reg_def->def |= FIELD_PREP(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
   raw);
break;
+   }
case SX9324_REG_AFE_CTRL8:
ret = device_property_read_u32(dev,
"semtech,input-precharge-resistor-ohms",
@@ -982,17 +989,21 @@ sx9324_get_default_reg(struct device *dev, int idx,
   6 + raw * (raw + 3) / 2);
break;
 
-   case SX9324_REG_ADV_CTRL5:
+   case SX9324_REG_ADV_CTRL5: {
+   u32 start = 0;
+
ret = device_property_read_u32(dev, "semtech,startup-sensor",
   );
if (ret)
break;
-
reg_def->def &= ~SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK;
reg_def->def |= 
FIELD_PREP(SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK,
   start);
break;
-   case SX9324_REG_PROX_CTRL4:
+   }
+   case SX9324_REG_PROX_CTRL4: {
+  

Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[Joe Perches]

On Wed, 2023-12-06 at 18:23 +0200, Nikolai Kondrashov wrote:
> On 12/6/23 10:12, David Gow wrote:
> > I'm pretty happy with this personally, though I definitely think we
> > need the support for tests which aren't just executable scripts (e.g.
> > the docs in patch 6).
> > 
> > The get_maintailer.pl bits, and hence the requirement to not include
> > '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails
> > and tests separate by some other means (either having --test _only_
> > print tests, not emails at all, or by giving them a prefix like
> > 'TEST:' or something). But that is diverging more from the existing
> > behaviour of get_maintainer.pl, so I could go either way.
> > 
> > Otherwise, this looks pretty good. I'll give it a proper test tomorrow
> > alongside the other patches.
> 
> Thanks for the review, David!
> 
> Yeah, I don't like the '@' bit myself, but it seems to be the path of least 
> resistance right now (not necessarily the best one, of course).
> 
> I'm up for adding an option to get_maintainer.pl that disables email output, 
> if people like that, though.

That already exists though I don't understand the
specific requirement here

--nom --nol --nor

from
$ ./scripts/get_maintainer.pl --help
[]
--m => include maintainer(s) if any
--r => include reviewer(s) if any
--n => include name 'Full Name '
--l => include list(s) if any
[]
   Most options have both positive and negative forms.
  The negative forms for -- are --no and --no-.

Re: [RFC PATCH v2 04/10] docs: submitting-patches: Introduce Tested-with:

[Joe Perches]

On Tue, 2023-12-05 at 11:59 -0700, Jonathan Corbet wrote:
> Nikolai Kondrashov  writes:
> 
> > Introduce a new tag, 'Tested-with:', documented in the
> > Documentation/process/submitting-patches.rst file.
[]
> I have to ask whether we *really* need to introduce yet another tag for
> this.  How are we going to use this information?  Are we going to try to
> make a tag for every way in which somebody might test a patch?

In general, I think
Link: 
would be good enough.

And remember that all this goes stale after awhile
and that includes old test suites.

Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[Joe Perches]

On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote:
> Require the entry values to not contain the '@' character, so they could
> be distinguished from emails (always) output by get_maintainer.pl.

Why is this useful?
Why the need to distinguish?

Re: [RFC PATCH v2 01/10] get_maintainer: Survive querying missing files

[Joe Perches]

On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote:
> Do not die, but only warn when scripts/get_maintainer.pl is asked to
> retrieve information about a missing file.
> 
> This allows scripts/checkpatch.pl to query MAINTAINERS while processing
> patches which are removing files.

Why is this useful?

Give a for-instance example please.

Re: [PATCH v3 2/7] kexec_file: print out debugging message if required

[Joe Perches]

On Thu, 2023-11-30 at 10:39 +0800, Baoquan He wrote:
> Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.

trivia:

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
[]
> @@ -551,9 +551,12 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, 
> int need_kernel_map,
>   phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;
>   phdr->p_align = 0;
>   ehdr->e_phnum++;
> - pr_debug("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, 
> paddr=0x%llx, sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> - phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
> - ehdr->e_phnum, phdr->p_offset);
> +#ifdef CONFIG_KEXEC_FILE
> + kexec_dprintk("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, 
> paddr=0x%llx, "
> +   "sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> +   phdr, phdr->p_vaddr, phdr->p_paddr, 
> phdr->p_filesz,
> +   ehdr->e_phnum, phdr->p_offset);
> +#endif

Perhaps run checkpatch and coalesce the format string.


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v3 2/7] kexec_file: print out debugging message if required

[Joe Perches]

On Thu, 2023-11-30 at 10:39 +0800, Baoquan He wrote:
> Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.

trivia:

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
[]
> @@ -551,9 +551,12 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, 
> int need_kernel_map,
>   phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;
>   phdr->p_align = 0;
>   ehdr->e_phnum++;
> - pr_debug("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, 
> paddr=0x%llx, sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> - phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
> - ehdr->e_phnum, phdr->p_offset);
> +#ifdef CONFIG_KEXEC_FILE
> + kexec_dprintk("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, 
> paddr=0x%llx, "
> +   "sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> +   phdr, phdr->p_vaddr, phdr->p_paddr, 
> phdr->p_filesz,
> +   ehdr->e_phnum, phdr->p_offset);
> +#endif

Perhaps run checkpatch and coalesce the format string.

Re: [PATCH v2 2/7] kexec_file: print out debugging message if required

[Joe Perches]

On Fri, 2023-11-24 at 11:36 +0800, Baoquan He wrote:
> Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.

trivia for pr_debug -> kexec_dprintk conversions for
the entire patch set:

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
[]
> @@ -551,9 +551,12 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, 
> int need_kernel_map,
>   phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;
>   phdr->p_align = 0;
>   ehdr->e_phnum++;
> - pr_debug("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, 
> paddr=0x%llx, sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> +#ifdef CONFIG_KEXEC_FILE
> + kexec_dprintk("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, 
> paddr=0x%llx, "
> + "sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
>   phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
>   ehdr->e_phnum, phdr->p_offset);

It's good form to rewrap continuation lines to the open parenthesis

> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
[]
> @@ -389,11 +391,12 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, 
> initrd_fd,
>   if (ret)
>   goto out;
>  
> + kexec_dprintk("nr_segments = %lu\n", image->nr_segments);
>   for (i = 0; i < image->nr_segments; i++) {
>   struct kexec_segment *ksegment;
>  
>   ksegment = >segment[i];
> - pr_debug("Loading segment %d: buf=0x%p bufsz=0x%zx mem=0x%lx 
> memsz=0x%zx\n",
> + kexec_dprintk("segment[%d]: buf=0x%p bufsz=0x%zx mem=0x%lx 
> memsz=0x%zx\n",
>i, ksegment->buf, ksegment->bufsz, ksegment->mem,
>ksegment->memsz);

here too etc...


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 2/7] kexec_file: print out debugging message if required

[Joe Perches]

On Fri, 2023-11-24 at 11:36 +0800, Baoquan He wrote:
> Replace pr_debug() with the newly added kexec_dprintk() in kexec_file
> loading related codes.

trivia for pr_debug -> kexec_dprintk conversions for
the entire patch set:

> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
[]
> @@ -551,9 +551,12 @@ int crash_prepare_elf64_headers(struct crash_mem *mem, 
> int need_kernel_map,
>   phdr->p_filesz = phdr->p_memsz = mend - mstart + 1;
>   phdr->p_align = 0;
>   ehdr->e_phnum++;
> - pr_debug("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, 
> paddr=0x%llx, sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
> +#ifdef CONFIG_KEXEC_FILE
> + kexec_dprintk("Crash PT_LOAD ELF header. phdr=%p vaddr=0x%llx, 
> paddr=0x%llx, "
> + "sz=0x%llx e_phnum=%d p_offset=0x%llx\n",
>   phdr, phdr->p_vaddr, phdr->p_paddr, phdr->p_filesz,
>   ehdr->e_phnum, phdr->p_offset);

It's good form to rewrap continuation lines to the open parenthesis

> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
[]
> @@ -389,11 +391,12 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, 
> initrd_fd,
>   if (ret)
>   goto out;
>  
> + kexec_dprintk("nr_segments = %lu\n", image->nr_segments);
>   for (i = 0; i < image->nr_segments; i++) {
>   struct kexec_segment *ksegment;
>  
>   ksegment = >segment[i];
> - pr_debug("Loading segment %d: buf=0x%p bufsz=0x%zx mem=0x%lx 
> memsz=0x%zx\n",
> + kexec_dprintk("segment[%d]: buf=0x%p bufsz=0x%zx mem=0x%lx 
> memsz=0x%zx\n",
>i, ksegment->buf, ksegment->bufsz, ksegment->mem,
>ksegment->memsz);

here too etc...

Re: [PATCH v3 01/10] iov_iter: Fix some checkpatch complaints in kunit tests

[Joe Perches]

On Wed, 2023-11-15 at 15:49 +, David Howells wrote:
> Fix some checkpatch complaints in the new iov_iter kunit tests:
> 
>  (1) Some lines had eight spaces instead of a tab at the start.
> 
>  (2) Checkpatch doesn't like (void*)(unsigned long)0xnULL, so switch to
>  using POISON_POINTER_DELTA plus an offset instead.

That's because checkpatch is fundamentally stupid and
that's a false positive.

> diff --git a/lib/kunit_iov_iter.c b/lib/kunit_iov_iter.c
[]
> @@ -548,7 +548,7 @@ static void __init iov_kunit_extract_pages_kvec(struct 
> kunit *test)
>   size_t offset0 = LONG_MAX;
>  
>   for (i = 0; i < ARRAY_SIZE(pagelist); i++)
> - pagelist[i] = (void *)(unsigned 
> long)0xaa55aa55aa55aa55ULL;
> + pagelist[i] = (void *)POISON_POINTER_DELTA + 0x5a;

I think the original is easier to understand
or would best be replaced by a single #define
without the addition.

> @@ -626,7 +626,7 @@ static void __init iov_kunit_extract_pages_bvec(struct 
> kunit *test)
>   size_t offset0 = LONG_MAX;
>  
>   for (i = 0; i < ARRAY_SIZE(pagelist); i++)
> - pagelist[i] = (void *)(unsigned 
> long)0xaa55aa55aa55aa55ULL;
> + pagelist[i] = (void *)POISON_POINTER_DELTA + 0x5a;

etc...

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Joe Perches]

On Wed, 2023-11-15 at 19:43 +0200, Nikolai Kondrashov wrote:
> Introduce a new 'V:' ("Verify") field to MAINTAINERS. The field accepts
> a name of a test suite which is required to be executed for each
> contribution to the subsystem.

Perhaps this is simply too much overhead
process requirements for most kernel work.

While the addition of V: seems ok, I think
putting the verification in checkpatch is
odd at best and the verification of test
execution should be a separate script.

Re: [PATCH 1/7] kexec_file: add kexec_file flag to control debug printing

[Joe Perches]

On Tue, 2023-11-14 at 23:32 +0800, Baoquan He wrote:
> When specifying 'kexec -c -d', kexec_load interface will print loading
> information, e.g the regions where kernel/initrd/purgatory/cmdline
> are put, the memmap passed to 2nd kernel taken as system RAM ranges,
> and printing all contents of struct kexec_segment, etc. These are
> very helpful for analyzing or positioning what's happening when
> kexec/kdump itself failed. The debugging printing for kexec_load
> interface is made in user space utility kexec-tools.
> 
> Whereas, with kexec_file_load interface, 'kexec -s -d' print nothing.
> Because kexec_file code is mostly implemented in kernel space, and the
> debugging printing functionality is missed. It's not convenient when
> debugging kexec/kdump loading and jumping with kexec_file_load
> interface.
> 
> Now add KEXEC_FILE_DEBUG to kexec_file flag to control the debugging
> message printing. And add global variable kexec_file_dbg_print and
> macro kexec_dprintk() to facilitate the printing.
> 
> This is a preparation, later kexec_dprintk() will be used to replace the
> existing pr_debug(). Once 'kexec -s -d' is specified, it will print out
> kexec/kdump loading information. If '-d' is not specified, it regresses
> to pr_debug().

Not quite as pr_debug is completely eliminated with
zero object size when DEBUG is not #defined.

Now the object size will be larger and contain the
formats in .text.

[]
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
[]
> @@ -264,6 +264,18 @@ arch_kexec_apply_relocations(struct purgatory_info *pi, 
> Elf_Shdr *section,
>   return -ENOEXEC;
>  }
>  #endif
> +
> +extern bool kexec_file_dbg_print;
> +
> +#define kexec_dprintk(fmt, args...)  \
> + do {\
> + if (kexec_file_dbg_print)   \
> + printk(KERN_INFO fmt, ##args);  \
> + else\
> + printk(KERN_DEBUG fmt, ##args); \
> + } while (0)
> +
> +

I don't know how many of these printks exist and if
overall object size matters but using

#define kexec_dprintkfmt, ...)  \
printk("%s" fmt,\
   kexec_file_dbg_print ? KERN_INFO : KERN_DEBUG,   \
   ##__VA_ARGS__)

should reduce overall object size by eliminating the
mostly duplicated format in .text which differs only
by the KERN_



___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/7] kexec_file: add kexec_file flag to control debug printing

[Joe Perches]

On Tue, 2023-11-14 at 23:32 +0800, Baoquan He wrote:
> When specifying 'kexec -c -d', kexec_load interface will print loading
> information, e.g the regions where kernel/initrd/purgatory/cmdline
> are put, the memmap passed to 2nd kernel taken as system RAM ranges,
> and printing all contents of struct kexec_segment, etc. These are
> very helpful for analyzing or positioning what's happening when
> kexec/kdump itself failed. The debugging printing for kexec_load
> interface is made in user space utility kexec-tools.
> 
> Whereas, with kexec_file_load interface, 'kexec -s -d' print nothing.
> Because kexec_file code is mostly implemented in kernel space, and the
> debugging printing functionality is missed. It's not convenient when
> debugging kexec/kdump loading and jumping with kexec_file_load
> interface.
> 
> Now add KEXEC_FILE_DEBUG to kexec_file flag to control the debugging
> message printing. And add global variable kexec_file_dbg_print and
> macro kexec_dprintk() to facilitate the printing.
> 
> This is a preparation, later kexec_dprintk() will be used to replace the
> existing pr_debug(). Once 'kexec -s -d' is specified, it will print out
> kexec/kdump loading information. If '-d' is not specified, it regresses
> to pr_debug().

Not quite as pr_debug is completely eliminated with
zero object size when DEBUG is not #defined.

Now the object size will be larger and contain the
formats in .text.

[]
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
[]
> @@ -264,6 +264,18 @@ arch_kexec_apply_relocations(struct purgatory_info *pi, 
> Elf_Shdr *section,
>   return -ENOEXEC;
>  }
>  #endif
> +
> +extern bool kexec_file_dbg_print;
> +
> +#define kexec_dprintk(fmt, args...)  \
> + do {\
> + if (kexec_file_dbg_print)   \
> + printk(KERN_INFO fmt, ##args);  \
> + else\
> + printk(KERN_DEBUG fmt, ##args); \
> + } while (0)
> +
> +

I don't know how many of these printks exist and if
overall object size matters but using

#define kexec_dprintkfmt, ...)  \
printk("%s" fmt,\
   kexec_file_dbg_print ? KERN_INFO : KERN_DEBUG,   \
   ##__VA_ARGS__)

should reduce overall object size by eliminating the
mostly duplicated format in .text which differs only
by the KERN_

Re: [Intel-wired-lan] [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules

[Joe Perches]

On 2023-10-27 12:40, Justin Stitt wrote:


Yeah you can push it but it's not really a standalone so perhaps I'll
just steal the diff and
wrap into v3?


Fine by me.
No need for my sign off.
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules

[Joe Perches]

On 2023-10-27 12:40, Justin Stitt wrote:


Yeah you can push it but it's not really a standalone so perhaps I'll
just steal the diff and
wrap into v3?


Fine by me.
No need for my sign off.

Re: [Intel-wired-lan] [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules

[Joe Perches]

On Thu, 2023-10-26 at 21:56 +, Justin Stitt wrote:
> Add some warnings for using ethtool_sprintf() where a simple
> ethtool_puts() would suffice.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7011,6 +7011,25 @@ sub process {
>"Prefer strscpy, strscpy_pad, or __nonstring over 
> strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
>   }
>  
> +# ethtool_sprintf uses that should likely be ethtool_puts
> + if ($line =~ 
> /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
> + if(WARN("ETHTOOL_SPRINTF",
> +"Prefer ethtool_puts over ethtool_sprintf with only 
> two arguments\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/;
> +   }
> + }
> +
> + # use $rawline because $line loses %s via sanitization and thus 
> we can't match against it.
> + if ($rawline =~ 
> /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
> + if(WARN("ETHTOOL_SPRINTF",
> +"Prefer ethtool_puts over ethtool_sprintf with 
> standalone \"%s\" specifier\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ 
> s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/;

Thanks, but:

This fix wouldn't work if the first argument was itself a function
with multiple arguments.

And this is whitespace formatted incorrectly.

It:

o needs a space after if
o needs a space after the comma in the fix
o needs to use the appropriate amount and tabs for indentation
o needs alignment to open parentheses
o the backslashes are not required in the fix block

Do you want me to push what I wrote in the link below?
https://lore.kernel.org/lkml/7eec92d9e72d28e7b5202f41b02a383eb28ddd26.ca...@perches.com/

> +   }
> + }
> +
> +
>  # typecasts on min/max could be min_t/max_t
>   if ($perl_version_ok &&
>   defined $stat &&
> 

___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH next v2 2/3] checkpatch: add ethtool_sprintf rules

[Joe Perches]

On Thu, 2023-10-26 at 21:56 +, Justin Stitt wrote:
> Add some warnings for using ethtool_sprintf() where a simple
> ethtool_puts() would suffice.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7011,6 +7011,25 @@ sub process {
>"Prefer strscpy, strscpy_pad, or __nonstring over 
> strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
>   }
>  
> +# ethtool_sprintf uses that should likely be ethtool_puts
> + if ($line =~ 
> /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
> + if(WARN("ETHTOOL_SPRINTF",
> +"Prefer ethtool_puts over ethtool_sprintf with only 
> two arguments\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/ethtool_sprintf\s*\(/ethtool_puts\(/;
> +   }
> + }
> +
> + # use $rawline because $line loses %s via sanitization and thus 
> we can't match against it.
> + if ($rawline =~ 
> /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
> + if(WARN("ETHTOOL_SPRINTF",
> +"Prefer ethtool_puts over ethtool_sprintf with 
> standalone \"%s\" specifier\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ 
> s/ethtool_sprintf\s*\(\s*(.*?),.*?,(.*?)\)/ethtool_puts\($1,$2)/;

Thanks, but:

This fix wouldn't work if the first argument was itself a function
with multiple arguments.

And this is whitespace formatted incorrectly.

It:

o needs a space after if
o needs a space after the comma in the fix
o needs to use the appropriate amount and tabs for indentation
o needs alignment to open parentheses
o the backslashes are not required in the fix block

Do you want me to push what I wrote in the link below?
https://lore.kernel.org/lkml/7eec92d9e72d28e7b5202f41b02a383eb28ddd26.ca...@perches.com/

> +   }
> + }
> +
> +
>  # typecasts on min/max could be min_t/max_t
>   if ($perl_version_ok &&
>   defined $stat &&
> 

Re: [Intel-wired-lan] [PATCH 0/3] ethtool: Add ethtool_puts()

[Joe Perches]

On Thu, 2023-10-26 at 10:49 -0700, Kees Cook wrote:
> On Thu, Oct 26, 2023 at 09:33:17AM -0700, Joe Perches wrote:
> > On Thu, 2023-10-26 at 08:47 -0700, Kees Cook wrote:
> > > On Wed, Oct 25, 2023 at 11:40:31PM +, Justin Stitt wrote:
> > > > @replace_2_args@
> > > > identifier BUF;
> > > > expression VAR;
> > > > @@
> > > > 
> > > > -   ethtool_sprintf
> > > > +   ethtool_puts
> > > > (, VAR)
> > > 
> > > I think cocci will do a better job at line folding if we adjust this
> > > rule like I had to adjust the next rule: completely remove and re-add
> > > the arguments:
> > > 
> > > -   ethtool_sprintf(, VAR)
> > > +   ethtool_puts(, VAR)
> > > 
> > > Then I think the handful of weird line wraps in the treewide patch will
> > > go away.
> > > 
> > 
> > Perhaps this, but i believe spatch needs
> >  --max-width=80
> > to fill all 80 columns
> 
> Ah, yeah. Default is 78. Current coding style max is 100... I'll adjust
> my local wrappers.

Coding style max is still 80 with exceptions allowed to 100
not a generic use of 100.


___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH 0/3] ethtool: Add ethtool_puts()

[Joe Perches]

On Thu, 2023-10-26 at 10:49 -0700, Kees Cook wrote:
> On Thu, Oct 26, 2023 at 09:33:17AM -0700, Joe Perches wrote:
> > On Thu, 2023-10-26 at 08:47 -0700, Kees Cook wrote:
> > > On Wed, Oct 25, 2023 at 11:40:31PM +, Justin Stitt wrote:
> > > > @replace_2_args@
> > > > identifier BUF;
> > > > expression VAR;
> > > > @@
> > > > 
> > > > -   ethtool_sprintf
> > > > +   ethtool_puts
> > > > (, VAR)
> > > 
> > > I think cocci will do a better job at line folding if we adjust this
> > > rule like I had to adjust the next rule: completely remove and re-add
> > > the arguments:
> > > 
> > > -   ethtool_sprintf(, VAR)
> > > +   ethtool_puts(, VAR)
> > > 
> > > Then I think the handful of weird line wraps in the treewide patch will
> > > go away.
> > > 
> > 
> > Perhaps this, but i believe spatch needs
> >  --max-width=80
> > to fill all 80 columns
> 
> Ah, yeah. Default is 78. Current coding style max is 100... I'll adjust
> my local wrappers.

Coding style max is still 80 with exceptions allowed to 100
not a generic use of 100.

Re: [Intel-wired-lan] [PATCH 3/3] checkpatch: add ethtool_sprintf rules

[Joe Perches]

On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote:
> Add some warnings for using ethtool_sprintf() where a simple
> ethtool_puts() would suffice.

Hi again Justin.

After I read patch 1/3 I don't object at all.

spatch/cocci will always be a better option than checkpatch
for conversions like this because it's a proper grammar parser
and checkpatch is a stupid little perl script.

If you resubmit this please:


> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7020,6 +7020,19 @@ sub process {
>"Prefer strscpy, strscpy_pad, or __nonstring over 
> strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
>   }
>  
> +# ethtool_sprintf uses that should likely be ethtool_puts
> + if (   $line =~ 
> /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/   ) {
> + WARN("ETHTOOL_SPRINTF",
> +  "Prefer ethtool_puts over ethtool_sprintf with 
> only two arguments" . $herecurr);
> + }
> +
> + # use $rawline because $line loses %s via sanitization and thus 
> we can't match against it.
> + if (   $rawline =~ 
> /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/   ) {
> + WARN("ETHTOOL_SPRINTF2",
> +  "Prefer ethtool_puts over ethtool_sprintf with 
> standalone \"%s\" specifier" . $herecurr);
> + }

o remove the whitespace before and after the parentheses
o use the same type "ETHTOOL_SPRINTF" or maybe "PREFER_ETHTOOL_PUTS"
  for both warnings.
o Add a newline on the message output
o Add a --fix option

Something like:
---
 scripts/checkpatch.pl | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda1128..6924731110d87 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7011,6 +7011,25 @@ sub process {
 "Prefer strscpy, strscpy_pad, or __nonstring over 
strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
}
 
+# ethtool_sprintf uses that should likely be ethtool_puts
+   if ($line =~ 
/\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
+   if (WARN("PREFER_ETHTOOL_PUTS",
+"Prefer ethtool_puts over ethtool_sprintf with 
only two arguments\n" . $herecurr) &&
+   $fix) {
+   $fixed[$fixlinenr] =~ 
s/\bethtool_sprintf\s*\(\s*($FuncArg)\s*,\s*($FuncArg)/ethtool_puts($1, $7)/;
+   }
+   }
+
+   # use $rawline because $line loses %s via sanitization and thus 
we can't match against it.
+   if ($rawline =~ 
/\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
+   if (WARN("PREFER_ETHTOOL_PUTS",
+"Prefer ethtool_puts over ethtool_sprintf with 
standalone \"%s\" specifier\n" . $herecurr) &&
+   $fix) {
+   $fixed[$fixlinenr] =~ 
s/\bethtool_sprintf\s*\(\s*($FuncArg)\s*,\s*"\%s"\s*,\s*($FuncArg)/ethtool_puts($1,
 $7)/;
+   }
+   }
+
+
 # typecasts on min/max could be min_t/max_t
if ($perl_version_ok &&
defined $stat &&



___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH 3/3] checkpatch: add ethtool_sprintf rules

[Joe Perches]

On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote:
> Add some warnings for using ethtool_sprintf() where a simple
> ethtool_puts() would suffice.

Hi again Justin.

After I read patch 1/3 I don't object at all.

spatch/cocci will always be a better option than checkpatch
for conversions like this because it's a proper grammar parser
and checkpatch is a stupid little perl script.

If you resubmit this please:


> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7020,6 +7020,19 @@ sub process {
>"Prefer strscpy, strscpy_pad, or __nonstring over 
> strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
>   }
>  
> +# ethtool_sprintf uses that should likely be ethtool_puts
> + if (   $line =~ 
> /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/   ) {
> + WARN("ETHTOOL_SPRINTF",
> +  "Prefer ethtool_puts over ethtool_sprintf with 
> only two arguments" . $herecurr);
> + }
> +
> + # use $rawline because $line loses %s via sanitization and thus 
> we can't match against it.
> + if (   $rawline =~ 
> /\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/   ) {
> + WARN("ETHTOOL_SPRINTF2",
> +  "Prefer ethtool_puts over ethtool_sprintf with 
> standalone \"%s\" specifier" . $herecurr);
> + }

o remove the whitespace before and after the parentheses
o use the same type "ETHTOOL_SPRINTF" or maybe "PREFER_ETHTOOL_PUTS"
  for both warnings.
o Add a newline on the message output
o Add a --fix option

Something like:
---
 scripts/checkpatch.pl | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda1128..6924731110d87 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7011,6 +7011,25 @@ sub process {
 "Prefer strscpy, strscpy_pad, or __nonstring over 
strncpy - see: https://github.com/KSPP/linux/issues/90\n; . $herecurr);
}
 
+# ethtool_sprintf uses that should likely be ethtool_puts
+   if ($line =~ 
/\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\)/) {
+   if (WARN("PREFER_ETHTOOL_PUTS",
+"Prefer ethtool_puts over ethtool_sprintf with 
only two arguments\n" . $herecurr) &&
+   $fix) {
+   $fixed[$fixlinenr] =~ 
s/\bethtool_sprintf\s*\(\s*($FuncArg)\s*,\s*($FuncArg)/ethtool_puts($1, $7)/;
+   }
+   }
+
+   # use $rawline because $line loses %s via sanitization and thus 
we can't match against it.
+   if ($rawline =~ 
/\bethtool_sprintf\s*\(\s*$FuncArg\s*,\s*\"\%s\"\s*,\s*$FuncArg\s*\)/) {
+   if (WARN("PREFER_ETHTOOL_PUTS",
+"Prefer ethtool_puts over ethtool_sprintf with 
standalone \"%s\" specifier\n" . $herecurr) &&
+   $fix) {
+   $fixed[$fixlinenr] =~ 
s/\bethtool_sprintf\s*\(\s*($FuncArg)\s*,\s*"\%s"\s*,\s*($FuncArg)/ethtool_puts($1,
 $7)/;
+   }
+   }
+
+
 # typecasts on min/max could be min_t/max_t
if ($perl_version_ok &&
defined $stat &&

Re: [Intel-wired-lan] [PATCH 0/3] ethtool: Add ethtool_puts()

[Joe Perches]

On Thu, 2023-10-26 at 08:47 -0700, Kees Cook wrote:
> On Wed, Oct 25, 2023 at 11:40:31PM +, Justin Stitt wrote:
> > @replace_2_args@
> > identifier BUF;
> > expression VAR;
> > @@
> > 
> > -   ethtool_sprintf
> > +   ethtool_puts
> > (, VAR)
> 
> I think cocci will do a better job at line folding if we adjust this
> rule like I had to adjust the next rule: completely remove and re-add
> the arguments:
> 
> -   ethtool_sprintf(, VAR)
> +   ethtool_puts(, VAR)
> 
> Then I think the handful of weird line wraps in the treewide patch will
> go away.
> 

Perhaps this, but i believe spatch needs
 --max-width=80
to fill all 80 columns

$ cat ethtool_puts.cocci
@@
expression a, b;
@@

-   ethtool_sprintf(a, b)
+   ethtool_puts(a, b)

@@
expression a, b;
@@

-   ethtool_sprintf(a, "%s", b)
+   ethtool_puts(a, b)

$ spatch -sp-file ethtool_puts.cocci --in-place --max-width=80 drivers/net
$ git diff --stat -p drivers/net
 drivers/net/dsa/lantiq_gswip.c |  2 +-
 drivers/net/dsa/mt7530.c   |  2 +-
 drivers/net/dsa/qca/qca8k-common.c |  2 +-
 drivers/net/dsa/realtek/rtl8365mb.c|  2 +-
 drivers/net/dsa/realtek/rtl8366-core.c |  2 +-
 drivers/net/dsa/vitesse-vsc73xx-core.c |  8 +--
 drivers/net/ethernet/amazon/ena/ena_ethtool.c  |  4 +-
 drivers/net/ethernet/brocade/bna/bnad_ethtool.c|  2 +-
 drivers/net/ethernet/freescale/fec_main.c  |  4 +-
 .../net/ethernet/fungible/funeth/funeth_ethtool.c  |  8 +--
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c |  2 +-
 .../net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c|  2 +-
 drivers/net/ethernet/hisilicon/hns/hns_ethtool.c   | 65 +++---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  6 +-
 drivers/net/ethernet/intel/iavf/iavf_ethtool.c |  3 +-
 drivers/net/ethernet/intel/ice/ice_ethtool.c   |  9 +--
 drivers/net/ethernet/intel/idpf/idpf_ethtool.c |  2 +-
 drivers/net/ethernet/intel/igb/igb_ethtool.c   |  6 +-
 drivers/net/ethernet/intel/igc/igc_ethtool.c   |  6 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c   |  5 +-
 .../net/ethernet/microchip/sparx5/sparx5_ethtool.c |  2 +-
 .../net/ethernet/netronome/nfp/nfp_net_ethtool.c   | 44 +++
 drivers/net/ethernet/pensando/ionic/ionic_stats.c  |  4 +-
 drivers/net/ethernet/wangxun/libwx/wx_ethtool.c|  2 +-
 drivers/net/hyperv/netvsc_drv.c|  4 +-
 drivers/net/phy/nxp-tja11xx.c  |  2 +-
 drivers/net/phy/smsc.c |  2 +-
 drivers/net/vmxnet3/vmxnet3_ethtool.c  | 10 ++--
 28 files changed, 100 insertions(+), 112 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 1a2d5797bf98c..ff67bbf5a789b 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1759,7 +1759,7 @@ static void gswip_get_strings(struct dsa_switch *ds, int 
port, u32 stringset,
return;
 
for (i = 0; i < ARRAY_SIZE(gswip_rmon_cnt); i++)
-   ethtool_sprintf(, "%s", gswip_rmon_cnt[i].name);
+   ethtool_puts(, gswip_rmon_cnt[i].name);
 }
 
 static u32 gswip_bcm_ram_entry_read(struct gswip_priv *priv, u32 table,
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ecf5d3deb36eb..3c2cce442facf 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -836,7 +836,7 @@ mt7530_get_strings(struct dsa_switch *ds, int port, u32 
stringset,
return;
 
for (i = 0; i < ARRAY_SIZE(mt7530_mib); i++)
-   ethtool_sprintf(, "%s", mt7530_mib[i].name);
+   ethtool_puts(, mt7530_mib[i].name);
 }
 
 static void
diff --git a/drivers/net/dsa/qca/qca8k-common.c 
b/drivers/net/dsa/qca/qca8k-common.c
index 9ff0a3c1cb91d..94a949e27445f 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -487,7 +487,7 @@ void qca8k_get_strings(struct dsa_switch *ds, int port, u32 
stringset,
return;
 
for (i = 0; i < priv->info->mib_count; i++)
-   ethtool_sprintf(, "%s", ar8327_mib[i].name);
+   ethtool_puts(, ar8327_mib[i].name);
 }
 
 void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c 
b/drivers/net/dsa/realtek/rtl8365mb.c
index d171c18dd354c..ba0bafaca9aa9 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -1303,7 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, 
int port, u32 stringset
 
for (i = 0; i < RTL8365MB_MIB_END; i++) {
struct rtl8365mb_mib_counter *mib = _mib_counters[i];
-   ethtool_sprintf(, "%s", mib->name);
+   ethtool_puts(, mib->name);
}
 }
 
diff --git a/drivers/net/dsa/realtek/rtl8366-core.c 
b/drivers/net/dsa/realtek/rtl8366-core.c

Re: [PATCH 0/3] ethtool: Add ethtool_puts()

[Joe Perches]

On Thu, 2023-10-26 at 08:47 -0700, Kees Cook wrote:
> On Wed, Oct 25, 2023 at 11:40:31PM +, Justin Stitt wrote:
> > @replace_2_args@
> > identifier BUF;
> > expression VAR;
> > @@
> > 
> > -   ethtool_sprintf
> > +   ethtool_puts
> > (, VAR)
> 
> I think cocci will do a better job at line folding if we adjust this
> rule like I had to adjust the next rule: completely remove and re-add
> the arguments:
> 
> -   ethtool_sprintf(, VAR)
> +   ethtool_puts(, VAR)
> 
> Then I think the handful of weird line wraps in the treewide patch will
> go away.
> 

Perhaps this, but i believe spatch needs
 --max-width=80
to fill all 80 columns

$ cat ethtool_puts.cocci
@@
expression a, b;
@@

-   ethtool_sprintf(a, b)
+   ethtool_puts(a, b)

@@
expression a, b;
@@

-   ethtool_sprintf(a, "%s", b)
+   ethtool_puts(a, b)

$ spatch -sp-file ethtool_puts.cocci --in-place --max-width=80 drivers/net
$ git diff --stat -p drivers/net
 drivers/net/dsa/lantiq_gswip.c |  2 +-
 drivers/net/dsa/mt7530.c   |  2 +-
 drivers/net/dsa/qca/qca8k-common.c |  2 +-
 drivers/net/dsa/realtek/rtl8365mb.c|  2 +-
 drivers/net/dsa/realtek/rtl8366-core.c |  2 +-
 drivers/net/dsa/vitesse-vsc73xx-core.c |  8 +--
 drivers/net/ethernet/amazon/ena/ena_ethtool.c  |  4 +-
 drivers/net/ethernet/brocade/bna/bnad_ethtool.c|  2 +-
 drivers/net/ethernet/freescale/fec_main.c  |  4 +-
 .../net/ethernet/fungible/funeth/funeth_ethtool.c  |  8 +--
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c |  2 +-
 .../net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c|  2 +-
 drivers/net/ethernet/hisilicon/hns/hns_ethtool.c   | 65 +++---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c |  6 +-
 drivers/net/ethernet/intel/iavf/iavf_ethtool.c |  3 +-
 drivers/net/ethernet/intel/ice/ice_ethtool.c   |  9 +--
 drivers/net/ethernet/intel/idpf/idpf_ethtool.c |  2 +-
 drivers/net/ethernet/intel/igb/igb_ethtool.c   |  6 +-
 drivers/net/ethernet/intel/igc/igc_ethtool.c   |  6 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c   |  5 +-
 .../net/ethernet/microchip/sparx5/sparx5_ethtool.c |  2 +-
 .../net/ethernet/netronome/nfp/nfp_net_ethtool.c   | 44 +++
 drivers/net/ethernet/pensando/ionic/ionic_stats.c  |  4 +-
 drivers/net/ethernet/wangxun/libwx/wx_ethtool.c|  2 +-
 drivers/net/hyperv/netvsc_drv.c|  4 +-
 drivers/net/phy/nxp-tja11xx.c  |  2 +-
 drivers/net/phy/smsc.c |  2 +-
 drivers/net/vmxnet3/vmxnet3_ethtool.c  | 10 ++--
 28 files changed, 100 insertions(+), 112 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 1a2d5797bf98c..ff67bbf5a789b 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1759,7 +1759,7 @@ static void gswip_get_strings(struct dsa_switch *ds, int 
port, u32 stringset,
return;
 
for (i = 0; i < ARRAY_SIZE(gswip_rmon_cnt); i++)
-   ethtool_sprintf(, "%s", gswip_rmon_cnt[i].name);
+   ethtool_puts(, gswip_rmon_cnt[i].name);
 }
 
 static u32 gswip_bcm_ram_entry_read(struct gswip_priv *priv, u32 table,
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ecf5d3deb36eb..3c2cce442facf 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -836,7 +836,7 @@ mt7530_get_strings(struct dsa_switch *ds, int port, u32 
stringset,
return;
 
for (i = 0; i < ARRAY_SIZE(mt7530_mib); i++)
-   ethtool_sprintf(, "%s", mt7530_mib[i].name);
+   ethtool_puts(, mt7530_mib[i].name);
 }
 
 static void
diff --git a/drivers/net/dsa/qca/qca8k-common.c 
b/drivers/net/dsa/qca/qca8k-common.c
index 9ff0a3c1cb91d..94a949e27445f 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -487,7 +487,7 @@ void qca8k_get_strings(struct dsa_switch *ds, int port, u32 
stringset,
return;
 
for (i = 0; i < priv->info->mib_count; i++)
-   ethtool_sprintf(, "%s", ar8327_mib[i].name);
+   ethtool_puts(, ar8327_mib[i].name);
 }
 
 void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c 
b/drivers/net/dsa/realtek/rtl8365mb.c
index d171c18dd354c..ba0bafaca9aa9 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -1303,7 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, 
int port, u32 stringset
 
for (i = 0; i < RTL8365MB_MIB_END; i++) {
struct rtl8365mb_mib_counter *mib = _mib_counters[i];
-   ethtool_sprintf(, "%s", mib->name);
+   ethtool_puts(, mib->name);
}
 }
 
diff --git a/drivers/net/dsa/realtek/rtl8366-core.c 
b/drivers/net/dsa/realtek/rtl8366-core.c

Re: [PATCH 2/3] treewide: Convert some ethtool_sprintf() to ethtool_puts()

[Joe Perches]

On Thu, 2023-10-26 at 16:48 +0200, Louis Peens wrote:
> On Wed, Oct 25, 2023 at 11:40:33PM +, Justin Stitt wrote:
> > This patch converts some basic cases of ethtool_sprintf() to
> > ethtool_puts().

[30k of quoted content]

> Thanks Justin. From my perspective the series looks good, though I've
> not spent very long on it. For the nfp parts:
> 
> Acked-by: Louis Peens 

Re: [Intel-wired-lan] [PATCH 2/3] treewide: Convert some ethtool_sprintf() to ethtool_puts()

[Joe Perches]

On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote:
> This patch converts some basic cases of ethtool_sprintf() to
> ethtool_puts().
> 
> The conversions are used in cases where ethtool_sprintf() was being used
> with just two arguments:
> >   ethtool_sprintf(, buffer[i].name);

OK.

> or when it's used with format string: "%s"
> >   ethtool_sprintf(, "%s", buffer[i].name);
> > which both now become:
> >   ethtool_puts(, buffer[i].name);

Why do you want this conversion?
Is it not possible for .name to contain a formatting field?

___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH 2/3] treewide: Convert some ethtool_sprintf() to ethtool_puts()

[Joe Perches]

On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote:
> This patch converts some basic cases of ethtool_sprintf() to
> ethtool_puts().
> 
> The conversions are used in cases where ethtool_sprintf() was being used
> with just two arguments:
> >   ethtool_sprintf(, buffer[i].name);

OK.

> or when it's used with format string: "%s"
> >   ethtool_sprintf(, "%s", buffer[i].name);
> > which both now become:
> >   ethtool_puts(, buffer[i].name);

Why do you want this conversion?
Is it not possible for .name to contain a formatting field?

Re: [Intel-wired-lan] [PATCH 3/3] checkpatch: add ethtool_sprintf rules

[Joe Perches]

On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote:
> Add some warnings for using ethtool_sprintf() where a simple
> ethtool_puts() would suffice.
> 
> The two cases are:
> 
> 1) Use ethtool_sprintf() with just two arguments:
> >   ethtool_sprintf(, driver[i].name);

OK.

> or
> 2) Use ethtool_sprintf() with a standalone "%s" fmt string:
> >   ethtool_sprintf(, "%s", driver[i].name);

I'm rather doubt this is really desired or appropriate.


___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [PATCH 3/3] checkpatch: add ethtool_sprintf rules

[Joe Perches]

On Wed, 2023-10-25 at 23:40 +, Justin Stitt wrote:
> Add some warnings for using ethtool_sprintf() where a simple
> ethtool_puts() would suffice.
> 
> The two cases are:
> 
> 1) Use ethtool_sprintf() with just two arguments:
> >   ethtool_sprintf(, driver[i].name);

OK.

> or
> 2) Use ethtool_sprintf() with a standalone "%s" fmt string:
> >   ethtool_sprintf(, "%s", driver[i].name);

I'm rather doubt this is really desired or appropriate.

Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching

[Joe Perches]

On Fri, 2023-09-29 at 11:07 +0900, Justin Stitt wrote:
> On Fri, Sep 29, 2023 at 12:52 AM Nick Desaulniers
>  wrote:
> > 
> > On Wed, Sep 27, 2023 at 11:09 PM Joe Perches  wrote:
> > > 
> > > On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote:
> > > > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches  wrote:
> > > > > 
> > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> > > > > > Changes in v2:
> > > > > > - remove formatting pass (thanks Joe) (but seriously the formatting 
> > > > > > is
> > > > > >   bad, is there opportunity to get a formatting pass in here at some
> > > > > >   point?)
> > > > > 

LG G7 Battery Replacement | Guide | Is it actually a Samsung? I t
> > > > > Why?  What is it that makes you believe the formatting is bad?
> > > > > 
> > > > 
> > > > Investigating further, it looked especially bad in my editor. There is
> > > > a mixture of
> > > > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting 
> > > > this to
> > > > 8 is a whole lot better. But I still see some weird spacing
> > > > 
> > > 
> > > Yes, it's a bit odd indentation.
> > > It's emacs default perl format.
> > > 4 space indent with 8 space tabs, maximal tab fill.
> > > 
> > 
> > Oh! What?! That's the most surprising convention I've ever heard of
> > (after the GNU C coding style).  Yet another thing to hold against
> > perl I guess. :P
> > 
> > I have my editor setup to highlight tabs vs spaces via visual cues, so
> > that I don't mess up kernel coding style. (`git clang-format HEAD~`
> > after a commit helps).  scripts/get_maintainer.pl has some serious
> > inconsistencies to the point where I'm not sure what it should or was
> > meant to be.  Now that you mention it, I see it, and it does seem
> > consistent in that regard.
> > 
> > Justin, is your formatter configurable to match that convention?
> > Maybe it's still useful, as long as you configure it to stick to the
> > pre-existing convention.
> 
> Negative, all the perl formatters I've tried will convert everything to 
> spaces.
> The best I've seen is perltidy.
> 
> https://gist.github.com/JustinStitt/347385921c80a5212c2672075aa769b6

emacs with cperl mode works fine.

I don't know much about vim, but when I open this file in vim
it looks perfectly normal and it's apparently properly syntax
highlighted.

Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching

[Joe Perches]

On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote:
> On Thu, Sep 28, 2023 at 2:01 PM Joe Perches  wrote:
> > 
> > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> > > Changes in v2:
> > > - remove formatting pass (thanks Joe) (but seriously the formatting is
> > >   bad, is there opportunity to get a formatting pass in here at some
> > >   point?)
> > 
> > Why?  What is it that makes you believe the formatting is bad?
> > 
> 
> Investigating further, it looked especially bad in my editor. There is
> a mixture of
> tabs and spaces and my vim tabstop is set to 4 for pl files. Setting this to
> 8 is a whole lot better. But I still see some weird spacing
> 

Yes, it's a bit odd indentation.
It's emacs default perl format.
4 space indent with 8 space tabs, maximal tab fill.

Re: [PATCH v2 1/2] get_maintainer: add patch-only keyword-matching

[Joe Perches]

On Thu, 2023-09-28 at 14:03 +0900, Justin Stitt wrote:
> On Thu, Sep 28, 2023 at 1:46 PM Joe Perches  wrote:
> > 
> > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> > > Add the "D:" type which behaves the same as "K:" but will only match
> > > content present in a patch file.
[]
> > > My opinion: Nack.
> > 
> > I think something like this would be better
> > as it avoids duplication of K and D content.
> 
> If I understand correctly, this puts the onus on the get_maintainer users
> to select the right argument whereas adding "D:", albeit with some
> duplicate code, allows maintainers themselves to decide in exactly
> which context they receive mail.

Maybe, but I doubt it'll be significantly different.


> This could all be a moot point, though, as I believe Konstantin
> is trying to separate out the whole idea of a patch-sender needing
> to specify the recipients of a patch.

As I understand it, by using get_maintainer.

Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching

[Joe Perches]

On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> Changes in v2:
> - remove formatting pass (thanks Joe) (but seriously the formatting is
>   bad, is there opportunity to get a formatting pass in here at some
>   point?)

Why?  What is it that makes you believe the formatting is bad?

Re: [PATCH v2 2/2] MAINTAINERS: migrate some K to D

[Joe Perches]

On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> Let's get the ball rolling with some changes from K to D.
> 
> Ultimately, if it turns out that 100% of K users want to change to D
> then really the behavior of K could just be changed.

Given my suggestion to 1/2, this would be unnecessary.

> 
> Signed-off-by: Justin Stitt 
> Original-author: Kees Cook 
> ---
>  MAINTAINERS | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 94e431daa7c2..80ffdaa8f044 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5038,7 +5038,7 @@ F:  Documentation/kbuild/llvm.rst
>  F:   include/linux/compiler-clang.h
>  F:   scripts/Makefile.clang
>  F:   scripts/clang-tools/
> -K:   \b(?i:clang|llvm)\b
> +D:   \b(?i:clang|llvm)\b
>  
>  CLK API
>  M:   Russell King 
> @@ -8149,7 +8149,7 @@ F:  lib/strcat_kunit.c
>  F:   lib/strscpy_kunit.c
>  F:   lib/test_fortify/*
>  F:   scripts/test_fortify.sh
> -K:   \b__NO_FORTIFY\b
> +D:   \b__NO_FORTIFY\b
>  
>  FPGA DFL DRIVERS
>  M:   Wu Hao 
> @@ -11405,8 +11405,10 @@ F:   
> Documentation/ABI/testing/sysfs-kernel-warn_count
>  F:   include/linux/overflow.h
>  F:   include/linux/randomize_kstack.h
>  F:   mm/usercopy.c
> -K:   \b(add|choose)_random_kstack_offset\b
> -K:   \b__check_(object_size|heap_object)\b
> +D:   \b(add|choose)_random_kstack_offset\b
> +D:   \b__check_(object_size|heap_object)\b
> +D:   \b__counted_by\b
> +
>  
>  KERNEL JANITORS
>  L:   kernel-janit...@vger.kernel.org
> @@ -17290,7 +17292,7 @@ F:drivers/acpi/apei/erst.c
>  F:   drivers/firmware/efi/efi-pstore.c
>  F:   fs/pstore/
>  F:   include/linux/pstore*
> -K:   \b(pstore|ramoops)
> +D:   \b(pstore|ramoops)
>  
>  PTP HARDWARE CLOCK SUPPORT
>  M:   Richard Cochran 
> @@ -19231,8 +19233,8 @@ F:include/uapi/linux/seccomp.h
>  F:   kernel/seccomp.c
>  F:   tools/testing/selftests/kselftest_harness.h
>  F:   tools/testing/selftests/seccomp/*
> -K:   \bsecure_computing
> -K:   \bTIF_SECCOMP\b
> +D:   \bsecure_computing
> +D:   \bTIF_SECCOMP\b
>  
>  SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) Broadcom BRCMSTB DRIVER
>  M:   Kamal Dasu 
> 

Re: [PATCH v2 1/2] get_maintainer: add patch-only keyword-matching

[Joe Perches]

On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> Add the "D:" type which behaves the same as "K:" but will only match
> content present in a patch file.
> 
> To illustrate:
> 
> Imagine this entry in MAINTAINERS:
> 
> NEW REPUBLIC
> M: Han Solo 
> W: https://www.jointheresistance.org
> D: \bstrncpy\b
> 
> Our maintainer, Han, will only be added to the recipients if a patch
> file is passed to get_maintainer (like what b4 does):
> $ ./scripts/get_maintainer.pl 0004-some-change.patch
> 
> If the above patch has a `strncpy` present in the subject, commit log or
> diff then Han will be to/cc'd.
> 
> However, in the event of a file from the tree given like:
> $ ./scripts/get_maintainer.pl ./lib/string.c
> 
> Han will not be noisily to/cc'd (like a K: type would in this
> circumstance)
> 
> Signed-off-by: Justin Stitt 
> ---
>  MAINTAINERS   |  5 +
>  scripts/get_maintainer.pl | 12 ++--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..94e431daa7c2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -59,6 +59,11 @@ Descriptions of section entries and preferred order
> matches patches or files that contain one or more of the words
> printk, pr_info or pr_err
>  One regex pattern per line.  Multiple K: lines acceptable.
> +  D: *Diff content regex* (perl extended) pattern match that applies only to
> + patches and not entire files (e.g. when using the get_maintainers.pl
> + script).
> + Usage same as K:.
> +
>  
>  Maintainers List
>  
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index ab123b498fd9..a3e697926ddd 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -342,6 +342,7 @@ if ($tree && !top_of_kernel_tree($lk_path)) {
>  
>  my @typevalue = ();
>  my %keyword_hash;
> +my %patch_keyword_hash;
>  my @mfiles = ();
>  my @self_test_info = ();
>  
> @@ -369,8 +370,10 @@ sub read_maintainer_file {
>   $value =~ s@([^/])$@$1/@;
>   }
>   } elsif ($type eq "K") {
> - $keyword_hash{@typevalue} = $value;
> - }
> +  $keyword_hash{@typevalue} = $value;
> + } elsif ($type eq "D") {
> +  $patch_keyword_hash{@typevalue} = $value;
> +  }
>   push(@typevalue, "$type:$value");
>   } elsif (!(/^\s*$/ || /^\s*\#/)) {
>   push(@typevalue, $line);
> @@ -607,6 +610,11 @@ foreach my $file (@ARGV) {
>   push(@keyword_tvi, $line);
>   }
>   }
> +foreach my $line(keys %patch_keyword_hash) {
> +  if ($patch_line =~ m/${patch_prefix}$patch_keyword_hash{$line}/x) {
> +push(@keyword_tvi, $line);
> +  }
> +}
>   }
>   }
>   close($patch);
> 


My opinion: Nack.

I think something like this would be better
as it avoids duplication of K and D content.
---
 scripts/get_maintainer.pl | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ab123b498fd9..07e7d744cadb 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -57,6 +57,7 @@ my $subsystem = 0;
 my $status = 0;
 my $letters = "";
 my $keywords = 1;
+my $keywords_in_file = 0;
 my $sections = 0;
 my $email_file_emails = 0;
 my $from_filename = 0;
@@ -272,6 +273,7 @@ if (!GetOptions(
'letters=s' => \$letters,
'pattern-depth=i' => \$pattern_depth,
'k|keywords!' => \$keywords,
+   'kf|keywords-in-file!' => \$keywords_in_file,
'sections!' => \$sections,
'fe|file-emails!' => \$email_file_emails,
'f|file' => \$from_filename,
@@ -318,6 +320,7 @@ if ($sections || $letters ne "") {
 $subsystem = 0;
 $web = 0;
 $keywords = 0;
+$keywords_in_file = 0;
 $interactive = 0;
 } else {
 my $selections = $email + $scm + $status + $subsystem + $web;
@@ -548,16 +551,14 @@ foreach my $file (@ARGV) {
$file =~ s/^\Q${cur_path}\E//;  #strip any absolute path
$file =~ s/^\Q${lk_path}\E//;   #or the path to the lk tree
push(@files, $file);
-   if ($file ne "MAINTAINERS" && -f $file && $keywords) {
+   if ($file ne "MAINTAINERS" && -f $file && $keywords && 
$keywords_in_file) {
open(my $f, '<', $file)
or die "$P: Can't open $file: $!\n";
my $text = do { local($/) ; <$f> };
close($f);
-   if ($keywords) {
-   foreach my $line (keys %keyword_hash) {
-   if ($text =~ m/$keyword_hash{$line}/x) {
-   push(@keyword_tvi, $line);
-   }
+   foreach my $line (keys %keyword_hash) {
+   if ($text =~ m/$keyword_hash{$line}/x) {
+   push(@keyword_tvi, $line);
}
}
}
@@ 

Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Joe Perches]

On Wed, 2023-09-27 at 09:15 -0700, Kees Cook wrote:
> On Wed, Sep 27, 2023 at 03:19:16AM +, Justin Stitt wrote:
> > Add the "D:" type which behaves the same as "K:" but will only match
> > content present in a patch file.
> > 
> > To illustrate:
> > 
> > Imagine this entry in MAINTAINERS:
> > 
> > NEW REPUBLIC
> > M: Han Solo 
> > W: https://www.jointheresistance.org
> > D: \bstrncpy\b
> > 
> > Our maintainer, Han, will only be added to the recipients if a patch
> > file is passed to get_maintainer (like what b4 does):
> > $ ./scripts/get_maintainer.pl 0004-some-change.patch
> > 
> > If the above patch has a `strncpy` present in the subject, commit log or
> > diff then Han will be to/cc'd.
> > 
> > However, in the event of a file from the tree given like:
> > $ ./scripts/get_maintainer.pl ./lib/string.c
> > 
> > Han will not be noisily to/cc'd (like a K: type would in this
> > circumstance)
> > 
> > Note that folks really shouldn't be using get_maintainer on tree files
> > anyways [1].
> > 
> > [1]: https://lore.kernel.org/all/20230726151515.1650519-1-k...@kernel.org/
> 
> As Greg suggested, please drop the above paragraph and link. Then this
> looks good to me.
> 
> I would immediately want to send this patch too, so please feel free to
> add this to your series (and I bet many other hints on "git grep 'K:.\\b'"
> would want to switch from K: to D: too):
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -5057,7 +5057,7 @@ F:  Documentation/kbuild/llvm.rst
>  F:   include/linux/compiler-clang.h
>  F:   scripts/Makefile.clang
>  F:   scripts/clang-tools/
> -K:   \b(?i:clang|llvm)\b
> +D:   \b(?i:clang|llvm)\b

etc...

My assumption is that the K: --file use is just unnecessary
and it'd be better to only use the K: lookup on patches.

(and I've somehow stuffed up the receiving side of my
 email configuration so please ignore any emails to me
 that bounce for a while)

Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type

[Joe Perches]

On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote:
> Add the "D:" type which behaves the same as "K:" but will only match
> content present in a patch file.

Likely it'd be less aggravating just to document
that K: is only for patches and add a !$file test.

Re: [PATCH 2/3] get_maintainer: run perltidy

[Joe Perches]

On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote:
> I'm a first time contributor to get_maintainer.pl and the formatting is
> suspicious. I am not sure if there is a particular reason it is the way
> it is but I let my editor format it and submitted the diff here in this
> patch.

Capital NACK.  Completely unnecessary and adds no value.

Re: [PATCH 1/3] MAINTAINERS: add documentation for D:

[Joe Perches]

On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote:
> Document what "D:" does.
> 
> This is more or less the same as what "K:" does but only works for patch
> files.

Nack.  I'd rather just add a !$file test to K: patterns.

Re: [PATCH][next] checkpatch: add a couple new alloc functions to alloc with multiplies check

[Joe Perches]

On Tue, 2023-09-12 at 11:04 -0600, Gustavo A. R. Silva wrote:
> vmalloc() and vzalloc() functions have now 2-factor multiplication
> argument forms vmalloc_array() and vcalloc(), correspondingly.

> Add alloc-with-multiplies checks for these new functions.
> 
> Link: https://github.com/KSPP/linux/issues/342
> Signed-off-by: Gustavo A. R. Silva 
> ---
>  scripts/checkpatch.pl | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7d16f863edf1..45265d0eee1b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7207,17 +7207,19 @@ sub process {
>   "Prefer $3(sizeof(*$1)...) over $3($4...)\n" . 
> $herecurr);
>   }
>  
> -# check for (kv|k)[mz]alloc with multiplies that could be 
> kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
> +# check for (kv|k|v)[mz]alloc with multiplies that could be 
> kmalloc_array/kvmalloc_array/vmalloc_array/kvcalloc/kcalloc/vcalloc
>   if ($perl_version_ok &&
>   defined $stat &&
> - $stat =~ 
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
>  {
> + $stat =~ 
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,?/)
>  {
>   my $oldfunc = $3;
>   my $a1 = $4;
>   my $a2 = $10;
>   my $newfunc = "kmalloc_array";
>   $newfunc = "kvmalloc_array" if ($oldfunc eq "kvmalloc");
> + $newfunc = "vmalloc_array" if ($oldfunc eq "vmalloc");
>   $newfunc = "kvcalloc" if ($oldfunc eq "kvzalloc");
>   $newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> + $newfunc = "vcalloc" if ($oldfunc eq "vzalloc");
>   my $r1 = $a1;
>   my $r2 = $a2;
>   if ($a1 =~ /^sizeof\s*\S/) {
> @@ -7233,7 +7235,7 @@ sub process {
>"Prefer $newfunc over $oldfunc with 
> multiply\n" . $herectx) &&
>   $cnt == 1 &&
>   $fix) {
> - $fixed[$fixlinenr] =~ 
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
>  . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> + $fixed[$fixlinenr] =~ 
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
>  . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
>   }
>   }
>   }

Seems ok.  Dunno how many more of these there might be like
devm_ variants, but maybe it'd be better style to use a hash
with replacement instead of the repeated if block

Something like:
---
 scripts/checkpatch.pl | 26 --
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7d16f863edf1c..bacb63518be14 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -854,6 +854,23 @@ foreach my $entry (keys %deprecated_apis) {
 }
 $deprecated_apis_search = "(?:${deprecated_apis_search})";
 
+our %alloc_with_multiply_apis = (
+   "kmalloc"   => "kmalloc_array",
+   "kvmalloc"  => "kvmalloc_array",
+   "vmalloc"   => "vmalloc_array",
+   "kvzalloc"  => "kvcalloc",
+   "kzalloc"   => "kcalloc",
+   "vzalloc"   => "vcalloc",
+);
+
+#Create a search pattern for all these strings to speed up a loop below
+our $alloc_with_multiply_search = "";
+foreach my $entry (keys %alloc_with_multiply_apis) {
+   $alloc_with_multiply_search .= '|' if ($alloc_with_multiply_search ne 
"");
+   $alloc_with_multiply_search .= $entry;
+}
+$alloc_with_multiply_search = "(?:${alloc_with_multiply_search})";
+
 our $mode_perms_world_writable = qr{
S_IWUGO |
S_IWOTH |
@@ -7210,14 +7227,11 @@ sub process {
 # check for (kv|k)[mz]alloc with multiplies that could be 
kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
if ($perl_version_ok &&
defined $stat &&
-   $stat =~ 
/^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
 {
+   $stat =~ 
/^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*($alloc_with_multiply_search)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
 {
my $oldfunc = $3;
+   my $newfunc = $alloc_with_multiply_apis{$oldfunc};
my $a1 = $4;
my $a2 = $10;
-   my $newfunc = "kmalloc_array";
-   

Re: [PATCH v5 22/22] checkpatch: reword long-line warn about commit-msg

[Joe Perches]

On Tue, 2023-08-01 at 17:35 -0600, Jim Cromie wrote:
> Reword the warning to complain about line length 1st, since thats
> whats actually tested.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3272,7 +3272,7 @@ sub process {
>   # A Fixes:, link or signature tag line
> $commit_log_possible_stack_dump)) {
>   WARN("COMMIT_LOG_LONG_LINE",
> -  "Possible unwrapped commit description (prefer a 
> maximum 75 chars per line)\n" . $herecurr);
> +  "Prefer a maximum 75 chars per line (possible 
> unwrapped commit description?)\n" . $herecurr);
>   $commit_log_long_line = 1;
>   }

I don't think this adds any clarity.  Anyone else? 

Re: [PATCH v4 0/5] docs & checkpatch: allow Closes tags with links

[Joe Perches]

On Mon, 2023-04-03 at 18:23 +0200, Matthieu Baerts wrote:
> Since v6.3, checkpatch.pl now complains about the use of "Closes:" tags
> followed by a link [1]. It also complains if a "Reported-by:" tag is
> followed by a "Closes:" one [2].

All these patches seems sensible, thanks.

Assuming Linus approves the use of "Closes:"

Acked-by: Joe Perches 

> As detailed in the first patch, this "Closes:" tag is used for a bit of
> time, mainly by DRM and MPTCP subsystems. It is used by some bug
> trackers to automate the closure of issues when a patch is accepted.
> It is even planned to use this tag with bugzilla.kernel.org [3].
> 
> The first patch updates the documentation to explain what is this
> "Closes:" tag and how/when to use it. The second patch modifies
> checkpatch.pl to stop complaining about it.
> 
> The DRM maintainers and their mailing list have been added in Cc as they
> are probably interested by these two patches as well.
> 
> [1] 
> https://lore.kernel.org/all/3b036087d80b8c0e07a46a1dbaaf4ad0d018f8d5.1674217480.git.li...@leemhuis.info/
> [2] 
> https://lore.kernel.org/all/bb5dfd55ea2026303ab2296f4a6df3da7dd64006.1674217480.git.li...@leemhuis.info/
> [3] 
> https://lore.kernel.org/linux-doc/20230315181205.f3av7h6owqzzw64p@meerkat.local/
> 
> Signed-off-by: Matthieu Baerts 
> ---
> Note: After having re-read the comments from the v1, it is still unclear
> to me if this "Closes:" can be accepted or not. But because it seems
> that the future Bugzilla bot for kernel.org and regzbot would like to
> use it as well, I'm sending here new versions. I'm sorry if I
> misunderstood the comments from v1. Please tell me if I did.
> 
> Changes in v4:
> - Patches 1/5, 3/5 and 4/5 have been added to ask using the "Closes" tag
>   instead of the "Link" one for any bug reports. (Thorsten)
> - The Fixes tags have been removed from patch 4/5. (Joe)
> - The "Reported-by being followed by a link tag" check is now only
>   looking for the tag, not the URL which is done elsewhere in patch 5/5.
>   (Thorsten)
> - A new patch has been added to fix a small issues in checkpatch.pl when
>   checking if "Reported-by:" tag is on the last line.
> - Link to v3: 
> https://lore.kernel.org/r/20230314-doc-checkpatch-closes-tag-v3-0-d1bdcf31c...@tessares.net
> 
> Changes in v3:
> - Patch 1/4 now allow using the "Closes" tag with any kind of bug
>   reports, as long as the link is public. (Thorsten)
> - The former patch 2/2 has been split in two: first to use a list for
>   the different "link" tags (Joe). Then to allow the 'Closes' tag.
> - A new patch has been added to let checkpatch.pl checking if "Closes"
>   and "Links" are used with a URL.
> - Link to v2: 
> https://lore.kernel.org/r/20230314-doc-checkpatch-closes-tag-v2-0-f4a417861...@tessares.net
> 
> Changes in v2:
> - The text on patch 1/2 has been reworked thanks to Jon, Bagas and
>   Thorsten. See the individual changelog on the patch for more details.
> - Private bug trackers and invalid URLs are clearly marked as forbidden
>   to avoid being misused. (Linus)
> - Rebased on top of Linus' repo.
> - Link to v1: 
> https://lore.kernel.org/r/20230314-doc-checkpatch-closes-tag-v1-0-1b83072e9...@tessares.net
> 
> ---
> Matthieu Baerts (5):
>   docs: process: allow Closes tags with links
>   checkpatch: don't print the next line if not defined
>   checkpatch: use a list of "link" tags
>   checkpatch: allow Closes tags with links
>   checkpatch: check for misuse of the link tags
> 
>  Documentation/process/5.Posting.rst  | 22 ++
>  Documentation/process/submitting-patches.rst | 26 +++--
>  scripts/checkpatch.pl| 43 
> ++--
>  3 files changed, 70 insertions(+), 21 deletions(-)
> ---
> base-commit: 7e364e56293bb98cae1b55fd835f5991c4e96e7d
> change-id: 20230314-doc-checkpatch-closes-tag-1731b57556b1
> 
> Best regards,

Re: [PATCH v3 3/4] checkpatch: allow Closes tags with links

[Joe Perches]

On Thu, 2023-03-30 at 20:13 +0200, Matthieu Baerts wrote:
> As a follow-up of a previous patch modifying the documentation to
> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
> 
> checkpatch.pl now no longer complain when the "Closes:" tag is used by
> itself or after the "Reported-by:" tag.
> 
> Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
> Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by 
> Link:")

I don't think this _fixes_ anything.
I believe it's merely a new capability.

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/373
> Signed-off-by: Matthieu Baerts 
> ---
> v3:
>  - split into 2 patches: the previous one adds a list with all the
>"link" tags. This one only allows the "Closes" tag. (Joe Perches)
>  - "Closes" is no longer printed between parenthesis. (Thorsten
>Leemhuis)
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9d092ff4fc16..ca58c734ff22 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -620,7 +620,7 @@ our $signature_tags = qr{(?xi:
>   Cc:
>  )};
>  
> -our @link_tags = qw(Link);
> +our @link_tags = qw(Link Closes);
>  
>  #Create a search and print patterns for all these strings to be used 
> directly below
>  our $link_tags_search = "";
> 

Re: [PATCH v2 2/2] checkpatch: allow Closes tags with links

[Joe Perches]

On Fri, 2023-03-24 at 19:52 +0100, Matthieu Baerts wrote:
> As a follow-up of the previous patch modifying the documentation to
> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
> 
> checkpatch.pl now mentions the "Closes:" tag between brackets to express
> the fact it should be used only if it makes sense.
> 
> While at it, checkpatch.pl will not complain if the "Closes" tag is used
> with a "long" line, similar to what is done with the "Link" tag.
> 
> Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
> Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by 
> Link:")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/373
> Signed-off-by: Matthieu Baerts 
> ---
>  scripts/checkpatch.pl | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bd44d12965c9..d6376e0b68cc 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3158,14 +3158,14 @@ sub process {
>   }
>   }
>  
> -# check if Reported-by: is followed by a Link:
> +# check if Reported-by: is followed by a Link: (or Closes:) tag
>   if ($sign_off =~ /^reported(?:|-and-tested)-by:$/i) {
>   if (!defined $lines[$linenr]) {
>   WARN("BAD_REPORTED_BY_LINK",
> -  "Reported-by: should be 
> immediately followed by Link: to the report\n" . $herecurr . 
> $rawlines[$linenr] . "\n");
> - } elsif ($rawlines[$linenr] !~ 
> m{^link:\s*https?://}i) {
> +  "Reported-by: should be 
> immediately followed by Link: (or Closes:) to the report\n" . $herecurr . 
> $rawlines[$linenr] . "\n");
> + } elsif ($rawlines[$linenr] !~ 
> m{^(link|closes):\s*https?://}i) {

Please do not use an unnecessary capture group.

(?:link|closes)

And because it's somewhat likely that _more_ of these keywords
could be added, perhaps use some array like deprecated_apis


>   WARN("BAD_REPORTED_BY_LINK",
> -  "Reported-by: should be 
> immediately followed by Link: with a URL to the report\n" . $herecurr . 
> $rawlines[$linenr] . "\n");
> +  "Reported-by: should be 
> immediately followed by Link: (or Closes:) with a URL to the report\n" . 
> $herecurr . $rawlines[$linenr] . "\n");
>   }
>   }
>   }
> @@ -3250,8 +3250,8 @@ sub process {
>   # file delta changes
> $line =~ /^\s*(?:[\w\.\-\+]*\/)++[\w\.\-\+]+:/ ||
>   # filename then :
> -   $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i ||
> - # A Fixes: or Link: line or signature 
> tag line
> +   $line =~ /^\s*(?:Fixes:|Link:|Closes:|$signature_tags)/i 
> ||
> + # A Fixes:, Link:, Closes: or signature 
> tag line
> $commit_log_possible_stack_dump)) {
>   WARN("COMMIT_LOG_LONG_LINE",
>"Possible unwrapped commit description (prefer a 
> maximum 75 chars per line)\n" . $herecurr);
> @@ -3266,13 +3266,13 @@ sub process {
>  
>  # Check for odd tags before a URI/URL
>   if ($in_commit_log &&
> - $line =~ /^\s*(\w+):\s*http/ && $1 ne 'Link') {
> + $line =~ /^\s*(\w+):\s*http/ && $1 ne 'Link' && $1 ne 
> 'Closes') {
>   if ($1 =~ /^v(?:ersion)?\d+/i) {
>   WARN("COMMIT_LOG_VERSIONING",
>"Patch version information should be after 
> the --- line\n" . $herecurr);
>   } else {
>   WARN("COMMIT_LOG_USE_LINK",
> -  "Unknown link reference '$1:', use 'Link:' 
> instead\n" . $herecurr);
> +  "Unknown link reference '$1:', use 'Link:' 
> (or 'Closes:') instead\n" . $herecurr);
>   }
>   }
>  
> 

Re: [PATCH] drm/amd/display: Simplify same effect if/else blocks

[Joe Perches]

On Fri, 2023-03-03 at 15:35 -0500, Harry Wentland wrote:
> Actually I was wrong. Too many similar-looking snippets in this
> function made me look at the wrong thing. This change is fine and
> Reviewed-by: Harry Wentland 

Re: [PATCH] drm/amd/display: Simplify same effect if/else blocks

[Joe Perches]

On Fri, 2023-03-03 at 15:35 -0500, Harry Wentland wrote:
> Actually I was wrong. Too many similar-looking snippets in this
> function made me look at the wrong thing. This change is fine and
> Reviewed-by: Harry Wentland 

Re: [PATCH v2 09/10] powerpc: Use ppc_md_progress()

[Joe Perches]

On Sat, 2023-02-18 at 10:15 +0100, Christophe Leroy wrote:
> Many places have:
> 
>   if (ppc.md_progress)
>   ppc.md_progress();
> 
> Use ppc_md_progress() instead.
> 
> Note that checkpatch complains about using function names,
> but this is not a function taking format strings, so we
> leave the function names for now.

If you are changing almost all of these uses, why not
drop the unused 2nd argument 'hex' at the same time?

void ppc_printk_progress(char *s, unsigned short hex)
{
pr_info("%s\n", s);
}

> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
[]
> @@ -127,8 +127,7 @@ __setup("l3cr=", ppc_setup_l3cr);
>  static int __init ppc_init(void)
>  {
>   /* clear the progress line */
> - if (ppc_md.progress)
> - ppc_md.progress(" ", 0x);
> + ppc_md_progress(" ", 0x);

ppc_md_progress(" ");

Although this example seems especially useless.

> diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
[]
> @@ -347,7 +347,7 @@ void __init MMU_init_hw(void)
>   if (!mmu_has_feature(MMU_FTR_HPTE_TABLE))
>   return;
>  
> - if ( ppc_md.progress ) ppc_md.progress("hash:enter", 0x105);
> + ppc_md_progress("hash:enter", 0x105);

ppc_md_progress("hash:enter");

[]

> diff --git a/arch/powerpc/platforms/52xx/efika.c 
> b/arch/powerpc/platforms/52xx/efika.c
[]
> @@ -189,8 +189,7 @@ static void __init efika_setup_arch(void)
>   mpc52xx_pm_init();
>  #endif
>  
> - if (ppc_md.progress)
> - ppc_md.progress("Linux/PPC " UTS_RELEASE " running on Efika 
> ;-)\n", 0x0);
> + ppc_md_progress("Linux/PPC " UTS_RELEASE " running on Efika ;-)\n", 
> 0x0);

And perhaps remove the extra newlines here and other places as
it doesn't seem useful to have unnecessary blank lines in the log.

Re: [PATCH] drm/amd/display: Simplify same effect if/else blocks

[Joe Perches]

On Sun, 2023-01-15 at 15:30 +0530, Deepak R Varma wrote:
> The if / else block code has same effect irrespective of the logical
> evaluation.  Hence, simply the implementation by removing the unnecessary
> conditional evaluation. While at it, also fix the long line checkpatch
> complaint. Issue identified using cond_no_effect.cocci Coccinelle
> semantic patch script.
> 
> Signed-off-by: Deepak R Varma 
> ---
> Please note: The proposed change is compile tested only. If there are any
> inbuilt test cases that I should run for further verification, I will 
> appreciate
> guidance about it. Thank you.

Preface: I do not know the code.

Perhaps Rodrigo Siqueira made a copy/paste error submitting the code for
commit 9114b55fabae ("drm/amd/display: Fix SubVP control flow in the MPO 
context")
as the code prior to this change is identical.

Perhaps one of the false uses should be true or dependent on the
interdependent_update_lock state.

> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
> b/drivers/gpu/drm/amd/display/dc/core/dc.c
[]
> @@ -3470,14 +3470,9 @@ static void commit_planes_for_stream(struct dc *dc,
>   /* Since phantom pipe programming is moved to 
> post_unlock_program_front_end,
>* move the SubVP lock to after the phantom pipes have been 
> setup
>*/
> - if (should_lock_all_pipes && 
> dc->hwss.interdependent_update_lock) {
> - if (dc->hwss.subvp_pipe_control_lock)
> - dc->hwss.subvp_pipe_control_lock(dc, context, 
> false, should_lock_all_pipes, NULL, subvp_prev_use);
> - } else {
> - if (dc->hwss.subvp_pipe_control_lock)
> - dc->hwss.subvp_pipe_control_lock(dc, context, 
> false, should_lock_all_pipes, NULL, subvp_prev_use);
> - }
> -

Perhaps something like:

if (dc->hwss.subvp_pipe_control_lock)
dc->hwss.subvp_pipe_control_lock(dc, context,
 should_lock_all_pipes 
&&
 
dc->hwss.interdependent_update_lock,
 should_lock_all_pipes, 
NULL, subvp_prev_use);

> + if (dc->hwss.subvp_pipe_control_lock)
> + dc->hwss.subvp_pipe_control_lock(dc, context, false, 
> should_lock_all_pipes,
> +  NULL, subvp_prev_use);
>   return;
>   }
>  

Re: [PATCH] drm/amd/display: Simplify same effect if/else blocks

[Joe Perches]

On Sun, 2023-01-15 at 15:30 +0530, Deepak R Varma wrote:
> The if / else block code has same effect irrespective of the logical
> evaluation.  Hence, simply the implementation by removing the unnecessary
> conditional evaluation. While at it, also fix the long line checkpatch
> complaint. Issue identified using cond_no_effect.cocci Coccinelle
> semantic patch script.
> 
> Signed-off-by: Deepak R Varma 
> ---
> Please note: The proposed change is compile tested only. If there are any
> inbuilt test cases that I should run for further verification, I will 
> appreciate
> guidance about it. Thank you.

Preface: I do not know the code.

Perhaps Rodrigo Siqueira made a copy/paste error submitting the code for
commit 9114b55fabae ("drm/amd/display: Fix SubVP control flow in the MPO 
context")
as the code prior to this change is identical.

Perhaps one of the false uses should be true or dependent on the
interdependent_update_lock state.

> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
> b/drivers/gpu/drm/amd/display/dc/core/dc.c
[]
> @@ -3470,14 +3470,9 @@ static void commit_planes_for_stream(struct dc *dc,
>   /* Since phantom pipe programming is moved to 
> post_unlock_program_front_end,
>* move the SubVP lock to after the phantom pipes have been 
> setup
>*/
> - if (should_lock_all_pipes && 
> dc->hwss.interdependent_update_lock) {
> - if (dc->hwss.subvp_pipe_control_lock)
> - dc->hwss.subvp_pipe_control_lock(dc, context, 
> false, should_lock_all_pipes, NULL, subvp_prev_use);
> - } else {
> - if (dc->hwss.subvp_pipe_control_lock)
> - dc->hwss.subvp_pipe_control_lock(dc, context, 
> false, should_lock_all_pipes, NULL, subvp_prev_use);
> - }
> -

Perhaps something like:

if (dc->hwss.subvp_pipe_control_lock)
dc->hwss.subvp_pipe_control_lock(dc, context,
 should_lock_all_pipes 
&&
 
dc->hwss.interdependent_update_lock,
 should_lock_all_pipes, 
NULL, subvp_prev_use);

> + if (dc->hwss.subvp_pipe_control_lock)
> + dc->hwss.subvp_pipe_control_lock(dc, context, false, 
> should_lock_all_pipes,
> +  NULL, subvp_prev_use);
>   return;
>   }
>  

Re: [PATCH AUTOSEL 4.9 1/3] powerpc/selftests: Use timersub() for gettimeofday()

[Joe Perches]

On Fri, 2022-10-14 at 09:54 -0400, Sasha Levin wrote:
> From: ye xingchen 
> 
> [ Upstream commit c814bf958926ff45a9c1e899bd001006ab6cfbae ]
> 
> Use timersub() function to simplify the code.

Why should a code simplification be backported?

> 
> Reported-by: Zeal Robot 
> Signed-off-by: ye xingchen 
> Signed-off-by: Michael Ellerman 
> Link: https://lore.kernel.org/r/20220816105106.82666-1-ye.xingc...@zte.com.cn
> Signed-off-by: Sasha Levin 
> ---
>  tools/testing/selftests/powerpc/benchmarks/gettimeofday.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/benchmarks/gettimeofday.c 
> b/tools/testing/selftests/powerpc/benchmarks/gettimeofday.c
> index 3af3c21e8036..7f4bb84f1c9c 100644
> --- a/tools/testing/selftests/powerpc/benchmarks/gettimeofday.c
> +++ b/tools/testing/selftests/powerpc/benchmarks/gettimeofday.c
> @@ -12,7 +12,7 @@ static int test_gettimeofday(void)
>  {
>   int i;
>  
> - struct timeval tv_start, tv_end;
> + struct timeval tv_start, tv_end, tv_diff;
>  
>   gettimeofday(_start, NULL);
>  
> @@ -20,7 +20,9 @@ static int test_gettimeofday(void)
>   gettimeofday(_end, NULL);
>   }
>  
> - printf("time = %.6f\n", tv_end.tv_sec - tv_start.tv_sec + 
> (tv_end.tv_usec - tv_start.tv_usec) * 1e-6);
> + timersub(_start, _end, _diff);
> +
> + printf("time = %.6f\n", tv_diff.tv_sec + (tv_diff.tv_usec) * 1e-6);
>  
>   return 0;
>  }

Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible

[Joe Perches]

On Wed, 2022-10-12 at 21:29 +, David Laight wrote:
> From: Joe Perches
> > Sent: 12 October 2022 20:17
> > 
> > On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> > > The prandom_u32() function has been a deprecated inline wrapper around
> > > get_random_u32() for several releases now, and compiles down to the
> > > exact same code. Replace the deprecated wrapper with a direct call to
> > > the real function.
> > []
> > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c 
> > > b/drivers/infiniband/hw/cxgb4/cm.c
> > []
> > > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
> > >  >com.remote_addr;
> > >   int ret;
> > >   enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> > > - u32 isn = (prandom_u32() & ~7UL) - 1;
> > > + u32 isn = (get_random_u32() & ~7UL) - 1;
> > 
> > trivia:
> > 
> > There are somewhat odd size mismatches here.
> > 
> > I had to think a tiny bit if random() returned a value from 0 to 7
> > and was promoted to a 64 bit value then truncated to 32 bit.
> > 
> > Perhaps these would be clearer as ~7U and not ~7UL
> 
> That makes no difference - the compiler will generate the same code.

True, more or less.  It's more a question for the reader.

> The real question is WTF is the code doing?

True.

> The '& ~7u' clears the bottom 3 bits.
> The '- 1' then sets the bottom 3 bits and decrements the
> (random) high bits.

Right.

> So is the same as get_random_u32() | 7.

True, it's effectively the same as the upper 29 bits are random
anyway and the bottom 3 bits are always set.

> But I bet the coder had something else in mind.

Likely.

And it was also likely copy/pasted a few times.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [f2fs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible

[Joe Perches]

On Wed, 2022-10-12 at 21:29 +, David Laight wrote:
> From: Joe Perches
> > Sent: 12 October 2022 20:17
> > 
> > On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> > > The prandom_u32() function has been a deprecated inline wrapper around
> > > get_random_u32() for several releases now, and compiles down to the
> > > exact same code. Replace the deprecated wrapper with a direct call to
> > > the real function.
> > []
> > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c 
> > > b/drivers/infiniband/hw/cxgb4/cm.c
> > []
> > > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
> > >  >com.remote_addr;
> > >   int ret;
> > >   enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> > > - u32 isn = (prandom_u32() & ~7UL) - 1;
> > > + u32 isn = (get_random_u32() & ~7UL) - 1;
> > 
> > trivia:
> > 
> > There are somewhat odd size mismatches here.
> > 
> > I had to think a tiny bit if random() returned a value from 0 to 7
> > and was promoted to a 64 bit value then truncated to 32 bit.
> > 
> > Perhaps these would be clearer as ~7U and not ~7UL
> 
> That makes no difference - the compiler will generate the same code.

True, more or less.  It's more a question for the reader.

> The real question is WTF is the code doing?

True.

> The '& ~7u' clears the bottom 3 bits.
> The '- 1' then sets the bottom 3 bits and decrements the
> (random) high bits.

Right.

> So is the same as get_random_u32() | 7.

True, it's effectively the same as the upper 29 bits are random
anyway and the bottom 3 bits are always set.

> But I bet the coder had something else in mind.

Likely.

And it was also likely copy/pasted a few times.


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible

[Joe Perches]

On Wed, 2022-10-12 at 21:29 +, David Laight wrote:
> From: Joe Perches
> > Sent: 12 October 2022 20:17
> > 
> > On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> > > The prandom_u32() function has been a deprecated inline wrapper around
> > > get_random_u32() for several releases now, and compiles down to the
> > > exact same code. Replace the deprecated wrapper with a direct call to
> > > the real function.
> > []
> > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c 
> > > b/drivers/infiniband/hw/cxgb4/cm.c
> > []
> > > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
> > >  >com.remote_addr;
> > >   int ret;
> > >   enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> > > - u32 isn = (prandom_u32() & ~7UL) - 1;
> > > + u32 isn = (get_random_u32() & ~7UL) - 1;
> > 
> > trivia:
> > 
> > There are somewhat odd size mismatches here.
> > 
> > I had to think a tiny bit if random() returned a value from 0 to 7
> > and was promoted to a 64 bit value then truncated to 32 bit.
> > 
> > Perhaps these would be clearer as ~7U and not ~7UL
> 
> That makes no difference - the compiler will generate the same code.

True, more or less.  It's more a question for the reader.

> The real question is WTF is the code doing?

True.

> The '& ~7u' clears the bottom 3 bits.
> The '- 1' then sets the bottom 3 bits and decrements the
> (random) high bits.

Right.

> So is the same as get_random_u32() | 7.

True, it's effectively the same as the upper 29 bits are random
anyway and the bottom 3 bits are always set.

> But I bet the coder had something else in mind.

Likely.

And it was also likely copy/pasted a few times.

Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible

[Joe Perches]

On Wed, 2022-10-12 at 21:29 +, David Laight wrote:
> From: Joe Perches
> > Sent: 12 October 2022 20:17
> > 
> > On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> > > The prandom_u32() function has been a deprecated inline wrapper around
> > > get_random_u32() for several releases now, and compiles down to the
> > > exact same code. Replace the deprecated wrapper with a direct call to
> > > the real function.
> > []
> > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c 
> > > b/drivers/infiniband/hw/cxgb4/cm.c
> > []
> > > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
> > >  >com.remote_addr;
> > >   int ret;
> > >   enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> > > - u32 isn = (prandom_u32() & ~7UL) - 1;
> > > + u32 isn = (get_random_u32() & ~7UL) - 1;
> > 
> > trivia:
> > 
> > There are somewhat odd size mismatches here.
> > 
> > I had to think a tiny bit if random() returned a value from 0 to 7
> > and was promoted to a 64 bit value then truncated to 32 bit.
> > 
> > Perhaps these would be clearer as ~7U and not ~7UL
> 
> That makes no difference - the compiler will generate the same code.

True, more or less.  It's more a question for the reader.

> The real question is WTF is the code doing?

True.

> The '& ~7u' clears the bottom 3 bits.
> The '- 1' then sets the bottom 3 bits and decrements the
> (random) high bits.

Right.

> So is the same as get_random_u32() | 7.

True, it's effectively the same as the upper 29 bits are random
anyway and the bottom 3 bits are always set.

> But I bet the coder had something else in mind.

Likely.

And it was also likely copy/pasted a few times.

Re: [ovs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible

[Joe Perches]

On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> The prandom_u32() function has been a deprecated inline wrapper around
> get_random_u32() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function.
[]
> diff --git a/drivers/infiniband/hw/cxgb4/cm.c 
> b/drivers/infiniband/hw/cxgb4/cm.c
[]
> @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
>  >com.remote_addr;
>   int ret;
>   enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;

trivia:

There are somewhat odd size mismatches here.

I had to think a tiny bit if random() returned a value from 0 to 7
and was promoted to a 64 bit value then truncated to 32 bit.

Perhaps these would be clearer as ~7U and not ~7UL

>   struct net_device *netdev;
>   u64 params;
>  
> @@ -2469,7 +2469,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff 
> *skb,
>   }
>  
>   if (!is_t4(adapter_type)) {
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;

etc...

drivers/infiniband/hw/cxgb4/cm.c:   u32 isn = (prandom_u32() & ~7UL) - 1;
drivers/infiniband/hw/cxgb4/cm.c:   u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:rpl5->iss = 
cpu_to_be32((prandom_u32() & ~7UL) - 1);
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/target/iscsi/cxgbit/cxgbit_cm.c:rpl5->iss = 
cpu_to_be32((prandom_u32() & ~7UL) - 1);

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [f2fs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible

[Joe Perches]

On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> The prandom_u32() function has been a deprecated inline wrapper around
> get_random_u32() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function.
[]
> diff --git a/drivers/infiniband/hw/cxgb4/cm.c 
> b/drivers/infiniband/hw/cxgb4/cm.c
[]
> @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
>  >com.remote_addr;
>   int ret;
>   enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;

trivia:

There are somewhat odd size mismatches here.

I had to think a tiny bit if random() returned a value from 0 to 7
and was promoted to a 64 bit value then truncated to 32 bit.

Perhaps these would be clearer as ~7U and not ~7UL

>   struct net_device *netdev;
>   u64 params;
>  
> @@ -2469,7 +2469,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff 
> *skb,
>   }
>  
>   if (!is_t4(adapter_type)) {
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;

etc...

drivers/infiniband/hw/cxgb4/cm.c:   u32 isn = (prandom_u32() & ~7UL) - 1;
drivers/infiniband/hw/cxgb4/cm.c:   u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:rpl5->iss = 
cpu_to_be32((prandom_u32() & ~7UL) - 1);
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/target/iscsi/cxgbit/cxgbit_cm.c:rpl5->iss = 
cpu_to_be32((prandom_u32() & ~7UL) - 1);



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible

[Joe Perches]

On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> The prandom_u32() function has been a deprecated inline wrapper around
> get_random_u32() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function.
[]
> diff --git a/drivers/infiniband/hw/cxgb4/cm.c 
> b/drivers/infiniband/hw/cxgb4/cm.c
[]
> @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
>  >com.remote_addr;
>   int ret;
>   enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;

trivia:

There are somewhat odd size mismatches here.

I had to think a tiny bit if random() returned a value from 0 to 7
and was promoted to a 64 bit value then truncated to 32 bit.

Perhaps these would be clearer as ~7U and not ~7UL

>   struct net_device *netdev;
>   u64 params;
>  
> @@ -2469,7 +2469,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff 
> *skb,
>   }
>  
>   if (!is_t4(adapter_type)) {
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;

etc...

drivers/infiniband/hw/cxgb4/cm.c:   u32 isn = (prandom_u32() & ~7UL) - 1;
drivers/infiniband/hw/cxgb4/cm.c:   u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:rpl5->iss = 
cpu_to_be32((prandom_u32() & ~7UL) - 1);
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/target/iscsi/cxgbit/cxgbit_cm.c:rpl5->iss = 
cpu_to_be32((prandom_u32() & ~7UL) - 1);

Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible

[Joe Perches]

On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> The prandom_u32() function has been a deprecated inline wrapper around
> get_random_u32() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function.
[]
> diff --git a/drivers/infiniband/hw/cxgb4/cm.c 
> b/drivers/infiniband/hw/cxgb4/cm.c
[]
> @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
>  >com.remote_addr;
>   int ret;
>   enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;

trivia:

There are somewhat odd size mismatches here.

I had to think a tiny bit if random() returned a value from 0 to 7
and was promoted to a 64 bit value then truncated to 32 bit.

Perhaps these would be clearer as ~7U and not ~7UL

>   struct net_device *netdev;
>   u64 params;
>  
> @@ -2469,7 +2469,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff 
> *skb,
>   }
>  
>   if (!is_t4(adapter_type)) {
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;

etc...

drivers/infiniband/hw/cxgb4/cm.c:   u32 isn = (prandom_u32() & ~7UL) - 1;
drivers/infiniband/hw/cxgb4/cm.c:   u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:rpl5->iss = 
cpu_to_be32((prandom_u32() & ~7UL) - 1);
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() & 
~7UL) - 1;
drivers/target/iscsi/cxgbit/cxgbit_cm.c:rpl5->iss = 
cpu_to_be32((prandom_u32() & ~7UL) - 1);

Re: [PATCH v9 09/10] leds: flash: mt6370: Add MediaTek MT6370 flashlight support

[Joe Perches]

On Fri, 2022-09-23 at 14:34 -0700, Han Jingoo wrote:
> On Wed, Sep 21, 2022 Andy Shevchenko  wrote:
> > 
> > On Wed, Sep 21, 2022 at 4:48 AM ChiaEn Wu  wrote:
> > > On Sun, Sep 18, 2022 at 3:22 AM Han Jingoo  wrote:
> > > > On Mon, Aug 29, 2022 ChiaEn Wu  wrote:
> > 
> > > > > +#define MT6370_ITORCH_MIN_uA   25000
> > > > > +#define MT6370_ITORCH_STEP_uA  12500
> > > > > +#define MT6370_ITORCH_MAX_uA   40
> > > > > +#define MT6370_ITORCH_DOUBLE_MAX_uA80
> > > > > +#define MT6370_ISTRB_MIN_uA5
> > > > > +#define MT6370_ISTRB_STEP_uA   12500
> > > > > +#define MT6370_ISTRB_MAX_uA150
> > > > > +#define MT6370_ISTRB_DOUBLE_MAX_uA 300
> > > > 
> > > > Use upper letters as below:
> > 
> > For microseconds (and other -seconds) the common practice (I assume
> > historically) is to use upper letters, indeed. But for current it's
> > more natural to use small letters for unit multiplier as it's easier
> > to read and understand.

I think it's fine. see:

commit 22735ce857a2d9f4e6eec37c36be3fcf9d21d154
Author: Joe Perches 
Date:   Wed Jul 3 15:05:33 2013 -0700

checkpatch: ignore SI unit CamelCase variants like "_uV"

Many existing variable names use SI like variants that should be otherwise
obvious and acceptable.

Re: [PATCH v1 3/3] checkpatch: warn on usage of VM_BUG_ON() and other BUG variants

[Joe Perches]

On Thu, 2022-09-22 at 19:05 -0700, John Hubbard wrote:
> On 9/20/22 05:23, David Hildenbrand wrote:
> > checkpatch does not point out that VM_BUG_ON() and friends should be
> > avoided, however, Linus notes:
> > 
> > VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally
> > no different, the only difference is "we can make the code smaller
> > because these are less important". [1]
> > 
> > So let's warn on VM_BUG_ON() and other BUG variants as well. While at it,
> > make it clearer that the kernel really shouldn't be crashed.
> > 
> > As there are some subsystem BUG macros that actually don't end up crashing
> > the kernel -- for example, KVM_BUG_ON() -- exclude these manually.
> > 
> > [1] 
> > https://lore.kernel.org/r/CAHk-=wg40eazofo16eviaj7mfqdhz2gvebvfsmf6gyzsprj...@mail.gmail.com
[]
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > @@ -4695,12 +4695,12 @@ sub process {
> > }
> > }
> >  
> > -# avoid BUG() or BUG_ON()
> > -   if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
> > +# do not use BUG() or variants
> > +   if ($line =~ 
> > /\b(?!AA_|BUILD_|DCCP_|IDA_|KVM_|RWLOCK_|snd_|SPIN_)(?:[a-zA-Z_]*_)?BUG(?:_ON)?(?:_[A-Z_]+)?\s*\(/)
> >  {
> 
> Should this be a separate patch? Adding a bunch of exceptions to the BUG() 
> rules is 
> a separate and distinct thing from adding VM_BUG_ON() and other *BUG*() 
> variants to
> the mix.

Not in my opinion.

> > my $msg_level = \
> > $msg_level = \ if ($file);
> > &{$msg_level}("AVOID_BUG",
> > - "Avoid crashing the kernel - try using 
> > WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr);
> > + "Do not crash the kernel unless it is 
> > unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than 
> > BUG() or variants.\n" . $herecurr);
> 
> Here's a requested tweak, to clean up the output and fix punctuation:
> 
> "Avoid crashing the kernel--use WARN_ON_ONCE() plus recovery code (if 
> feasible) instead of BUG() or variants.\n" . $herecurr);

Fixing punctuation here would be removing the trailing period as checkpatch
only has periods for multi-sentence output messages.

And I think that "Do not crash" is a stronger statement than "Avoid crashing"
so I prefer the original suggestion but it's not a big deal either way.

Re: [PATCH] drm/print: cleanup coding style in drm_print.h

[Joe Perches]

On Mon, 2022-09-05 at 11:49 +0300, Jani Nikula wrote:
> On Mon, 05 Sep 2022, Jani Nikula  wrote:
> > On Mon, 05 Sep 2022, Jingyu Wang  wrote:
> > > Fix everything checkpatch.pl complained about in drm_print.h
> 
> [...]
> 
> > >  static inline void
> > > -drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va)
> > > +drm_vprintf(struct drm_printer *p, const char *fmt, va_list * va)
> > 
> > Checkpatch is just confused here. Look at all the other params, why
> > would you add an extra space here?
> 
> Joe, can you help me out here please, I can't figure out why checkpatch
> is complaining here:
> 
> include/drm/drm_print.h:106: CHECK:SPACING: spaces preferred around that '*' 
> (ctx:WxV)
> #106: FILE: include/drm/drm_print.h:106:
> +drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va)

checkpatch doesn't know what a va_list is.

It's similar to the FILE typedef that also causes this output.

A recent link to add an exception for 'FILE' to checkpatch:

https://lore.kernel.org/all/20220902111923.1488671-1-...@digikod.net/

Re: [RFC PATCH 28/30] Improved symbolic error names

[Joe Perches]

On Tue, 2022-08-30 at 14:49 -0700, Suren Baghdasaryan wrote:
> From: Kent Overstreet 
> 
> This patch adds per-error-site error codes, with error strings that
> include their file and line number.
> 
> To use, change code that returns an error, e.g.
> return -ENOMEM;
> to
> return -ERR(ENOMEM);
> 
> Then, errname() will return a string that includes the file and line
> number of the ERR() call, for example
> printk("Got error %s!\n", errname(err));
> will result in
> Got error ENOMEM at foo.c:1234

Why? Something wrong with just using %pe ?

printk("Got error %pe at %s:%d!\n", ERR_PTR(err), __FILE__, __LINE__);

Likely __FILE__ and __LINE__ aren't particularly useful.

And using ERR would add rather a lot of bloat as each codetag_error_code
struct would be unique.

+#define ERR(_err)  \
+({ \
+   static struct codetag_error_code\
+   __used  \
+   __section("error_code_tags")\
+   __aligned(8) e = {  \
+   .str= #_err " at " __FILE__ ":" __stringify(__LINE__),\
+   .err= _err, \
+   };  \
+   \
+   e.err;  \
+})

Re: [PATCH RFC 2/2] checkpatch: warn on usage of VM_BUG_ON() and friends

[Joe Perches]

On Wed, 2022-08-24 at 18:31 +0200, David Hildenbrand wrote:
> checkpatch does not point out that VM_BUG_ON() and friends should be
> avoided, however, Linus notes:
> 
> VM_BUG_ON() has the exact same semantics as BUG_ON. It is literally
> no different, the only difference is "we can make the code smaller
> because these are less important". [1]
> 
> So let's warn on VM_BUG_ON() and friends as well. While at it, make it
> clearer that the kernel really shouldn't be crashed.
> 
> Note that there are some other *_BUG_ON flavors, but they are not all
> bad: for example, KVM_BUG_ON() only triggers a WARN_ON_ONCE and then
> flags KVM as being buggy, so we'll not care about them for now here.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4695,12 +4695,12 @@ sub process {
>   }
>   }
>  
> -# avoid BUG() or BUG_ON()
> - if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
> +# do not use BUG(), BUG_ON(), VM_BUG_ON() and friends.
> + if ($line =~ /\b(?:BUG|BUG_ON|VM_BUG_ON|VM_BUG_ON_[A-Z]+)\b/) {

Perhaps better as something like the below to pick up more variants

if ($line =~ 
/\b(?!KVM_|BUILD_)(?:[A-Z_]*_)?BUG(?:_ON)?(?:_[A-Z_]+)?\s*\(/

>   my $msg_level = \
>   $msg_level = \ if ($file);
>   &{$msg_level}("AVOID_BUG",
> -   "Avoid crashing the kernel - try using 
> WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr);

and maybe:

  "Do not crash the kernel unless it is 
unavoidable - use WARN_ON_ONCE & recovery code (if reasonable) rather than 
BUG() or variants\n" . $herecurr);


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [f2fs-dev] [PATCH -next] f2fs: Replace kmalloc() with f2fs_kmalloc

[Joe Perches]

On Mon, 2022-08-01 at 11:23 -0700, Christoph Hellwig wrote:
> On Mon, Aug 01, 2022 at 05:22:02PM +0800, studentxs...@163.com wrote:
> > From: Xie Shaowen 
> > 
> > replace kmalloc with f2fs_kmalloc to keep f2fs code consistency.
> 
> For that removing f2fs_kmalloc entirely would be way better.

Dunno, maybe doubtful as there's a specific "fault injector" test
built around f2fs_alloc. (CONFIG_F2FS_FAULT_INJECTION)

For a student lesson, it would significantly better to compile any
patch, especially to avoid broken patches, before submitting them.




___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] VMCI: Update maintainers for VMCI

[Joe Perches]

On Tue, 2022-07-26 at 15:55 +, Vishnu Dasa wrote:
> > On Jul 26, 2022, at 8:10 AM, Greg KH  > On Mon, Jul 25, 2022 at 06:29:25PM +, Vishnu Dasa wrote:
> > > > On Jul 25, 2022, at 11:05 AM, Greg KH  
> > > > wrote:
> > > > On Mon, Jul 25, 2022 at 09:32:46AM -0700, vd...@vmware.com wrote:
> > > > > From: Vishnu Dasa 
> > > > > Remove Rajesh as a maintainer for the VMCI driver.
> > > > Why?
> > > Rajesh is no longer with VMware and won't be working on VMCI.
> > 
> > But employment does not matter for maintainership and has nothing to do
> > with it.  Maintainership follows the person, not the company, you all
> > know this.
> > 
> > So for obvious reasons, I can't take this type of change without
> > Rajesh acking it.
> 
> I understand.  After getting in touch with Rajesh, cc'ing him via his
> personal email ID.
> 
> Rajesh, could you please provide your ack if you agree with this patch to
> remove you as the maintainer for VMCI?

If being an employee of a company is a criteria for maintainership
of this subsystem, likely the subsystem entry should be:

"S: Supported" not "S:  Maintained"

MAINTAINERS:VMWARE VMCI DRIVER
MAINTAINERS-M:  Bryan Tan 
MAINTAINERS-M:  Rajesh Jalisatgi 
MAINTAINERS-M:  Vishnu Dasa 
MAINTAINERS-R:  VMware PV-Drivers Reviewers 
MAINTAINERS-L:  linux-ker...@vger.kernel.org
MAINTAINERS-S:  Maintained

Likely that's true for every VMware entry.

MAINTAINERS:VMWARE BALLOON DRIVER
MAINTAINERS-M:  Nadav Amit 
MAINTAINERS-R:  VMware PV-Drivers Reviewers 
MAINTAINERS-L:  linux-ker...@vger.kernel.org
MAINTAINERS-S:  Maintained
MAINTAINERS-F:  drivers/misc/vmw_balloon.c
MAINTAINERS-
MAINTAINERS:VMWARE PVRDMA DRIVER
MAINTAINERS-M:  Bryan Tan 
MAINTAINERS-M:  Vishnu Dasa 
MAINTAINERS-R:  VMware PV-Drivers Reviewers 
MAINTAINERS-L:  linux-r...@vger.kernel.org
MAINTAINERS-S:  Maintained
MAINTAINERS-F:  drivers/infiniband/hw/vmw_pvrdma/
MAINTAINERS-
MAINTAINERS-VMware PVSCSI driver
MAINTAINERS-M:  Vishal Bhakta 
MAINTAINERS-R:  VMware PV-Drivers Reviewers 
MAINTAINERS-L:  linux-s...@vger.kernel.org
MAINTAINERS-S:  Maintained
MAINTAINERS-F:  drivers/scsi/vmw_pvscsi.c
MAINTAINERS-F:  drivers/scsi/vmw_pvscsi.h
MAINTAINERS-
MAINTAINERS:VMWARE VMMOUSE SUBDRIVER
MAINTAINERS-M:  Zack Rusin 
MAINTAINERS-R:  VMware Graphics Reviewers 
MAINTAINERS-R:  VMware PV-Drivers Reviewers 
MAINTAINERS-L:  linux-in...@vger.kernel.org
MAINTAINERS-S:  Maintained
MAINTAINERS-F:  drivers/input/mouse/vmmouse.c
MAINTAINERS-F:  drivers/input/mouse/vmmouse.h
MAINTAINERS-
MAINTAINERS:VMWARE VMXNET3 ETHERNET DRIVER
MAINTAINERS-M:  Ronak Doshi 
MAINTAINERS-R:  VMware PV-Drivers Reviewers 
MAINTAINERS-L:  net...@vger.kernel.org
MAINTAINERS-S:  Maintained
MAINTAINERS-F:  drivers/net/vmxnet3/
MAINTAINERS-

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2] docs: Fix typo in comment

[Joe Perches]

On Mon, 2022-07-25 at 09:57 -0400, William Breathitt Gray wrote:
> On Mon, Jul 25, 2022 at 06:52:15AM -0700, Joe Perches wrote:
> > On Fri, 2022-07-22 at 07:45 +0800, Baoquan He wrote:
> > > The fix is done with below command:
> > > sed -i "s/the the /the /g" `git grep -l "the the " Documentation`
> > 
> > This command misses entries at EOL:
> > 
> > Documentation/trace/histogram.rst:  Here's an example where we use a 
> > compound key composed of the the
> > 
> > Perhaps a better conversion would be 's/\bthe the\b/the/g'
> 
> It would be good to check for instances that cross newlines as well;
> i.e. "the" at the end of a line followed by "the" at the start of the
> next line. However, this would require some thought to properly account
> for comment blocks ("*") and other similar prefixes that should be
> ignored.

checkpatch already attempts that duplicated word across a newline test.


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [Linux-cachefs] [PATCH v2] docs: Fix typo in comment

[Joe Perches]

On Mon, 2022-07-25 at 09:57 -0400, William Breathitt Gray wrote:
> On Mon, Jul 25, 2022 at 06:52:15AM -0700, Joe Perches wrote:
> > On Fri, 2022-07-22 at 07:45 +0800, Baoquan He wrote:
> > > The fix is done with below command:
> > > sed -i "s/the the /the /g" `git grep -l "the the " Documentation`
> > 
> > This command misses entries at EOL:
> > 
> > Documentation/trace/histogram.rst:  Here's an example where we use a 
> > compound key composed of the the
> > 
> > Perhaps a better conversion would be 's/\bthe the\b/the/g'
> 
> It would be good to check for instances that cross newlines as well;
> i.e. "the" at the end of a line followed by "the" at the start of the
> next line. However, this would require some thought to properly account
> for comment blocks ("*") and other similar prefixes that should be
> ignored.

checkpatch already attempts that duplicated word across a newline test.

--
Linux-cachefs mailing list
Linux-cachefs@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-cachefs

Re: [Linux-cachefs] [PATCH v2] docs: Fix typo in comment

[Joe Perches]

On Fri, 2022-07-22 at 07:45 +0800, Baoquan He wrote:
> On 07/21/22 at 11:40am, Randy Dunlap wrote:
> > On 7/21/22 11:36, Jonathan Corbet wrote:
> > > "Slark Xiao"  writes:
> > > > May I know the maintainer of one subsystem could merge the changes
> > > > contains lots of subsystem?  I also know this could be filtered by
> > > > grep and sed command, but that patch would have dozens of maintainers
> > > > and reviewers.
> > > 
> > > Certainly I don't think I can merge a patch touching 166 files across
> > > the tree.  This will need to be broken down by subsystem, and you may
> > > well find that there are some maintainers who don't want to deal with
> > > this type of minor fix.
> > 
> > We have also seen cases where "the the" should be replaced by "then the"
> > or some other pair of words, so some of these changes could fall into
> > that category.
> 
> It's possible. I searched in Documentation and went through each place,
> seems no typo of "then the". Below patch should clean up all the 'the the'
> typo under Documentation.
[]
> The fix is done with below command:
> sed -i "s/the the /the /g" `git grep -l "the the " Documentation`

This command misses entries at EOL:

Documentation/trace/histogram.rst:  Here's an example where we use a compound 
key composed of the the

Perhaps a better conversion would be 's/\bthe the\b/the/g'

--
Linux-cachefs mailing list
Linux-cachefs@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-cachefs

Re: [PATCH v2] docs: Fix typo in comment

[Joe Perches]

On Fri, 2022-07-22 at 07:45 +0800, Baoquan He wrote:
> On 07/21/22 at 11:40am, Randy Dunlap wrote:
> > On 7/21/22 11:36, Jonathan Corbet wrote:
> > > "Slark Xiao"  writes:
> > > > May I know the maintainer of one subsystem could merge the changes
> > > > contains lots of subsystem?  I also know this could be filtered by
> > > > grep and sed command, but that patch would have dozens of maintainers
> > > > and reviewers.
> > > 
> > > Certainly I don't think I can merge a patch touching 166 files across
> > > the tree.  This will need to be broken down by subsystem, and you may
> > > well find that there are some maintainers who don't want to deal with
> > > this type of minor fix.
> > 
> > We have also seen cases where "the the" should be replaced by "then the"
> > or some other pair of words, so some of these changes could fall into
> > that category.
> 
> It's possible. I searched in Documentation and went through each place,
> seems no typo of "then the". Below patch should clean up all the 'the the'
> typo under Documentation.
[]
> The fix is done with below command:
> sed -i "s/the the /the /g" `git grep -l "the the " Documentation`

This command misses entries at EOL:

Documentation/trace/histogram.rst:  Here's an example where we use a compound 
key composed of the the

Perhaps a better conversion would be 's/\bthe the\b/the/g'


___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v2 24/39] drm/i915: dvo_sil164.c: use SPDX header

[Joe Perches]

On Fri, 2022-07-15 at 17:35 -0400, Rodrigo Vivi wrote:
> On Wed, Jul 13, 2022 at 09:12:12AM +0100, Mauro Carvalho Chehab wrote:
> > This file is licensed with MIT license. Change its license text
> > to use SPDX.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> 
> Reviewed-by: Rodrigo Vivi 

Not exactly the MIT license as it's missing "or copyright holders"
> 
> > ---
> > 
> > To avoid mailbombing on a large number of people, only mailing lists were 
> > C/C on the cover.
> > See [PATCH v2 00/39] at: 
> > https://lore.kernel.org/all/cover.1657699522.git.mche...@kernel.org/
> > 
> >  drivers/gpu/drm/i915/display/dvo_sil164.c | 32 +--
> >  1 file changed, 6 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/dvo_sil164.c 
> > b/drivers/gpu/drm/i915/display/dvo_sil164.c
> > index 0dfa0a0209ff..12974f7c9dc1 100644
> > --- a/drivers/gpu/drm/i915/display/dvo_sil164.c
> > +++ b/drivers/gpu/drm/i915/display/dvo_sil164.c
> > @@ -1,30 +1,10 @@
> > -/**
> > +// SPDX-License-Identifier: MIT
> >  
> > -Copyright © 2006 Dave Airlie
> > -
> > -All Rights Reserved.
> > -
> > -Permission is hereby granted, free of charge, to any person obtaining a
> > -copy of this software and associated documentation files (the
> > -"Software"), to deal in the Software without restriction, including
> > -without limitation the rights to use, copy, modify, merge, publish,
> > -distribute, sub license, and/or sell copies of the Software, and to
> > -permit persons to whom the Software is furnished to do so, subject to
> > -the following conditions:
> > -
> > -The above copyright notice and this permission notice (including the
> > -next paragraph) shall be included in all copies or substantial portions
> > -of the Software.
> > -
> > -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > -OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > -MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> > -IN NO EVENT SHALL THE AUTHOR

Missing "Authors or copyright holders"

> > BE LIABLE FOR
> > -ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> > -TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> > -SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> > -
> > -**/
> > +/*
> > + * Copyright © 2006 Dave Airlie
> > + *
> > + * All Rights Reserved.
> > + */
> >  
> >  #include "intel_display_types.h"
> >  #include "intel_dvo_dev.h"
> > -- 
> > 2.36.1
> > 

Re: [Intel-gfx] [PATCH v2 24/39] drm/i915: dvo_sil164.c: use SPDX header

[Joe Perches]

On Fri, 2022-07-15 at 17:35 -0400, Rodrigo Vivi wrote:
> On Wed, Jul 13, 2022 at 09:12:12AM +0100, Mauro Carvalho Chehab wrote:
> > This file is licensed with MIT license. Change its license text
> > to use SPDX.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> 
> Reviewed-by: Rodrigo Vivi 

Not exactly the MIT license as it's missing "or copyright holders"
> 
> > ---
> > 
> > To avoid mailbombing on a large number of people, only mailing lists were 
> > C/C on the cover.
> > See [PATCH v2 00/39] at: 
> > https://lore.kernel.org/all/cover.1657699522.git.mche...@kernel.org/
> > 
> >  drivers/gpu/drm/i915/display/dvo_sil164.c | 32 +--
> >  1 file changed, 6 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/dvo_sil164.c 
> > b/drivers/gpu/drm/i915/display/dvo_sil164.c
> > index 0dfa0a0209ff..12974f7c9dc1 100644
> > --- a/drivers/gpu/drm/i915/display/dvo_sil164.c
> > +++ b/drivers/gpu/drm/i915/display/dvo_sil164.c
> > @@ -1,30 +1,10 @@
> > -/**
> > +// SPDX-License-Identifier: MIT
> >  
> > -Copyright © 2006 Dave Airlie
> > -
> > -All Rights Reserved.
> > -
> > -Permission is hereby granted, free of charge, to any person obtaining a
> > -copy of this software and associated documentation files (the
> > -"Software"), to deal in the Software without restriction, including
> > -without limitation the rights to use, copy, modify, merge, publish,
> > -distribute, sub license, and/or sell copies of the Software, and to
> > -permit persons to whom the Software is furnished to do so, subject to
> > -the following conditions:
> > -
> > -The above copyright notice and this permission notice (including the
> > -next paragraph) shall be included in all copies or substantial portions
> > -of the Software.
> > -
> > -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > -OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > -MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> > -IN NO EVENT SHALL THE AUTHOR

Missing "Authors or copyright holders"

> > BE LIABLE FOR
> > -ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
> > -TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
> > -SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> > -
> > -**/
> > +/*
> > + * Copyright © 2006 Dave Airlie
> > + *
> > + * All Rights Reserved.
> > + */
> >  
> >  #include "intel_display_types.h"
> >  #include "intel_dvo_dev.h"
> > -- 
> > 2.36.1
> > 

Re: [PATCH] remove CONFIG_ANDROID

[Joe Perches]

On Thu, 2022-06-30 at 02:50 +0200, Jason A. Donenfeld wrote:
> On Wed, Jun 29, 2022 at 05:36:57PM -0700, Joe Perches wrote:
> > > > +static ssize_t pm_userspace_autosleeper_show(struct kobject *kobj,
> > > > +   struct kobj_attribute *attr, char *buf)
> > > > +{
> > > > +   return sprintf(buf, "%d\n", pm_userspace_autosleeper_enabled);
> > 
> > This should use sysfs_emit no?
> 
> Probably, yea. Note that I just copy and pasted a nearby function,
> pm_async_show, `:%s/`d the variable name, and then promptly `git diff |
> clip`d it and plonked it into my email. Looking at the file, it uses
> sprintf all over the place in this fashion. So you may want to submit a
> cleanup to Rafael on this if you're right about sysfs_emit() being
> universally preferred.

Perhaps:

(trivial refactored and added a missing newline in autosleep_show)

---
 kernel/power/main.c | 102 ++--
 1 file changed, 52 insertions(+), 50 deletions(-)

diff --git a/kernel/power/main.c b/kernel/power/main.c
index e3694034b7536..c8a030319b15c 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -100,7 +100,7 @@ int pm_async_enabled = 1;
 static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
 char *buf)
 {
-   return sprintf(buf, "%d\n", pm_async_enabled);
+   return sysfs_emit(buf, "%d\n", pm_async_enabled);
 }
 
 static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute 
*attr,
@@ -124,27 +124,25 @@ power_attr(pm_async);
 static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute 
*attr,
  char *buf)
 {
-   char *s = buf;
+   int len = 0;
suspend_state_t i;
 
for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
if (i >= PM_SUSPEND_MEM && cxl_mem_active())
continue;
-   if (mem_sleep_states[i]) {
-   const char *label = mem_sleep_states[i];
+   if (!mem_sleep_states[i])
+   continue;
 
-   if (mem_sleep_current == i)
-   s += sprintf(s, "[%s] ", label);
-   else
-   s += sprintf(s, "%s ", label);
-   }
+   len += sysfs_emit_at(buf, len,
+mem_sleep_current == i ? "[%s] " : "%s ",
+mem_sleep_states[i]);
}
 
-   /* Convert the last space to a newline if needed. */
-   if (s != buf)
-   *(s-1) = '\n';
+   /* Convert the last space to a newline if needed */
+   if (len)
+   sysfs_emit_at(buf, len - 1, "\n");
 
-   return (s - buf);
+   return len;
 }
 
 static suspend_state_t decode_suspend_state(const char *buf, size_t n)
@@ -205,7 +203,7 @@ bool sync_on_suspend_enabled = 
!IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC);
 static ssize_t sync_on_suspend_show(struct kobject *kobj,
   struct kobj_attribute *attr, char *buf)
 {
-   return sprintf(buf, "%d\n", sync_on_suspend_enabled);
+   return sysfs_emit(buf, "%d\n", sync_on_suspend_enabled);
 }
 
 static ssize_t sync_on_suspend_store(struct kobject *kobj,
@@ -242,22 +240,22 @@ static const char * const pm_tests[__TEST_AFTER_LAST] = {
 static ssize_t pm_test_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buf)
 {
-   char *s = buf;
+   int len = 0;
int level;
 
-   for (level = TEST_FIRST; level <= TEST_MAX; level++)
-   if (pm_tests[level]) {
-   if (level == pm_test_level)
-   s += sprintf(s, "[%s] ", pm_tests[level]);
-   else
-   s += sprintf(s, "%s ", pm_tests[level]);
-   }
+   for (level = TEST_FIRST; level <= TEST_MAX; level++) {
+   if (!pm_tests[level])
+   continue;
+   len += sysfs_emit_at(buf, len,
+level == pm_test_level ? "[%s] " : "%s ",
+pm_tests[level]);
+   }
 
-   if (s != buf)
-   /* convert the last space to a newline */
-   *(s-1) = '\n';
+   /* convert the last space to a newline if needed */
+   if (len)
+   sysfs_emit_at(buf, len - 1, "\n");
 
-   return (s - buf);
+   return len;
 }
 
 static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -314,7 +312,7 @@ static char *suspend_step_name(enum suspend_stat_step step)

Re: [PATCH] remove CONFIG_ANDROID

[Joe Perches]

On Wed, 2022-06-29 at 16:19 -0700, Kalesh Singh wrote:
> On Wed, Jun 29, 2022 at 4:02 PM Jason A. Donenfeld  wrote:
> > On Wed, Jun 29, 2022 at 03:26:33PM -0700, Kalesh Singh wrote:
> > > Thanks for taking a look. I'm concerned holding the sys/power/state
> > > open would have unintentional side effects. Adding the
> > > /sys/power/userspace_autosuspender seems more appropriate. We don't
> > > have a use case for the refcounting, so would prefer the simpler
> > > writing '0' / '1' to toggle semantics.
> > 
> > Alright. So I've cooked you up some code that you can submit, since I
> > assume based on Christoph's bristliness that he won't do so. The below
> > adds /sys/power/pm_userspace_autosleeper, which you can write a 0 or a 1
> > into, and fixes up wireguard and random.c to use it. The code is
> > untested, but should generally be the correct thing, I think.
> > 
> > So in order of operations:
> > 
> > 1. You write a patch for SystemSuspend.cpp and post it on Gerrit.
> > 
> > 2. You take the diff below, clean it up or bikeshed the naming a bit or
> >do whatever there, and submit it to Rafael's PM tree, including as a
> >`Link: ...` this thread and the Gerrit link.
> > 
> > 3. When/if Rafael accepts the patch, you submit the Gerrit CL.
> > 
> > 4. When both have landed, Christoph moves forward with his
> >CONFIG_ANDROID removal.
> > 
> > Does that seem like a reasonable way forward?
> 
> Sounds like a plan. I'll clean up and repost your patch once the
> Gerrit change is ready.

trivial note:

> > diff --git a/kernel/power/main.c b/kernel/power/main.c
[]
> > @@ -120,6 +120,23 @@ static ssize_t pm_async_store(struct kobject *kobj, 
> > struct kobj_attribute *attr,
> > 
> >  power_attr(pm_async);
> > 
> > +bool pm_userspace_autosleeper_enabled;
> > +
> > +static ssize_t pm_userspace_autosleeper_show(struct kobject *kobj,
> > +   struct kobj_attribute *attr, char *buf)
> > +{
> > +   return sprintf(buf, "%d\n", pm_userspace_autosleeper_enabled);

This should use sysfs_emit no?

Re: [PATCH] staging: fbtft: fix alignment should match open parenthesis

[Joe Perches]

On Sat, 2022-06-25 at 19:00 -0700, David Reaver wrote:
> Fix alignment of this line of code with the previous parenthesis, as
> suggested by checkpatch.pl:
[]
> diff --git a/drivers/staging/fbtft/fb_tinylcd.c 
> b/drivers/staging/fbtft/fb_tinylcd.c
[]
> @@ -38,7 +38,7 @@ static int init_display(struct fbtft_par *par)
>   write_reg(par, 0xE5, 0x00);
>   write_reg(par, 0xF0, 0x36, 0xA5, 0x53);
>   write_reg(par, 0xE0, 0x00, 0x35, 0x33, 0x00, 0x00, 0x00,
> -0x00, 0x35, 0x33, 0x00, 0x00, 0x00);
> +   0x00, 0x35, 0x33, 0x00, 0x00, 0x00);

It's probably better to ignore the message in this case as the first
argument means something and the second and subsequent are the data
being written via a specific macro using NUMARGS.

Re: [Nouveau] [PATCH] drm/nouveau/mmu: Fix a typo

[Joe Perches]

On Wed, 2022-06-22 at 09:52 +0800, Zhang Jiaming wrote:
> There is a typo in comments. Change 'neeed' to 'need'.
[]
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c 
> b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
[]
> @@ -280,7 +280,7 @@ nvkm_vmm_unref_ptes(struct nvkm_vmm_iter *it, bool pfn, 
> u32 ptei, u32 ptes)
>   if (desc->type == SPT && (pgt->refs[0] || pgt->refs[1]))
>   nvkm_vmm_unref_sptes(it, pgt, desc, ptei, ptes);
>  
> - /* PT no longer neeed?  Destroy it. */
> + /* PT no longer need?  Destroy it. */

needed

Re: [PATCH] drm/nouveau/mmu: Fix a typo

[Joe Perches]

On Wed, 2022-06-22 at 09:52 +0800, Zhang Jiaming wrote:
> There is a typo in comments. Change 'neeed' to 'need'.
[]
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c 
> b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
[]
> @@ -280,7 +280,7 @@ nvkm_vmm_unref_ptes(struct nvkm_vmm_iter *it, bool pfn, 
> u32 ptei, u32 ptes)
>   if (desc->type == SPT && (pgt->refs[0] || pgt->refs[1]))
>   nvkm_vmm_unref_sptes(it, pgt, desc, ptei, ptes);
>  
> - /* PT no longer neeed?  Destroy it. */
> + /* PT no longer need?  Destroy it. */

needed

Re: [PATCH v3 4/4] drm/msm/adreno: Fix up formatting

[Joe Perches]

On Sat, 2022-05-28 at 18:03 +0200, Konrad Dybcio wrote:
> Leading spaces are not something checkpatch likes, and it says so when
> they are present. Use tabs consistently to indent function body and
> unwrap a 83-char-long line, as 100 is cool nowadays.

unassociated trivia:

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
[]
> @@ -199,7 +199,7 @@ static inline int adreno_is_a420(struct adreno_gpu *gpu)
>  
>  static inline int adreno_is_a430(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 430;
> + return gpu->revn == 430;
>  }

looks like these could/should return bool

>  static inline int adreno_is_a506(struct adreno_gpu *gpu)
> @@ -239,7 +239,7 @@ static inline int adreno_is_a540(struct adreno_gpu *gpu)
>  
>  static inline int adreno_is_a618(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 618;
> + return gpu->revn == 618;
>  }

etc...

Re: [Freedreno] [PATCH v3 4/4] drm/msm/adreno: Fix up formatting

[Joe Perches]

On Sat, 2022-05-28 at 18:03 +0200, Konrad Dybcio wrote:
> Leading spaces are not something checkpatch likes, and it says so when
> they are present. Use tabs consistently to indent function body and
> unwrap a 83-char-long line, as 100 is cool nowadays.

unassociated trivia:

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h 
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
[]
> @@ -199,7 +199,7 @@ static inline int adreno_is_a420(struct adreno_gpu *gpu)
>  
>  static inline int adreno_is_a430(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 430;
> + return gpu->revn == 430;
>  }

looks like these could/should return bool

>  static inline int adreno_is_a506(struct adreno_gpu *gpu)
> @@ -239,7 +239,7 @@ static inline int adreno_is_a540(struct adreno_gpu *gpu)
>  
>  static inline int adreno_is_a618(struct adreno_gpu *gpu)
>  {
> -   return gpu->revn == 618;
> + return gpu->revn == 618;
>  }

etc...

Re: [PATCH v3] workqueue: Wrap flush_workqueue() using a macro

[Joe Perches]

On Mon, 2022-05-16 at 14:00 +0900, Tetsuo Handa wrote:
[]
> Changes in v3:
>   Revert suggested change in v2, for kernel test robot  found
> 
> warning: Function parameter or member 'flush_workqueue' not described in 
> 'void'
> warning: expecting prototype for flush_workqueue(). Prototype was for 
> void() instead
> 
>   when built with W=1 option.

Odd, perhaps that not a valid error message and is a defect
in gcc's parsing of function definitions.

> Changes in v2:
>   Use "void (flush_workqueue)(struct workqueue_struct *wq)" and remove
>   "#undef flush_workqueue", suggested by Joe Perches .

Re: [dm-devel] [PATCH] hex2bin: make the function hex_to_bin constant-time

[Joe Perches]

On Sun, 2022-04-24 at 16:54 -0400, Mikulas Patocka wrote:
> This patch changes the function hex_to_bin so that it contains no branches
> and no memory accesses.
[]
> +++ linux-2.6/lib/hexdump.c   2022-04-24 18:51:20.0 +0200
[]
> + * the next line is similar to the previous one, but we need to decode both
> + *   uppercase and lowercase letters, so we use (ch & 0xdf), which converts
> + *   lowercase to uppercase
>   */
>  int hex_to_bin(char ch)
>  {
> - if ((ch >= '0') && (ch <= '9'))
> - return ch - '0';
> - ch = tolower(ch);
> - if ((ch >= 'a') && (ch <= 'f'))
> - return ch - 'a' + 10;
> - return -1;
> + return -1 +
> + ((ch - '0' + 1) & (((ch - '9' - 1) & ('0' - 1 - ch)) >> 8)) +
> + (((ch & 0xdf) - 'A' + 11) & ch & 0xdf) - 'F' - 1) & ('A' - 
> 1 - (ch & 0xdf))) >> 8));

probably easier to read using a temporary for ch & 0xdf

int CH = ch & 0xdf;

return -1 +
   ((ch - '0' +  1) & (((ch - '9' - 1) & ('0' - 1 - ch)) >> 8)) +
   ((CH - 'A' + 11) & (((CH - 'F' - 1) & ('A' - 1 - CH)) >> 8));


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH] tty/hvc_opal: simplify if-if to if-else

[Joe Perches]

On Sun, 2022-04-24 at 17:25 +0800, Wan Jiabing wrote:
> Use if and else instead of if(A) and if (!A).
[]
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
[]
> @@ -344,14 +344,15 @@ void __init hvc_opal_init_early(void)
>   opal = of_find_node_by_path("/ibm,opal/consoles");
>   if (opal)
>   pr_devel("hvc_opal: Found consoles in new location\n");
> - if (!opal) {
> + else {
>   opal = of_find_node_by_path("/ibm,opal");
>   if (opal)
>   pr_devel("hvc_opal: "
>"Found consoles in old location\n");
> + else
> + return;

A few things:

o add {} braces to first block before else
o see about using pr_fmt to prefix the pr_ output
o reverse the test and unindent the pr_devel

if (!opal)
return;
pr_devel("...);

Maybe a few more just to quiet checkpatch noise.  Something like:
---
 drivers/tty/hvc/hvc_opal.c | 58 +-
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index 84776bc641e6b..a42d5697ae198 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -5,6 +5,8 @@
  * Copyright 2011 Benjamin Herrenschmidt , IBM Corp.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #undef DEBUG
 
 #include 
@@ -43,6 +45,7 @@ struct hvc_opal_priv {
hv_protocol_t   proto;  /* Raw data or HVSI packets */
struct hvsi_privhvsi;   /* HVSI specific data */
 };
+
 static struct hvc_opal_priv *hvc_opal_privs[MAX_NR_HVC_CONSOLES];
 
 /* For early boot console */
@@ -124,7 +127,7 @@ static int hvc_opal_hvsi_tiocmget(struct hvc_struct *hp)
 }
 
 static int hvc_opal_hvsi_tiocmset(struct hvc_struct *hp, unsigned int set,
-   unsigned int clear)
+ unsigned int clear)
 {
struct hvc_opal_priv *pv = hvc_opal_privs[hp->vtermno];
 
@@ -167,8 +170,7 @@ static int hvc_opal_probe(struct platform_device *dev)
proto = HV_PROTOCOL_HVSI;
ops = _opal_hvsi_ops;
} else {
-   pr_err("hvc_opal: Unknown protocol for %pOF\n",
-  dev->dev.of_node);
+   pr_err("Unknown protocol for %pOF\n", dev->dev.of_node);
return -ENXIO;
}
 
@@ -195,15 +197,16 @@ static int hvc_opal_probe(struct platform_device *dev)
 termno, 0);
}
 
-   /* Instanciate now to establish a mapping index==vtermno */
+   /* Instantiate now to establish a mapping index==vtermno */
hvc_instantiate(termno, termno, ops);
} else {
-   pr_err("hvc_opal: Device %pOF has duplicate terminal number 
#%d\n",
+   pr_err("Device %pOF has duplicate terminal number #%d\n",
   dev->dev.of_node, termno);
return -ENXIO;
}
 
-   pr_info("hvc%d: %s protocol on %pOF%s\n", termno,
+   pr_info("hvc%d: %s protocol on %pOF%s\n",
+   termno,
proto == HV_PROTOCOL_RAW ? "raw" : "hvsi",
dev->dev.of_node,
boot ? " (boot console)" : "");
@@ -211,13 +214,13 @@ static int hvc_opal_probe(struct platform_device *dev)
irq = irq_of_parse_and_map(dev->dev.of_node, 0);
if (!irq) {
pr_info("hvc%d: No interrupts property, using OPAL event\n",
-   termno);
+   termno);
irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
}
 
if (!irq) {
-   pr_err("hvc_opal: Unable to map interrupt for device %pOF\n",
-   dev->dev.of_node);
+   pr_err("Unable to map interrupt for device %pOF\n",
+  dev->dev.of_node);
return irq;
}
 
@@ -275,7 +278,7 @@ static void udbg_opal_putc(char c)
udbg_opal_putc('\r');
 
do {
-   switch(hvc_opal_boot_priv.proto) {
+   switch (hvc_opal_boot_priv.proto) {
case HV_PROTOCOL_RAW:
count = opal_put_chars(termno, , 1);
break;
@@ -288,7 +291,7 @@ static void udbg_opal_putc(char c)
 * when there aren't any interrupts.
 */
opal_flush_console(termno);
-   } while(count == 0 || count == -EAGAIN);
+   } while (count == 0 || count == -EAGAIN);
 }
 
 static int udbg_opal_getc_poll(void)
@@ -297,7 +300,7 @@ static int udbg_opal_getc_poll(void)
int rc = 0;
char c;
 
-   switch(hvc_opal_boot_priv.proto) {
+   switch (hvc_opal_boot_priv.proto) {

Re: [Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization

[Joe Perches]

On Sat, 2022-04-16 at 13:48 -0700, Tom Rix wrote:
> On 4/16/22 11:33 AM, Joe Perches wrote:
> > On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> > > In insert_mappable_node(), the parameter node is
> > > cleared late in node's use with memset.
> > > insert_mappable_node() is a singleton, called only
> > > from i915_gem_gtt_prepare() which itself is only
> > > called by i915_gem_gtt_pread() and
> > > i915_gem_gtt_pwrite_fast() where the definition of
> > > node originates.
> > > 
> > > Instead of using memset, initialize node to 0 at it's
> > > definitions.
> > trivia: /it's/its/
> > 
> > Only reason _not_ to do this is memset is guaranteed to
> > zero any padding that might go to userspace.
> > 
> > But it doesn't seem there is any padding anyway nor is
> > the struct available to userspace.
> > 
> > So this seems fine though it might increase overall code
> > size a tiny bit.
> > 
> > I do have a caveat: see below:
> > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > []
> > > @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct 
> > > drm_i915_gem_object *obj,
> > >   goto err_ww;
> > >   } else if (!IS_ERR(vma)) {
> > >   node->start = i915_ggtt_offset(vma);
> > > - node->flags = 0;
> > Why is this unneeded?
> 
> node = {} initializes all of node's elements to 0, including this one.

true, but could the call to insert_mappable_node combined with the
retry goto in i915_gem_gtt_prepare set this to non-zero?

Re: [PATCH] drm/i915: change node clearing from memset to initialization

[Joe Perches]

On Sat, 2022-04-16 at 13:48 -0700, Tom Rix wrote:
> On 4/16/22 11:33 AM, Joe Perches wrote:
> > On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> > > In insert_mappable_node(), the parameter node is
> > > cleared late in node's use with memset.
> > > insert_mappable_node() is a singleton, called only
> > > from i915_gem_gtt_prepare() which itself is only
> > > called by i915_gem_gtt_pread() and
> > > i915_gem_gtt_pwrite_fast() where the definition of
> > > node originates.
> > > 
> > > Instead of using memset, initialize node to 0 at it's
> > > definitions.
> > trivia: /it's/its/
> > 
> > Only reason _not_ to do this is memset is guaranteed to
> > zero any padding that might go to userspace.
> > 
> > But it doesn't seem there is any padding anyway nor is
> > the struct available to userspace.
> > 
> > So this seems fine though it might increase overall code
> > size a tiny bit.
> > 
> > I do have a caveat: see below:
> > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > []
> > > @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct 
> > > drm_i915_gem_object *obj,
> > >   goto err_ww;
> > >   } else if (!IS_ERR(vma)) {
> > >   node->start = i915_ggtt_offset(vma);
> > > - node->flags = 0;
> > Why is this unneeded?
> 
> node = {} initializes all of node's elements to 0, including this one.

true, but could the call to insert_mappable_node combined with the
retry goto in i915_gem_gtt_prepare set this to non-zero?

Re: [Intel-gfx] [PATCH] drm/i915: change node clearing from memset to initialization

[Joe Perches]

On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> In insert_mappable_node(), the parameter node is
> cleared late in node's use with memset.
> insert_mappable_node() is a singleton, called only
> from i915_gem_gtt_prepare() which itself is only
> called by i915_gem_gtt_pread() and
> i915_gem_gtt_pwrite_fast() where the definition of
> node originates.
> 
> Instead of using memset, initialize node to 0 at it's
> definitions.

trivia: /it's/its/

Only reason _not_ to do this is memset is guaranteed to
zero any padding that might go to userspace.

But it doesn't seem there is any padding anyway nor is
the struct available to userspace.

So this seems fine though it might increase overall code
size a tiny bit.

I do have a caveat: see below:

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
> @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct 
> drm_i915_gem_object *obj,
>   goto err_ww;
>   } else if (!IS_ERR(vma)) {
>   node->start = i915_ggtt_offset(vma);
> - node->flags = 0;

Why is this unneeded?

from: drm_mm_insert_node_in_range which can set node->flags

__set_bit(DRM_MM_NODE_ALLOCATED_BIT, >flags);

Re: [PATCH] drm/i915: change node clearing from memset to initialization

[Joe Perches]

On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> In insert_mappable_node(), the parameter node is
> cleared late in node's use with memset.
> insert_mappable_node() is a singleton, called only
> from i915_gem_gtt_prepare() which itself is only
> called by i915_gem_gtt_pread() and
> i915_gem_gtt_pwrite_fast() where the definition of
> node originates.
> 
> Instead of using memset, initialize node to 0 at it's
> definitions.

trivia: /it's/its/

Only reason _not_ to do this is memset is guaranteed to
zero any padding that might go to userspace.

But it doesn't seem there is any padding anyway nor is
the struct available to userspace.

So this seems fine though it might increase overall code
size a tiny bit.

I do have a caveat: see below:

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
> @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct 
> drm_i915_gem_object *obj,
>   goto err_ww;
>   } else if (!IS_ERR(vma)) {
>   node->start = i915_ggtt_offset(vma);
> - node->flags = 0;

Why is this unneeded?

from: drm_mm_insert_node_in_range which can set node->flags

__set_bit(DRM_MM_NODE_ALLOCATED_BIT, >flags);

Re: [PATCH 16/22] dvb-usb: Replace comments with C99 initializers

[Joe Perches]

On Sat, 2022-03-26 at 19:27 +0100, Mauro Carvalho Chehab wrote:
> Em Sat, 26 Mar 2022 19:24:54 +0100
> Mauro Carvalho Chehab  escreveu:
> 
> > Em Sat, 26 Mar 2022 17:59:03 +0100
> > Benjamin Stürz  escreveu:
> > 
> > > This replaces comments with C99's designated
> > > initializers because the kernel supports them now.
> > > 
> > > Signed-off-by: Benjamin Stürz 
> > > ---
> > >  drivers/media/usb/dvb-usb/dibusb-mb.c | 62 +--
> > >  drivers/media/usb/dvb-usb/dibusb-mc.c | 34 +++
> > >  2 files changed, 48 insertions(+), 48 deletions(-)
> > > 
> > > diff --git a/drivers/media/usb/dvb-usb/dibusb-mb.c 
> > > b/drivers/media/usb/dvb-usb/dibusb-mb.c
> > > index e9dc27f73970..f188e07f518b 100644
> > > --- a/drivers/media/usb/dvb-usb/dibusb-mb.c
> > > +++ b/drivers/media/usb/dvb-usb/dibusb-mb.c
> > > @@ -122,40 +122,40 @@ static int dibusb_probe(struct usb_interface *intf,
> > >  
> > >  /* do not change the order of the ID table */
> > >  static struct usb_device_id dibusb_dib3000mb_table [] = {
> > > -/* 00 */ { USB_DEVICE(USB_VID_WIDEVIEW,  
> > > USB_PID_AVERMEDIA_DVBT_USB_COLD) },
> > > -/* 01 */ { USB_DEVICE(USB_VID_WIDEVIEW,  
> > > USB_PID_AVERMEDIA_DVBT_USB_WARM) },
> > > -/* 02 */ { USB_DEVICE(USB_VID_COMPRO,
> > > USB_PID_COMPRO_DVBU2000_COLD) },
> > > -/* 03 */ { USB_DEVICE(USB_VID_COMPRO,
> > > USB_PID_COMPRO_DVBU2000_WARM) },
> > > -/* 04 */ { USB_DEVICE(USB_VID_COMPRO_UNK,
> > > USB_PID_COMPRO_DVBU2000_UNK_COLD) },
> > > -/* 05 */ { USB_DEVICE(USB_VID_DIBCOM,
> > > USB_PID_DIBCOM_MOD3000_COLD) },
> > > -/* 06 */ { USB_DEVICE(USB_VID_DIBCOM,
> > > USB_PID_DIBCOM_MOD3000_WARM) },
> > > -/* 07 */ { USB_DEVICE(USB_VID_EMPIA, 
> > > USB_PID_KWORLD_VSTREAM_COLD) },
> > > -/* 08 */ { USB_DEVICE(USB_VID_EMPIA, 
> > > USB_PID_KWORLD_VSTREAM_WARM) },
> > > -/* 09 */ { USB_DEVICE(USB_VID_GRANDTEC,  
> > > USB_PID_GRANDTEC_DVBT_USB_COLD) },
> > > -/* 10 */ { USB_DEVICE(USB_VID_GRANDTEC,  
> > > USB_PID_GRANDTEC_DVBT_USB_WARM) },
> > > -/* 11 */ { USB_DEVICE(USB_VID_GRANDTEC,  
> > > USB_PID_DIBCOM_MOD3000_COLD) },
> > > -/* 12 */ { USB_DEVICE(USB_VID_GRANDTEC,  
> > > USB_PID_DIBCOM_MOD3000_WARM) },
> > > -/* 13 */ { USB_DEVICE(USB_VID_HYPER_PALTEK,  
> > > USB_PID_UNK_HYPER_PALTEK_COLD) },
> > > -/* 14 */ { USB_DEVICE(USB_VID_HYPER_PALTEK,  
> > > USB_PID_UNK_HYPER_PALTEK_WARM) },
> > > -/* 15 */ { USB_DEVICE(USB_VID_VISIONPLUS,
> > > USB_PID_TWINHAN_VP7041_COLD) },
> > > -/* 16 */ { USB_DEVICE(USB_VID_VISIONPLUS,
> > > USB_PID_TWINHAN_VP7041_WARM) },
> > > -/* 17 */ { USB_DEVICE(USB_VID_TWINHAN,   
> > > USB_PID_TWINHAN_VP7041_COLD) },
> > > -/* 18 */ { USB_DEVICE(USB_VID_TWINHAN,   
> > > USB_PID_TWINHAN_VP7041_WARM) },
> > > -/* 19 */ { USB_DEVICE(USB_VID_ULTIMA_ELECTRONIC, 
> > > USB_PID_ULTIMA_TVBOX_COLD) },
> > > -/* 20 */ { USB_DEVICE(USB_VID_ULTIMA_ELECTRONIC, 
> > > USB_PID_ULTIMA_TVBOX_WARM) },
> > > -/* 21 */ { USB_DEVICE(USB_VID_ULTIMA_ELECTRONIC, 
> > > USB_PID_ULTIMA_TVBOX_AN2235_COLD) },
> > > -/* 22 */ { USB_DEVICE(USB_VID_ULTIMA_ELECTRONIC, 
> > > USB_PID_ULTIMA_TVBOX_AN2235_WARM) },
> > > -/* 23 */ { USB_DEVICE(USB_VID_ADSTECH,   
> > > USB_PID_ADSTECH_USB2_COLD) },
> > > +[0]  =   { USB_DEVICE(USB_VID_WIDEVIEW,  
> > > USB_PID_AVERMEDIA_DVBT_USB_COLD) },
> > > +[1]  =   { USB_DEVICE(USB_VID_WIDEVIEW,  
> > > USB_PID_AVERMEDIA_DVBT_USB_WARM) },  
> > 
> > While here, please properly indent this table, and respect the 80-columns 
> > limit,
> > e. g.:
> > 
> > static struct usb_device_id dibusb_dib3000mb_table [] = {
> > [0] = { USB_DEVICE(USB_VID_WIDEVIEW 
> >USB_PID_AVERMEDIA_DVBT_USB_COLD) 
> > },
> > [1]  =  { USB_DEVICE(USB_VID_WIDEVIEW,
> >  USB_PID_AVERMEDIA_DVBT_USB_WARM)
> > },
> > ...
> 
> Err something went wrong with my space bar and I ended hitting send to
> soon... I meant:
> 
> static struct usb_device_id dibusb_dib3000mb_table [] = {
>   [0] = { USB_DEVICE(USB_VID_WIDEVIEW 
>  USB_PID_AVERMEDIA_DVBT_USB_COLD) 
>   },
>   [1] = { USB_DEVICE(USB_VID_WIDEVIEW,
>  USB_PID_AVERMEDIA_DVBT_USB_WARM)
>   },
>   ...
> };

maybe static const too

and

maybe

#define DIB_DEVICE(vid, pid)\
{ USB_DEVICE(USB_VID_ ## vid, USB_PID_ ## pid) }

so maybe

static const struct usb_device_id dibusb_dib3000mb_table[] = {
[0] = DIB_DEVICE(WIDEVIEW, AVERMEDIA_DVBT_USB_COLD),
[1] = DIB_DEVICE(WIDEVIEW, AVERMEDIA_DVBT_USB_WARM),
...
};

though I _really_ doubt the value of the specific indexing.

I think this isn't really worth changing at all.

Re: [PATCH 02/22] s3c: Replace comments with C99 initializers

[Joe Perches]

On Sat, 2022-03-26 at 17:58 +0100, Benjamin Stürz wrote:
> This replaces comments with C99's designated
> initializers because the kernel supports them now.
[]
> diff --git a/arch/arm/mach-s3c/bast-irq.c b/arch/arm/mach-s3c/bast-irq.c
[]
> @@ -29,22 +29,22 @@
>   * the irq is not implemented
>  */
>  static const unsigned char bast_pc104_irqmasks[] = {
> - 0,   /* 0 */
> - 0,   /* 1 */
> - 0,   /* 2 */
> - 1,   /* 3 */
> - 0,   /* 4 */
> - 2,   /* 5 */
> - 0,   /* 6 */
> - 4,   /* 7 */
> - 0,   /* 8 */
> - 0,   /* 9 */
> - 8,   /* 10 */
> - 0,   /* 11 */
> - 0,   /* 12 */
> - 0,   /* 13 */
> - 0,   /* 14 */
> - 0,   /* 15 */
> + [0]  = 0,
> + [1]  = 0,
> + [2]  = 0,
> + [3]  = 1,
> + [4]  = 0,
> + [5]  = 2,
> + [6]  = 0,
> + [7]  = 4,
> + [8]  = 0,
> + [9]  = 0,
> + [10] = 8,
> + [11] = 0,
> + [12] = 0,
> + [13] = 0,
> + [14] = 0,
> + [15] = 0,
>  };

I don't find this better than the initial array.

>  
>  static const unsigned char bast_pc104_irqs[] = { 3, 5, 7, 10 };

For the same reason this array is just an array
without the specified indexing.

Re: [PATCH] drm/v3d: Use kvcalloc

[Joe Perches]

On Sat, 2022-03-12 at 07:26 -0800, Harshit Mogalapalli wrote:
> kvcalloc is same as kvmalloc_array + __GFP_ZERO.
[]
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
[]
> @@ -308,9 +308,8 @@ v3d_lookup_bos(struct drm_device *dev,
>   return -EINVAL;
>   }
>  
> - job->bo = kvmalloc_array(job->bo_count,
> -  sizeof(struct drm_gem_cma_object *),
> -  GFP_KERNEL | __GFP_ZERO);
> + job->bo = kvcalloc(job->bo_count, sizeof(struct drm_gem_cma_object *),
> +GFP_KERNEL);
>   if (!job->bo) {
>   DRM_DEBUG("Failed to allocate validated BO pointers\n");
>   return -ENOMEM;

trivia:

The DRM_DEBUG could also be removed as the the alloc will do a
a dump_stack on failure.

Re: [Nouveau] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

[Joe Perches]

On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote:

> a multi-line indent gets curly braces for readability even though
> it's not required by C.  And then both sides would get curly braces.

That's more your personal preference than a coding style guideline.

Re: [PATCH 03/10] staging: wfx: format comments on 100 columns

[Joe Perches]

On Fri, 2022-02-25 at 12:23 +0100, Jerome Pouiller wrote:
> From: Jérôme Pouiller 
> 
> A few comments were not yet formatted on 100 columns.

IMO, none of these changes are necessary or good changes.

80 columns is preferred.

Really comments should most always use 80 columns, and
only occasionally should code be more than 80 columns
and almost never should code be more than 100 columns.

> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c
[]
> @@ -117,9 +117,7 @@ static int wfx_tx_policy_get(struct wfx_vif *wvif, struct 
> ieee80211_tx_rate *rat
>   if (idx >= 0) {
>   *renew = false;
>   } else {
> - /* If policy is not found create a new one using the oldest
> -  * entry in "free" list
> -  */
> + /* If policy is not found create a new one using the oldest 
> entry in "free" list */
>   *renew = true;
>   entry = list_entry(cache->free.prev, struct wfx_tx_policy, 
> link);
>   memcpy(entry->rates, wanted.rates, sizeof(entry->rates));
> @@ -494,9 +492,7 @@ void wfx_tx_confirm_cb(struct wfx_dev *wdev, const struct 
> wfx_hif_cnf_tx *arg)
>   wfx_tx_fill_rates(wdev, tx_info, arg);
>   skb_trim(skb, skb->len - tx_priv->icv_size);
>  
> - /* From now, you can touch to tx_info->status, but do not touch to
> -  * tx_priv anymore
> -  */
> + /* From now, you can touch to tx_info->status, but do not touch to 
> tx_priv anymore */
>   /* FIXME: use ieee80211_tx_info_clear_status() */
>   memset(tx_info->rate_driver_data, 0, sizeof(tx_info->rate_driver_data));
>   memset(tx_info->pad, 0, sizeof(tx_info->pad));
> diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
[]
> @@ -210,8 +210,8 @@ bool wfx_tx_queues_has_cab(struct wfx_vif *wvif)
>   if (wvif->vif->type != NL80211_IFTYPE_AP)
>   return false;
>   for (i = 0; i < IEEE80211_NUM_ACS; ++i)
> - /* Note: since only AP can have mcast frames in queue and only
> -  * one vif can be AP, all queued frames has same interface id
> + /* Note: since only AP can have mcast frames in queue and only 
> one vif can be AP,
> +  * all queued frames has same interface id
>*/
>   if (!skb_queue_empty_lockless(>tx_queue[i].cab))
>   return true;
> @@ -253,9 +253,8 @@ static struct sk_buff *wfx_tx_queues_get_skb(struct 
> wfx_dev *wdev)
>   skb = skb_dequeue([i]->cab);
>   if (!skb)
>   continue;
> - /* Note: since only AP can have mcast frames in queue
> -  * and only one vif can be AP, all queued frames has
> -  * same interface id
> + /* Note: since only AP can have mcast frames in queue 
> and only one vif can
> +  * be AP, all queued frames has same interface id
>*/
>   hif = (struct wfx_hif_msg *)skb->data;
>   WARN_ON(hif->interface != wvif->id);


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel