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:
> >
> > > > #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));
> > > > }
> > > >
> > > > static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
> > > > {
> > > > - outl(val, uhci->io_addr + reg);
> > > > + UHCI_OUT(outl(val, uhci->io_addr + reg));
> > >
> > > I'm confused now.
> > >
> > > So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
> > >
> > > 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?
> > >
> > > Why not just not build the driver at all if this option is not enabled?
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 reg)
{
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.
>
> > That said, there is a minor problem with the empty definition
> >
> > +#define UHCI_OUT(x)
> >
> > I think this should be "do { } while (0)" to avoid warnings
> > about empty if/else blocks.
>
> I'm sure Niklas wouldn't mind making such a change. But do we really
> get such warnings? Does the compiler really think that this kind of
> (macro-expanded) code:
>
> if (uhci_has_pci_registers(uhci))
> ;
> else if (uhci_is_aspeed(uhci))
> writel(val, uhci->regs + uhci_aspeed_reg(reg));
>
> deserves a warning? I write stuff like that fairly often; it's a good
> way to showcase a high-probability do-nothing pathway at the start of a
> series of conditional cases. And I haven't noticed any complaints from
> the compiler.
>
> 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
