On 2019-06-06 12:09 p.m., Russell King - ARM Linux admin wrote: >> @@ -1466,6 +1467,11 @@ static void sfp_sm_mod_remove(struct sfp *sfp) >> static void sfp_sm_event(struct sfp *sfp, unsigned int event) >> { >> mutex_lock(&sfp->sm_mutex); >> + if (unlikely(sfp->shutdown)) { >> + /* Do not handle any more state machine events. */ >> + mutex_unlock(&sfp->sm_mutex); >> + return; >> + } >> >> dev_dbg(sfp->dev, "SM: enter %s:%s:%s event %s\n", >> mod_state_to_str(sfp->sm_mod_state), >> @@ -1704,6 +1710,13 @@ static void sfp_check_state(struct sfp *sfp) >> { >> unsigned int state, i, changed; >> >> + mutex_lock(&sfp->sm_mutex); >> + if (unlikely(sfp->shutdown)) { >> + /* No more state checks */ >> + mutex_unlock(&sfp->sm_mutex); >> + return; >> + } >> + > > I don't think you need to add the mutex locking - just check for > sfp->shutdown and be done with it...
The idea there was to deal with the case where GPIO interrupts were previously raised before shutdown and not yet handled by the threaded interrupt handler by the time shutdown is called. After shutdown on the SFP completes, the bus the GPIO stuff is on could potentially be shut down at any moment, so we really don't want to be digging into the GPIO states after that. Locking the mutex there ensures that we don't read a stale value for the shutdown flag in the interrupt handler, since AFAIK there's no other synchronization around that value. It may also be helpful that the lock is now held for the subsequent code in sfp_check_state that's comparing the previous and new states - it seems like you could otherwise run into trouble if that function was being concurrently called from the polling thread and the interrupt handler (for example if you had an SFP where some GPIOs supported interrupts and some didn't). > >> +static void sfp_shutdown(struct platform_device *pdev) >> +{ >> + struct sfp *sfp = platform_get_drvdata(pdev); >> + >> + mutex_lock(&sfp->sm_mutex); >> + sfp->shutdown = true; >> + mutex_unlock(&sfp->sm_mutex); >> + >> + cancel_delayed_work_sync(&sfp->poll); >> + cancel_delayed_work_sync(&sfp->timeout); > > Since the work cancellation will ensure that the works are not running > at the point they return, and should they then run again, they'll hit > the sfp->shutdown condition. > >> +} >> + >> static struct platform_driver sfp_driver = { >> .probe = sfp_probe, >> .remove = sfp_remove, >> + .shutdown = sfp_shutdown, >> .driver = { >> .name = "sfp", >> .of_match_table = sfp_of_match, >> -- >> 1.8.3.1 >> >> > -- Robert Hancock Senior Software Developer SED Systems, a division of Calian Ltd. Email: hanc...@sedsystems.ca