netdev - Re: [PATCH 2/3] phylib: move to dynamic allocation of struct mii

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <3586F2DA-EF30-41CE-82F4-1AE514C3C7CD@freescale.com>
Date:	Mon, 29 Sep 2008 16:31:24 -0500
From:	Andy Fleming <afleming@...escale.com>
To:	Lennert Buytenhek <buytenh@...tstofly.org>
Cc:	netdev@...r.kernel.org, Byron Bradley <byron.bbradley@...il.com>,
	Tim Ellis <tim.ellis@....com>, Imre Kaloz <kaloz@...nwrt.org>,
	Nicolas Pitre <nico@....org>, Dirk Teurlings <dirk@...xia.nl>,
	Peter van Valderen <p.v.valderen@...il.com>
Subject: Re: [PATCH 2/3] phylib: move to dynamic allocation of struct mii_bus


On Sep 28, 2008, at 21:13, Lennert Buytenhek wrote:

> This patch introduces mdiobus_alloc() and mdiobus_free(), and
> makes all mdio bus drivers use these functions to allocate their
> struct mii_bus'es dynamically.
>
> Signed-off-by: Lennert Buytenhek <buytenh@...vell.com>
> ---
> drivers/net/au1000_eth.c          |   43 +++++++++++++++---------
> drivers/net/au1000_eth.h          |    2 +-
> drivers/net/bfin_mac.c            |   31 ++++++++++-------
> drivers/net/bfin_mac.h            |    2 +-
> drivers/net/cpmac.c               |   51 +++++++++++++++++------------
> drivers/net/fec_mpc52xx_phy.c     |    6 ++--
> drivers/net/fs_enet/mii-bitbang.c |    7 ++--
> drivers/net/fs_enet/mii-fec.c     |    6 ++--
> drivers/net/gianfar_mii.c         |    7 ++--
> drivers/net/macb.c                |   49 ++++++++++++++++-----------
> drivers/net/macb.h                |    2 +-
> drivers/net/mv643xx_eth.c         |   32 +++++++++++-------
> drivers/net/phy/fixed.c           |   29 ++++++++++-----
> drivers/net/phy/mdio-bitbang.c    |    6 +--
> drivers/net/phy/mdio-ofgpio.c     |    9 ++---
> drivers/net/phy/mdio_bus.c        |   24 +++++++++++++
> drivers/net/sb1250-mac.c          |   36 ++++++++++++--------
> drivers/net/sh_eth.c              |    2 +-
> drivers/net/tc35815.c             |   45 +++++++++++++++----------
> drivers/net/tg3.c                 |   66 ++++++++++++++++++ 
> +------------------
> drivers/net/tg3.h                 |    2 +-
> drivers/net/ucc_geth_mii.c        |    7 ++--
> include/linux/phy.h               |    2 +
> 23 files changed, 279 insertions(+), 187 deletions(-)


Hrm.  I know this is to set up for your next patch, which does  
something with mdiobus_alloc(), but this patch creates an abstraction  
which actually increases the total amount of code.  I'm not quite sure  
the following patch provides additional functionality that justifies  
abstracting kzalloc.

What is the motivation for doing this?  Is it necessary to make the  
bus dynamically allocated for the device layer to function properly?

A few more comments below:



> +	if (aup->mii_bus != NULL) {
> +		mdiobus_unregister(aup->mii_bus);
> +		mdiobus_free(aup->mii_bus);

Should we consider merging free and unregister, since they seem like  
they'd be called together every time.


>
> @@ -231,12 +231,11 @@ static int fs_enet_mdio_remove(struct  
> of_device *ofdev)
> 	struct bb_info *bitbang = bus->priv;
>
> 	mdiobus_unregister(bus);
> -	free_mdio_bitbang(bus);
> 	dev_set_drvdata(&ofdev->dev, NULL);
> 	kfree(bus->irq);
> +	free_mdio_bitbang(bus);
> 	iounmap(bitbang->dir);
> 	kfree(bitbang);
> -	kfree(bus);
>
> 	return 0;
> }

Hm.  Was the original kfree(bus) a bug?  It looks like  
free_mdio_bitbang was doing that, anyway...

If not, why don't we need an mdiobus_free() here?



> --- a/drivers/net/phy/mdio-bitbang.c
> +++ b/drivers/net/phy/mdio-bitbang.c
> @@ -165,9 +165,7 @@ struct mii_bus *alloc_mdio_bitbang(struct  
> mdiobb_ctrl *ctrl)
> {
> 	struct mii_bus *bus;
>
> -	bus = kzalloc(sizeof(struct mii_bus), GFP_KERNEL);
> -	if (!bus)
> -		return NULL;
> +	bus = mdiobus_alloc();


I think you removed the error checking, here.



> @@ -168,11 +168,10 @@ static int mdio_ofgpio_remove(struct of_device  
> *ofdev)
> 	struct mdio_gpio_info *bitbang = bus->priv;
>
> 	mdiobus_unregister(bus);
> +	kfree(bus->irq);
> 	free_mdio_bitbang(bus);
> 	dev_set_drvdata(&ofdev->dev, NULL);
> -	kfree(bus->irq);
> 	kfree(bitbang);
> -	kfree(bus);


Same question here as the previous free_mdio_bitbang() invocation.




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html