Re: [Qemu-arm] [Qemu-devel] [PATCH] hw/arm/bcm2836: Mark the bcm2836 / bcm2837 devices with user_creatable = false

[Thomas Huth]

On 16.07.2018 16:18, Markus Armbruster wrote:
> Thomas Huth <address@hidden> writes:
> 
>> These devices are currently causing some problems when a user is trying
>> to hot-plug or introspect them during runtime. Since these devices can
>> not be instantiated by the user at all (they need to be wired up in code
>> instead), we should mark them with user_creatable = false anyway, then we
>> avoid at least the crashes with the hot-plugging. The introspection problem
>> will be handled by a separate patch.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  hw/arm/bcm2836.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
>> index 6805a7d..45d9e40 100644
>> --- a/hw/arm/bcm2836.c
>> +++ b/hw/arm/bcm2836.c
>> @@ -185,6 +185,8 @@ static void bcm283x_class_init(ObjectClass *oc, void 
>> *data)
>>      bc->info = data;
>>      dc->realize = bcm2836_realize;
>>      dc->props = bcm2836_props;
>> +    /* Reason: Must be wired up in code (see raspi_init() function) */
>> +    dc->user_creatable = false;
>>  }
>>  
>>  static const TypeInfo bcm283x_type_info = {
> 
> A more common way to phrase this is
> 
>        /* Reason: needs to be wired-up by raspi_init() */
> 
> If you you expect other functions to wire this one up in the future,
> insert ", e.g." before "by".

$ grep -r Reason.*wire hw/
hw/acpi/piix4.c:     * Reason: part of PIIX4 southbridge, needs to be wired up,
hw/core/or-irq.c:    /* Reason: Needs to be wired up to work, e.g. see 
stm32f205_soc.c */
hw/core/register.c:    /* Reason: needs to be wired up to work */
hw/core/split-irq.c:    /* Reason: Needs to be wired up to work */
hw/dma/i8257.c:    /* Reason: needs to be wired up by isa_bus_dma() to work */
hw/i2c/smbus_ich9.c:     * Reason: part of ICH9 southbridge, needs to be wired 
up by
hw/ide/microdrive.c:    /* Reason: Needs to be wired-up in code, see 
dscm1xxxx_init() */
hw/intc/apic_common.c:     * Reason: APIC and CPU need to be wired up by
hw/intc/nios2_iic.c:    /* Reason: needs to be wired up, e.g. by 
nios2_10m50_ghrd_init() */
hw/isa/piix4.c:     * Reason: part of PIIX4 southbridge, needs to be wired up,
hw/isa/vt82c686.c:     * Reason: part of VIA VT82C686 southbridge, needs to be 
wired up,
hw/isa/lpc_ich9.c:     * Reason: part of ICH9 southbridge, needs to be wired up 
by
hw/pci-host/piix.c:     * Reason: part of PIIX3 southbridge, needs to be wired 
up by
hw/pci-host/piix.c:    /* Reason: needs to be wired up by pc_init1 */
hw/pci-host/q35.c:    /* Reason: needs to be wired up by pc_q35_init */
hw/timer/mc146818rtc.c:    /* Reason: needs to be wired up by rtc_init() */

... does not look very consistent to me.

But if Peter feels like it should be changed, too, please feel free
to modify the comment when picking up the patch (I won't resend it
again just because of this).

> I like consistency in such things, but it's matter of taste, thus:
> Reviewed-by: Markus Armbruster <address@hidden>

 Thanks,
  Thomas