[bug report] sfp: add SFP module support

[bug report] sfp: add SFP module support

[Dan Carpenter]

Hi Russell,

The patch 73970055450e: "sfp: add SFP module support" from Jul 25,
2017, leads to the following Smatch static checker warnings:

drivers/net/phy/sfp.c:474 sfp_soft_get_state() warn: passing zero to 'ERR_PTR'
drivers/net/phy/sfp.c:1710 sfp_sm_mod_hpower() warn: passing zero to 'ERR_PTR'
drivers/net/phy/sfp.c:1728 sfp_sm_mod_hpower() warn: passing zero to 'ERR_PTR'
drivers/net/phy/sfp.c:1781 sfp_cotsworks_fixup_check() warn: passing zero to 'ERR_PTR'
drivers/net/phy/sfp.c:1794 sfp_cotsworks_fixup_check() warn: passing zero to 'ERR_PTR'
drivers/net/phy/sfp.c:1827 sfp_sm_mod_probe() warn: passing zero to 'ERR_PTR'
drivers/net/phy/sfp.c:1854 sfp_sm_mod_probe() warn: passing zero to 'ERR_PTR'
drivers/net/phy/sfp.c:1903 sfp_sm_mod_probe() warn: passing zero to 'ERR_PTR'

drivers/net/phy/sfp.c
    1767 static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)
    1768 {
    1769         u8 check;
    1770         int err;
    1771 
    1772         if (id->base.phys_id != SFF8024_ID_SFF_8472 ||
    1773             id->base.phys_ext_id != SFP_PHYS_EXT_ID_SFP ||
    1774             id->base.connector != SFF8024_CONNECTOR_LC) {
    1775                 dev_warn(sfp->dev, "Rewriting fiber module EEPROM with corrected values\n");
    1776                 id->base.phys_id = SFF8024_ID_SFF_8472;
    1777                 id->base.phys_ext_id = SFP_PHYS_EXT_ID_SFP;
    1778                 id->base.connector = SFF8024_CONNECTOR_LC;
    1779                 err = sfp_write(sfp, false, SFP_PHYS_ID, &id->base, 3);
    1780                 if (err != 3) {
    1781                         dev_err(sfp->dev,
    1782                                 "Failed to rewrite module EEPROM: %pe\n",
    1783                                 ERR_PTR(err));

The sfp_i2c_read/write() functions return negatives for errors, zero for
partial read/writes and len (3 in this case) for success.  If we have
to bail out at this point, the I feel we should return a negative error
code instead of success.

    1784                         return err;
    1785                 }
    1786 
    1787                 /* Cotsworks modules have been found to require a delay between write operations. */
    1788                 mdelay(50);
    1789 
    1790                 /* Update base structure checksum */
    1791                 check = sfp_check(&id->base, sizeof(id->base) - 1);
    1792                 err = sfp_write(sfp, false, SFP_CC_BASE, &check, 1);
    1793                 if (err != 1) {
--> 1794                         dev_err(sfp->dev,
    1795                                 "Failed to update base structure checksum in fiber module EEPROM: %pe\n",
    1796                                 ERR_PTR(err));
    1797                         return err;
    1798                 }
    1799         }
    1800         return 0;
    1801 }

regards,
dan carpenter

Re: [bug report] sfp: add SFP module support

[Russell King (Oracle)]

On Mon, Jul 18, 2022 at 04:00:55PM +0300, Dan Carpenter wrote:
> Hi Russell,
> 
> The patch 73970055450e: "sfp: add SFP module support" from Jul 25,
> 2017, leads to the following Smatch static checker warnings:
> 
> drivers/net/phy/sfp.c:474 sfp_soft_get_state() warn: passing zero to 'ERR_PTR'
> drivers/net/phy/sfp.c:1710 sfp_sm_mod_hpower() warn: passing zero to 'ERR_PTR'
> drivers/net/phy/sfp.c:1728 sfp_sm_mod_hpower() warn: passing zero to 'ERR_PTR'
> drivers/net/phy/sfp.c:1781 sfp_cotsworks_fixup_check() warn: passing zero to 'ERR_PTR'
> drivers/net/phy/sfp.c:1794 sfp_cotsworks_fixup_check() warn: passing zero to 'ERR_PTR'
> drivers/net/phy/sfp.c:1827 sfp_sm_mod_probe() warn: passing zero to 'ERR_PTR'
> drivers/net/phy/sfp.c:1854 sfp_sm_mod_probe() warn: passing zero to 'ERR_PTR'
> drivers/net/phy/sfp.c:1903 sfp_sm_mod_probe() warn: passing zero to 'ERR_PTR'
> 
> drivers/net/phy/sfp.c
>     1767 static int sfp_cotsworks_fixup_check(struct sfp *sfp, struct sfp_eeprom_id *id)

It seems this report is itself buggy. Commit 73970055450e does not
contain this function. The correct commit that introduced the underlying
problem was:

b18432c5a49c ("net: phy: sfp: Cotsworks SFF module EEPROM fixup")

But the commit which would have triggered the "passing zero to ERR_PTR"
would have been:

9ae1ef4b1634 ("net: sfp: use %pe for printing errors")

Nevertheless, thanks for spotting the error!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!