Hi Marc,
On Tuesday, 15 August 2017 03:13:03 PDT Marc Zyngier wrote:
> Hi Paul,
>
> On 13/08/17 05:36, Paul Burton wrote:
> > This series cleans up the MIPS Global Interrupt Controller (GIC) driver
> > somewhat. It moves us towards using a header in a similar vein to the
> > ones we have for the MIPS Coherence Manager (CM) & Cluster Power
> > Controller (CPC) which allows us to access the GIC outside of the
> > irqchip driver - something beneficial already for the clocksource &
> > clock event driver, and which will be beneficial for further drivers
> > (eg. one for the GIC watchdog timer) and for multi-cluster work. Using
> > this header is also beneficial for consistency & code-sharing.
> >
> > In addition to cleanups the series also optimises the driver in various
> > ways, including by using a per-CPU variable for pcpu_masks & removing
> > the need to read the GIC_SH_MASK_* registers when decoding interrupts in
> > gic_handle_shared_int().
> >
> > This series requires my "[PATCH 00/19] MIPS: Initial multi-cluster
> > support" series to be applied first.
>
> I'm not on Cc on this one, so it is a bit hard to see what's going on.
That other series is only really related insomuch as it adds asm/mips-cps.h
which this series makes use of, and changes a couple of lines in irq-mips-
gic.c which would cause conflicts if this series is applied without the other
first.
> But overall, it is incredibly difficult to follow what is going on here.
> Everything seems to move around, and while I'm sure that you have
> something in mind, the mix of fixes+optimizations+new features is a bit
> hard to swallow (not to mention the VDSO stuff in the middle of what is
> supposed to be an irqchip series).
In general the non-irqchip patches preceed the irqchip patch that they enable.
For example, the VDSO patch you mention (presumably patch 22 "MIPS: VDSO: Drop
git_get_usm_range() usage") is followed immediately by the removal of that
function in patch 23 "irqchip: mips-gic: Remove gic_get_usm_range()".
The issue is that the MIPS GIC irqchip driver currently provides a bunch of
functions which have nothing to do with interrupts, and so probably ought to
be elsewhere. Moving that code elsewhere involves both adjusting the callers
of those functions & then removing them from the irqchip driver, hence the
non-irqchip patches followed by the related irqchip patches.
In general the only things that move are:
- The GIC register definitions, to asm/mips-gic.h for reasons described in
patch 2.
- Functions as I mentioned in the previous paragraph which have nothing to do
with the irqchip driver & don't need to be there once we have access to GIC
registers elsewhere through asm/mips-gic.h.
- A few other bits from linux/irqchip/mips-gic.h just so we can drop that
header.
> Is there any chance you could rework this to have a more logical
> ordering? Something like fixes first, new features next, and
> optimizations in the end, organized by domains (arch stuff first, then
> irqchip, then timer, then userspace)?
It's already almost grouped like that. We have:
- Patch 1 is a fix.
- Patches 2 through 34 are cleanup, though a couple could probably be
secondarily considered optimisations too.
- Patch 35 is an optimisation.
- Patches 36 through 38 could be considered optimisation or cleanup depending
upon the tint of your glasses.
So I'm not sure what you're asking me to do here - to group them better I
could perhaps move patch 35 to the end, but I'm not sure I see how that would
make review any easier.
I didn't group by domain like you suggest for the reason I mention before -
the non-irqchip patches are purely there to enable the following irqchip patch
so it made sense to me to group those together. If you really want I could
move all the non-irqchip patches to the start & all the irqchip patches to the
end, but again I'm not sure I see how that helps - it would just mean that
tightly related patches no longer follow one another.
> Because at the moment, this is a bit overwhelming...
My intent in splitting this into so many patches, and in grouping together the
related non-irqchip & irqchip patches, was to reduce that overwhelming-ness by
making each patch pretty readable by itself & perhaps taking into account only
a patch or two before it. I guess that didn't work though.. :)
Thanks,
PaulAttachment:
signature.asc
Description: This is a digitally signed message part.
