Re: [PATCH] iommu: dont fail silently

[Dragan Simic <dsimic@manjaro.org>]

On 2024-01-05 18:50, Tom Rini wrote:
> 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.

Sounds good to me, thanks for the clarification.