Subject: Re: [PATCH V2 1/2] pseries/eeh: Refactor the configure bridge RTAS tokens

From: Gavin Shan <[email-masked]>
Date: Tue, 29 Mar 2016 20:26:34 +1100

On Tue, Mar 29, 2016 at 03:53:19PM +1000, Russell Currey wrote:
>On Tue, 2016-03-29 at 16:26 +1100, Gavin Shan wrote:
>> On Tue, Mar 29, 2016 at 12:51:50PM +1000, Russell Currey wrote:
><snip>
>> > 	/*
>> > 	 * Necessary sanity check. We needn't check "get-config-addr-info"
>> > @@ -93,8 +98,7 @@ static int pseries_eeh_init(void)
>> > 	    (ibm_read_slot_reset_state2 == RTAS_UNKNOWN_SERVICE &&
>> > 	     ibm_read_slot_reset_state == RTAS_UNKNOWN_SERVICE)	||
>> > 	    ibm_slot_error_detail == RTAS_UNKNOWN_SERVICE	||
>> > -	    (ibm_configure_pe == RTAS_UNKNOWN_SERVICE		&
>> > &
>> > -	     ibm_configure_bridge == RTAS_UNKNOWN_SERVICE)) {
>> > +	    ibm_configure_pe == RTAS_UNKNOWN_SERVICE) {
>> > 		pr_info("EEH functionality not supported\n");
>> > 		return -EINVAL;
>> > 	}
>> Since you're here, you can do similar thing to @ibm_read_slot_reset_state
>> and @ibm_read_slot_reset_state?
>
>Ah, didn't notice there was a similar thing going on there.  Will fix.

Ok.

>> 
>> > 
>> > @@ -621,18 +625,9 @@ static int pseries_eeh_configure_bridge(struct
>> > eeh_pe *pe)
>> > 	if (pe->addr)
>> > 		config_addr = pe->addr;
>> > 
>> > -	/* Use new configure-pe function, if supported */
>> > -	if (ibm_configure_pe != RTAS_UNKNOWN_SERVICE) {
>> > -		ret = rtas_call(ibm_configure_pe, 3, 1, NULL,
>> > -				config_addr, BUID_HI(pe->phb->buid),
>> > -				BUID_LO(pe->phb->buid));
>> > -	} else if (ibm_configure_bridge != RTAS_UNKNOWN_SERVICE) {
>> > -		ret = rtas_call(ibm_configure_bridge, 3, 1, NULL,
>> > -				config_addr, BUID_HI(pe->phb->buid),
>> > -				BUID_LO(pe->phb->buid));
>> > -	} else {
>> > -		return -EFAULT;
>> > -	}
>> > +	ret = rtas_call(ibm_configure_pe, 3, 1, NULL,
>> > +			config_addr, BUID_HI(pe->phb->buid),
>> > +			BUID_LO(pe->phb->buid));
>> > 
>> Russell, it seems not working if "ibm,configure-pe" and "ibm,configure-
>> bridge" are all
>> missed from "/rtas".
>
>If they're both missing, then the init should fail as ibm_configure_pe will
>be RTAS_UNKNOWN_SERVICE, so this code should never be called.
>

Yeah, I missed the point, thanks.

>>  Also, I don't think we need backport it to 3.10+ as it's not fixing
>> any bugs if I'm correct enough.
>
>This patch doesn't, but the second patch does.
>

Ok. In the commit log of this patch, you have something like below and that
means it needs by stable kernels. I agree the next one is needed by stable
kernels, so the two patches would have inversed order if you agree. In that
case, the next one (to be in stable kernels) won't depend on current on which
isn't required by stable kernels.

Cc: <[email-masked]> # 3.10-   <<< The format would be 3.10+

Thanks,
Gavin

>> > 
>> > 	if (ret)
>> > 		pr_warn("%s: Unable to configure bridge PHB#%d-PE#%x
>> > (%d)\n",
>