[PATCH] mpt3sas - remove unused fw_event_work delayed_work

[PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Joe Lawrence]

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);
-- 
1.8.3.1

Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Laurence Oberman]

Looks fine to me.

Reviewed-by: Laurence Oberman <loberman@redhat.com>

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Joe Lawrence" <joe.lawrence@stratus.com>
To: linux-scsi@vger.kernel.org
Cc: "Sathya Prakash" <sathya.prakash@broadcom.com>, "Chaitra P B" <chaitra.basappa@broadcom.com>, "Suganath Prabu Subramani" <suganath-prabu.subramani@broadcom.com>, "Calvin Owens" <calvinowens@fb.com>, "Joe Lawrence" <joe.lawrence@stratus.com>
Sent: Friday, April 1, 2016 1:56:29 PM
Subject: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

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);
-- 
1.8.3.1

--
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

Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Ewan D. Milne]

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

Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Joe Lawrence]

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

Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Ewan D. Milne]

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

RE: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Sathya Prakash]

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

RE: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Chaitra Basappa]

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

Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Joe Lawrence]

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

Re: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Martin K. Petersen]

>>>>> "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?

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Chaitra Basappa]

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?

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH] mpt3sas - remove unused fw_event_work delayed_work

[Sathya Prakash Veerichetty]

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?

-- 
Martin K. Petersen	Oracle Linux Engineering