Re: [PATCH/RFC] Hookable IO operations

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

> I think it would be helpful to not generate the function name itself,
> but pass it down to the macro, so you grep for the definition, either
> 
> DEF_MMIO_IN_BE(in_be16, u16, lwz)
> 
> or going further, making it look more like a declaration
> 
> static inline u16 DEF_MMIO_IN_BE(in_be16, u16, lwz)
> 
> which may even do the right thing with ctags (need to check that)

I'll look into it.

> > +/*
> > + * IO stream instructions are defined out of line
> > + */
> > +extern void _insb(volatile u8 __iomem *port, void *buf, long count);
> > +extern void _outsb(volatile u8 __iomem *port, const void *buf, long count);
> > +extern void _insw_ns(volatile u16 __iomem *port, void *buf, long count);
> > +extern void _outsw_ns(volatile u16 __iomem *port, const void *buf, long count);
> > +extern void _insl_ns(volatile u32 __iomem *port, void *buf, long count);
> > +extern void _outsl_ns(volatile u32 __iomem *port, const void *buf, long count);
> 
> If you leave out the 'extern', it fits into a normal line ;-)

Did I change them from the initial definition ? Anyway, I dislike
prototypes in headers without "extern".

> 
> similarly here, you could write these using slightly different macros
> as 
> 
> DEF_PCI_AC_OUT_MMIO(writeb, u8)
> DEF_PCI_AC_OUT_PIO(outb, u8)
> DEF_PCI_AC_OUT_MMIO(writew, u16)
> DEF_PCI_AC_OUT_PIO(outw, u16)

I'll look at it.

> > +
> > +/* We still use EEH versions for now. Ultimately, we might just get rid of
> > + * EEH in here and use it as a set of __memset_io etc... hooks
> > + */
> 
> Yes, I fully agree. The definition of eeh_memset_io and the others
> looks like they really want to be out-of-line.

Yes.

> > +
> > +extern void (*__memset_io)(volatile void __iomem *addr, int c,
> > +			   unsigned long n);
> > +extern void (*__memcpy_fromio)(void *dest, const volatile void __iomem *src,
> > +			       unsigned long n);
> > +extern void (*__memcpy_toio)(volatile void __iomem *dest, const void *src,
> > +			     unsigned long n);
> 
> Why are these all separate symbols? Wouldn't it be clearer to group them
> in ppc_md, or a similar structure of function pointers?

Because the IO ones are separate already and I want to keep things
consistent with them.

> This is the point where it gets completely unreadable. I ended up
> running the macros through 'gcc -E' to see what they do ;-)

Come on ... :-) They are almost the same as the ones in the .h, they
just only define the function pointer.

> If you resolve all the macros, you actually get fewer lines, and
> it suddenly becomes readable, as well as parsable with grep and ctags:
> 
> u8 (*__ind_readb) (const volatile void __iomem * addr);
> EXPORT_SYMBOL(__ind_readb);
> u8 (*__ind_inb) (unsigned long port);
> EXPORT_SYMBOL(__ind_inb);
> u16 (*__ind_readw) (const volatile void __iomem * addr);
> EXPORT_SYMBOL(__ind_readw);
> u16 (*__ind_inw) (unsigned long port);
> ...
> 
> I do the same when I start using macros, it's always hard to know exactly
> when to stop ;-)

I'll look and see what the result looks like.

> > -static u8 iSeries_Read_Byte(const volatile void __iomem *IoAddress)
> > +static u8 iseries_readb(const volatile void __iomem *IoAddress)
> 
> Since you're converting iSeries_Read_Byte from SilLycAps, shouldn't
> you change IoAddress to something more sensible at the same time?

No, I'm only changing the prototype & name to better match my hooks,
further cleanups of the content of these functions is something I'll do
in a separate patch.
 
> >  {
> >  	u64 BarOffset;
> >  	u64 dsa;
> > @@ -462,7 +411,8 @@ static u8 iSeries_Read_Byte(const volati
> >  			num_printed = 0;
> >  		}
> >  		if (num_printed++ < 10)
> > -			printk(KERN_ERR "iSeries_Read_Byte: invalid access at IO address %p\n", IoAddress);
> > +			printk(KERN_ERR "iSeries_Read_Byte: invalid access at IO address %p\n",
> > +			       IoAddress);
> >  		return 0xff;
> 
> And the name in the comment should also be changed.

Ooopsa.

Ben.