| Message ID | 20210606200911.32076-1-andy.shevchenko@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/3] usb: typec: intel_pmc_mux: Put fwnode in error case during ->probe() | expand |
On Sun, Jun 06, 2021 at 11:09:09PM +0300, Andy Shevchenko wrote: > device_get_next_child_node() bumps a reference counting of a returned variable. > We have to balance it whenever we return to the caller. > > Fixes: 6701adfa9693 ("usb: typec: driver for Intel PMC mux control") > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > drivers/usb/typec/mux/intel_pmc_mux.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c > index 46a25b8db72e..134325444e6a 100644 > --- a/drivers/usb/typec/mux/intel_pmc_mux.c > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c > @@ -645,6 +645,7 @@ static int pmc_usb_probe(struct platform_device *pdev) > return 0; > > err_remove_ports: > + fwnode_handle_put(fwnode); Wouldn't it be more clear to do that in the condition that jumps to this lable? > for (i = 0; i < pmc->num_ports; i++) { > typec_switch_unregister(pmc->port[i].typec_sw); > typec_mux_unregister(pmc->port[i].typec_mux); > -- > 2.31.1 -- heikki
On Sun, Jun 06, 2021 at 11:09:11PM +0300, Andy Shevchenko wrote: > For ACPI devices we have a symmetric API to put them, so use it in the driver. > > Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/mux/intel_pmc_mux.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c > index de40276cc18b..2d0a863956c7 100644 > --- a/drivers/usb/typec/mux/intel_pmc_mux.c > +++ b/drivers/usb/typec/mux/intel_pmc_mux.c > @@ -582,12 +582,12 @@ static int pmc_usb_probe_iom(struct pmc_usb *pmc) > acpi_dev_free_resource_list(&resource_list); > > if (!pmc->iom_base) { > - put_device(&adev->dev); > + acpi_dev_put(adev); > return -ENOMEM; > } > > if (IS_ERR(pmc->iom_base)) { > - put_device(&adev->dev); > + acpi_dev_put(adev); > return PTR_ERR(pmc->iom_base); > } > > @@ -657,7 +657,7 @@ static int pmc_usb_probe(struct platform_device *pdev) > usb_role_switch_unregister(pmc->port[i].usb_sw); > } > > - put_device(&pmc->iom_adev->dev); > + acpi_dev_put(pmc->iom_adev); > > return ret; > } > @@ -673,7 +673,7 @@ static int pmc_usb_remove(struct platform_device *pdev) > usb_role_switch_unregister(pmc->port[i].usb_sw); > } > > - put_device(&pmc->iom_adev->dev); > + acpi_dev_put(pmc->iom_adev); > > return 0; > } > -- > 2.31.1 -- heikki
On Mon, Jun 7, 2021 at 12:23 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > On Sun, Jun 06, 2021 at 11:09:09PM +0300, Andy Shevchenko wrote: > > device_get_next_child_node() bumps a reference counting of a returned variable. ... > > err_remove_ports: > > + fwnode_handle_put(fwnode); > > Wouldn't it be more clear to do that in the condition that jumps to > this lable? In this case it doesn't matter. As a general pattern, no, because this will help to keep this in mind in complex error handling ladders. That said, I prefer my variant unless there is a strong opinion to move it into the conditional. > > for (i = 0; i < pmc->num_ports; i++) { > > typec_switch_unregister(pmc->port[i].typec_sw); > > typec_mux_unregister(pmc->port[i].typec_mux); -- With Best Regards, Andy Shevchenko
On Mon, Jun 07, 2021 at 12:29:53PM +0300, Andy Shevchenko wrote: > On Mon, Jun 7, 2021 at 12:23 PM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > On Sun, Jun 06, 2021 at 11:09:09PM +0300, Andy Shevchenko wrote: > > > device_get_next_child_node() bumps a reference counting of a returned variable. > > ... > > > > err_remove_ports: > > > + fwnode_handle_put(fwnode); > > > > Wouldn't it be more clear to do that in the condition that jumps to > > this lable? > > In this case it doesn't matter. As a general pattern, no, because this > will help to keep this in mind in complex error handling ladders. That > said, I prefer my variant unless there is a strong opinion to move it > into the conditional. Now it looks like you are releasing the mux device fwnode instead of a port fwnode because everything else related to the ports is destroyed in below loop. That's too confusing. Just handle it inside the condition, and the whole thing becomes clear. > > > for (i = 0; i < pmc->num_ports; i++) { > > > typec_switch_unregister(pmc->port[i].typec_sw); > > > typec_mux_unregister(pmc->port[i].typec_mux); -- heikki
On Mon, Jun 7, 2021 at 12:49 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > On Mon, Jun 07, 2021 at 12:29:53PM +0300, Andy Shevchenko wrote: > > On Mon, Jun 7, 2021 at 12:23 PM Heikki Krogerus > > <heikki.krogerus@linux.intel.com> wrote: > > > On Sun, Jun 06, 2021 at 11:09:09PM +0300, Andy Shevchenko wrote: > > > > device_get_next_child_node() bumps a reference counting of a returned variable. > > > > ... > > > > > > err_remove_ports: > > > > + fwnode_handle_put(fwnode); > > > > > > Wouldn't it be more clear to do that in the condition that jumps to > > > this lable? > > > > In this case it doesn't matter. As a general pattern, no, because this > > will help to keep this in mind in complex error handling ladders. That > > said, I prefer my variant unless there is a strong opinion to move it > > into the conditional. > > Now it looks like you are releasing the mux device fwnode instead of a > port fwnode because everything else related to the ports is destroyed > in below loop. That's too confusing. > > Just handle it inside the condition, and the whole thing becomes > clear. I see your point, okay, I will update in v2. Thanks for your review! > > > > for (i = 0; i < pmc->num_ports; i++) { > > > > typec_switch_unregister(pmc->port[i].typec_sw); > > > > typec_mux_unregister(pmc->port[i].typec_mux); -- With Best Regards, Andy Shevchenko
diff --git a/drivers/usb/typec/mux/intel_pmc_mux.c b/drivers/usb/typec/mux/intel_pmc_mux.c index 46a25b8db72e..134325444e6a 100644 --- a/drivers/usb/typec/mux/intel_pmc_mux.c +++ b/drivers/usb/typec/mux/intel_pmc_mux.c @@ -645,6 +645,7 @@ static int pmc_usb_probe(struct platform_device *pdev) return 0; err_remove_ports: + fwnode_handle_put(fwnode); for (i = 0; i < pmc->num_ports; i++) { typec_switch_unregister(pmc->port[i].typec_sw); typec_mux_unregister(pmc->port[i].typec_mux);
device_get_next_child_node() bumps a reference counting of a returned variable. We have to balance it whenever we return to the caller. Fixes: 6701adfa9693 ("usb: typec: driver for Intel PMC mux control") Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Signed-off-by: Andy Shevchenko <andy.shevchenko@gmail.com> --- drivers/usb/typec/mux/intel_pmc_mux.c | 1 + 1 file changed, 1 insertion(+)