Re: [PATCH v3 1/12] Create a powerpc update_devicetree interface

[Nathan Fontenot <nfont@linux.vnet.ibm.com>]

On 04/22/2013 07:15 PM, Benjamin Herrenschmidt wrote:
> On Mon, 2013-04-22 at 13:30 -0500, Nathan Fontenot wrote:
> 
>> This patch exposes a method for updating the device tree via
>> ppc_md.update_devicetree that takes a single 32-bit value as a parameter.
>> For pseries platforms this is the existing pseries_devicetree_update routine
>> which is updated to take the new parameter which is a scope value
>> to indicate the the reason for making the rtas calls. This parameter is
>> required by the ibm,update-nodes/ibm,update-properties RTAS calls, and
>> the appropriate value is contained within the RTAS event for PRRN
>> notifications. In pseries_devicetree_update() it was previously
>> hard-coded to 1, the scope value for partition migration.
> 
> I think that's too much abstraction.... (see below)
> 
> Also you add this helper:
> 
>> Index: powerpc/arch/powerpc/kernel/rtas.c
>> ===================================================================
>> --- powerpc.orig/arch/powerpc/kernel/rtas.c	2013-03-08 19:23:06.000000000 -0600
>> +++ powerpc/arch/powerpc/kernel/rtas.c	2013-04-17 13:02:29.000000000 -0500
>> @@ -1085,3 +1085,13 @@
>>  	timebase = 0;
>>  	arch_spin_unlock(&timebase_lock);
>>  }
>> +
>> +int update_devicetree(s32 scope)
>> +{
>> +	int rc = 0;
>> +
>> +	if (ppc_md.update_devicetree)
>> +		rc = ppc_md.update_devicetree(scope);
>> +
>> +	return rc;
>> +}
> 
> But then don't use it afaik (you call directly ppc_md.update_... from
> prrn_work_fn().
> 
> In the end, the caller (PRRN stuff), while in rtasd, is really pseries
> specific and the resulting update_device_tree() as well, so I don't
> think we need the ppc_md. hook in the middle with that "oddball" scope
> parameter which is not defined outside of pseries specific areas.
> 
> In this case, it might be better to make sure the PRRN related stuff in
> rtasd is inside an ifdef CONFIG_PPC_PSERIES and have it call directly
> into pseries_update_devicetree().
> 
> It makes the code somewhat easier to follow and I doubt anybody else
> will ever use that specific hook, at least not in its current form. If
> we need an abstraction later, we can add one then.
> 

ok, good. I was not crazy about using ppc_md to do this and if you're fine
with putting the pseries specific stuff in ifdef CONFIG_PPC_PSERIES I'll
update the code to do that.

Question concerning rtas code. There is quite a bit of pseries specific 
pieces in the general powerpc/kernel directory. Has there been, or should
there be, any effort to move that to the pseries directory?

-Nathan