RE: [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR'

[Biju Das <biju.das@xxxxxxxxxxxxxx>]

Hu Dan Carpenter,

Thanks for the feedback.

> drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to
> 'PTR_ERR'
> 
> On Mon, Oct 07, 2019 at 12:36:10PM +0000, Biju Das wrote:
> > > err_unreg_port:
> > > 	typec_unregister_port(hd3ss3220->port);
> > > err_put_role:
> > > 	usb_role_switch_put(hd3ss3220->role_sw);
> > > err_put_handle:
> > > 	fwnode_handle_put(foo bar);
> > >
> > > 	return ret;
> > > The rule behind this style of error handling is that you just have
> > > to keep track of the most recently allocated resource and at the
> > > bottom you free them in the reverse order from how you allocated
> > > them.  Here we had allocated -
> > > >role_sw but the typec_register_port() so we do goto free_role_sw;
> > > >Now
> > > people can guess what the goto does because the name is descriptive
> > > and since it matches the most recently allocated resource that means it's
> okay.
> >
> > Yes I agree. But In this case, only one error label is sufficient.
> 
> Yes.  You could fix the leak by passing an invalid pointer to
> typec_unregister_port() but that way is asking for trouble in the future...
> These are the kinds of bugs I fix all the time because I'm working with static
> analysis.  Clearly defined error labels are more readable and less bug prone.

OK.  Are you ok with the below changes?

@@ -178,7 +178,7 @@ static int hd3ss3220_probe(struct i2c_client *client,
 
        hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);
        fwnode_handle_put(connector);
-       if (IS_ERR_OR_NULL(hd3ss3220->role_sw))
+       if (IS_ERR(hd3ss3220->role_sw))
                return PTR_ERR(hd3ss3220->role_sw);
 
        hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
@@ -188,20 +188,22 @@ static int hd3ss3220_probe(struct i2c_client *client,
 
        hd3ss3220->port = typec_register_port(&client->dev,
                                              &hd3ss3220->typec_cap);
-       if (IS_ERR(hd3ss3220->port))
+       if (IS_ERR(hd3ss3220->port)) {
+               usb_role_switch_put(hd3ss3220->role_sw);
                return PTR_ERR(hd3ss3220->port);
+       }
 
        hd3ss3220_set_role(hd3ss3220);
        ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL, &data);
        if (ret < 0)
-               goto error;
+               goto err_unreg_port;
 
        if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
                ret = regmap_write(hd3ss3220->regmap,
                                HD3SS3220_REG_CN_STAT_CTRL,
                                data | HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
                if (ret < 0)
-                       goto error;
+                       goto err_unreg_port;
        }
 
        if (client->irq > 0) {
@@ -210,7 +212,7 @@ static int hd3ss3220_probe(struct i2c_client *client,
                                        IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
                                        "hd3ss3220", &client->dev);
                if (ret)
-                       goto error;
+                       goto err_unreg_port;
        }
 
        ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
@@ -220,8 +222,9 @@ static int hd3ss3220_probe(struct i2c_client *client,
        dev_info(&client->dev, "probed revision=0x%x\n", ret);
 
        return 0;
-error:
+err_unreg_port:
        typec_unregister_port(hd3ss3220->port);
+err_put_role:
        usb_role_switch_put(hd3ss3220->role_sw);
 
        return ret;

regards,
Biju