On Mon, Jan 03, 2022 at 12:35:45PM +0100, Niklas Schnelle wrote:
> On Fri, 2021-12-31 at 12:15 -0500, Alan Stern wrote:
> > On Fri, Dec 31, 2021 at 12:06:24PM +0100, Niklas Schnelle wrote:
> > > On Mon, 2021-12-27 at 15:36 -0500, Alan Stern wrote:
> > > > On Mon, Dec 27, 2021 at 05:43:16PM +0100, Niklas Schnelle wrote:
> > > > > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > > > > index 8ae5ccd26753..8e30116b6fd2 100644
> > > > > --- a/drivers/usb/host/uhci-hcd.h
> > > > > +++ b/drivers/usb/host/uhci-hcd.h
> > > > > @@ -586,12 +586,14 @@ static inline int uhci_aspeed_reg(unsigned int reg)
> > > > >
> > > > > static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
> > > > > {
> > > > > +#ifdef CONFIG_HAS_IOPORT
> > > > > if (uhci_has_pci_registers(uhci))
> > > > > return inl(uhci->io_addr + reg);
> > > > > - else if (uhci_is_aspeed(uhci))
> > > > > +#endif
> > > >
> > > > Instead of making all these changes (here and in the hunks below), you
> > > > can simply modify the definition of uhci_has_pci_registers() so that it
> > > > always gives 0 when CONFIG_HAS_IOPORT is N.
> > > >
> > > > Alan Stern
> > >
> > > I don't think that works, for example in the hunk you quoted returning
> > > 0 from uhci_has_pci_registers() only skips over the inl() at run-time.
> > > We're aiming to have inl() undeclared if HAS_IOPORT is unset though.
> >
> > I see. Do you think the following would be acceptable? Add:
> >
> > #ifdef CONFIG_HAS_IOPORT
> > #define UHCI_IN(x) x
> > #define UHCI_OUT(x) x
> > #else
> > #define UHCI_IN(x) 0
> > #define UHCI_OUT(x)
> > #endif
> >
> > and then replace for example inl(uhci->io_addr + reg) with
> > UHCI_IN(inl(uhci->io_addr + reg)).
>
> In principle that looks like a valid approach. Not sure this is better
> than explicit ifdefs though.
The general preference in the kernel is to avoid sprinkling #ifdef's
throughout function definitions, and instead encapsulate their effects
with macros or inline functions -- like this.
> With this approach one could add
> UHCI_IN()/UHCI_OUT() calls which end up as nops without realizing it as
> it would disable any compile time warning for using them without
> guarding against CONFIG_HAS_IOPORT being undefined.
To help prevent that, we can add
#undef UHCI_IN
#undef UHCI_OUT
at the end of this section.
> > The definition of uhci_has_pci_registers() should be updated in any
> > case; there's no reason for it to do a runtime check of uhci->io_addr
> > when HAS_IOPORT is disabled.
>
> Agree. Interestingly same as with the "if
> (IS_ENABLED(CONFIG_HAS_IOPORT))" it seems having
> uhci_has_pci_registers() compile-time defined to 0 (I added a
> defined(CONFIG_HAS_IOPORT) to it) makes the compiler ignore the missing
> inl() decleration already. But I'm not sure if we should rely on that.
I definitely would not rely on it.
Alan Stern
