Re: [Qemu-devel] [PATCH] MIPS: Correct FCR0 initialization

[Maciej W. Rozycki]

Hi Andreas,

> >  I find quilt patches easier to manage when I need to reorder them, 
> > revert, manually edit the diffs (that I routinely do), etc.  Perhaps I'm 
> > just outdated, but that's the workflow I've found most efficient for me 
> > while not disturbing anyone else.  I've used quilt patches with the Linux 
> > kernel including upstream submission successfully as well, for many years 
> > now, and I don't remember anyone complaining about their formatting.  
> > Also automatic patch retriever/tester scripts that some mailing lists have 
> > watching process them correctly.
> > 
> >  Let's therefore please focus on the technical value of these changes 
> > rather than their envelope.
> 
> Both are important to those of us reviewing and working in maintenance.
> 
> http://wiki.qemu.org/Contribute/SubmitAPatch
> 
> For example, our usual convention would've been to use a "target-mips:"
> rather than "MIPS:" prefix (the directory name), if you follow the list
> and history. We also don't usually indent paragraphs within the commit
> message, especially not with differing indentation, and our Coding Style
> is without space before parenthesis. Patches that look odd may get less
> review attention. Not everything is mandatory of course, but maybe you
> can also see the other side of being flooded with patches.

 Sure, I'll take your suggestions into account as much as I can, that's 
not a problem, I see your point and I don't want to make your work harder 
than it already is -- I know a good technical review can take a lot of 
effort, especially with complicated changes.  I just addressed the issue 
of the diff itself (or actually the heading only) that you specifically 
raised.  I read the document before posting those proposals and I've 
thought I got all the points right, but it looks that I missed a bit here 
and there -- apologies for that.

> >  I don't recollect retracting any of my patches although I'll be making 
> > the adjustments previously requested and produce more.  Any patches that 
> > may have overlapped with an earlier submission come from the same tree, 
> > except I regenerated them and retested against the then current top of the 
> > tree; I may have updated them too to address any problems spotted while 
> > processing them.
> 
> OK, so you didn't retract them but Meador did comment:
> 
> > I submitted a patch to fix this issue and the FCR0 issue a few months back 
> > [1].
> > Andreas reviewed it, but the patch never got committed.
> > 
> > [1] http://patchwork.ozlabs.org/patch/144353/
> 
> They're definitely different in scope and changes, whatever process and
> tools you guys use internally. And our policy is to give preference to
> the earlier patch to not invite people to redo other people's patches
> with different SoB (not saying you are, same company after all).

 I have had a look and it appears to me this is merely a number of patches 
that I posted as individual changes folded into one.  Personally I try to 
follow the one-change-at-a-time principle whenever possible for easier 
tracking down any issues that may arise later on if nothing else (e.g. 
`git bisect'), so I maintain it's better if they're committed separately.

> Either way, the committed patch is now missing the info that this issue
> was originally Reported-by and attempted to fix by Khansa Butt, not
> employed by Mentor nor using their tree.

 Hopefully that can be fixed up.  Please note that Richard Henderson had 
concerns about these growing functions having been defined as static 
inline.  I agree with that concern, technically; the original choice 
merely having been by following the convention observed elsewhere 
throughout QEMU's tree under the assumption the simulator is unusual 
enough a piece of software that there must have been a valid justification 
for such an arrangement.  I'll be fixing this up with the repost.

> >> Doing follow-ups based on this one or, in worst case, reverting is
> >> certainly possible but the decision-making would best be done by someone
> >> who actually uses mips - not that there's no users, just no volunteer
> >> for a staging tree yet.
> > 
> >  I'm willing to provide assistance as time permits, in particular to move 
> > forward with any changes I have proposed either myself or on behalf of 
> > someone else, although owing to other commitments regrettably I can't 
> > commit to regular testing/mainentance.
> 
> You could keep the status in MAINTAINERS as "Odd Fixes" but set up a git
> branch where maintainers can pull from and that you / other contributors
> can develop against.

 As I say, I feel I am too constrained, at least the moment, to give QEMU 
the level of attention it deserves, but certainly I am going to address 
any concerns anyone may have about my changes, be it at the submission 
time, or anytime in the future.  Feel free to ask me about any hardware 
specifics as well.

> Me, I'm still interested in modelling MIPS CPU models as proper QOM
> subclasses if we can sort out the initialization and code placement
> issues that Meador was poking at for FCR0 - moving code from
> cpu_mips_init() into mips_cpu_initfn() and killing cpu_state_reset().

 I'll take that into account before reposting.  Thanks for your input.

  Maciej