Hey Greg,
On 22.05.20 09:04, Greg KH wrote:
On Fri, May 22, 2020 at 09:29:32AM +0300, Andra Paraschiv wrote:
+/**
+ * ne_setup_msix - Setup MSI-X vectors for the PCI device.
+ *
+ * @pdev: PCI device to setup the MSI-X for.
+ *
+ * @returns: 0 on success, negative return value on failure.
+ */
+static int ne_setup_msix(struct pci_dev *pdev)
+{
+ struct ne_pci_dev *ne_pci_dev = NULL;
+ int nr_vecs = 0;
+ int rc = -EINVAL;
+
+ if (WARN_ON(!pdev))
+ return -EINVAL;
How can this ever happen? If it can not, don't test for it. If it can,
don't warn for it as that will crash systems that do panic-on-warn, just
test and return an error.
I think the point here is to catch situations that should never happen,
but keep a sanity check in in case they do happen. This would've usually
been a BUG_ON, but people tend to dislike those these days because they
can bring down your system ...
So in this particular case here I agree that it's a bit silly to check
whether pdev is != NULL. In other device code internal APIs though it's
not quite as clear of a cut. I by far prefer code that tells me it's
broken over reverse engineering stray pointer accesses ...
+
+ ne_pci_dev = pci_get_drvdata(pdev);
+ if (WARN_ON(!ne_pci_dev))
+ return -EINVAL;
Same here, don't use WARN_ON if at all possible.
+
+ nr_vecs = pci_msix_vec_count(pdev);
+ if (nr_vecs < 0) {
+ rc = nr_vecs;
+
+ dev_err_ratelimited(&pdev->dev,
+ NE "Error in getting vec count [rc=%d]\n",
+ rc);
+
Why ratelimited, can this happen over and over and over?
In this particular function, no, so here it really should just be
dev_err. Other functions are implicitly callable from user space through
an ioctl, which means they really need to stay rate limited.
Thanks a lot for looking through the code and pointing all those bits out :)
Alex
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
