| Message ID | 20190529175851.GA10760@hari-Inspiron-1545 (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] wd719x: pass GFP_ATOMIC instead of GFP_KERNEL | expand |
On Wed, 29 May 2019, Hariprasad Kelam wrote: > dont acquire lock before calling wd719x_chip_init. > > Issue identified by coccicheck > > Signed-off-by: Hariprasad Kelam <hariprasad.kelam@gmail.com> > ----- > changes in v1: Replace GFP_KERNEL with GFP_ATOMIC. > changes in v2: Call wd719x_chip_init without lock as suggested > in review Why was host_lock taken here initially? I assume it's to protect some race in init that leads to an undefined state.
On Wed, May 29, 2019 at 02:13:18PM -0700, David Rientjes wrote: > On Wed, 29 May 2019, Hariprasad Kelam wrote: > > > dont acquire lock before calling wd719x_chip_init. > > > > Issue identified by coccicheck > > > > Signed-off-by: Hariprasad Kelam <hariprasad.kelam@gmail.com> > > ----- > > changes in v1: Replace GFP_KERNEL with GFP_ATOMIC. > > changes in v2: Call wd719x_chip_init without lock as suggested > > in review > > Why was host_lock taken here initially? I assume it's to protect some > race in init that leads to an undefined state. wd719x_chip_init is getting called from wd719x_host_reset and wd719x_board_found. In wd719x_board_found case its not acquiring any lock. In wd719x_host_reset it is called under spin_lock. Acquiring spin_lock in wd719x_host_reset is there from initial commit so its better we wont remove this lock. I think we Patch v1 is correct fix(pass GFP_ATOMIC instead of GPF_KERNEL )
On Thursday 30 May 2019 20:10:44 Hariprasad Kelam wrote: > On Wed, May 29, 2019 at 02:13:18PM -0700, David Rientjes wrote: > > On Wed, 29 May 2019, Hariprasad Kelam wrote: > > > > > dont acquire lock before calling wd719x_chip_init. > > > > > > Issue identified by coccicheck > > > > > > Signed-off-by: Hariprasad Kelam <hariprasad.kelam@gmail.com> > > > ----- > > > changes in v1: Replace GFP_KERNEL with GFP_ATOMIC. > > > changes in v2: Call wd719x_chip_init without lock as suggested > > > in review > > > > Why was host_lock taken here initially? I assume it's to protect some > > race in init that leads to an undefined state. > > wd719x_chip_init is getting called from wd719x_host_reset > and wd719x_board_found. > > In wd719x_board_found case its not acquiring any lock. > In wd719x_host_reset it is called under spin_lock. > > Acquiring spin_lock in wd719x_host_reset is there from initial commit > so its better we wont remove this lock. Looks like I haven't tested it properly before. Host reset is broken - sg_reset -N -H /dev/sdX will oops: wd719x 0000:02:01.0: host reset requested ------------[ cut here ]------------ kernel BUG at fs/buffer.c:1218! invalid opcode: 0000 [#1] SMP CPU: 0 PID: 1913 Comm: sg_reset Not tainted 5.1.0+ #323 Hardware name: /848P-ICH5, BIOS 6.00 PG 02/03/2005 EIP: check_irqs_on+0xb/0xf ... That's because of request_firmware called under the spin lock, as Christoph pointed out. Patch v2 is also wrong - wd719x_finish_cmd must be called under lock. I'm currently testing a proper fix (disable chip and flush SCBs under lock, then initialize chip without lock). It works mostly but I can crash it easily by doing a device reset under load (e.g. dd from a SCSI drive) and then doing a host reset - NULL pointer dereference in list_del. > I think we Patch v1 is correct fix(pass GFP_ATOMIC instead of GPF_KERNEL ) > >
diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c index c2f4006..340ec92 100644 --- a/drivers/scsi/wd719x.c +++ b/drivers/scsi/wd719x.c @@ -505,11 +505,9 @@ static int wd719x_host_reset(struct scsi_cmnd *cmd) { struct wd719x *wd = shost_priv(cmd->device->host); struct wd719x_scb *scb, *tmp; - unsigned long flags; int result; dev_info(&wd->pdev->dev, "host reset requested\n"); - spin_lock_irqsave(wd->sh->host_lock, flags); /* Try to reinit the RISC */ if (wd719x_chip_init(wd) == 0) result = SUCCESS; @@ -519,7 +517,6 @@ static int wd719x_host_reset(struct scsi_cmnd *cmd) /* flush all SCBs */ list_for_each_entry_safe(scb, tmp, &wd->active_scbs, list) wd719x_finish_cmd(scb, result); - spin_unlock_irqrestore(wd->sh->host_lock, flags); return result; }
dont acquire lock before calling wd719x_chip_init. Issue identified by coccicheck Signed-off-by: Hariprasad Kelam <hariprasad.kelam@gmail.com> ----- changes in v1: Replace GFP_KERNEL with GFP_ATOMIC. changes in v2: Call wd719x_chip_init without lock as suggested in review ---- --- drivers/scsi/wd719x.c | 3 --- 1 file changed, 3 deletions(-)