Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934815Ab1ETBPu (ORCPT ); Thu, 19 May 2011 21:15:50 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:52113 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933379Ab1ETBPs convert rfc822-to-8bit (ORCPT ); Thu, 19 May 2011 21:15:48 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=qHAkx82MPlBFrFXb8JTHKPdN36Lj2tRwvUi63+a++snMi/KwcVl5qaObTc+xhJVXlu 3ahdgBHyo7sfdQVQgrFjbj1f03I682gTTm8Fwt2SeRrXyoZNXLjhCxSM4K0t49krmwgZ JLntVn1jWnDHV3KjaqPO4tcgVRs7eWTk99sPs= MIME-Version: 1.0 In-Reply-To: <1305849293-25437-1-git-send-email-roland@kernel.org> References: <20110519181500.GF6139@elte.hu> <1305849293-25437-1-git-send-email-roland@kernel.org> Date: Fri, 20 May 2011 10:15:46 +0900 Message-ID: Subject: Re: [PATCH] x86: Remove 32-bit versions of readq()/writeq() From: Hitoshi Mitake To: Roland Dreier Cc: Ingo Molnar , linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, Benjamin Herrenschmidt , Milton Miller , James Bottomley , Kashyap Desai , Len Brown , Ravi Anand , Vikas Chaudhary , Matthew Garrett Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8502 Lines: 272 On Fri, May 20, 2011 at 08:54, Roland Dreier wrote: > From: Roland Dreier > > The presense of a writeq() implementation on 32-bit x86 that splits > the 64-bit write into two 32-bit writes turns out to break the mpt2sas > driver (and in general is risky for drivers as was discussed in > ). ?To fix this, > revert 2c5643b1c5c7 ("x86: provide readq()/writeq() on 32-bit too") > and follow-on cleanups. > > This unfortunately leads to pushing non-atomic definitions of readq() > and write() to various x86-only drivers that in the mean time started > using the definitions in the x86 version of . ?However as > discussed exhaustively, this is actually the right thing to do, > because the right way to split a 64-bit transaction is hardware > dependent and therefore belongs in the hardware driver (eg mpt2sas > needs a spinlock to make sure no other accesses occur in between the > two halves of the access). > > Build tested on 32- and 64-bit x86 allmodconfig. > > Link: http://lkml.kernel.org/r/x86-32-writeq-is-broken@mdm.bga.com > Cc: Hitoshi Mitake > Cc: Kashyap Desai > Cc: Len Brown > Cc: Ravi Anand > Cc: Vikas Chaudhary > Cc: Matthew Garrett > Signed-off-by: Roland Dreier I should ack this patch based on the detailed explanation from guys in the previous thread. Acked-by: Hitoshi Mitake Thanks for your cleaning drivers, Roland! > --- > ?arch/x86/Kconfig ? ? ? ? ? ? ? ? | ? ?2 -- > ?arch/x86/include/asm/io.h ? ? ? ?| ? 24 ++---------------------- > ?drivers/acpi/apei/einj.c ? ? ? ? | ? ?8 ++++++++ > ?drivers/acpi/atomicio.c ? ? ? ? ?| ? ?4 ++++ > ?drivers/edac/i3200_edac.c ? ? ? ?| ? 13 +++++++++++++ > ?drivers/platform/x86/ibm_rtl.c ? | ? 13 +++++++++++++ > ?drivers/platform/x86/intel_ips.c | ? 13 +++++++++++++ > ?drivers/scsi/qla4xxx/ql4_nx.c ? ?| ? 21 +++++++++++++++++++++ > ?8 files changed, 74 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index cc6c53a..ceb41f3 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -16,8 +16,6 @@ config X86_64 > ?config X86 > ? ? ? ?def_bool y > ? ? ? ?select HAVE_AOUT if X86_32 > - ? ? ? select HAVE_READQ > - ? ? ? select HAVE_WRITEQ > ? ? ? ?select HAVE_UNSTABLE_SCHED_CLOCK > ? ? ? ?select HAVE_IDE > ? ? ? ?select HAVE_OPROFILE > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h > index 0722730..d02804d 100644 > --- a/arch/x86/include/asm/io.h > +++ b/arch/x86/include/asm/io.h > @@ -38,7 +38,6 @@ > > ?#include > ?#include > -#include > ?#include > > ?#include > @@ -87,27 +86,6 @@ build_mmio_write(__writel, "l", unsigned int, "r", ) > ?build_mmio_read(readq, "q", unsigned long, "=r", :"memory") > ?build_mmio_write(writeq, "q", unsigned long, "r", :"memory") > > -#else > - > -static inline __u64 readq(const volatile void __iomem *addr) > -{ > - ? ? ? const volatile u32 __iomem *p = addr; > - ? ? ? u32 low, high; > - > - ? ? ? low = readl(p); > - ? ? ? high = readl(p + 1); > - > - ? ? ? return low + ((u64)high << 32); > -} > - > -static inline void writeq(__u64 val, volatile void __iomem *addr) > -{ > - ? ? ? writel(val, addr); > - ? ? ? writel(val >> 32, addr+4); > -} > - > -#endif > - > ?#define readq_relaxed(a) ? ? ? readq(a) > > ?#define __raw_readq(a) ? ? ? ? readq(a) > @@ -117,6 +95,8 @@ static inline void writeq(__u64 val, volatile void __iomem *addr) > ?#define readq ? ? ? ? ? ? ? ? ?readq > ?#define writeq ? ? ? ? ? ? ? ? writeq > > +#endif > + > ?/** > ?* ? ? virt_to_phys ? ?- ? ? ? map virtual addresses to physical > ?* ? ? @address: address to remap > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c > index 096aebf..f74b2ea 100644 > --- a/drivers/acpi/apei/einj.c > +++ b/drivers/acpi/apei/einj.c > @@ -101,6 +101,14 @@ static DEFINE_MUTEX(einj_mutex); > > ?static struct einj_parameter *einj_param; > > +#ifndef writeq > +static inline void writeq(__u64 val, volatile void __iomem *addr) > +{ > + ? ? ? writel(val, addr); > + ? ? ? writel(val >> 32, addr+4); > +} > +#endif > + > ?static void einj_exec_ctx_init(struct apei_exec_context *ctx) > ?{ > ? ? ? ?apei_exec_ctx_init(ctx, einj_ins_type, ARRAY_SIZE(einj_ins_type), > diff --git a/drivers/acpi/atomicio.c b/drivers/acpi/atomicio.c > index 542e539..7489b89 100644 > --- a/drivers/acpi/atomicio.c > +++ b/drivers/acpi/atomicio.c > @@ -280,9 +280,11 @@ static int acpi_atomic_read_mem(u64 paddr, u64 *val, u32 width) > ? ? ? ?case 32: > ? ? ? ? ? ? ? ?*val = readl(addr); > ? ? ? ? ? ? ? ?break; > +#ifdef readq > ? ? ? ?case 64: > ? ? ? ? ? ? ? ?*val = readq(addr); > ? ? ? ? ? ? ? ?break; > +#endif > ? ? ? ?default: > ? ? ? ? ? ? ? ?return -EINVAL; > ? ? ? ?} > @@ -307,9 +309,11 @@ static int acpi_atomic_write_mem(u64 paddr, u64 val, u32 width) > ? ? ? ?case 32: > ? ? ? ? ? ? ? ?writel(val, addr); > ? ? ? ? ? ? ? ?break; > +#ifdef writeq > ? ? ? ?case 64: > ? ? ? ? ? ? ? ?writeq(val, addr); > ? ? ? ? ? ? ? ?break; > +#endif > ? ? ? ?default: > ? ? ? ? ? ? ? ?return -EINVAL; > ? ? ? ?} > diff --git a/drivers/edac/i3200_edac.c b/drivers/edac/i3200_edac.c > index d41f900..aa08497 100644 > --- a/drivers/edac/i3200_edac.c > +++ b/drivers/edac/i3200_edac.c > @@ -101,6 +101,19 @@ struct i3200_priv { > > ?static int nr_channels; > > +#ifndef readq > +static inline __u64 readq(const volatile void __iomem *addr) > +{ > + ? ? ? const volatile u32 __iomem *p = addr; > + ? ? ? u32 low, high; > + > + ? ? ? low = readl(p); > + ? ? ? high = readl(p + 1); > + > + ? ? ? return low + ((u64)high << 32); > +} > +#endif > + > ?static int how_many_channels(struct pci_dev *pdev) > ?{ > ? ? ? ?unsigned char capid0_8b; /* 8th byte of CAPID0 */ > diff --git a/drivers/platform/x86/ibm_rtl.c b/drivers/platform/x86/ibm_rtl.c > index 94a114a..b1396e5 100644 > --- a/drivers/platform/x86/ibm_rtl.c > +++ b/drivers/platform/x86/ibm_rtl.c > @@ -81,6 +81,19 @@ static void __iomem *rtl_cmd_addr; > ?static u8 rtl_cmd_type; > ?static u8 rtl_cmd_width; > > +#ifndef readq > +static inline __u64 readq(const volatile void __iomem *addr) > +{ > + ? ? ? const volatile u32 __iomem *p = addr; > + ? ? ? u32 low, high; > + > + ? ? ? low = readl(p); > + ? ? ? high = readl(p + 1); > + > + ? ? ? return low + ((u64)high << 32); > +} > +#endif > + > ?static void __iomem *rtl_port_map(phys_addr_t addr, unsigned long len) > ?{ > ? ? ? ?if (rtl_cmd_type == RTL_ADDR_TYPE_MMIO) > diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c > index 85c8ad4..5ffe7c3 100644 > --- a/drivers/platform/x86/intel_ips.c > +++ b/drivers/platform/x86/intel_ips.c > @@ -344,6 +344,19 @@ struct ips_driver { > ?static bool > ?ips_gpu_turbo_enabled(struct ips_driver *ips); > > +#ifndef readq > +static inline __u64 readq(const volatile void __iomem *addr) > +{ > + ? ? ? const volatile u32 __iomem *p = addr; > + ? ? ? u32 low, high; > + > + ? ? ? low = readl(p); > + ? ? ? high = readl(p + 1); > + > + ? ? ? return low + ((u64)high << 32); > +} > +#endif > + > ?/** > ?* ips_cpu_busy - is CPU busy? > ?* @ips: IPS driver struct > diff --git a/drivers/scsi/qla4xxx/ql4_nx.c b/drivers/scsi/qla4xxx/ql4_nx.c > index 35381cb..03e522b 100644 > --- a/drivers/scsi/qla4xxx/ql4_nx.c > +++ b/drivers/scsi/qla4xxx/ql4_nx.c > @@ -655,6 +655,27 @@ static int qla4_8xxx_pci_is_same_window(struct scsi_qla_host *ha, > ? ? ? ?return 0; > ?} > > +#ifndef readq > +static inline __u64 readq(const volatile void __iomem *addr) > +{ > + ? ? ? const volatile u32 __iomem *p = addr; > + ? ? ? u32 low, high; > + > + ? ? ? low = readl(p); > + ? ? ? high = readl(p + 1); > + > + ? ? ? return low + ((u64)high << 32); > +} > +#endif > + > +#ifndef writeq > +static inline void writeq(__u64 val, volatile void __iomem *addr) > +{ > + ? ? ? writel(val, addr); > + ? ? ? writel(val >> 32, addr+4); > +} > +#endif > + > ?static int qla4_8xxx_pci_mem_read_direct(struct scsi_qla_host *ha, > ? ? ? ? ? ? ? ?u64 off, void *data, int size) > ?{ > -- Hitoshi Mitake h.mitake@gmail.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/