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@bp.renesas.com>]

Hello Dan,

Thanks for the feedback.

> -----Original Message-----
> Subject: [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182
> hd3ss3220_probe() warn: passing zero to 'PTR_ERR'
> 
> [ Resending with Fixed email headers - dan ]
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-
> next
> head:   dd3fd317e2beb899cbffcf364de049b9f9a02db5
> commit: 1c48c759ef4bb9031b3347277f04484e07e27d97 [32/38] usb: typec:
> driver for TI HD3SS3220 USB Type-C DRP port controller
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to
> 'PTR_ERR'
> 
> #
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?id=
> 1c48c759ef4bb9031b3347277f04484e07e27d97
> git remote add usb
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> git remote update usb
> git checkout 1c48c759ef4bb9031b3347277f04484e07e27d97
> vim +/PTR_ERR +182 drivers/usb/typec/hd3ss3220.c
> 
> 1c48c759ef4bb9 Biju Das 2019-09-04  152

> 1c48c759ef4bb9 Biju Das 2019-09-04  178
> 1c48c759ef4bb9 Biju Das 2019-09-04  179  	hd3ss3220->role_sw =
> fwnode_usb_role_switch_get(connector);
> 1c48c759ef4bb9 Biju Das 2019-09-04  180
> 	fwnode_handle_put(connector);
> 1c48c759ef4bb9 Biju Das 2019-09-04  181  	if
> (IS_ERR_OR_NULL(hd3ss3220->role_sw))
> 1c48c759ef4bb9 Biju Das 2019-09-04 @182  		return
> PTR_ERR(hd3ss3220->role_sw);
> 
> When fwnode_usb_role_switch_get() returns NULL then we return success
> here.  It seems like a bug.

OK.  I will change the check condition from (IS_ERR_OR_NULL(hd3ss3220->role_sw))
to IS_ERR(hd3ss3220->role_sw)). 

> When function returns a mix of NULL and error pointers then NULL is a
> special case of success.  For example, a module tries to request a feature but
> that feature is deliberately turned off.  It's not an error so we can't return an
> error pointer, but at same time we can't return a valid pointer because the
> feature is disabled.
> 
> For fwnode_usb_role_switch_get() it is a bit unclear to me what NULL means
> in that context, and there are no comments to explain it...  The
> fwnode_connection_find_match() is the same way where it mixes NULL and
> error pointers, doesn't have a comment, and it really seems like NULL is an
> error, not a special case of success like it's supposed to be.  I have added
> Heikki Krogerus to the CC list.
> 
> 1c48c759ef4bb9 Biju Das 2019-09-04  183
> 1c48c759ef4bb9 Biju Das 2019-09-04  184  	hd3ss3220-
> >typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> 1c48c759ef4bb9 Biju Das 2019-09-04  185  	hd3ss3220-
> >typec_cap.dr_set = hd3ss3220_dr_set;
> 1c48c759ef4bb9 Biju Das 2019-09-04  186  	hd3ss3220->typec_cap.type =
> TYPEC_PORT_DRP;
> 1c48c759ef4bb9 Biju Das 2019-09-04  187  	hd3ss3220->typec_cap.data =
> TYPEC_PORT_DRD;
> 1c48c759ef4bb9 Biju Das 2019-09-04  188
> 1c48c759ef4bb9 Biju Das 2019-09-04  189  	hd3ss3220->port =
> typec_register_port(&client->dev,
> 1c48c759ef4bb9 Biju Das 2019-09-04  190
> 	      &hd3ss3220->typec_cap);
> 1c48c759ef4bb9 Biju Das 2019-09-04  191  	if (IS_ERR(hd3ss3220->port))
> 1c48c759ef4bb9 Biju Das 2019-09-04  192  		return
> PTR_ERR(hd3ss3220->port);
> 
> This error path should call usb_role_switch_put(hd3ss3220->role_sw) and
> probably a fwnode_handle_put() as well?

I agree, We need to have  usb_role_switch_put(hd3ss3220->role_sw) on the error path.
But not the  "fwnode_handle_put. "

Basically it used to find the role switch associated with connector device.
The usage of "connector "  is done with below code and "typec_register_port"
comes after that.

175         connector = device_get_named_child_node(hd3ss3220->dev, "connector");    
176         if (IS_ERR(connector))                                                   
177                 return PTR_ERR(connector);                                       
178                                                                                  
179         hd3ss3220->role_sw = fwnode_usb_role_switch_get(connector);              
180         fwnode_handle_put(connector);  
.......
........
........
hd3ss3220->port = typec_register_port(&client->dev,                      
190                                               &hd3ss3220->typec_cap);


> I noticed this because I scrolled to the bottom of the fucntion and noticed
> that there was only one error label.  When you see a single error label like
> that, it's normally a red flag that the error handling is buggy.  Error labels
> should have descriptive names which say what they do.  I suggest something
> like:

> 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.

Regards,
Biju