On 05/20/2011 02:33 PM, Jayamohan.Kallickal@xxxxxxxxxx wrote:
> Thanks for the patch. Pl see my comments in line
>
>
np.
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 24e20ba..8d71e47 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -874,16 +874,20 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
> struct hwi_controller *phwi_ctrlr;
> struct hwi_context_memory *phwi_context;
> int ret, msix_vec, i, j;
> - char desc[32];
>
> phwi_ctrlr = phba->phwi_ctrlr;
> phwi_context = phwi_ctrlr->phwi_ctxt;
>
> if (phba->msix_enabled) {
> for (i = 0; i < phba->num_cpus; i++) {
> - sprintf(desc, "beiscsi_msix_%04x", i);
> + phba->msi_name[i] = kzalloc(BEISCSI_MSI_NAME,
> + GFP_KERNEL);
> + if (!phba->msi_name[i])
> + goto free_msix_irqs;
> We need to ensure i != 0 before jumping to free_msix_irqs
>
Will fix in next version ... I think I may have to do a bit more work
here because I just realized that if we free_irq() on a non-allocated
irq we'll get an angry message from the kernel ;) Unfortunately this
may complicate the code.... I'll rework this and see if I can come up
with something better.
> + sprintf(phba->msi_name[i], "beiscsi_msix_%04x", i);
> msix_vec = phba->msix_entries[i].vector;
> - ret = request_irq(msix_vec, be_isr_msix, 0, desc,
> + ret = request_irq(msix_vec, be_isr_msix, 0,
> + phba->msi_name[i],
> &phwi_context->be_eq[i]);
> if (ret) {
> shost_printk(KERN_ERR, phba->shost,
> @@ -915,8 +919,10 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
> }
> return 0;
> free_msix_irqs:
> - for (j = i - 1; j == 0; j++)
> + for (j = i - 1; j == 0; j++) {
> free_irq(msix_vec, &phwi_context->be_eq[j]);
> + kfree(phba->msi_name[i]);
> + }
>
... I was looking at this, and I'm confused by it.
Should this really be 'j++'? Or should it be j-- ?
It seems to make sense that this code is j-- ...
> return ret;
> }
>
> @@ -4117,10 +4123,13 @@ static void beiscsi_remove(struct pci_dev *pcidev)
> for (i = 0; i <= phba->num_cpus; i++) {
> msix_vec = phba->msix_entries[i].vector;
> free_irq(msix_vec, &phwi_context->be_eq[i]);
> + kfree(phba->msi_name[i]);
> }
> } else
> - if (phba->pcidev->irq)
> + if (phba->pcidev->irq) {
> free_irq(phba->pcidev->irq, phba);
> + kfree(phba->msi_name[i]);
> Do we need the free for non-msix case as we are not allocation?
>
Fixed in next version.
<snip>
P.
--
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
