From: Biju Das <biju.das@bp.renesas.com>
To: Dan Carpenter <dan.carpenter@oracle.com>,
Heikki Krogerus <heikki.krogerus@linux.intel.com>,
"kbuild@01.org" <kbuild@01.org>
Cc: "kbuild-all@01.org" <kbuild-all@01.org>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: RE: [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR'
Date: Mon, 7 Oct 2019 12:36:10 +0000 [thread overview]
Message-ID: <OSBPR01MB2103ECC1BA7A7A0D9ABA15CBB89B0@OSBPR01MB2103.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20191007112939.GG21515@kadam>
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
next prev parent reply other threads:[~2019-10-07 12:36 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-07 11:29 [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR' Dan Carpenter
2019-10-07 12:36 ` Biju Das [this message]
2019-10-07 13:17 ` Dan Carpenter
2019-10-07 14:19 ` Biju Das
2019-10-07 14:33 ` Dan Carpenter
2019-10-07 14:51 ` Biju Das
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=OSBPR01MB2103ECC1BA7A7A0D9ABA15CBB89B0@OSBPR01MB2103.jpnprd01.prod.outlook.com \
--to=biju.das@bp.renesas.com \
--cc=dan.carpenter@oracle.com \
--cc=gregkh@linuxfoundation.org \
--cc=heikki.krogerus@linux.intel.com \
--cc=kbuild-all@01.org \
--cc=kbuild@01.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).