On 2024-01-05 14:55, Caleb Connolly wrote:
On 04/01/2024 17:47, Dragan Simic wrote:
On 2024-01-04 18:12, Caleb Connolly wrote:
When attempting to probe a device which has an associated IOMMU, if the IOMMU device can't be found (no driver, disabled driver, driver failed to probe, etc) then we currently fail to probe the device with no discernable error.
If we fail to hook the device up to its IOMMU, we should make sure that the user knows about it. Write some better error messages for dev_iommu_enable() to facilitate this.
Quite frankly, I think those should remain as debug-only error messages, but of course with the additional details you added. The reasoning behind it would be to use debug builds while testing, to be able to see any error messages, and then use production, size-optimized builds later. I hope you agree.
There are many other places where using log messages instead of debug messages would be really useful in the field, such as in the MMC drivers, but we'd quickly start increasing the image sizes if we start converting those from debug to log messages.
The problem is that with a debug build there are SO MANY messages, it's hard to spot actual errors in the midst of the other stuff. I would rather see some actual numbers to justify breaking ease-of-use like this.
As a reminder, debugging can be enabled on a per-file basis, which is usually the way it's performed. In other words, debug messages usually get enabled during development only on the files you're actually working on, and perhaps on a few more related files.
I can definitely foresee folks hitting this error path when using an incompatible devicetree, having absolutely no idea where to start debugging is pretty frustrating and I don't think it's the right tradeoff to make in this case.
I see. Perhaps we should hear opinions from other people as well.
If you're really space constrained then wouldn't you disable log messages altogether?
Of course not, but there aren't _that_ many log messages already. Actually, "oh, logging that would be nice" has crossed my mind many times, while either debug messages were already in place, or there was nothing logged at all.
Signed-off-by: Caleb Connolly caleb.connolly@linaro.org
drivers/iommu/iommu-uclass.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/iommu-uclass.c b/drivers/iommu/iommu-uclass.c index 6babc0e3a672..dff3239cccb1 100644 --- a/drivers/iommu/iommu-uclass.c +++ b/drivers/iommu/iommu-uclass.c @@ -86,16 +86,16 @@ int dev_iommu_enable(struct udevice *dev) ret = dev_read_phandle_with_args(dev, "iommus", "#iommu-cells", 0, i, &args); if (ret) { - debug("%s: dev_read_phandle_with_args failed: %d\n", - __func__, ret); + log_err("%s: Failed to parse 'iommus' property for '%s': %d\n", + __func__, dev->name, ret); return ret; }
ret = uclass_get_device_by_ofnode(UCLASS_IOMMU, args.node, &dev_iommu); if (ret) { - debug("%s: uclass_get_device_by_ofnode failed: %d\n", - __func__, ret); + log_err("%s: Failed to find IOMMU device for '%s': %d\n", + __func__, dev->name, ret); return ret; } dev->iommu = dev_iommu; @@ -106,8 +106,11 @@ int dev_iommu_enable(struct udevice *dev) ops = device_get_ops(dev->iommu); if (ops && ops->connect) { ret = ops->connect(dev); - if (ret) + if (ret) { + log_err("%s: Failed to connect '%s' to IOMMU '%s': %d\n", + __func__, dev->name, dev->iommu->name, ret); return ret; + } } }