| Message ID | 1459533389-19648-1-git-send-email-joe.lawrence@stratus.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2016-04-01 at 13:56 -0400, Joe Lawrence wrote: > The driver's fw events are queued up using the the fw_event_work's > struct work, not its delayed_work member. The latter appears to be > unused and may provoke CONFIG_DEBUG_OBJECTS_TIMERS "assert_init not > available" false warnings in _scsih_fw_event_cleanup_queue. Remove it > and update _scsih_fw_event_cleanup_queue accordingly. > > Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com> > --- > > I think this goes all the way back to the introduction of the mpt3sas > driver. The previous generation mpt2sas driver uses delayed_work, so > perhaps it was simply copied and pasted into the mpt3sas but never > updated. > > drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c > index e0e4920d0fa6..67643602efbc 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c > @@ -189,7 +189,6 @@ struct fw_event_work { > struct list_head list; > struct work_struct work; > u8 cancel_pending_work; > - struct delayed_work delayed_work; > > struct MPT3SAS_ADAPTER *ioc; > u16 device_handle; > @@ -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) 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@lsi.com> Date: Tue Mar 9 16:31:43 2010 +0530 [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. 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. Signed-off-by: Kashyap Desai <kashyap.desai@lsi.com> Reviewed-by: Eric Moore <eric.moore@lsi.com> Signed-off-by: James Bottomley <James.Bottomley@suse.de> Portions of that patch include: @@ -125,11 +127,11 @@ struct sense_info { */ struct fw_event_work { struct list_head list; - struct work_struct work; + u8 cancel_pending_work; + struct delayed_work delayed_work; struct MPT2SAS_ADAPTER *ioc; u8 VF_ID; u8 VP_ID; - u8 host_reset_handling; u8 ignore; u16 event; void *event_data; and @@ -2325,8 +2327,9 @@ _scsih_fw_event_add(struct MPT2SAS_ADAPTER *ioc, struct fw_event_work *fw_event) spin_lock_irqsave(&ioc->fw_event_lock, flags); list_add_tail(&fw_event->list, &ioc->fw_event_list); - INIT_WORK(&fw_event->work, _firmware_event_work); - queue_work(ioc->firmware_event_thread, &fw_event->work); + INIT_DELAYED_WORK(&fw_event->delayed_work, _firmware_event_work); + queue_delayed_work(ioc->firmware_event_thread, + &fw_event->delayed_work, 0); spin_unlock_irqrestore(&ioc->fw_event_lock, flags); } The delay argument to queue_delayed_work() is zero though. So, Broadcom, presumably Joe's fix is the correct fix? -Ewan -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lsi.com> > 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. Regards, -- Joe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lsi.com> > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com] Sent: Friday, April 01, 2016 2:04 PM To: Joe Lawrence Cc: linux-scsi@vger.kernel.org; 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@lsi.com> > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Please consider this patch as Ack-by: Chaitra P B <chaitra.basappa@broadcom.com> Thanks, Chaitra -----Original Message----- From: Sathya Prakash [mailto:sathya.prakash@broadcom.com] Sent: Saturday, April 02, 2016 1:45 AM To: emilne@redhat.com; Joe Lawrence Cc: linux-scsi@vger.kernel.org; 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@redhat.com] Sent: Friday, April 01, 2016 2:04 PM To: Joe Lawrence Cc: linux-scsi@vger.kernel.org; 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@lsi.com> > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@broadcom.com> > > Thanks, > Chaitra > > -----Original Message----- > From: Sathya Prakash [mailto:sathya.prakash@broadcom.com] > Sent: Saturday, April 02, 2016 1:45 AM > To: emilne@redhat.com; Joe Lawrence > Cc: linux-scsi@vger.kernel.org; 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@redhat.com] > Sent: Friday, April 01, 2016 2:04 PM > To: Joe Lawrence > Cc: linux-scsi@vger.kernel.org; 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@lsi.com> >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "Joe" == Joe Lawrence <joe.lawrence@stratus.com> writes:
Joe> Do we know why f1c35e6aea579 "mpt2sas: RESCAN Barrier work is added
Joe> in case of HBA reset" was unneeded for the mpt3 version? If that
Joe> is interesting, that info could be added to v2 commit message as
Joe> well.
Chaitra?
Joe , The below mentioned patch is an older patch, verified latest code also the code before merging(mpt2sas & mpt3sas) didn't find changes of below patch. So I am searching for the patch which has removed the functionality/changes of the below patch. I shall get back to you on this by Monday. Thanks, Chaitra -----Original Message----- From: Martin K. Petersen [mailto:martin.petersen@oracle.com] Sent: Friday, April 15, 2016 8:13 AM To: Joe Lawrence Cc: Chaitra Basappa; Sathya Prakash Veerichetty; emilne@redhat.com; linux-scsi@vger.kernel.org; Suganath Prabu Subramani; Calvin Owens Subject: Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work >>>>> "Joe" == Joe Lawrence <joe.lawrence@stratus.com> writes: Joe> Do we know why f1c35e6aea579 "mpt2sas: RESCAN Barrier work is added Joe> in case of HBA reset" was unneeded for the mpt3 version? If that Joe> is interesting, that info could be added to v2 commit message as Joe> well. Chaitra?
Chaitra, I think you are looking for the patch "mpt2sas: [Resend] Host Reset code cleanup". Joe, The initial drivers for SAS2 controller's handle firmware reset using the rescan barrier and later we redesigned it through the above patch. I hope it clarifies, please let us know if you are looking for additional details? Thanks Sathya -----Original Message----- From: Chaitra Basappa [mailto:chaitra.basappa@broadcom.com] Sent: Friday, April 15, 2016 3:58 AM To: Martin K. Petersen; Joe Lawrence Cc: Sathya Prakash Veerichetty; emilne@redhat.com; linux-scsi@vger.kernel.org; Suganath Prabu Subramani; Calvin Owens Subject: RE: [PATCH] mpt3sas - remove unused fw_event_work delayed_work Joe , The below mentioned patch is an older patch, verified latest code also the code before merging(mpt2sas & mpt3sas) didn't find changes of below patch. So I am searching for the patch which has removed the functionality/changes of the below patch. I shall get back to you on this by Monday. Thanks, Chaitra -----Original Message----- From: Martin K. Petersen [mailto:martin.petersen@oracle.com] Sent: Friday, April 15, 2016 8:13 AM To: Joe Lawrence Cc: Chaitra Basappa; Sathya Prakash Veerichetty; emilne@redhat.com; linux-scsi@vger.kernel.org; Suganath Prabu Subramani; Calvin Owens Subject: Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work >>>>> "Joe" == Joe Lawrence <joe.lawrence@stratus.com> writes: Joe> Do we know why f1c35e6aea579 "mpt2sas: RESCAN Barrier work is added Joe> in case of HBA reset" was unneeded for the mpt3 version? If that Joe> is interesting, that info could be added to v2 commit message as Joe> well. Chaitra?
diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c index e0e4920d0fa6..67643602efbc 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c @@ -189,7 +189,6 @@ struct fw_event_work { struct list_head list; struct work_struct work; u8 cancel_pending_work; - struct delayed_work delayed_work; struct MPT3SAS_ADAPTER *ioc; u16 device_handle; @@ -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);
The driver's fw events are queued up using the the fw_event_work's struct work, not its delayed_work member. The latter appears to be unused and may provoke CONFIG_DEBUG_OBJECTS_TIMERS "assert_init not available" false warnings in _scsih_fw_event_cleanup_queue. Remove it and update _scsih_fw_event_cleanup_queue accordingly. Signed-off-by: Joe Lawrence <joe.lawrence@stratus.com> --- I think this goes all the way back to the introduction of the mpt3sas driver. The previous generation mpt2sas driver uses delayed_work, so perhaps it was simply copied and pasted into the mpt3sas but never updated. drivers/scsi/mpt3sas/mpt3sas_scsih.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)