| 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
| ||
|
Date: Wed, 6 Feb 2019 14:41:27 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Moritz Fischer <mdf@...nel.org>, Andrew Lunn <andrew@...n.ch>
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
netdev@...r.kernel.org, colin.king@...onical.com,
linus.walleij@...aro.org, yuehaibing@...wei.com, mcgrof@...nel.org,
frowand.list@...il.com, robh+dt@...nel.org,
UNGLinuxDriver@...rochip.com, woojung.huh@...rochip.com,
hkallweit1@...il.com, davem@...emloft.net,
vivien.didelot@...il.com, moritz@...us.com, alex.williams@...com
Subject: Re: [RFC net-next] net: fixed_phy: Move the DT based link GPIO
parsing to of_mdio.c
On 2/6/19 2:38 PM, Moritz Fischer wrote:
> Hi Andrew,
>
> thanks for your feedback.
>
> On Wed, Feb 06, 2019 at 10:53:22PM +0100, Andrew Lunn wrote:
>> On Wed, Feb 06, 2019 at 12:51:06PM -0800, Moritz Fischer wrote:
>>> Move the DT based link GPIO parsing to of_mdio and let the places
>>> that register a fixed_phy pass in a GPIO descriptor or NULL.
>>>
>>> This allows fixed_phy on non-DT platforms to have link GPIOs, too.
>>>
>>> Signed-off-by: Moritz Fischer <mdf@...nel.org>
>>> ---
>>> drivers/net/dsa/dsa_loop.c | 2 +-
>>> drivers/net/ethernet/broadcom/bgmac.c | 2 +-
>>> drivers/net/ethernet/broadcom/genet/bcmmii.c | 2 +-
>>> drivers/net/phy/fixed_phy.c | 48 ++------------------
>>> drivers/net/usb/lan78xx.c | 2 +-
>>> drivers/of/of_mdio.c | 15 +++++-
>>> include/linux/phy_fixed.h | 3 +-
>>> 7 files changed, 23 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
>>> index 17482ae09aa5..7f124c620092 100644
>>> --- a/drivers/net/dsa/dsa_loop.c
>>> +++ b/drivers/net/dsa/dsa_loop.c
>>> @@ -343,7 +343,7 @@ static int __init dsa_loop_init(void)
>>> unsigned int i;
>>>
>>> for (i = 0; i < NUM_FIXED_PHYS; i++)
>>> - phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL);
>>> + phydevs[i] = fixed_phy_register(PHY_POLL, &status, NULL, NULL);
>>>
>>> return mdio_driver_register(&dsa_loop_drv);
>>> }
>>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>>> index 4632dd5dbad1..bce644dec5c2 100644
>>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>>> @@ -1446,7 +1446,7 @@ int bgmac_phy_connect_direct(struct bgmac *bgmac)
>>> struct phy_device *phy_dev;
>>> int err;
>>>
>>> - phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>>> + phy_dev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
>>> if (!phy_dev || IS_ERR(phy_dev)) {
>>> dev_err(bgmac->dev, "Failed to register fixed PHY device\n");
>>> return -ENODEV;
>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> index 51880d83131a..7cbd737aba80 100644
>>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>>> @@ -525,7 +525,7 @@ static int bcmgenet_mii_pd_init(struct bcmgenet_priv *priv)
>>> .asym_pause = 0,
>>> };
>>>
>>> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>>> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
>>> if (!phydev || IS_ERR(phydev)) {
>>> dev_err(kdev, "failed to register fixed PHY device\n");
>>> return -ENODEV;
>>> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
>>> index d810f914aaa4..845bd7c2065a 100644
>>> --- a/drivers/net/phy/fixed_phy.c
>>> +++ b/drivers/net/phy/fixed_phy.c
>>> @@ -18,6 +18,7 @@
>>> #include <linux/err.h>
>>> #include <linux/slab.h>
>>> #include <linux/of.h>
>>> +#include <linux/of_mdio.h>
>>> #include <linux/gpio/consumer.h>
>>> #include <linux/seqlock.h>
>>> #include <linux/idr.h>
>>> @@ -191,50 +192,12 @@ static void fixed_phy_del(int phy_addr)
>>> }
>>> }
>>>
>>> -#ifdef CONFIG_OF_GPIO
>>> -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
>>> -{
>>> - struct device_node *fixed_link_node;
>>> - struct gpio_desc *gpiod;
>>> -
>>> - if (!np)
>>> - return NULL;
>>> -
>>> - fixed_link_node = of_get_child_by_name(np, "fixed-link");
>>> - if (!fixed_link_node)
>>> - return NULL;
>>> -
>>> - /*
>>> - * As the fixed link is just a device tree node without any
>>> - * Linux device associated with it, we simply have obtain
>>> - * the GPIO descriptor from the device tree like this.
>>> - */
>>> - gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
>>> - GPIOD_IN, "mdio");
>>> - of_node_put(fixed_link_node);
>>> - if (IS_ERR(gpiod)) {
>>> - if (PTR_ERR(gpiod) == -EPROBE_DEFER)
>>> - return gpiod;
>>> - pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
>>> - fixed_link_node);
>>> - gpiod = NULL;
>>> - }
>>> -
>>> - return gpiod;
>>> -}
>>> -#else
>>> -static struct gpio_desc *fixed_phy_get_gpiod(struct device_node *np)
>>> -{
>>> - return NULL;
>>> -}
>>> -#endif
>>> -
>>> struct phy_device *fixed_phy_register(unsigned int irq,
>>> struct fixed_phy_status *status,
>>> - struct device_node *np)
>>> + struct device_node *np,
>>> + struct gpio_desc *gpiod)
>>> {
>>> struct fixed_mdio_bus *fmb = &platform_fmb;
>>> - struct gpio_desc *gpiod = NULL;
>>> struct phy_device *phy;
>>> int phy_addr;
>>> int ret;
>>> @@ -242,11 +205,6 @@ struct phy_device *fixed_phy_register(unsigned int irq,
>>> if (!fmb->mii_bus || fmb->mii_bus->state != MDIOBUS_REGISTERED)
>>> return ERR_PTR(-EPROBE_DEFER);
>>>
>>> - /* Check if we have a GPIO associated with this fixed phy */
>>> - gpiod = fixed_phy_get_gpiod(np);
>>> - if (IS_ERR(gpiod))
>>> - return ERR_CAST(gpiod);
>>> -
>>> /* Get the next available PHY address, up to PHY_MAX_ADDR */
>>> phy_addr = ida_simple_get(&phy_fixed_ida, 0, PHY_MAX_ADDR, GFP_KERNEL);
>>> if (phy_addr < 0)
>>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>>> index 3d92ea6fcc02..bd88f0aef2fa 100644
>>> --- a/drivers/net/usb/lan78xx.c
>>> +++ b/drivers/net/usb/lan78xx.c
>>> @@ -2051,7 +2051,7 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
>>> phydev = phy_find_first(dev->mdiobus);
>>> if (!phydev) {
>>> netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
>>> - phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
>>> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL, NULL);
>>> if (IS_ERR(phydev)) {
>>> netdev_err(dev->net, "No PHY/fixed_PHY found\n");
>>> return NULL;
>>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>>> index de6157357e26..6be2120b5f03 100644
>>> --- a/drivers/of/of_mdio.c
>>> +++ b/drivers/of/of_mdio.c
>>> @@ -20,6 +20,7 @@
>>> #include <linux/of_mdio.h>
>>> #include <linux/of_net.h>
>>> #include <linux/module.h>
>>> +#include <linux/gpio/consumer.h>
>>>
>>> #define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
>>>
>>> @@ -460,6 +461,7 @@ int of_phy_register_fixed_link(struct device_node *np)
>>> {
>>> struct fixed_phy_status status = {};
>>> struct device_node *fixed_link_node;
>>> + struct gpio_desc *gpiod = NULL;
>>> u32 fixed_link_prop[5];
>>> const char *managed;
>>>
>>> @@ -483,7 +485,17 @@ int of_phy_register_fixed_link(struct device_node *np)
>>> status.pause = of_property_read_bool(fixed_link_node, "pause");
>>> status.asym_pause = of_property_read_bool(fixed_link_node,
>>> "asym-pause");
>>> +
>>> + gpiod = gpiod_get_from_of_node(fixed_link_node, "link-gpios", 0,
>>> + GPIOD_IN, "mdio");
>>> of_node_put(fixed_link_node);
>>> + if (IS_ERR(gpiod)) {
>>> + if (PTR_ERR(gpiod) == -EPROBE_DEFER)
>>> + return PTR_ERR(gpiod);
>>> + pr_err("error getting GPIO for fixed link %pOF, proceed without\n",
>>> + fixed_link_node);
>>> + gpiod = NULL;
>>> extern struct phy_device *fixed_phy_register(unsigned int irq,
>>> struct fixed_phy_status *status,
>>> - struct device_node *np);
>>> + struct device_node *np,
>>> + struct gpio_desc *gpiod);
>>
>> Hi Moritz
>>
>> I think it would be better to add a
>>
>> extern struct phy_device *fixed_phy_register_gpiod(unsigned int irq,
>> struct fixed_phy_status *status,
>> struct gpio_desc *gpiod);
>>
>> If you are not using DT, the np is pointless. So lets keep the API
>> simple.
>
> To clarify: Do you agree with moving the parsing to of_mdio, or do you
> want to keep the parsing as it was and just add an additional function.
>
> In the latter case you'd have a
> static struct phy_device *__fixed_phy_register(unsigned int irq,
> struct fixed_phy_status *status,
> struct device_node *np,
> struct gpio_desc *gpiod)
> {
> if (gpiod)
> use that
> else
> old behavior / scan dt?
> }
>
> That we then call from fixed_phy_register_gpiod() with a np of NULL,
> and from fixed_phy_register() with a gpiod of NULL?
If you have existing users in tree, of something like:
foo(a, b, c)
and you want to introduce a "d" argument, it's a good practice to do:
foo(a, b, c) -> __foo(a, b, c, 0)
foo_with_d(a, b, c, d) __foo(a, b, c, d)
and have the body of foo() become __foo() while taking the new argument.
Hope this is clear.
--
Florian
Powered by blists - more mailing lists