[PATCH 1/2] miiphyutil: Fix inconsistent miiphy_write() error return value
miiphy_write() should not directly return the error return value from bus->write(), because that is typically a -errno value, while generally miiphy_write() and other miiphy_*() functions return 1 on error. Some miiphy_write() callers only check for > 0 to detect errors.
Fix it to match miiphy_read(), which also converts bus->read() failure to "return 1".
Signed-off-by: Daniel Klauer daniel.klauer@gin.de --- common/miiphyutil.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/common/miiphyutil.c b/common/miiphyutil.c index 7d4d15ed91..86a27665e3 100644 --- a/common/miiphyutil.c +++ b/common/miiphyutil.c @@ -236,7 +236,7 @@ static struct mii_dev *miiphy_get_active_dev(const char *devname) * This API is deprecated. Use phy_read on a phy_device found via phy_connect * * Returns: - * 0 on success + * 0 on success, 1 on error */ int miiphy_read(const char *devname, unsigned char addr, unsigned char reg, unsigned short *value) @@ -264,18 +264,23 @@ int miiphy_read(const char *devname, unsigned char addr, unsigned char reg, * This API is deprecated. Use phy_write on a phy_device found by phy_connect * * Returns: - * 0 on success + * 0 on success, 1 on error */ int miiphy_write(const char *devname, unsigned char addr, unsigned char reg, unsigned short value) { struct mii_dev *bus; + int ret;
bus = miiphy_get_active_dev(devname); - if (bus) - return bus->write(bus, addr, MDIO_DEVAD_NONE, reg, value); + if (!bus) + return 1;
- return 1; + ret = bus->write(bus, addr, MDIO_DEVAD_NONE, reg, value); + if (ret < 0) + return 1; + + return 0; }
/*****************************************************************************
The miiphy_read/miiphy_write functions return 1 on error, not -errno. Fix up the checks accordingly and insert -EIO as fallback error code.
Signed-off-by: Daniel Klauer daniel.klauer@gin.de --- drivers/net/phy/mv88e6352.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/net/phy/mv88e6352.c b/drivers/net/phy/mv88e6352.c index 56060762d8..a87af7ed24 100644 --- a/drivers/net/phy/mv88e6352.c +++ b/drivers/net/phy/mv88e6352.c @@ -36,16 +36,14 @@ static int sw_wait_rdy(const char *devname, u8 phy_addr) { u16 command; u32 timeout = 100; - int ret;
/* wait till the SMI is not busy */ do { /* read command register */ - ret = miiphy_read(devname, phy_addr, COMMAND_REG, &command); - if (ret < 0) { + if (miiphy_read(devname, phy_addr, COMMAND_REG, &command)) { printf("%s: Error reading command register\n", __func__); - return ret; + return -EIO; } if (timeout-- == 0) { printf("Err..(%s) SMI busy timeout\n", __func__); @@ -69,17 +67,17 @@ static int sw_reg_read(const char *devname, u8 phy_addr, u8 port, command = SMI_HDR | SMIRD_OP | ((port&SMI_MASK) << PORT_SHIFT) | (reg & SMI_MASK); debug("%s: write to command: %#x\n", __func__, command); - ret = miiphy_write(devname, phy_addr, COMMAND_REG, command); - if (ret) - return ret; + if (miiphy_write(devname, phy_addr, COMMAND_REG, command)) + return -EIO;
ret = sw_wait_rdy(devname, phy_addr); if (ret) return ret;
- ret = miiphy_read(devname, phy_addr, DATA_REG, data); + if (miiphy_read(devname, phy_addr, DATA_REG, data)) + return -EIO;
- return ret; + return 0; }
static int sw_reg_write(const char *devname, u8 phy_addr, u8 port, @@ -93,16 +91,14 @@ static int sw_reg_write(const char *devname, u8 phy_addr, u8 port, return ret;
debug("%s: write to data: %#x\n", __func__, data); - ret = miiphy_write(devname, phy_addr, DATA_REG, data); - if (ret) - return ret; + if (miiphy_write(devname, phy_addr, DATA_REG, data)) + return -EIO;
value = SMI_HDR | SMIWR_OP | ((port & SMI_MASK) << PORT_SHIFT) | (reg & SMI_MASK); debug("%s: write to command: %#x\n", __func__, value); - ret = miiphy_write(devname, phy_addr, COMMAND_REG, value); - if (ret) - return ret; + if (miiphy_write(devname, phy_addr, COMMAND_REG, value)) + return -EIO;
ret = sw_wait_rdy(devname, phy_addr); if (ret)
On Wed, Feb 9, 2022 at 5:41 PM Daniel Klauer daniel.klauer@gin.de wrote:
The miiphy_read/miiphy_write functions return 1 on error, not -errno.
Why don't you just fix the miiphy_read/miiphy_write functions ?
On 12.02.22 12:50, Ramon Fried wrote:
On Wed, Feb 9, 2022 at 5:41 PM Daniel Klauer daniel.klauer@gin.de wrote:
The miiphy_read/miiphy_write functions return 1 on error, not -errno.
Why don't you just fix the miiphy_read/miiphy_write functions ?
Other functions from that module do the same, so I assumed it's intentional. It could be fixed too though, with a corresponding fix up of the few callers that expect the positive return value on error.
On Mon, Feb 14, 2022 at 12:33 PM Daniel Klauer daniel.klauer@gin.de wrote:
On 12.02.22 12:50, Ramon Fried wrote:
On Wed, Feb 9, 2022 at 5:41 PM Daniel Klauer daniel.klauer@gin.de wrote:
The miiphy_read/miiphy_write functions return 1 on error, not -errno.
Why don't you just fix the miiphy_read/miiphy_write functions ?
Other functions from that module do the same, so I assumed it's intentional. It could be fixed too though, with a corresponding fix up of the few callers that expect the positive return value on error.
Sometimes a real fix needs more work. What caused you to change it on your side to -EIO, is there someone checking for EIO in the framework code ?
On 15.02.22 08:08, Ramon Fried wrote:
On Mon, Feb 14, 2022 at 12:33 PM Daniel Klauer daniel.klauer@gin.de wrote:
On 12.02.22 12:50, Ramon Fried wrote:
On Wed, Feb 9, 2022 at 5:41 PM Daniel Klauer daniel.klauer@gin.de wrote:
The miiphy_read/miiphy_write functions return 1 on error, not -errno.
Why don't you just fix the miiphy_read/miiphy_write functions ?
Other functions from that module do the same, so I assumed it's intentional. It could be fixed too though, with a corresponding fix up of the few callers that expect the positive return value on error.
Sometimes a real fix needs more work. What caused you to change it on your side to -EIO, is there someone checking for EIO in the framework code ?
It's not that anything specifically expects -EIO, I just picked it because I had to pick something and it seemed like a good solution. Of course that only makes sense with the current miiphy_read/write functions which don't propagate the real errno. If that were to be changed, this patch for mv88e6352.c would not be useful anymore.
participants (2)
-
Daniel Klauer -
Ramon Fried