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.ke...@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 ) > > -- Ondrej Zary
