Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754019AbcCWWAQ (ORCPT ); Wed, 23 Mar 2016 18:00:16 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:52880 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751945AbcCWWAO (ORCPT ); Wed, 23 Mar 2016 18:00:14 -0400 Date: Wed, 23 Mar 2016 15:00:12 -0700 From: Stephen Boyd To: Hauke Mehrtens Cc: Aaro Koskinen , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Andrew Morton , linux-mips@linux-mips.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] drivers/firmware/broadcom/bcm47xx_nvram.c: fix incorrect __ioread32_copy Message-ID: <20160323220012.GB18567@codeaurora.org> References: <1458083178-8207-1-git-send-email-aaro.koskinen@iki.fi> <56F30E1F.5020108@hauke-m.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56F30E1F.5020108@hauke-m.de> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2500 Lines: 57 On 03/23, Hauke Mehrtens wrote: > On 03/16/2016 12:06 AM, Aaro Koskinen wrote: > > Commit 1f330c327900 ("drivers/firmware/broadcom/bcm47xx_nvram.c: use > > __ioread32_copy() instead of open-coding") switched to use a generic copy > > function, but failed to notice that the header pointer is updated between > > the two copies, resulting in bogus data being copied in the latter one. > > Fix by keeping the old header pointer. > > > > The patch fixes totally broken networking on WRT54GL router (both LAN > > and WLAN interfaces fail to probe). > > > > Fixes: 1f330c327900 ("drivers/firmware/broadcom/bcm47xx_nvram.c: use __ioread32_copy() instead of open-coding") > > Signed-off-by: Aaro Koskinen > > --- > > > > v2: Avoid using the device memory after the first copy when > > checking the nvram length, suggested by Stephen Boyd. > > > > v1: http://marc.info/?t=145807850800003&r=1&w=2 > > > > drivers/firmware/broadcom/bcm47xx_nvram.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/firmware/broadcom/bcm47xx_nvram.c b/drivers/firmware/broadcom/bcm47xx_nvram.c > > index 0c2f0a6..0b631e5 100644 > > --- a/drivers/firmware/broadcom/bcm47xx_nvram.c > > +++ b/drivers/firmware/broadcom/bcm47xx_nvram.c > > @@ -94,15 +94,14 @@ static int nvram_find_and_copy(void __iomem *iobase, u32 lim) > > > > found: > > __ioread32_copy(nvram_buf, header, sizeof(*header) / 4); > > - header = (struct nvram_header *)nvram_buf; > > - nvram_len = header->len; > > + nvram_len = ((struct nvram_header *)(nvram_buf))->len; > > I do not understand why this change is needed? Doesn't the old code do > exactly the same as the new one? > > The old code updated the header pointer and then accesses a member, the > new one directly accesses this member without updating this pointer. > > I assume, I am missing something. ;-) The goal is to access 'nvram_buf' which is a copy of 'header'. This is to avoid any problems with accessing device memory, i.e. 'header', without using the appropriate I/O accessors (readl, readw, readb). The bug that's being fixed though is to make sure 'header' doesn't get overwritten with the pointer to the in-memory copy that we just made. Further down in this function we copy the second 'header' that lives in device memory, and repointing 'header' to the in-memory copy breaks that. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project