On Fri, Jan 05, 2024 at 05:48:58PM +0100, Dragan Simic wrote:
On 2024-01-05 17:09, Caleb Connolly wrote:
On 05/01/2024 14:47, Dragan Simic wrote:
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.
Yes, and this would be fine, but when I need to know to add "#define LOG_DEBUG" specifically to drivers/iommu/iommu-uclass.c to know that my driver failed to probe due to the IOMMU device not being found... Well, you can see how that isn't super useful in this case.
Oh, I see and I know it very well first hand. For example, I spent ages once debugging some MMC-related issue, just because no logs were produced unless debugging was enabled. On the other hand, pinpointing the issue with the debugging enabled was a five-minute job. That was the hard way for me to learn to enable debugging first where it's required during development.
Maybe adding a single error message to device_probe() if the call to dev_iommu_enable() fails would be a sensible middleground here?
Please, don't get me wrong, I'm not opposing an approach like that, but how about maybe going one step further and introducing another debug level at the same time? That new debug level would include a small-enough subset of highly important debug messages so that it could be enabled at the top level during development, without breaking the image size constraints.
Of course, it would take time for various parts of U-Boot to migrate to using that new debug level, but given enough time and taking the usability into account, it probably should happen eventually. It would be highly useful during development, IMHO, while still keeping the production image sizes down.
We already have "good" log levels, the issue is that code isn't using them consistently. I think here, a log_err is fine. I'll size check things when merging and worst case we'll change it to log_warn as one of the standard "I need to reduce size" is to lower LOGLEVEL to only err, and discard warning and higher.