On Fri, Jan 24, 2014 at 12:57 PM, Helmut Schaa
<helmut.sc...@googlemail.com> wrote:
> When setting the associated interface down and up again a new
> switch device will be registered due to b53_phy_config_init
> doing the necessary allocations and registrations.
>
> Instead, register the switch device already in b53_phy_probe.
Generally I'm fine with it, but two issues I saw ...
>
> Signed-off-by: Helmut Schaa <helmut.sc...@googlemail.com>
> ---
> .../generic/files/drivers/net/phy/b53/b53_common.c | 2 +-
> .../generic/files/drivers/net/phy/b53/b53_mdio.c | 40
> ++++++++--------------
> .../generic/files/drivers/net/phy/b53/b53_priv.h | 2 ++
> 3 files changed, 17 insertions(+), 27 deletions(-)
>
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> b/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> index b82bc93..37f520d 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_common.c
> @@ -478,7 +478,7 @@ static void b53_switch_reset_gpio(struct b53_device *dev)
> dev->current_page = 0xff;
> }
>
> -static int b53_switch_reset(struct b53_device *dev)
> +int b53_switch_reset(struct b53_device *dev)
> {
> u8 mgmt;
>
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> b/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> index b86ea1a..e626217 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_mdio.c
> @@ -253,56 +253,44 @@ static struct b53_io_ops b53_mdio_ops = {
>
> static int b53_phy_probe(struct phy_device *phydev)
> {
> - struct b53_device dev;
> + struct b53_device *dev;
> int ret;
>
> /* allow the generic phy driver to take over */
> if (phydev->addr != B53_PSEUDO_PHY && phydev->addr != 0)
> return -ENODEV;
>
> - dev.current_page = 0xff;
> - dev.priv = phydev->bus;
> - dev.ops = &b53_mdio_ops;
> - dev.pdata = NULL;
> - mutex_init(&dev.reg_mutex);
> + dev = b53_switch_alloc(&phydev->dev, &b53_mdio_ops, phydev->bus);
> + if (!dev)
> + return -ENOMEM;
>
> - ret = b53_switch_detect(&dev);
> - if (ret)
> + dev->current_page = 0xff;
> +
> + ret = b53_switch_register(dev);
> + if (ret) {
> + kfree(dev);
The b53_device is allocated with devm_kzalloc and automatically free'd
on exit/failure, so this would cause a double free.
> return ret;
> + }
>
> - if (is5325(&dev) || is5365(&dev))
> + if (is5325(dev) || is5365(dev))
> phydev->supported = SUPPORTED_100baseT_Full;
> else
> phydev->supported = SUPPORTED_1000baseT_Full;
>
> phydev->advertising = phydev->supported;
> + phydev->priv = dev;
>
> return 0;
> }
>
> static int b53_phy_config_init(struct phy_device *phydev)
> {
> - struct b53_device *dev;
> - int ret;
> + struct b53_device *dev = phydev->priv;
>
> - dev = b53_switch_alloc(&phydev->dev, &b53_mdio_ops, phydev->bus);
> - if (!dev)
> - return -ENOMEM;
> -
> - /* we don't use page 0xff, so force a page set */
> - dev->current_page = 0xff;
> /* force the ethX as alias */
> dev->sw_dev.alias = phydev->attached_dev->name;
I'm not sure it is safe to modify sw_dev after registration. We should
probably either drop the ethX alias or set it in _probe() (IIRC
phydev->attached_dev->name wasn't set yet in _probe(), which was the
reason for doing it in _config_init(), but I might misremember).
>
> - ret = b53_switch_register(dev);
> - if (ret) {
> - dev_err(dev->dev, "failed to register switch: %i\n", ret);
> - return ret;
> - }
> -
> - phydev->priv = dev;
> -
> - return 0;
> + return b53_switch_reset(dev);
> }
>
> static void b53_phy_remove(struct phy_device *phydev)
> diff --git a/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> b/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> index ce5b530..75b86dc 100644
> --- a/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> +++ b/target/linux/generic/files/drivers/net/phy/b53/b53_priv.h
> @@ -180,6 +180,8 @@ int b53_switch_detect(struct b53_device *dev);
>
> int b53_switch_register(struct b53_device *dev);
>
> +int b53_switch_reset(struct b53_device *dev);
> +
> static inline void b53_switch_remove(struct b53_device *dev)
> {
> unregister_switch(&dev->sw_dev);
Jonas
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
