[PATCH] iommu: dont fail silently

[PATCH] iommu: dont fail silently

[Caleb Connolly]

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.

Signed-off-by: Caleb Connolly <caleb.connolly@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;
+			}
 		}
 	}
 
-- 
2.42.1

Re: [PATCH] iommu: dont fail silently

[Dragan Simic]

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.

> Signed-off-by: Caleb Connolly <caleb.connolly@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;
> +			}
>  		}
>  	}

Re: [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@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)

Re: [PATCH] iommu: dont fail silently

[Dragan Simic]

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.

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

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

>>> Signed-off-by: Caleb Connolly <caleb.connolly@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;
>>> +            }
>>>          }
>>>      }

Re: [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@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)

Re: [PATCH] iommu: dont fail silently

[Dragan Simic]

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.

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

Yes, and I'd love to also hear opinions about the new debug level, 
described above.  Maybe it would simply be too convoluted;  if deemed 
so, I'm fine with the middle ground that you proposed above.

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

Quite frankly, I've always felt that simply using printf() is a bit out 
of place.  We should instead aim toward having granular and manageable 
log levels, if you agree.

>>>>> Signed-off-by: Caleb Connolly <caleb.connolly@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;
>>>>> +            }
>>>>>          }
>>>>>      }

Re: [PATCH] iommu: dont fail silently

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4034 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] iommu: dont fail silently

[Dragan Simic]

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.

Re: [PATCH] iommu: dont fail silently

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On Thu, Jan 04, 2024 at 05:12:22PM +0000, 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.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]