A few code review comments:
Jayamohan Kallickal wrote:
+static struct bin_attribute sysfs_drvr_fw_prgrm_attr = {
+ .attr = {
+ .name = "be2iscsi_fw_prgrm",
+ .mode = S_IRUSR | S_IWUSR,
+ .owner = THIS_MODULE,
+ },
+ .size = 0,
+ .read = beiscsi_sysfs_fw_rd,
+ .write = beiscsi_sysfs_fw_wr,
+};
+
Use the DEVICE_ATTR macro...
@@ -355,13 +526,32 @@ static irqreturn_t be_isr(int irq, void *dev_id)
static int beiscsi_init_irqs(struct beiscsi_hba *phba)
{
struct pci_dev *pcidev = phba->pcidev;
- int ret;
+ struct hwi_controller *phwi_ctrlr;
+ struct hwi_context_memory *phwi_context;
+ int ret, msix_vec, i = 0;
+ char desc[32];
- ret = request_irq(pcidev->irq, be_isr, IRQF_SHARED, "beiscsi", phba);
- if (ret) {
- shost_printk(KERN_ERR, phba->shost, "beiscsi_init_irqs-"
- "Failed to register irq\\n");
- return ret;
+ phwi_ctrlr = phba->phwi_ctrlr;
+ phwi_context = phwi_ctrlr->phwi_ctxt;
+
+ if (enable_msix) {
This should be checking phba->msix_enabled instead. This code is checking the
module-global variable for whether to attempt MSIX mode. The phba-specific
value must be checked to see if pci_enable_msix was successful on this adapter.
There are lots of other occurrences of the same issue in the cq/eq create
routines and handlers, as well as error paths, etc.
-static int beiscsi_create_eq(struct beiscsi_hba *phba,
+static int beiscsi_create_eqs(struct beiscsi_hba *phba,
struct hwi_context_memory *phwi_context)
{
- unsigned int idx;
- int ret;
+ unsigned int i, num_eq_pages;
+ int ret, eq_for_mcc;
struct be_queue_info *eq;
struct be_dma_mem *mem;
- struct be_mem_descriptor *mem_descr;
void *eq_vaddress;
+ dma_addr_t paddr;
- idx = 0;
- eq = &phwi_context->be_eq.q;
- mem = &eq->dma_mem;
- mem_descr = phba->init_mem;
- mem_descr += HWI_MEM_EQ;
- eq_vaddress = mem_descr->mem_array[idx].virtual_address;
-
- ret = be_fill_queue(eq, phba->params.num_eq_entries,
- sizeof(struct be_eq_entry), eq_vaddress);
- if (ret) {
- shost_printk(KERN_ERR, phba->shost,
- "be_fill_queue Failed for EQ \n");
- return ret;
- }
+ num_eq_pages = PAGES_REQUIRED(phba->params.num_eq_entries * \
+ sizeof(struct be_eq_entry));
- mem->dma = mem_descr->mem_array[idx].bus_address.u.a64.address;
+ if (enable_msix)
+ eq_for_mcc = 1;
+ else
+ eq_for_mcc = 0;
+ for (i = 0; i < (phba->num_cpus + eq_for_mcc); i++) {
+ eq = &phwi_context->be_eq[i].q;
+ mem = &eq->dma_mem;
+ phwi_context->be_eq[i].phba = phba;
+ eq_vaddress = pci_alloc_consistent(phba->pcidev,
+ num_eq_pages * PAGE_SIZE,
+ &paddr);
Missing check for NULL return from pci_alloc_consistent(). There are several
other occurrences of the same issue in the patch.
I see you are no longer applying the "round down" logic for the eq's/cq's.
+static int
+beiscsi_alloc_sysfs_attr(struct Scsi_Host *shost)
+{
+ int ret;
+
+ ret = sysfs_create_bin_file(&shost->shost_dev.kobj,
+ &sysfs_drvr_fw_prgrm_attr);
+ if (ret)
+ shost_printk(KERN_WARNING, shost,
+ "Failed in be2iscsi_alloc_sysfs_attr\n");
+ return ret;
+}
+
+static void beiscsi_free_sysfs_attr(struct Scsi_Host *shost)
+{
+ sysfs_remove_bin_file(&shost->shost_dev.kobj,
+ &sysfs_drvr_fw_prgrm_attr);
+}
This style of creating sysfs attributes is odd for SCSI hosts. Attributes
usually use the DEVICE_ATTR macro, with an attribute array that is set in the
scsi_host_template via the .shost_attrs field.
+static void beiscsi_msix_enable(struct beiscsi_hba *phba)
+{
+ int i, status;
+
+ for (i = 0; i <= phba->num_cpus; i++)
+ phba->msix_entries[i].entry = i;
+
+ status = pci_enable_msix(phba->pcidev, phba->msix_entries,
+ (phba->num_cpus + 1));
+ if (!status)
+ phba->msix_enabled = true;
+
+ return;
+}
+
Here's where you set the per-phba setting for msix enabled or not....
+ if (beiscsi_alloc_sysfs_attr(phba->shost))
+ goto free_attr_mem;
See comment above on sysfs attributes on scsi hosts....
-- james s
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
