mpt fusion: error recovery improvements, and synchronizing internal commands

3) [email protected] - fix's provided from SGI after testing with single threaded internal commands

What fixes?

The object of a change log is to preserve the history of a particular change (as per the Developer Certificate of Origin). This means if you
get a patch from someone, you should also collect their signoff with it,
then you pass it on to me complete with your signoff added (and a note if you had to modify it to fit it into the patch series or otherwise alter it).

James

Quoted 11 lines · James Bottomley

What fixes?

The object of a change log is to preserve the history of a particular change (as per the Developer Certificate of Origin). This means if you
get a patch from someone, you should also collect their signoff with it,
then you pass it on to me complete with your signoff added (and a note if you had to modify it to fit it into the patch series or otherwise alter it).

Quoted 4 lines

James

James, Mike sent me an email back on June 13, contain 11 suggested changes associated with his test efforts with my new code having rewrote internal command error handling, and adding new polling function for
controller fault handling. The changes he quoted associated to the
ones in mptfc seem vague. I have the email I could forward, but at that time, It probably doesnt matter.

1) cleanup ioc_reset callback handlers, introducing wrappers for synronizing error recovery (mpt_set_taskmgmt_in_progress_flag, mpt_clear_taskmgmt_in_progress_flag), as the fusion firmware only handles one task management request at a time.
2) set vtarget->deleted flag when devices have been removed.
3) [email protected] - fix's provided from SGI after testing with single threaded internal commands

OK, I thought I'd wait here for the breakout, but in the meantime I tried to compile the first five patches, but they don't:

CC [M] drivers/message/fusion/mptscsih.o
drivers/message/fusion/mptscsih.c: In function 'mptscsih_qcmd':
drivers/message/fusion/mptscsih.c:1357: error: 'MPT_SCSI_HOST' has no member named 'resetPending' drivers/message/fusion/mptscsih.c: In function 'mptscsih_TMHandler':
drivers/message/fusion/mptscsih.c:1554: error: 'MPT_ADAPTER' has no member named 'diagLock'
...

The series can't be applied in this form, because if git bisect steps into the middle of this, the compile will break (and hence the bisection) and a lot of people will say a lot of nasty things.

A patch series really needs to be one patch per logical change (with other peoples' patches broken out) in a form that is separately compilable for each patch. Additionally, with a separate change log and
summary (i.e. not four patches all saying "mpt fusion: error recovery improvements, and synchronizing internal commands").

I'll back all of this out; can you resend the series conforming to the above request?

Quoted 4 lines · James Bottomley

OK, I thought I'd wait here for the breakout, but in the meantime I tried to compile the first five patches, but they don't:

Quoted 8 lines

CC [M] drivers/message/fusion/mptscsih.o
drivers/message/fusion/mptscsih.c: In function 'mptscsih_qcmd':
drivers/message/fusion/mptscsih.c:1357: error: 'MPT_SCSI_HOST' has no member named 'resetPending' drivers/message/fusion/mptscsih.c: In function 'mptscsih_TMHandler':
drivers/message/fusion/mptscsih.c:1554: error: 'MPT_ADAPTER' has no member named 'diagLock'
...

Its not going to compile if you didn't pull down all 6 patchs associated
to "error recovery improvements". Those six patchs were divided by
sources. Meaning patch 4 was mptbase.[h,c] changes, then patch5 was mptctl.[c,h] changes, and so on. So the changes assoicated for
mptscsich.c are in patch 8. Since you didn't apply the 8th patch,
offcourse your going to get the errrors in mptscish.c. All patchs 4-9
need to go togeather, as mentioned in the first patch 0.

Quoted 6 lines

The series can't be applied in this form, because if git bisect steps into the middle of this, the compile will break (and hence the bisection) and a lot of people will say a lot of nasty things.

Quoted 7 lines

A patch series really needs to be one patch per logical change (with other peoples' patches broken out) in a form that is separately compilable for each patch. Additionally, with a separate change log and
summary (i.e. not four patches all saying "mpt fusion: error recovery improvements, and synchronizing internal commands").

I understand, and thats been going on for the past couple months,
starting with Sathya's patchs early August. The changes associated
with the rewrite of internal commands, and errror recovery is rather difficult to seperate due to many depandancies and new structure changes, and my concerned risk of what small steps could introduce more issues, so IMO, the least risk was jump starting you to the customer and factory tested code.

Quoted 3 lines

I'll back all of this out; can you resend the series conforming to the above request?

The first three patchs did conform that, which are:

1) adding usage of shost_priv and removing all the typecasting
2) Fixing sparse warnings
3) locking down ScsiLookup

Youve rejected the error recovery patchs, which is fair enough.

Quoted 1 line

Youve rejected the error recovery patchs, which is fair enough.

Just the separation ... the actual patch looks OK.

Quoted 3 lines · James Bottomley

Quoted 1 line

Youve rejected the error recovery patchs, which is fair enough.

Just the separation ... the actual patch looks OK.

I'll will continue working to separate the "error recovery
improvements:" into smaller feature add, but will take some time. I
want some constructive feedback, and too big of patch is deterring some people from looking at it.

Quoted 6 lines · Eric

Quoted 3 lines · James Bottomley

Quoted 1 line

Youve rejected the error recovery patchs, which is fair enough.

Just the separation ... the actual patch looks OK.

Quoted 4 lines

I'll will continue working to separate the "error recovery
improvements:" into smaller feature add, but will take some time. I
want some constructive feedback, and too big of patch is deterring some people from looking at it.

I think it's fair to break it up, as long as its clearly noted that the patch is for review only.

Jeff

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 12 lines · Jeff Garzik

Quoted 10 lines · Eric

Quoted 3 lines · James Bottomley

Quoted 1 line

Youve rejected the error recovery patchs, which is fair enough.

Just the separation ... the actual patch looks OK.

I'll will continue working to separate the "error recovery
improvements:" into smaller feature add, but will take some time. I
want some constructive feedback, and too big of patch is deterring some people from looking at it.

Quoted 2 lines

I think it's fair to break it up, as long as its clearly noted that the patch is for review only.

Any chance the changes, properly documented, could be accepted as one big patch? Breaking this code into smaller patches, when it's really intended as a driver update, could prove problematic.

Quoted 10 lines · Jeff Garzik

Quoted 9 lines · Eric

Quoted 3 lines · James Bottomley

Quoted 1 line

Youve rejected the error recovery patchs, which is fair enough.

Just the separation ... the actual patch looks OK.

I'll will continue working to separate the "error recovery
improvements:" into smaller feature add, but will take some time. I
want some constructive feedback, and too big of patch is deterring some people from looking at it.

Quoted 2 lines

I think it's fair to break it up, as long as its clearly noted that the patch is for review only.

Any chance the changes, properly documented, could be accepted as one big patch? Breaking this code into smaller patches, when it's really intended as a driver update, could prove problematic.