| Message ID | b0a9f6f455aa03b7769e6d9cc2e7fdbc06732b2f.1380703263.git.agordeev@redhat.com |
|---|---|
| State | RFC |
| Headers | show |
On Wed, 2 Oct 2013 12:49:02 +0200 Alexander Gordeev <agordeev@redhat.com> wrote: NACK. This change does not do anything logically as far as I can tell. pci_enable_msix in the current upstream kernel itself calls pci_msix_table_size. The current code yields the same results as the code suggested below. (i.e., the suggested code has no effect on optimality). BTW, pci_msix_table_size never returns a value < 0 (if msix is not enabled, it returns 0 for the table size), so the (err < 0) check here is not correct. (I also do not like using "err" here anyway for the value returned by pci_msix_table_size(). There is no error here, and it is simply confusing. -Jack > As result of recent re-design of the MSI/MSI-X interrupts enabling > pattern this driver has to be updated to use the new technique to > obtain a optimal number of MSI/MSI-X interrupts required. > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > --- > drivers/net/ethernet/mellanox/mlx4/main.c | 17 ++++++++--------- > 1 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c > b/drivers/net/ethernet/mellanox/mlx4/main.c index 60c9f4f..377a5ea > 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c > @@ -1852,8 +1852,16 @@ static void mlx4_enable_msi_x(struct mlx4_dev > *dev) int i; > > if (msi_x) { > + err = pci_msix_table_size(dev->pdev); > + if (err < 0) > + goto no_msi; > + > + /* Try if at least 2 vectors are available */ > nreq = min_t(int, dev->caps.num_eqs - > dev->caps.reserved_eqs, nreq); > + nreq = min_t(int, nreq, err); > + if (nreq < 2) > + goto no_msi; > > entries = kcalloc(nreq, sizeof *entries, GFP_KERNEL); > if (!entries) > @@ -1862,17 +1870,8 @@ static void mlx4_enable_msi_x(struct mlx4_dev > *dev) for (i = 0; i < nreq; ++i) > entries[i].entry = i; > > - retry: > err = pci_enable_msix(dev->pdev, entries, nreq); > if (err) { > - /* Try again if at least 2 vectors are > available */ > - if (err > 1) { > - mlx4_info(dev, "Requested %d > vectors, " > - "but only %d MSI-X vectors > available, " > - "trying again\n", nreq, > err); > - nreq = err; > - goto retry; > - } > kfree(entries); > goto no_msi; > } -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2 Oct 2013 12:49:02 +0200 Alexander Gordeev <agordeev@redhat.com> wrote: UPDATING THIS REPLY. Your change log confused me. The change below is not from a "recent re-design", it is required due to an earlier patch in this patch set. From the log, I assumed that the change you are talking about is already upstream. I will re-review. -Jack NACK. This change does not do anything logically as far as I can tell. pci_enable_msix in the current upstream kernel itself calls pci_msix_table_size. The current code yields the same resultswill as the code suggested below. (i.e., the suggested code has no effect on optimality). BTW, pci_msix_table_size never returns a value < 0 (if msix is not enabled, it returns 0 for the table size), so the (err < 0) check here is not correct. (I also do not like using "err" here anyway for the value returned by pci_msix_table_size(). There is no error here, and it is simply confusing. -Jack > As result of recent re-design of the MSI/MSI-X interrupts enabling > pattern this driver has to be updated to use the new technique to > obtain a optimal number of MSI/MSI-X interrupts required. > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > --- > drivers/net/ethernet/mellanox/mlx4/main.c | 17 ++++++++--------- > 1 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c > b/drivers/net/ethernet/mellanox/mlx4/main.c index 60c9f4f..377a5ea > 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c > @@ -1852,8 +1852,16 @@ static void mlx4_enable_msi_x(struct mlx4_dev > *dev) int i; > > if (msi_x) { > + err = pci_msix_table_size(dev->pdev); > + if (err < 0) > + goto no_msi; > + > + /* Try if at least 2 vectors are available */ > nreq = min_t(int, dev->caps.num_eqs - > dev->caps.reserved_eqs, nreq); > + nreq = min_t(int, nreq, err); > + if (nreq < 2) > + goto no_msi; > > entries = kcalloc(nreq, sizeof *entries, GFP_KERNEL); > if (!entries) > @@ -1862,17 +1870,8 @@ static void mlx4_enable_msi_x(struct mlx4_dev > *dev) for (i = 0; i < nreq; ++i) > entries[i].entry = i; > > - retry: > err = pci_enable_msix(dev->pdev, entries, nreq); > if (err) { > - /* Try again if at least 2 vectors are > available */ > - if (err > 1) { > - mlx4_info(dev, "Requested %d > vectors, " > - "but only %d MSI-X vectors > available, " > - "trying again\n", nreq, > err); > - nreq = err; > - goto retry; > - } > kfree(entries); > goto no_msi; > } -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2 Oct 2013 12:49:02 +0200 Alexander Gordeev <agordeev@redhat.com> wrote: > As result of recent re-design of the MSI/MSI-X interrupts enabling > pattern this driver has to be updated to use the new technique to > obtain a optimal number of MSI/MSI-X interrupts required. > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> New review -- ACK (i.e., patch is OK), subject to acceptance of patches 05 and 07 of this patch set. I sent my previous review (NACK) when I was not yet aware that changes proposed were due to the two earlier patches (mentioned above) in the current patch set. The change log here should actually read something like the following: As a result of changes to the MSI/MSI_X enabling procedures, this driver must be modified in order to preserve its current msi/msi_x enablement logic. -Jack -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c index 60c9f4f..377a5ea 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c +++ b/drivers/net/ethernet/mellanox/mlx4/main.c @@ -1852,8 +1852,16 @@ static void mlx4_enable_msi_x(struct mlx4_dev *dev) int i; if (msi_x) { + err = pci_msix_table_size(dev->pdev); + if (err < 0) + goto no_msi; + + /* Try if at least 2 vectors are available */ nreq = min_t(int, dev->caps.num_eqs - dev->caps.reserved_eqs, nreq); + nreq = min_t(int, nreq, err); + if (nreq < 2) + goto no_msi; entries = kcalloc(nreq, sizeof *entries, GFP_KERNEL); if (!entries) @@ -1862,17 +1870,8 @@ static void mlx4_enable_msi_x(struct mlx4_dev *dev) for (i = 0; i < nreq; ++i) entries[i].entry = i; - retry: err = pci_enable_msix(dev->pdev, entries, nreq); if (err) { - /* Try again if at least 2 vectors are available */ - if (err > 1) { - mlx4_info(dev, "Requested %d vectors, " - "but only %d MSI-X vectors available, " - "trying again\n", nreq, err); - nreq = err; - goto retry; - } kfree(entries); goto no_msi; }
As result of recent re-design of the MSI/MSI-X interrupts enabling pattern this driver has to be updated to use the new technique to obtain a optimal number of MSI/MSI-X interrupts required. Signed-off-by: Alexander Gordeev <agordeev@redhat.com> --- drivers/net/ethernet/mellanox/mlx4/main.c | 17 ++++++++--------- 1 files changed, 8 insertions(+), 9 deletions(-)