Page MenuHomeFreeBSD

aw_mmc: refine locking in aw_mmc_helper_cd_handler()
Needs ReviewPublic

Authored by mhorne on Tue, Feb 4, 8:25 PM.

Details

Reviewers
imp
manu
andrew
Summary

It is invalid to call device_add_child() while holding the driver's
mutex. This was previously of no consequence, but a recent change
to device array allocation (f3d3c63442ff) switched the logic to use
M_WAITOK. Now, a LOR warning is printed when the card is inserted.

Fix this by reducing the scope of the device lock in the helper function
to:

  1. Reading device state (card & child device presence)
  2. Updating device state (after child added/before removed)
Test Plan

The LOR in question during boot:

uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex aw_mmc0 (aw_mmc) r = 0 (0xffffffd000bb1d80) locked @ /usr/home/mitchell/dev/freebsd/sys/arm/allwinner/aw_mmc.c:338
stack backtrace:
#0 0xffffffc00038104e at witness_debugger+0x5c
#1 0xffffffc000382152 at witness_warn+0x3fc
#2 0xffffffc00058daf2 at uma_zalloc_debug+0x28
#3 0xffffffc00058d7c6 at uma_zalloc_arg+0x2a
#4 0xffffffc0002f1a7e at malloc+0x78
#5 0xffffffc0002f22ba at realloc+0x82
#6 0xffffffc0002f23a6 at reallocf+0x16
#7 0xffffffc00034e3d6 at devclass_add_device+0x1a2
#8 0xffffffc00034cb46 at make_device+0xd0
#9 0xffffffc00034c9dc at device_add_child_ordered+0x22
#10 0xffffffc00034c9ae at device_add_child+0x12
#11 0xffffffc0005ecf52 at aw_mmc_helper_cd_handler+0xa6
#12 0xffffffc000128732 at cd_card_task+0x46
#13 0xffffffc0003745bc at taskqueue_run_locked+0x158
#14 0xffffffc00037441c at taskqueue_run+0x50
#15 0xffffffc0003756d0 at taskqueue_swi_giant_run+0x14
#16 0xffffffc0002d9fb4 at ithread_loop+0x22a
#17 0xffffffc0002d6c7a at fork_exit+0x68

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 62258
Build 59142: arc lint + arc unit

Event Timeline

mhorne requested review of this revision.Tue, Feb 4, 8:25 PM

Can aw_mmc_helper_cd_handler be called twice at the same time? If so then it could see present && sc->child == NULL twice, if not then it looks like the locking shouldn't be needed as this is the only function to use sc->child.

sys/arm/allwinner/aw_mmc.c
325

Should be at the start of the function