[PATCH 00/29] cxlflash: Miscellaneous bug fixes and corrections

[PATCH 00/29] cxlflash: Miscellaneous bug fixes and corrections

[Matthew R. Ochs]

This patch set contains various fixes and corrections for issues that
were found during test and code review. The series is based upon the
code upstreamed in 4.3 and is intended for the rc phase. The entire
set is bisectable.

Manoj Kumar (3):
  cxlflash: Move global.mutex to caller of find_and_create_lun()
  cxlflash: Add a literal for units of max_sector
  cxlflash: Increase timeout value for read capacity to 30 seconds

Matthew R. Ochs (26):
  cxlflash: Obtain additional sdev reference
  cxlflash: Always attempt LUN table initialization
  cxlflash: Change rht_needs_ws from bool to u8
  cxlflash: Add ULL specifier to context encode mask
  cxlflash: Add ioctl drain
  cxlflash: Check for removal when processing interrupt
  cxlflash: Rename limbo to reset
  cxlflash: Make functions static
  cxlflash: Refine host/device attributes
  cxlflash: Refine print statements
  cxlflash: Protect TMF waitq with spinlock
  cxlflash: Only set resid when needed
  cxlflash: Rescan host following link up event
  cxlflash: Bypass async interrupt bits that are off
  cxlflash: Remove dual port online dependency
  cxlflash: Properly obtain AFU version and check it
  cxlflash: Correct usage of scsi_host_put()
  cxlflash: Move workq termination earlier on remove
  cxlflash: Recheck state after adapter reset
  cxlflash: Remove unnecessary scsi_block_requests
  cxlflash: Update function prologs
  cxlflash: Fix MMIO and endianness errors
  cxlflash: Drop context mutex while processing sense
  cxlflash: Spelling, grammar, and alignment changes
  cxlflash: Clear AFU RRQ following reset
  MAINTAINERS: Add cxlflash driver

 MAINTAINERS                       |    9 +
 drivers/scsi/cxlflash/common.h    |   28 +-
 drivers/scsi/cxlflash/lunmgt.c    |    9 +-
 drivers/scsi/cxlflash/main.c      | 1539 ++++++++++++++++++++-----------------
 drivers/scsi/cxlflash/main.h      |    1 +
 drivers/scsi/cxlflash/sislite.h   |    8 +-
 drivers/scsi/cxlflash/superpipe.c |  172 +++--
 drivers/scsi/cxlflash/superpipe.h |   11 +-
 drivers/scsi/cxlflash/vlun.c      |   39 +-
 9 files changed, 998 insertions(+), 818 deletions(-)

-- 
2.1.0

Re: [PATCH 00/29] cxlflash: Miscellaneous bug fixes and corrections

[Ian Munsie]

Hi Matt & Manoj,

Just a general comment about this series - I'd like to see more detailed
commit messages for almost all these patches. Of course James is the
scsi maintainer and it's up to him whether to take these as is or not,
but generally when you write a commit message for a bug fix you want to
explain:

- What problem can occur, possibly including an example
- Why it occurs
- How this patch addresses it

You don't necessarily need to go overboard because at some point people
can just read the code, but some of these patches don't have any detail
beyond a single subject line, which is too little.

Speaking of the subject line - if the patch is fixing a bug there should
be some indication of that in the subject (it should probably include
the word "fix" somewhere). If the subject just states what you are
changing then it's not immediately obvious that it is a bug fix.

A few examples:

> [PATCH 01/29] cxlflash: Move global.mutex to caller of find_and_create_lun()
>
> Since the retrieved LUN is modified in the caller, hold the mutex
> across modifications as well.

Here the subject line just states what this patch does, but does not
indicate that it is a bug fix. Perhaps something more like "cxlflash:
fix global.mutex not being held while LUN is modified"? Without that, I
can't tell at a glance if this is fixing a bug, or if it's just a bit of
refactoring.

You have a suggestion in the body that there may be an issue and what
you are doing to address it, but you have not explained why this is a
problem.  Give an example of something that could go wrong - if you have
an actual documented case use it as an example, otherwise give us a
theoretical example, like "such and such could be modified while such
and such is happening, possibly leading to such and such". If you don't
have a solid example and are just doing this to harden the code because
you think there might be a problem, say so.

> [PATCH 02/29] cxlflash: Add a literal for units of max_sector

Ok, looking at the code this is fairly obvious what you are doing and
why, but you should give me some explanation in the commit message as
well, even if it's just something like "The magic number 512 in the code
is not very meaningful - replace it with the literal MAX_SECTOR_UNIT to
make it self-explanatory". This patch could probably be combined with
patch 3, then the commit message could just be a run down of the
literals you are adding/renaming.

> [PATCH 04/29] cxlflash: Obtain additional sdev reference
> 
> When a LUN is removed, the sdev that is associted with the LUN remains
> intact until its reference count drops to 0. In order to prevent an sdev
> from being removed while a context is still associated with it, obtain an
> additional reference per-context for each LUN attached to the context.
> 
> This resovles a potential Oops in the release handler when a dealing with
> a LUN that has already been removed.

This is much better - the commit message indicates that there is a
potential oops (what), why it occurs, and how you are addressing it :)

I'd probably change the subject line to indicate that it is a fix
though, perhaps:
"cxlflash: Fix potential oops by obtaining additional sdev reference"

> [PATCH 05/29] cxlflash: Always attempt LUN table initialization
> 
> On a virtual LUN open, there should always be an attempt made to
> update the LUN table for the adapter, regardless of the the global
> LUN's state prior to the open. This is necessary in a multi-adapter
> environment so that access to a LUN [that has already been transitioned
> to virtual mode] through a second (or greater) adapter is configured
> correctly by programming the adapter's LUN table.

After reading this I can see you have tried to explain the problem, but
it's still not super clear to me what it is (ok, you've said it's
necessary to do this, but what is the consequence of not doing this?)...
Perhaps I'm just not versed enough in scsi land though, and maybe this
is obvious to someone who is ;-)

I'd at least add one sentence explaining what this patch does - I assume
it fixes the problem you described, but the commit message doesn't state
that. Again, the subject should indicate this is a bug fix.

> [PATCH 06/29] cxlflash: Change rht_needs_ws from bool to u8
> 
> Fix sparse sizeof(bool) warning by changing type from bool to u8.

I'd almost reverse these two lines - the subject should indicate this is
fixing a sparse warning, and the body can indicate how. If you have the
actual warning from sparse I'd include that in the body as well.

> [PATCH 08/29] cxlflash: Add ioctl drain
> 
> Create the ability to drain ioctls by wrapping the ioctl handler
> call in a read semaphore and then implementing a small routine that
> obtains the write semaphore, effectively creating a wait point for
> all currently executing ioctls.

There's no indication of what the problem being fixed by this patch is,
or why it occurs - this commit message could just as easily be for a new
feature, which wouldn't go in the rc phase. If this is a new feature
that is going to be used by another patch to fix a bug, say so. If this
patch fixes a bug by itself, say so.

> [PATCH 10/29] cxlflash: Rename limbo to reset
> 
> Rename the state and waitq.

OK, this is fairly trivial and isn't a bug fix, but a little more
explanation would be good here - are you renaming them because they were
ambiguous or misleading in some way, or are the new names just more
precise?. It doesn't have to be long, it just has to answer "why?". If
you are renaming them now to address comments from the review phase
maybe mention that as well.


And so on - I think you get the idea.


Cheers,
-Ian

Re: [PATCH 00/29] cxlflash: Miscellaneous bug fixes and corrections

[Matthew R. Ochs]

> On Sep 13, 2015, at 8:12 PM, Ian Munsie <imunsie@au1.ibm.com> wrote:
> 
> Hi Matt & Manoj,
> 
> Just a general comment about this series - I'd like to see more detailed
> commit messages for almost all these patches. Of course James is the
> scsi maintainer and it's up to him whether to take these as is or not,
> but generally when you write a commit message for a bug fix you want to
> explain:
> 
> - What problem can occur, possibly including an example
> - Why it occurs
> - How this patch addresses it
> 
> You don't necessarily need to go overboard because at some point people
> can just read the code, but some of these patches don't have any detail
> beyond a single subject line, which is too little.
> 
> Speaking of the subject line - if the patch is fixing a bug there should
> be some indication of that in the subject (it should probably include
> the word "fix" somewhere). If the subject just states what you are
> changing then it's not immediately obvious that it is a bug fix.

This is a reasonable request. Will incorporate your suggestions
and send out in a v2 series. Thanks for the examples of what you
would like to see.


-matt

Re: [PATCH 00/29] cxlflash: Miscellaneous bug fixes and corrections

[Ian Munsie]

Hi Matt & Manoj,

Can you also add linuxppc-dev@lists.ozlabs.org to the Cc list for
version 2?

Cheers,
-Ian

Excerpts from Matthew R. Ochs's message of 2015-09-15 06:12:29 +1000:
> > On Sep 13, 2015, at 8:12 PM, Ian Munsie <imunsie@au1.ibm.com> wrote:
> > 
> > Hi Matt & Manoj,
> > 
> > Just a general comment about this series - I'd like to see more detailed
> > commit messages for almost all these patches. Of course James is the
> > scsi maintainer and it's up to him whether to take these as is or not,
> > but generally when you write a commit message for a bug fix you want to
> > explain:
> > 
> > - What problem can occur, possibly including an example
> > - Why it occurs
> > - How this patch addresses it
> > 
> > You don't necessarily need to go overboard because at some point people
> > can just read the code, but some of these patches don't have any detail
> > beyond a single subject line, which is too little.
> > 
> > Speaking of the subject line - if the patch is fixing a bug there should
> > be some indication of that in the subject (it should probably include
> > the word "fix" somewhere). If the subject just states what you are
> > changing then it's not immediately obvious that it is a bug fix.
> 
> This is a reasonable request. Will incorporate your suggestions
> and send out in a v2 series. Thanks for the examples of what you
> would like to see.
> 
> 
> -matt

Re: [PATCH 00/29] cxlflash: Miscellaneous bug fixes and corrections

[Matthew R. Ochs]

> On Sep 15, 2015, at 9:50 PM, Ian Munsie <imunsie@au1.ibm.com> wrote:
> 
> Hi Matt & Manoj,
> 
> Can you also add linuxppc-dev@lists.ozlabs.org to the Cc list for
> version 2?

Will do.