[PATCH] iommu: dont fail silently

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240105/f86b6a23/attachment.sig>