Hi Chaitra, while discussing this patch with Ewan, I realized that
cancel_pending_work is unused as well. I can send a v2 with that and
the associated kerneldoc update.
Do we know why f1c35e6aea579 "mpt2sas: RESCAN Barrier work is added in
case of HBA reset" was unneeded for the mpt3 version? If that is
interesting, that info could be added to v2 commit message as well.
Thanks,
-- Joe
On 04/11/2016 07:13 AM, Chaitra Basappa wrote:
Hi,
Please consider this patch as Ack-by: Chaitra P B
<chaitra.basappa@xxxxxxxxxxxx>
Thanks,
Chaitra
-----Original Message-----
From: Sathya Prakash [mailto:sathya.prakash@xxxxxxxxxxxx]
Sent: Saturday, April 02, 2016 1:45 AM
To: emilne@xxxxxxxxxx; Joe Lawrence
Cc: linux-scsi@xxxxxxxxxxxxxxx; Chaitra Basappa; Suganath Prabu Subramani;
Calvin Owens
Subject: RE: [PATCH] mpt3sas - remove unused fw_event_work delayed_work
We will look into this early next week and provide a detailed response. On
the first look this is ACK from Broadcom, will reconfirm.
-----Original Message-----
From: Ewan D. Milne [mailto:emilne@xxxxxxxxxx]
Sent: Friday, April 01, 2016 2:04 PM
To: Joe Lawrence
Cc: linux-scsi@xxxxxxxxxxxxxxx; Sathya Prakash; Chaitra P B; Suganath Prabu
Subramani; Calvin Owens
Subject: Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work
On Fri, 2016-04-01 at 15:13 -0400, Joe Lawrence wrote:
On 04/01/2016 02:51 PM, Ewan D. Milne wrote:
On Fri, 2016-04-01 at 13:56 -0400, Joe Lawrence wrote:
@@ -2804,12 +2803,12 @@ _scsih_fw_event_cleanup_queue(struct
MPT3SAS_ADAPTER *ioc)
/*
* Wait on the fw_event to complete. If this returns 1, then
* the event was never executed, and we need a put for the
- * reference the delayed_work had on the fw_event.
+ * reference the work had on the fw_event.
*
* If it did execute, we wait for it to finish, and the put will
* happen from _firmware_event_work()
*/
- if (cancel_delayed_work_sync(&fw_event->delayed_work))
+ if (cancel_work_sync(&fw_event->work))
fw_event_work_put(fw_event);
fw_event_work_put(fw_event);
Hmm... Fixes: 146b16c8 (mpt3sas: Refcount fw_events and fix unsafe
list usage)
This could technically go back to f92363d12359 (mpt3sas: add new
driver supporting 12GB SAS) ... but will probably only apply cleanly
to _scsih_fw_event_cleanup_queue after 146b16c8 (mpt3sas: Refcount
fw_events and fix unsafe list usage), so you're right.
Since mpt3sas uses ->work instead of _delayed_work this would seem
to be correct, however the deprecated mpt2sas driver had a commit
that changed the firmware event work mechanism to use ->delayed_work
instead of ->work:
commit f1c35e6aea579d5bdb6dc02dfa99c67c7c3b3f67
Author: Kashyap, Desai <kashyap.desai@xxxxxxx>
Date: Tue Mar 9 16:31:43 2010 +0530
Okay, so this is pre-mpt3sas split.
[SCSI] mpt2sas: RESCAN Barrier work is added in case of HBA reset.
Add the cancel_pending_work flag from the fw_event_work
structure, and then to
set the flag during host reset, check the flag later from work
threads
context and if cancel_pending_work_flag is set ingore those events.
More unused mpt2 vestiges in the mpt3 version?
% cd drivers/scsi/mpt3sas/
% grep 'cancel_pending_work' *.{c,h}
mpt3sas_scsih.c: * @cancel_pending_work: flag set during reset handling
mpt3sas_scsih.c: u8 cancel_pending_work;
Now Rescan after host reset is changed.
Added special task MPT2SAS_RESCAN_AFTER_HOST_RESET. This task
will be queued
at the time of HBA reset. this task is treated as barrier. All
work after
MPT2SAS_RESCAN_AFTER_HOST_RESET will be treated as new work and
will be
server by callback handle. If host_recovery is going on while
running RESCAN
task, it will wait for shos_recovery_done completion which will
be called
from HBA reset DONE context.
FWIW, I don't see anything like this in today's mpt3sas driver.
Well, that's the question. Is there some functionality missing? Were the
changes abandoned/replaced? mpt2sas used delayed_work for something else,
so maybe that's why the firmware event changes initially used it (albeit
with a 0 delay) but it's hard to know...
cancel_delayed_work() will call del_timer() on delayed_work->timer, but it
looks like kzalloc is used to allocate the fw_event_work objects so perhaps
nothing bad will happen. I was wondering, though, because I have seen dumps
of hung systems with requests that should have timed out but are not on any
timer list.
-Ewan
Regards,
-- Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
