| Message ID | 3ff5236944aae69f2cd934b5b6da7c1c269df7c1.1380703262.git.agordeev@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Oct 02, 2013 at 12:48:17PM +0200, Alexander Gordeev wrote: > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> Since you are changing the behavior of the msix_capability_init function on populate_msi_sysfs error, a comment describing why in this commit would be nice. > --- > drivers/pci/msi.c | 11 +++++------ > 1 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index d5f90d6..b43f391 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -719,7 +719,7 @@ static int msix_capability_init(struct pci_dev *dev, > > ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX); > if (ret) > - goto error; > + goto out_avail; > > /* > * Some devices require MSI-X to be enabled before we can touch the > @@ -732,10 +732,8 @@ static int msix_capability_init(struct pci_dev *dev, > msix_program_entries(dev, entries); > > ret = populate_msi_sysfs(dev); > - if (ret) { > - ret = 0; > - goto error; > - } > + if (ret) > + goto out_free; > > /* Set MSI-X enabled bits and unmask the function */ > pci_intx_for_msi(dev, 0); > @@ -746,7 +744,7 @@ static int msix_capability_init(struct pci_dev *dev, > > return 0; > > -error: > +out_avail: > if (ret < 0) { > /* > * If we had some success, report the number of irqs > @@ -763,6 +761,7 @@ error: > ret = avail; > } > > +out_free: > free_msi_irqs(dev); > > return ret; > -- > 1.7.7.6 > -- 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, 2013-10-02 at 17:39 -0700, Jon Mason wrote: > On Wed, Oct 02, 2013 at 12:48:17PM +0200, Alexander Gordeev wrote: > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > > Since you are changing the behavior of the msix_capability_init > function on populate_msi_sysfs error, a comment describing why in this > commit would be nice. [...] This function was already treating that error as fatal, and freeing the MSIs. The change in behaviour is that it now returns the error code in this case, rather than 0. This is obviously correct and properly described by the one-line summary. Ben.
On Thu, Oct 03, 2013 at 10:46:21PM +0100, Ben Hutchings wrote: > On Wed, 2013-10-02 at 17:39 -0700, Jon Mason wrote: > > On Wed, Oct 02, 2013 at 12:48:17PM +0200, Alexander Gordeev wrote: > > > Signed-off-by: Alexander Gordeev <agordeev@redhat.com> > > > > Since you are changing the behavior of the msix_capability_init > > function on populate_msi_sysfs error, a comment describing why in this > > commit would be nice. > [...] > > This function was already treating that error as fatal, and freeing the > MSIs. The change in behaviour is that it now returns the error code in > this case, rather than 0. This is obviously correct and properly > described by the one-line summary. If someone dumb, like me, is looking at this commit and trying to figure out what is happening, having ANY commit message is good. "Fix the return value" doesn't tell me anything. Documenting what issue(s) would've been seen had the error case been encountered and what will now been seen would be very nice. > > Ben. > > -- > Ben Hutchings, Staff Engineer, Solarflare > Not speaking for my employer; that's the marketing department's job. > They asked us to note that Solarflare product names are trademarked. > -- 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/pci/msi.c b/drivers/pci/msi.c index d5f90d6..b43f391 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -719,7 +719,7 @@ static int msix_capability_init(struct pci_dev *dev, ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX); if (ret) - goto error; + goto out_avail; /* * Some devices require MSI-X to be enabled before we can touch the @@ -732,10 +732,8 @@ static int msix_capability_init(struct pci_dev *dev, msix_program_entries(dev, entries); ret = populate_msi_sysfs(dev); - if (ret) { - ret = 0; - goto error; - } + if (ret) + goto out_free; /* Set MSI-X enabled bits and unmask the function */ pci_intx_for_msi(dev, 0); @@ -746,7 +744,7 @@ static int msix_capability_init(struct pci_dev *dev, return 0; -error: +out_avail: if (ret < 0) { /* * If we had some success, report the number of irqs @@ -763,6 +761,7 @@ error: ret = avail; } +out_free: free_msi_irqs(dev); return ret;
Signed-off-by: Alexander Gordeev <agordeev@redhat.com> --- drivers/pci/msi.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)