diff mbox series

[-next] scsi: efct: Use GFP_ATOMIC under spin lock

Message ID 20211221113706.329791-1-yangyingliang@huawei.com (mailing list archive)
State Changes Requested
Headers show
Series [-next] scsi: efct: Use GFP_ATOMIC under spin lock | expand

Commit Message

Yang Yingliang Dec. 21, 2021, 11:37 a.m. UTC
A spin lock is taken here so we should use GFP_ATOMIC.

Fixes: efac162a4e4d ("scsi: efct: Don't pass GFP_DMA to dma_alloc_coherent()")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/scsi/elx/libefc/efc_els.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Christoph Hellwig Dec. 21, 2021, 2:28 p.m. UTC | #1
On Tue, Dec 21, 2021 at 07:37:06PM +0800, Yang Yingliang wrote:
> A spin lock is taken here so we should use GFP_ATOMIC.
> 
> Fixes: efac162a4e4d ("scsi: efct: Don't pass GFP_DMA to dma_alloc_coherent()")

No, it does not fix that commit.  The driver did sleeping allocations
even before the commit.

But wher is "here"?  Can we look into not holding that lock over an
allocation if it is preferable?  If not we should at least pass down
the gfp_flags so that only the caller(s) that can't sleep pass GFP_ATOMIC.
Yang Yingliang Dec. 23, 2021, 3:56 a.m. UTC | #2
On 2021/12/21 22:28, Christoph Hellwig wrote:
> On Tue, Dec 21, 2021 at 07:37:06PM +0800, Yang Yingliang wrote:
>> A spin lock is taken here so we should use GFP_ATOMIC.
>>
>> Fixes: efac162a4e4d ("scsi: efct: Don't pass GFP_DMA to dma_alloc_coherent()")
> No, it does not fix that commit.  The driver did sleeping allocations
> even before the commit.
>
> But wher is "here"?  Can we look into not holding that lock over an
> allocation if it is preferable?  If not we should at least pass down
> the gfp_flags so that only the caller(s) that can't sleep pass GFP_ATOMIC.

According the comment of els_ios_lock, it's used to protect els ios 
list, I think we

can move down the spin lock like this:


--- a/drivers/scsi/elx/libefc/efc_els.c
+++ b/drivers/scsi/elx/libefc/efc_els.c
@@ -46,8 +46,6 @@ efc_els_io_alloc_size(struct efc_node *node, u32 
reqlen, u32 rsplen)

         efc = node->efc;

-       spin_lock_irqsave(&node->els_ios_lock, flags);
-
         if (!node->els_io_enabled) {
                 efc_log_err(efc, "els io alloc disabled\n");
                 spin_unlock_irqrestore(&node->els_ios_lock, flags);
@@ -88,6 +86,8 @@ efc_els_io_alloc_size(struct efc_node *node, u32 
reqlen, u32 rsplen)
                 els = NULL;
         }

+       spin_lock_irqsave(&node->els_ios_lock, flags);
+
         if (els) {
                 /* initialize fields */
                 els->els_retries_remaining = EFC_FC_ELS_DEFAULT_RETRIES;
Christoph Hellwig Jan. 10, 2022, 8:54 a.m. UTC | #3
On Thu, Dec 23, 2021 at 11:56:08AM +0800, Yang Yingliang wrote:
>
> On 2021/12/21 22:28, Christoph Hellwig wrote:
>> On Tue, Dec 21, 2021 at 07:37:06PM +0800, Yang Yingliang wrote:
>>> A spin lock is taken here so we should use GFP_ATOMIC.
>>>
>>> Fixes: efac162a4e4d ("scsi: efct: Don't pass GFP_DMA to dma_alloc_coherent()")
>> No, it does not fix that commit.  The driver did sleeping allocations
>> even before the commit.
>>
>> But wher is "here"?  Can we look into not holding that lock over an
>> allocation if it is preferable?  If not we should at least pass down
>> the gfp_flags so that only the caller(s) that can't sleep pass GFP_ATOMIC.
>
> According the comment of els_ios_lock, it's used to protect els ios list, I 
> think we
>
> can move down the spin lock like this:

This looks sensible to me.  Please submit it to the maintainer as a proper
patch.
diff mbox series

Patch

diff --git a/drivers/scsi/elx/libefc/efc_els.c b/drivers/scsi/elx/libefc/efc_els.c
index 7bb4f9aad2c8..7043a61d553d 100644
--- a/drivers/scsi/elx/libefc/efc_els.c
+++ b/drivers/scsi/elx/libefc/efc_els.c
@@ -71,7 +71,7 @@  efc_els_io_alloc_size(struct efc_node *node, u32 reqlen, u32 rsplen)
 	/* now allocate DMA for request and response */
 	els->io.req.size = reqlen;
 	els->io.req.virt = dma_alloc_coherent(&efc->pci->dev, els->io.req.size,
-					      &els->io.req.phys, GFP_KERNEL);
+					      &els->io.req.phys, GFP_ATOMIC);
 	if (!els->io.req.virt) {
 		mempool_free(els, efc->els_io_pool);
 		spin_unlock_irqrestore(&node->els_ios_lock, flags);
@@ -80,7 +80,7 @@  efc_els_io_alloc_size(struct efc_node *node, u32 reqlen, u32 rsplen)
 
 	els->io.rsp.size = rsplen;
 	els->io.rsp.virt = dma_alloc_coherent(&efc->pci->dev, els->io.rsp.size,
-					      &els->io.rsp.phys, GFP_KERNEL);
+					      &els->io.rsp.phys, GFP_ATOMIC);
 	if (!els->io.rsp.virt) {
 		dma_free_coherent(&efc->pci->dev, els->io.req.size,
 				  els->io.req.virt, els->io.req.phys);