From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753138Ab2AWQFI (ORCPT ); Mon, 23 Jan 2012 11:05:08 -0500 Received: from mail-tul01m020-f174.google.com ([209.85.214.174]:62467 "EHLO mail-tul01m020-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751690Ab2AWQFG convert rfc822-to-8bit (ORCPT ); Mon, 23 Jan 2012 11:05:06 -0500 MIME-Version: 1.0 In-Reply-To: <20120121165830.GA9216@elte.hu> References: <1327021265-22184-1-git-send-email-matthew.r.wilcox@intel.com> <20120121082857.GC32134@elte.hu> <20120121165830.GA9216@elte.hu> Date: Tue, 24 Jan 2012 01:05:05 +0900 Message-ID: Subject: Re: [PATCH] NVMe: Fix compilation on architecturs without readq/writeq From: Hitoshi Mitake To: Ingo Molnar Cc: Linus Torvalds , Matthew Wilcox , Roland Dreier , Andrew Morton , James Bottomley , linux-kernel@vger.kernel.org, linux-nvme@infradead.org, hpa@linux.intel.com 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 On Sun, Jan 22, 2012 at 01:58, Ingo Molnar wrote: > > * Hitoshi Mitake wrote: > >> On Sat, Jan 21, 2012 at 17:28, Ingo Molnar wrote: >> > >> > * Linus Torvalds wrote: >> > >> >> On Thu, Jan 19, 2012 at 5:01 PM, Matthew Wilcox >> >> wrote: >> >> > The only places that uses readq/writeq are in the initialisation >> >> > path.  Since they're not performance critical, always use readl/writel. >> >> >> >> The arch rules are that i fthe architecture has readq/writeq, they >> >> will be #define'd (they may be inline functions, but there will be a >> >> >> >>   #define readq readq >> >> >> >> to make it visible to the preprocessor as well). >> >> >> >> So if you don't need the atomicity guarantees of a "real" readq, you >> >> can do this instead: >> >> >> >>   #ifndef readq >> >>   static inline u64 readq(void __iomem *addr) >> >>   { >> >>         return readl(addr) | (((u64) readl(addr + 4)) << 32LL); >> >>   } >> >>   #endif >> >> >> >> and then use readq() as if it existed. >> >> >> >> And I do think we should expose this in some generic manner. Because >> >> we currently don't, we already have that pattern copied in quite a few >> >> drivers. >> >> >> >> Maybe or something? Making it >> >> clear that its not atomic, but avoiding the silly duplication >> >> we do now.. >> >> >> >> This whole mess was introduced in commit dbee8a0affd5 ("x86: >> >> remove 32-bit versions of readq()/writeq()"), and it already >> >> talked about the problems but didn't help with the drivers >> >> that simply don't care. >> >> >> >> All the people in those threads were doing their >> >> self-satisfied "writeq is broken", without much acknowledging >> >> that 99% of users simply don't seem to care. >> >> >> >> "Occupy Writeq - We are the 99%" >> > >> > Agreed, and offering a generic facility for silly duplication >> > was the motivation of the original commit by Hitoshi Mitake. >> > >> > This: >> > >> > | 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 >> > |   ). >> > >> > is actually a mostly bogus statement and creates more problems >> > than it solves. >> > >> > Hitoshi-san, would you be interested in re-adding the generic >> > readq/writeq definitions in a slight variation to 2c5643b1c5, to >> > a separate io-nonatomic.h file, so that drivers that want it can >> > #include that file and be happy? >> >> It sounds nice. In the previous discussion, I suggested that >> chaning the name of non-atomic readq/writeq to >> readq_nonatomic/writeq_nonatomic. And James Bottomley >> replied that it is fine but not really very useful: >> >> https://lkml.org/lkml/2011/5/19/13 >> >> The idea of providing non-atomic readq/writeq in the new file >> with the name which express non-atomicity clearly might be >> able to satisfy both of safety and usefulness. >> >> It will reduce the duplication of the definition. In addition >> readq/writeq users don't have to type the long symbols with >> _nonatomic suffix and can know non-atomicity from the name >> of header file. >> >> I'd like to hear opinions from James, Roland and folks who >> dislike non-atomic readq/writeq. > > Drivers that want the definition add this line to their driver: > > #include > > and then readq()/writeq() does the obvious thing. No need for > readq_nonatomic()/writeq_nonatomic() - that extra line declares > things clearly enough and cannot be added accidentally. > I wrote the patch which adds the new file include/asm-generic/io-nonatomic.h. io-nonatomic.h provides non-atomic version readq()/writeq(). The patch also removes some duplicated readq()/writeq() definitions which are added in the commit: dbee8a0affd5e6eaa5d7c816c4bc233f6f110f50. The commit was made by Roland Dreier and removed readq()/writeq() from arch/x86/include/asm.h. If I find no error after the building allyesconfig and allmodconfig test, I'll send the patch for review. This may take a few hour. BTW, I placed io-nonatomic.h under include/asm-generic/ because non-atomic version readq()/writeq() is architecture dependent. If you have comments on this point, I'd like to hear. Thanks, -- Hitoshi Mitake h.mitake@gmail.com