<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div><span></span></div><div><span></span><br><blockquote type="cite"><span>On 21-May-2015, at 05:16, Michael Neuling <<a href="mailto:mikey@neuling.org">mikey@neuling.org</a>> wrote:</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>+ */</span><br></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ssize_t cxl_afu_read_err_buffer(struct cxl_afu *afu, char *buf,</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ loff_t off, size_t count)</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+{</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ loff_t aligned_off;</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ size_t aligned_count;</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ const void __iomem *ebuf = afu->afu_desc_mmio + afu->eb_offset;</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ </span><font color="#000000"><span style="background-color: rgba(255, 255, 255, 0);">if (!afu->eb_len || count == 0 || off < 0)</span></font><br></blockquote></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>if eb_len == 0 we don't even create the sysfs file. So is this check</span><br></blockquote><blockquote type="cite"><span>needed?</span><br></blockquote><span>This function is non static so it can be potentially called from kernel.</span><br><span>Condition check of "</span><span style="background-color: rgba(255, 255, 255, 0);">if (count == 0 || off < 0 || (size_t)off >= afu->eb_len) "</span></div><div>should work solving the problem below too.<br><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ return 0;</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ /* calculate aligned read window */</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ count = min((size_t)(afu->eb_len - off), count);</span><br></blockquote></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>What if count ends up being negative because off > afu->eb_len??</span><br></blockquote><span>Agreed. Thanks for catching this. Size_t being unsigned would overflow</span><br><span>To a large positive value.</span><br><span></span><br><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ aligned_off = round_down(off, 8);</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ aligned_count = round_up(off + count, 8) - aligned_off;</span><br></blockquote></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>I kinda preferred the start end variables, and length was just end -</span><br></blockquote><blockquote type="cite"><span>start.</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>I though it was more readable. IMHO</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>How about:</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> aligned_start = round_down(off, 8);</span><br></blockquote><blockquote type="cite"><span> aligned_end = round_up(off + count, 8);</span><br></blockquote><blockquote type="cite"><span> aligned_length = aligned_end - aligned_start;</span><br></blockquote><span>Agreed on aligned_start and aligned_end but would not</span><br><span>need aligned_end in rest of the code.</span><br><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ /* fast path */</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ if ((aligned_off == off) && (aligned_count == count)) {</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ /* no need to use the bounce buffer */</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ _memcpy_fromio(buf, ebuf + off, count);</span><br></blockquote></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>I would drop this, as the other code path should work fine.</span><br></blockquote><blockquote type="cite"><span>Premature optimisation.....</span><br></blockquote><span>I am inclined to differ on this. Code below uses a bounce buffer</span><br><span>which needs more than double the amount of loads and stores.</span><br><span>If the caller wants to speed up things it can simply ask for aligned</span><br><span>read that won't have this overhead. This will be especially useful </span><br><span>In large reads.</span><br><span></span><br><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ } else {</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ /* use bounce buffer for copy */</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ void *tbuf = (void *)__get_free_page(GFP_TEMPORARY);</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ if (!tbuf)</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ return -ENOMEM;</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ /* max we can copy in one read is PAGE_SIZE */</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ aligned_count = min(aligned_count, PAGE_SIZE);</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ _memcpy_fromio(tbuf, ebuf + aligned_off, aligned_count);</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ count = min(count, aligned_count);</span><br></blockquote></blockquote><span>Thanks for catching this.</span><br><span></span><br><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>This doesn't seem right. count will equal PAGE_SIZE if it's too big but</span><br></blockquote><blockquote type="cite"><span>it has to be smaller by (off & 7) in this case.</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>How about this?</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> #define MAX_COPY_SIZE PAGE_SIZE</span><br></blockquote><blockquote type="cite"><span> ...</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> void *bbuf;</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> /* Bounds check count with err buf length */</span><br></blockquote><blockquote type="cite"><span> count = min((size_t)(afu->eb_len - off), count);</span><br></blockquote><blockquote type="cite"><span> if ((off < 0) || (count < 0))</span><br></blockquote><blockquote type="cite"><span> return 0;</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> /* Create aligned bounce buffer to copy into */</span><br></blockquote><blockquote type="cite"><span> aligned_start = round_down(off, 8);</span><br></blockquote><blockquote type="cite"><span> aligned_end = round_up(off + count, 8);</span><br></blockquote><blockquote type="cite"><span> aligned_length = aligned_end - aligned_start;</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> if (aligned_length > MAX_COPY_SIZE) {</span><br></blockquote><blockquote type="cite"><span> aligned_length = MAX_COPY_SIZE;</span><br></blockquote><blockquote type="cite"><span> count = MAX_COPY_SIZE - (off & 0x7);</span><br></blockquote><blockquote type="cite"><span> }</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> bbuf = (void *)__get_free_page(GFP_TEMPORARY);</span><br></blockquote><blockquote type="cite"><span> if (!bbuf)</span><br></blockquote><blockquote type="cite"><span> return -ENOMEM;</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> /* Use _memcpy_fromio() so the reads are aligned */</span><br></blockquote><blockquote type="cite"><span> _memcpy_fromio(bbuf, ebuf + aligned_start, aligned_length);</span><br></blockquote><blockquote type="cite"><span> memcpy(buf, bbuf + (off & 0x7), count);</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span> free_page(bbuf);</span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ memcpy(buf, tbuf + (off & 0x7), count);</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ free_page((unsigned long)tbuf);</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ }</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+</span><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><span>+ return count;</span><br></blockquote></blockquote><blockquote type="cite"><span></span><br></blockquote><blockquote type="cite"><span>_______________________________________________</span><br></blockquote><blockquote type="cite"><span>Linuxppc-dev mailing list</span><br></blockquote><blockquote type="cite"><span><a href="mailto:Linuxppc-dev@lists.ozlabs.org">Linuxppc-dev@lists.ozlabs.org</a></span><br></blockquote><blockquote type="cite"><span><a href="https://lists.ozlabs.org/listinfo/linuxppc-dev">https://lists.ozlabs.org/listinfo/linuxppc-dev</a></span><br></blockquote></div></body></html>