From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com ([192.55.52.115]:34363 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161511AbeCAUVq (ORCPT ); Thu, 1 Mar 2018 15:21:46 -0500 Message-ID: <1519935703.10722.375.camel@linux.intel.com> Subject: Re: [PATCH v4 1/2] r8169: Dereference MMIO address immediately before use From: Andy Shevchenko To: Heiner Kallweit , nic_swsd@realtek.com, "David S . Miller" , netdev@vger.kernel.org Date: Thu, 01 Mar 2018 22:21:43 +0200 In-Reply-To: References: <20180301112735.28822-1-andriy.shevchenko@linux.intel.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On Thu, 2018-03-01 at 21:01 +0100, Heiner Kallweit wrote: > Am 01.03.2018 um 12:27 schrieb Andy Shevchenko: > > Next step might be a conversion of RTL_Wxx() / RTL_Rxx() macros > > to inline functions for sake of type checking. ^^^^^ > > /* write/read MMIO register */ > > -#define RTL_W8(reg, val8) writeb ((val8), ioaddr + (reg)) > > -#define RTL_W16(reg, val16) writew ((val16), ioaddr + (reg)) > > -#define RTL_W32(reg, val32) writel ((val32), ioaddr + (reg)) > > -#define RTL_R8(reg) readb (ioaddr + (reg)) > > -#define RTL_R16(reg) readw (ioaddr + (reg)) > > -#define RTL_R32(reg) readl (ioaddr + (reg)) > > +#define RTL_W8(tp, reg, val8) writeb((val8), tp->mmio_addr + > > (reg)) > > +#define RTL_W16(tp, reg, val16) writew((val16), tp- > > >mmio_addr + (reg)) > > +#define RTL_W32(tp, reg, val32) writel((val32), tp- > > >mmio_addr + (reg)) > > +#define RTL_R8(tp, reg) readb(tp->mmio_addr + (reg)) > > +#define RTL_R16(tp, reg) readw(tp->mmio_addr + > > (reg)) > > +#define RTL_R32(tp, reg) readl(tp->mmio_addr + > > (reg)) > I like te idea because I also found the frequent definition of > variable ioaddr > w/o explicit use quite ugly. Just instead of using macro's we could > define > RTL_R32 et al as inline functions. I left a last paragraph in my commit message. I wouldn't really to do a functional change right now. It may be easily done as simple followup, plain to test, etc. > In parallel to the readl API there's the ioread32() API. Even though I > read > somewhere that the readl API is a legacy API, I don't have an idea > which one > to prefer and why. You don't need ioread*(). Or do you have an example of hardware that uses I/O instead of MMIO? Rather some _relaxed variants might be useful. -- Andy Shevchenko Intel Finland Oy