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;
Date:   Fri, 25 Nov 2022 14:12:21 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To:     Akihiko Odaki <akihiko.odaki@daynix.com>
CC:     <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
        <intel-wired-lan@lists.osuosl.org>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Tony Nguyen <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "Yan Vugenfirer" <yan@daynix.com>,
        Yuri Benditovich <yuri.benditovich@daynix.com>
Subject: Re: [PATCH v3] igb: Allocate MSI-X vector when testing
Message-ID: <Y4C/NZEDxH00z/hT@boxer>
References: <20221123010926.7924-1-akihiko.odaki@daynix.com>
 <Y34/LDxCnYd6VGJ2@boxer>
 <4a2d4e3e-5b15-2c58-dc49-92908ab80ad0@daynix.com>
 <1434bd67-1707-7e43-96b6-d2294ff7f04d@daynix.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1434bd67-1707-7e43-96b6-d2294ff7f04d@daynix.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?geyhBr02xqR3m8FxsfyPBqd6xHUp7etIQVCmfatyVdNZNG+ia1DRCN7KYE?=
 =?iso-8859-1?Q?bNK87LgFMHRf0NrIzEEwlPERd3Ly8kfrRlQrRAchQXG6Fcf6TmqrHT96Nb?=
 =?iso-8859-1?Q?hx2DhZMQz6qkuUMPyAVNui06eEDrfPNyjSMWnBJCgePwbh3Lki5eAxOuNg?=
 =?iso-8859-1?Q?32DMBYV+7L6S65DC4ewBXPAlqNoTzDCKRHORCfXkj/3mpZq29bHcrPvsL6?=
 =?iso-8859-1?Q?DUzfeMAQu7qI/FQgzEEn/bivaX9xgqjk3HGNRMv+V2CC2V6GqyUkmmCMFk?=
 =?iso-8859-1?Q?l5qS698+znx465kK+XjOpy/uJZ+H4OIunQ9B3kfqc48yW/QX7iXqDlsIe4?=
 =?iso-8859-1?Q?uwlqkZiJ8W/YwXwk3xEbOTpADsWaZjUBdyusOzjs1CA1mLxVm/1wtuWCil?=
 =?iso-8859-1?Q?miCFM3nvMYFX33ro0YPaOhUCp0gAxSY9pjMh0gMHBHSn21G3md5IebCctH?=
 =?iso-8859-1?Q?3v89lAzOget9vN/jvgkL2paHbpazcJRhUNKo3LIKmdIsdDT5cRmN1Zii4U?=
 =?iso-8859-1?Q?deV0Y770Fo1M6JzmpI0emNG72MY8wR0qQpOMsFr3VWOqprQ0j3EXBd0gaA?=
 =?iso-8859-1?Q?lCs5J+5ieW5r/zr5h4wctQkkNaG8BYlPjnSNnj4sOP3bBIXkN4xl6AX82w?=
 =?iso-8859-1?Q?JNqksmlZ/Jc5URLQXiTWghgtPf72LC0ZLigYleH4IxUxmEgSQFCQr5gSiY?=
 =?iso-8859-1?Q?C26sW+mFStxAH76Sw8enmIJnOu2+a7nGYiZ0M7NpHJ25AjLDzQ+2TlfjPY?=
 =?iso-8859-1?Q?xyx/c24sz5xYU6zA9EzIWlHVposQ9Rr2rOKORfgfdCjYr77l67l36THwX1?=
 =?iso-8859-1?Q?anYpHKU+SWK/9ugDjT/2qCpK0jVIjiJiNLbs8OVpfzDazig3AGneFXVDuU?=
 =?iso-8859-1?Q?ZmQ1IPIubM2iEYSG/NhygVPXBzM5I4bsJzIncWQRcAuxjBjbq1Oy4ob3f9?=
 =?iso-8859-1?Q?HoYNY6b43Y54Kiz0pwIz/4E2vb9nKZZWpcNX4D7ySCQ6Qs2mlNb4EOgyz6?=
 =?iso-8859-1?Q?x2Ve/745Twwgekfyik/f9Pbsmc93F3o1jgl/3yHZzW17ERsUhhxr5ovHAg?=
 =?iso-8859-1?Q?Pb+wVBI5NR0FvK3pzKhG8/HIdTk5lfoPMQWLZeHR44RUE4K5xqm2WzdWja?=
 =?iso-8859-1?Q?w6X9g9n/XaSJnFn07MBV85TXPVeDWz71T/A5rgjG0mI99bF6G5nTWDTKx2?=
 =?iso-8859-1?Q?7KMkzVpbWjCe2TeJAOqPNAQ5iOwY2t0Hm5uHrr2ZYgDb8PeqLSG4l/tufy?=
 =?iso-8859-1?Q?0yAWZ4UiaW7ag71Y0ue98iDBbMGgOd4v3CF/Hv3mVYtrOIlYSm+JopvWd8?=
 =?iso-8859-1?Q?HAF8cPZ54Ulf7fGvjyWZ2giiy1E+dovZIUbavibX+e7cajbGUGEQcwZAAf?=
 =?iso-8859-1?Q?hc9yQtlopymDD7ctOJ0E8A3vqmfBuS+392mcL/1wkcAmlI1mhy/FvtmlTM?=
 =?iso-8859-1?Q?oV8NihKl6t0cxTYXsK5SnGPgmgdATQjeFAwAO9cLbpk+nPzk2uvER9VFtY?=
 =?iso-8859-1?Q?JdBNlb1Y+ihYTfUev1ZBIkauCG4uOWw0nNL3TMmmybZ4AOrfc9oLYrMegR?=
 =?iso-8859-1?Q?F0e7+gA2zQKzk3SrR1LOEKm/L02DRXZWAraQS8JvXdlSmziCg+050UwbUw?=
 =?iso-8859-1?Q?dtDN+1oRS1oJPV1tPq/4UOK9neyZVs/oA3ktMC5vdf6UCIzkTHmO45ug?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 1101e3b4-e323-42b6-220c-08dacee6b7f4
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6117.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Nov 2022 13:12:35.1782
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: DtbfaGZPqb9rG96T/4pqdPXGwoHxc7W5rGH1GX6nKnb8oK/4xfBNMGkCz89RHX5k3/IlokT97CuQDL1PY1ozdtuFWje8sReZ0GArJYsuSjM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5259
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Nov 25, 2022 at 06:50:45PM +0900, Akihiko Odaki wrote:
> On 2022/11/25 18:48, Akihiko Odaki wrote:
> > 
> > 
> > On 2022/11/24 0:41, Maciej Fijalkowski wrote:
> > > On Wed, Nov 23, 2022 at 10:09:26AM +0900, Akihiko Odaki wrote:
> > > > Without this change, the interrupt test fail with MSI-X environment:
> > > > 
> > > > $ sudo ethtool -t enp0s2 offline
> > > > [?? 43.921783] igb 0000:00:02.0: offline testing starting
> > > > [?? 44.855824] igb 0000:00:02.0 enp0s2: igb: enp0s2 NIC Link is Down
> > > > [?? 44.961249] igb 0000:00:02.0 enp0s2: igb: enp0s2 NIC Link is
> > > > Up 1000 Mbps Full Duplex, Flow Control: RX/TX
> > > > [?? 51.272202] igb 0000:00:02.0: testing shared interrupt
> > > > [?? 56.996975] igb 0000:00:02.0 enp0s2: igb: enp0s2 NIC Link is
> > > > Up 1000 Mbps Full Duplex, Flow Control: RX/TX
> > > > The test result is FAIL
> > > > The test extra info:
> > > > Register test? (offline)???? 0
> > > > Eeprom test??? (offline)???? 0
> > > > Interrupt test (offline)???? 4
> > > > Loopback test? (offline)???? 0
> > > > Link test?? (on/offline)???? 0
> > > > 
> > > > Here, "4" means an expected interrupt was not delivered.
> > > > 
> > > > To fix this, route IRQs correctly to the first MSI-X vector by setting
> > > > IVAR_MISC. Also, set bit 0 of EIMS so that the vector will not be
> > > > masked. The interrupt test now runs properly with this change:
> > > 
> > > Much better!
> > > 
> > > > 
> > > > $ sudo ethtool -t enp0s2 offline
> > > > [?? 42.762985] igb 0000:00:02.0: offline testing starting
> > > > [?? 50.141967] igb 0000:00:02.0: testing shared interrupt
> > > > [?? 56.163957] igb 0000:00:02.0 enp0s2: igb: enp0s2 NIC Link is
> > > > Up 1000 Mbps Full Duplex, Flow Control: RX/TX
> > > > The test result is PASS
> > > > The test extra info:
> > > > Register test? (offline)???? 0
> > > > Eeprom test??? (offline)???? 0
> > > > Interrupt test (offline)???? 0
> > > > Loopback test? (offline)???? 0
> > > > Link test?? (on/offline)???? 0
> > > > 
> > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > 
> > > Same comment as on other patch - justify why there is no fixes tag and
> > > specify the tree in subject.
> > 
> > I couldn't identify what commit introduced the problem. Please see:
> > https://lore.kernel.org/netdev/f2457229-865a-57a0-94a1-c5c63b2f30a5@daynix.com/
> 
> Sorry, the URL was wrong. The correct URL is:
> https://lore.kernel.org/netdev/be5617fe-d332-447a-b836-bec9a6c6d42d@daynix.com/

Please change the subject to:
[PATCH net v4] igb: Allocate MSI-X vector when testing

and add n the body
Fixes: 4eefa8f01314 ("igb: add single vector msi-x testing to interrupt test")

Also, it is a good practice to include changes between revisions even if
it is only a rewrite of a commit message.

> 
> Regards,
> Akihiko Odaki
> 
> > 
> > Regards,
> > Akihiko Odaki
> > 
> > > 
> > > > ---
> > > > ? drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++
> > > > ? 1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > > b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > > index e5f3e7680dc6..ff911af16a4b 100644
> > > > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> > > > @@ -1413,6 +1413,8 @@ static int igb_intr_test(struct
> > > > igb_adapter *adapter, u64 *data)
> > > > ????????????? *data = 1;
> > > > ????????????? return -1;
> > > > ????????? }
> > > > +??????? wr32(E1000_IVAR_MISC, E1000_IVAR_VALID << 8);
> > > > +??????? wr32(E1000_EIMS, BIT(0));

Should these registers be cleared at the end of igb_intr_test?

> > > > ????? } else if (adapter->flags & IGB_FLAG_HAS_MSI) {
> > > > ????????? shared_int = false;
> > > > ????????? if (request_irq(irq,
> > > > -- 
> > > > 2.38.1
> > > >