Messages in this thread | | | Subject | Re: [PATCH v2 04/18] nitro_enclaves: Init PCI device driver | From | "Paraschiv, Andra-Irina" <> | Date | Mon, 25 May 2020 14:15:02 +0300 |
| |
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.
| |