On Thu, Dec 03, 2020 at 11:10:09AM +0100, Thomas Lamprecht wrote:
> > The be_fill_queue() function can only fail when "eq_vaddress" is NULL
> > and since it's non-NULL here that means the function call can't fail.
> > But imagine if it could, then in that situation we would want to store
> > the "paddr" so that dma memory can be released.
> >
> > Fixes: bfead3b2cb46 ("[SCSI] be2iscsi: Adding msix and mcc_rings V3")
> > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> This came in here through the stable 5.4 tree with v5.4.74, and we have some
> users of ours report that it results in kernel oopses and delayed boot on their
> HP DL 380 Gen 9 (and other Gen 9, FWICT) servers:
>
Thanks for the report Thomas. I see the bug in my patch:
drivers/scsi/be2iscsi/be_main.c
3008 eq_for_mcc = 1;
3009 else
3010 eq_for_mcc = 0;
3011 for (i = 0; i < (phba->num_cpus + eq_for_mcc); i++) {
3012 eq = &phwi_context->be_eq[i].q;
3013 mem = &eq->dma_mem;
3014 phwi_context->be_eq[i].phba = phba;
3015 eq_vaddress = dma_alloc_coherent(&phba->pcidev->dev,
3016 num_eq_pages * PAGE_SIZE,
3017 &paddr, GFP_KERNEL);
3018 if (!eq_vaddress) {
3019 ret = -ENOMEM;
3020 goto create_eq_error;
3021 }
3022
3023 mem->dma = paddr;
^^^^^^^^^^^^^^^^
I moved this assignment ahead of the call to be_fill_queue().
3024 mem->va = eq_vaddress;
3025 ret = be_fill_queue(eq, phba->params.num_eq_entries,
3026 sizeof(struct be_eq_entry), eq_vaddress);
3027 if (ret) {
3028 beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
3029 "BM_%d : be_fill_queue Failed for EQ\n");
3030 goto create_eq_error;
3031 }
drivers/scsi/be2iscsi/be_main.c
2978 static int be_fill_queue(struct be_queue_info *q,
2979 u16 len, u16 entry_size, void *vaddress)
2980 {
2981 struct be_dma_mem *mem = &q->dma_mem;
2982
2983 memset(q, 0, sizeof(*q));
^^^^^^^^^^^^^^^^^^^^^^^
But the first thing that it does is it overwrites it with zeros.
2984 q->len = len;
2985 q->entry_size = entry_size;
2986 mem->size = len * entry_size;
2987 mem->va = vaddress;
It also overwrites the "mem->va = eq_vaddress;" assignment as well, but
but it sets that back again here...
2988 if (!mem->va)
2989 return -ENOMEM;
2990 memset(mem->va, 0, mem->size);
2991 return 0;
2992 }
I will just revert my patch. This code is messy but it works so far as
I can see.
regards,
dan carpenter
