Messages in this thread | | | Subject | Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver | From | Alexander Graf <> | Date | Sat, 23 May 2020 22:25:25 +0200 |
| |
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
| |