diff mbox

[V2,1/2] pseries/eeh: Refactor the configure bridge RTAS tokens

Message ID 1459219911-14110-1-git-send-email-ruscur@russell.cc (mailing list archive)
State Changes Requested
Headers show

Commit Message

Russell Currey March 29, 2016, 2:51 a.m. UTC 
The RTAS calls configure-pe and configure-bridge perform the same
actions, however the former can skip configuration if unnecessary.  The
existing code treats them as different tokens even though only one will
ever be called.  Refactor this by making a single token that is assigned
during init.

Cc: <stable@vger.kernel.org> # 3.10-
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
 arch/powerpc/platforms/pseries/eeh_pseries.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

Comments

Gavin Shan March 29, 2016, 5:26 a.m. UTC | #1 
On Tue, Mar 29, 2016 at 12:51:50PM +1000, Russell Currey wrote:
>The RTAS calls configure-pe and configure-bridge perform the same
>actions, however the former can skip configuration if unnecessary.  The
>existing code treats them as different tokens even though only one will
>ever be called.  Refactor this by making a single token that is assigned
>during init.
>
>Cc: <stable@vger.kernel.org> # 3.10-
>Signed-off-by: Russell Currey <ruscur@russell.cc>
>---
> arch/powerpc/platforms/pseries/eeh_pseries.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
>diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
>index ac3ffd9..231b1df 100644
>--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
>+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
>@@ -53,7 +53,6 @@ static int ibm_read_slot_reset_state2;
> static int ibm_slot_error_detail;
> static int ibm_get_config_addr_info;
> static int ibm_get_config_addr_info2;
>-static int ibm_configure_bridge;
> static int ibm_configure_pe;
> 
> /*
>@@ -81,7 +80,13 @@ static int pseries_eeh_init(void)
> 	ibm_get_config_addr_info2	= rtas_token("ibm,get-config-addr-info2");
> 	ibm_get_config_addr_info	= rtas_token("ibm,get-config-addr-info");
> 	ibm_configure_pe		= rtas_token("ibm,configure-pe");
>-	ibm_configure_bridge		= rtas_token("ibm,configure-bridge");
>+
>+	/*
>+	 * configure-pe and configure-bridge perform the same actions, however
>+	 * the former is preferred as it can skip configuration if unnecessary.
>+	 */
>+	if (ibm_configure_pe == RTAS_UNKNOWN_SERVICE)
>+		ibm_configure_pe	= rtas_token("ibm,configure-bridge");
> 
> 	/*
> 	 * 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?

>@@ -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". Also, I don't think we need backport it to 3.10+ as it's not fixing
any bugs if I'm correct enough.

Thanks,
Gavin


> 	if (ret)
> 		pr_warn("%s: Unable to configure bridge PHB#%d-PE#%x (%d)\n",
>-- 
>2.7.4
>
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@lists.ozlabs.org
>https://lists.ozlabs.org/listinfo/linuxppc-dev
Russell Currey March 29, 2016, 5:53 a.m. UTC | #2 
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.
> 
> > 
> > @@ -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.

>  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.

> 
> Thanks,
> Gavin
> 
> 
> > 
> > 	if (ret)
> > 		pr_warn("%s: Unable to configure bridge PHB#%d-PE#%x
> > (%d)\n",
Gavin Shan March 29, 2016, 9:26 a.m. UTC | #3 
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: <stable@vger.kernel.org> # 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",
>
diff mbox

Patch

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index ac3ffd9..231b1df 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -53,7 +53,6 @@  static int ibm_read_slot_reset_state2;
 static int ibm_slot_error_detail;
 static int ibm_get_config_addr_info;
 static int ibm_get_config_addr_info2;
-static int ibm_configure_bridge;
 static int ibm_configure_pe;
 
 /*
@@ -81,7 +80,13 @@  static int pseries_eeh_init(void)
 	ibm_get_config_addr_info2	= rtas_token("ibm,get-config-addr-info2");
 	ibm_get_config_addr_info	= rtas_token("ibm,get-config-addr-info");
 	ibm_configure_pe		= rtas_token("ibm,configure-pe");
-	ibm_configure_bridge		= rtas_token("ibm,configure-bridge");
+
+	/*
+	 * configure-pe and configure-bridge perform the same actions, however
+	 * the former is preferred as it can skip configuration if unnecessary.
+	 */
+	if (ibm_configure_pe == RTAS_UNKNOWN_SERVICE)
+		ibm_configure_pe	= rtas_token("ibm,configure-bridge");
 
 	/*
 	 * 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;
 	}
@@ -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));
 
 	if (ret)
 		pr_warn("%s: Unable to configure bridge PHB#%d-PE#%x (%d)\n",