Around 02/11/2016 08:59PM in some timezone, Andrew Jeffery wrote:
> Hi Milton,
>
> Excuse the unnecessary quote markers that might appear through the
> patch, my mail client (Evolution) seems to have a mind of its own
> sometimes. Starting to shave that yak. Anyway.
It can't be worse than me manually quoting in smart cloud notes client.
>
> As an aside I've tested an earlier version of the code and confirmed
> the approach works (i.e. no JFFS2 corruption on re-mount).
Yea.
> On Wed, 2016-02-10 at 21:50 -0600, OpenBMC Patches wrote:
>> From: "Milton D. Miller II" <miltonm at us.ibm.com>
>>
>> Make and use new routines to access the fifo when in user mode.
>> Instead of using memcpy_fromio, base the new routines on the io
>> port string accessors.
>>
...
> typo: unsigned
fixed
>> +static void *ins_align(unsigned long io, void *buf, size_t len)
>> +{
>> +> > BUILD_BUG_ON(0 & len & ~3);
>
> Should just be BUG_ON()? Given that you disabled it with '0 &', do we
> want it at all?
It should definitively not be a driver BUG that crashes the whole system. If
you need I can search for Linus' quote about the use of BUG in drivers.
It can be killed.
>> + */
>> +
>> +static void aspeed_smc_from_fifo(void *buf, const void __iomem *iop, size_t len)
>> +{
>> +> > unsigned long io = (__force unsigned long)iop;
>> +
...
>> +> > > buf = ins_align(io, buf, len & 1);
>> +> > } else {
>> +> > > size_t l = (4 - (unsigned long)buf) & 3;
>> +
>> +> > > l = min(l, len);
>> +> > > buf = ins_align(io, buf, l);
>> +> > > len -= l;
>> +
>> +> > > if (len >= 4) {
>> +> > > > insl(io, buf, len >> 2);
>> +> > > > buf += len & ~3;
>> +> > > }
>> +> > > buf = ins_align(io, buf, len & 3);
>> +> > }
>
> So I've tried deduplicating this function - the code in the latter two
> cases is largely the same bar the magic numbers and insw vs insl. I've
> come up with the code below (note that I haven't actually probed the
> modifications), but I'm not convinced it's much of a win in terms of
> clarity:
>
> /* Is this available somewhere else? Rusty doesn't think so.
> * Probably needs a better name (first set value) */
> #define FS_VAL(x) ((x) & ~((x) - 1))
That is a horrible name! What is this magic file system value about?
>
> static void aspeed_smc_from_fifo(void *buf, const void __iomem *iop, size_t len)
> {
> unsigned long io = (__force unsigned long)iop;
> u8 align = FS_VAL((io & 7) | 4); /* align is one of 1, 2 or 4 */
I see masking & 7 and comment about being 1, 2, or 4 and think:
That's not true, the port bit doesn't always have bit 4 on when
the others are off.
Only after a long pause coming back do I see the | 4, and then I have
to go back to a macro above to figure out is it before or after the
fancy math.
> u8 mask = align - 1;
> size_t to_align = align - ((unsigned long)buf) & mask);
>
> if (align == 1) {
> insb(io, buf, len);
> return;
> }
> to_align = min(to_align, len);
> buf = ins_align(io, buf, to_align);
> len -= to_align;
> if (len >= align) {
> size_t nr_blocks = len / align;
> if (align == 2) {
> insw(io, buf, nr_blocks);
> } else {
> insl(io, buf, nr_blocks);
> }
> buf += (nr_blocks * align);
> }
> buf = ins_align(io, buf, len & mask);
>
> Is it worth it? A similar approach can be taken for
> aspeed_smc_to_fifo() if so.
So I think this approach hides what is supposed to happen
in the case we are actually using.
I decided on a different approach to the remove the duplication:
Don't add code that will not be used.
We always pass a 4 byte aligned port. Rather than bug,
just call insb if the io port is not aligned.
Then remove the ins_align and outs_align function and
bring them inline, but expand top and bottom call sites
to order the byte vs (half)word transfers to remove
the get/put_unaligned pessimisation since we are testing
the alignemnt and can choose the order by context.
I then removed the updates to buf and len that were dead
in the tail code.
With 2 one-line comments and no white space it fits in
80x25 which means you can see all the code after the
preliminary 0 length and unaligned checks on one screen.
>
> As for whether there's a better approach short of fixing mmiocpy(), I
> don't know, so no further suggestions really (i.e. I guess we just live
> with the IO_SPACE_LIMIT jiggery-pokery).
>
When I started to go back and comment the assembly I figured out the
memcpy has 2 places it can stutter:
It first aligns source to a word boundary with byte copies. After that
it works on aligning the destination. It will do an extra load at this
point if the destination is not aligned to the source. Then it sometimes
aligns to cache lines (depends on core if it helps), then transfers
blocks, back to words, then if there are upto 3 bytes it backs up and
switches back to bytes (reading bytes a second time here).
So there are two places to fix, one stutters output and one input.
I thought about switching read/write to engine mode. I probably still
will sometime. The stutters will cause it to stop and resend the
address slowing down the access. Hmm, that might cause double write
attempt at the stutter...
Anyways, see what you think of the current code.
Thanks,
milton