From: Alan Cox <alan@redhat.com>

Reasoning


- Earlier in boot if we find a PCI bus we set raw_pci_ops to the function
  table to use for PCI accesses.
- When we enter this function we now check if we have such a method
- If not then we know it will otherwise crash because the call sequence
  through the code goes

        pci_irq_init
        pirq_peer_trick
        pci_scan_bus_parented
        which always leads down into the device scan and a pci config access
                via raw_pci_ops

  The moment the scan searches for a device it has to crash as it has no
  access methods.

- The other case which does nothing but pci_fixup_irqs is a no-op with no
  PCI devices

- THUS any situation that is changed by the raw_pci_ops check was
  previously an oops case anyway or did nothing anyway.


Tested with pci=off and without

It oopses during modprobe becasue the first load of acpiphp didn't clean up
properly.

Zhenmin's checker tool <zli4@cs.uiuc.edu> detected this:

 9. /drivers/pci/hotplug/shpchp_ctrl.c, Line 1575:
 err("%s: Failed to disable slot, error code(%d)\n", __FUNCTION__, rc);

 Maybe change to:
 err("%s: Failed to disable slot, error code(%d)\n", __FUNCTION__,
 retval);

 I think it is right because at line 1564, the slot is turned off, and
in this line (1575) is checked the status to see if we got an error; if
so, the error number is shown. This number is in 'retval', not in 'rc'
('rc' does have the return of configure_new_device()).

 The patch bellow fixes that:

Am Donnerstag, 6. Mai 2004 02:48 schrieb Sy, Dely L:
> On Fri, 30 Apr 2004 12:31:18 +0200, Rolf Eike Beer wrote:

>> -
>> -static int cleanup_slots (struct controller * ctrl)
>> +static void cleanup_slots(const struct controller *ctrl)
>> {
>> -	struct slot *old_slot, *next_slot;
>> +	struct slot *old_slot;
>>
>> 	old_slot = ctrl->slot;
>> 	ctrl->slot = NULL;
>>
>> 	while (old_slot) {
>> -		next_slot = old_slot->next;
>> -		pci_hp_deregister (old_slot->hotplug_slot);
>> -		kfree(old_slot->hotplug_slot->info);
>> -		kfree(old_slot->hotplug_slot->name);
>> -		kfree(old_slot->hotplug_slot);
>> -		kfree(old_slot);
>> -		old_slot = next_slot;
>> +		pci_hp_deregister(old_slot->hotplug_slot);
>> +		old_slot = old_slot->next;
>>  	}
>
> The variable next_slot and its assignment should be kept, for once
> pci_hp_deregister() is called and the release callback function, which
> you added, may come in and clean up old_slot structure before the next
> ptr is saved.  The code should be:
>
> static void cleanup_slots(const struct controller *ctrl)
> {
> 	struct slot *old_slot, *next_slot;
>
> 	old_slot = ctrl->slot;
>  	ctrl->slot = NULL;
>
>  	while (old_slot) {
> 		next_slot = old_slot->next;
> 		pci_hp_deregister (old_slot->hotplug_slot);
> 		old_slot = next_slot;
>  	}
> }

Yes, good point. Greg, this patch makes does it. Please apply on top of the
other stuff.

This 2 get_* functions do the same the hotplug core does if this functions
would not be there. Kill Bil^H^H^Hem!

Eike

slot_paranoia_check is only another kind of checking everything for NULL.
Removing this leads to function get_slot is reduced to a simple cast, so this
function can be killed also.

Eike

If the "struct hotplug_slot *" parameter is ever NULL then something bogus is
going on, we should not hide this bug by catching it. Also the name used in
debug messages is fixed.

Eike

Convert cpci_hotplug_core.c::cpci_hp_register_bus to use goto for error
handling.

Eike

This two checks are useless: cpci_hp_register_bus is only called from two
places, one has constant arguments, the other one passes the module
parameters which are checked for this condition on module load. And the bus
argument of cpci_hp_unregister_bus can never be NULL, all functions calling
this function use fields of the bus struct before so they will oops if it
would ever be NULL.

Eike

This one removes useless code and fixes the issue that the return code of
set_attention_status for cpcihp is always 0 even if cpci_set_attention_status
returns an error.

Eike

On Wed, May 05, 2004 at 03:31:02PM -0700, Greg KH wrote:
> On Thu, Apr 29, 2004 at 02:53:01PM -0500, Matt Domsch wrote:
> > a) need this be a warning, wouldn't KERN_DEBUG suffice, if a message
> > is needed at all?  This is printed in pci_generic_prep_mwi().
>
> Yes, we should make that KERN_DEBUG.  I don't have a problem with that.
> Care to make a patch?

Attached is a revised version of rpaphp.patch.  It has the following fixes:
- Set up slot->name
- Kill some dbgs
- Eike's fixes
- New fixes for incorrect "goto" in rpaphp_slot.c.

This fixes allyesconfig

Remove a useless cast: pci_add_new_bus returns a struct pci_bus*, so no need
to cast.

A big bunch of coding style fixes for shpchp_ctrl.c and shpchp_pci.c

Eike

Some small coding style fixes for shpchp_core.c.

Convert shpchp_core.c::init_slots to use goto for error handling.

shpchp is the only driver which does not use a release function for the slot
struct. This adds one and does some minor coding style fixes. Also no one
cares about the return value of cleanup_slots (which is always 0 anyway) so
we can make the function void.

shpchp_core.c::hardware_test is empty. If we remove it we tell the user that
hardware tests are not supported at all.

Convert shpchp_core.c to use new style of module handling.

Eike

Some coding style fixes for rpaphp_pci.c.

Convert rpaphp_slot.c::alloc_slot_struct to use goto for error handling. Also
some small coding style fixes.

Remove two useless NULL checks from rpaphp_core.c

rpaphp_core.c::init_slots is not more than a for loop and is called only from
one place, this inlines the important 3 lines. Als add some __init and __exit.

Some coding style fixes for rpaphp_core.c:
-s/return(foo)/return foo/
-some whitespace fixes
-document function in proper way

Eike

The get_cur_bus_speed function of rpaphp does nothing that the PCI Hotplug Core
would not do by itself if this function does not exist, so just kill it.

The debug parameter of rpaphp is only used as a boolean so we can scan the
commandline of it like a boolean parameter.

This is a bunch of coding style fixes (wrap long lines, whitspacing etc.) for
pciehp_pci.c

This patch does two things:
-remove casts of pointers which are void* or already the correct type for the
 target
-if we dereferenced a struct member and copied this to it's own variable use
 this and don't dereference the member again

Don't know why, but it looks like a good idea to mark this global variables
static.

Some coding style fixes and small cleanups for pciehp_core.c:
-wrap long lines
-kill spaces before opening braces of functions
-remove code duplication where both parts of an if statement do exactly the
 same
-kill some useless comments
-kill an unneeded initialisation

The result of kmalloc does not need to be casted to any other pointer type.
Also use kmalloc(*foo) instead of kmalloc(type_of_foo) and wrap some long
lines.

Some small coding style fixes and a typo fix for pciehp.h

This changes pciehp_core.c::init_slots to use goto for error hanling. Also a
missing magic missed by previous patches is killed.

Remove useless NULL checks from cpqphp_ctrl.c. Under normal circumstances
there is no chance for any of this functions to get called with a NULL
argument. If we are in such trouble that we get a NULL pointer don't hide it,
just oops.

The hardware_test function of the PCI Express Hotplug driver is empty. It's
better to completely kill this to tell the user hardware tests are not
supported by this driver.