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

[Markus Armbruster <armbru@redhat.com>]

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

> On 6/26/20 7:49 AM, Markus Armbruster wrote:
>> 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.

This is common for onboard devices.  I hate it.

Some boards do better than others.  For instance, ast2600-evb has 33
sensibly named entries under /machine/soc/, and only 9 entries in the
/machine/unattached/ orphanage.  i440fx has two sensible named ones
under /machine/, and 26 in the orphanage.

>> 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?
>
> Separate chips in the machine (actually not even on the machine mother
> board where is the CPU, but on a daughter board card).
>
> So in the composition tree I expect to see them as
>
>   /machine/pca0
>   /machine/pca1

As long as the final canonical path component is something readable
rather than auto-generated crap like "device[6]",
object_get_canonical_path_component() is usable for tracing.

>>> 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.
>
> The I2C busses is provided by Aspeed peripherals. I counted 19 different
> I2C busses on this machine.
>
> "pca0" is connected to i2c bus #11, "pca1" to bus #3.
>
> I still don't think this will be practical, but I'll try your
> suggestion.

"bus=11 addr=0x60" isn't exactly wonderful, but it seems practical
enough for me.  Even "device[6]" seems workable, just bothersome,
because it's needlessly hard to remember, and prone to change.

Mind, I'm not recommending any particular solution for "want a more
useful device ID in traces", I'm just throwing out ideas on how to solve
the problem in a more general way.

Working towards a a neater QOM composition tree might help with more
than tracing.