Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5156132d-f55b-8d1a-1783-f82733400753@linaro.org>
Date:   Mon, 24 Oct 2022 19:13:53 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH] net: ipa: don't configure IDLE_INDICATION on v3.1
Content-Language: en-US
To:     Caleb Connolly <caleb.connolly@linaro.org>,
        Alex Elder <elder@linaro.org>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jami Kettunen <jami.kettunen@somainline.org>
Cc:     Alex Elder <elder@kernel.org>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
References: <20221024234850.4049778-1-caleb.connolly@linaro.org>
From:   Alex Elder <elder@linaro.org>
In-Reply-To: <20221024234850.4049778-1-caleb.connolly@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/24/22 6:48 PM, Caleb Connolly wrote:
> IPA v3.1 doesn't support the IDLE_INDICATION_CFG register, this was
> causing a harmless splat in ipa_idle_indication_cfg(), add a version
> check to prevent trying to fetch this register on v3.1
> 
> Fixes: 6a244b75cfab ("net: ipa: introduce ipa_reg()")

Actually, the bug first appeared long ago.  This commit:
   1bb1a117878b9 ("net: ipa: add IPA v3.1 configuration data")
marked IPA v3.1 supported.  But it did not update the code
to avoid accessing the IDLE_INDICATION register for IPA v3.1
(in ipa_reg_idle_indication_cfg_offset()).  That being said,
we have no evidence that it caused harm, and until we do I'd
rather not try to fix the problem that far back.

The commit you point out is the one where we actually
start checking (and WARNing), and I think it's reasonable
to say that's what this fixes.

> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
> This will need to wait for Jami's Tested-by as I don't have any v3.1 hardware.

I would very much like to get a Tested-by tag.

But from my perspective, this patch looks good.  Previously
offset 0x220 was used, even though it was not well-defined
for IPA v3.1.

This is a bug fix destined for the net/master branch.

Reviewed-by: Alex Elder <elder@linaro.org>

Thank you.

> ---
>   drivers/net/ipa/ipa_main.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 3461ad3029ab..49537fccf6ad 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -434,6 +434,9 @@ static void ipa_idle_indication_cfg(struct ipa *ipa,
>   	const struct ipa_reg *reg;
>   	u32 val;
>   
> +	if (ipa->version < IPA_VERSION_3_5_1)
> +		return;
> +
>   	reg = ipa_reg(ipa, IDLE_INDICATION_CFG);
>   	val = ipa_reg_encode(reg, ENTER_IDLE_DEBOUNCE_THRESH,
>   			     enter_idle_debounce_thresh);