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@xxxxxxxxx> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > 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
