Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp2162908ybl; Thu, 30 Jan 2020 12:28:28 -0800 (PST) X-Google-Smtp-Source: APXvYqzLzDvwx2DBQJEatekyG9xziI5ONVRx1VD7KfrlYdK/jLgwhAUxsQkfEswZSrF9GWy3Z/ox X-Received: by 2002:a05:6830:4cd:: with SMTP id s13mr4842396otd.181.1580416108507; Thu, 30 Jan 2020 12:28:28 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1580416108; cv=none; d=google.com; s=arc-20160816; b=CePakiTUHoe9YYoaVe0iZxYw9dTgfNG6SiDRur0XKMIqLV5AInV1leN/+Yb/rMI0iA yGdrInu61zEtBLFbRkMDom7l62qI/6WOKJcymuTwsPLW5p25dEwwkTZaZ57eBobI1X7g mM4/DLbr0YjRz8FP3p0JQfx+gKy1JkG6LmdpD484ePuD/XA3v7h3YnKfjefXidi0rmBC +HGIoa/NVBycl2gZT9A4dVzdnBEv5xBGZAdgsUxg5zrh5vDk8vi4uvXyz9AC3qmAYQFe Stu6R/05NqkO89Ph6MyJRdMZkHNYOPpH8PWemRcgQ8NB+xBzvxb9ENhQgiwwj/Yv4ccz RRXQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=mMmbTPRv5n9fcnTR4JD/Hiobk7HR6X/mODzSmDElFRk=; b=gb/5cRUSgIanFdrvs2yrVRMTKWz3e5UDvTbDceNsmlXQuwb5lp3aa44laNlZJ7zDqn lW/W+bXEeA15xOG6zOT9TEw7Oubi8EThvAE3jpN52WWzkl24+0b3Xx1l48xZi1/NkxLu an2kU2u5T/dcgMlCC3RAFE20Rq46hkZkkODHRWl6AYH46fb7+ESpKS065wzNldLQjAFQ qkndo3jqy4L63US3fFtUjgiXLLhnmG8L9INVbCwKD1RZJI33faQkWW1AuRFcxzNYbFn+ CEkQXBkWSzYnFxvE+KwfCnCO5LPEi79I6ApH4KoT0r9s77slR54S1wTaE5Nv53vPKAnh e1rw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gateworks-com.20150623.gappssmtp.com header.s=20150623 header.b=UJG07q0Z; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id g4si2269870oib.152.2020.01.30.12.28.14; Thu, 30 Jan 2020 12:28:28 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gateworks-com.20150623.gappssmtp.com header.s=20150623 header.b=UJG07q0Z; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727240AbgA3U0N (ORCPT + 99 others); Thu, 30 Jan 2020 15:26:13 -0500 Received: from mail-il1-f194.google.com ([209.85.166.194]:45246 "EHLO mail-il1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726514AbgA3U0M (ORCPT ); Thu, 30 Jan 2020 15:26:12 -0500 Received: by mail-il1-f194.google.com with SMTP id p8so4184692iln.12 for ; Thu, 30 Jan 2020 12:26:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gateworks-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mMmbTPRv5n9fcnTR4JD/Hiobk7HR6X/mODzSmDElFRk=; b=UJG07q0ZvllEgVwuAoevdh4W2PsMLK8OB4w5jNu08HE+oqaV0dPCzf6G0s28627iHX MAeM0+1hO7Q2rcZRlzA757D7sD7JXrirx/PgZPiMwHhbj9oInkKx0RKNdrD7wlxxP3HN 8H8GRgU6guwk3DDN9gxR7XFi8L9Fu+IuylCY9CCcIViFiuQnch7A4mOtzRzlLvDD64xC 5EM4anhFUfbutFIJc+/Kq+JL6zrAp6xok25sEwrImK0K1jVsjqykH1fnKHssrE6VaZ74 ujMrhqsKK0AHG3KYqLz2HkDkWvBm5V4I3H+jqJzqGlGiLwL5z60horRKkm5RSuYM1SLV IzzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mMmbTPRv5n9fcnTR4JD/Hiobk7HR6X/mODzSmDElFRk=; b=fBYKE3+lqinxsPNgHWrftN/mjrl5orkvJn61TQtpoklds8x0l08sTRPak9PJA/IX+F 1J9dDCEWBKT1Les2xYt2X9UPKWtKN6gWIgUgn9dg+ke4mY3qNkI80cAbKVPdQoyWndK+ P1Rsk3Q42eqqplGl+kG3q6wYkq0AO99fCYjI5gRSt6MHBo4Iw/kH7Y/ste+Fqr+oqWRL yjruBzw57AZELicrTOJG5WxXsZ5ZMD/PtdT2qPhbro5yC3LLMg+rjfX2Sn8weQ1bwXhu hIuDuVvuPFeeK2lJSz0ZSZzmleZMJqAjDRPbXpGt2Up4cqG9nuHThjb8FPXfhylRtX4U V+qw== X-Gm-Message-State: APjAAAXYUYgPGApUGHE/519ukOkkW0nIfzWLsxnhB+jWXlqTZz7NkqU2 TXnsLrCr/XM2ZGkTFBRKbTFek5cKxnQjI7clNtim6w== X-Received: by 2002:a92:3a8d:: with SMTP id i13mr6549486ilf.112.1580415971935; Thu, 30 Jan 2020 12:26:11 -0800 (PST) MIME-Version: 1.0 References: <20200129223609.9327-1-rjones@gateworks.com> <20200130091055.159d63ed@cakuba> In-Reply-To: <20200130091055.159d63ed@cakuba> From: Bobby Jones Date: Thu, 30 Jan 2020 12:26:01 -0800 Message-ID: Subject: Re: [PATCH net] net: thunderx: workaround BGX TX Underflow issue To: Jakub Kicinski Cc: Sunil Goutham , Robert Richter , David Miller , "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" , netdev@vger.kernel.org, linux-kernel , Tim Harvey Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 30, 2020 at 9:10 AM Jakub Kicinski wrote: > > On Wed, 29 Jan 2020 14:36:09 -0800, Robert Jones wrote: > > From: Tim Harvey > > > > While it is not yet understood why a TX underflow can easily occur > > for SGMII interfaces resulting in a TX wedge. It has been found that > > disabling/re-enabling the LMAC resolves the issue. > > > > Signed-off-by: Tim Harvey > > Reviewed-by: Robert Jones > > Sunil or Robert (i.e. one of the maintainers) will have to review this > patch (as indicated by Dave by marking it with "Needs Review / ACK" in > patchwork). > > At a quick look there are some things which jump out at me: > > > +static int bgx_register_intr(struct pci_dev *pdev) > > +{ > > + struct bgx *bgx = pci_get_drvdata(pdev); > > + struct device *dev = &pdev->dev; > > + int num_vec, ret; > > + > > + /* Enable MSI-X */ > > + num_vec = pci_msix_vec_count(pdev); > > + ret = pci_alloc_irq_vectors(pdev, num_vec, num_vec, PCI_IRQ_MSIX); > > + if (ret < 0) { > > + dev_err(dev, "Req for #%d msix vectors failed\n", num_vec); > > + return 1; > > Please propagate real error codes, or make this function void as the > caller never actually checks the return value. > > > + } > > + sprintf(bgx->irq_name, "BGX%d", bgx->bgx_id); > > + ret = request_irq(pci_irq_vector(pdev, GMPX_GMI_TX_INT), > > There is a alloc_irq and request_irq call added in this patch but there > is never any freeing. Are you sure this is fine? Devices can be > reprobed (unbound and bound to drivers via sysfs). I agree there needs to be accompanying free calls. I'm referencing drivers/net/ethernet/cavium/thunder/nic_main.c and see instances of both pci_free_irq_vectors() and free_irq(). My initial thought was that I should use pci_free_irq_vectors() in the error check conditional of the above request irq and also in the bgx_remove() function. Would that be appropriate in this case? I'd also plan on using a conditional like this for the free calls: if (bgx->irq_name) pci_free_irq_vectors(pdev); I'm new to kernel development so suggestions are welcome. > > > + bgx_intr_handler, 0, bgx->irq_name, bgx); > > Please align the continuation line with the opening bracket (checkpatch > --strict should help catch this). > > > + if (ret) > > + return 1; > > + > > + return 0; > > +}