From: Bartosz Golaszewski <[email protected]>
Earlier today I sent the first patch as a solution to a regression
introduced during the v4.16 merge window, but after testing David's
common clock series on top of 4.18-rc1 + this patch it turned out that
the problem persisted.
This is a follow-up containing the regression fix and two additional
patches that make suspend/resume work with David's changes.
Bartosz Golaszewski (3):
net: ethernet: fix suspend/resume in davinci_emac
net: phy: set the of_node in the mdiodev's struct device
net: davinci_emac: match the mdio device against its compatible if
possible
drivers/net/ethernet/ti/davinci_emac.c | 19 +++++++++++++++++--
drivers/net/phy/phy_device.c | 1 +
2 files changed, 18 insertions(+), 2 deletions(-)
--
2.17.1
From: Bartosz Golaszewski <[email protected]>
Device tree based systems without of_dev_auxdata will have the mdio
device named differently than "davinci_mdio(.0)". In this case use the
device's compatible string for matching.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/net/ethernet/ti/davinci_emac.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index a1a6445b5a7e..c28a35bb852f 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1387,6 +1387,10 @@ static int emac_devioctl(struct net_device *ndev, struct ifreq *ifrq, int cmd)
static int match_first_device(struct device *dev, void *data)
{
+ if (dev->of_node)
+ return of_device_is_compatible(dev->of_node,
+ "ti,davinci_mdio");
+
return !strncmp(dev_name(dev), "davinci_mdio", 12);
}
--
2.17.1
From: Bartosz Golaszewski <[email protected]>
Copy the of_node over from mii_bus's struct device. This is needed
for device-tree systems to be able to check the mdio device's
compatible string.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/net/phy/phy_device.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index bd0f339f69fd..a92d5ee61813 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -411,6 +411,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
mdiodev->dev.parent = &bus->dev;
mdiodev->dev.bus = &mdio_bus_type;
mdiodev->dev.type = &mdio_bus_phy_type;
+ mdiodev->dev.of_node = bus->dev.of_node;
mdiodev->bus = bus;
mdiodev->bus_match = phy_bus_match;
mdiodev->addr = addr;
--
2.17.1
From: Bartosz Golaszewski <[email protected]>
This patch reverts commit 3243ff2a05ec ("net: ethernet: davinci_emac:
Deduplicate bus_find_device() by name matching") and adds a comment
which should stop anyone from reintroducing the same "fix" in the future.
We can't use bus_find_device_by_name() here because the device name is
not guaranteed to be 'davinci_mdio'. On some systems it can be
'davinci_mdio.0' so we need to use strncmp() against the first part of
the string to correctly match it.
Fixes: 3243ff2a05ec ("net: ethernet: davinci_emac: Deduplicate bus_find_device() by name matching")
Cc: [email protected]
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/net/ethernet/ti/davinci_emac.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 06d7c9e4dcda..a1a6445b5a7e 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -1385,6 +1385,11 @@ static int emac_devioctl(struct net_device *ndev, struct ifreq *ifrq, int cmd)
return -EOPNOTSUPP;
}
+static int match_first_device(struct device *dev, void *data)
+{
+ return !strncmp(dev_name(dev), "davinci_mdio", 12);
+}
+
/**
* emac_dev_open - EMAC device open
* @ndev: The DaVinci EMAC network adapter
@@ -1484,8 +1489,14 @@ static int emac_dev_open(struct net_device *ndev)
/* use the first phy on the bus if pdata did not give us a phy id */
if (!phydev && !priv->phy_id) {
- phy = bus_find_device_by_name(&mdio_bus_type, NULL,
- "davinci_mdio");
+ /* NOTE: we can't use bus_find_device_by_name() here because
+ * the device name is not guaranteed to be 'davinci_mdio'. On
+ * some systems it can be 'davinci_mdio.0' so we need to use
+ * strncmp() against the first part of the string to correctly
+ * match it.
+ */
+ phy = bus_find_device(&mdio_bus_type, NULL, NULL,
+ match_first_device);
if (phy) {
priv->phy_id = dev_name(phy);
if (!priv->phy_id || !*priv->phy_id)
--
2.17.1
On 06/19/2018 09:09 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> This patch reverts commit 3243ff2a05ec ("net: ethernet: davinci_emac:
> Deduplicate bus_find_device() by name matching") and adds a comment
> which should stop anyone from reintroducing the same "fix" in the future.
>
> We can't use bus_find_device_by_name() here because the device name is
> not guaranteed to be 'davinci_mdio'. On some systems it can be
> 'davinci_mdio.0' so we need to use strncmp() against the first part of
> the string to correctly match it.
>
> Fixes: 3243ff2a05ec ("net: ethernet: davinci_emac: Deduplicate bus_find_device() by name matching")
> Cc: [email protected]
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/net/ethernet/ti/davinci_emac.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index 06d7c9e4dcda..a1a6445b5a7e 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1385,6 +1385,11 @@ static int emac_devioctl(struct net_device *ndev, struct ifreq *ifrq, int cmd)
> return -EOPNOTSUPP;
> }
>
> +static int match_first_device(struct device *dev, void *data)
> +{
> + return !strncmp(dev_name(dev), "davinci_mdio", 12);
const char *bus_name = "davinci_mdio";
return !strncmp(dev_name(dev), bus_name, strlen(bus_name));
Or even better yet, if you want to make sure this really is a PHY device
that you are trying to match, you could try to use sscanf() with PHY_ID_FMT.
--
Florian
On 06/19/2018 09:09 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Copy the of_node over from mii_bus's struct device. This is needed
> for device-tree systems to be able to check the mdio device's
> compatible string.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/net/phy/phy_device.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index bd0f339f69fd..a92d5ee61813 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -411,6 +411,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
> mdiodev->dev.parent = &bus->dev;
> mdiodev->dev.bus = &mdio_bus_type;
> mdiodev->dev.type = &mdio_bus_phy_type;
> + mdiodev->dev.of_node = bus->dev.of_node;
That does not quite make sense to me, the mdio device's parent already
points to &bus->dev, which would get you the correct of_node. You are
breaking the parent/child relationship here. From patch 3, see my
comments there, it does not look like you are matching on the right
device level.
> mdiodev->bus = bus;
> mdiodev->bus_match = phy_bus_match;
> mdiodev->addr = addr;
>
--
Florian
On 06/19/2018 09:09 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Device tree based systems without of_dev_auxdata will have the mdio
> device named differently than "davinci_mdio(.0)". In this case use the
> device's compatible string for matching.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/net/ethernet/ti/davinci_emac.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index a1a6445b5a7e..c28a35bb852f 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1387,6 +1387,10 @@ static int emac_devioctl(struct net_device *ndev, struct ifreq *ifrq, int cmd)
>
> static int match_first_device(struct device *dev, void *data)
> {
> + if (dev->of_node)
> + return of_device_is_compatible(dev->of_node,
> + "ti,davinci_mdio");
Why would we be matching the PHY device with the MDIO controller
compatibe string? Why not check dev->parent.of_node instead which would
make more sense?
--
Florian
On Tue, Jun 19, 2018 at 06:09:48PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> This patch reverts commit 3243ff2a05ec ("net: ethernet: davinci_emac:
> Deduplicate bus_find_device() by name matching") and adds a comment
> which should stop anyone from reintroducing the same "fix" in the future.
>
> We can't use bus_find_device_by_name() here because the device name is
> not guaranteed to be 'davinci_mdio'. On some systems it can be
> 'davinci_mdio.0' so we need to use strncmp() against the first part of
> the string to correctly match it.
>
> Fixes: 3243ff2a05ec ("net: ethernet: davinci_emac: Deduplicate bus_find_device() by name matching")
> Cc: [email protected]
> Signed-off-by: Bartosz Golaszewski <[email protected]>
This is still
Acked-by: Lukas Wunner <[email protected]>
given that my patch which is reverted here seems to have been incorrect.
Feel free to keep the ack if you respin in response to Florian Fainelli's
comments.
Thanks,
Lukas
> ---
> drivers/net/ethernet/ti/davinci_emac.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index 06d7c9e4dcda..a1a6445b5a7e 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -1385,6 +1385,11 @@ static int emac_devioctl(struct net_device *ndev, struct ifreq *ifrq, int cmd)
> return -EOPNOTSUPP;
> }
>
> +static int match_first_device(struct device *dev, void *data)
> +{
> + return !strncmp(dev_name(dev), "davinci_mdio", 12);
> +}
> +
> /**
> * emac_dev_open - EMAC device open
> * @ndev: The DaVinci EMAC network adapter
> @@ -1484,8 +1489,14 @@ static int emac_dev_open(struct net_device *ndev)
>
> /* use the first phy on the bus if pdata did not give us a phy id */
> if (!phydev && !priv->phy_id) {
> - phy = bus_find_device_by_name(&mdio_bus_type, NULL,
> - "davinci_mdio");
> + /* NOTE: we can't use bus_find_device_by_name() here because
> + * the device name is not guaranteed to be 'davinci_mdio'. On
> + * some systems it can be 'davinci_mdio.0' so we need to use
> + * strncmp() against the first part of the string to correctly
> + * match it.
> + */
> + phy = bus_find_device(&mdio_bus_type, NULL, NULL,
> + match_first_device);
> if (phy) {
> priv->phy_id = dev_name(phy);
> if (!priv->phy_id || !*priv->phy_id)
> --
> 2.17.1
>
2018-06-19 18:55 GMT+02:00 Florian Fainelli <[email protected]>:
> On 06/19/2018 09:09 AM, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <[email protected]>
>>
>> This patch reverts commit 3243ff2a05ec ("net: ethernet: davinci_emac:
>> Deduplicate bus_find_device() by name matching") and adds a comment
>> which should stop anyone from reintroducing the same "fix" in the future.
>>
>> We can't use bus_find_device_by_name() here because the device name is
>> not guaranteed to be 'davinci_mdio'. On some systems it can be
>> 'davinci_mdio.0' so we need to use strncmp() against the first part of
>> the string to correctly match it.
>>
>> Fixes: 3243ff2a05ec ("net: ethernet: davinci_emac: Deduplicate bus_find_device() by name matching")
>> Cc: [email protected]
>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>> ---
>> drivers/net/ethernet/ti/davinci_emac.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
>> index 06d7c9e4dcda..a1a6445b5a7e 100644
>> --- a/drivers/net/ethernet/ti/davinci_emac.c
>> +++ b/drivers/net/ethernet/ti/davinci_emac.c
>> @@ -1385,6 +1385,11 @@ static int emac_devioctl(struct net_device *ndev, struct ifreq *ifrq, int cmd)
>> return -EOPNOTSUPP;
>> }
>>
>> +static int match_first_device(struct device *dev, void *data)
>> +{
>> + return !strncmp(dev_name(dev), "davinci_mdio", 12);
>
> const char *bus_name = "davinci_mdio";
>
> return !strncmp(dev_name(dev), bus_name, strlen(bus_name));
>
> Or even better yet, if you want to make sure this really is a PHY device
> that you are trying to match, you could try to use sscanf() with PHY_ID_FMT.
> --
> Florian
I don't think this is necessary. This simple function would get too
complicated with the additional buffer for the sscanf'ed phy name etc.
Thanks,
Bart