Re: [PATCH v5 01/10] hw/fsi: Introduce IBM's Local bus

[Ninad Palsule <ninad@linux.ibm.com>]

Hello Daniel,

On 10/19/23 11:09, Daniel P. Berrangé wrote:
> On Thu, Oct 19, 2023 at 10:34:52AM -0500, Ninad Palsule wrote:
>> Hello Daniel,
>>
>> On 10/19/23 03:14, Daniel P. Berrangé wrote:
>>> On Wed, Oct 11, 2023 at 10:13:30AM -0500, Ninad Palsule wrote:
>>>> This is a part of patchset where IBM's Flexible Service Interface is
>>>> introduced.
>>>>
>>>> The LBUS is modelled to maintain the qdev bus hierarchy and to take
>>>> advantage of the object model to automatically generate the CFAM
>>>> configuration block. The configuration block presents engines in the
>>>> order they are attached to the CFAM's LBUS. Engine implementations
>>>> should subclass the LBusDevice and set the 'config' member of
>>>> LBusDeviceClass to match the engine's type.
>>>>
>>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>>>> ---
>>>> v2:
>>>> - Incorporated Joel's review comments.
>>>> v5:
>>>> - Incorporated review comments by Cedric.
>>>> ---
>>>>    include/hw/fsi/lbus.h | 51 +++++++++++++++++++++++++
>>>>    include/qemu/bitops.h |  6 +++
>>>>    hw/fsi/lbus.c         | 87 +++++++++++++++++++++++++++++++++++++++++++
>>>>    hw/Kconfig            |  1 +
>>>>    hw/fsi/Kconfig        |  2 +
>>>>    hw/fsi/meson.build    |  1 +
>>>>    hw/meson.build        |  1 +
>>>>    7 files changed, 149 insertions(+)
>>>>    create mode 100644 include/hw/fsi/lbus.h
>>>>    create mode 100644 hw/fsi/lbus.c
>>>>    create mode 100644 hw/fsi/Kconfig
>>>>    create mode 100644 hw/fsi/meson.build
>>>> +DeviceState *lbus_create_device(FSILBus *bus, const char *type, uint32_t addr)
>>>> +{
>>>> +    DeviceState *dev;
>>>> +    FSILBusNode *node;
>>>> +    BusState *state = BUS(bus);
>>>> +
>>>> +    dev = qdev_new(type);
>>>> +    qdev_prop_set_uint8(dev, "address", addr);
>>>> +    qdev_realize_and_unref(dev, state, &error_fatal);
>>>> +
>>>> +    /* Move to post_load */
>>>> +    node = g_malloc(sizeof(struct FSILBusNode));
>>> This allocation pattern is discouraged in favour of:
>>>
>>>       node = g_new0(FSILBusNode, 1);
>> I am using g_malloc() because I want program to terminate. I don't think
>> g_new0 provide this functionality. Please let me know.
> All the glib memory allocation functions terminate on OOM, except
> for the ones with '_try_' in their name.

Sorry, you are right. I have removed this function as per Cedric's comment.

Thanks for the review.

Regards,

Ninad

>
> With regards,
> Daniel