Re: [PATCH] cxl: Unlock on error in probe

[walter harms]

Am 05.05.2017 09:14, schrieb Andrew Donnellan:
> On 05/05/17 15:34, Dan Carpenter wrote:
>> We should unlock if get_cxl_adapter() fails.
>>
>> Fixes: 594ff7d067ca ("cxl: Support to flash a new image on the adapter
>> from a guest")
>> Signed-off-by: Dan Carpenter 
>>
>> diff --git a/drivers/misc/cxl/flash.c b/drivers/misc/cxl/flash.c
>> index 7c61c70ba3f6..37475abea3e6 100644
>> --- a/drivers/misc/cxl/flash.c
>> +++ b/drivers/misc/cxl/flash.c
>> @@ -401,8 +401,10 @@ static int device_open(struct inode *inode,
>> struct file *file)
>>  if (down_interruptible() != 0)
>>  return -EPERM;
>>
>> -if (!(adapter = get_cxl_adapter(adapter_num)))
>> -return -ENODEV;
>> +if (!(adapter = get_cxl_adapter(adapter_num))) {
>> +rc = -ENODEV;
>> +goto err_unlock;
>> +}
>>
>>  file->private_data = adapter;
>>  continue_token = 0;
>> @@ -446,6 +448,8 @@ static int device_open(struct inode *inode, struct
>> file *file)
>>  free_page((unsigned long) le);
>>  err:
>>  put_device(>dev);
>> +err_unlock:
>> +up();
>>
>>  return rc;
>>  }
> 
> sem is a global and it looks like it's intended to be held after
> device_open() returns and only released in device_close(), so this looks
> wrong.
> 

the patch relates to the error path, do you expect a close() after the open() 
failed ?

re,
 wh

Re: [PATCH] cxl: Fix memory allocation failure test

[walter harms]

Am 30.10.2016 20:35, schrieb Christophe JAILLET:
> 'cxl_context_alloc()' does not return an error pointer. It is just a
> shortcut for a call to 'kzalloc' with 'sizeof(struct cxl_context)' as the
> size parameter.
> 
> So its return value should be compared with NULL.
> While fixing it, simplify a bit the code.
> 
> Signed-off-by: Christophe JAILLET 
> ---
> un-compiled because I don't have the required  cross build environment.
> ---
>  drivers/misc/cxl/api.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
> index 2e5233b60971..2b88ad8a2a89 100644
> --- a/drivers/misc/cxl/api.c
> +++ b/drivers/misc/cxl/api.c
> @@ -30,10 +30,8 @@ struct cxl_context *cxl_dev_context_init(struct pci_dev 
> *dev)
>   return ERR_CAST(afu);
>  
>   ctx = cxl_context_alloc();
> - if (IS_ERR(ctx)) {
> - rc = PTR_ERR(ctx);
> - goto err_dev;
> - }
> + if (!ctx)
> + return ERR_PTR(-ENOMEM);
>  

So far i see it is only used 2 times,
To avoid such problems it should be replaced with
kzalloc(sizeof(struct cxl_context), GFP_KERNEL); (from context.c)

or even better:
ctx = kzalloc(*ctx, GFP_KERNEL);

This way the error had been spotted much more early.

just my 2 cents,
re,
 wh

>   ctx->kernelapi = true;
>  
> @@ -61,7 +59,6 @@ struct cxl_context *cxl_dev_context_init(struct pci_dev 
> *dev)
>   kfree(mapping);
>  err_ctx:
>   kfree(ctx);
> -err_dev:
>   return ERR_PTR(rc);
>  }
>  EXPORT_SYMBOL_GPL(cxl_dev_context_init);

Re: [PATCH] powerpc/powernv/pci: Use kmalloc_array() in two functions

[walter harms]

Am 24.08.2016 22:36, schrieb SF Markus Elfring:
> From: Markus Elfring 
> Date: Wed, 24 Aug 2016 22:26:37 +0200
> 
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus reuse the corresponding function "kmalloc_array".
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 
> ---
>  arch/powerpc/platforms/powernv/pci-ioda.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
> b/arch/powerpc/platforms/powernv/pci-ioda.c
> index fd9444f..2366552 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -1305,7 +1305,9 @@ static int pnv_pci_vf_assign_m64(struct pci_dev *pdev, 
> u16 num_vfs)
>   else
>   m64_bars = 1;
>  
> - pdn->m64_map = kmalloc(sizeof(*pdn->m64_map) * m64_bars, GFP_KERNEL);
> + pdn->m64_map = kmalloc_array(m64_bars,
> +  sizeof(*pdn->m64_map),
> +  GFP_KERNEL);
>   if (!pdn->m64_map)
>   return -ENOMEM;
>   /* Initialize the m64_map to IODA_INVALID_M64 */
> @@ -1572,8 +1574,9 @@ int pnv_pci_sriov_enable(struct pci_dev *pdev, u16 
> num_vfs)
>  
>   /* Allocating pe_num_map */
>   if (pdn->m64_single_mode)
> - pdn->pe_num_map = kmalloc(sizeof(*pdn->pe_num_map) * 
> num_vfs,
> - GFP_KERNEL);
> + pdn->pe_num_map = kmalloc_array(num_vfs,
> + 
> sizeof(*pdn->pe_num_map),
> + GFP_KERNEL);
>   else
>   pdn->pe_num_map = kmalloc(sizeof(*pdn->pe_num_map), 
> GFP_KERNEL);
>  


what is the value of num_vfs in the !pdn->m64_single_mode case ?
otherwise someone could make it like:

if (!pdn->m64_single_mode)
   num_vfs=1;

pdn->pe_num_map = kmalloc_array(num_vfs, 

so it looks a bit oversophisticated.

re,
 wh

Re: [PATCH] cxl: replace loop with for_each_child_of_node(), remove unneeded of_node_put()

[walter harms]

Am 29.07.2016 05:55, schrieb Andrew Donnellan:
> Rewrite the cxl_guest_init_afu() loop in cxl_of_probe() to use
> for_each_child_of_node() rather than a hand-coded for loop.
> 
> Remove the useless of_node_put(afu_np) call after the loop, where it's
> guaranteed that afu_np == NULL.
> 
> Reported-by: SF Markus Elfring 
> Reported-by: Julia Lawall 
> Signed-off-by: Andrew Donnellan 
> 
> ---
> 
> Checked the of_node_put() with Fred, he thinks it was probably just left
> over from an earlier private version of the code and we can just get rid of
> it.
> ---
>  drivers/misc/cxl/of.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/cxl/of.c b/drivers/misc/cxl/of.c
> index edc4583..ec175ea 100644
> --- a/drivers/misc/cxl/of.c
> +++ b/drivers/misc/cxl/of.c
> @@ -460,7 +460,7 @@ int cxl_of_probe(struct platform_device *pdev)
>   struct device_node *afu_np = NULL;
>   struct cxl *adapter = NULL;
>   int ret;
> - int slice, slice_ok;
> + int slice = 0, slice_ok = 0;
>  
>   pr_devel("in %s\n", __func__);
>  
> @@ -476,13 +476,13 @@ int cxl_of_probe(struct platform_device *pdev)
>   }
>  
>   /* init afu */
> - slice_ok = 0;
> - for (afu_np = NULL, slice = 0; (afu_np = of_get_next_child(np, 
> afu_np)); slice++) {
> + for_each_child_of_node(np, afu_np) {
>   if ((ret = cxl_guest_init_afu(adapter, slice, afu_np)))
>   dev_err(>dev, "AFU %i failed to initialise: %i\n",
>   slice, ret);
>   else
>   slice_ok++;
> + slice++;
>   }

while you are here ..
you could move the assign out of the condition..

ret = cxl_guest_init_afu(adapter, slice, afu_np);
if (ret) 

just my 2 cents,

re,
 wh

>  
>   if (slice_ok == 0) {
> @@ -490,8 +490,6 @@ int cxl_of_probe(struct platform_device *pdev)
>   adapter->slices = 0;
>   }
>  
> - if (afu_np)
> - of_node_put(afu_np);
>   return 0;
>  }
>  
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [patch -next] wan/fsl_ucc_hdlc: info leak in uhdlc_ioctl()

[walter harms]

Am 14.07.2016 12:34, schrieb Dan Carpenter:
> There is a 2 byte struct whole after line.loopback so we need to clear
> that out to avoid disclosing stack information.
> 
> Fixes: c19b6d246a35 ('drivers/net: support hdlc function for QE-UCC')
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 19174ac..7608561 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -635,6 +635,7 @@ static int uhdlc_ioctl(struct net_device *dev, struct 
> ifreq *ifr, int cmd)
>   ifr->ifr_settings.size = size; /* data size wanted */
>   return -ENOBUFS;
>   }
> + memset(, 0, sizeof(line));
>   line.clock_type = priv->clocking;
>   line.clock_rate = 0;
>   line.loopback = 0;


In this case
line.clock_rate = 0;
line.loopback = 0;

are not need any more and can be removed
except like them to have for documentation or so.

re,
 wh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: Inconsistent use of size argument in kzalloc and memcpy in 'drivers/net/ethernet/toshiba/ps3_gelic_wireless.c'

[walter harms]

this is a case for kmemdup().

target->hwinfo=kmemdup(scan_info,be16_to_cpu(scan_info->size), GFP_KERNEL);


re,
 wh


Am 11.04.2016 12:00, schrieb Christophe JAILLET:
> Hi,
> 
> while looking at potential clean-up, I ended on the following code which
> looks spurious to me.
> 
> We allocate 'be16_to_cpu(scan_info->size)' bytes, but then copy
> 'scan_info->size'.
> This is not consistent.
> 
> 
> I don't know which one is the correct one.
> 
> 
> CJ
> 
> --- drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
> +++ /tmp/cocci-output-24201-0dddbd-ps3_gelic_wireless.c
> @@ -1616,13 +1616,10 @@ static void gelic_wl_scan_complete_event
>  target->valid = 1;
>  target->eurus_index = i;
>  kfree(target->hwinfo);
> -target->hwinfo = kzalloc(be16_to_cpu(scan_info->size),
> - GFP_KERNEL);
>  if (!target->hwinfo)
>  continue;
> 
>  /* copy hw scan info */
> -memcpy(target->hwinfo, scan_info, scan_info->size);
>  target->essid_len = strnlen(scan_info->essid,
>  sizeof(scan_info->essid));
>  target->rate_len = 0;
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] ALSA: aoa: convert bus code to use dev_groups

[walter harms]

Am 11.06.2015 10:03, schrieb Quentin Lambert:
 The dev_attrs field of struct bus_type is going away, use dev_groups instead.
 This converts the soundbus code to use the correct field.
 
 Given that all other usages of the macro define the struct attribute
 *xxx_attrs[] in the same file they assign the .dev_groups field, this patch
 merges sysfs.c into core.c.
 
 These modifications were made using Coccinelle.
 
 Signed-off-by: Quentin Lambert lambert.quen...@gmail.com
 ---
  Changes since v1:
   - Fix the commit message to actually talk about soundbus rather than MDIO
   - This version attempt to fix a problem resulting from the macro
 ATTRIBUTE_GROUPS declaring the structure as static by merging sysfs.c into
 core.c. I understand that this may not be the prefered solution since 
 Takashi suggested that adding a comment line to the previous version could
 be acceptable. 
 
  sound/aoa/soundbus/Makefile   |2 -
  sound/aoa/soundbus/core.c |   44 
 +-
  sound/aoa/soundbus/soundbus.h |2 -
  sound/aoa/soundbus/sysfs.c|   42 
  4 files changed, 44 insertions(+), 46 deletions(-)
 
 --- a/sound/aoa/soundbus/Makefile
 +++ b/sound/aoa/soundbus/Makefile
 @@ -1,3 +1,3 @@
  obj-$(CONFIG_SND_AOA_SOUNDBUS) += snd-aoa-soundbus.o
 -snd-aoa-soundbus-objs := core.o sysfs.o
 +snd-aoa-soundbus-objs := core.o
  obj-$(CONFIG_SND_AOA_SOUNDBUS_I2S) += i2sbus/
 --- a/sound/aoa/soundbus/core.c
 +++ b/sound/aoa/soundbus/core.c
 @@ -150,6 +150,48 @@ static int soundbus_device_resume(struct
  
  #endif /* CONFIG_PM */
  
 +#define soundbus_config_of_attr(field, format_string)
 \
 +static ssize_t   
 \
 +field##_show (struct device *dev, struct device_attribute *attr, \
 +  char *buf) \
 +{\
 + struct soundbus_dev *mdev = to_soundbus_device (dev);   \
 + return sprintf (buf, format_string, mdev-ofdev.dev.of_node-field); \
 +}
 +
 +static ssize_t modalias_show(struct device *dev, struct device_attribute 
 *attr,
 +  char *buf)
 +{
 + struct soundbus_dev *sdev = to_soundbus_device(dev);
 + struct platform_device *of = sdev-ofdev;
 + int length;
 +
 + if (*sdev-modalias) {
 + strlcpy(buf, sdev-modalias, sizeof(sdev-modalias) + 1);
 + strcat(buf, \n);
 + length = strlen(buf);


for my curiosity:
why not sprintf() ? almost any other code seems to use sprintf().

re,
 wh


 + } else {
 + length = sprintf(buf, of:N%sT%s\n,
 +  of-dev.of_node-name, of-dev.of_node-type);
 + }
 +
 + return length;
 +}
 +static DEVICE_ATTR_RO(modalias);
 +
 +soundbus_config_of_attr (name, %s\n);
 +static DEVICE_ATTR_RO(name);
 +soundbus_config_of_attr (type, %s\n);
 +static DEVICE_ATTR_RO(type);
 +
 +struct attribute *soundbus_dev_attrs[] = {
 + dev_attr_name.attr,
 + dev_attr_type.attr,
 + dev_attr_modalias.attr,
 + NULL,
 +};
 +ATTRIBUTE_GROUPS(soundbus_dev);
 +
  static struct bus_type soundbus_bus_type = {
   .name   = aoa-soundbus,
   .probe  = soundbus_probe,
 @@ -160,7 +202,7 @@ static struct bus_type soundbus_bus_type
   .suspend= soundbus_device_suspend,
   .resume = soundbus_device_resume,
  #endif
 - .dev_attrs  = soundbus_dev_attrs,
 + .dev_groups = soundbus_dev_groups,
  };
  
  int soundbus_add_one(struct soundbus_dev *dev)
 --- a/sound/aoa/soundbus/soundbus.h
 +++ b/sound/aoa/soundbus/soundbus.h
 @@ -199,6 +199,4 @@ struct soundbus_driver {
  extern int soundbus_register_driver(struct soundbus_driver *drv);
  extern void soundbus_unregister_driver(struct soundbus_driver *drv);
  
 -extern struct device_attribute soundbus_dev_attrs[];
 -
  #endif /* __SOUNDBUS_H */
 --- a/sound/aoa/soundbus/sysfs.c
 +++ /dev/null
 @@ -1,42 +0,0 @@
 -#include linux/kernel.h
 -#include linux/stat.h
 -/* FIX UP */
 -#include soundbus.h
 -
 -#define soundbus_config_of_attr(field, format_string)
 \
 -static ssize_t   
 \
 -field##_show (struct device *dev, struct device_attribute *attr, \
 -  char *buf) \
 -{\
 - struct soundbus_dev *mdev = to_soundbus_device (dev);   \
 - return sprintf (buf, format_string, mdev-ofdev.dev.of_node-field); \
 -}
 -
 -static ssize_t modalias_show(struct device *dev, struct device_attribute 
 *attr,
 -  char *buf)
 -{
 - struct soundbus_dev *sdev = to_soundbus_device(dev);
 - struct platform_device *of = sdev-ofdev;
 - int length;
 -
 - 

Re: [PATCH 2/2] arch/powerpc/platforms/powermac/setup.c: add missing of_node_put

[walter harms]

Am 21.08.2011 18:10, schrieb Julia Lawall:
 From: Julia Lawall ju...@diku.dk
 
 np is initialized to the result of calling a function that calls
 of_node_get, so of_node_put should be called before the pointer is dropped.
 
 The semantic match that finds this problem is as follows:
 (http://coccinelle.lip6.fr/)
 
 // smpl
 @@
 expression e,e1,e2;
 @@
 
 * e = \(of_find_node_by_type\|of_find_node_by_name\)(...)
   ... when != of_node_put(e)
   when != true e == NULL
   when != e2 = e
   e = e1
 // /smpl
 
 Signed-off-by: Julia Lawall ju...@diku.dk
 
 ---
  arch/powerpc/platforms/powermac/setup.c |8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)
 
 diff --git a/arch/powerpc/platforms/powermac/setup.c 
 b/arch/powerpc/platforms/powermac/setup.c
 index 96580b1..970ea1d 100644
 --- a/arch/powerpc/platforms/powermac/setup.c
 +++ b/arch/powerpc/platforms/powermac/setup.c
 @@ -494,11 +494,15 @@ static int __init pmac_declare_of_platform_devices(void)
   return -1;
  
   np = of_find_node_by_name(NULL, valkyrie);
 - if (np)
 + if (np) {
   of_platform_device_create(np, valkyrie, NULL);
 + of_node_put(np);
 + }
   np = of_find_node_by_name(NULL, platinum);
 - if (np)
 + if (np) {
   of_platform_device_create(np, platinum, NULL);
 + of_node_put(np);
 + }
  np = of_find_node_by_type(NULL, smu);
  if (np) {
   of_platform_device_create(np, smu, NULL);
 


hi,
it seems save to call of_node_put(np) with np==NULL, i assume the same is true
for of_platform_device_create().

so the code collapses to:

_test_node(char *name)
{
 struct device_node *np;
 np = of_find_node_by_name(NULL, name);
 of_platform_device_create(np, name, NULL);
 of_node_put(np);
 return NULL?:0:1;
}

maybe there is already something like find_node() ?

re,
 wh




___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[walter harms]

Grant Likely schrieb:
 On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov sego...@gmail.com wrote:
 On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote:
 On Tue, 31 Aug 2010, walter harms wrote:
   if (strncmp(model, PowerBook, strlen(PowerBook)) != 0 
   strncmp(model, iBook, strlen(iBook)) != 0 
   strcmp(model, PowerMac7,2) != 0 

 is there any rule that says when to use strncmp ? it seems perfecly valid 
 to use strcpy here
 (what is done in the last cmp).
 Perhaps there are some characters after eg PowerBook that one doesn't want
 to compare with?
 It seems to me that model has no '\0' in the end. If model is got from
 the hardware then we should double check it - maybe harware is buggy.
 Otherwise we'll overflow model.
 
 Model does have \0 at the end.  This code is using strncmp to
 purposefully ignore the model suffix.
 
 But why strcmp(model, PowerMac7,2)? IMO it should be replaced
 with strncmp().
 
 We use strcmp when parsing the device tree because the the length of
 the model property string is unknown and in most cases we *must* match
 the exact entire string, such as with this PowerMac7,2 example.  Using
 strncmp would also happen to match with something like
 PowerMac7,2345 which is not the desired behaviour.
 

hi Grant,
whould you mind to use you explanation as comment in the code ?
Tthat the strncpy/strcpy difference is important should be noted. that would be 
clearly a
bonos with further audits.

re,
 wh
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak

[walter harms]

Julia Lawall schrieb:
 Add a call to of_node_put in the error handling code following a call to
 of_find_node_by_path.
 
 The semantic match that finds this problem is as follows:
 (http://coccinelle.lip6.fr/)
 
 // smpl
 @r exists@
 local idexpression x;
 expression E,E1;
 statement S;
 @@
 
 *x = 
 (of_find_node_by_path
 |of_find_node_by_name
 |of_find_node_by_phandle
 |of_get_parent
 |of_get_next_parent
 |of_get_next_child
 |of_find_compatible_node
 |of_match_node
 )(...);
 ...
 if (x == NULL) S
 ... when != x = E
 *if (...) {
   ... when != of_node_put(x)
   when != if (...) { ... of_node_put(x); ... }
 (
   return +...x...+;
 |
 *  return ...;
 )
 }
 ...
 of_node_put(x);
 // /smpl
 
 Signed-off-by: Julia Lawall ju...@diku.dk
 
 ---
  drivers/macintosh/via-pmu-led.c |4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
 index d242976..19c3718 100644
 --- a/drivers/macintosh/via-pmu-led.c
 +++ b/drivers/macintosh/via-pmu-led.c
 @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
   if (dt == NULL)
   return -ENODEV;
   model = of_get_property(dt, model, NULL);
 - if (model == NULL)
 + if (model == NULL) {
 + of_node_put(dt);
   return -ENODEV;
 + }
   if (strncmp(model, PowerBook, strlen(PowerBook)) != 0 
   strncmp(model, iBook, strlen(iBook)) != 0 
   strcmp(model, PowerMac7,2) != 0 
 

is there any rule that says when to use strncmp ? it seems perfecly valid to 
use strcpy here
(what is done in the last cmp).

re,
 wh



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev