[PATCH] iommu: dont fail silently

[Caleb Connolly]

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.

Maybe adding a single error message to device_probe() if the call to
dev_iommu_enable() fails would be a sensible middleground here?
> 
>> 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.

Yeah, I'm curious to know how others feel about this.
> 
>> 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.

Well, you can disable logging but still get printf() outputs I mean...
> 
>>>> Signed-off-by: Caleb Connolly <caleb.connolly at 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;
>>>> +            }
>>>>          }
>>>>      }

-- 
// Caleb (they/them)