This adds a debugfs file for ULPI devices which contains a dump of their
registers. This is useful for debugging basic connectivity problems. The
file is created in ulpi_register because many devices will never have a
driver bound (as they are managed in hardware by the USB controller
device).
This also modifies the error handling in ulpi_register a bit to ensure
that ulpi->dev.of_node is always put.
Signed-off-by: Sean Anderson <[email protected]>
---
Changes in v2:
- Always create debugfs files and ignore errors
- Look up dentries dynamically
drivers/usb/common/ulpi.c | 71 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 70 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
index 4169cf40a03b..87deb514eb78 100644
--- a/drivers/usb/common/ulpi.c
+++ b/drivers/usb/common/ulpi.c
@@ -13,6 +13,7 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/acpi.h>
+#include <linux/debugfs.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/clk/clk-conf.h>
@@ -228,9 +229,64 @@ static int ulpi_read_id(struct ulpi *ulpi)
return 0;
}
+static int ulpi_regs_read(struct seq_file *seq, void *data)
+{
+ struct ulpi *ulpi = seq->private;
+
+#define ulpi_print(name, reg) do { \
+ int ret = ulpi_read(ulpi, reg); \
+ if (ret < 0) \
+ return ret; \
+ seq_printf(seq, name " %.02x\n", ret); \
+} while (0)
+
+ ulpi_print("Vendor ID Low ", ULPI_VENDOR_ID_LOW);
+ ulpi_print("Vendor ID High ", ULPI_VENDOR_ID_HIGH);
+ ulpi_print("Product ID Low ", ULPI_PRODUCT_ID_LOW);
+ ulpi_print("Product ID High ", ULPI_PRODUCT_ID_HIGH);
+ ulpi_print("Function Control ", ULPI_FUNC_CTRL);
+ ulpi_print("Interface Control ", ULPI_IFC_CTRL);
+ ulpi_print("OTG Control ", ULPI_OTG_CTRL);
+ ulpi_print("USB Interrupt Enable Rising ", ULPI_USB_INT_EN_RISE);
+ ulpi_print("USB Interrupt Enable Falling", ULPI_USB_INT_EN_FALL);
+ ulpi_print("USB Interrupt Status ", ULPI_USB_INT_STS);
+ ulpi_print("USB Interrupt Latch ", ULPI_USB_INT_LATCH);
+ ulpi_print("Debug ", ULPI_DEBUG);
+ ulpi_print("Scratch Register ", ULPI_SCRATCH);
+ ulpi_print("Carkit Control ", ULPI_CARKIT_CTRL);
+ ulpi_print("Carkit Interrupt Delay ", ULPI_CARKIT_INT_DELAY);
+ ulpi_print("Carkit Interrupt Enable ", ULPI_CARKIT_INT_EN);
+ ulpi_print("Carkit Interrupt Status ", ULPI_CARKIT_INT_STS);
+ ulpi_print("Carkit Interrupt Latch ", ULPI_CARKIT_INT_LATCH);
+ ulpi_print("Carkit Pulse Control ", ULPI_CARKIT_PLS_CTRL);
+ ulpi_print("Transmit Positive Width ", ULPI_TX_POS_WIDTH);
+ ulpi_print("Transmit Negative Width ", ULPI_TX_NEG_WIDTH);
+ ulpi_print("Receive Polarity Recovery ", ULPI_POLARITY_RECOVERY);
+
+ return 0;
+}
+
+static int ulpi_regs_open(struct inode *inode, struct file *f)
+{
+ struct ulpi *ulpi = inode->i_private;
+
+ return single_open(f, ulpi_regs_read, ulpi);
+}
+
+static const struct file_operations ulpi_regs_ops = {
+ .owner = THIS_MODULE,
+ .open = ulpi_regs_open,
+ .release = single_release,
+ .read = seq_read,
+ .llseek = seq_lseek
+};
+
+#define ULPI_ROOT debugfs_lookup(KBUILD_MODNAME, NULL)
+
static int ulpi_register(struct device *dev, struct ulpi *ulpi)
{
int ret;
+ struct dentry *root;
ulpi->dev.parent = dev; /* needed early for ops */
ulpi->dev.bus = &ulpi_bus;
@@ -251,6 +307,9 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
if (ret)
return ret;
+ root = debugfs_create_dir(dev_name(dev), ULPI_ROOT);
+ debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops);
+
dev_dbg(&ulpi->dev, "registered ULPI PHY: vendor %04x, product %04x\n",
ulpi->id.vendor, ulpi->id.product);
@@ -296,6 +355,8 @@ EXPORT_SYMBOL_GPL(ulpi_register_interface);
*/
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);
}
@@ -305,13 +366,21 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_interface);
static int __init ulpi_init(void)
{
- return bus_register(&ulpi_bus);
+ int ret;
+ struct dentry *root;
+
+ root = debugfs_create_dir(KBUILD_MODNAME, NULL);
+ ret = bus_register(&ulpi_bus);
+ if (ret)
+ debugfs_remove(root);
+ return ret;
}
subsys_initcall(ulpi_init);
static void __exit ulpi_exit(void)
{
bus_unregister(&ulpi_bus);
+ debugfs_remove_recursive(ULPI_ROOT);
}
module_exit(ulpi_exit);
--
2.25.1
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 <[email protected]>
---
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;
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);
--
2.25.1
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.
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> 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().
If you first fix that, you should then be able to call
fwnode_handle_put() (instead of of_node_put()) 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,
--
heikki
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 <[email protected]>
>> ---
>>
>> 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 <[email protected]>
>>> ---
>>>
>>> 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
On Mon, Jan 24, 2022 at 12:33:43PM -0500, Sean Anderson wrote:
> This adds a debugfs file for ULPI devices which contains a dump of their
> registers. This is useful for debugging basic connectivity problems. The
> file is created in ulpi_register because many devices will never have a
> driver bound (as they are managed in hardware by the USB controller
> device).
>
> This also modifies the error handling in ulpi_register a bit to ensure
> that ulpi->dev.of_node is always put.
>
> Signed-off-by: Sean Anderson <[email protected]>
> ---
>
> Changes in v2:
> - Always create debugfs files and ignore errors
> - Look up dentries dynamically
>
> drivers/usb/common/ulpi.c | 71 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
> index 4169cf40a03b..87deb514eb78 100644
> --- a/drivers/usb/common/ulpi.c
> +++ b/drivers/usb/common/ulpi.c
> @@ -13,6 +13,7 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/acpi.h>
> +#include <linux/debugfs.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/clk/clk-conf.h>
> @@ -228,9 +229,64 @@ static int ulpi_read_id(struct ulpi *ulpi)
> return 0;
> }
>
> +static int ulpi_regs_read(struct seq_file *seq, void *data)
> +{
> + struct ulpi *ulpi = seq->private;
> +
> +#define ulpi_print(name, reg) do { \
> + int ret = ulpi_read(ulpi, reg); \
> + if (ret < 0) \
> + return ret; \
> + seq_printf(seq, name " %.02x\n", ret); \
> +} while (0)
> +
> + ulpi_print("Vendor ID Low ", ULPI_VENDOR_ID_LOW);
> + ulpi_print("Vendor ID High ", ULPI_VENDOR_ID_HIGH);
> + ulpi_print("Product ID Low ", ULPI_PRODUCT_ID_LOW);
> + ulpi_print("Product ID High ", ULPI_PRODUCT_ID_HIGH);
> + ulpi_print("Function Control ", ULPI_FUNC_CTRL);
> + ulpi_print("Interface Control ", ULPI_IFC_CTRL);
> + ulpi_print("OTG Control ", ULPI_OTG_CTRL);
> + ulpi_print("USB Interrupt Enable Rising ", ULPI_USB_INT_EN_RISE);
> + ulpi_print("USB Interrupt Enable Falling", ULPI_USB_INT_EN_FALL);
> + ulpi_print("USB Interrupt Status ", ULPI_USB_INT_STS);
> + ulpi_print("USB Interrupt Latch ", ULPI_USB_INT_LATCH);
> + ulpi_print("Debug ", ULPI_DEBUG);
> + ulpi_print("Scratch Register ", ULPI_SCRATCH);
> + ulpi_print("Carkit Control ", ULPI_CARKIT_CTRL);
> + ulpi_print("Carkit Interrupt Delay ", ULPI_CARKIT_INT_DELAY);
> + ulpi_print("Carkit Interrupt Enable ", ULPI_CARKIT_INT_EN);
> + ulpi_print("Carkit Interrupt Status ", ULPI_CARKIT_INT_STS);
> + ulpi_print("Carkit Interrupt Latch ", ULPI_CARKIT_INT_LATCH);
> + ulpi_print("Carkit Pulse Control ", ULPI_CARKIT_PLS_CTRL);
> + ulpi_print("Transmit Positive Width ", ULPI_TX_POS_WIDTH);
> + ulpi_print("Transmit Negative Width ", ULPI_TX_NEG_WIDTH);
> + ulpi_print("Receive Polarity Recovery ", ULPI_POLARITY_RECOVERY);
> +
> + return 0;
> +}
> +
> +static int ulpi_regs_open(struct inode *inode, struct file *f)
> +{
> + struct ulpi *ulpi = inode->i_private;
> +
> + return single_open(f, ulpi_regs_read, ulpi);
> +}
> +
> +static const struct file_operations ulpi_regs_ops = {
> + .owner = THIS_MODULE,
> + .open = ulpi_regs_open,
> + .release = single_release,
> + .read = seq_read,
> + .llseek = seq_lseek
> +};
> +
> +#define ULPI_ROOT debugfs_lookup(KBUILD_MODNAME, NULL)
> +
> static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> {
> int ret;
> + struct dentry *root;
>
> ulpi->dev.parent = dev; /* needed early for ops */
> ulpi->dev.bus = &ulpi_bus;
> @@ -251,6 +307,9 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
> if (ret)
> return ret;
>
> + root = debugfs_create_dir(dev_name(dev), ULPI_ROOT);
> + debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops);
> +
> dev_dbg(&ulpi->dev, "registered ULPI PHY: vendor %04x, product %04x\n",
> ulpi->id.vendor, ulpi->id.product);
>
> @@ -296,6 +355,8 @@ EXPORT_SYMBOL_GPL(ulpi_register_interface);
> */
> 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);
> }
> @@ -305,13 +366,21 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_interface);
>
> static int __init ulpi_init(void)
> {
> - return bus_register(&ulpi_bus);
> + int ret;
> + struct dentry *root;
> +
> + root = debugfs_create_dir(KBUILD_MODNAME, NULL);
The file could be accesed now, but you don't register the bus until
after this returns:
> + ret = bus_register(&ulpi_bus);
> + if (ret)
> + debugfs_remove(root);
Can you flip the order around please?
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 <[email protected]>
> >> ---
> >>
> >> 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,
--
heikki
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
On 1/26/22 8:05 AM, Greg Kroah-Hartman wrote:
> On Mon, Jan 24, 2022 at 12:33:43PM -0500, Sean Anderson wrote:
>> This adds a debugfs file for ULPI devices which contains a dump of their
>> registers. This is useful for debugging basic connectivity problems. The
>> file is created in ulpi_register because many devices will never have a
>> driver bound (as they are managed in hardware by the USB controller
>> device).
>>
>> This also modifies the error handling in ulpi_register a bit to ensure
>> that ulpi->dev.of_node is always put.
>>
>> Signed-off-by: Sean Anderson <[email protected]>
>> ---
>>
>> Changes in v2:
>> - Always create debugfs files and ignore errors
>> - Look up dentries dynamically
>>
>> drivers/usb/common/ulpi.c | 71 ++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>> index 4169cf40a03b..87deb514eb78 100644
>> --- a/drivers/usb/common/ulpi.c
>> +++ b/drivers/usb/common/ulpi.c
>> @@ -13,6 +13,7 @@
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> #include <linux/acpi.h>
>> +#include <linux/debugfs.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> #include <linux/clk/clk-conf.h>
>> @@ -228,9 +229,64 @@ static int ulpi_read_id(struct ulpi *ulpi)
>> return 0;
>> }
>>
>> +static int ulpi_regs_read(struct seq_file *seq, void *data)
>> +{
>> + struct ulpi *ulpi = seq->private;
>> +
>> +#define ulpi_print(name, reg) do { \
>> + int ret = ulpi_read(ulpi, reg); \
>> + if (ret < 0) \
>> + return ret; \
>> + seq_printf(seq, name " %.02x\n", ret); \
>> +} while (0)
>> +
>> + ulpi_print("Vendor ID Low ", ULPI_VENDOR_ID_LOW);
>> + ulpi_print("Vendor ID High ", ULPI_VENDOR_ID_HIGH);
>> + ulpi_print("Product ID Low ", ULPI_PRODUCT_ID_LOW);
>> + ulpi_print("Product ID High ", ULPI_PRODUCT_ID_HIGH);
>> + ulpi_print("Function Control ", ULPI_FUNC_CTRL);
>> + ulpi_print("Interface Control ", ULPI_IFC_CTRL);
>> + ulpi_print("OTG Control ", ULPI_OTG_CTRL);
>> + ulpi_print("USB Interrupt Enable Rising ", ULPI_USB_INT_EN_RISE);
>> + ulpi_print("USB Interrupt Enable Falling", ULPI_USB_INT_EN_FALL);
>> + ulpi_print("USB Interrupt Status ", ULPI_USB_INT_STS);
>> + ulpi_print("USB Interrupt Latch ", ULPI_USB_INT_LATCH);
>> + ulpi_print("Debug ", ULPI_DEBUG);
>> + ulpi_print("Scratch Register ", ULPI_SCRATCH);
>> + ulpi_print("Carkit Control ", ULPI_CARKIT_CTRL);
>> + ulpi_print("Carkit Interrupt Delay ", ULPI_CARKIT_INT_DELAY);
>> + ulpi_print("Carkit Interrupt Enable ", ULPI_CARKIT_INT_EN);
>> + ulpi_print("Carkit Interrupt Status ", ULPI_CARKIT_INT_STS);
>> + ulpi_print("Carkit Interrupt Latch ", ULPI_CARKIT_INT_LATCH);
>> + ulpi_print("Carkit Pulse Control ", ULPI_CARKIT_PLS_CTRL);
>> + ulpi_print("Transmit Positive Width ", ULPI_TX_POS_WIDTH);
>> + ulpi_print("Transmit Negative Width ", ULPI_TX_NEG_WIDTH);
>> + ulpi_print("Receive Polarity Recovery ", ULPI_POLARITY_RECOVERY);
>> +
>> + return 0;
>> +}
>> +
>> +static int ulpi_regs_open(struct inode *inode, struct file *f)
>> +{
>> + struct ulpi *ulpi = inode->i_private;
>> +
>> + return single_open(f, ulpi_regs_read, ulpi);
>> +}
>> +
>> +static const struct file_operations ulpi_regs_ops = {
>> + .owner = THIS_MODULE,
>> + .open = ulpi_regs_open,
>> + .release = single_release,
>> + .read = seq_read,
>> + .llseek = seq_lseek
>> +};
>> +
>> +#define ULPI_ROOT debugfs_lookup(KBUILD_MODNAME, NULL)
>> +
>> static int ulpi_register(struct device *dev, struct ulpi *ulpi)
>> {
>> int ret;
>> + struct dentry *root;
>>
>> ulpi->dev.parent = dev; /* needed early for ops */
>> ulpi->dev.bus = &ulpi_bus;
>> @@ -251,6 +307,9 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
>> if (ret)
>> return ret;
>>
>> + root = debugfs_create_dir(dev_name(dev), ULPI_ROOT);
>> + debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops);
>> +
>> dev_dbg(&ulpi->dev, "registered ULPI PHY: vendor %04x, product %04x\n",
>> ulpi->id.vendor, ulpi->id.product);
>>
>> @@ -296,6 +355,8 @@ EXPORT_SYMBOL_GPL(ulpi_register_interface);
>> */
>> 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);
>> }
>> @@ -305,13 +366,21 @@ EXPORT_SYMBOL_GPL(ulpi_unregister_interface);
>>
>> static int __init ulpi_init(void)
>> {
>> - return bus_register(&ulpi_bus);
>> + int ret;
>> + struct dentry *root;
>> +
>> + root = debugfs_create_dir(KBUILD_MODNAME, NULL);
>
> The file could be accesed now, but you don't register the bus until
> after this returns:
>
>> + ret = bus_register(&ulpi_bus);
>> + if (ret)
>> + debugfs_remove(root);
>
> Can you flip the order around please?
This is just registering the holding directory. To ensure that we
don't race like
CPU 1 CPU 2
bus_register()
ulpi_register_interface()
debugfs_lookup() /* whoops, no directory */
debugfs_create_dir()
By registering the directory first, we ensure that devices added
will always have a directory to put their debugfs files under.
--Sean