| Message ID | 20220209153257.284853-2-daniel.klauer@gin.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | [1/2] miiphyutil: Fix inconsistent miiphy_write() error return value | expand |
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.
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)
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(-)