On 24/05/2020 09:32, Greg KH wrote:
On Sat, May 23, 2020 at 10:25:25PM +0200, Alexander Graf wrote:
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 ...
Same for WARN_ON when you run with panic-on-warn enabled :(
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 ...
For static calls where you control the callers, don't do checks like
this. Otherwise the kernel would just be full of these all over the
place and things would slow down. It's just not needed.
+ 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.
Think through these as the driver seems to ONLY use these ratelimited
calls right now, which is not correct.
Also, if a user can create a printk, that almost always is not a good
idea. But yes, those should be ratelimited.
I updated the static calls checks and removed the WARN_ONs. And
ratelimited is used now only in the ioctl call paths.
Thank you both.
Andra
Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
