Re: [PATCH 14/14] scsi: message: fusion: Remove in_interrupt() usage in mptsas_cleanup_fw_event_q().

Daniel Wagner
2020-11-30T16:08:11Z

On Thu, Nov 26, 2020 at 02:29:52PM +0100, Sebastian Andrzej Siewior wrote:
> mptsas_cleanup_fw_event_q() uses in_interrupt() to determine if it is
> safe to cancel a worker item.
>
> Aside of that in_interrupt() is deprecated as it does not provide what the
> name suggests. It covers more than hard/soft interrupt servicing context
> and is semantically ill defined.
>
> Looking closer there are a few problems with the current construct:
> - It could be invoked from an interrupt handler / non-blocking context
> because cancel_delayed_work() has no such restriction. Also,
> mptsas_free_fw_event() has no such restriction.
>
> - The list is accessed unlocked. It may dequeue a valid work-item but at
> the time of invoking cancel_delayed_work() the memory may be released
> or reused because the worker has already run.
>
> mptsas_cleanup_fw_event_q() is invoked via mptsas_shutdown() which is
> always invoked from preemtible context on device shutdown.
> It is also invoked via mptsas_ioc_reset(, MPT_IOC_POST_RESET) which is a
> MptResetHandlers callback. The only caller here are
> mpt_SoftResetHandler(), mpt_HardResetHandler() and
> mpt_Soft_Hard_ResetHandler().

mpt_diag_reset(), iterates over all reset handler...

> All these functions have a `sleepFlag'

...and also uses the same flag.

> argument and each caller uses caller uses `CAN_SLEEP' here and according
> to current documentation:
> | @sleepFlag: Indicates if sleep or schedule must be called
>
> So it is safe to sleep.
>
> Add mptsas_hotplug_event::users member. Initialize it to one by default
> so mptsas_free_fw_event() will free the memory.
> mptsas_cleanup_fw_event_q() will increment its value for items it
> dequeues and then it may keep a pointer after dropping the lock.
> Invoke cancel_delayed_work_sync() to cancel the work item and wait if
> the worker is currently busy. Free the memory afterwards since it owns
> the last reference to it.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Sathya Prakash <sathya.prakash@broadcom.com>
> Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
> Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
> Cc: MPT-FusionLinux.pdl@broadcom.com

Reviewed-by: Daniel Wagner <dwagner@suse.de>