[PATCH] iommu: dont fail silently

[Caleb Connolly]

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.

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.

If you're really space constrained then wouldn't you disable log
messages altogether?

> 
>> 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)