Message ID | 20100809103727.GH9031@bicker |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Aug 9, 2010, at 4:07 PM, Dan Carpenter wrote: > qlcnic_pci_info structs are 128 bytes so an array of 8 uses 1024 bytes. > That's a lot if you run with 4K stacks. I allocated them with kcalloc() > instead. > > Signed-off-by: Dan Carpenter <error27@gmail.com> > > diff --git a/drivers/net/qlcnic/qlcnic_main.c b/drivers/net/qlcnic/qlcnic_main.c > index 7f27e2a..da84229 100644 > --- a/drivers/net/qlcnic/qlcnic_main.c > +++ b/drivers/net/qlcnic/qlcnic_main.c > @@ -473,14 +473,20 @@ qlcnic_cleanup_pci_map(struct qlcnic_adapter *adapter) > static int > qlcnic_init_pci_info(struct qlcnic_adapter *adapter) > { > - struct qlcnic_pci_info pci_info[QLCNIC_MAX_PCI_FUNC]; > + struct qlcnic_pci_info *pci_info; > int i, ret = 0, err; > u8 pfn; > > + pci_info = kcalloc(QLCNIC_MAX_PCI_FUNC, sizeof(*pci_info), GFP_KERNEL); > + if (!pci_info) > + return -ENOMEM; > + > adapter->npars = kzalloc(sizeof(struct qlcnic_npar_info) * > QLCNIC_MAX_PCI_FUNC, GFP_KERNEL); > - if (!adapter->npars) > - return -ENOMEM; > + if (!adapter->npars) { > + err = -ENOMEM; > + goto err_pci_info; > + } > > adapter->eswitch = kzalloc(sizeof(struct qlcnic_eswitch) * > QLCNIC_NIU_MAX_XG_PORTS, GFP_KERNEL); > @@ -508,6 +514,7 @@ qlcnic_init_pci_info(struct qlcnic_adapter *adapter) > for (i = 0; i < QLCNIC_NIU_MAX_XG_PORTS; i++) > adapter->eswitch[i].flags |= QLCNIC_SWITCH_ENABLE; > > + kfree(pci_info); > return 0; > > err_eswitch: > @@ -516,6 +523,8 @@ err_eswitch: > err_npars: > kfree(adapter->npars); > adapter->eswitch = NULL; > +err_pci_info: > + kfree(pci_info); > > return ret; > } > @@ -3362,15 +3371,21 @@ qlcnic_sysfs_read_pci_config(struct file *file, struct kobject *kobj, > struct device *dev = container_of(kobj, struct device, kobj); > struct qlcnic_adapter *adapter = dev_get_drvdata(dev); > struct qlcnic_pci_func_cfg pci_cfg[QLCNIC_MAX_PCI_FUNC]; > - struct qlcnic_pci_info pci_info[QLCNIC_MAX_PCI_FUNC]; > + struct qlcnic_pci_info *pci_info; > int i, ret; > > if (size != sizeof(pci_cfg)) > return QL_STATUS_INVALID_PARAM; > > + pci_info = kcalloc(QLCNIC_MAX_PCI_FUNC, sizeof(*pci_info), GFP_KERNEL); > + if (!pci_info) > + return -ENOMEM; > + > ret = qlcnic_get_pci_info(adapter, pci_info); > - if (ret) > + if (ret) { > + kfree(pci_info); > return ret; > + } > > for (i = 0; i < QLCNIC_MAX_PCI_FUNC ; i++) { > pci_cfg[i].pci_func = pci_info[i].id; > @@ -3381,8 +3396,8 @@ qlcnic_sysfs_read_pci_config(struct file *file, struct kobject *kobj, > memcpy(&pci_cfg[i].def_mac_addr, &pci_info[i].mac, ETH_ALEN); > } > memcpy(buf, &pci_cfg, size); > + kfree(pci_info); > return size; > - > } > static struct bin_attribute bin_attr_npar_config = { > .attr = {.name = "npar_config", .mode = (S_IRUGO | S_IWUSR)}, It looks fine except that I'd use kzalloc instead of kcalloc above. thanks, -Anirban-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 09, 2010 at 09:46:32AM -0700, Anirban Chakraborty wrote: > > It looks fine except that I'd use kzalloc instead of kcalloc above. > It's no problem to do that, and I'm already respinning the patches but I'm confused. It looks like pci_info gets initialized correctly. What am I missing? regards, dan carpenter > thanks, > -Anirban -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 10, 2010, at 12:12 AM, Dan Carpenter wrote: > On Mon, Aug 09, 2010 at 09:46:32AM -0700, Anirban Chakraborty wrote: >> >> It looks fine except that I'd use kzalloc instead of kcalloc above. >> > > It's no problem to do that, and I'm already respinning the patches but > I'm confused. It looks like pci_info gets initialized correctly. What > am I missing? Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra argument that you are passing to kcalloc. -Anirban-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2010-08-09 at 18:43 -0700, Anirban Chakraborty wrote: > Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra > argument that you are passing to kcalloc. You probably meant to write "my preferred way" as the kcalloc to "kzalloc with a multiply" ratio is pretty high. It's actually about 2.5 to 1 in favor of kcalloc. $ grep -rw --include=*.[ch] kcalloc * | wc -l 419 $ grep -rP --include=*.[ch] "\bkzalloc\s*\(\s*\w+\s*\*\s*\w+" * | \ grep -vP "\bkzalloc\s*\(\s*sizeof\s+\*\s*\w+\s*," | wc -l 164 (the grep -vP avoids kzalloc(sizeof *p, GFP_foo) Actually, there might be a reason to use kzalloc in that location to match the other similar use a few lines away, but I'd prefer that the other use be converted to kcalloc. cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 10, 2010, at 7:33 AM, Joe Perches wrote: > On Mon, 2010-08-09 at 18:43 -0700, Anirban Chakraborty wrote: >> Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra >> argument that you are passing to kcalloc. > > You probably meant to write "my preferred way" > as the kcalloc to "kzalloc with a multiply" > ratio is pretty high. > > It's actually about 2.5 to 1 in favor of kcalloc. > > $ grep -rw --include=*.[ch] kcalloc * | wc -l > 419 > > $ grep -rP --include=*.[ch] "\bkzalloc\s*\(\s*\w+\s*\*\s*\w+" * | \ > grep -vP "\bkzalloc\s*\(\s*sizeof\s+\*\s*\w+\s*," | wc -l > 164 > > (the grep -vP avoids kzalloc(sizeof *p, GFP_foo) > > Actually, there might be a reason to use kzalloc > in that location to match the other similar use > a few lines away, but I'd prefer that the other > use be converted to kcalloc. I was suggesting based on the following: http://lwn.net/Articles/147014/ thanks, Anirban -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2010-08-09 at 20:31 -0700, Anirban Chakraborty wrote: > On Aug 10, 2010, at 7:33 AM, Joe Perches wrote: > > On Mon, 2010-08-09 at 18:43 -0700, Anirban Chakraborty wrote: > >> Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra > >> argument that you are passing to kcalloc. > > You probably meant to write "my preferred way" > > as the kcalloc to "kzalloc with a multiply" > > ratio is pretty high. > I was suggesting based on the following: > http://lwn.net/Articles/147014/ Note that article suggests kzalloc for allocating a single zeroed object. kcalloc is used for multiple zeroed objects and protects against oversized allocations. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Joe Perches <joe@perches.com> Date: Mon, 09 Aug 2010 20:39:06 -0700 > On Mon, 2010-08-09 at 20:31 -0700, Anirban Chakraborty wrote: >> On Aug 10, 2010, at 7:33 AM, Joe Perches wrote: >> > On Mon, 2010-08-09 at 18:43 -0700, Anirban Chakraborty wrote: >> >> Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra >> >> argument that you are passing to kcalloc. >> > You probably meant to write "my preferred way" >> > as the kcalloc to "kzalloc with a multiply" >> > ratio is pretty high. >> I was suggesting based on the following: >> http://lwn.net/Articles/147014/ > > Note that article suggests kzalloc for allocating > a single zeroed object. > > kcalloc is used for multiple zeroed objects and > protects against oversized allocations. Agreed, kcalloc should be used here. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/qlcnic/qlcnic_main.c b/drivers/net/qlcnic/qlcnic_main.c index 7f27e2a..da84229 100644 --- a/drivers/net/qlcnic/qlcnic_main.c +++ b/drivers/net/qlcnic/qlcnic_main.c @@ -473,14 +473,20 @@ qlcnic_cleanup_pci_map(struct qlcnic_adapter *adapter) static int qlcnic_init_pci_info(struct qlcnic_adapter *adapter) { - struct qlcnic_pci_info pci_info[QLCNIC_MAX_PCI_FUNC]; + struct qlcnic_pci_info *pci_info; int i, ret = 0, err; u8 pfn; + pci_info = kcalloc(QLCNIC_MAX_PCI_FUNC, sizeof(*pci_info), GFP_KERNEL); + if (!pci_info) + return -ENOMEM; + adapter->npars = kzalloc(sizeof(struct qlcnic_npar_info) * QLCNIC_MAX_PCI_FUNC, GFP_KERNEL); - if (!adapter->npars) - return -ENOMEM; + if (!adapter->npars) { + err = -ENOMEM; + goto err_pci_info; + } adapter->eswitch = kzalloc(sizeof(struct qlcnic_eswitch) * QLCNIC_NIU_MAX_XG_PORTS, GFP_KERNEL); @@ -508,6 +514,7 @@ qlcnic_init_pci_info(struct qlcnic_adapter *adapter) for (i = 0; i < QLCNIC_NIU_MAX_XG_PORTS; i++) adapter->eswitch[i].flags |= QLCNIC_SWITCH_ENABLE; + kfree(pci_info); return 0; err_eswitch: @@ -516,6 +523,8 @@ err_eswitch: err_npars: kfree(adapter->npars); adapter->eswitch = NULL; +err_pci_info: + kfree(pci_info); return ret; } @@ -3362,15 +3371,21 @@ qlcnic_sysfs_read_pci_config(struct file *file, struct kobject *kobj, struct device *dev = container_of(kobj, struct device, kobj); struct qlcnic_adapter *adapter = dev_get_drvdata(dev); struct qlcnic_pci_func_cfg pci_cfg[QLCNIC_MAX_PCI_FUNC]; - struct qlcnic_pci_info pci_info[QLCNIC_MAX_PCI_FUNC]; + struct qlcnic_pci_info *pci_info; int i, ret; if (size != sizeof(pci_cfg)) return QL_STATUS_INVALID_PARAM; + pci_info = kcalloc(QLCNIC_MAX_PCI_FUNC, sizeof(*pci_info), GFP_KERNEL); + if (!pci_info) + return -ENOMEM; + ret = qlcnic_get_pci_info(adapter, pci_info); - if (ret) + if (ret) { + kfree(pci_info); return ret; + } for (i = 0; i < QLCNIC_MAX_PCI_FUNC ; i++) { pci_cfg[i].pci_func = pci_info[i].id; @@ -3381,8 +3396,8 @@ qlcnic_sysfs_read_pci_config(struct file *file, struct kobject *kobj, memcpy(&pci_cfg[i].def_mac_addr, &pci_info[i].mac, ETH_ALEN); } memcpy(buf, &pci_cfg, size); + kfree(pci_info); return size; - } static struct bin_attribute bin_attr_npar_config = { .attr = {.name = "npar_config", .mode = (S_IRUGO | S_IWUSR)},
qlcnic_pci_info structs are 128 bytes so an array of 8 uses 1024 bytes. That's a lot if you run with 4K stacks. I allocated them with kcalloc() instead. Signed-off-by: Dan Carpenter <error27@gmail.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html