On Thu, 2015-05-21 at 09:06 +0530, trigg wrote:
>
>
> > On 21-May-2015, at 05:16, Michael Neuling <mi...@neuling.org> wrote:
> >
> > + */
> > > +ssize_t cxl_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
> > > + loff_t off, size_t count)
> > > +{
> > > + loff_t aligned_off;
> > > + size_t aligned_count;
> > > + const void __iomem *ebuf = afu->afu_desc_mmio +
> > > afu->eb_offset;
> > > +
> > > + if (!afu->eb_len || count == 0 || off < 0)
> >
> > if eb_len == 0 we don't even create the sysfs file. So is this
> > check
> > needed?
> This function is non static so it can be potentially called from
> kernel.
What? You mean outside this file?
> Condition check of "if (count == 0 || off < 0 || (size_t)off
> >= afu->eb_len) "
> should work solving the problem below too.
It makes no sense to call this outside of sysfs reading the error
buffer.
> > > + aligned_off = round_down(off, 8);
> > > + aligned_count = round_up(off + count, 8) - aligned_off;
> >
> > I kinda preferred the start end variables, and length was just end -
> > start.
> >
> > I though it was more readable. IMHO
> >
> > How about:
> >
> > aligned_start = round_down(off, 8);
> > aligned_end = round_up(off + count, 8);
> > aligned_length = aligned_end - aligned_start;
> Agreed on aligned_start and aligned_end but would not
> need aligned_end in rest of the code.
Aligned_end makes it more readable.
> >
> > > +
> > > + /* fast path */
> > > + if ((aligned_off == off) && (aligned_count == count)) {
> > > + /* no need to use the bounce buffer */
> > > + _memcpy_fromio(buf, ebuf + off, count);
> >
> > I would drop this, as the other code path should work fine.
> > Premature optimisation.....
> I am inclined to differ on this. Code below uses a bounce buffer
> which needs more than double the amount of loads and stores.
Cacheable vs non-cacheable is important here though.
> If the caller wants to speed up things it can simply ask for aligned
> read that won't have this overhead. This will be especially useful
> In large reads.
The overhead will mostly be in the non-cachable/MMIO load/stores rather
than the cacheable load/stores, so there may be little performance
difference anyway.
The only reason this could be useful is if you have a really really
large error buffer. Even then, why do you need to read it that quickly?
Mikey
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
