From: Nathan Lynch <ntl@pobox•com>
To: Roel Kluin <12o3l@tiscali•nl>
Cc: linuxppc-dev@ozlabs•org
Subject: Re: [PATCH] Balance alloc/free and ioremap/iounmap in gpio_mdio_probe (powerpc/platforms/pasemi/gpio_mdio.c)
Date: Sun, 4 Nov 2007 11:18:14 -0600 [thread overview]
Message-ID: <20071104171814.GI9695@localdomain> (raw)
In-Reply-To: <472DF914.7020601@tiscali.nl>
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);
>
next prev parent reply other threads:[~2007-11-04 17:18 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-11-04 16:53 [PATCH] Balance alloc/free and ioremap/iounmap in gpio_mdio_probe (powerpc/platforms/pasemi/gpio_mdio.c) Roel Kluin
2007-11-04 17:18 ` Nathan Lynch [this message]
2007-11-04 17:44 ` Roel Kluin
2007-11-04 17:47 ` Olof Johansson
2007-11-04 17:46 ` Roel Kluin
2007-11-04 21:37 ` [PATCH] pasemi: clean up gpio_mdio init Olof Johansson
2007-11-05 0:23 ` Stephen Rothwell
2007-11-05 1:10 ` Olof Johansson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20071104171814.GI9695@localdomain \
--to=ntl@pobox$(echo .)com \
--cc=12o3l@tiscali$(echo .)nl \
--cc=linuxppc-dev@ozlabs$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox