Jonas Gorski <j...@openwrt.org> schrieb:
>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.
Thanks for the review, I'll fix in the next version.
>> 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).
Hmm, that could habe been the reason. However, using probe to register the
switch device sounds way more sane to me then using config_init.
Let me check next week ...
Helmut
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
