Re: vhost-vdpa: Refactor copy_to_user() usage in vhost_vdpa_get_config()

[Markus Elfring]

>> Assign the return value from a copy_to_user() call to an additional
>> local variable so that a kvfree() call and return statement can be
>> omitted accordingly.
>
> Ugly and unidiomatic.
>
>> This issue was detected by using the Coccinelle software.
>
> What issue?

Opportunities for the reduction of duplicate source code.

Regards,
Markus

[PATCH] vhost-vdpa: Refactor copy_to_user() usage in vhost_vdpa_get_config()

[Markus Elfring]

From: Markus Elfring 
Date: Wed, 25 Sep 2024 20:36:35 +0200

Assign the return value from a copy_to_user() call to an additional
local variable so that a kvfree() call and return statement can be
omitted accordingly.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/vhost/vdpa.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 5a49b5a6d496..ca69527a822c 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -370,13 +370,11 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,

vdpa_get_config(vdpa, config.off, buf, config.len);

-   if (copy_to_user(c->buf, buf, config.len)) {
+   {
+   unsigned long ctu = copy_to_user(c->buf, buf, config.len);
kvfree(buf);
-   return -EFAULT;
+   return ctu ? -EFAULT : 0;
}
-
-   kvfree(buf);
-   return 0;
 }

 static long vhost_vdpa_set_config(struct vhost_vdpa *v,
--
2.46.1

[PATCH] remoteproc: qcom: q6v5-mss: Use common error handling code in q6v5_mpss_load()

[Markus Elfring]

From: Markus Elfring 
Date: Tue, 24 Sep 2024 15:55:06 +0200

Add jump targets so that a bit of exception handling can be better reused
at the end of this function implementation.

Signed-off-by: Markus Elfring 
---
 drivers/remoteproc/qcom_q6v5_mss.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
b/drivers/remoteproc/qcom_q6v5_mss.c
index 2a42215ce8e0..b398ae3083a1 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1451,9 +1451,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
dev_err(qproc->dev,
"failed to load segment %d from 
truncated file %s\n",
i, fw_name);
-   ret = -EINVAL;
-   memunmap(ptr);
-   goto release_firmware;
+   goto e_inval_unmap;
}

memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz);
@@ -1464,18 +1462,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
ptr, phdr->p_filesz);
if (ret) {
dev_err(qproc->dev, "failed to load %s\n", 
fw_name);
-   memunmap(ptr);
-   goto release_firmware;
+   goto unmap_mem;
}

if (seg_fw->size != phdr->p_filesz) {
dev_err(qproc->dev,
"failed to load segment %d from 
truncated file %s\n",
i, fw_name);
-   ret = -EINVAL;
release_firmware(seg_fw);
-   memunmap(ptr);
-   goto release_firmware;
+   goto e_inval_unmap;
}

release_firmware(seg_fw);
@@ -1528,6 +1523,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
kfree(fw_name);

return ret < 0 ? ret : 0;
+
+e_inval_unmap:
+   ret = -EINVAL;
+unmap_mem:
+   memunmap(ptr);
+   goto release_firmware;
 }

 static void qcom_q6v5_dump_segment(struct rproc *rproc,
--
2.46.1

[PATCH] remoteproc: k3: Call of_node_put(rmem_np) only once in three functions

[Markus Elfring]

From: Markus Elfring 
Date: Tue, 24 Sep 2024 14:28:35 +0200

An of_node_put(rmem_np) call was immediately used after a pointer check
for a of_reserved_mem_lookup() call in three function implementations.
Thus call such a function only once instead directly before the checks.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/remoteproc/ti_k3_dsp_remoteproc.c | 6 ++
 drivers/remoteproc/ti_k3_m4_remoteproc.c  | 6 ++
 drivers/remoteproc/ti_k3_r5_remoteproc.c  | 3 +--
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c 
b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
index 8be3f631c192..d08a3a98ada1 100644
--- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
@@ -576,11 +576,9 @@ static int k3_dsp_reserved_mem_init(struct k3_dsp_rproc 
*kproc)
return -EINVAL;

rmem = of_reserved_mem_lookup(rmem_np);
-   if (!rmem) {
-   of_node_put(rmem_np);
-   return -EINVAL;
-   }
of_node_put(rmem_np);
+   if (!rmem)
+   return -EINVAL;

kproc->rmem[i].bus_addr = rmem->base;
/* 64-bit address regions currently not supported */
diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c 
b/drivers/remoteproc/ti_k3_m4_remoteproc.c
index 09f0484a90e1..a16fb165fced 100644
--- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
@@ -433,11 +433,9 @@ static int k3_m4_reserved_mem_init(struct k3_m4_rproc 
*kproc)
return -EINVAL;

rmem = of_reserved_mem_lookup(rmem_np);
-   if (!rmem) {
-   of_node_put(rmem_np);
-   return -EINVAL;
-   }
of_node_put(rmem_np);
+   if (!rmem)
+   return -EINVAL;

kproc->rmem[i].bus_addr = rmem->base;
/* 64-bit address regions currently not supported */
diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c 
b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 747ee467da88..d0ebdd5cfa70 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -1001,12 +1001,11 @@ static int k3_r5_reserved_mem_init(struct k3_r5_rproc 
*kproc)
}

rmem = of_reserved_mem_lookup(rmem_np);
+   of_node_put(rmem_np);
if (!rmem) {
-   of_node_put(rmem_np);
ret = -EINVAL;
goto unmap_rmem;
}
-   of_node_put(rmem_np);

kproc->rmem[i].bus_addr = rmem->base;
/*
--
2.46.1

[PATCH] nvdimm: Call nvdimm_put_key(key) only once in nvdimm_key_revalidate()

[Markus Elfring]

From: Markus Elfring 
Date: Mon, 23 Sep 2024 17:05:39 +0200

A nvdimm_put_key(key) call was immediately used after a return code
check for a change_key() call in this function implementation.
Thus call such a function only once instead directly before the check.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/nvdimm/security.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/nvdimm/security.c b/drivers/nvdimm/security.c
index a03e3c45f297..83c30980307c 100644
--- a/drivers/nvdimm/security.c
+++ b/drivers/nvdimm/security.c
@@ -152,12 +152,10 @@ static int nvdimm_key_revalidate(struct nvdimm *nvdimm)
 * verify that the key is good.
 */
rc = nvdimm->sec.ops->change_key(nvdimm, data, data, NVDIMM_USER);
-   if (rc < 0) {
-   nvdimm_put_key(key);
+   nvdimm_put_key(key);
+   if (rc < 0)
return rc;
-   }

-   nvdimm_put_key(key);
nvdimm->sec.flags = nvdimm_security_flags(nvdimm, NVDIMM_USER);
return 0;
 }
--
2.46.1

Re: [PATCH] driver core: Fix a null-ptr-deref in module_add_driver()

[Markus Elfring]

> Inject fault while probing of-fpga-region, if kasprintf() fails in
> module_add_driver(), the second sysfs_remove_link() in exit path will cause
> null-ptr-deref as below because kernfs_name_hash() will call strlen() with
> NULL driver_name.
…

How do you think about to use the term “null pointer dereference”
for the commit message (and summary phrase)?


…
> +++ b/drivers/base/module.c
> @@ -66,27 +66,31 @@ int module_add_driver(struct module *mod, const struct 
> device_driver *drv)
…
>   sysfs_remove_link(mk->drivers_dir, driver_name);
> +
> +out_free_driver_name:
>   kfree(driver_name);
>
> +out_remove_kobj:
> + sysfs_remove_link(&drv->p->kobj, "module");
…

I suggest to omit two blank lines here.

Regards,
Markus

[PATCH] tracing: Replace 21 seq_puts() calls by seq_putc() calls

[Markus Elfring]

From: Markus Elfring 
Date: Sun, 14 Jul 2024 15:40:34 +0200

Single characters should be put into a sequence.
Thus use the corresponding function “seq_putc”.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 kernel/trace/trace_events_hist.c | 26 +-
 kernel/trace/trace_events_user.c |  8 
 kernel/trace/trace_hwlat.c   |  4 ++--
 kernel/trace/trace_osnoise.c |  4 ++--
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 6ece1308d36a..5992278cbfb5 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4898,14 +4898,14 @@ static void print_action_spec(struct seq_file *m,
for (i = 0; i < hist_data->n_save_vars; i++) {
seq_printf(m, "%s", 
hist_data->save_vars[i]->var->var.name);
if (i < hist_data->n_save_vars - 1)
-   seq_puts(m, ",");
+   seq_putc(m, ',');
}
} else if (data->action == ACTION_TRACE) {
if (data->use_trace_keyword)
seq_printf(m, "%s", data->synth_event_name);
for (i = 0; i < data->n_params; i++) {
if (i || data->use_trace_keyword)
-   seq_puts(m, ",");
+   seq_putc(m, ',');
seq_printf(m, "%s", data->params[i]);
}
}
@@ -4924,7 +4924,7 @@ static void print_track_data_spec(struct seq_file *m,

print_action_spec(m, hist_data, data);

-   seq_puts(m, ")");
+   seq_putc(m, ')');
 }

 static void print_onmatch_spec(struct seq_file *m,
@@ -4938,7 +4938,7 @@ static void print_onmatch_spec(struct seq_file *m,

print_action_spec(m, hist_data, data);

-   seq_puts(m, ")");
+   seq_putc(m, ')');
 }

 static bool actions_match(struct hist_trigger_data *hist_data,
@@ -5413,9 +5413,9 @@ static void hist_trigger_print_key(struct seq_file *m,
}

if (!multiline)
-   seq_puts(m, " ");
+   seq_putc(m, ' ');

-   seq_puts(m, "}");
+   seq_putc(m, '}');
 }

 /* Get the 100 times of the percentage of @val in @total */
@@ -5511,13 +5511,13 @@ static void hist_trigger_entry_print(struct seq_file *m,
if (flags & HIST_FIELD_FL_VAR || flags & HIST_FIELD_FL_EXPR)
continue;

-   seq_puts(m, " ");
+   seq_putc(m, ' ');
hist_trigger_print_val(m, i, field_name, flags, stats, elt);
}

print_actions(m, hist_data, elt);

-   seq_puts(m, "\n");
+   seq_putc(m, '\n');
 }

 static int print_entries(struct seq_file *m,
@@ -5971,7 +5971,7 @@ static int event_hist_trigger_print(struct seq_file *m,
field = hist_data->fields[i];

if (i > hist_data->n_vals)
-   seq_puts(m, ",");
+   seq_putc(m, ',');

if (field->flags & HIST_FIELD_FL_STACKTRACE) {
if (field->field)
@@ -5997,7 +5997,7 @@ static int event_hist_trigger_print(struct seq_file *m,
seq_puts(m, "hitcount");
} else {
if (show_val)
-   seq_puts(m, ",");
+   seq_putc(m, ',');
hist_field_print(m, field);
}
show_val = true;
@@ -6006,14 +6006,14 @@ static int event_hist_trigger_print(struct seq_file *m,
if (have_var) {
unsigned int n = 0;

-   seq_puts(m, ":");
+   seq_putc(m, ':');

for_each_hist_val_field(i, hist_data) {
field = hist_data->fields[i];

if (field->flags & HIST_FIELD_FL_VAR) {
if (n++)
-   seq_puts(m, ",");
+   seq_putc(m, ',');
hist_field_print(m, field);
}
}
@@ -6035,7 +6035,7 @@ static int event_hist_trigger_print(struct seq_file *m,
return -EINVAL;

if (i > 0)
-   seq_puts(m, ",");
+   seq_putc(m, ',');

if (idx == HITCOUNT_IDX)
seq_puts(m, "hitcount");
diff --git a/kernel/tra

[PATCH] module: tracking: Extend format string of a seq_printf() call in unloaded_tainted_modules_seq_show()

[Markus Elfring]

From: Markus Elfring 
Date: Sun, 14 Jul 2024 13:43:06 +0200
Subject: [PATCH] module: tracking: Extend format string of a seq_printf() call 
in unloaded_tainted_modules_seq_show()

* Move the specification of a single line break from a seq_puts() call
  into the format string of a previous seq_printf() call.

* Omit an extra call of the function “seq_puts”
  because of this refactoring.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 kernel/module/tracking.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
index 16742d1c630c..9f84271c23b3 100644
--- a/kernel/module/tracking.c
+++ b/kernel/module/tracking.c
@@ -94,9 +94,7 @@ static int unloaded_tainted_modules_seq_show(struct seq_file 
*m, void *p)
l = module_flags_taint(mod_taint->taints, buf);
buf[l++] = '\0';

-   seq_printf(m, "%s (%s) %llu", mod_taint->name, buf, mod_taint->count);
-   seq_puts(m, "\n");
-
+   seq_printf(m, "%s (%s) %llu\n", mod_taint->name, buf, mod_taint->count);
return 0;
 }

--
2.45.2

[PATCH] module: Use seq_putc() in two functions

[Markus Elfring]

From: Markus Elfring 
Date: Sun, 14 Jul 2024 13:13:15 +0200

Single characters should be put into a sequence.
Thus use the corresponding function “seq_putc”.

This issue was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 kernel/module/procfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/module/procfs.c b/kernel/module/procfs.c
index 0a4841e88adb..dc91d3dba8f3 100644
--- a/kernel/module/procfs.c
+++ b/kernel/module/procfs.c
@@ -35,7 +35,7 @@ static inline void print_unload_info(struct seq_file *m, 
struct module *mod)
}

if (!printed_something)
-   seq_puts(m, "-");
+   seq_putc(m, '-');
 }
 #else /* !CONFIG_MODULE_UNLOAD */
 static inline void print_unload_info(struct seq_file *m, struct module *mod)
@@ -99,7 +99,7 @@ static int m_show(struct seq_file *m, void *p)
if (mod->taints)
seq_printf(m, " %s", module_flags(mod, buf, true));

-   seq_puts(m, "\n");
+   seq_putc(m, '\n');
return 0;
 }

--
2.45.2

Re: [PATCH] remoteproc: Remove unneeded check in elf_strtbl_add()

[Markus Elfring]

…
> useless.

 because …?


> Fix this issue by removing unneeded check.

Another wording suggestion:
  Thus remove a redundant check.


…
> +++ b/drivers/remoteproc/remoteproc_elf_helpers.h
> @@ -107,7 +107,7 @@ static inline unsigned int elf_strtbl_add(const char 
> *name, void *ehdr, u8 class
>   shdr = ehdr + elf_size_of_hdr(class) + shstrndx * 
> elf_size_of_shdr(class);
>   strtab = ehdr + elf_shdr_get_sh_offset(class, shdr);
>   idx = index ? *index : 0;
> - if (!strtab || !name)
> + if (!name)
>   return 0;
…

How do you think about to perform the remaining null pointer check
as the first statement (because of input parameter validation in
this function implementation)?

Regards,
Markus

Re: [v4 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Markus Elfring]

>> Various source code places can be updated also according to referenced
>> programming interfaces.
>> https://elixir.bootlin.com/linux/v6.10-rc4/source/include/linux/cleanup.h#L8
>>
>> Will corresponding collateral evolution become better supported?
>
> Plesae stop this. cleanup.h might be a nice thing, but it should not be
> used to make code less obvious or worse.

These APIs were added to improve several software components,
weren't they?

Regards,
Markus

Re: [v4 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Markus Elfring]

>> Would you become interested to apply a statement like 
>> “guard(mutex)(&chip->mutex);”?
>> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196
>
> This does not look like real improvement for code this trivial.

Various source code places can be updated also according to referenced
programming interfaces.
https://elixir.bootlin.com/linux/v6.10-rc4/source/include/linux/cleanup.h#L8

Will corresponding collateral evolution become better supported?

Regards,
Markus

Re: [PATCH v4 2/3] leds: sy7802: Add support for Silergy SY7802 flash LED controller

[Markus Elfring]

> The SY7802 is a current-regulated charge pump which can regulate two
> current levels for Flash and Torch modes.
>
> It is a high-current synchronous boost converter with 2-channel high
> side current sources. Each channel is able to deliver 900mA current.

Would you like to improve such a change description with imperative wordings?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc3#n94


…
> +++ b/drivers/leds/flash/leds-sy7802.c
> @@ -0,0 +1,542 @@
…
> +static int sy7802_strobe_get(struct led_classdev_flash *fl_cdev, bool *state)
> +{
…
> + mutex_lock(&chip->mutex);
> + *state = !!(chip->fled_strobe_used & BIT(led->led_id));
> + mutex_unlock(&chip->mutex);
> +
> + return 0;
> +}
…

Would you become interested to apply a statement like 
“guard(mutex)(&chip->mutex);”?
https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196

Regards,
Markus

Re: [PATCH] livepatch: introduce klp_func called interface

[Markus Elfring]

Please add a version identifier to the message subject.


…
> If the patched function have bug, it may cause serious result
> such as kernel crash.

Wording suggestion:

   If the patched function has a bug, it might cause serious side effects
   like a kernel crash.


> This is a kobject attribute of klp_func. Sysfs interface named
>  "called" is introduced to livepatch …

Under which circumstances will imperative wordings be applied for
another improved change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n94


…
> ---
>  include/linux/livepatch.h |  2 ++
…

You may present version descriptions behind the marker line.
Would you like to indicate any adjustments according to your change approach
(from yesterday)?
https://lore.kernel.org/lkml/20240519074343.5833-1-zhangwar...@gmail.com/

Regards,
Markus

Re: [PATCH] livepatch: introduce klp_func called interface

[Markus Elfring]

…
> This commit introduce a read only interface of livepatch

Please improve the changelog with an imperative wording.


…
> find out which function is successfully called. Any testing process can make 
> sure they
> have successfully cover all the patched function that changed with the help 
> of this interface.

* I suggest to take preferred line lengths better into account
  also for such a change description.

* Please provide the tag “Signed-off-by”.
  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n398

Regards,
Markus

Re: [PATCH] eventfs: Directly return NULL to avoid null point dereferenced

[Markus Elfring]

> When the condition ei->is_free holds,we return NULL directly to
> avoid update_events_attr to use NULL point about ei.

* Please avoid typos in the summary phrase and the commit message.

* Would you like to use an imperative wording for an improved change 
description?
  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n94


…
> +++ b/fs/tracefs/event_inode.c
> @@ -346,8 +346,7 @@ static struct eventfs_inode *eventfs_find_events(struct 
> dentry *dentry)
>* doesn't matter.
>*/
>   if (ei->is_freed) {
> - ei = NULL;
> - break;
> + return NULL;
>   }
…

How do you think about to omit curly brackets here?

Regards,
Markus

Re: [v3] tracing/probes: Fix memory leak in traceprobe_parse_probe_arg_body()

[Markus Elfring]

…
> > it jumps to the label 'out' instead of 'fail' by mistake.In the result,
…
>
> Looks good to me.

* Do you care for a typo in this change description?

* Would you like to read any improved (patch) version descriptions (or 
changelogs)?

Regards,
Markus

Re: [PATCH v3] tracing/probes: Fix memory leak in traceprobe_parse_probe_arg_body()

[Markus Elfring]

> … by mistake.In the result,
…

I propose once more to start the second sentence of this change description
on a subsequent line.

> ---
> kernel/trace/trace_probe.c | 2 +-
…

Unfortunately, you overlooked to add patch version descriptions behind the 
marker line.

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n713

Regards,
Markus

Re: [PATCH v2] tracing/probes: Fix memory leak in traceprobe_parse_probe_arg_body

[Markus Elfring]

I suggest to append parentheses to the function name in the summary phrase.


> If traceprobe_parse_probe_arg_body() fails to allocate 'parg->fmt', it
> jumps to 'out' instead of 'fail' by mistake. In the result, in this
> case the 'tmp' buffer is not freed and leaks its memory.
>
> Fix it by jumping to 'fail' in that case.

I propose to improve such a change description another bit like the following.

   If traceprobe_parse_probe_arg_body() failed to allocate the object 
“parg->fmt”,
   it jumps to the label “out” instead of “fail” by mistake.
   In the result, the buffer “tmp” is not freed in this case and leaks its 
memory.

   Thus jump to the label “fail” in that error case.


Regards,
Markus

Re: tracing/probes: Fix a memory leak in traceprobe_parse_probe_arg_body()

[Markus Elfring]

…
> 
>  If traceprobe_parse_probe_arg_body() fails to allocate 'parg->fmt', it
>  jumps to 'out' instead of 'fail' by mistake. In the result, in this
>  case the 'tmp' buffer is not freed and leaks its memory.
>
>  Fix it by jumping to 'fail' in that case.
> 
…

How do you think about another approach for a more desirable change description?

   If traceprobe_parse_probe_arg_body() failed to allocate the object 
“parg->fmt”,
   it jumps to the label “out” instead of “fail” by mistake.
   In the result, the buffer “tmp” is not freed in this case and leaks its 
memory.

   Thus jump to the label “fail” in that case (so that the exception handling
   will be improved another bit).


Regards,
Markus

Re: [PATCH] tracing/probes: Fix memory leak issues in traceprobe_parse_probe_arg_body()

[Markus Elfring]

…
> Therefore, the program should jump to the fail label instead of the out 
> label. This commit fixes this bug.
>
> Signed-off-by: LuMingYin <11570291+yin-lum...@user.noreply.gitee.com>

Please improve your patch attempt considerably.

See also:
https://kernelnewbies.org/FirstKernelPatch


Are there further change opportunities to take better into account?
https://elixir.bootlin.com/linux/v6.9-rc5/source/kernel/trace/trace_probe.c#L1403

Regards,
Markus

Re: [PATCH v2] ftrace: Fix possible use-after-free issue in ftrace_location()

[Markus Elfring]

…
> To fix it, we hold rcu lock as lookuping ftrace record, and call
> synchronize_rcu() before freeing any ftrace pages.

I suggest to convert this description into an imperative wording.

Regards,
Markus

Re: [v2] ice: Fix freeing uninitialized pointers

[Markus Elfring]

>>> Automatically cleaned up pointers need to be initialized before exiting
>>> their scope.  In this case, they need to be initialized to NULL before
>>> any return statement.
>>
>> * May we expect that compilers should report that affected variables
>>   were only declared here instead of appropriately defined
>>   (despite of attempts for scope-based resource management)?
>>
>
> We disabled GCC's check for uninitialized variables a long time ago
> because it had too many false positives.

Can further case distinctions (and compilation parameters) become more helpful
according to the discussed handling of the attribute “__cleanup” (or “__free”)?


>> * Did you extend detection support in the source code analysis tool “Smatch”
>>   for a questionable implementation detail?
>
> Yes.  Smatch detects this as an uninitialized variable.

Does the corresponding warning indicate requirements for scope-based resource 
management?

Regards,
Markus

Re: [PATCH v2 net] ice: Fix freeing uninitialized pointers

[Markus Elfring]

> Automatically cleaned up pointers need to be initialized before exiting
> their scope.  In this case, they need to be initialized to NULL before
> any return statement.

* May we expect that compilers should report that affected variables
  were only declared here instead of appropriately defined
  (despite of attempts for scope-based resource management)?

* Did you extend detection support in the source code analysis tool “Smatch”
  for a questionable implementation detail?


Regards,
Markus

Re: [2/2] virtiofs: Improve error handling in virtio_fs_get_tree()

[Markus Elfring]

>> It is probably clear that the function call “kfree(NULL)” does not perform
>> data processing which is really useful for the caller.
>> Such a call is kept in some cases because programmers did not like to invest
>> development resources for its avoidance.
>
> on the contrary, it is extremely useful for callers to not have to perform
> the NULL check themselves.

Some function calls indicate a resource allocation failure by a null pointer.
Such pointer checks are generally performed for error detection
so that appropriate exception handling can be chosen.


> It also mirrors userspace where free(NULL)
> is valid according to ISO/ANSI C, so eases the transition for programmers
> who are coming from userspace.  It costs nothing in the implementation
> as it is part of the check for the ZERO_PTR.

How many development efforts do you dare to spend on more complete
and efficient error/exception handling?

Regards,
Markus

Re: [2/2] virtiofs: Improve error handling in virtio_fs_get_tree()

[Markus Elfring]

> Do you consider more clarity in your argumentation?

It is probably clear that the function call “kfree(NULL)” does not perform
data processing which is really useful for the caller.
Such a call is kept in some cases because programmers did not like to invest
development resources for its avoidance.

Regards,
Markus

Re: [2/2] virtiofs: Improve error handling in virtio_fs_get_tree()

[Markus Elfring]

>>> So what?  kfree(NULL) is perfectly acceptable.
>>
>> I suggest to reconsider the usefulness of such a special function call.
>
> Can you be more explicit in your suggestion?

I hope that the change acceptance can grow for the presented transformation.
Are you looking for an improved patch description?

Regards,
Markus

Re: [PATCH 2/2] virtiofs: Improve error handling in virtio_fs_get_tree()

[Markus Elfring]

>> The kfree() function was called in two cases by
>> the virtio_fs_get_tree() function during error handling
>> even if the passed variable contained a null pointer.
>
> So what?  kfree(NULL) is perfectly acceptable.

I suggest to reconsider the usefulness of such a special function call.


> Are you trying to optimise an error path?

I would appreciate if further improvements can be achieved.
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources

Regards,
Markus

[PATCH 2/2] virtiofs: Improve error handling in virtio_fs_get_tree()

[Markus Elfring]

From: Markus Elfring 
Date: Fri, 29 Dec 2023 09:15:07 +0100

The kfree() function was called in two cases by
the virtio_fs_get_tree() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.

* Thus use another label.

* Move an error code assignment into an if branch.

* Delete an initialisation (for the variable “fc”)
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 fs/fuse/virtio_fs.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 2f8ba9254c1e..0746f54ec743 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1415,10 +1415,10 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
 {
struct virtio_fs *fs;
struct super_block *sb;
-   struct fuse_conn *fc = NULL;
+   struct fuse_conn *fc;
struct fuse_mount *fm;
unsigned int virtqueue_size;
-   int err = -EIO;
+   int err;

/* This gets a reference on virtio_fs object. This ptr gets installed
 * in fc->iq->priv. Once fuse_conn is going away, it calls ->put()
@@ -1431,13 +1431,15 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
}

virtqueue_size = virtqueue_get_vring_size(fs->vqs[VQ_REQUEST].vq);
-   if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD))
-   goto out_err;
+   if (WARN_ON(virtqueue_size <= FUSE_HEADER_OVERHEAD)) {
+   err = -EIO;
+   goto lock_mutex;
+   }

err = -ENOMEM;
fc = kzalloc(sizeof(*fc), GFP_KERNEL);
if (!fc)
-   goto out_err;
+   goto lock_mutex;

fm = kzalloc(sizeof(*fm), GFP_KERNEL);
if (!fm)
@@ -1476,6 +1478,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc)

 out_err:
kfree(fc);
+lock_mutex:
mutex_lock(&virtio_fs_mutex);
virtio_fs_put(fs);
mutex_unlock(&virtio_fs_mutex);
--
2.43.0

[PATCH 1/2] virtiofs: Improve three size determinations

[Markus Elfring]

From: Markus Elfring 
Date: Fri, 29 Dec 2023 08:42:04 +0100

Replace the specification of data structures by pointer dereferences
as the parameter for the operator “sizeof” to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 fs/fuse/virtio_fs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 5f1be1da92ce..2f8ba9254c1e 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -1435,11 +1435,11 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
goto out_err;

err = -ENOMEM;
-   fc = kzalloc(sizeof(struct fuse_conn), GFP_KERNEL);
+   fc = kzalloc(sizeof(*fc), GFP_KERNEL);
if (!fc)
goto out_err;

-   fm = kzalloc(sizeof(struct fuse_mount), GFP_KERNEL);
+   fm = kzalloc(sizeof(*fm), GFP_KERNEL);
if (!fm)
goto out_err;

@@ -1495,7 +1495,7 @@ static int virtio_fs_init_fs_context(struct fs_context 
*fsc)
if (fsc->purpose == FS_CONTEXT_FOR_SUBMOUNT)
return fuse_init_fs_context_submount(fsc);

-   ctx = kzalloc(sizeof(struct fuse_fs_context), GFP_KERNEL);
+   ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;
fsc->fs_private = ctx;
--
2.43.0

[PATCH 0/2] virtiofs: Adjustments for two function implementations

[Markus Elfring]

From: Markus Elfring 
Date: Fri, 29 Dec 2023 09:28:09 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Improve three size determinations
  Improve error handling in virtio_fs_get_tree()

 fs/fuse/virtio_fs.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

--
2.43.0

Re: [v5 0/1] dax: enable dax fault handler to report VM_FAULT_HWPOISON

[Markus Elfring]

>> Would you insist on the usage of cover letters also for single patches?
>
> I would neither insist on it, nor prohibit it.

It seems that you can tolerate an extra message here.


> It simply does not make enough difference.

Can it occasionally be a bit nicer to receive all relevant information within a 
single patch
instead of a combination of two messages?

Regards,
Markus

Re: [v5 0/1] dax: enable dax fault handler to report VM_FAULT_HWPOISON

[Markus Elfring]

>> How do you think about to put additional information below triple dashes
>> (or even into improved change descriptions)?
>>
>> See also:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n686
>
> Markus,
>
> Please go away.  Your feedback is not helpful.

Would you insist on the usage of cover letters also for single patches?

Regards,
Markus

Re: [v5 0/1] dax: enable dax fault handler to report VM_FAULT_HWPOISON

[Markus Elfring]

> The thought was to put descriptions unsuitable for commit header in the cover 
> letter.

How do you think about to put additional information below triple dashes
(or even into improved change descriptions)?

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n686

Regards,
Markus

Re: [PATCH v5 0/1] dax: enable dax fault handler to report VM_FAULT_HWPOISON

[Markus Elfring]

> Change from v4:
…

I suggest to omit the cover letter for a single patch.

Will any patch series evolve for your proposed changes?

Regards,
Markus

Re: nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

[Markus Elfring]

> FYI - I'm a tiny bit taken aback that in response to me applying,
> and providing feedback, on your patch,

This will probably trigger collateral evolution, won't it?


> you respond with 2 links for me to follow

I offered another bit of background information according to your enquiry.


> and cut off a chunk of my feedback.

Will this part become relevant for a subsequent patch?


> Seems like it would taken the same amount of time to just answer my
> two questions directly.

Do you find linked information sources also helpful?


> Was this part of a larger patch set?

Not for this software module.

But one of my scripts for the semantic patch language pointed several update 
candidates out.
Thus I sent 19 patches according to these change possibilities so far.
(Would you become interested to take another look by the means of mailing list 
archives?)


> Andy's comment seems to indicate that.

Andy Shevchenko was informed because he is involved also in the evolution of 
other components.


>>> What is the risk of undefined behavior?
>>
>> See also:
>> https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504137#comment-405504137
>
> Where is the NULL pointer dereference here?

I hope that you can become more aware that access attempts for data structure 
members
(also by using the arrow operator) can occasionally be problematic before null 
pointer checks.



 This issue was detected by using the Coccinelle software.
>>> Which cocci script?
>>
>> See also:
>> Reconsidering pointer dereferences before null pointer checks (with SmPL)
>> https://lore.kernel.org/cocci/1a11455f-ab57-dce0-1677-6beb8492a...@web.de/
>> https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00021.html
>
> The cocci script linked above does not seem to apply here.

Which command did you try out?

Do you find the following data processing result reasonable?

Markus_Elfring@Sonne:…/Projekte/Linux/next-analyses> spatch 
…/Projekte/Coccinelle/janitor/show_pointer_dereferences_before_check7.cocci 
drivers/nvdimm/pfn_devs.c
…
@@ -456,9 +456,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pf
unsigned long align, start_pad;
struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
struct nd_namespace_common *ndns = nd_pfn->ndns;
-   const uuid_t *parent_uuid = nd_dev_to_uuid(&ndns->dev);

-   if (!pfn_sb || !ndns)
return -ENODEV;

if (!is_memory(nd_pfn->dev.parent))


Regards,
Markus

Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

[Markus Elfring]

>> The address of a data structure member was determined before
>> a corresponding null pointer check in the implementation of
>> the function “nd_pfn_validate”.
>>
>> Thus avoid the risk for undefined behaviour by replacing the usage of
>> the local variable “parent_uuid” by a direct function call within
>> a later condition check.
>
> Hi Markus,
>
> I think I understand what you are saying above, but I don't follow
> how that applies here. This change seems to be a nice simplification,
> parent_uuid, is used once, just grab it when needed.

Thanks for your positive feedback.


> What is the risk of undefined behavior?

See also:
https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504137#comment-405504137


>> This issue was detected by using the Coccinelle software.
> Which cocci script?

See also:
Reconsidering pointer dereferences before null pointer checks (with SmPL)
https://lore.kernel.org/cocci/1a11455f-ab57-dce0-1677-6beb8492a...@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00021.html


How do you think about to review and improve any similarly affected software 
components?

Regards,
Markus

[PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()

[Markus Elfring]

Date: Fri, 14 Apr 2023 12:01:15 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “nd_pfn_validate”.

Thus avoid the risk for undefined behaviour by replacing the usage of
the local variable “parent_uuid” by a direct function call within
a later condition check.

This issue was detected by using the Coccinelle software.

Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid 
helpers")
Signed-off-by: Markus Elfring 
---
 drivers/nvdimm/pfn_devs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index af7d9301520c..f14cbfa500ed 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -456,7 +456,6 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
unsigned long align, start_pad;
struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
struct nd_namespace_common *ndns = nd_pfn->ndns;
-   const uuid_t *parent_uuid = nd_dev_to_uuid(&ndns->dev);

if (!pfn_sb || !ndns)
return -ENODEV;
@@ -476,7 +475,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char *sig)
return -ENODEV;
pfn_sb->checksum = cpu_to_le64(checksum);

-   if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
+   if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(&ndns->dev), 16) != 0)
return -ENODEV;

if (__le16_to_cpu(pfn_sb->version_minor) < 1) {
--
2.40.0

Re: [PATCH] net/mlx5e: fix bpf_prog reference count leaks in mlx5e_alloc_rq

[Markus Elfring]

…
> The refcount leak issues take place in two error handling paths.

Can an other wording be a bit nicer for the commit message?


> Fix the issue by …

I suggest to replace this wording by the tag “Fixes”.

Regards,
Markus

Re: [PATCH V2] dmaengine: bcm-sba-raid: add missing put_device() call in sba_probe()

[Markus Elfring]

> if of_find_device_by_node() succeed, sba_probe() doesn't have a
> corresponding put_device(). …

Wording adjustment:
  If a of_find_device_by_node() call succeeded, sba_probe() did not
  contain a corresponding put_device() call. …

Regards,
Markus

Re: [PATCH] atm: Fix atm_dev reference count leaks in atmtcp_remove_persistent()

[Markus Elfring]

…
> The refcount leaks issues occur in two error handling paths.

Can it be nicer to use the term “reference count” for the commit message?


> Fix the issue by …

I suggest to replace this wording by the tag “Fixes”.


…
> +++ b/drivers/atm/atmtcp.c
> @@ -433,9 +433,15 @@  static int atmtcp_remove_persistent(int itf)
>   return -EMEDIUMTYPE;
>   }
>   dev_data = PRIV(dev);
> - if (!dev_data->persist) return 0;
> + if (!dev_data->persist) {
> + atm_dev_put(dev);
> + return 0;
> + }
…

I propose to add a jump target for the desired exception handling
in this function implementation.

+   if (!dev_data->persist)
+   goto put_device;


Regards,
Markus

Re: [PATCH] ethernet: fix potential memory leak in gemini_ethernet_port_probe()

[Markus Elfring]

> If some processes in gemini_ethernet_port_probe() fail,
> free_netdev(dev) needs to be called to avoid a memory leak.

Would you like to use an imperative wording for this change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=6ba1b005ffc388c2aeaddae20da29e4810dea298#n151


…
> +++ b/drivers/net/ethernet/cortina/gemini.c
> @@ -2501,8 +2513,10 @@  static int gemini_ethernet_port_probe(struct 
> platform_device *pdev)
>   IRQF_SHARED,
>   port_names[port->id],
>   port);
> - if (ret)
> + if (ret) {
> + free_netdev(netdev);
>   return ret;
> + }
>
>   ret = register_netdev(netdev);
…

I suggest to add a jump target for the desired exception handling
in this function implementation.

if (ret)
-   return ret;
+   goto free_netdev;

Regards,
Markus

Re: [PATCH] net: nixge: fix potential memory leak in nixge_probe()

[Markus Elfring]

> If some processes in nixge_probe() fail, free_netdev(dev)
> needs to be called to aviod a memory leak.

* Would you like to avoid a typo in this change description?

* An imperative wording can be preferred here, can't it?
  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=6ba1b005ffc388c2aeaddae20da29e4810dea298#n151

Regards,
Markus

Re: [PATCH] ARM: zx: remove redundant dev_err call in zx296702_pd_probe()

[Markus Elfring]

…
> +++ b/arch/arm/mach-zx/zx296702-pm-domain.c
> @@ -170,7 +170,6 @@ static int zx296702_pd_probe(struct platform_device *pdev)
>
>   pcubase = devm_ioremap_resource(&pdev->dev, res);
>   if (IS_ERR(pcubase)) {
> - dev_err(&pdev->dev, "ioremap fail.\n");
>   return -EIO;
>   }

Would you like to delete also the curly brackets besides the error message?

Regards,
Markus

Re: [PATCH] m68k/amiga: Add missing platform_device_unregister() call in amiga_init_devices()

[Markus Elfring]

> Add the missing platform_device_unregister() before return
> from amiga_init_devices() in the error handling case.

Will the tag “Fixes” become helpful for the commit message?


…
> +++ b/arch/m68k/amiga/platform.c
> @@ -188,8 +188,10 @@  static int __init amiga_init_devices(void)
>   return PTR_ERR(pdev);
>   error = platform_device_add_data(pdev, &a1200_ide_pdata,
>sizeof(a1200_ide_pdata));
> - if (error)
> + if (error) {
> + platform_device_unregister(pdev);
>   return error;
> + }
>   }
…

I suggest to add a jump target for the desired exception handling.

if (error)
+   goto unregister_device;


Regards,
Markus

Re: [PATCH] usb: hso: check for return value in hso_serial_common_create()

[Markus Elfring]

> in case of an error tty_register_device_attr() returns ERR_PTR(),
> add IS_ERR() check

I suggest to improve this change description a bit.

Will the tag “Fixes” become helpful for the commit message?


…
> +++ b/drivers/net/usb/hso.c
…
> @@ -2311,6 +2313,7 @@  static int hso_serial_common_create(struct hso_serial 
> *serial, int num_urbs,
>   return 0;
>  exit:
>   hso_serial_tty_unregister(serial);
> +exit2:
>   hso_serial_common_free(serial);
>   return -1;
>  }

Can other labels (like “unregister_serial” and “free_serial”) be preferred here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=92ed301919932f13b9172e525674157e983d#n485

Regards,
Markus

Re: [PATCH v3 0/3] Update memdup_user.cocci

[Markus Elfring]

…
> > Changes in v3:
> >  - add missing '-' for patch rule in kmalloc/kzalloc call args
> >  - selfcheck rule dropped from patchset
…
> All three applied. …

Will the software development discussion be continued by patches according to
previously mentioned ideas and remaining open issues?

Regards,
Markus

Re: [Cocci] [PATCH v3 0/3] Update memdup_user.cocci

[Markus Elfring]

> All three applied. …

Can the accepted software adjustment be seen by the interface
of a public development repository?

Regards,
Markus

Re: [PATCH v2] ARM: milbeaut: Add missing of_node_put() call in m10v_smp_init()

[Markus Elfring]

> The variable np in function m10v_smp_init takes the return value
> of of_find_compatible_node, which gets a node but does not put it.

Such information is useful.


> If this node is not put it may cause a memory leak.

Is the reference management generally improvable for this function 
implementation?


Will the tag “Fixes” become helpful for the commit message?

Regards,
Markus

Re: [PATCH] block: Fix reference count leak in blk_integrity_add

[Markus Elfring]

> kobject_init_and_add() takes reference even when it fails. If this
> function returns an error, kobject_put() must be called to properly
> clean up the memory associated with the object.

* An imperative wording can be preferred for the change description,
  can't it?

* Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] sh: sh4: Fix reference count leak in sq_dev_add

[Markus Elfring]

> kobject_init_and_add() takes reference even when it fails. If this
> function returns an error, kobject_put() must be called to properly
> clean up the memory associated with the object.

* An imperative wording can be preferred for the change description,
  can't it?

* Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] sh: sh4: Fix reference count leak in sq_dev_add

[Markus Elfring]

> kobject_init_and_add() takes reference even when it fails. If this
> function returns an error, kobject_put() must be called to properly
> clean up the memory associated with the object.

* An imperative wording can be preferred for the change description,
  can't it?

* Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] nbd: add missed destroy_workqueue when nbd_start_device fails

[Markus Elfring]

> destroy_workqueue() should be called to destroy ndev->tx_wq
> when nbd_start_device init resources fails.

* An imperative wording can be preferred for the change description,
  can't it?

* Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] RDMA/core: Fix return error value to negative in _ib_modify_qp()

[Markus Elfring]

Please add a change description (according to a missing minus character).

Regards,
Markus

Re: [PATCH] ipv6: Fix nexthop reference count in ip6_route_info_create()

[Markus Elfring]

…
> When ip6_route_info_create() returns, local variable "nh" becomes
> invalid, so the refcount should be decreased to keep refcount balanced.

Can it be helpful to use the term “reference count” in consistent ways?


> Fix this issue

How do you think about to replace this wording by the tag “Fixes”?


>by pulling up the error source routing handling when
> nexthops can not be used with source routing.

Does the movement of an if statement according to the check of the
data structure member “rt->fib6_src.plen” in an if branch really help here?

Regards,
Markus

Re: [PATCH] regulator: fix memory leak on error path of regulator_register()

[Markus Elfring]

> … introduced as a side ef another …

Would the following wording variant be more appropriate?

  … introduced as a side effect of another …


How do you think about to replace the wording “…, I believe …”
by an imperative description?

Regards,
Markus

Re: [f2fs-dev] [PATCH v2] f2fs: fix use-after-free issue in f2fs_put_super()

[Markus Elfring]

> During umount, …

Do you refer to the action “unmount” here?


> f2fs_destroy_segment_manager(), it may cause …

Wording adjustments:
f2fs_destroy_segment_manager(). It might cause …


> … with procfs accessing, …

Avoid another typo?:
… with procfs accesses, …


> …, fix it by …

Please replace this wording by the tag “Fixes”.

Regards,
Markus

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Markus Elfring]

> In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
> and unregisters the V4L2 device. But then this is done again by
> vpif_probe() itself. The patch removes the cleaning from
> vpif_probe_complete().

* An imperative wording can be preferred for the change description,
  can't it?

* Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] media: camss: fix memory leaks on error handling paths in probe

[Markus Elfring]

> camss_probe() does not free camss on error handling paths. The patch
> introduces an additional error label for this purpose.

* I suggest to use an imperative wording for the change description.

* Would you like to use also a jump target like the following
  at the end of this function implementation?

+e_nomem:
+   ret = -ENOMEM;
+   goto err_free;


* Will the tag “Fixes” become helpful for the commit message?


>Besides, it
> removes call of v4l2_async_notifier_cleanup() from
> camss_of_parse_ports() since its caller, camss_probe(), cleans up all
> its resources itself.

I propose to offer such a change by a separate update step.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=d15be546031cf65a0fc34879beca02fd90fe7ac7#n138


Regards,
Markus

Re: [PATCH] ARM: milbeaut: Add missing of_node_put()

[Markus Elfring]

> After finishing using device node got from of_find_compatible_node(),
> of_node_put() needs to be called.

* An imperative wording can be preferred for the change description,
  can't it?

* Will the tag “Fixes” become helpful for the commit message?

Regards,
Markus

Re: [PATCH] efi: add missed destroy_workqueue when efisubsys_init fails

[Markus Elfring]

> destroy_workqueue() should be called to destroy efi_rts_wq
> when efisubsys_init() init resources fails.

* Can such exception handling depend on the data structure member 
“efi.runtime_supported_mask”?

* An imperative wording would be preferred for the change description
  (besides another bit of fine-tuning), wouldn't it?

* Will the tag “Fixes” become helpful for the commit message?


…
> +++ b/drivers/firmware/efi/efi.c
> @@ -379,6 +379,7 @@ static int __init efisubsys_init(void)
>   efi_kobj = kobject_create_and_add("efi", firmware_kobj);
>   if (!efi_kobj) {
>   pr_err("efi: Firmware registration failed.\n");
> + destroy_workqueue(efi_rts_wq);
>   return -ENOMEM;
>   }

How do you think about to use the following statements instead
in the if branch?

-   return -ENOMEM;
+   error = -ENOMEM;
+   goto destroy_workqueue;


Regards,
Markus

Re: [PATCH] mailbox: pcc: Put the PCCT table for error path

[Markus Elfring]

…
> In acpi_pcc_probe(), the PCCT table entries will be used as private
> data for communication chan at runtime, but the table should be put
> for error path.

* An imperative wording can be preferred for the change description,
  can't it?

* Will the tag “Fixes” become helpful for the commit message?

Regards,
Markus

Re: [PATCH] drm/amdkfd: Put ACPI table after using it

[Markus Elfring]

…
> and in kfd_create_vcrat_image_cpu(), the acpi_table is only used to
> get the OEM info, so those table mappings need to be release after
…

1. Please avoid a typo for this change description.

2. An imperative wording can be preferred here, can't it?

3. Will the tag “Fixes” become helpful for the commit message?

Regards,
Markus

Re: [v3 2/3] coccinelle: api: extend memdup_user rule with vmemdup_user()

[Markus Elfring]

>> Under which circumstances would you begin to care more for involved 
>> differences
>> in corresponding run time characteristics?
>
> When the difference is hours vs seconds.

Such a view can be influenced by execution environments in considerable ways.


> In this case, there are tradeoffs between the two options,

I am trying to improve the awareness for possible software design variations.


> andI don't know which would be faster,

I am curious if more contributors would get into the mood to compare
data processing approaches a bit more.


> but I think it is extremely unlikely that the difference is noticible.

But the following effects may be known for the evaluation of a SmPL rule
which was combined for two code parts.

* The SmPL dependencies are checked only once.

* Can the source code search efforts be reduced a bit?


> But if you are concerned about it, you are welcome to try.

Will anybody become interested to measure the software behaviour exactly?

Regards,
Markus

Re: [v3 2/3] coccinelle: api: extend memdup_user rule with vmemdup_user()

[Markus Elfring]

> Markus, you are welcome to try this since you are concerned about it.

I dare to point software design variations for some reasons.


> But it doesn't matter.

Under which circumstances would you begin to care more for involved differences
in corresponding run time characteristics?

Regards,
Markus

Re: [PATCH v3 2/3] coccinelle: api: extend memdup_user rule with vmemdup_user()

[Markus Elfring]

>>> +@depends on patch@
>>> +expression from,to,size;
>>> +identifier l1,l2;
>>> +@@
>>> +
>>> +-  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
>>> ++  to = vmemdup_user(from,size);
>>
>> I propose to combine the desired adjustment with the previous SmPL rule
>> by using another disjunction.

How do you think about to check run time characteristics for
the following SmPL script sketches?

A)
@R1@
@@
// Change something

@R2@
@@
// Change another thing


B)
@Replacement_with_disjunction@
@@
(
// R1: Change something
|
// R2: Change another thing
)


>>> +@rv depends on !patch@
>>> +expression from,to,size;
>>> +position p;
>>> +statement S1,S2;
>>> +@@
>>> +
>>> +*  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
>>> +   if (to==NULL || ...) S1
>>> +   if (copy_from_user(to, from, size) != 0)
>>> +   S2
>>
>> * Can it be helpful to omit the SmPL asterisk functionality from
>>   the operation modes “org” and “report”?
>>
>> * Should the operation mode “context” work without an extra position 
>> metavariable?
>
> This is fine as is in all three aspects.

Is every technique from the Coccinelle software required for
each operation mode in such data processing approaches?

Regards,
Markus

Re: [v3 2/3] coccinelle: api: extend memdup_user rule with vmemdup_user()

[Markus Elfring]

> This is fine as is in all three aspects.

I have tried again to point specific data processing consequences out
for operation modes of scripts in the semantic patch language.

Regards,
Markus

Re: [PATCH v3 3/3] coccinelle: api: filter out memdup_user definitions

[Markus Elfring]

> Don't match memdup_user/vmemdup_user.

I find such a change description insufficient.

Regards,
Markus

Re: [PATCH v3 2/3] coccinelle: api: extend memdup_user rule with vmemdup_user()

[Markus Elfring]

…
> +++ b/scripts/coccinelle/api/memdup_user.cocci
> @@ -39,6 +39,28 @@ …
…
> +@depends on patch@
> +expression from,to,size;
> +identifier l1,l2;
> +@@
> +
> +-  to = \(kvmalloc\|kvzalloc\)(size,\(GFP_KERNEL\|GFP_USER\));
> ++  to = vmemdup_user(from,size);

I propose to combine the desired adjustment with the previous SmPL rule
by using another disjunction.


> +@rv depends on !patch@
> +expression from,to,size;
> +position p;
> +statement S1,S2;
> +@@
> +
> +*  to = \(kvmalloc@p\|kvzalloc@p\)(size,\(GFP_KERNEL\|GFP_USER\));
> +   if (to==NULL || ...) S1
> +   if (copy_from_user(to, from, size) != 0)
> +   S2

* Can it be helpful to omit the SmPL asterisk functionality from
  the operation modes “org” and “report”?

* Should the operation mode “context” work without an extra position 
metavariable?

Regards,
Markus

Re: [PATCH v3 1/3] coccinelle: api: extend memdup_user transformation with GFP_USER

[Markus Elfring]

> Match GFP_USER and optional __GFP_NOWARN allocations with
> memdup_user.cocci rule.

I suggest to clarify software design consequences according to such information
a bit more.


I find it helpful if you would have included also my email address directly
in the message field “To” or “Cc”.
Are there further reasons to consider for the extension of the recipient lists?


> +-  to = \(kmalloc\|kzalloc\)
> +-(size,\(GFP_KERNEL\|GFP_USER\|
> +-  \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));

* Would you ever dare to specify such a source code search pattern
  on a single line?

* I find the specification of SmPL disjunctions questionable
  for the determination of relevant flags.
  Could previous patch review trigger concerns and further considerations
  for the proper handling of optional source code parts?


> +*  to = \(kmalloc@p\|kzalloc@p\)
> + (size,\(GFP_KERNEL\|GFP_USER\|
> +   \(GFP_KERNEL\|GFP_USER\)|__GFP_NOWARN\));

Would you like to use the SmPL asterisk really only for a single line?


How will the chances evolve to continue the clarification also for
the open issue “Safer source code analysis by "memdup_user.cocci"”?
https://github.com/coccinelle/coccinelle/issues/78

Regards,
Markus

Re: [PATCH v2] drm/virtio: Fix memory leak in virtio_gpu_execbuffer_ioctl()

[Markus Elfring]

> … To balance the reference
> count initialized when allocating fence, dma_fence_put()
> should not be deleted.

* Would an imperative wording be more appropriate for the change description?

* Is the information “hexin” sufficient for a real name?

Regards,
Markus

Re: [PATCH] clk: ti: clkctrl: add the missed kfree() for _ti_omap4_clkctrl_setup()

[Markus Elfring]

…
> +++ b/drivers/clk/ti/clkctrl.c
> @@ -655,8 +655,10 @@ static void __init _ti_omap4_clkctrl_setup(struct 
> device_node *node)
>   }
>
>   hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> - if (!hw)
> + if (!hw) {
> + kfree(clkctrl_name);
>   return;
> + }
…

I suggest to use an additional label instead.

if (!hw)
-   return;
+   goto free_control_name;


By the way:
How do you think about to replace the label “cleanup” by other jump targets
for better exception handling in this function implementation?

Regards,
Markus

Re: [PATCH] clk: ti: clkctrl: add the missed kfree() for _ti_omap4_clkctrl_setup()

[Markus Elfring]

…
> +++ b/drivers/clk/ti/clkctrl.c
> @@ -655,8 +655,10 @@ static void __init _ti_omap4_clkctrl_setup(struct 
> device_node *node)
>   }
>
>   hw = kzalloc(sizeof(*hw), GFP_KERNEL);
> - if (!hw)
> + if (!hw) {
> + kfree(clkctrl_name);
>   return;
> + }
…

I suggest to use an additional label instead.

if (!hw)
-   return;
+   goto free_control_name;


By the way:
How do you think about to replace the label “cleanup” by other jump targets
for better exception handling in this function implementation?

Regards,
Markus

Re: [f2fs-dev] [PATCH] f2fs: compress: Avoid memory leak on cc->cpages

[Markus Elfring]

>>> Memory allocated for storing compressed pages' poitner should be
>>> released after f2fs_write_compressed_pages(), otherwise it will
>>> cause memory leak issue.
>>
>> * Would an imperative wording be more appropriate (without a typo)
>>   for the change description?
>>
>> * Will the tag “Fixes” become helpful for the commit message?
>
> It looks this is replied from patch-robot? since I found all comments
> you replied are almost the same.

I dare to repeat such suggestions because several patches contain
improvable details (not only in the affected source code).

Regards,
Markus

Re: [PATCH] ipmi: Remove duplicate code in __ipmi_bmc_register()

[Markus Elfring]

> > __ipmi_bmc_register() jumps to the label 'out_free_my_dev_name' in an
> > error path. So we can remove duplicate code in the if (rv).
>
> Looks correct, queued for next release.

1. Can an imperative wording be preferred for the change description?

2. Will the tag “Fixes” become helpful for the commit message?

3. Did you avoid a typo in the patch subject?

Regards,
Markus

Re: [PATCH] drm/virtio: Fix memory leak in virtio_gpu_execbuffer_ioctl()

[Markus Elfring]

* I suggest to add a change description.

* Is “hexin.op” a real name?

Regards,
Markus

Re: [f2fs-dev] [PATCH] f2fs: compress: Avoid memory leak on cc->cpages

[Markus Elfring]

> Memory allocated for storing compressed pages' poitner should be
> released after f2fs_write_compressed_pages(), otherwise it will
> cause memory leak issue.

* Would an imperative wording be more appropriate (without a typo)
  for the change description?

* Will the tag “Fixes” become helpful for the commit message?

Regards,
Markus

Re: [PATCH] ath11k: Fix memory leak in ath11k_qmi_init_service()

[Markus Elfring]

> When qmi_add_lookup fail, we should destroy the workqueue

Can the following wording variant be nicer for the change description?

   Destroy the work queue object in an if branch
   after a call of the function “qmi_add_lookup” failed.

Regards,
Markus

Re: [v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Markus Elfring]

>> you do indeed need to put - in front of the second and third lines as well.
>
> Thanks, Markus, Julia. I will send v3.

How do you think about to discuss any remaining open issues already
on the current software development version for your transformation approach?

Regards,
Markus

Re: [PATCH] mt76: mt76u: add missing release on skb in __mt76x02u_mcu_send_msg

[Markus Elfring]

…
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
…
> @@ -111,6 +113,7 @@ __mt76x02u_mcu_send_msg(struct mt76_dev *dev, struct 
> sk_buff *skb,
>   if (wait_resp)
>   ret = mt76x02u_mcu_wait_resp(dev, seq);
>
> +out:
>   consume_skb(skb);
…

I suggest to use the label “consume_skb”.

Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] mt7601u: add missing release on skb in mt7601u_mcu_msg_send

[Markus Elfring]

…
> +++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
> @@ -116,8 +116,10 @@ mt7601u_mcu_msg_send(struct mt7601u_dev *dev, struct 
> sk_buff *skb,
>   int sent, ret;
>   u8 seq = 0;
>
> - if (test_bit(MT7601U_STATE_REMOVED, &dev->state))
> + if (test_bit(MT7601U_STATE_REMOVED, &dev->state)) {
> + consume_skb(skb);
>   return 0;
> + }
…

How do you think about to use the the following statements instead
in the if branch?

ret = 0;
goto consume_skb;


Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Markus Elfring]

> You proposed essentially  \( A \| B \) \( | C \| \)

Will the software development attention grow for a topic
like “Extend support for handling of optional source code parts”?
https://github.com/coccinelle/coccinelle/issues/53

How do you think about another tiny example script for the semantic patch 
language?

@display@
constant A, B;
@@
*A
?| B


elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-cocci 
optional_disjunction_test3-20200718.cocci
…
warning: incompatible arity found on line 4
incompatible minus and plus code starting on lines 4 and 4


Regards,
Markus

Re: [v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Markus Elfring]

 * 
 https://lore.kernel.org/cocci/5c0dae88-e172-3ba6-f86c-d1a6238bb...@web.de/
   https://lkml.org/lkml/2020/6/9/568
>>>
>>> This one it complete nonsense.
>>
>> I hope that different views can be clarified for such a software situation
>> in more constructive ways.
>
> You proposed essentially  \( A \| B \) \( | C \| \)

I suggested also another adjustment.

Can additional minus characters be avoided if such a source code search pattern
would be specified in a single line?


> This is not valid syntax in the semantic patch language.

I hope that a solution can be found by our discussion.


> The branches of a \( \| \) have to be a valid expression, statement, type, 
> etc,

Such information can become more interesting for safe application of
SmPL disjunctions.


> not some random string of tokens.

I got further imaginations in this software area.

Will the handling of optional transformation parameters be clarified better?


>> Patch reviews contain usual risks that suggestions are presented
>> which can be still questionable.
>
> These are not "usual risks".  You can easily test out your suggestion by
> yourself to see if it produces valid code.

Such an expectation can be reasonable in some cases.


> If it doesn't, then don't make the suggestion.

Would software limitations hinder any more improvements then?


>>> like that putting all of the virtual declarations on
>>> the same line would save space (it does, but who cares),
>>
>> It seems that you admit a possibly desirable effect.
>
> No, I don't consider the effect to be desirable.

I propose to take another look at variations around source code verbosity.


>> Your change acceptance is varying to your development mood
>> (and other factors), isn't it?
>
> Not really.  My "change acceptance" increases when the person reporting
> them raises real problems that is blocking them in some work.

I presented open issues accordingly.


> And it decreases rapidly when the changes are almost all related to presumed
> "efficiencies" that have no impact in practice.

Change possibilities can get varying attention and corresponding development 
priorities.

Regards,
Markus

Re: [v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Markus Elfring]

>>> Applied.
>>
>> Do you care for patch review concerns according to this SmPL script 
>> adjustment?
>>
>> * https://lore.kernel.org/cocci/5c0dae88-e172-3ba6-f86c-d1a6238bb...@web.de/
>>   https://lkml.org/lkml/2020/6/9/568
>
> This one it complete nonsense.

I hope that different views can be clarified for such a software situation
in more constructive ways.


>> * https://lore.kernel.org/cocci/c3464cad-e567-9ef5-b4e3-a01e3b111...@web.de/
>>   https://lkml.org/lkml/2020/6/8/637
>
> This on is indeed a problem.

I find this feedback interesting.

* How does it fit to your response “Applied”?

* Will it trigger any more consequences?


> Markus, if you would limit your comments to suggesting SmPL code
> that is actually correct, ie that you have tested,

Patch reviews contain usual risks that suggestions are presented
which can be still questionable.


> and 2) stop suggesting stupid things over and over

We come along different development views.


> like that putting all of the virtual declarations on
> the same line would save space (it does, but who cares),

It seems that you admit a possibly desirable effect.
Will any more developers care also for SmPL coding style aspects?


> then I would take your suggestions more seriously.

Your change acceptance is varying to your development mood
(and other factors), isn't it?

Regards,
Markus

Re: [v2 1/4] coccinelle: api: extend memdup_user transformation with GFP_USER

[Markus Elfring]

> Applied.

Do you care for patch review concerns according to this SmPL script adjustment?

* https://lore.kernel.org/cocci/5c0dae88-e172-3ba6-f86c-d1a6238bb...@web.de/
  https://lkml.org/lkml/2020/6/9/568

* https://lore.kernel.org/cocci/c3464cad-e567-9ef5-b4e3-a01e3b111...@web.de/
  https://lkml.org/lkml/2020/6/8/637

Regards,
Markus

Re: [PATCH] net: cxgb3: add missed destroy_workqueue in cxgb3 probe failure

[Markus Elfring]

> Add the missed calls to fix it.

You propose to add only a single function call.


…
> +++ b/drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c
> @@ -3407,6 +3407,7 @@ static int init_one(struct pci_dev *pdev, const struct 
> pci_device_id *ent)
>  out_disable_device:
>   pci_disable_device(pdev);
>  out:
> + destroy_workqueue(cxgb3_wq);
>   return err;
>  }

I suggest to adjust also the usage of the label “out” accordingly.

Regards,
Markus

Re: [PATCH v4 1/2] drivers: provide devm_platform_request_irq()

[Markus Elfring]

> v3 -> v4:
>   - The patch v3 sent on May 27 may be lost somewhere in the
> world, so resend it.

Can previous patch review aspects get any more attention?

https://lore.kernel.org/cocci/5dad9b19-ceb5-1606-9f62-7626e5677...@web.de/
https://lkml.org/lkml/2020/5/27/1326

Regards,
Markus

Re: [PATCH v4] coccinelle: api: add kzfree script

[Markus Elfring]

I dare to repeat previous patch review aspects once more.
https://lore.kernel.org/cocci/a316f076-1686-25d8-18fe-1bbc0cf9a...@web.de/

…
> +virtual context
> +virtual patch
> +virtual org
> +virtual report

+virtual context, patch, org, report

Is such a SmPL code variant more succinct?


…
> +if (...)
> +  \(memset@ok\|memzero_explicit@ok\)(...);

Would you like to tolerate any extra source code around such a function call
in an if branch?


…
> +(
> +* memset@m((T)E, 0, ...);
> +|
> +* memzero_explicit@m((T)E, ...);
> +)
…

I suggest to move a semicolon.

+(
+*memset@m((T)E, 0, ...)
+|
+*memzero_explicit@m((T)E, ...)
+);


…
> +- \(kfree\|vfree\|kvfree\)(E);
> ++ kvfree_sensitive(E, size);
…

Would you like to increase the precision a bit for the change specification?

+-\(kfree\|vfree\|kvfree\)
++kvfree_sensitive
+ (E
++ , size
+ );


…
> +(
> +- kfree(E);
> ++ kzfree(E);
> +|
> +- \(vfree\|kvfree\)(E);
> ++ kvfree_sensitive(E, size);
> +)
…

+(
+-kfree
++kzfree
+  (E)
+|
+-\(vfree\|kvfree\)
++kvfree_sensitive
+ (E
++ , size
+ )
+);


…
> +// TODO: uncomment when kfree_sensitive will be merged.
> +// Only this case is commented out because developers
> +// may not like patches like this since kzfree uses memset
> +// internally (not memzero_explicit).

Will this information trigger any further clarification?


…
> +coccilib.org.print_todo(p[0],
> +  "WARNING: opportunity for kzfree/kvfree_sensitive")

I propose to align the second function parameter.

+coccilib.org.print_todo(p[0],
+"WARNING: opportunity for kzfree/kvfree_sensitive")


Regards,
Markus

Re: [PATCH v2] nfc: nci: add missed destroy_workqueue in nci_register_device

[Markus Elfring]

> When nfc_register_device fails in nci_register_device,
> destroy_workqueue() shouled be called to destroy ndev->tx_wq.

Would an other imperative wording be preferred for the commit message?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=f8456690ba8eb18ea4714e68554e242a04f65cff#n151


…
> +++ b/net/nfc/nci/core.c
> @@ -1228,10 +1228,13 @@  int nci_register_device(struct nci_dev *ndev)
>
>   rc = nfc_register_device(ndev->nfc_dev);
>   if (rc)
> - goto destroy_rx_wq_exit;
> + goto destroy_tx_wq_exit;
>
>   goto exit;
>
> +destroy_tx_wq_exit:
> + destroy_workqueue(ndev->tx_wq);
…


How do you think about to use the following source code variant
for this function implementation?

if (!rc)
goto exit;

destroy_workqueue(ndev->tx_wq);


Regards,
Markus

Re: [PATCH v4 1/8] irqchip/loongson-htpic: Remove redundant kfree operation

[Markus Elfring]

> In the function htpic_of_init(), when kzalloc htpic fails, it should
> return -ENOMEM directly, no need to execute "goto" to kfree.

Would another imperative wording be preferred for the commit message?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=f8456690ba8eb18ea4714e68554e242a04f65cff#n151

Regards,
Markus

Re: [PATCH] net: smc91x: Fix possible memory leak in smc_drv_probe()

[Markus Elfring]

> If try_toggle_control_gpio() failed in smc_drv_probe(), free_netdev(ndev)
> should be called to free the ndev created earlier. Otherwise, a memleak
> will occur.

* Will it be nicer to use the term “memory leak” also in this change 
description?

* Would another imperative wording be preferred for the commit message?
  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=f8456690ba8eb18ea4714e68554e242a04f65cff#n151

Regards,
Markus

Re: [PATCH v2] tipc: Don't using smp_processor_id() in preemptible code

[Markus Elfring]

* I find the patch subject improvable.

* Please add a change description for the desired commit.

Regards,
Markus

Re: [PATCH] drm/nouveau: add the missed kfree() for nouveau_bo_alloc()

[Markus Elfring]

> … to fix it.

I suggest to replace this wording by the tag “Fixes”.


…
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -276,8 +276,10 @@  nouveau_bo_alloc(struct nouveau_cli *cli, u64 *size, 
> int *align, u32 flags,
>   break;
>   }
>
> - if (WARN_ON(pi < 0))
> + if (WARN_ON(pi < 0)) {
> + kfree(nvbo);
>   return ERR_PTR(-EINVAL);
> + }

I propose to move such common exception handling to the end of
this function implementation so that a bit of duplicate code will be avoided.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=e9919e11e219eaa5e8041b7b1a196839143e9125#n481

Regards,
Markus

Re: [PATCH] net: xilinx: fix potential NULL dereference in temac_probe()

[Markus Elfring]

> If you use devm_ioremap_resource() you can remove the !res check
> entirely which would be equally acceptable as a fix.

Would you like to use the wrapper function 
“devm_platform_get_and_ioremap_resource” then?
https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/base/platform.c#L66
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/platform.c?id=0dc589da873b58b70f4caf4b070fb0cf70fdd1dc#n66

Regards,
Markus

Re: [PATCH 2/5] spi: lpspi: add NULL check when probe device

[Markus Elfring]

…
> +++ b/drivers/spi/spi-fsl-lpspi.c
> @@ -841,6 +841,11 @@ static int fsl_lpspi_probe(struct platform_device *pdev)
>   u32 temp;
>   bool is_slave;
>
> + if (!np && !lpspi_platform_info) {
> + dev_err(&pdev->dev, "can't get the platform data\n");
> + return -EINVAL;
> + }
…

How do you think about to combine these null pointer checks by the logical
operator “or” instead of “and”?

Regards,
Markus

Re: [PATCH 2/2] Crypto/chcr: Fix some pr_xxx messages

[Markus Elfring]

…
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -1224,7 +1224,7 @@ static int chcr_handle_cipher_resp(struct 
> skcipher_request *req,
>   wrparam.bytes = bytes;
>   skb = create_cipher_wr(&wrparam);
>   if (IS_ERR(skb)) {
> - pr_err("chcr : %s : Failed to form WR. No memory\n", __func__);
> + pr_err("%s : Failed to form WR. No memory\n", __func__);
>   err = PTR_ERR(skb);
>   goto unmap;
>   }

I suggest to omit also a space character before the colon in such format 
strings.

+   pr_err("%s: Failed to form WR. No memory\n", __func__);


Would you like to apply any further fine-tuning around affected error messages?

Regards,
Markus

Re: [PATCH 1/2] Crypto/chcr: Avoid some code duplication

[Markus Elfring]

> The error handling path of 'chcr_authenc_setkey()' is the same as this
> error handling code.

I find this change description improvable.


> So just 'goto out' as done everywhere in the function to simplify the code.

I propose to adjust jump targets a bit more for better exception handling
in this function implementation.

Regards,
Markus

Re: [PATCH] RDMA/usnic: switch from 'pci_' to 'dma_' API

[Markus Elfring]

> The wrappers in include/linux/pci-dma-compat.h should go away.
>
> The patch has been generated with the coccinelle script bellow.

* I suggest to avoid a typo in a word at the sentence end.

* The script for the semantic patch language could be a bit nicer
  by the application of disjunctions instead of separate SmPL rules.
  Would you become interested in any further adjustments?


…
>@@
> expression e1, e2, e3;
> @@
> -pci_alloc_consistent(e1, e2, e3)
> +dma_alloc_coherent(&e1->dev, e2, e3, GFP_)

I find the last function parameter questionable.
An other flag was specified in a corresponding diff hunk.

Regards,
Markus

Re: [PATCH] rsxx: switch from 'pci_free_consistent()' to 'dma_free_coherent()'

[Markus Elfring]

> The wrappers in include/linux/pci-dma-compat.h should go away.
>
> The patch has been generated with the coccinelle script bellow.

* I suggest to avoid a typo in a word at the sentence end.

* The script for the semantic patch language could be a bit nicer
  by the application of a disjunction instead of separate SmPL rules.
  Would you become interested in any further adjustments?

Regards,
Markus

Re: scsi: virtio_scsi: Remove unnecessary condition checks

[Markus Elfring]

>> Can a bit more “compliance” (with the Linux coding style) matter here?
>
> No.

Please take another look at the corresponding software documentation.


>>> Having a single error loop is an advantage by itself.
>>
>> I do not see that a loop is involved in the implementation of the function 
>> “init”.
>
> s/loop/label/ sorry.

Thanks for this information.

Can the development attention grow for corresponding implementation details
around the specifcation of efficient exception handling?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=42f82040ee66db13525dc6f14b8559890b2f4c1c#n465

Regards,
Markus