Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR'
@ 2019-10-07 11:29 Dan Carpenter
  2019-10-07 12:36 ` Biju Das
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-10-07 11:29 UTC (permalink / raw)
  To: Heikki Krogerus, kbuild, Biju Das
  Cc: kbuild-all, linux-usb, Greg Kroah-Hartman

[ 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  153  static int hd3ss3220_probe(struct i2c_client *client,
1c48c759ef4bb9 Biju Das 2019-09-04  154  		const struct i2c_device_id *id)
1c48c759ef4bb9 Biju Das 2019-09-04  155  {
1c48c759ef4bb9 Biju Das 2019-09-04  156  	struct hd3ss3220 *hd3ss3220;
1c48c759ef4bb9 Biju Das 2019-09-04  157  	struct fwnode_handle *connector;
1c48c759ef4bb9 Biju Das 2019-09-04  158  	int ret;
1c48c759ef4bb9 Biju Das 2019-09-04  159  	unsigned int data;
1c48c759ef4bb9 Biju Das 2019-09-04  160  
1c48c759ef4bb9 Biju Das 2019-09-04  161  	hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
1c48c759ef4bb9 Biju Das 2019-09-04  162  				 GFP_KERNEL);
1c48c759ef4bb9 Biju Das 2019-09-04  163  	if (!hd3ss3220)
1c48c759ef4bb9 Biju Das 2019-09-04  164  		return -ENOMEM;
1c48c759ef4bb9 Biju Das 2019-09-04  165  
1c48c759ef4bb9 Biju Das 2019-09-04  166  	i2c_set_clientdata(client, hd3ss3220);
1c48c759ef4bb9 Biju Das 2019-09-04  167  
1c48c759ef4bb9 Biju Das 2019-09-04  168  	hd3ss3220->dev = &client->dev;
1c48c759ef4bb9 Biju Das 2019-09-04  169  	hd3ss3220->regmap = devm_regmap_init_i2c(client, &config);
1c48c759ef4bb9 Biju Das 2019-09-04  170  	if (IS_ERR(hd3ss3220->regmap))
1c48c759ef4bb9 Biju Das 2019-09-04  171  		return PTR_ERR(hd3ss3220->regmap);
1c48c759ef4bb9 Biju Das 2019-09-04  172  
1c48c759ef4bb9 Biju Das 2019-09-04  173  	hd3ss3220_set_source_pref(hd3ss3220,
1c48c759ef4bb9 Biju Das 2019-09-04  174  				  HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
1c48c759ef4bb9 Biju Das 2019-09-04  175  	connector = device_get_named_child_node(hd3ss3220->dev, "connector");
1c48c759ef4bb9 Biju Das 2019-09-04  176  	if (IS_ERR(connector))
1c48c759ef4bb9 Biju Das 2019-09-04  177  		return PTR_ERR(connector);
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.

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

1c48c759ef4bb9 Biju Das 2019-09-04  193  
1c48c759ef4bb9 Biju Das 2019-09-04  194  	hd3ss3220_set_role(hd3ss3220);
1c48c759ef4bb9 Biju Das 2019-09-04  195  	ret = regmap_read(hd3ss3220->regmap, HD3SS3220_REG_CN_STAT_CTRL, &data);
1c48c759ef4bb9 Biju Das 2019-09-04  196  	if (ret < 0)
1c48c759ef4bb9 Biju Das 2019-09-04  197  		goto error;
1c48c759ef4bb9 Biju Das 2019-09-04  198  
1c48c759ef4bb9 Biju Das 2019-09-04  199  	if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
1c48c759ef4bb9 Biju Das 2019-09-04  200  		ret = regmap_write(hd3ss3220->regmap,
1c48c759ef4bb9 Biju Das 2019-09-04  201  				HD3SS3220_REG_CN_STAT_CTRL,
1c48c759ef4bb9 Biju Das 2019-09-04  202  				data | HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
1c48c759ef4bb9 Biju Das 2019-09-04  203  		if (ret < 0)
1c48c759ef4bb9 Biju Das 2019-09-04  204  			goto error;
1c48c759ef4bb9 Biju Das 2019-09-04  205  	}
1c48c759ef4bb9 Biju Das 2019-09-04  206  
1c48c759ef4bb9 Biju Das 2019-09-04  207  	if (client->irq > 0) {
1c48c759ef4bb9 Biju Das 2019-09-04  208  		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
1c48c759ef4bb9 Biju Das 2019-09-04  209  					hd3ss3220_irq_handler,
1c48c759ef4bb9 Biju Das 2019-09-04  210  					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
1c48c759ef4bb9 Biju Das 2019-09-04  211  					"hd3ss3220", &client->dev);
1c48c759ef4bb9 Biju Das 2019-09-04  212  		if (ret)
1c48c759ef4bb9 Biju Das 2019-09-04  213  			goto error;
1c48c759ef4bb9 Biju Das 2019-09-04  214  	}
1c48c759ef4bb9 Biju Das 2019-09-04  215  
1c48c759ef4bb9 Biju Das 2019-09-04  216  	ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
1c48c759ef4bb9 Biju Das 2019-09-04  217  	if (ret < 0)
1c48c759ef4bb9 Biju Das 2019-09-04  218  		goto error;
1c48c759ef4bb9 Biju Das 2019-09-04  219  
1c48c759ef4bb9 Biju Das 2019-09-04  220  	dev_info(&client->dev, "probed revision=0x%x\n", ret);
1c48c759ef4bb9 Biju Das 2019-09-04  221  
1c48c759ef4bb9 Biju Das 2019-09-04  222  	return 0;
1c48c759ef4bb9 Biju Das 2019-09-04  223  error:
1c48c759ef4bb9 Biju Das 2019-09-04  224  	typec_unregister_port(hd3ss3220->port);
1c48c759ef4bb9 Biju Das 2019-09-04  225  	usb_role_switch_put(hd3ss3220->role_sw);
1c48c759ef4bb9 Biju Das 2019-09-04  226  
1c48c759ef4bb9 Biju Das 2019-09-04  227  	return ret;
1c48c759ef4bb9 Biju Das 2019-09-04  228  }

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR'
  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
  2019-10-07 13:17   ` Dan Carpenter
  0 siblings, 1 reply; 6+ messages in thread
From: Biju Das @ 2019-10-07 12:36 UTC (permalink / raw)
  To: Dan Carpenter, Heikki Krogerus, kbuild
  Cc: kbuild-all, linux-usb, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR'
  2019-10-07 12:36 ` Biju Das
@ 2019-10-07 13:17   ` Dan Carpenter
  2019-10-07 14:19     ` Biju Das
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-10-07 13:17 UTC (permalink / raw)
  To: Biju Das
  Cc: Heikki Krogerus, kbuild, kbuild-all, linux-usb, Greg Kroah-Hartman

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.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR'
  2019-10-07 13:17   ` Dan Carpenter
@ 2019-10-07 14:19     ` Biju Das
  2019-10-07 14:33       ` Dan Carpenter
  0 siblings, 1 reply; 6+ messages in thread
From: Biju Das @ 2019-10-07 14:19 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Heikki Krogerus, kbuild, kbuild-all, linux-usb, Greg Kroah-Hartman

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR'
  2019-10-07 14:19     ` Biju Das
@ 2019-10-07 14:33       ` Dan Carpenter
  2019-10-07 14:51         ` Biju Das
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-10-07 14:33 UTC (permalink / raw)
  To: Biju Das
  Cc: Heikki Krogerus, kbuild, kbuild-all, linux-usb, Greg Kroah-Hartman

On Mon, Oct 07, 2019 at 02:19:05PM +0000, Biju Das wrote:
> OK.  Are you ok with the below changes?
> 

It will generate a compile warning so no...  :P

> @@ -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);

		ret = PTR_ERR(hd3ss3220->port);
		goto err_put_role

> +       }

Otherwise I think it's the right thing.  Thanks!

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [kbuild] [usb:usb-next 32/38] drivers/usb/typec/hd3ss3220.c:182 hd3ss3220_probe() warn: passing zero to 'PTR_ERR'
  2019-10-07 14:33       ` Dan Carpenter
@ 2019-10-07 14:51         ` Biju Das
  0 siblings, 0 replies; 6+ messages in thread
From: Biju Das @ 2019-10-07 14:51 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Heikki Krogerus, kbuild, kbuild-all, linux-usb, Greg Kroah-Hartman

Hi 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 02:19:05PM +0000, Biju Das wrote:
> > OK.  Are you ok with the below changes?
> >
> 
> It will generate a compile warning so no...  :P
> 
> > @@ -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);
> 
> 		ret = PTR_ERR(hd3ss3220->port);
> 		goto err_put_role

Got it. Will send a patch to  fix this.

Regards,
Biju

> > +       }
> 
> Otherwise I think it's the right thing.  Thanks!
> 
> regards,
> dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git