From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-x231.google.com (mail-pd0-x231.google.com [IPv6:2607:f8b0:400e:c02::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id DE0FF1A00B5 for ; Thu, 21 May 2015 13:36:50 +1000 (AEST) Received: by pdbqa5 with SMTP id qa5so92515526pdb.0 for ; Wed, 20 May 2015 20:36:48 -0700 (PDT) Content-Type: multipart/alternative; boundary=Apple-Mail-BA454BD8-9228-4D7A-8BBF-5C1AE172D4CE Mime-Version: 1.0 (1.0) Subject: Re: [PATCH v3] cxl: Export AFU error buffer via sysfs From: trigg In-Reply-To: <1432165570.27130.5.camel@neuling.org> Date: Thu, 21 May 2015 09:06:43 +0530 Cc: Vaibhav Jain , Michael Ellerman , "linuxppc-dev@lists.ozlabs.org" , Ian Munsie Message-Id: References: <1432142313-7741-1-git-send-email-vaibhav@linux.vnet.ibm.com> <1432165570.27130.5.camel@neuling.org> To: Michael Neuling List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , --Apple-Mail-BA454BD8-9228-4D7A-8BBF-5C1AE172D4CE Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable > On 21-May-2015, at 05:16, Michael Neuling wrote: >=20 > + */ >> +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 =3D afu->afu_desc_mmio + afu->eb_offset; >> + >> + if (!afu->eb_len || count =3D=3D 0 || off < 0) >=20 > if eb_len =3D=3D 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. Condition check of "if (count =3D=3D 0 || off < 0 || (size_t)off >=3D afu->e= b_len) " should work solving the problem below too. >=20 >> + return 0; >> + >> + /* calculate aligned read window */ >> + count =3D min((size_t)(afu->eb_len - off), count); >=20 > What if count ends up being negative because off > afu->eb_len?? Agreed. Thanks for catching this. Size_t being unsigned would overflow To a large positive value. >=20 >> + aligned_off =3D round_down(off, 8); >> + aligned_count =3D round_up(off + count, 8) - aligned_off; >=20 > I kinda preferred the start end variables, and length was just end - > start. >=20 > I though it was more readable. IMHO >=20 > How about: >=20 > aligned_start =3D round_down(off, 8); > aligned_end =3D round_up(off + count, 8); > aligned_length =3D aligned_end - aligned_start; Agreed on aligned_start and aligned_end but would not need aligned_end in rest of the code. >=20 >> + >> + /* fast path */ >> + if ((aligned_off =3D=3D off) && (aligned_count =3D=3D count)) { >> + /* no need to use the bounce buffer */ >> + _memcpy_fromio(buf, ebuf + off, count); >=20 > 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. 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=20 In large reads. >> + >> + } else { >> + /* use bounce buffer for copy */ >> + void *tbuf =3D (void *)__get_free_page(GFP_TEMPORARY); >> + >> + if (!tbuf) >> + return -ENOMEM; >> + >> + /* max we can copy in one read is PAGE_SIZE */ >> + aligned_count =3D min(aligned_count, PAGE_SIZE); >> + _memcpy_fromio(tbuf, ebuf + aligned_off, aligned_count); >> + >> + count =3D min(count, aligned_count); Thanks for catching this. >=20 > This doesn't seem right. count will equal PAGE_SIZE if it's too big but > it has to be smaller by (off & 7) in this case. >=20 > How about this? >=20 > #define MAX_COPY_SIZE PAGE_SIZE > ... >=20 > void *bbuf; >=20 > /* Bounds check count with err buf length */ > count =3D min((size_t)(afu->eb_len - off), count); > if ((off < 0) || (count < 0)) > return 0; >=20 > /* Create aligned bounce buffer to copy into */ > aligned_start =3D round_down(off, 8); > aligned_end =3D round_up(off + count, 8); > aligned_length =3D aligned_end - aligned_start; >=20 > if (aligned_length > MAX_COPY_SIZE) { > aligned_length =3D MAX_COPY_SIZE; > count =3D MAX_COPY_SIZE - (off & 0x7); > } >=20 > bbuf =3D (void *)__get_free_page(GFP_TEMPORARY); > if (!bbuf) > return -ENOMEM; >=20 > /* Use _memcpy_fromio() so the reads are aligned */ > _memcpy_fromio(bbuf, ebuf + aligned_start, aligned_length); > memcpy(buf, bbuf + (off & 0x7), count); >=20 > free_page(bbuf); >=20 >=20 >> + memcpy(buf, tbuf + (off & 0x7), count); >> + >> + free_page((unsigned long)tbuf); >> + } >> + >> + return count; >=20 > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev --Apple-Mail-BA454BD8-9228-4D7A-8BBF-5C1AE172D4CE Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
<= br>
On 21-May-2015, at 05:16, Michael Neuling= <mikey@neuling.org> wrote:

+ */
+ssize_t cxl_afu_read_err_buffer= (struct cxl_afu *afu, char *buf,
+     = ;           loff_t of= f, size_t count)
+{
+    = loff_t aligned_off;
+    size_t aligned_cou= nt;
+    const void __iomem *ebuf =3D afu-&= gt;afu_desc_mmio + afu->eb_offset;
+
= +    if (!afu->eb_len || count =3D=3D 0 || of= f < 0)

if eb_len =3D= =3D 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 kerne= l.
Condition check of "if (count =3D=3D 0 || off < 0 || (size_t)off &= gt;=3D afu->eb_len) "
should work solving the probl= em below too.

+    &= nbsp;   return 0;
+
=
+ &nb= sp;  /* calculate aligned read window */
+  = ;  count =3D min((size_t)(afu->eb_len - off), count);

What if count ends up being negative b= ecause off > afu->eb_len??
Agreed. Thanks= for catching this. Size_t being unsigned would overflow
To a= large positive value.

=
+    aligned_off =3D round_down(off, 8);
+    aligned_count =3D round_up(off + count, 8) - a= ligned_off;

I kinda preferre= d the start end variables, and length was just end -
=
start.

I t= hough it was more readable. IMHO

How abou= t:

  aligned_start =3D round_d= own(off, 8);
 &= nbsp;aligned_end =3D round_up(off + count, 8);
  aligned_length =3D aligned_end - alig= ned_start;
Agreed on aligned_start and aligned_= end but would not
need aligned_end in rest of the code.

+
+  &nbs= p; /* fast path */
+    if ((aligned_o= ff =3D=3D off) && (aligned_count =3D=3D count)) {
= +        /* no need to use the bounce buf= fer */
+        _memcpy= _fromio(buf, ebuf + off, count);

<= span>I would drop this, as the other code path should work fine.
<= /blockquote>
Premature optimisation.....
I am inclined to differ on this. Code below uses a b= ounce buffer
which needs more than double the amount of load= s and stores.
If the caller wants to speed up things it can s= imply ask for aligned
read that won't have this overhead. Th= is will be especially useful
In large reads.

+
+    } else {
+   &n= bsp;    /* use bounce buffer for copy */
+        void *tbuf =3D (void *)__get_= free_page(GFP_TEMPORARY);
+
+  &nbs= p;     if (!tbuf)
+   &= nbsp;        return -ENOMEM;<= br>
+
<= blockquote type=3D"cite">+        /= * max we can copy in one read is PAGE_SIZE */
+  &nbs= p;     aligned_count =3D min(aligned_count, PAGE_SI= ZE);
+        _memcpy_f= romio(tbuf, ebuf + aligned_off, aligned_count);
+
+        count =3D min(count, a= ligned_count);
Thanks for catching= this.

This doesn't seem right.  = ;count will equal PAGE_SIZE if it's too big but
it has to be smaller by (off & 7) in this cas= e.

How about this?

      #define MAX_COPY_SIZE PAGE= _SIZE
  ..= .

  void *bbuf;

  /* Bounds check count with err buf length *= /
  count =3D= min((size_t)(afu->eb_len - off), count);
  if ((off < 0) || (count < 0))
    = ;  return 0;

  /* Cr= eate aligned bounce buffer to copy into */
  aligned_start =3D round_down(off, 8);
  aligned_end= =3D round_up(off + count, 8);
  aligned_length =3D aligned_end - aligned_start;

  if (aligned_length > MAX_COPY_= SIZE) {
  =     aligned_length =3D MAX_COPY_SIZE;
      co= unt =3D MAX_COPY_SIZE - (off & 0x7);
      }

  bbuf =3D (void *)__get_free_page(GFP_TEMPORARY= );
  if (!= bbuf)
  &n= bsp;   return -ENOMEM;

&n= bsp; /* Use _memcpy_fromio() so the reads are aligned */
  _memcpy_fromio(bbuf, e= buf + aligned_start, aligned_length);
  memcpy(buf, bbuf + (off & 0x7), count);

  free_page(bbuf);


+        memc= py(buf, tbuf + (off & 0x7), count);
=
+
+        free_page((unsigned long)tbuf)= ;
+    }
+
+    return count;

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
= --Apple-Mail-BA454BD8-9228-4D7A-8BBF-5C1AE172D4CE--