Re: Latest Altix I/O code reorganization code

Christoph Hellwig
2004-09-03T23:40:24Z

Crosscheck of your new code vs the review:

> io_init.c:
>
> - sal_get_hubdev_info
> umm, you're getting a kernel pointer by a SAL
> call. I wonders how this is supposed to work when the kernel
> changes it's VA layout. (Dito for a bunch of other functions)

Still not explanation

> - sn_io_init
> what's going on here?

gone

> iomv.c:
>
> - okay, could use some reformatting to fit 80 chars

ok

> pci_dma.c:
>
> - still using all those indirections althoug there's only a single backend

this comment is fixed, but the one later sent in private (stupid choice
of interface beteen upper pci dma code and pcibr code ignored)

> pci_extension.c:
>
> - dito. Why does this single function need a separate file?

Not addressed. In general your file organization is mess still.

Just do arch/ia64/sn/pci/ for all pci code and move the rest to
arch/ia64/sn/kernel. That how most arches work.

> pcibr_ate.c:
>
> - doesn't look to bad, but should probably merged into pcibr_dma.c. And
> the trivial < 10 line function opencoded in their only callers.

Not addressed or commented on yet.

> pcibr_provider.c
>
> actual code code looks okay, but:
>
> - sal_pcibr_slot_enable/sal_pcibr_slot_disabl are completely unused

you still have typedefs for these around

> - the pci_provider abstraction is right now completely useless, please
> add such abstractions only when you need them (and if you'll ever need
> that a few hints: stop that casting of methods silliness but use
> container_of, use C99 initializers, stop the typedef abuse)

ok

> - request_irq return value needs checking

ok

>
> pcibr_reg.c:
>
> - all this casting is rather horrible. At least keep a pointer to each
> of struct tiocp and pic_s (and kill the _s postifx) in syruct pcibus_info.
> the volatiles looks bogus, if you need it you're missing memorry barries.

the type pointer isn't done. Any specific reason?

> xtalk_providers.c:
>
> - bogus indirection again. if you actually do have different lowlevel
> implementations they should be hidden inside the prom. While at it I think
> the two calls could just move to arch/ia64/sn/kernel/irq.c

ok

More items:

- sal_pcibr_rrb_alloc should be merged into its only caller
- io_init.c still has KDB ifdefs
- sn_pci_set_vchan should absolutely _not_ be placed in qla1280.c,
and while you're at it there extern for snia_pcibr_rrb_alloc should
move to a header
- kill HUBREG_CAST instead of touching it
- please don't put your ASSERT into asm/ headers. In general you
should use BUG_ON
- why do you add a pfn_t typedef that's not used anywhere
- the patch adds include/asm-ia64/sn/xtalk/xtalk_provider.h but there's
no more xtalk providers

Your headers are still a complete mess. There's no point exposing tons
of defails about the pci interface in include/asm - just keep and
pci_extensions.h there for non-standard PCI APIs, and the rest should
stay private inisde arch/ia64/sn/pci/.