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