[Linux-ia64] RE: PCI Hotplug Drivers for 2.5

["Lee, Jung-Ik" <jung-ik.lee@intel.com>]

Hi Greg,

My answers are next to your comments.

thanks,
J.I.


> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, October 23, 2002 10:10 PM
> To: Lee, Jung-Ik
> Cc: 'KOCHI, Takayoshi'; Luck, Tony;
> pcihpd-discuss@lists.sourceforge.net; linux ia64 kernel list;
> linux-kernel
> Subject: Re: PCI Hotplug Drivers for 2.5
> 
> 
> On Wed, Oct 23, 2002 at 09:33:09PM -0700, Lee, Jung-Ik wrote:
> > Greg,
> > 
> > Please find the attached ACPI based PCI Hotplug driver.
> 
> But the code you sent has all of the ACPI stuff not enabled, right?

ACPI is a must for PHP enumeration/configuration and resource management for
DIG64/ACPI compliant platforms. ACPI method is optional for controller/slot
operations(event management). intcphp driver conforms to ACPI resource
management. I didn't enable ACPI based event management for PHP since it is
optional and provides less feature than controller based solution - LED,
MRL, Bus/Adapter speeds/capabilities, etc.

> 
> As an example from your patch:
> 
> +enum php_ctlr_type phphpc_get_ctlr_type()
> +{
> +       return PCI;
> +}
> 
> It never returns any other type, so the ACPI or ISA sections of the
> driver will never get called.  Or am I missing something?
> 
This is because this release only addresses PCI version only. I can take
this out too, with ease :)

> >  intcphp:
> >     Php driver source for Compaq or compatible Intel Hotplug
> >     controllers on IA32 or DIG64-ACPI compliant IA64 platforms.
> 
> So this overloads the current Compaq driver?  It looks like this "new"
> driver will also handle all of the same controllers the current Compaq
> driver does, right?  If not, it sure looks like you are 
> accepting all of
> the same PCI ID values :)
> 
Probably, as PCI ID indicates :)

> >     intcphp driver is overhauled per your requirements:
> >     + Abstraction module is removed.
> >       It's now two modules driver like others.
> 
> Thank you for making this change, I appreciate it.
> 
> >     + typedefs are removed except callback function.
> 
> Thanks.
> 
> >     + LINUX_VERSION checks are removed.
> 
> And replaced with the odd BEFORE_2_5 check :)
> Please just rip these out and send a version that is only for the 2.5
> kernel.
> 
Ah, you noticed it.. OK (pain in my heart :))

> Some of your #ifdef CONFIG_IA64 should be moved to header files only
> (and probably documented why you really need to sleep extra 
> amounts for
> ia64 machines only.
> 
> What's the #ifdef WORK_QUEUE for?

Not used. Will take out.

> 
> > 	intcphp is much based on cpqphp driver but has been 
> modified to be
> > controller independent on DIG64/ACPI compliant IPF servers 
> as well as
> > non-ACPI based IA32 servers. Thus code looks similar but 
> integration is not
> > that easy and will take time and consents of affected 
> drivers owners.
> 
> The code looks _very_ similar.  In fact, at first glance it looks like
> almost a straight copy of the existing Compaq code.  Why not 
> just submit
> a patch against that driver that adds the extra functionality that you
> need for your hardware?  That would be much smaller, and decrease the
> amount of duplicated code in the kernel tree.
> 
> Also, why doesn't the ACPI PCI hotplug driver work for your machines?
> I've seen it work on a very wide range of processors (i386 and ia64),
> and manufacturers, and any specific issues with your hardware would
> probably be better addressed with patches to the existing ACPI driver.
> 
With some reasons such as what Tak described.

> > 	We understand there needs more integration and cleanup to make
> > common codes to pci_hotplug core as you indicated. This 
> task, however,
> > requires time and changes in every php driver with owners' 
> consensus on
> > common php controller/slot objects, while satisfying 
> requirements in the
> > near future. We look forward to discussing this with you and other
> > contributors.
> 
> Great, I do too.  Please, make a proposal about what to merge into the
> core.  I do NOT want to see another driver have to duplicate the PCI
> resource management code again without a very good reason for 
> doing it.
> 
> > 	Until then, please allow us to co-exist this driver.
> 
> There's no rush, let's work together to get this done properly.
> 
We need this driver as it's the only solution for DIG64 compliant IPF
platforms.
Can we work in parallel ? - Make this driver available and we all work
together for enhanced pci_hotplug core. Also I'll talk with cpqphp owner
over the integration of the two.

> thanks,
> 
> greg k-h
>