Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <33f6bd2277d2bdc5c5455c2987f479c3b2cd554d.camel@linux.ibm.com>
Subject: Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Alan Stern <stern@rowland.harvard.edu>,
        Arnd Bergmann <arnd@arndb.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-kernel@vger.kernel.org,
        Linux-Arch <linux-arch@vger.kernel.org>,
        linux-pci@vger.kernel.org, Arnd Bergmann <arnd@kernel.org>,
        linux-usb@vger.kernel.org
Date:   Wed, 17 May 2023 10:29:21 +0200
In-Reply-To: <23936929-80e4-4599-827a-d09b4960f3ab@rowland.harvard.edu>
References: <20230516110038.2413224-1-schnelle@linux.ibm.com>
         <20230516110038.2413224-36-schnelle@linux.ibm.com>
         <2023051643-overtime-unbridle-7cdd@gregkh>
         <4e291030-99d9-4b8b-9389-9b8f2560b8e8@app.fastmail.com>
         <23936929-80e4-4599-827a-d09b4960f3ab@rowland.harvard.edu>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.1 (3.48.1-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-05-16 at 15:51 -0400, Alan Stern wrote:
> On Tue, May 16, 2023 at 06:44:34PM +0200, Arnd Bergmann wrote:
> > On Tue, May 16, 2023, at 18:29, Greg Kroah-Hartman wrote:
> > > On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
> >=20
> > > >  #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
> > > >  /* Support PCI only */
> > > >  static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
> > > >  {
> > > > -	return inl(uhci->io_addr + reg);
> > > > +	return UHCI_IN(inl(uhci->io_addr + reg));
> > > >  }
> > > > =20
> > > >  static inline void uhci_writel(const struct uhci_hcd *uhci, u32 va=
l, int reg)
> > > >  {
> > > > -	outl(val, uhci->io_addr + reg);
> > > > +	UHCI_OUT(outl(val, uhci->io_addr + reg));
> > >=20
> > > I'm confused now.
> > >=20
> > > So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
> > >=20
> > > But if it isn't, then these are just no-ops that do nothing?  So then
> > > the driver will fail to work?  Why have these stubs at all?
> > >=20
> > > Why not just not build the driver at all if this option is not enable=
d?

The driver supports multiple access methods in several functions
similar to the following:

static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int re=
g)
{
	if (uhci_has_pci_registers(uhci))
		UHCI_OUT(outl(val, uhci->io_addr + reg));
	else if (uhci_is_aspeed(uhci))
		writel(val, uhci->regs + uhci_aspeed_reg(reg));
#ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
	else if (uhci_big_endian_mmio(uhci))
		writel_be(val, uhci->regs + reg);
#endif
	else
		writel(val, uhci->regs + reg);
}

Instead of adding more #ifdefs Alan Stern suggested to just stub out
both uhci_has_pci_registers() and the access itself. So with a half way
optimizing compiler this shouldn't even leave no-ops in the binary.


>=20
> > That said, there is a minor problem with the empty definition
> >=20
> > +#define UHCI_OUT(x)
> >=20
> > I think this should be "do { } while (0)" to avoid warnings
> > about empty if/else blocks.
>=20
> I'm sure Niklas wouldn't mind making such a change.  But do we really=20
> get such warnings?  Does the compiler really think that this kind of=20
> (macro-expanded) code:
>=20
> 	if (uhci_has_pci_registers(uhci))
> 		;
> 	else if (uhci_is_aspeed(uhci))
> 		writel(val, uhci->regs + uhci_aspeed_reg(reg));
>=20
> deserves a warning?  I write stuff like that fairly often; it's a good=
=20
> way to showcase a high-probability do-nothing pathway at the start of a=
=20
> series of conditional cases.  And I haven't noticed any complaints from=
=20
> the compiler.
>=20
> Alan Stern

I changed it to "do {} while (0)" for v5 but agree I haven't seen
warnings for this either. Still doesn't hurt.

Thanks,
Niklas