[bug report] phy: usb: Add USB2.0 phy driver for Sunplus SP7021

[Dan Carpenter <dan.carpenter@linaro.org>]

Hello Vincent Shih,

Commit 99d9ccd97385 ("phy: usb: Add USB2.0 phy driver for Sunplus
SP7021") from Jul 25, 2022 (linux-next), leads to the following
Smatch static checker warning:

	drivers/phy/sunplus/phy-sunplus-usb2.c:92 update_disc_vol()
	error: 'cell' dereferencing possible ERR_PTR()

drivers/phy/sunplus/phy-sunplus-usb2.c
    78 static int update_disc_vol(struct sp_usbphy *usbphy)
    79 {
    80         struct nvmem_cell *cell;
    81         char *disc_name = "disc_vol";
    82         ssize_t otp_l = 0;
    83         char *otp_v;
    84         u32 val, set;
    85 
    86         cell = nvmem_cell_get(usbphy->dev, disc_name);
    87         if (IS_ERR_OR_NULL(cell)) {
    88                 if (PTR_ERR(cell) == -EPROBE_DEFER)
    89                         return -EPROBE_DEFER;

I don't love the if if (IS_ERR_OR_NULL(cell)) check.  The
nvmem_cell_get() function can't actually return NULL.  The normal thing
would be to make it return NULL if CONFIG_NVMEM wasn't set.  Even then
we would still only check for:

	if (IS_ERR(cell))
		return PTR_ERR(cell);

I have written a longer explanation here:
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

Can PHY_SUNPLUS_USB really work without CONFIG_NVMEM?  If not then we
could just add that as a depend in the Kconfig.  The other way to fix
this would be:

	if (IS_ERR(cell) && PTR_ERR(cell) != -EOPNOTSUPP)
		return PTR_ERR(cell);

    90         }
    91 
--> 92         otp_v = nvmem_cell_read(cell, &otp_l);
                                       ^^^^
This will crash if the nvmem_cell_get() has an error.

    93         nvmem_cell_put(cell);
    94 
    95         if (!IS_ERR(otp_v)) {
    96                 set = *(otp_v + 1);
    97                 set = (set << (sizeof(char) * 8)) | *otp_v;
    98                 set = (set >> usbphy->disc_vol_addr_off) & J_DISC;
    99         }
    100 
    101         if (IS_ERR(otp_v) || set == 0)
    102                 set = OTP_DISC_LEVEL_DEFAULT;
    103 
    104         val = readl(usbphy->phy_regs + CONFIG7);
    105         val = (val & ~J_DISC) | set;
    106         writel(val, usbphy->phy_regs + CONFIG7);
    107 
    108         return 0;
    109 }

regards,
dan carpenter

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy