Re: [PATCH v3 1/2] powerpc/powernv: Enhance opal message read interface

[Vasant Hegde <hegdevasant@linux.vnet.ibm.com>]

On 8/22/19 9:06 PM, Vasant Hegde wrote:
> On 8/22/19 11:21 AM, Oliver O'Halloran wrote:
>> On Wed, 2019-08-21 at 13:43 +0530, Vasant Hegde wrote:
>>> Use "opal-msg-size" device tree property to allocate memory for "opal_msg".
>>>
>>> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>> Cc: Jeremy Kerr <jk@ozlabs.org>
>>> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>>> ---
>>> Changes in v3:
>>>    - Call BUG_ON, if we fail to allocate memory during init.
>>>
>>> -Vasant
>>>
>>>   arch/powerpc/platforms/powernv/opal.c | 29 ++++++++++++++++++---------
>>>   1 file changed, 19 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/opal.c 
>>> b/arch/powerpc/platforms/powernv/opal.c
>>> index aba443be7daa..4f1f68f568bf 100644
>>> --- a/arch/powerpc/platforms/powernv/opal.c
>>> +++ b/arch/powerpc/platforms/powernv/opal.c
>>> @@ -58,6 +58,8 @@ static DEFINE_SPINLOCK(opal_write_lock);
>>>   static struct atomic_notifier_head opal_msg_notifier_head[OPAL_MSG_TYPE_MAX];
>>>   static uint32_t opal_heartbeat;
>>>   static struct task_struct *kopald_tsk;
>>> +static struct opal_msg *opal_msg;
>>> +static uint64_t opal_msg_size;
>>>   void opal_configure_cores(void)
>>>   {
>>> @@ -271,14 +273,9 @@ static void opal_message_do_notify(uint32_t msg_type, 
>>> void *msg)
>>>   static void opal_handle_message(void)
>>>   {
>>>       s64 ret;
>>> -    /*
>>> -     * TODO: pre-allocate a message buffer depending on opal-msg-size
>>> -     * value in /proc/device-tree.
>>> -     */
>>> -    static struct opal_msg msg;
>>>       u32 type;
>>> -    ret = opal_get_msg(__pa(&msg), sizeof(msg));
>>> +    ret = opal_get_msg(__pa(opal_msg), opal_msg_size);
>>>       /* No opal message pending. */
>>>       if (ret == OPAL_RESOURCE)
>>>           return;
>>> @@ -290,14 +287,14 @@ static void opal_handle_message(void)
>>>           return;
>>>       }
>>> -    type = be32_to_cpu(msg.msg_type);
>>> +    type = be32_to_cpu(opal_msg->msg_type);
>>>       /* Sanity check */
>>>       if (type >= OPAL_MSG_TYPE_MAX) {
>>>           pr_warn_once("%s: Unknown message type: %u\n", __func__, type);
>>>           return;
>>>       }
>>> -    opal_message_do_notify(type, (void *)&msg);
>>> +    opal_message_do_notify(type, (void *)opal_msg);
>>>   }
>>>   static irqreturn_t opal_message_notify(int irq, void *data)
>>> @@ -306,9 +303,21 @@ static irqreturn_t opal_message_notify(int irq, void *data)
>>>       return IRQ_HANDLED;
>>>   }
>>> -static int __init opal_message_init(void)
>>> +static int __init opal_message_init(struct device_node *opal_node)
>>>   {
>>>       int ret, i, irq;
>>
>>> +    const __be32 *val;
>>> +
>>> +    val = of_get_property(opal_node, "opal-msg-size", NULL);
>>> +    if (val)
>>> +        opal_msg_size = be32_to_cpup(val);
>>
>> Use of_property_read_u32()
> 
> Yes. Will fix it.
> 
>>
>>> +
>>> +    /* If opal-msg-size property is not available then use default size */
>>> +    if (!opal_msg_size)
>>> +        opal_msg_size = sizeof(struct opal_msg);
>>> +
>>> +    opal_msg = kmalloc(opal_msg_size, GFP_KERNEL);
>>
>>> +    BUG_ON(opal_msg == NULL);
>>
>> Seems questionable. Why not fall back to using a staticly allocated
>> struct opal_msg? Or re-try the allocation with the size limited to
>> sizeof(struct opal_msg)?
> 
> If we are not able to allocate memory during init then we have bigger problem.
> No point in continuing. Hence added BUG_ON().
> May be I can retry allocation with fixed size before calling BUG_ON().
> How about something like below :
> 
> +       /* If opal-msg-size property is not available then use default size */
> +       if (!opal_msg_size)
> +               opal_msg_size = sizeof(struct opal_msg);
> +
> +       opal_msg = kmalloc(opal_msg_size, GFP_KERNEL);
> +       if (!opal_msg) {

Just to be clear I will adjust `opal_msg_size` size here.

-Vasant

> +               opal_msg = kmalloc(sizeof(struct opal_msg), GFP_KERNEL);
> +               BUG_ON(opal_msg == NULL);
> +       }
> 
> 
> -Vasant
>