Re: 2.6.12-rc1-mm1: Kernel BUG at pci:389

[Li Shaohua <shaohua.li@intel.com>]

On Tue, 2005-03-22 at 09:35, Pavel Machek wrote:
> Hi!
> 
> > > and that says:
> > > 
> > > #define PMSG_FREEZE     ((__force pm_message_t) 3)
> > > 
> > > ... I certainly have _FREEZE defined as 1 in my local tree, but I
> do
> > > not see that change in -mm yet.
> > 
> > Both 2.6.12-rc1-mm1 and 2.6.12-rc1 have:
> > 
> > #define PMSG_FREEZE     ((__force pm_message_t) 3)
> > #define PMSG_SUSPEND    ((__force pm_message_t) 3)
> > #define PMSG_ON         ((__force pm_message_t) 0)
> > 
> > which looks odd.
> 
> Yes, but it is needed. There are many drivers, and they look at
> numerical value of PMSG_*. I'm proceeding in steps. I hopefully killed
> all direct accesses to the constants, and will switch constants to
> something else... But that is going to be tommorow (need some sleep).
The patches are going to acquire correct PCI device sleep state for
suspend/resume. We discussed the issue several months ago. My plan is we
first introduce 'platform_pci_set_power_state', then merge the
'platform_pci_choose_state' patch after Pavel's pm_message_t conversion
finished. Maybe Len mislead my comments. 

Anyway for the callback, my intend is platform_pci_choose_state accept
the pm_message_t parameter, and it return an 'int', since platform
method possibly failed and then pci_choose_state translate the return
value to pci_power_t.

> > > I reproduced it here.. I do not know who introduced
> > > platform_pci_choose_state, but it is *very* wrong. It returns
> > > it. Should it return pci_power_t? It probably should to match
> > > pci_choose_state, but that int is retyped to pm_message_t. Oops.
> > 
> > That change came from Len.  I've appended the two relevant patches
> below.
> > 
> > So hm.  We have incompatible changes in flight.  That doesn't happen
> very
> > often.
> > 
> > Could I suggest that you prepare a fixup against 2.6.12-rc1-mm1 and
> send
> > that to Len and myself?  If that fixup is not suitable for a
> 2.6.12-rc1
> > based tree then I can look after it until things get flushed out.
> 
> Could you just revert those two patches? First one is very
> wrong. Second one might be fixed, but... See comments below.
I think the platform_pci_set_power_state should be ok, did you see it
causes oops?

> 
> And they are both "dangerous" -- they introduce new and untested
> functionality while I'm trying to transition from int to
> pm_message_t. They also affect all the drivers.
> 
> Len, please Cc me on patches that affect suspend.
> 
> > @@ -17,6 +17,7 @@
> >  #include <acpi/acpi_bus.h>
> >  
> >  #include <linux/pci-acpi.h>
> > +#include "pci.h"
> 
> 
> Should be <linux/pci.h>?
I suppose it's not exported out side of PCI, so I used 'pci.h' 

> 
> > +static int acpi_pci_choose_state(struct pci_dev *pdev, pm_message_t
> state)
> > +{
> 
> Should return pci_power_t, probably.
Should return int as I said above.

> 
> > +     char dstate_str[] = "_S0D";
> > +     acpi_status status;
> > +     unsigned long val;
> > +     struct device *dev = &pdev->dev;
> > +
> > +     /* Fixme: the check is wrong after pm_message_t is a struct */
> 
> Exactly.
> 
> > +     if ((state >= PM_SUSPEND_MAX) || !DEVICE_ACPI_HANDLE(dev))
> 
> PM_SUSPEND_MAX and friends is going to disappear.
Yep, this should be fixed. 

> 
> > +             return -EINVAL;
> > +     dstate_str[2] += state; /* _S1D, _S2D, _S3D, _S4D */
> 
> Ugh, assumes numerical values of states actually meaning anything. It
> definitely should not. Should be switch(state.event), but that code
> is not merged, yet.... => I'll send code that switches pm_message_t to
> struct, tommorow. But it may compile-time break some obscure
> drivers...
> 
> > diff -Nru a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> > --- a/drivers/pci/pci-acpi.c  2005-03-21 17:02:38 -08:00
> > +++ b/drivers/pci/pci-acpi.c  2005-03-21 17:02:38 -08:00
> > @@ -253,6 +253,24 @@
> >       return -ENODEV;
> >  }
> >  
> > +static int acpi_pci_set_power_state(struct pci_dev *dev,
> pci_power_t state)
> > +{
> > +     acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
> > +     static int state_conv[] = {
> > +             [0] = 0,
> > +             [1] = 1,
> > +             [2] = 2,
> > +             [3] = 3,
> > +             [4] = 3
> > +     };
> > +     int acpi_state = state_conv[(int __force) state];
> 
> The table should be
>                 [PCI_D0] = 0,
> ...
Ok, please revert the 'platform_pci_choose_pci' patch, I will add it
after Pavel's conversion is finished. Or after Pavel's is done, I can
send a quick fix.

Thanks,
Shaohua