Re: [PATCH] nvdimm/pmem: Fix an error handling path in 'pmem_attach_disk()'

[Marion & Christophe JAILLET]

Le 07/11/2021 à 18:25, Marion & Christophe JAILLET a écrit :



Le 07/11/2021 à 18:20, Christophe JAILLET a écrit :

Le 07/11/2021 à 18:11, Ira Weiny a écrit :

On Sat, Nov 06, 2021 at 06:27:11PM +0100, Christophe JAILLET wrote:
If 'devm_init_badblocks()' fails, a previous 'blk_alloc_disk()' call 
must

be undone.


I think this is a problem...



Signed-off-by: Christophe JAILLET 
---
This patch is speculative. Several fixes on error handling paths 
have been

done recently, but this one has been left as-is. There was maybe a good
reason that I have missed for that. So review with care!

I've not been able to identify a Fixes tag that please me :(
---
  drivers/nvdimm/pmem.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index fe7ece1534e1..c37a1e6750b3 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -490,8 +490,9 @@ static int pmem_attach_disk(struct device *dev,
  nvdimm_namespace_disk_name(ndns, disk->disk_name);
  set_capacity(disk, (pmem->size - pmem->pfn_pad - 
pmem->data_offset)

  / 512);
-    if (devm_init_badblocks(dev, >bb))
-    return -ENOMEM;
+    rc = devm_init_badblocks(dev, >bb);
+    if (rc)
+    goto out;


But I don't see this 'out' label in the function currently?  Was that 
part of

your patch missing?


Hi,
the patch is based on the latest linux-next.
See [1]. The 'out' label exists there and is already used.

In fact, I run an own-made coccinelle script which tries to spot 
mix-up between return and goto.
In this case, we have a 'return -ENOMEM' after a 'goto out' which 
looks spurious. Hence, my patch.


[1]:https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/nvdimm/pmem.c#n512 



Lol, the #n512 above is in fact another place that should be updated as 
well. I missed it and only fixed #n494!


In fact, no, line 512 should be left as-is. The clean-up wilol be made 
by 'pmem_release_disk()'.


The patch attached at the very first mail of this thread looks good to me.

CJ



CJ



CJ



Ira


  nvdimm_badblocks_populate(nd_region, >bb, _range);
  disk->bb = >bb;
--
2.30.2

Re: [PATCH] nvdimm/pmem: Fix an error handling path in 'pmem_attach_disk()'

[Marion & Christophe JAILLET]

Le 07/11/2021 à 18:20, Christophe JAILLET a écrit :

Le 07/11/2021 à 18:11, Ira Weiny a écrit :

On Sat, Nov 06, 2021 at 06:27:11PM +0100, Christophe JAILLET wrote:
If 'devm_init_badblocks()' fails, a previous 'blk_alloc_disk()' call 
must

be undone.


I think this is a problem...



Signed-off-by: Christophe JAILLET 
---
This patch is speculative. Several fixes on error handling paths have 
been

done recently, but this one has been left as-is. There was maybe a good
reason that I have missed for that. So review with care!

I've not been able to identify a Fixes tag that please me :(
---
  drivers/nvdimm/pmem.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index fe7ece1534e1..c37a1e6750b3 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -490,8 +490,9 @@ static int pmem_attach_disk(struct device *dev,
  nvdimm_namespace_disk_name(ndns, disk->disk_name);
  set_capacity(disk, (pmem->size - pmem->pfn_pad - 
pmem->data_offset)

  / 512);
-    if (devm_init_badblocks(dev, >bb))
-    return -ENOMEM;
+    rc = devm_init_badblocks(dev, >bb);
+    if (rc)
+    goto out;


But I don't see this 'out' label in the function currently?  Was that 
part of

your patch missing?


Hi,
the patch is based on the latest linux-next.
See [1]. The 'out' label exists there and is already used.

In fact, I run an own-made coccinelle script which tries to spot mix-up 
between return and goto.
In this case, we have a 'return -ENOMEM' after a 'goto out' which looks 
spurious. Hence, my patch.


[1]:https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/nvdimm/pmem.c#n512 



Lol, the #n512 above is in fact another place that should be updated as 
well. I missed it and only fixed #n494!


CJ



CJ



Ira


  nvdimm_badblocks_populate(nd_region, >bb, _range);
  disk->bb = >bb;
--
2.30.2

Re: [PATCH] irqdomain: Add documentation for irq_create_of_mapping()

[Marion & Christophe JAILLET]

Le 18/12/2020 à 19:59, Marc Zyngier a écrit :

Hi Christophe,

On Tue, 15 Dec 2020 20:07:47 +,
Christophe JAILLET  wrote:

Add a description for 'irq_create_of_mapping()' and make explicit the fact
that the resources allocated by this function can be freed by calling
'irq_dispose_mapping()' when needed (i.e. error handling path, remove
function, ...)

Signed-off-by: Christophe JAILLET 
---
The wording can certainly be improved.

My goal is only to make clear if patches such as:

https://lore.kernel.org/lkml/20201214202117.146293-1-christophe.jail...@wanadoo.fr/
are needed or not.
---
  kernel/irq/irqdomain.c | 9 +
  1 file changed, 9 insertions(+)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 6aacd342cd14..d761ece8d43e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -858,6 +858,15 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec 
*fwspec)
  }
  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
  
+/**

+ * irq_create_of_mapping() - Map an interrupt

I think this deserves a bit more work. My immediate questions when
reading this are "map where? and to what?".


I won't be of great help here.
I don't know this code enough to be able to provide an accurate description.


+ * @irq_data: structure of_phandle_args returned by a previous
+ * of_irq_parse_xxx() call

That's not strictly true. A of_phandle_args structure can be created
from scratch (and numerous drivers do that).


+ *
+ * The resources allocated by this function should be freed by
+ * calling irq_dispose_mapping() when the mapping if not useful
+ * anymore.

This really is a bit of documentation for irq_dispose_mapping(), isn't it?


Well, I don't agree.

I think it is easier to see that some resources need to be freed with a 
dedicated function if it is explained in the description of the function 
which allocates the resource.


CJ




+ */
  unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data)
  {
struct irq_fwspec fwspec;

Thanks,

M.

Re: [PATCH net] atl1c: fix error return code in atl1c_probe()

[Marion & Christophe JAILLET]

Le 17/11/2020 à 03:55, Zhang Changzhong a écrit :

Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.

Fixes: 85eb5bc33717 ("net: atheros: switch from 'pci_' to 'dma_' API")

Hi, should it have any importance, the Fixes tag is wrong.

The issue was already there before 85eb5bc33717 which was just a 
mechanical update.


just my 2c

CJ


Reported-by: Hulk Robot 
Signed-off-by: Zhang Changzhong 
---
  drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c 
b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 0c12cf7..3f65f2b 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -2543,8 +2543,8 @@ static int atl1c_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 * various kernel subsystems to support the mechanics required by a
 * fixed-high-32-bit system.
 */
-   if ((dma_set_mask(>dev, DMA_BIT_MASK(32)) != 0) ||
-   (dma_set_coherent_mask(>dev, DMA_BIT_MASK(32)) != 0)) {
+   err = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
+   if (err) {
dev_err(>dev, "No usable DMA configuration,aborting\n");
goto err_dma;
}

Re: [PATCH net] atl1e: fix error return code in atl1e_probe()

[Marion & Christophe JAILLET]

Le 17/11/2020 à 03:57, Zhang Changzhong a écrit :

Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.

Fixes: 85eb5bc33717 ("net: atheros: switch from 'pci_' to 'dma_' API")

Hi, should it have any importance, the Fixes tag is wrong.

The issue was already there before 85eb5bc33717 which was just a 
mechanical update.


just my 2c

CJ


Reported-by: Hulk Robot 
Signed-off-by: Zhang Changzhong 
---
  drivers/net/ethernet/atheros/atl1e/atl1e_main.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c 
b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
index 098b032..ff9f96d 100644
--- a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
+++ b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
@@ -2312,8 +2312,8 @@ static int atl1e_probe(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 * various kernel subsystems to support the mechanics required by a
 * fixed-high-32-bit system.
 */
-   if ((dma_set_mask(>dev, DMA_BIT_MASK(32)) != 0) ||
-   (dma_set_coherent_mask(>dev, DMA_BIT_MASK(32)) != 0)) {
+   err = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
+   if (err) {
dev_err(>dev, "No usable DMA configuration,aborting\n");
goto err_dma;
}

Re: [PATCH] drm: amdgpu: Use the correct size when allocating memory

[Marion & Christophe JAILLET]

Le 10/08/2020 à 17:42, Dan Carpenter a écrit :

On Sun, Aug 09, 2020 at 10:34:06PM +0200, Christophe JAILLET wrote:

When '*sgt' is allocated, we must allocated 'sizeof(**sgt)' bytes instead
of 'sizeof(*sg)'. 'sg' (i.e. struct scatterlist) is smaller than
'sgt' (i.e struct sg_table), so this could lead to memory corruption.

The sizeof(*sg) is bigger than sizeof(**sgt) so this wastes memory but
it won't lead to corruption.

 11  struct scatterlist {
 12  unsigned long   page_link;
 13  unsigned intoffset;
 14  unsigned intlength;
 15  dma_addr_t  dma_address;
 16  #ifdef CONFIG_NEED_SG_DMA_LENGTH
 17  unsigned intdma_length;
 18  #endif
 19  };

 42  struct sg_table {
 43  struct scatterlist *sgl;/* the list */
 44  unsigned int nents; /* number of mapped entries */
 45  unsigned int orig_nents;/* original size of list */
 46  };

regards,
dan carpenter



My bad. I read 'struct scatterlist sgl' (without the *)
Thanks for the follow-up, Dan.

Doesn't smatch catch such mismatch?
(I've not run smatch for a while, so it is maybe reported)

Well, the proposal is still valid, even if it has less impact as 
initially thought.


Thx for the review.

CJ

Re: [PATCH v2] dma-pool: Fix too large DMA pools on medium systems

[Marion & Christophe JAILLET]

Le 24/06/2020 à 09:38, Christoph Hellwig a écrit :

Hi Guenter,

can you try the patch below?  This just converts the huge allocations
in mptbase to use GFP_KERNEL.  Christophe (added to Cc) actually has
a scripted conversion for the rest that he hasn't posted yet, so I'll
aim for the minimal version here.


Hi,

I'm sorry, but I will not send pci_ --> dma_ conversion for this driver.
I'm a bit puzzled by some choice of GFP_KERNEL and GFP_ATOMIC that not 
all that obvious to me.


I'll try to send some patches for other easier drivers in the coming weeks.

Best regards,

CJ

Re: [PATCH V3] firmware: stratix10-svc: Fix some error handling code

[Marion & Christophe JAILLET]

Le 27/06/2020 à 07:15, Greg KH a écrit :

On Fri, Jun 26, 2020 at 09:37:20PM +0200, Christophe JAILLET wrote:

---
v2: takes Dan's comment into account and fix another resource leak.
v3: merge the previous 4 patches in a single one to ease review


No, 4 small patches are _MUCH_ easier to review than one larger one that
mixes everything together.  Who told you to put them together?


The cover letter of v2 serie can be found at [1].
The request for merging them in 1 patch is in [2].

V3, should be the same as v2, but all in one.

[1]: https://lkml.org/lkml/2020/4/29/77
[2]: https://lkml.org/lkml/2020/5/5/541

CJ

Re: [PATCH 5.7 318/477] pinctrl: freescale: imx: Use devm_of_iomap() to avoid a resource leak in case of error in imx_pinctrl_probe()

[Marion & Christophe JAILLET]

Hi,

This one must NOT be included. It generates a regression.
This should be removed from 5.4 as well.

See 13f2d25b951f139064ec2dd53c0c7ebdf8d8007e.

There is also a thread on ML about it. I couldn't find it right away, 
but I'm sure that Dan will be quicker than me for finding it, if needed ;-).


CJ

Le 23/06/2020 à 21:55, Greg Kroah-Hartman a écrit :

From: Christophe JAILLET 

[ Upstream commit ba403242615c2c99e27af7984b1650771a2cc2c9 ]

Use 'devm_of_iomap()' instead 'of_iomap()' to avoid a resource leak in
case of error.

Update the error handling code accordingly.

Fixes: 26d8cde5260b ("pinctrl: freescale: imx: add shared input select reg 
support")
Suggested-by: Dan Carpenter 
Signed-off-by: Christophe JAILLET 
Reviewed-by: Dan Carpenter 
Link: 
https://lore.kernel.org/r/20200602200626.677981-1-christophe.jail...@wanadoo.fr
Signed-off-by: Linus Walleij 
Signed-off-by: Sasha Levin 
---
  drivers/pinctrl/freescale/pinctrl-imx.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c 
b/drivers/pinctrl/freescale/pinctrl-imx.c
index 1f81569c7ae3b..cb7e0f08d2cf4 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -824,12 +824,13 @@ int imx_pinctrl_probe(struct platform_device *pdev,
return -EINVAL;
}
  
-			ipctl->input_sel_base = of_iomap(np, 0);

+   ipctl->input_sel_base = devm_of_iomap(>dev, np,
+ 0, NULL);
of_node_put(np);
-   if (!ipctl->input_sel_base) {
+   if (IS_ERR(ipctl->input_sel_base)) {
dev_err(>dev,
"iomuxc input select base address not 
found\n");
-   return -ENOMEM;
+   return PTR_ERR(ipctl->input_sel_base);
}
}
}

Re: [PATCH] scsi: cumana_2: Fix different dev_id between 'request_irq()' and 'free_irq()'

[Marion & Christophe JAILLET]

Le 30/05/2020 à 11:43, Russell King - ARM Linux admin a écrit :

On Sat, May 30, 2020 at 09:35:55AM +0200, Christophe JAILLET wrote:

The dev_id used in 'request_irq()' and 'free_irq()' should match.
So use 'host' in both cases.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Christophe JAILLET 

This is itself wrong.  cumanascsi_2_intr() requires "info" as the devid.
Either cumanascsi_2_intr() needs changing to use shost_priv(host) along
with this change, or free_irq() needs changing to use "info".


My bad.

I've only looked at the difference of the dev_id for the 2 functions, 
not at the usage of it with the function registered by 'request_irq'. 
This one is obviously correct, or the driver would have some problems 
somewhere.

I don't know why have chosen to change request_irq and not free_irq.

So obvious. I'm a little embarrassed and will send a v2.

Thx for the quick reply and review.


All the 3 patches being in "/drivers/scsi/arm/", do you prefer only 1 
patch for the 3, or separated as I've done so far?


CJ


Likely the same for the other patches, I haven't looked.

Re: [PATCH 3.16 35/99] pxa168fb: Fix the function used to release some memory in an error handling path

[Marion & Christophe JAILLET]

Hi,

sorry for the noise, I have messed up my 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ usage.

I thought I was looking at the 3.16.83 branch, but I was not.

The patch looks good to me.

CJ

Le 21/05/2020 à 16:09, Marion & Christophe JAILLET a écrit :

Hi,

I don't think that this one is applicable to 3.16.x

The remove function and the error handling path of the probe function 
both use 'dma_free_wc'.
I've not look in details, but it looks consistent and the patch would 
not apply as-is anyway.


just my 2c.

CJ

Le 20/05/2020 à 16:14, Ben Hutchings a écrit :
3.16.84-rc1 review patch.  If anyone has any objections, please let 
me know.


--

From: Christophe JAILLET 

commit 3c911fe799d1c338d94b78e7182ad452c37af897 upstream.

In the probe function, some resources are allocated using 
'dma_alloc_wc()',

they should be released with 'dma_free_wc()', not 'dma_free_coherent()'.

We already use 'dma_free_wc()' in the remove function, but not in the
error handling path of the probe function.

Also, remove a useless 'PAGE_ALIGN()'. 'info->fix.smem_len' is already
PAGE_ALIGNed.

Fixes: 638772c7553f ("fb: add support of LCD display controller on 
pxa168/910 (base layer)")

Signed-off-by: Christophe JAILLET 
Reviewed-by: Lubomir Rintel 
CC: YueHaibing 
Signed-off-by: Bartlomiej Zolnierkiewicz 
Link: 
https://patchwork.freedesktop.org/patch/msgid/20190831100024.3248-1-christophe.jail...@wanadoo.fr

[bwh: Backported to 3.16: Use dma_free_writecombine().]
Signed-off-by: Ben Hutchings 
---
  drivers/video/fbdev/pxa168fb.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

--- a/drivers/video/fbdev/pxa168fb.c
+++ b/drivers/video/fbdev/pxa168fb.c
@@ -772,8 +772,8 @@ failed_free_cmap:
  failed_free_clk:
  clk_disable(fbi->clk);
  failed_free_fbmem:
-    dma_free_coherent(fbi->dev, info->fix.smem_len,
-    info->screen_base, fbi->fb_start_dma);
+    dma_free_writecombine(fbi->dev, info->fix.smem_len,
+  info->screen_base, fbi->fb_start_dma);
  failed_free_info:
  kfree(info);
  failed_put_clk:
@@ -809,7 +809,7 @@ static int pxa168fb_remove(struct platfo
    irq = platform_get_irq(pdev, 0);
  -    dma_free_writecombine(fbi->dev, PAGE_ALIGN(info->fix.smem_len),
+    dma_free_writecombine(fbi->dev, info->fix.smem_len,
  info->screen_base, info->fix.smem_start);
    clk_disable(fbi->clk);

Re: [PATCH 3.16 35/99] pxa168fb: Fix the function used to release some memory in an error handling path

[Marion & Christophe JAILLET]

Hi,

I don't think that this one is applicable to 3.16.x

The remove function and the error handling path of the probe function 
both use 'dma_free_wc'.
I've not look in details, but it looks consistent and the patch would 
not apply as-is anyway.


just my 2c.

CJ

Le 20/05/2020 à 16:14, Ben Hutchings a écrit :

3.16.84-rc1 review patch.  If anyone has any objections, please let me know.

--

From: Christophe JAILLET 

commit 3c911fe799d1c338d94b78e7182ad452c37af897 upstream.

In the probe function, some resources are allocated using 'dma_alloc_wc()',
they should be released with 'dma_free_wc()', not 'dma_free_coherent()'.

We already use 'dma_free_wc()' in the remove function, but not in the
error handling path of the probe function.

Also, remove a useless 'PAGE_ALIGN()'. 'info->fix.smem_len' is already
PAGE_ALIGNed.

Fixes: 638772c7553f ("fb: add support of LCD display controller on pxa168/910 (base 
layer)")
Signed-off-by: Christophe JAILLET 
Reviewed-by: Lubomir Rintel 
CC: YueHaibing 
Signed-off-by: Bartlomiej Zolnierkiewicz 
Link: 
https://patchwork.freedesktop.org/patch/msgid/20190831100024.3248-1-christophe.jail...@wanadoo.fr
[bwh: Backported to 3.16: Use dma_free_writecombine().]
Signed-off-by: Ben Hutchings 
---
  drivers/video/fbdev/pxa168fb.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

--- a/drivers/video/fbdev/pxa168fb.c
+++ b/drivers/video/fbdev/pxa168fb.c
@@ -772,8 +772,8 @@ failed_free_cmap:
  failed_free_clk:
clk_disable(fbi->clk);
  failed_free_fbmem:
-   dma_free_coherent(fbi->dev, info->fix.smem_len,
-   info->screen_base, fbi->fb_start_dma);
+   dma_free_writecombine(fbi->dev, info->fix.smem_len,
+ info->screen_base, fbi->fb_start_dma);
  failed_free_info:
kfree(info);
  failed_put_clk:
@@ -809,7 +809,7 @@ static int pxa168fb_remove(struct platfo
  
  	irq = platform_get_irq(pdev, 0);
  
-	dma_free_writecombine(fbi->dev, PAGE_ALIGN(info->fix.smem_len),

+   dma_free_writecombine(fbi->dev, info->fix.smem_len,
info->screen_base, info->fix.smem_start);
  
  	clk_disable(fbi->clk);

Re: [PATCH] memory: tegra: Fix an error handling path in 'tegra186_emc_probe()'

[Marion & Christophe JAILLET]

Le 08/05/2020 à 10:49, Dan Carpenter a écrit :

On Wed, May 06, 2020 at 10:09:07PM +0200, Christophe JAILLET wrote:

@@ -237,7 +239,7 @@ static int tegra186_emc_probe(struct platform_device *pdev)
"failed to set rate range [%lu-%lu] for %pC\n",
emc->debugfs.min_rate, emc->debugfs.max_rate,
emc->clk);
-   return err;
+   goto err_put_bpmp;
}
  
  	emc->debugfs.root = debugfs_create_dir("emc", NULL);

Not really related to this patch but the error handling on this
debugfs_create_dir() call is wrong.  It never returns NULL.  The error
should just be ignored.  It shouldn't try print a message when debugfs
is deliberately disabled.

As in the correct code looks like:

  
 emc->debugfs.root = debugfs_create_dir("emc", NULL);

-   if (!emc->debugfs.root) {
-   dev_err(>dev, "failed to create debugfs directory\n");
-   return 0;
-   }
-
 debugfs_create_file("available_rates", S_IRUGO, emc->debugfs.root,
 emc, _emc_debug_available_rates_fops);
 debugfs_create_file("min_rate", S_IRUGO | S_IWUSR, emc->debugfs.root,

debugfs_create_file() will return an error pointer if debugfs_create_dir()
fails or if debugfs is disabled.  (It is a no-op).

regards,
dan carpenter


LGTM.

I let you propose a patch for it,.

CJ

Re: [PATCH] iio: sca3000: Remove an erroneous 'get_device()'

[Marion & Christophe JAILLET]

Le 06/05/2020 à 12:38, Andy Shevchenko a écrit :

On Wed, May 6, 2020 at 6:55 AM Christophe JAILLET
 wrote:

This looks really unusual to have a 'get_device()' hidden in a 'dev_err()'
call.
Remove it.

While at it add a missing \n at the end of the message.


It should have Fixes tag because it is a quite an issue (get_device()
breaks reference counting with all problems we may expect).


Agreed and I usually do, but here, I've lost track when this driver has 
gone out of staging.


Based on:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/iio/accel/sca3000.c
The issue was already there on 2016/10/23, but when I try to go one step 
further:


https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/staging/iio/accel/sca3000.c?id=2ccf61442ff142d2dde7c47471c2798a4d78b0ad
     ^^^
works but if I try to see the log for that:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/staging/iio/accel/sca3000.c
^^^ ^^^
is empty.

Most of the time, when I do it like that it works just fine, but not on 
this file.


Any other way to navigate in history of moved file would be appreciated.

CJ




Signed-off-by: Christophe JAILLET 
---
This patch is purely speculative.
I've looked a bit arround and see no point for this get_device() but other
eyes are welcomed :)
---
  drivers/iio/accel/sca3000.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 66d768d971e1..6e429072e44a 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -980,7 +980,7 @@ static int sca3000_read_data(struct sca3000_state *st,
 st->tx[0] = SCA3000_READ_REG(reg_address_high);
 ret = spi_sync_transfer(st->us, xfer, ARRAY_SIZE(xfer));
 if (ret) {
-   dev_err(get_device(>us->dev), "problem reading register");
+   dev_err(>us->dev, "problem reading register\n");
 return ret;
 }

--
2.25.1

Re: [PATCH] net: dsa: loop: Check for memory allocation failure

[Marion & Christophe JAILLET]

Le 10/05/2017 à 06:46, Julia Lawall a écrit :


On Wed, 10 May 2017, Christophe JAILLET wrote:


Le 09/05/2017 à 17:18, Joe Perches a écrit :

On Mon, 2017-05-08 at 17:35 -0700, Florian Fainelli wrote:

On 05/08/2017 04:46 PM, Julia Lawall wrote:

On Mon, 8 May 2017, Joe Perches wrote:

Each time -EPROBE_DEFER occurs, another set of calls to
dsa_switch_alloc and dev_kzalloc also occurs.

Perhaps it'd be better to do:

if (ps->netdev) {
devm_kfree(>dev, ps);
devm_kfree(>dev, ds);
return -EPROBE_DEFER;
}

Is EPROBE_DEFER handled differently than other kinds of errors?

In the core device driver model, yes, EPROBE_DEFER is treated
differently than other errors because it puts the driver on a retry queue.

EPROBE_DEFER is already a slow and exceptional path, and this is a
mock-up driver, so I am not sure what value there is in trying to
balance devm_kzalloc() with corresponding devm_kfree()...

Example code should be as correct as possible.


Le 09/05/2017 à 17:18, Joe Perches a écrit :

On Mon, 2017-05-08 at 17:35 -0700, Florian Fainelli wrote:

On 05/08/2017 04:46 PM, Julia Lawall wrote:

On Mon, 8 May 2017, Joe Perches wrote:

Each time -EPROBE_DEFER occurs, another set of calls to
dsa_switch_alloc and dev_kzalloc also occurs.

Perhaps it'd be better to do:

if (ps->netdev) {
devm_kfree(>dev, ps);
devm_kfree(>dev, ds);
return -EPROBE_DEFER;
}

Is EPROBE_DEFER handled differently than other kinds of errors?

In the core device driver model, yes, EPROBE_DEFER is treated
differently than other errors because it puts the driver on a retry queue.

EPROBE_DEFER is already a slow and exceptional path, and this is a
mock-up driver, so I am not sure what value there is in trying to
balance devm_kzalloc() with corresponding devm_kfree()...

Example code should be as correct as possible.


(* number of people/mailing list in copy has been reduced *)


The coccinelle script below gives the following list of candidates for such
improvement.

char/hw_random/omap-rng.c
clk/clk-si5351.c
clk/clk-versaclock5.c
crypto/mediatek/mtk-platform.c
devfreq/rk3399_dmc.c
dma/mv_xor_v2.c
dma/omap-dma.c
gpu/drm/arc/arcpgu_hdmi.c
gpu/drm/bridge/dumb-vga-dac.c
gpu/drm/bridge/lvds-encoder.c
gpu/drm/exynos/exynos_dp.c
gpu/drm/exynos/exynos_drm_dsi.c
gpu/drm/imx/dw_hdmi-imx.c
gpu/drm/mediatek/mtk_dpi.c
gpu/drm/mediatek/mtk_drm_ddp_comp.c
gpu/drm/mediatek/mtk_dsi.c
gpu/drm/panel/panel-lvds.c
gpu/drm/panel/panel-simple.c
gpu/drm/panel/panel-sitronix-st7789v.c
gpu/drm/rcar-du/rcar_du_lvdscon.c
gpu/drm/rockchip/cdn-dp-core.c
gpu/drm/rockchip/dw_hdmi-rockchip.c
gpu/drm/sti/sti_hdmi.c
gpu/drm/tegra/sor.c
gpu/drm/tilcdc/tilcdc_panel.c
gpu/drm/vc4/vc4_hdmi.c
gpu/ipu-v3/ipu-common.c
gpu/ipu-v3/ipu-pre.c
gpu/ipu-v3/ipu-prg.c
hwtracing/coresight/coresight-stm.c
i2c/busses/i2c-designware-platdrv.c
i2c/busses/i2c-mv64xxx.c
i2c/muxes/i2c-mux-gpio.c
i2c/muxes/i2c-mux-pinctrl.c
i2c/muxes/i2c-mux-reg.c
iommu/mtk_iommu.c
iommu/mtk_iommu_v1.c
irqchip/qcom-irq-combiner.c
mailbox/mailbox-test.c
media/i2c/mt9m111.c
media/i2c/ov2640.c
media/i2c/ov7670.c
media/i2c/smiapp/smiapp-core.c
media/i2c/soc_camera/imx074.c
media/platform/coda/coda-common.c
media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
media/platform/s5p-cec/s5p_cec.c
media/platform/sti/cec/stih-cec.c
memory/tegra/tegra124-emc.c
mfd/twl6040.c
mtd/nand/lpc32xx_mlc.c
mtd/nand/lpc32xx_slc.c
net/dsa/dsa_loop.c
net/ethernet/mediatek/mtk_eth_soc.c
net/phy/xilinx_gmii2rgmii.c
net/wireless/ti/wlcore/spi.c
pci/host/pcie-iproc-platform.c
phy/phy-exynos5250-sata.c
phy/phy-mt65xx-usb3.c
phy/phy-qcom-qusb2.c
phy/phy-sun4i-usb.c
pinctrl/core.c
pinctrl/pinctrl-at91.c
platform/x86/intel_cht_int33fe.c
power/supply/act8945a_charger.c
power/supply/axp20x_ac_power.c
power/supply/axp20x_battery.c
power/supply/axp288_charger.c
power/supply/bq24190_charger.c
power/supply/cpcap-charger.c
power/supply/gpio-charger.c
soc/bcm/raspberrypi-power.c
thermal/samsung/exynos_tmu.c
tty/serial/8250/8250_dw.c
tty/serial/max310x.c
tty/serial/sccnxp.c
usb/chipidea/ci_hdrc_msm.c
usb/gadget/udc/mv_udc_core.c
usb/host/xhci-mtk.c
usb/mtu3/mtu3_plat.c
usb/musb/sunxi.c
usb/phy/phy-am335x.c
usb/phy/phy-generic.c
usb/phy/phy-twl6030-usb.c
video/backlight/hx8357.c
video/backlight/lp855x_bl.c
video/fbdev/simplefb.c


Coccinelle script :
=
// find calls to kmalloc or equivalent function
@call@
expression ptr;
position p;
@@

(
*   ptr@p = kmalloc(...)
|
*   ptr@p = kzalloc(...)
|
*   ptr@p = kcalloc(...)
|
*   ptr@p = kmalloc_array(...)

Do you get any reports for the above function?  Those would normally just
be memory leaks.

Only one, but the corresponding kfree was in place.


julia


|
*   ptr@p = devm_kmalloc(...)
|
*   ptr@p = devm_kzalloc(...)
|
*   ptr@p = devm_kcalloc(...)
|
*   ptr@p = devm_kmalloc_array(...)
)
  ...
*  return -EPROBE_DEFER;

--
To 

Re: [PATCH] net: dsa: loop: Check for memory allocation failure

[Marion & Christophe JAILLET]

Le 10/05/2017 à 06:46, Julia Lawall a écrit :


On Wed, 10 May 2017, Christophe JAILLET wrote:


Le 09/05/2017 à 17:18, Joe Perches a écrit :

On Mon, 2017-05-08 at 17:35 -0700, Florian Fainelli wrote:

On 05/08/2017 04:46 PM, Julia Lawall wrote:

On Mon, 8 May 2017, Joe Perches wrote:

Each time -EPROBE_DEFER occurs, another set of calls to
dsa_switch_alloc and dev_kzalloc also occurs.

Perhaps it'd be better to do:

if (ps->netdev) {
devm_kfree(>dev, ps);
devm_kfree(>dev, ds);
return -EPROBE_DEFER;
}

Is EPROBE_DEFER handled differently than other kinds of errors?

In the core device driver model, yes, EPROBE_DEFER is treated
differently than other errors because it puts the driver on a retry queue.

EPROBE_DEFER is already a slow and exceptional path, and this is a
mock-up driver, so I am not sure what value there is in trying to
balance devm_kzalloc() with corresponding devm_kfree()...

Example code should be as correct as possible.


Le 09/05/2017 à 17:18, Joe Perches a écrit :

On Mon, 2017-05-08 at 17:35 -0700, Florian Fainelli wrote:

On 05/08/2017 04:46 PM, Julia Lawall wrote:

On Mon, 8 May 2017, Joe Perches wrote:

Each time -EPROBE_DEFER occurs, another set of calls to
dsa_switch_alloc and dev_kzalloc also occurs.

Perhaps it'd be better to do:

if (ps->netdev) {
devm_kfree(>dev, ps);
devm_kfree(>dev, ds);
return -EPROBE_DEFER;
}

Is EPROBE_DEFER handled differently than other kinds of errors?

In the core device driver model, yes, EPROBE_DEFER is treated
differently than other errors because it puts the driver on a retry queue.

EPROBE_DEFER is already a slow and exceptional path, and this is a
mock-up driver, so I am not sure what value there is in trying to
balance devm_kzalloc() with corresponding devm_kfree()...

Example code should be as correct as possible.


(* number of people/mailing list in copy has been reduced *)


The coccinelle script below gives the following list of candidates for such
improvement.

char/hw_random/omap-rng.c
clk/clk-si5351.c
clk/clk-versaclock5.c
crypto/mediatek/mtk-platform.c
devfreq/rk3399_dmc.c
dma/mv_xor_v2.c
dma/omap-dma.c
gpu/drm/arc/arcpgu_hdmi.c
gpu/drm/bridge/dumb-vga-dac.c
gpu/drm/bridge/lvds-encoder.c
gpu/drm/exynos/exynos_dp.c
gpu/drm/exynos/exynos_drm_dsi.c
gpu/drm/imx/dw_hdmi-imx.c
gpu/drm/mediatek/mtk_dpi.c
gpu/drm/mediatek/mtk_drm_ddp_comp.c
gpu/drm/mediatek/mtk_dsi.c
gpu/drm/panel/panel-lvds.c
gpu/drm/panel/panel-simple.c
gpu/drm/panel/panel-sitronix-st7789v.c
gpu/drm/rcar-du/rcar_du_lvdscon.c
gpu/drm/rockchip/cdn-dp-core.c
gpu/drm/rockchip/dw_hdmi-rockchip.c
gpu/drm/sti/sti_hdmi.c
gpu/drm/tegra/sor.c
gpu/drm/tilcdc/tilcdc_panel.c
gpu/drm/vc4/vc4_hdmi.c
gpu/ipu-v3/ipu-common.c
gpu/ipu-v3/ipu-pre.c
gpu/ipu-v3/ipu-prg.c
hwtracing/coresight/coresight-stm.c
i2c/busses/i2c-designware-platdrv.c
i2c/busses/i2c-mv64xxx.c
i2c/muxes/i2c-mux-gpio.c
i2c/muxes/i2c-mux-pinctrl.c
i2c/muxes/i2c-mux-reg.c
iommu/mtk_iommu.c
iommu/mtk_iommu_v1.c
irqchip/qcom-irq-combiner.c
mailbox/mailbox-test.c
media/i2c/mt9m111.c
media/i2c/ov2640.c
media/i2c/ov7670.c
media/i2c/smiapp/smiapp-core.c
media/i2c/soc_camera/imx074.c
media/platform/coda/coda-common.c
media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
media/platform/s5p-cec/s5p_cec.c
media/platform/sti/cec/stih-cec.c
memory/tegra/tegra124-emc.c
mfd/twl6040.c
mtd/nand/lpc32xx_mlc.c
mtd/nand/lpc32xx_slc.c
net/dsa/dsa_loop.c
net/ethernet/mediatek/mtk_eth_soc.c
net/phy/xilinx_gmii2rgmii.c
net/wireless/ti/wlcore/spi.c
pci/host/pcie-iproc-platform.c
phy/phy-exynos5250-sata.c
phy/phy-mt65xx-usb3.c
phy/phy-qcom-qusb2.c
phy/phy-sun4i-usb.c
pinctrl/core.c
pinctrl/pinctrl-at91.c
platform/x86/intel_cht_int33fe.c
power/supply/act8945a_charger.c
power/supply/axp20x_ac_power.c
power/supply/axp20x_battery.c
power/supply/axp288_charger.c
power/supply/bq24190_charger.c
power/supply/cpcap-charger.c
power/supply/gpio-charger.c
soc/bcm/raspberrypi-power.c
thermal/samsung/exynos_tmu.c
tty/serial/8250/8250_dw.c
tty/serial/max310x.c
tty/serial/sccnxp.c
usb/chipidea/ci_hdrc_msm.c
usb/gadget/udc/mv_udc_core.c
usb/host/xhci-mtk.c
usb/mtu3/mtu3_plat.c
usb/musb/sunxi.c
usb/phy/phy-am335x.c
usb/phy/phy-generic.c
usb/phy/phy-twl6030-usb.c
video/backlight/hx8357.c
video/backlight/lp855x_bl.c
video/fbdev/simplefb.c


Coccinelle script :
=
// find calls to kmalloc or equivalent function
@call@
expression ptr;
position p;
@@

(
*   ptr@p = kmalloc(...)
|
*   ptr@p = kzalloc(...)
|
*   ptr@p = kcalloc(...)
|
*   ptr@p = kmalloc_array(...)

Do you get any reports for the above function?  Those would normally just
be memory leaks.

Only one, but the corresponding kfree was in place.


julia


|
*   ptr@p = devm_kmalloc(...)
|
*   ptr@p = devm_kzalloc(...)
|
*   ptr@p = devm_kcalloc(...)
|
*   ptr@p = devm_kmalloc_array(...)
)
  ...
*  return -EPROBE_DEFER;

--
To 

[RFC] mips: ath79: clock:- Unmap region obtained by of_iomap

[Marion & Christophe JAILLET]

Hi,

I had a patch similar to:

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/mips/ath79/clock.c?id=b3d91db3f71d5f70ea60d900425a3f96aeb3d065

in my own tree.

However, mine was slightly different and was also freeing the memory 
mapping in the normal case, when 'pll_base' seems to be no more useful.


Best regards,

CJ

=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+

diff --git a/arch/mips/ath79/clock.c b/arch/mips/ath79/clock.c
index c1102cffe37d..b5d81acb2d7a 100644
--- a/arch/mips/ath79/clock.c
+++ b/arch/mips/ath79/clock.c
@@ -508,9 +508,11 @@ static void __init ath79_clocks_init_dt_ng(struct 
device_node *np)

 ar9330_clk_init(ref_clk, pll_base);
 else {
 pr_err("%s: could not find any appropriate clk_init()\n", dnfn);
-goto err_clk;
+goto err_unmap;
 }

+iounmap(pll_base);
+
 if (of_clk_add_provider(np, of_clk_src_onecell_get, _data)) {
 pr_err("%s: could not register clk provider\n", dnfn);
 goto err_clk;
@@ -518,6 +520,8 @@ static void __init ath79_clocks_init_dt_ng(struct 
device_node *np)


 return;

+err_unmap:
+iounmap(pll_base);
 err_clk:
 clk_put(ref_clk);

[RFC] mips: ath79: clock:- Unmap region obtained by of_iomap

[Marion & Christophe JAILLET]

Hi,

I had a patch similar to:

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/mips/ath79/clock.c?id=b3d91db3f71d5f70ea60d900425a3f96aeb3d065

in my own tree.

However, mine was slightly different and was also freeing the memory 
mapping in the normal case, when 'pll_base' seems to be no more useful.


Best regards,

CJ

=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+

diff --git a/arch/mips/ath79/clock.c b/arch/mips/ath79/clock.c
index c1102cffe37d..b5d81acb2d7a 100644
--- a/arch/mips/ath79/clock.c
+++ b/arch/mips/ath79/clock.c
@@ -508,9 +508,11 @@ static void __init ath79_clocks_init_dt_ng(struct 
device_node *np)

 ar9330_clk_init(ref_clk, pll_base);
 else {
 pr_err("%s: could not find any appropriate clk_init()\n", dnfn);
-goto err_clk;
+goto err_unmap;
 }

+iounmap(pll_base);
+
 if (of_clk_add_provider(np, of_clk_src_onecell_get, _data)) {
 pr_err("%s: could not register clk provider\n", dnfn);
 goto err_clk;
@@ -518,6 +520,8 @@ static void __init ath79_clocks_init_dt_ng(struct 
device_node *np)


 return;

+err_unmap:
+iounmap(pll_base);
 err_clk:
 clk_put(ref_clk);

[RFC] Question about freeing of resource in 'atlas7_pinmux_probe()', in file 'drivers/pinctrl/sirf/pinctrl-atlas7.c'

[Marion & Christophe JAILLET]

Hi,

while playing with coccinelle, a missing 'of_node_put()' triggered in 
'atlas7_pinmux_probe()', in file 'drivers/pinctrl/sirf/pinctrl-atlas7.c'.


/* The sd3 and sd9 shared all pins, and the function select by
 * SYS2PCI_SDIO9SEL register
 */
sys2pci_np = of_find_node_by_name(NULL, "sys2pci");
if (!sys2pci_np)
return -EINVAL;
ret = of_address_to_resource(sys2pci_np, 0, );
if (ret) <-missing of_node_put(sys2pci_np);
return ret;
pmx->sys2pci_base = devm_ioremap_resource(>dev, );
if (IS_ERR(pmx->sys2pci_base)) {
of_node_put(sys2pci_np); <-added by commit 
151b8c5ba1eb

return -ENOMEM;
}

Looking at the history of this file, I found a recent commit that added 
another missing of_node_put (see above).



Adding missing 'of_node_put()' in error handling paths is fine, but in 
this particular case, I was wondering if one was not also missing in the 
normal path?



In such a case, I would revert 151b8c5ba1eb and propose something like:
/* The sd3 and sd9 shared all pins, and the function select by
 * SYS2PCI_SDIO9SEL register
 */
sys2pci_np = of_find_node_by_name(NULL, "sys2pci");
if (!sys2pci_np)
return -EINVAL;
ret = of_address_to_resource(sys2pci_np, 0, );
if (ret) {
of_node_put(sys2pci_np);
return ret;
}
of_node_put(sys2pci_np);

pmx->sys2pci_base = devm_ioremap_resource(>dev, );
if (IS_ERR(pmx->sys2pci_base))
return -ENOMEM;

Thanks for your comment,

best regards,

CJ

[RFC] Question about freeing of resource in 'atlas7_pinmux_probe()', in file 'drivers/pinctrl/sirf/pinctrl-atlas7.c'

[Marion & Christophe JAILLET]

Hi,

while playing with coccinelle, a missing 'of_node_put()' triggered in 
'atlas7_pinmux_probe()', in file 'drivers/pinctrl/sirf/pinctrl-atlas7.c'.


/* The sd3 and sd9 shared all pins, and the function select by
 * SYS2PCI_SDIO9SEL register
 */
sys2pci_np = of_find_node_by_name(NULL, "sys2pci");
if (!sys2pci_np)
return -EINVAL;
ret = of_address_to_resource(sys2pci_np, 0, );
if (ret) <-missing of_node_put(sys2pci_np);
return ret;
pmx->sys2pci_base = devm_ioremap_resource(>dev, );
if (IS_ERR(pmx->sys2pci_base)) {
of_node_put(sys2pci_np); <-added by commit 
151b8c5ba1eb

return -ENOMEM;
}

Looking at the history of this file, I found a recent commit that added 
another missing of_node_put (see above).



Adding missing 'of_node_put()' in error handling paths is fine, but in 
this particular case, I was wondering if one was not also missing in the 
normal path?



In such a case, I would revert 151b8c5ba1eb and propose something like:
/* The sd3 and sd9 shared all pins, and the function select by
 * SYS2PCI_SDIO9SEL register
 */
sys2pci_np = of_find_node_by_name(NULL, "sys2pci");
if (!sys2pci_np)
return -EINVAL;
ret = of_address_to_resource(sys2pci_np, 0, );
if (ret) {
of_node_put(sys2pci_np);
return ret;
}
of_node_put(sys2pci_np);

pmx->sys2pci_base = devm_ioremap_resource(>dev, );
if (IS_ERR(pmx->sys2pci_base))
return -ENOMEM;

Thanks for your comment,

best regards,

CJ

Spurious code in commit 1bf40ada6290 ("amd-xgbe: Add support for clause 37 auto-negotiation"

[Marion & Christophe JAILLET]

Hi,

in commit 1bf40ada6290 ("amd-xgbe: Add support for clause 37 
auto-negotiation"), we can find:


diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-common.h 
b/drivers/net/ethernet/amd/xgbe/xgbe-common.h

index 695e982..8bcf4ef 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-common.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
   [...]
   #define XGBE_AN_CL37_PCS_MODE_MASK0x06
   #define XGBE_AN_CL37_PCS_MODE_BASEX0x00
   #define XGBE_AN_CL37_PCS_MODE_SGMII0x04
   #define XGBE_AN_CL37_TX_CONFIG_MASK0x08
   [...]

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c 
b/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c

index d5bfbe4..723eb90 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
   [...]
   /* Set up the Control register */
   reg = XMDIO_READ(pdata, MDIO_MMD_VEND2, MDIO_VEND2_AN_CTRL);
   reg &= XGBE_AN_CL37_TX_CONFIG_MASK;
   reg &= XGBE_AN_CL37_PCS_MODE_MASK;
   [...]

the "reg &=" statements look spurious. The 2 constants being 0x06 and 
0x08, the current code is equivalent to "reg = 0"


It is likely that "reg |=" (or "reg &= ~") was expected here.

Best regards,
CJ

Spurious code in commit 1bf40ada6290 ("amd-xgbe: Add support for clause 37 auto-negotiation"

[Marion & Christophe JAILLET]

Hi,

in commit 1bf40ada6290 ("amd-xgbe: Add support for clause 37 
auto-negotiation"), we can find:


diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-common.h 
b/drivers/net/ethernet/amd/xgbe/xgbe-common.h

index 695e982..8bcf4ef 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-common.h
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
   [...]
   #define XGBE_AN_CL37_PCS_MODE_MASK0x06
   #define XGBE_AN_CL37_PCS_MODE_BASEX0x00
   #define XGBE_AN_CL37_PCS_MODE_SGMII0x04
   #define XGBE_AN_CL37_TX_CONFIG_MASK0x08
   [...]

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c 
b/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c

index d5bfbe4..723eb90 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
   [...]
   /* Set up the Control register */
   reg = XMDIO_READ(pdata, MDIO_MMD_VEND2, MDIO_VEND2_AN_CTRL);
   reg &= XGBE_AN_CL37_TX_CONFIG_MASK;
   reg &= XGBE_AN_CL37_PCS_MODE_MASK;
   [...]

the "reg &=" statements look spurious. The 2 constants being 0x06 and 
0x08, the current code is equivalent to "reg = 0"


It is likely that "reg |=" (or "reg &= ~") was expected here.

Best regards,
CJ

Re: Question about commit f9a67b1182e5 ("md/bitmap: clear bitmap if bitmap_create failed").

[Marion & Christophe JAILLET]

Le 14/09/2016 à 10:25, Guoqing Jiang a écrit :



On 09/13/2016 01:24 PM, Shaohua Li wrote:

On Mon, Sep 12, 2016 at 09:09:48PM +0200, Christophe JAILLET wrote:

Hi,

I'm puzzled by commit f9a67b1182e5 ("md/bitmap: clear bitmap if
bitmap_create failed").

Hi Christophe,
Thank you very much to help check this!


Part of the commit is:

@@ -1865,8 +1866,10 @@ int bitmap_copy_from_slot(struct mddev 
*mddev, int

slot,
  struct bitmap_counts *counts;
  struct bitmap *bitmap = bitmap_create(mddev, slot);

-if (IS_ERR(bitmap))
+if (IS_ERR(bitmap)) {
+bitmap_free(bitmap);
  return PTR_ERR(bitmap);
+}

but if 'bitmap' is an error, I think that bad things will happen in
'bitmap_free()' when, at the beginning of the function, we will 
execute:


 if (bitmap->sysfs_can_clear) <-
 sysfs_put(bitmap->sysfs_can_clear);


I guess it is safe, since below part is at the beginning of bitmap_free.

if (!bitmap) /* there was no bitmap */
return;


I don't share your feeling.
bitmap_create() can return ERR_PTR(-ENOMEM) or ERR_PTR(-EINVAL).

In such cases 'if (!bitmap)' will not be helpful.

Maybe it should be turned into 'if (IS_ERR_OR_NULL(bitmap))' to handle 
errors returned by bitmap_create.

Maybe just removing the call to 'bitmap_free(bitmap)' is enough.

In any case, I think that the current logic is somehow broken.

Best regards,
CJ

Re: Question about commit f9a67b1182e5 ("md/bitmap: clear bitmap if bitmap_create failed").

[Marion & Christophe JAILLET]

Le 14/09/2016 à 10:25, Guoqing Jiang a écrit :



On 09/13/2016 01:24 PM, Shaohua Li wrote:

On Mon, Sep 12, 2016 at 09:09:48PM +0200, Christophe JAILLET wrote:

Hi,

I'm puzzled by commit f9a67b1182e5 ("md/bitmap: clear bitmap if
bitmap_create failed").

Hi Christophe,
Thank you very much to help check this!


Part of the commit is:

@@ -1865,8 +1866,10 @@ int bitmap_copy_from_slot(struct mddev 
*mddev, int

slot,
  struct bitmap_counts *counts;
  struct bitmap *bitmap = bitmap_create(mddev, slot);

-if (IS_ERR(bitmap))
+if (IS_ERR(bitmap)) {
+bitmap_free(bitmap);
  return PTR_ERR(bitmap);
+}

but if 'bitmap' is an error, I think that bad things will happen in
'bitmap_free()' when, at the beginning of the function, we will 
execute:


 if (bitmap->sysfs_can_clear) <-
 sysfs_put(bitmap->sysfs_can_clear);


I guess it is safe, since below part is at the beginning of bitmap_free.

if (!bitmap) /* there was no bitmap */
return;


I don't share your feeling.
bitmap_create() can return ERR_PTR(-ENOMEM) or ERR_PTR(-EINVAL).

In such cases 'if (!bitmap)' will not be helpful.

Maybe it should be turned into 'if (IS_ERR_OR_NULL(bitmap))' to handle 
errors returned by bitmap_create.

Maybe just removing the call to 'bitmap_free(bitmap)' is enough.

In any case, I think that the current logic is somehow broken.

Best regards,
CJ

Re: [tpmdd-devel] [PATCH] TPM: Avoid reference to potentially freed memory

[Marion & Christophe JAILLET]

Le 27/10/2015 11:27, Jarkko Sakkinen a écrit :

On Fri, Oct 23, 2015 at 10:37:33AM +0300, Jarkko Sakkinen wrote:

On Thu, Oct 22, 2015 at 10:32:41PM +0200, Christophe JAILLET wrote:

Reference to the 'np' node is dropped before dereferencing the 'sizep' and
'basep' pointers, which could by then point to junk if the node has been
freed.

Refactor code to call 'of_node_pup' later.

Signed-off-by: Christophe JAILLET 

LGTM.

Is there anyone able to provide Tested-by for this?

Christophe, were you able to reproduce the crash (insmod/rmmod couple
of times maybe?) and validate that it was gone after fixing the bug?


Hi,
no, I never triggered the bug.
This is just something noticed while looking at potential issues related 
to incorrect use of 'of_node_pup'.

I only compile tested the patch.

Best regards,
CJ




Reviewed-by: Jarkko Sakkinen 

/Jarkko



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [tpmdd-devel] [PATCH] TPM: Avoid reference to potentially freed memory

[Marion & Christophe JAILLET]

Le 27/10/2015 11:27, Jarkko Sakkinen a écrit :

On Fri, Oct 23, 2015 at 10:37:33AM +0300, Jarkko Sakkinen wrote:

On Thu, Oct 22, 2015 at 10:32:41PM +0200, Christophe JAILLET wrote:

Reference to the 'np' node is dropped before dereferencing the 'sizep' and
'basep' pointers, which could by then point to junk if the node has been
freed.

Refactor code to call 'of_node_pup' later.

Signed-off-by: Christophe JAILLET 

LGTM.

Is there anyone able to provide Tested-by for this?

Christophe, were you able to reproduce the crash (insmod/rmmod couple
of times maybe?) and validate that it was gone after fixing the bug?


Hi,
no, I never triggered the bug.
This is just something noticed while looking at potential issues related 
to incorrect use of 'of_node_pup'.

I only compile tested the patch.

Best regards,
CJ




Reviewed-by: Jarkko Sakkinen 

/Jarkko



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] gpio: x-gene: Remove a useless memset

[Marion & Christophe JAILLET]

Hi,

yes, you are correct, it's a mistake.
Should I resubmit with an updated subject?

CJ


Le 01/05/2015 16:19, Dan Carpenter a écrit :

The "[PATCH 2/2]" bit is a mistake.  It's not part of a patchset.  It's
just that you have sent two patches today.  The first patch wasn't even
named [1/2]...

Unless, it is part of a patchset??  If so where is the first patch?

regards,
dan carpenter




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] gpio: x-gene: Remove a useless memset

[Marion Christophe JAILLET]

Hi,

yes, you are correct, it's a mistake.
Should I resubmit with an updated subject?

CJ


Le 01/05/2015 16:19, Dan Carpenter a écrit :

The [PATCH 2/2] bit is a mistake.  It's not part of a patchset.  It's
just that you have sent two patches today.  The first patch wasn't even
named [1/2]...

Unless, it is part of a patchset??  If so where is the first patch?

regards,
dan carpenter




--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/