| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 24 Mar 2009 12:05:41 +0100
From: Rolf Eike Beer <eike-kernel@...tec.de>
To: Krishna Gudipati <kgudipat@...cade.com>
Cc: James.Bottomley@...senpartnership.com, huangj@...cade.com,
linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org,
rvadivel@...cade.com, vravindr@...cade.com
Subject: Re: [PATCH 1/5] bfa: Brocade BFA FC SCSI driver (bfad)
Krishna Gudipati wrote:
> From: Krishna Chaitanya Gudipati <kgudipat@...cade.com>
>
> This patch contains Brocade linux driver specific code that interfaces to
> upper layer linux kernel for PCI, SCSI mid-layer, sysfs related stuff. The
> code handles things such as module load/unload, PCI probe/release, handling
> interrupts, interfacing with sysfs etc. It interacts with the
> firmware/hardware via the code under files with prefix bfa_*.
>
> Signed-off-by: Krishna Chaitanya Gudipati <kgudipat@...cade.com>
> +int
> +bfad_pci_init(struct pci_dev *pdev, struct bfad_s *bfad)
> +{
> + unsigned long bar0_len;
> + int rc = -ENODEV;
> +
> + if (pci_enable_device(pdev)) {
> + BFA_PRINTF(BFA_ERR, "pci_enable_device fail %p\n", pdev);
> + goto out;
> + }
> +
> + if (pci_request_regions(pdev, BFAD_DRIVER_NAME))
> + goto out_disable_device;
> +
> + pci_set_master(pdev);
> +
> +
> + if (pci_set_dma_mask(pdev, DMA_64BIT_MASK) != 0)
> + if (pci_set_dma_mask(pdev, DMA_32BIT_MASK) != 0) {
> + BFA_PRINTF(BFA_ERR, "pci_set_dma_mask fail %p\n",
> pdev); + goto out_release_region;
> + }
Save the return value of pci_set_dma_mask() to rc and use this as error code.
> +#ifdef SUPPORT_PCI_AER
> + /*
> + * Enable PCIE Advanced Error Recovery (AER) if the kernel version
> + * supports.
> + */
> + BFAD_ENABLE_PCIE_AER(pdev);
> +#endif
> +
> + bfad->pci_bar0_map = pci_resource_start(pdev, 0);
> + bar0_len = pci_resource_len(pdev, 0);
> + bfad->pci_bar0_kva = ioremap(bfad->pci_bar0_map, bar0_len);
bfad->pci_bar0_kva = pci_iomap(pdev, 0, 0);
> + if (bfad->pci_bar0_kva == NULL) {
> + BFA_PRINTF(BFA_ERR, "Fail to map bar0\n");
> + goto out_release_region;
> + }
> +
> + bfad->hal_pcidev.pci_slot = PCI_SLOT(pdev->devfn);
> + bfad->hal_pcidev.pci_func = PCI_FUNC(pdev->devfn);
> + bfad->hal_pcidev.pci_bar_kva = bfad->pci_bar0_kva;
> + bfad->hal_pcidev.device_id = pdev->device;
> + bfad->pci_name = pci_name(pdev);
> +
> + bfad->pci_attr.vendor_id = pdev->vendor;
> + bfad->pci_attr.device_id = pdev->device;
> + bfad->pci_attr.ssid = pdev->subsystem_device;
> + bfad->pci_attr.ssvid = pdev->subsystem_vendor;
> + bfad->pci_attr.pcifn = PCI_FUNC(pdev->devfn);
> +
> + bfad->pcidev = pdev;
> + return 0;
> +
> +out_release_region:
> + pci_release_regions(pdev);
> +out_disable_device:
> + pci_disable_device(pdev);
> +out:
> + return rc;
> +}
What about using devres for your driver? (See
Documentation/driver-model/devres.txt) This would take care of the
release_regions and disable device in the error paths here and also in the
remove handler.
> +/**
> + * bfa_isr BFA driver interrupt functions
> + */
> +irqreturn_t bfad_intx(int irq, void *dev_id);
This declaration isn't needed at all as the function follows directly after
that.
> +/**
> + * Line based interrupt handler.
> + */
> +irqreturn_t
> +bfad_intx(int irq, void *dev_id)
> +{
[...]
> +static int
> +bfad_install_msix_handler(struct bfad_s *bfad)
> +{
> + int i, error = 0;
^^^^^^^^^^^^^
One space.
> + for (i = 0; i < bfad->nvec; i++) {
> + error = request_irq(bfad->msix_tab[i].msix.vector,
> + bfad->msix_tab[i].handler, 0,
> + BFAD_DRIVER_NAME, bfad);
> + printk(KERN_WARNING "%s: bfad%d irq %d\n", __FUNCTION__,
> + bfad->inst_no, bfad->msix_tab[i].msix.vector);
> +
> + if (error) {
> + int j;
> +
> + for (j = 0; j < i; j++)
> + free_irq(bfad->msix_tab[j].msix.vector, bfad);
> +
> + return 1;
"return error" would allow doint something useful with this value later (e.g.
printing them in your warning) so one can actually see what went wrong.
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * Setup MSIX based interrupt.
> + */
> +int
> +bfad_setup_intr(struct bfad_s *bfad)
> +{
> + int error = 0;
> +
> + if (!msix_disable) {
> + u32 mask = 0, i, num_bit = 0, max_bit = 0;
> + struct msix_entry msix_entries[MAX_MSIX_ENTRY];
> +
> + /* Call BFA to get the msix map for this PCI function. */
> + bfa_msix_getvecs(&bfad->bfa, &mask, &num_bit, &max_bit);
> +
> + /* Set up the msix entry table */
> + bfad_init_msix_entry(bfad, msix_entries, mask, max_bit);
> +
> + error = pci_enable_msix(bfad->pcidev, msix_entries, bfad->nvec);
> + if (error) {
> + /*
> + * Only error number of vector is available.
"of vectors are"
> + * We don't have a mechanism to map multiple
> + * interrupts into one vector, so even if we
> + * can try to request less vectors, we don't
> + * know how to associate interrupt events to
> + * vectors. Linux doesn't dupicate vectors
> + * in the MSIX table for this case.
> + */
> +
> + printk(KERN_WARNING
> + "%s: enable_msix failed, %d bfad%d\n",
> + __FUNCTION__, error, bfad->inst_no);
Also the message should really be more descriptive, like "enable_msix for %i
vectors failed, only %i vectors available". And if you use dev_warn() that
function will add the correct device description for you (You should use it
whereever possible).
__FUNCTION__ should not be used anymore, use __func__. Or remove it
completely, it's rather obvious where this message actually comes from.
> +
> + goto line_based;
> + }
> +
> + /* Save the vectors */
> + for (i = 0; i < bfad->nvec; i++)
> + bfad->msix_tab[i].msix.vector = msix_entries[i].vector;
> +
> + /* Set up interrupt handler for each vectors */
> + if (bfad_install_msix_handler(bfad)) {
> + printk(KERN_WARNING "%s: install_msix failed, bfad%d\n",
> + __FUNCTION__, bfad->inst_no);
> + goto line_based;
> + }
This is just your choice, but when MSI was requested and the number of
interrupts were reserved successfully and now installing the IRQ handler
fails this should IMHO be an error and not a situation to fallback to line
based. YMMV.
> + bfad->bfad_flags |= BFAD_MSIX_ON;
> +
> + goto success;
> + }
> +
> +line_based:
> + error = 0;
> + if (request_irq
> + (bfad->pcidev->irq, (irq_handler_t) bfad_intx, BFAD_IRQ_FLAGS,
Cast not needed, bfad_intx() should be of the correct signature.
> + BFAD_DRIVER_NAME, bfad) != 0) {
> + /* Enable interrupt handler failed */
> + return 1;
> + }
> +success:
> + return error;
> +}
> +
> +void
> +bfad_remove_intr(struct bfad_s *bfad)
> +{
> + int i;
> +
> + if (bfad->bfad_flags & BFAD_MSIX_ON) {
> + for (i = 0; i < bfad->nvec; i++)
> + free_irq(bfad->msix_tab[i].msix.vector, bfad);
> +
> + pci_disable_msix(bfad->pcidev);
> + bfad->bfad_flags &= ~BFAD_MSIX_ON;
> + } else {
> + free_irq(bfad->pcidev->irq, bfad);
> + }
> +}
> +
> +#else /* CONFIG_PCI_MSI */
> +/**
> + * Setup line-based interrupt.
> + */
> +int
> +bfad_setup_intr(struct bfad_s *bfad)
> +{
> + if (request_irq
> + (bfad->pcidev->irq, (irq_handler_t) bfad_intx, SA_SHIRQ,
Same here.
> + BFAD_DRIVER_NAME, bfad) != 0) {
> + /* Enable interrupt handler failed */
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +void
> +bfad_remove_intr(struct bfad_s *bfad)
> +{
> + bfa_trc(bfad, bfad->pcidev->irq);
> + free_irq(bfad->pcidev->irq, bfad);
> +}
> +
> +#endif /* CONFIG_PCI_MSI */
> +
> +
Remove trailing empty lines.
Greetings,
Eike
Download attachment "signature.asc " of type "application/pgp-signature" (198 bytes)
Powered by blists - more mailing lists