| Message ID | 20220124173344.874885-2-sean.anderson@seco.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Heikki, On 1/25/22 4:18 AM, Heikki Krogerus wrote: > On Mon, Jan 24, 2022 at 12:33:44PM -0500, Sean Anderson wrote: >> of_node_put should always be called on device nodes gotten from >> of_get_*. Additionally, it should only be called after there are no >> remaining users. To address the first issue, call of_node_put if later >> steps in ulpi_register fail. To address the latter, call of_node_put >> only after calling device_unregister. > > This looks like a fix, but you don't have the fix tag. You're right this should have Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> Changes in v2: >> - New >> >> drivers/usb/common/ulpi.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c >> index 87deb514eb78..c6ba72544f2b 100644 >> --- a/drivers/usb/common/ulpi.c >> +++ b/drivers/usb/common/ulpi.c >> @@ -301,11 +301,11 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) >> >> ret = ulpi_read_id(ulpi); >> if (ret) >> - return ret; >> + goto err; >> >> ret = device_register(&ulpi->dev); >> if (ret) >> - return ret; >> + goto err; > > I think there is another bug in the code here. Missing put_device(). So what is the correct way to create a device? Shouldn't device_register be paired with device_unregister? And from what I can tell, device_unregister does not put the of_node. > If you first fix that, you should then be able to call > fwnode_handle_put() (instead of of_node_put()) Well, we currently only have a ulpi_of_register, so I don't think we will have a fwnode here. But I can use that if you wish. --Sean > from > ulpi_dev_release(), and that should cover all cases. > >> root = debugfs_create_dir(dev_name(dev), ULPI_ROOT); >> debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops); >> @@ -314,6 +314,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) >> ulpi->id.vendor, ulpi->id.product); >> >> return 0; >> + >> +err: >> + of_node_put(ulpi->dev.of_node); >> + return ret; > > So no need for that. > >> } >> >> /** >> @@ -357,8 +361,8 @@ void ulpi_unregister_interface(struct ulpi *ulpi) >> { >> debugfs_remove_recursive(debugfs_lookup(dev_name(&ulpi->dev), >> ULPI_ROOT)); >> - of_node_put(ulpi->dev.of_node); >> device_unregister(&ulpi->dev); >> + of_node_put(ulpi->dev.of_node); >> } > > And here you can just remove that of_node_put() call. > > thanks, >
On 1/25/22 11:53 AM, Sean Anderson wrote: > Hi Heikki, > > On 1/25/22 4:18 AM, Heikki Krogerus wrote: >> On Mon, Jan 24, 2022 at 12:33:44PM -0500, Sean Anderson wrote: >>> of_node_put should always be called on device nodes gotten from >>> of_get_*. Additionally, it should only be called after there are no >>> remaining users. To address the first issue, call of_node_put if later >>> steps in ulpi_register fail. To address the latter, call of_node_put >>> only after calling device_unregister. >> >> This looks like a fix, but you don't have the fix tag. > > You're right this should have > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > >>> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >>> --- >>> >>> Changes in v2: >>> - New >>> >>> drivers/usb/common/ulpi.c | 10 +++++++--- >>> 1 file changed, 7 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c >>> index 87deb514eb78..c6ba72544f2b 100644 >>> --- a/drivers/usb/common/ulpi.c >>> +++ b/drivers/usb/common/ulpi.c >>> @@ -301,11 +301,11 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) >>> >>> ret = ulpi_read_id(ulpi); >>> if (ret) >>> - return ret; >>> + goto err; >>> >>> ret = device_register(&ulpi->dev); >>> if (ret) >>> - return ret; >>> + goto err; >> >> I think there is another bug in the code here. Missing put_device(). > > So what is the correct way to create a device? Shouldn't device_register > be paired with device_unregister? And from what I can tell, > device_unregister does not put the of_node. > >> If you first fix that, you should then be able to call >> fwnode_handle_put() (instead of of_node_put()) > > Well, we currently only have a ulpi_of_register, so I don't think we > will have a fwnode here. But I can use that if you wish. Hm, looks like I missed the ACPI_COMPANION_SET So this should probably be fwnode_handle_put. But ACPI_COMPANION_SET doesn't seem to get the fwnode? > --Sean > >> from >> ulpi_dev_release(), and that should cover all cases. >> >>> root = debugfs_create_dir(dev_name(dev), ULPI_ROOT); >>> debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops); >>> @@ -314,6 +314,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) >>> ulpi->id.vendor, ulpi->id.product); >>> >>> return 0; >>> + >>> +err: >>> + of_node_put(ulpi->dev.of_node); >>> + return ret; >> >> So no need for that. >> >>> } >>> >>> /** >>> @@ -357,8 +361,8 @@ void ulpi_unregister_interface(struct ulpi *ulpi) >>> { >>> debugfs_remove_recursive(debugfs_lookup(dev_name(&ulpi->dev), >>> ULPI_ROOT)); >>> - of_node_put(ulpi->dev.of_node); >>> device_unregister(&ulpi->dev); >>> + of_node_put(ulpi->dev.of_node); >>> } >> >> And here you can just remove that of_node_put() call. >> >> thanks, >> >
On Tue, Jan 25, 2022 at 11:53:58AM -0500, Sean Anderson wrote: > Hi Heikki, > > On 1/25/22 4:18 AM, Heikki Krogerus wrote: > > On Mon, Jan 24, 2022 at 12:33:44PM -0500, Sean Anderson wrote: > >> of_node_put should always be called on device nodes gotten from > >> of_get_*. Additionally, it should only be called after there are no > >> remaining users. To address the first issue, call of_node_put if later > >> steps in ulpi_register fail. To address the latter, call of_node_put > >> only after calling device_unregister. > > > > This looks like a fix, but you don't have the fix tag. > > You're right this should have > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") Please resend this as 2 independent patches as they should not depend on each other, allowing this one to be backported, but the debugfs addition added only to 5.18-rc1. thanks, greg k-h
Hi Sean, On Tue, Jan 25, 2022 at 11:53:58AM -0500, Sean Anderson wrote: > Hi Heikki, > > On 1/25/22 4:18 AM, Heikki Krogerus wrote: > > On Mon, Jan 24, 2022 at 12:33:44PM -0500, Sean Anderson wrote: > >> of_node_put should always be called on device nodes gotten from > >> of_get_*. Additionally, it should only be called after there are no > >> remaining users. To address the first issue, call of_node_put if later > >> steps in ulpi_register fail. To address the latter, call of_node_put > >> only after calling device_unregister. > > > > This looks like a fix, but you don't have the fix tag. > > You're right this should have > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > > >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> > >> --- > >> > >> Changes in v2: > >> - New > >> > >> drivers/usb/common/ulpi.c | 10 +++++++--- > >> 1 file changed, 7 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c > >> index 87deb514eb78..c6ba72544f2b 100644 > >> --- a/drivers/usb/common/ulpi.c > >> +++ b/drivers/usb/common/ulpi.c > >> @@ -301,11 +301,11 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) > >> > >> ret = ulpi_read_id(ulpi); > >> if (ret) > >> - return ret; > >> + goto err; > >> > >> ret = device_register(&ulpi->dev); > >> if (ret) > >> - return ret; > >> + goto err; > > > > I think there is another bug in the code here. Missing put_device(). > > So what is the correct way to create a device? Shouldn't device_register > be paired with device_unregister? And from what I can tell, > device_unregister does not put the of_node. I think this is best explained in the documentation. Check the "Rule of thumb is" section (device_register() is really just a wrapper that can only fail if device_add() fails): https://docs.kernel.org/driver-api/infrastructure.html?highlight=device_add#c.device_add So you just want to drop the reference if device_register() fails. That will make sure ulpi_dev_release() is called also in this case. > > If you first fix that, you should then be able to call > > fwnode_handle_put() (instead of of_node_put()) > > Well, we currently only have a ulpi_of_register, so I don't think we > will have a fwnode here. But I can use that if you wish. > > --Sean > > > from > > ulpi_dev_release(), and that should cover all cases. > > > >> root = debugfs_create_dir(dev_name(dev), ULPI_ROOT); > >> debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops); > >> @@ -314,6 +314,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) > >> ulpi->id.vendor, ulpi->id.product); > >> > >> return 0; > >> + > >> +err: > >> + of_node_put(ulpi->dev.of_node); > >> + return ret; > > > > So no need for that. > > > >> } > >> > >> /** > >> @@ -357,8 +361,8 @@ void ulpi_unregister_interface(struct ulpi *ulpi) > >> { > >> debugfs_remove_recursive(debugfs_lookup(dev_name(&ulpi->dev), > >> ULPI_ROOT)); > >> - of_node_put(ulpi->dev.of_node); > >> device_unregister(&ulpi->dev); > >> + of_node_put(ulpi->dev.of_node); > >> } > > > > And here you can just remove that of_node_put() call. Note. Just by calling device_unregister() does not guarantee that there are no more users left. We can be certain that the last user is gone only when ulpi_dev_release() is called, so that's the place where you want to release the fwnode (of_node). thanks,
On 1/26/22 8:04 AM, Greg Kroah-Hartman wrote: > On Tue, Jan 25, 2022 at 11:53:58AM -0500, Sean Anderson wrote: >> Hi Heikki, >> >> On 1/25/22 4:18 AM, Heikki Krogerus wrote: >> > On Mon, Jan 24, 2022 at 12:33:44PM -0500, Sean Anderson wrote: >> >> of_node_put should always be called on device nodes gotten from >> >> of_get_*. Additionally, it should only be called after there are no >> >> remaining users. To address the first issue, call of_node_put if later >> >> steps in ulpi_register fail. To address the latter, call of_node_put >> >> only after calling device_unregister. >> > >> > This looks like a fix, but you don't have the fix tag. >> >> You're right this should have >> >> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > > Please resend this as 2 independent patches as they should not depend on > each other, allowing this one to be backported, but the debugfs addition > added only to 5.18-rc1. The patches modify adjacent lines, so one will depend on the other. I will order this patch first the next time. --Sean
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c index 87deb514eb78..c6ba72544f2b 100644 --- a/drivers/usb/common/ulpi.c +++ b/drivers/usb/common/ulpi.c @@ -301,11 +301,11 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) ret = ulpi_read_id(ulpi); if (ret) - return ret; + goto err; ret = device_register(&ulpi->dev); if (ret) - return ret; + goto err; root = debugfs_create_dir(dev_name(dev), ULPI_ROOT); debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops); @@ -314,6 +314,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi) ulpi->id.vendor, ulpi->id.product); return 0; + +err: + of_node_put(ulpi->dev.of_node); + return ret; } /** @@ -357,8 +361,8 @@ void ulpi_unregister_interface(struct ulpi *ulpi) { debugfs_remove_recursive(debugfs_lookup(dev_name(&ulpi->dev), ULPI_ROOT)); - of_node_put(ulpi->dev.of_node); device_unregister(&ulpi->dev); + of_node_put(ulpi->dev.of_node); } EXPORT_SYMBOL_GPL(ulpi_unregister_interface);
of_node_put should always be called on device nodes gotten from of_get_*. Additionally, it should only be called after there are no remaining users. To address the first issue, call of_node_put if later steps in ulpi_register fail. To address the latter, call of_node_put only after calling device_unregister. Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- Changes in v2: - New drivers/usb/common/ulpi.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)