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

From: Gavin Shan <gwshan@linux.vnet.ibm.com>
To: Russell Currey <ruscur@russell.cc>
Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH V2 1/2] pseries/eeh: Refactor the configure bridge RTAS tokens
Date: Tue, 29 Mar 2016 20:26:34 +1100	[thread overview]
Message-ID: <20160329092634.GA10585@gwshan> (raw)
In-Reply-To: <1459230799.21529.2.camel@russell.cc>

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)
>> > 	=A0=A0=A0=A0(ibm_read_slot_reset_state2 =3D=3D RTAS_UNKNOWN_SERVICE=
 &&
>> > 	=A0=A0=A0=A0=A0ibm_read_slot_reset_state =3D=3D RTAS_UNKNOWN_SERVIC=
E)	||
>> > 	=A0=A0=A0=A0ibm_slot_error_detail =3D=3D RTAS_UNKNOWN_SERVICE	||
>> > -	=A0=A0=A0=A0(ibm_configure_pe =3D=3D RTAS_UNKNOWN_SERVICE		&
>> > &
>> > -	=A0=A0=A0=A0=A0ibm_configure_bridge =3D=3D RTAS_UNKNOWN_SERVICE)) =
{
>> > +	=A0=A0=A0=A0ibm_configure_pe =3D=3D 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_st=
ate
>> and @ibm_read_slot_reset_state?
>
>Ah, didn't notice there was a similar thing going on there. =A0Will fix.

Ok.

>>=20
>> >=20
>> > @@ -621,18 +625,9 @@ static int pseries_eeh_configure_bridge(struct
>> > eeh_pe *pe)
>> > 	if (pe->addr)
>> > 		config_addr =3D pe->addr;
>> >=20
>> > -	/* Use new configure-pe function, if supported */
>> > -	if (ibm_configure_pe !=3D RTAS_UNKNOWN_SERVICE) {
>> > -		ret =3D 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 !=3D RTAS_UNKNOWN_SERVICE) {
>> > -		ret =3D rtas_call(ibm_configure_bridge, 3, 1, NULL,
>> > -				config_addr, BUID_HI(pe->phb->buid),
>> > -				BUID_LO(pe->phb->buid));
>> > -	} else {
>> > -		return -EFAULT;
>> > -	}
>> > +	ret =3D rtas_call(ibm_configure_pe, 3, 1, NULL,
>> > +			config_addr, BUID_HI(pe->phb->buid),
>> > +			BUID_LO(pe->phb->buid));
>> >=20
>> 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=A0ibm_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 th=
at
means it needs by stable kernels. I agree the next one is needed by stabl=
e
kernels, so the two patches would have inversed order if you agree. In th=
at
case, the next one (to be in stable kernels) won't depend on current on w=
hich
isn't required by stable kernels.

Cc: <stable@vger.kernel.org> # 3.10-   <<< The format would be 3.10+

Thanks,
Gavin

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