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

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

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) {
+               opal_msg = kmalloc(sizeof(struct opal_msg), GFP_KERNEL);
+               BUG_ON(opal_msg == NULL);
+       }


-Vasant