From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S933266AbcI3R0W (ORCPT <rfc822;w@1wt.eu>);
        Fri, 30 Sep 2016 13:26:22 -0400
Received: from mout.kundenserver.de ([212.227.126.135]:61210 "EHLO
        mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S933039AbcI3R0U (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 30 Sep 2016 13:26:20 -0400
From: Arnd Bergmann <arnd@arndb.de>
To: Boris Brezillon <boris.brezillon@free-electrons.com>
Subject: Re: [PATCH] mtd: mtk: avoid warning in mtk_ecc_encode
Date: Fri, 30 Sep 2016 19:25:17 +0200
User-Agent: KMail/1.12.2 (Linux/4.7.0-rc7+; KDE/4.3.2; x86_64; ; )
Cc: David Woodhouse <dwmw2@infradead.org>,
        Brian Norris <computersforpeace@gmail.com>,
        Richard Weinberger <richard@nod.at>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        linux-mtd@lists.infradead.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20160930163429.380785-1-arnd@arndb.de> <20160930185139.15c8be66@bbrezillon>
In-Reply-To: <20160930185139.15c8be66@bbrezillon>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Message-Id: <201609301925.17577.arnd@arndb.de>
X-Provags-ID: V03:K0:lSiSQ0/R9VHZk/blIUI57SO1nR3mHedtSSeEINeP5FtL1lbYfSA
 A1eb298WohpypaDp0QNtzID8qwyleo3IeTT72uek++4B5MhQz60RERqTINtlddz7N3wRCxm
 mvNzyelXJEme5w4cBid0GDwxq2YElx6jYE8pAs+zETc5DdpAcWJM1y2gLs1QiAdKeMznT0H
 0+ne67T564adVKavMxlMA==
X-UI-Out-Filterresults: notjunk:1;V01:K0:eUdrjLJdwYg=:kc5QEivSTLMt3lpAISy2tT
 Jdnww0Yxw/ASnfjrFPegTJ+/hWEJRnGM2tUU5aK9isyQxvI87ugMCbx4rYIPone25u5LUt5Zl
 nGMZkQbLU7sX2IGm2f78/TJhxmPkmcLDVXGd8NWeUfUmBZTQTtKsNeltGFAM/jLv6sS9Eh9uL
 DBT49+hW9i0RQBZodPERrexeNugQuIBmxbT+KdLRFXe8ElwYXVDFnnAC+LtsDXlBVrKXJOl+m
 YMCbv7O+zhF45kbCd+dTy1z7qXticb+omnUyqvqF3hHKcl+4tR+OZ/qTH/iPJeOgrsvdQSqIH
 C8EJw6b/rNoNvobgvOW2/7zobYr6Bxn2RtcUMtKFVNy93oz6JLQOYsyUO0VNOnZF8RiV/2Eay
 6sD1dfVYnAF9yv+wr6mhq3BL5SZZNViyOGd5NZxFUsQVgXdoR/XuTvnA2h3jHUYxUwPSKk3bt
 v5zsJUHCib0dHB+9Qq8ro5/1pqyzuzzoUdt/22NL/hihUaA9i9zC95zvEaYi/grqUANGwwkGW
 dANXhGf9FTN6zjjMHh0rORfLVFKFGtvzkEUbaPZd9T5zujzGKZTvE/VnV2Xy1ySQXUqpksc9U
 EnxeFjPT1HKe7Y7rChjTqXxKmEtbwY9vva6cODQNGbjFlL/bvlUfSuY7W/ExSd02BuA+IQYcM
 EHawKosV5yOK2sJghgjWPFkhQYoWVrcWQLGz5Al+j6O74InwtJ3D+AhuC6x++2TJ7YZ5wPw73
 BanQRLqmMv/1wUFT
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Friday 30 September 2016, Boris Brezillon wrote:
> > +     /* copy into possibly unaligned OOB region with actual length */
> > +     memcpy(data + bytes, eccdata, len);
> 
> Is it better than
> 
>         for (i = 0; i < len; i += 4) {
>                 u32 val = __raw_readl(ecc->regs + ECC_ENCPAR(i / 4));
> 
>                 memcpy(data + bytes + i, &val, min(len, 4));
>         }
> 
> I'm probably missing something, but what's the point of creating a
> temporary buffer of 112 bytes on the stack since you'll have to copy
> this data to the oob buffer at some point?


I tried something like that first, but wasn't too happy with it for
a number of small reasons:

- __raw_readl in a driver is not usually the right API, __memcpy32_from_io
  uses it internally, but it's better for a driver not to rely on that,
  in case we need some barriers (which we may in factt need for other drivers).

- the min(len,4) expression is incorrect, fixing that makes it more complicated
  again

- I didn't like to call memcpy() multiple times, as that might get turned
  into an external function call (the compiler is free to optimize small
  memcpy calls or not).

I agree that he 112 byte buffer isn't ideal either, it just seemed to
be the lesser annoyance.

	Arnd