[PATCH net] device property: change device_get_phy_mode() to prevent signedess bugs

* [PATCH net] device property: change device_get_phy_mode() to prevent signedess bugs
@ 2020-01-31  4:59 Dan Carpenter
  2020-01-31  8:12 ` Andy Shevchenko
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: Dan Carpenter @ 2020-01-31  4:59 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Ajay Gupta, David S. Miller
  Cc: Rafael J. Wysocki, Iyappan Subramanian, Keyur Chudgar,
	Quan Nguyen, Steve Glendinning, Jassi Brar, Ilias Apalodimas,
	Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
	Maxime Coquelin, Dmitry Torokhov, Andy Shevchenko, Sakari Ailus,
	Heikki Krogerus, linux-kernel, netdev, linux-stm32,
	kernel-janitors

The device_get_phy_mode() was returning negative error codes on
failure and positive phy_interface_t values on success.  The problem is
that the phy_interface_t type is an enum which GCC treats as unsigned.
This lead to recurring signedness bugs where we check "if (phy_mode < 0)"
and "phy_mode" is unsigned.

In the commit 0c65b2b90d13 ("net: of_get_phy_mode: Change API to solve
int/unit warnings") we updated of_get_phy_mode() take a pointer to
phy_mode and only return zero on success and negatives on failure.  This
patch does the same thing for device_get_phy_mode().  Plus it's just
nice for the API to be the same in both places.

Fixes: b9f0b2f634c0 ("net: stmmac: platform: fix probe for ACPI devices")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
This is a change to drivers/base/ but all the users are ethernet drivers
so probably it makes sense for Dave to take this?

Also this fixes a bug in stmmac.  If you wanted I could make a one
liner fix for that and then write this change on top of that.  The bug
is only in v5.6 so it doesn't affect old kernels.

 drivers/base/property.c                               | 13 +++++++++++--
 drivers/net/ethernet/apm/xgene-v2/main.c              |  9 ++++-----
 drivers/net/ethernet/apm/xgene-v2/main.h              |  2 +-
 drivers/net/ethernet/apm/xgene/xgene_enet_main.c      |  6 +++---
 drivers/net/ethernet/apm/xgene/xgene_enet_main.h      |  2 +-
 drivers/net/ethernet/smsc/smsc911x.c                  |  8 +++-----
 drivers/net/ethernet/socionext/netsec.c               |  5 ++---
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c |  6 +++---
 include/linux/property.h                              |  3 ++-
 9 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 511f6d7acdfe..8854cfbd213b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -863,9 +863,18 @@ EXPORT_SYMBOL_GPL(fwnode_get_phy_mode);
  * 'phy-connection-type', and return its index in phy_modes table, or errno in
  * error case.
  */
-int device_get_phy_mode(struct device *dev)
+int device_get_phy_mode(struct device *dev, phy_interface_t *interface)
 {
-	return fwnode_get_phy_mode(dev_fwnode(dev));
+	int mode;
+
+	*interface = PHY_INTERFACE_MODE_NA;
+
+	mode = fwnode_get_phy_mode(dev_fwnode(dev));
+	if (mode < 0)
+		return mode;
+
+	*interface = mode;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(device_get_phy_mode);
 
diff --git a/drivers/net/ethernet/apm/xgene-v2/main.c b/drivers/net/ethernet/apm/xgene-v2/main.c
index c48f60996761..706602918dd1 100644
--- a/drivers/net/ethernet/apm/xgene-v2/main.c
+++ b/drivers/net/ethernet/apm/xgene-v2/main.c
@@ -15,7 +15,7 @@ static int xge_get_resources(struct xge_pdata *pdata)
 {
 	struct platform_device *pdev;
 	struct net_device *ndev;
-	int phy_mode, ret = 0;
+	int ret = 0;
 	struct resource *res;
 	struct device *dev;
 
@@ -41,12 +41,11 @@ static int xge_get_resources(struct xge_pdata *pdata)
 
 	memcpy(ndev->perm_addr, ndev->dev_addr, ndev->addr_len);
 
-	phy_mode = device_get_phy_mode(dev);
-	if (phy_mode < 0) {
+	ret = device_get_phy_mode(dev, &pdata->resources.phy_mode);
+	if (ret) {
 		dev_err(dev, "Unable to get phy-connection-type\n");
-		return phy_mode;
+		return ret;
 	}
-	pdata->resources.phy_mode = phy_mode;
 
 	if (pdata->resources.phy_mode != PHY_INTERFACE_MODE_RGMII) {
 		dev_err(dev, "Incorrect phy-connection-type specified\n");
diff --git a/drivers/net/ethernet/apm/xgene-v2/main.h b/drivers/net/ethernet/apm/xgene-v2/main.h
index d41439d2709d..d687f0185908 100644
--- a/drivers/net/ethernet/apm/xgene-v2/main.h
+++ b/drivers/net/ethernet/apm/xgene-v2/main.h
@@ -35,7 +35,7 @@
 
 struct xge_resource {
 	void __iomem *base_addr;
-	int phy_mode;
+	phy_interface_t phy_mode;
 	u32 irq;
 };
 
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index 6aee2f0fc0db..da35e70ccceb 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -1736,10 +1736,10 @@ static int xgene_enet_get_resources(struct xgene_enet_pdata *pdata)
 
 	memcpy(ndev->perm_addr, ndev->dev_addr, ndev->addr_len);
 
-	pdata->phy_mode = device_get_phy_mode(dev);
-	if (pdata->phy_mode < 0) {
+	ret = device_get_phy_mode(dev, &pdata->phy_mode);
+	if (ret) {
 		dev_err(dev, "Unable to get phy-connection-type\n");
-		return pdata->phy_mode;
+		return ret;
 	}
 	if (!phy_interface_mode_is_rgmii(pdata->phy_mode) &&
 	    pdata->phy_mode != PHY_INTERFACE_MODE_SGMII &&
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.h b/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
index 18f4923b1723..600cddf1942d 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.h
@@ -209,7 +209,7 @@ struct xgene_enet_pdata {
 	void __iomem *pcs_addr;
 	void __iomem *ring_csr_addr;
 	void __iomem *ring_cmd_addr;
-	int phy_mode;
+	phy_interface_t phy_mode;
 	enum xgene_enet_rm rm;
 	struct xgene_enet_cle cle;
 	u64 *extd_stats;
diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 49a6a9167af4..2d773e5e67f8 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c
@@ -2361,14 +2361,12 @@ static const struct smsc911x_ops shifted_smsc911x_ops = {
 static int smsc911x_probe_config(struct smsc911x_platform_config *config,
 				 struct device *dev)
 {
-	int phy_interface;
 	u32 width = 0;
 	int err;
 
-	phy_interface = device_get_phy_mode(dev);
-	if (phy_interface < 0)
-		phy_interface = PHY_INTERFACE_MODE_NA;
-	config->phy_interface = phy_interface;
+	err = device_get_phy_mode(dev, &config->phy_interface);
+	if (err)
+		config->phy_interface = PHY_INTERFACE_MODE_NA;
 
 	device_get_mac_address(dev, config->mac, ETH_ALEN);
 
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index e8224b543dfc..95ff91230523 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -1994,10 +1994,9 @@ static int netsec_probe(struct platform_device *pdev)
 	priv->msg_enable = NETIF_MSG_TX_ERR | NETIF_MSG_HW | NETIF_MSG_DRV |
 			   NETIF_MSG_LINK | NETIF_MSG_PROBE;
 
-	priv->phy_interface = device_get_phy_mode(&pdev->dev);
-	if ((int)priv->phy_interface < 0) {
+	ret = device_get_phy_mode(&pdev->dev, &priv->phy_interface);
+	if (ret) {
 		dev_err(&pdev->dev, "missing required property 'phy-mode'\n");
-		ret = -ENODEV;
 		goto free_ndev;
 	}
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index d10ac54bf385..aa77c332ea1d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -412,9 +412,9 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
 		*mac = NULL;
 	}
 
-	plat->phy_interface = device_get_phy_mode(&pdev->dev);
-	if (plat->phy_interface < 0)
-		return ERR_PTR(plat->phy_interface);
+	rc = device_get_phy_mode(&pdev->dev, &plat->phy_interface);
+	if (rc)
+		return ERR_PTR(rc);
 
 	plat->interface = stmmac_of_get_mac_mode(np);
 	if (plat->interface < 0)
diff --git a/include/linux/property.h b/include/linux/property.h
index d86de017c689..2ffe9842c997 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -12,6 +12,7 @@
 
 #include <linux/bits.h>
 #include <linux/fwnode.h>
+#include <linux/phy.h>
 #include <linux/types.h>
 
 struct device;
@@ -368,7 +369,7 @@ enum dev_dma_attr device_get_dma_attr(struct device *dev);
 
 const void *device_get_match_data(struct device *dev);
 
-int device_get_phy_mode(struct device *dev);
+int device_get_phy_mode(struct device *dev, phy_interface_t *interface);
 
 void *device_get_mac_address(struct device *dev, char *addr, int alen);
 
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread