Re: [PATCH v4 4/8] hw/misc/pca9552: Add a 'description' property for debugging purpose

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 6/25/20 10:12 AM, Philippe Mathieu-Daudé wrote:
>> On 6/25/20 8:37 AM, Markus Armbruster wrote:
>>> Cédric Le Goater <clg@kaod.org> writes:
>>>
>>>> On 6/22/20 10:31 AM, Philippe Mathieu-Daudé wrote:
>>>>> On 6/22/20 8:27 AM, Cédric Le Goater wrote:
>>>>>> On 6/21/20 12:58 AM, Philippe Mathieu-Daudé wrote:
>>>>>>> Add a description field to distinguish between multiple devices.
>>>
>>> Pardon my lack of imagination: how does this help you with debugging?
>> 
>> Ah, the patch subject is indeed incorrect, this should be:
>> "... for *tracing* purpose" (I use tracing when debugging).
>> 
>> In the next patch, we use the 'description' property:
>> 
>> +# pca9552.c
>> +pca9552_gpio_status(const char *description, const char *buf) "%s GPIOs
>> 0-15 [%s]"
>> 
>> So when the machine has multiple PCA9552 and guest accesses both,
>> we can distinct which one is used. For me having "pca1" / "pca0"
>> is easier to follow than the address of the QOM object.
>> 
>>>
>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>>>
>>>>>> Could it be a QOM attribute ? 
>>>>>
>>>>> What do you call a 'QOM attribute'?
>>>>> Is it what qdev properties implement?
>>>>> (in this case via DEFINE_PROP_STRING).
>>>>
>>>> I meant a default Object property, which would apply to all Objects. 
>>>
>>> Good point.  Many devices have multiple component objects of the same
>>> type.
>>>
>>>> What you did is fine, so :
>>>>
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>>
>>>> but, may be, a well defined child name is enough for the purpose.
>>>
>>> object_get_canonical_path() returns a distinct path for each (component)
>>> object.  The path components are the child property names.
>>>
>>> Properties can have descriptions: object_property_set_description().
>> 
>> TIL object_property_set_description :>
>> 
>> Ah, there is no equivalent object_property_get_description(),
>> we have to use object_get_canonical_path(). Hmm, not obvious.
>> 
>>>
>>> Sufficient?
>> 
>> I don't know... This seems a complex way to do something simple...
>> This is already a QDEV. Having to use QOM API seems going
>> backward, since we have the DEFINE_PROP_STRING() macros available
>> in "hw/qdev-properties.h".
>> 
>> Maybe I'm not seeing the advantages clearly. I'll try later.
>
> The canonical path is not very helpful in trace log...

Why?

Okay, I checked the code.  Since the devices in question don't get a
composition tree parent assigned, realize puts them in the
/machine/unattached orphanage.  The canonical path is something like
"/machine/unattached/device[6]", which is less than clear.

The components of the canonical path are the names of the QOM child
properties.  object_get_canonical_path_component() returns the last one,
in this case "device[6]".

If we made the devices QOM children of some other device, we could name
the child properties "pca0" and "pca1".
object_get_canonical_path_component() would then return the strings you
want to see.

We make a device a QOM child of some QOM parent device only if the child
is a component device of the parent (hence the name "composition
tree").

Are these devices integral components of something else, or are they
separate chips?

> The description I set matches the hardware definitions
> and is easier to follow, see patch #6 (where it is set)
> where the description comes from:
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714658.html
>
>   Description name taken from:
>   https://github.com/open-power/witherspoon-xml/blob/master/witherspoon.xml
>
> So in this particular case I don't find the canonical pathname
> practical (from an hardware debugging perspective).

Personally, I'd be content with i2c bus and address for debugging
purposes.

The i2c buses *are* components: canonical paths look like
"/machine/soc/i2c/aspeed.i2c.3".  The combination of
object_get_canonical_path_component(dev) and
object_property_get_uint(dev, "address", &error_abort) identifies any
i2c device on this machine, not just the two you decorate with a
description string.