| Message ID | 1547479489-11560-1-git-send-email-wdjjwb@163.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: wd719x Replace GFP_KERNEL with GFP_ATOMIC in wd719x_chip_init | expand |
On Mon, Jan 14, 2019 at 11:24:49PM +0800, wangbo wrote: > wd719x_host_reset get spinlock first then call wd719x_chip_init, > so replace GFP_KERNEL with GFP_ATOMIC in wd719x_chip_init. Please move the allocation outside the lock instead. GFP_ATOMIC DMA allocations are generally a bad idea and should be avoided where we can. More importantly we should never actually trigger the allocation under the lock as far as fw_virt will always be set already in that case. So I think you can safely move the request firmware + allocation + memcpy from wd719x_chip_init to wd719x_board_found, but I'd rather have Ondrej review that plan.
On 2019-01-14 10:29 a.m., Christoph Hellwig wrote: > On Mon, Jan 14, 2019 at 11:24:49PM +0800, wangbo wrote: >> wd719x_host_reset get spinlock first then call wd719x_chip_init, >> so replace GFP_KERNEL with GFP_ATOMIC in wd719x_chip_init. > > Please move the allocation outside the lock instead. GFP_ATOMIC > DMA allocations are generally a bad idea and should be avoided where > we can. > > More importantly we should never actually trigger the allocation > under the lock as far as fw_virt will always be set already > in that case. > > So I think you can safely move the request firmware + allocation > + memcpy from wd719x_chip_init to wd719x_board_found, but I'd rather > have Ondrej review that plan. Further to this, the result of holding a lock (probably with _irqsave() tacked onto it) during a GFP_KERNEL is a message like this in the log: hrtimer: interrupt took 1084 ns It is not always easy to find since it is a "_once" message. The sg v3 driver (the one in production) produces these. I have been able to stamp them out by taking care in the sg v4 driver (in testing) around allocations. It also meant adding a new state in my state machine to fend off "bad things" happening to that object while it is unlocked. So there may be a cost to dropping the lock. Doug Gilbert
diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c index e3310e9..7b1b645 100644 --- a/drivers/scsi/wd719x.c +++ b/drivers/scsi/wd719x.c @@ -318,7 +318,7 @@ static int wd719x_chip_init(struct wd719x *wd) if (!wd->fw_virt) wd->fw_virt = dma_alloc_coherent(&wd->pdev->dev, wd->fw_size, - &wd->fw_phys, GFP_KERNEL); + &wd->fw_phys, GFP_ATOMIC); if (!wd->fw_virt) { ret = -ENOMEM; goto wd719x_init_end;
wd719x_host_reset get spinlock first then call wd719x_chip_init, so replace GFP_KERNEL with GFP_ATOMIC in wd719x_chip_init. Signed-off-by: wangbo <wang.bo116@zte.com.cn> --- drivers/scsi/wd719x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)