Re: [PATCH v2 6/8] arm64: Import latest memcpy()/memmove() implementation

From: Neil Armstrong <narmstrong@baylibre.com>
To: Marek Szyprowski <m.szyprowski@samsung.com>,
	Robin Murphy <robin.murphy@arm.com>,
	will@kernel.org, catalin.marinas@arm.com,
	Kevin Hilman <khilman@baylibre.com>
Cc: linux-arm-kernel@lists.infradead.org, mark.rutland@arm.com,
	linux-amlogic@lists.infradead.org,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: Re: [PATCH v2 6/8] arm64: Import latest memcpy()/memmove() implementation
Date: Tue, 8 Jun 2021 14:36:26 +0200	[thread overview]
Message-ID: <66d01d91-f52c-1318-aa12-175afff86bad@baylibre.com> (raw)
In-Reply-To: <7624e09f-cd52-9430-ab18-1d816c8d2898@samsung.com>

Hi,

On 08/06/2021 14:21, Marek Szyprowski wrote:
> + Kevin
> 
> On 08.06.2021 13:37, Robin Murphy wrote:
>> Hi Marek,
>>
>> On 2021-06-08 12:15, Marek Szyprowski wrote:
>>> Hi Robin,
>>>
>>> On 27.05.2021 17:34, Robin Murphy wrote:
>>>> Import the latest implementation of memcpy(), based on the
>>>> upstream code of string/aarch64/memcpy.S at commit afd6244 from
>>>> https://protect2.fireeye.com/v1/url?k=0e25d630-51beef28-0e245d7f-0cc47a314e9a-b41fdb2d4d06ff75&q=1&e=fcfaf71d-f01a-4bc4-8e16-8ae86e0c0116&u=https%3A%2F%2Fgithub.com%2FARM-software%2Foptimized-routines, 
>>>> and subsuming
>>>> memmove() in the process.
>>>>
>>>> Note that for simplicity Arm have chosen to contribute this code
>>>> to Linux under GPLv2 rather than the original MIT license.
>>>>
>>>> Note also that the needs of the usercopy routines vs. regular memcpy()
>>>> have now diverged so far that we abandon the shared template idea
>>>> and the damage which that incurred to the tuning of LDP/STP loops.
>>>> We'll be back to tackle those routines separately in future.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>
>>> This patch landed recently in linux-next as commit 285133040e6c ("arm64:
>>> Import latest memcpy()/memmove() implementation"). Sadly it causes
>>> serious issues on Khadas VIM3 board. Reverting it on top of linux
>>> next-20210607 (together with 6b8f648959e5 and resolving the conflict in
>>> the Makefile) fixes the issue. Here is the kernel log:
>>>
>>> Unable to handle kernel paging request at virtual address 
>>> ffff8000136bd204
>>> Mem abort info:
>>>     ESR = 0x96000061
>>>     EC = 0x25: DABT (current EL), IL = 32 bits
>>>     SET = 0, FnV = 0
>>>     EA = 0, S1PTW = 0
>>> Data abort info:
>>>     ISV = 0, ISS = 0x00000061
>>
>> That's an alignment fault, which implies we're accessing something 
>> which isn't normal memory.
>>
>>>     CM = 0, WnR = 1
>>> swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000009da6000
>>> [ffff8000136bd204] pgd=10000000f4806003, p4d=10000000f4806003,
>>> pud=10000000f4805003, pmd=1000000000365003, pte=00680000ffe03713
>>> Internal error: Oops: 96000061 [#1] PREEMPT SMP
>>> Modules linked in: brcmfmac brcmutil cfg80211 dw_hdmi_i2s_audio
>>> meson_gxl hci_uart btqca btbcm bluetooth panfrost ecdh_generic ecc
>>> snd_soc_meson_axg_sound_card crct10dif_ce snd_soc_meson_card_utils
>>> rfkill rtc_hym8563 gpu_sched dwmac_generic rc_khadas meson_gxbb_wdt
>>> meson_ir pwm_meson snd_soc_meson_axg_tdmin snd_soc_meson_g12a_tohdmitx
>>> rtc_meson_vrtc snd_soc_meson_axg_tdmout snd_soc_meson_axg_frddr
>>> reset_meson_audio_arb snd_soc_meson_codec_glue axg_audio meson_rng
>>> sclk_div dwmac_meson8b snd_soc_meson_axg_toddr mdio_mux_meson_g12a
>>> clk_phase stmmac_platform rng_core snd_soc_meson_axg_fifo meson_dw_hdmi
>>> stmmac meson_drm meson_canvas dw_hdmi pcs_xpcs display_connector
>>> snd_soc_meson_axg_tdm_interface nvmem_meson_efuse adc_keys
>>> snd_soc_meson_axg_tdm_formatter
>>> CPU: 4 PID: 135 Comm: kworker/4:3 Not tainted 5.13.0-rc5-next-20210607
>>> #10441
>>> Hardware name: Khadas VIM3 (DT)
>>> Workqueue: events request_firmware_work_func
>>> pstate: 20000005 (nzCv daif -PAN -UAO -TCO BTYPE=--)
>>> pc : __memcpy+0x2c/0x260
>>> lr : sg_copy_buffer+0x90/0x118
>>> ...
>>> Call trace:
>>>    __memcpy+0x2c/0x260
>>>    sg_copy_to_buffer+0x14/0x20
>>>    meson_mmc_start_cmd+0xf4/0x2c8
>>>    meson_mmc_request+0x4c/0xb8
>>>    __mmc_start_request+0xa4/0x2a8
>>>    mmc_start_request+0x80/0xa8
>>>    mmc_wait_for_req+0x68/0xd8
>>>    mmc_io_rw_extended+0x1d4/0x2e0
>>>    sdio_io_rw_ext_helper+0xb0/0x1e8
>>>    sdio_memcpy_toio+0x20/0x28
>>>    brcmf_sdiod_skbuff_write.isra.18+0x2c/0x68 [brcmfmac]
>>>    brcmf_sdiod_ramrw+0xe0/0x230 [brcmfmac]
>>>    brcmf_sdio_firmware_callback+0xa8/0x7c8 [brcmfmac]
>>>    brcmf_fw_request_done+0x7c/0x100 [brcmfmac]
>>>    request_firmware_work_func+0x4c/0xd8
>>>    process_one_work+0x2a8/0x718
>>>    worker_thread+0x48/0x460
>>>    kthread+0x12c/0x160
>>>    ret_from_fork+0x10/0x18
>>> Code: 540000c3 a9401c26 a97f348c a9001c06 (a93f34ac)
>>> ---[ end trace be83fa283dc82415 ]---
>>>
>>> I hope that the above log helps fixing the issue. IIRC the SDHCI driver
>>> on VIM3 board uses internal SRAM for transferring data (instead of DMA),
>>> so the issue is somehow related to that.
>>
>> Drivers shouldn't be using memcpy() on iomem mappings. Even if they 
>> happen to have got away with it sometimes ;)
>>
>> Taking a quick look at that driver,
>>
>>     host->bounce_buf = host->regs + SD_EMMC_SRAM_DATA_BUF_OFF;
>>
>> is completely bogus, as Sparse will readily point out.

My bad, what's the correct way to copy data to an iomem mapping ?

Neil

>>
>> Robin.
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel