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: <CAGXr9JFhD+tguddQ3c9Lv+akbJo2d0Fp0_JBx7pU3A+L=b6RHw@mail.gmail.com>
Date:	Wed, 1 Oct 2014 15:16:38 -0700
From:	Petri Gynther <pgynther@...gle.com>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
	thomas.petazzoni@...e-electrons.com
Subject: Re: [PATCH net-next] net: phy: adjust fixed_phy_register() return value

Hi Florian,

On Wed, Oct 1, 2014 at 2:56 PM, Florian Fainelli <f.fainelli@...il.com> wrote:
> On 10/01/2014 02:45 PM, Petri Gynther wrote:
>> Adjust fixed_phy_register() to return struct phy_device *, so that
>> it becomes easy to use fixed PHYs without device tree support:
>>
>>   phydev = fixed_phy_register(PHY_POLL, &fixed_phy_status, NULL);
>>   fixed_phy_set_link_update(phydev, fixed_phy_link_update);
>>   phy_connect_direct(netdev, phydev, handler_fn, phy_interface);
>>
>> This change is a prerequisite for modifying bcmgenet driver to work
>> without a device tree on Broadcom's MIPS-based 7xxx platforms.
>
> Without a Device Tree enabled platform, you still have the
> fixed_phy_add() API to register a fixed PHY at a known location (the
> second parameter), did we someone broke that with Thomas' patches?
>

If a driver uses fixed_phy_add() directly, it then has to call
get_phy_device() and phy_device_register() as well. But, all that is
already handled nicely inside fixed_phy_register(). And,
fixed_phy_register() also handles the fixed PHY address allocation
automatically.

In my opinion, this change makes it really easy to use fixed PHYs from
non-OF code, as I showed in the change description.

-- Petri

> You could for instance register a fixed PHY at address 1 for the
> GENET/MoCA instance to find it.
>
> Other than that, I am not opposed to the change, since it allows us to
> eventually get rid of fixed_phy_add() in the future.
>
> Thanks!
>
>>
>> Signed-off-by: Petri Gynther <pgynther@...gle.com>
>> ---
>>  drivers/net/phy/fixed.c   | 16 ++++++++--------
>>  drivers/of/of_mdio.c      |  7 +++++--
>>  include/linux/phy_fixed.h | 14 +++++++-------
>>  3 files changed, 20 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/phy/fixed.c b/drivers/net/phy/fixed.c
>> index 5b19fbb..47872caa 100644
>> --- a/drivers/net/phy/fixed.c
>> +++ b/drivers/net/phy/fixed.c
>> @@ -233,9 +233,9 @@ EXPORT_SYMBOL_GPL(fixed_phy_del);
>>  static int phy_fixed_addr;
>>  static DEFINE_SPINLOCK(phy_fixed_addr_lock);
>>
>> -int fixed_phy_register(unsigned int irq,
>> -                    struct fixed_phy_status *status,
>> -                    struct device_node *np)
>> +struct phy_device *fixed_phy_register(unsigned int irq,
>> +                                   struct fixed_phy_status *status,
>> +                                   struct device_node *np)
>>  {
>>       struct fixed_mdio_bus *fmb = &platform_fmb;
>>       struct phy_device *phy;
>> @@ -246,19 +246,19 @@ int fixed_phy_register(unsigned int irq,
>>       spin_lock(&phy_fixed_addr_lock);
>>       if (phy_fixed_addr == PHY_MAX_ADDR) {
>>               spin_unlock(&phy_fixed_addr_lock);
>> -             return -ENOSPC;
>> +             return ERR_PTR(-ENOSPC);
>>       }
>>       phy_addr = phy_fixed_addr++;
>>       spin_unlock(&phy_fixed_addr_lock);
>>
>>       ret = fixed_phy_add(PHY_POLL, phy_addr, status);
>>       if (ret < 0)
>> -             return ret;
>> +             return ERR_PTR(ret);
>>
>>       phy = get_phy_device(fmb->mii_bus, phy_addr, false);
>>       if (!phy || IS_ERR(phy)) {
>>               fixed_phy_del(phy_addr);
>> -             return -EINVAL;
>> +             return ERR_PTR(-EINVAL);
>>       }
>>
>>       of_node_get(np);
>> @@ -269,10 +269,10 @@ int fixed_phy_register(unsigned int irq,
>>               phy_device_free(phy);
>>               of_node_put(np);
>>               fixed_phy_del(phy_addr);
>> -             return ret;
>> +             return ERR_PTR(ret);
>>       }
>>
>> -     return 0;
>> +     return phy;
>>  }
>>
>>  static int __init fixed_mdio_bus_init(void)
>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>> index a85d800..907baa9 100644
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -286,6 +286,7 @@ int of_phy_register_fixed_link(struct device_node *np)
>>       struct device_node *fixed_link_node;
>>       const __be32 *fixed_link_prop;
>>       int len;
>> +     struct phy_device *phy;
>>
>>       /* New binding */
>>       fixed_link_node = of_get_child_by_name(np, "fixed-link");
>> @@ -299,7 +300,8 @@ int of_phy_register_fixed_link(struct device_node *np)
>>               status.asym_pause = of_property_read_bool(fixed_link_node,
>>                                                         "asym-pause");
>>               of_node_put(fixed_link_node);
>> -             return fixed_phy_register(PHY_POLL, &status, np);
>> +             phy = fixed_phy_register(PHY_POLL, &status, np);
>> +             return (!phy || IS_ERR(phy));
>>       }
>>
>>       /* Old binding */
>> @@ -310,7 +312,8 @@ int of_phy_register_fixed_link(struct device_node *np)
>>               status.speed = be32_to_cpu(fixed_link_prop[2]);
>>               status.pause = be32_to_cpu(fixed_link_prop[3]);
>>               status.asym_pause = be32_to_cpu(fixed_link_prop[4]);
>> -             return fixed_phy_register(PHY_POLL, &status, np);
>> +             phy = fixed_phy_register(PHY_POLL, &status, np);
>> +             return (!phy || IS_ERR(phy));
>>       }
>>
>>       return -ENODEV;
>> diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
>> index 9411386..f2ca1b4 100644
>> --- a/include/linux/phy_fixed.h
>> +++ b/include/linux/phy_fixed.h
>> @@ -14,9 +14,9 @@ struct device_node;
>>  #ifdef CONFIG_FIXED_PHY
>>  extern int fixed_phy_add(unsigned int irq, int phy_id,
>>                        struct fixed_phy_status *status);
>> -extern int fixed_phy_register(unsigned int irq,
>> -                           struct fixed_phy_status *status,
>> -                           struct device_node *np);
>> +extern struct phy_device *fixed_phy_register(unsigned int irq,
>> +                                          struct fixed_phy_status *status,
>> +                                          struct device_node *np);
>>  extern void fixed_phy_del(int phy_addr);
>>  extern int fixed_phy_set_link_update(struct phy_device *phydev,
>>                       int (*link_update)(struct net_device *,
>> @@ -27,11 +27,11 @@ static inline int fixed_phy_add(unsigned int irq, int phy_id,
>>  {
>>       return -ENODEV;
>>  }
>> -static inline int fixed_phy_register(unsigned int irq,
>> -                                  struct fixed_phy_status *status,
>> -                                  struct device_node *np)
>> +static inline struct phy_device *fixed_phy_register(unsigned int irq,
>> +                                             struct fixed_phy_status *status,
>> +                                             struct device_node *np)
>>  {
>> -     return -ENODEV;
>> +     return ERR_PTR(-ENODEV);
>>  }
>>  static inline int fixed_phy_del(int phy_addr)
>>  {
>>
>
--
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ