[PATCH] Balance alloc/free and ioremap/iounmap in gpio_mdio_probe (powerpc/platforms/pasemi/gpio_mdio.c)

2007-11-04 Thread Roel Kluin 
I think this is how it should be done, but
please review: it was not tested.
--
Balance alloc/free and ioremap/iounmap

Signed-off-by: Roel Kluin [EMAIL PROTECTED]
---
diff --git a/arch/powerpc/platforms/pasemi/gpio_mdio.c 
b/arch/powerpc/platforms/pasemi/gpio_mdio.c
index dae9f65..f250ba4 100644
--- a/arch/powerpc/platforms/pasemi/gpio_mdio.c
+++ b/arch/powerpc/platforms/pasemi/gpio_mdio.c
@@ -208,95 +208,116 @@ static int gpio_mdio_write(struct mii_bus *bus, int 
phy_id, int location, u16 va
 }
 
 static int gpio_mdio_reset(struct mii_bus *bus)
 {
/*nothing here - dunno how to reset it*/
return 0;
 }
 
-
-static int __devinit gpio_mdio_probe(struct of_device *ofdev,
-const struct of_device_id *match)
+static int __devinit __gpio_mdio_register_bus(struct of_device *ofdev,
+   const struct of_device_id *match)
 {
struct device *dev = ofdev-dev;
struct device_node *np = ofdev-node;
-   struct device_node *gpio_np;
struct mii_bus *new_bus;
struct resource res;
struct gpio_priv *priv;
const unsigned int *prop;
-   int err = 0;
int i;
 
-   gpio_np = of_find_compatible_node(NULL, gpio, 1682m-gpio);
-
-   if (!gpio_np)
-   return -ENODEV;
-
-   err = of_address_to_resource(gpio_np, 0, res);
-   of_node_put(gpio_np);
-
-   if (err)
-   return -EINVAL;
-
-   if (!gpio_regs)
-   gpio_regs = ioremap(res.start, 0x100);
-
-   if (!gpio_regs)
-   return -EPERM;
-
priv = kzalloc(sizeof(struct gpio_priv), GFP_KERNEL);
-   if (priv == NULL)
+   if (unlikely(priv == NULL))
return -ENOMEM;
 
new_bus = kzalloc(sizeof(struct mii_bus), GFP_KERNEL);
 
-   if (new_bus == NULL)
-   return -ENOMEM;
+   if (unlikely(new_bus == NULL))
+   goto free_priv;
 
new_bus-name = pasemi gpio mdio bus,
new_bus-read = gpio_mdio_read,
new_bus-write = gpio_mdio_write,
new_bus-reset = gpio_mdio_reset,
 
prop = of_get_property(np, reg, NULL);
new_bus-id = *prop;
new_bus-priv = priv;
 
new_bus-phy_mask = 0;
 
new_bus-irq = kmalloc(sizeof(int)*PHY_MAX_ADDR, GFP_KERNEL);
+   if (unlikely(new_bus-irq == NULL))
+   goto free_new_bus;
+
for(i = 0; i  PHY_MAX_ADDR; ++i)
new_bus-irq[i] = irq_create_mapping(NULL, 10);
 
 
prop = of_get_property(np, mdc-pin, NULL);
priv-mdc_pin = *prop;
 
prop = of_get_property(np, mdio-pin, NULL);
priv-mdio_pin = *prop;
 
new_bus-dev = dev;
dev_set_drvdata(dev, new_bus);
 
err = mdiobus_register(new_bus);
 
-   if (0 != err) {
+   if (unlikely(0 != err)) {
printk(KERN_ERR %s: Cannot register as MDIO bus, err %d\n,
new_bus-name, err);
goto bus_register_fail;
}
 
return 0;
 
 bus_register_fail:
+   kfree(new_bus-irq);
+free_new_bus:
kfree(new_bus);
+free_priv:
+   kfree(priv);
+
+   return -ENOMEM;
+}
+
+
+static int __devinit gpio_mdio_probe(struct of_device *ofdev,
+const struct of_device_id *match)
+{
+   struct device_node *gpio_np;
+   int err;
+
+   gpio_np = of_find_compatible_node(NULL, gpio, 1682m-gpio);
+
+   if (!gpio_np)
+   return -ENODEV;
+
+   err = of_address_to_resource(gpio_np, 0, res);
+   of_node_put(gpio_np);
+
+   if (err)
+   return -EINVAL;
+
+   if (!gpio_regs) {
+
+   gpio_regs = ioremap(res.start, 0x100);
+   if (unlikely(!gpio_regs))
+   return -EPERM;
+
+   err = __gpio_mdio_register_bus(ofdev, match);
+   if (err  0)
+   iounmap(gpio_regs);
+   } else
+   err = __gpio_mdio_register_bus(ofdev, match);
 
return err;
+
 }
 
 
 static int gpio_mdio_remove(struct of_device *dev)
 {
struct mii_bus *bus = dev_get_drvdata(dev-dev);
 
mdiobus_unregister(bus);

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

Re: [PATCH] Balance alloc/free and ioremap/iounmap in gpio_mdio_probe (powerpc/platforms/pasemi/gpio_mdio.c)

2007-11-04 Thread Nathan Lynch 
Hi,

Roel Kluin wrote:
 I think this is how it should be done, but
 please review: it was not tested.
 --
 Balance alloc/free and ioremap/iounmap

It would be more helpful if your changelog identified the objects
which could be leaked.  More comments below.


 -static int __devinit gpio_mdio_probe(struct of_device *ofdev,
 -  const struct of_device_id *match)
 +static int __devinit __gpio_mdio_register_bus(struct of_device *ofdev,
 + const struct of_device_id *match)
  {
   struct device *dev = ofdev-dev;
   struct device_node *np = ofdev-node;
 - struct device_node *gpio_np;
   struct mii_bus *new_bus;
   struct resource res;
   struct gpio_priv *priv;
   const unsigned int *prop;
 - int err = 0;
   int i;
  
 - gpio_np = of_find_compatible_node(NULL, gpio, 1682m-gpio);
 -
 - if (!gpio_np)
 - return -ENODEV;
 -
 - err = of_address_to_resource(gpio_np, 0, res);
 - of_node_put(gpio_np);
 -
 - if (err)
 - return -EINVAL;
 -
 - if (!gpio_regs)
 - gpio_regs = ioremap(res.start, 0x100);
 -
 - if (!gpio_regs)
 - return -EPERM;
 -
   priv = kzalloc(sizeof(struct gpio_priv), GFP_KERNEL);
 - if (priv == NULL)
 + if (unlikely(priv == NULL))
   return -ENOMEM;

Please don't add likely/unlikely to non-hot paths such as this.


   new_bus = kzalloc(sizeof(struct mii_bus), GFP_KERNEL);
  
 - if (new_bus == NULL)
 - return -ENOMEM;
 + if (unlikely(new_bus == NULL))
 + goto free_priv;

again

  
   new_bus-name = pasemi gpio mdio bus,
   new_bus-read = gpio_mdio_read,
   new_bus-write = gpio_mdio_write,
   new_bus-reset = gpio_mdio_reset,
  
   prop = of_get_property(np, reg, NULL);
   new_bus-id = *prop;
   new_bus-priv = priv;
  
   new_bus-phy_mask = 0;
  
   new_bus-irq = kmalloc(sizeof(int)*PHY_MAX_ADDR, GFP_KERNEL);
 + if (unlikely(new_bus-irq == NULL))
 + goto free_new_bus;
 +

again

   for(i = 0; i  PHY_MAX_ADDR; ++i)
   new_bus-irq[i] = irq_create_mapping(NULL, 10);
  
  
   prop = of_get_property(np, mdc-pin, NULL);
   priv-mdc_pin = *prop;
  
   prop = of_get_property(np, mdio-pin, NULL);
   priv-mdio_pin = *prop;
  
   new_bus-dev = dev;
   dev_set_drvdata(dev, new_bus);
  
   err = mdiobus_register(new_bus);
  
 - if (0 != err) {
 + if (unlikely(0 != err)) {

again

   printk(KERN_ERR %s: Cannot register as MDIO bus, err %d\n,
   new_bus-name, err);
   goto bus_register_fail;
   }
  
   return 0;
  
  bus_register_fail:
 + kfree(new_bus-irq);
 +free_new_bus:
   kfree(new_bus);
 +free_priv:
 + kfree(priv);
 +
 + return -ENOMEM;
 +}
 +
 +
 +static int __devinit gpio_mdio_probe(struct of_device *ofdev,
 +  const struct of_device_id *match)
 +{
 + struct device_node *gpio_np;
 + int err;
 +
 + gpio_np = of_find_compatible_node(NULL, gpio, 1682m-gpio);
 +
 + if (!gpio_np)
 + return -ENODEV;
 +
 + err = of_address_to_resource(gpio_np, 0, res);

Hmm, where is res defined?


 + of_node_put(gpio_np);
 +
 + if (err)
 + return -EINVAL;
 +
 + if (!gpio_regs) {
 +

Unneeded newline


 + gpio_regs = ioremap(res.start, 0x100);
 + if (unlikely(!gpio_regs))
 + return -EPERM;
 +
 + err = __gpio_mdio_register_bus(ofdev, match);
 + if (err  0)
 + iounmap(gpio_regs);
 + } else
 + err = __gpio_mdio_register_bus(ofdev, match);
  
   return err;
 +

again

  }
  
  
  static int gpio_mdio_remove(struct of_device *dev)
  {
   struct mii_bus *bus = dev_get_drvdata(dev-dev);
  
   mdiobus_unregister(bus);
 
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] Balance alloc/free and ioremap/iounmap in gpio_mdio_probe (powerpc/platforms/pasemi/gpio_mdio.c)

2007-11-04 Thread Olof Johansson 
On Sun, Nov 04, 2007 at 05:53:40PM +0100, Roel Kluin wrote:
 I think this is how it should be done, but
 please review: it was not tested.

Hi,

Thanks for the bug report. The mdio driver needs a set of other cleanups
as well. I have a more comprehensive patch that I'll post tomorrow after
I have a chance to test them.


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

Re: [PATCH] Balance alloc/free and ioremap/iounmap in gpio_mdio_probe (powerpc/platforms/pasemi/gpio_mdio.c)

2007-11-04 Thread Roel Kluin 
Nathan Lynch wrote:
 Hi,

I moved res to the wrong function, that's fixed as well as the unlikely's and
the extra new lines.

Thanks for your quick response. Here is an updated version:
--
Upon errors gpio_regs was not iounmapped, and later priv nor new_bus-irq was
freed. Testing succes of the allocation of new_bus-irq was missing as well.

This patch creates a new function __gpio_mdio_register_bus to allow the 
iounmapping after an ioremap.

Signed-off-by: Roel Kluin [EMAIL PROTECTED]
---

diff --git a/arch/powerpc/platforms/pasemi/gpio_mdio.c 
b/arch/powerpc/platforms/pasemi/gpio_mdio.c
index dae9f65..af5b241 100644
--- a/arch/powerpc/platforms/pasemi/gpio_mdio.c
+++ b/arch/powerpc/platforms/pasemi/gpio_mdio.c
@@ -208,68 +208,50 @@ static int gpio_mdio_write(struct mii_bus *bus, int 
phy_id, int location, u16 va
 }
 
 static int gpio_mdio_reset(struct mii_bus *bus)
 {
/*nothing here - dunno how to reset it*/
return 0;
 }
 
-
-static int __devinit gpio_mdio_probe(struct of_device *ofdev,
-const struct of_device_id *match)
+static int __devinit __gpio_mdio_register_bus(struct of_device *ofdev,
+   const struct of_device_id *match)
 {
struct device *dev = ofdev-dev;
struct device_node *np = ofdev-node;
-   struct device_node *gpio_np;
struct mii_bus *new_bus;
-   struct resource res;
struct gpio_priv *priv;
const unsigned int *prop;
-   int err = 0;
int i;
 
-   gpio_np = of_find_compatible_node(NULL, gpio, 1682m-gpio);
-
-   if (!gpio_np)
-   return -ENODEV;
-
-   err = of_address_to_resource(gpio_np, 0, res);
-   of_node_put(gpio_np);
-
-   if (err)
-   return -EINVAL;
-
-   if (!gpio_regs)
-   gpio_regs = ioremap(res.start, 0x100);
-
-   if (!gpio_regs)
-   return -EPERM;
-
priv = kzalloc(sizeof(struct gpio_priv), GFP_KERNEL);
if (priv == NULL)
return -ENOMEM;
 
new_bus = kzalloc(sizeof(struct mii_bus), GFP_KERNEL);
 
if (new_bus == NULL)
-   return -ENOMEM;
+   goto free_priv;
 
new_bus-name = pasemi gpio mdio bus,
new_bus-read = gpio_mdio_read,
new_bus-write = gpio_mdio_write,
new_bus-reset = gpio_mdio_reset,
 
prop = of_get_property(np, reg, NULL);
new_bus-id = *prop;
new_bus-priv = priv;
 
new_bus-phy_mask = 0;
 
new_bus-irq = kmalloc(sizeof(int)*PHY_MAX_ADDR, GFP_KERNEL);
+   if (new_bus-irq == NULL)
+   goto free_new_bus;
+
for(i = 0; i  PHY_MAX_ADDR; ++i)
new_bus-irq[i] = irq_create_mapping(NULL, 10);
 
 
prop = of_get_property(np, mdc-pin, NULL);
priv-mdc_pin = *prop;
 
prop = of_get_property(np, mdio-pin, NULL);
@@ -284,17 +266,54 @@ static int __devinit gpio_mdio_probe(struct of_device 
*ofdev,
printk(KERN_ERR %s: Cannot register as MDIO bus, err %d\n,
new_bus-name, err);
goto bus_register_fail;
}
 
return 0;
 
 bus_register_fail:
+   kfree(new_bus-irq);
+free_new_bus:
kfree(new_bus);
+free_priv:
+   kfree(priv);
+
+   return -ENOMEM;
+}
+
+
+static int __devinit gpio_mdio_probe(struct of_device *ofdev,
+const struct of_device_id *match)
+{
+   struct device_node *gpio_np;
+   struct resource res;
+   int err;
+
+   gpio_np = of_find_compatible_node(NULL, gpio, 1682m-gpio);
+
+   if (!gpio_np)
+   return -ENODEV;
+
+   err = of_address_to_resource(gpio_np, 0, res);
+   of_node_put(gpio_np);
+
+   if (err)
+   return -EINVAL;
+
+   if (!gpio_regs) {
+   gpio_regs = ioremap(res.start, 0x100);
+   if (unlikely(!gpio_regs))
+   return -EPERM;
+
+   err = __gpio_mdio_register_bus(ofdev, match);
+   if (err  0)
+   iounmap(gpio_regs);
+   } else
+   err = __gpio_mdio_register_bus(ofdev, match);
 
return err;
 }
 
 
 static int gpio_mdio_remove(struct of_device *dev)
 {
struct mii_bus *bus = dev_get_drvdata(dev-dev);
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] Balance alloc/free and ioremap/iounmap in gpio_mdio_probe (powerpc/platforms/pasemi/gpio_mdio.c)

2007-11-04 Thread Roel Kluin 
Olof Johansson wrote:
 On Sun, Nov 04, 2007 at 05:53:40PM +0100, Roel Kluin wrote:
 I think this is how it should be done, but
 please review: it was not tested.
 
 Hi,
 
 Thanks for the bug report. The mdio driver needs a set of other cleanups
 as well. I have a more comprehensive patch that I'll post tomorrow after
 I have a chance to test them.

Ok, that's fine with me,

Roel

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