Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

> > > Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
> > > regular devices and seem to ignore the fact that for bridge PCI-X
> > > capability has a different structure.
> > >
> > > Is this intentional? 
> > 
> > Probably not a such.  I don't think we have any drivers for bridge
> > devices so I don't think it matters.  It likely wouldn't hurt to fix
> > it just in case though.
> > 
> > Do any of the mellanox cards do anything with the bridge on the card?
> 
> Yes but they do their own thing wrt saving/restoring registers.
> Look at drivers/infiniband/hw/mthca/mthca_reset.c
> 
> > > If not, here's a patch to fix this. Warning: completely untested.
> > 
> > If you fix the offsets and diff this against my last fix (to never
> > free the buffer) I think your patch makes sense.
> 
> Let's agree what the correct offsets are.
> 
> > > PCI: restore bridge PCI-X capability registers after PM event
> > >
> > > Restore PCI-X bridge up/downstream capability registers
> > > after PM event.  This includes maxumum split transaction
> > > commitment limit which might be vital for PCI X.
> > >
> > > Signed-off-by: Michael S. Tsirkin <[EMAIL PROTECTED]>
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index df49530..4b788ef 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
> > >   if (pos <= 0)
> > >   return 0;
> > >  
> > > - save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
> > > + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
> > >   if (!save_state) {
> > > - dev_err(>dev, "Out of memory in pci_save_pcie_state\n");
> > > + dev_err(>dev, "Out of memory in pci_save_pcix_state\n");
> > >   return -ENOMEM;
> > >   }
> > >   cap = (u16 *)_state->data[0];
> > >  
> > > - pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
> > > + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > 
> > This appears to be the proper test.
> > 
> > > + pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, [i++]);
> > > + pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, [i++]);
> > > + } else
> > > + pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
> > > +
> > >   pci_add_saved_cap(dev, save_state);
> > >   return 0;
> > >  }
> > > @@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev 
> > > *dev)
> > >   return;
> > >   cap = (u16 *)_state->data[0];
> > >  
> > > - pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> > > + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > > + pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
> > > + pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
> > 
> > These look like the proper two registers to save.
> > 
> > > + } else
> > > + pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> > >   pci_remove_saved_cap(save_state);
> > >   kfree(save_state);
> > >  }
> > > diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> > > index f09cce2..fb7eefd 100644
> > > --- a/include/linux/pci_regs.h
> > > +++ b/include/linux/pci_regs.h
> > > @@ -332,6 +332,8 @@
> > >  #define PCI_X_STATUS_SPL_ERR 0x2000 /* Rcvd Split Completion Error 
> > > Msg */
> > >  #define  PCI_X_STATUS_266MHZ 0x4000  /* 266 MHz capable */
> > >  #define  PCI_X_STATUS_533MHZ 0x8000  /* 533 MHz capable */
> > > +#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction 
> > > limit */
> > > +#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction 
> > > limit */
> > 
> > Unless I am completely misreading the spec. While you have picked the
> > right register to save the offsets should be 0x08 and 0x0c or 8 and 12
> 
> No, the spec is written in terms of dwords (32 bit), we are storing words (16 
> bits).
> The data at offsets 8 and 12 is read-only split transaction capacity.
> Split transaction limit starts at bit 16 so you need to add 2 to byte offset.

So, Eric, what are your thoughts on this patch (probably wrt 2.6.22)?
Since the rest of the save/restore code uses 16 bit transactions,
it should be safe here too?

-- 
MST
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

   Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
   regular devices and seem to ignore the fact that for bridge PCI-X
   capability has a different structure.
  
   Is this intentional? 
  
  Probably not a such.  I don't think we have any drivers for bridge
  devices so I don't think it matters.  It likely wouldn't hurt to fix
  it just in case though.
  
  Do any of the mellanox cards do anything with the bridge on the card?
 
 Yes but they do their own thing wrt saving/restoring registers.
 Look at drivers/infiniband/hw/mthca/mthca_reset.c
 
   If not, here's a patch to fix this. Warning: completely untested.
  
  If you fix the offsets and diff this against my last fix (to never
  free the buffer) I think your patch makes sense.
 
 Let's agree what the correct offsets are.
 
   PCI: restore bridge PCI-X capability registers after PM event
  
   Restore PCI-X bridge up/downstream capability registers
   after PM event.  This includes maxumum split transaction
   commitment limit which might be vital for PCI X.
  
   Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]
  
   diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
   index df49530..4b788ef 100644
   --- a/drivers/pci/pci.c
   +++ b/drivers/pci/pci.c
   @@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
 if (pos = 0)
 return 0;

   - save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
   + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
 if (!save_state) {
   - dev_err(dev-dev, Out of memory in pci_save_pcie_state\n);
   + dev_err(dev-dev, Out of memory in pci_save_pcix_state\n);
 return -ENOMEM;
 }
 cap = (u16 *)save_state-data[0];

   - pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
   + if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
  
  This appears to be the proper test.
  
   + pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
   + pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
   + } else
   + pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
   +
 pci_add_saved_cap(dev, save_state);
 return 0;
}
   @@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev 
   *dev)
 return;
 cap = (u16 *)save_state-data[0];

   - pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
   + if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
   + pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
   + pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
  
  These look like the proper two registers to save.
  
   + } else
   + pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
 pci_remove_saved_cap(save_state);
 kfree(save_state);
}
   diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
   index f09cce2..fb7eefd 100644
   --- a/include/linux/pci_regs.h
   +++ b/include/linux/pci_regs.h
   @@ -332,6 +332,8 @@
#define PCI_X_STATUS_SPL_ERR 0x2000 /* Rcvd Split Completion Error 
   Msg */
#define  PCI_X_STATUS_266MHZ 0x4000  /* 266 MHz capable */
#define  PCI_X_STATUS_533MHZ 0x8000  /* 533 MHz capable */
   +#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction 
   limit */
   +#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction 
   limit */
  
  Unless I am completely misreading the spec. While you have picked the
  right register to save the offsets should be 0x08 and 0x0c or 8 and 12
 
 No, the spec is written in terms of dwords (32 bit), we are storing words (16 
 bits).
 The data at offsets 8 and 12 is read-only split transaction capacity.
 Split transaction limit starts at bit 16 so you need to add 2 to byte offset.

So, Eric, what are your thoughts on this patch (probably wrt 2.6.22)?
Since the rest of the save/restore code uses 16 bit transactions,
it should be safe here too?

-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

> Quoting Eric W. Biederman <[EMAIL PROTECTED]>:
> Subject: Re: SATA resume slowness, e1000 MSI warning
> 
> "Michael S. Tsirkin" <[EMAIL PROTECTED]> writes:
> 
> > OK I guess. I gather we assume writing read-only registers has no side 
> > effects?
> > Are there rumors circulating wrt to these?
> 
> I haven't heard anything about that, and if we are writing the same value back
> it should be pretty safe.
> 
> I have heard it asserted that at least one version of the pci spec
> only required 32bit accesses to be supported by the hardware.  One of
> these days I will have to look that and see if it is true.

Maybe. But surely before the PCI-X days.

> I do know
> it can be weird for hardware developers to support multiple kinds of
> decode.

Is this the only place where Linux uses 
pci_read_config_word/pci_read_config_dword?
I think such hardware will be pretty much DOA on all OS-es.  Why don't we wait
and see whether someone reports a broken config?

> As I recall for pci and pci-x at the hardware level the only
> difference in between 32bit transactions and smaller ones is the state
> of the byte-enable lines.

True, and same holds for PCI-Express.

So let's assume hardware implements RO correctly but ignores the BE bits -
nothing bad happens then, right?

-- 
MST
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

"Michael S. Tsirkin" <[EMAIL PROTECTED]> writes:

> OK I guess. I gather we assume writing read-only registers has no side 
> effects?
> Are there rumors circulating wrt to these?

I haven't heard anything about that, and if we are writing the same value back
it should be pretty safe.

I have heard it asserted that at least one version of the pci spec
only required 32bit accesses to be supported by the hardware.  One of
these days I will have to look that and see if it is true.  I do know
it can be weird for hardware developers to support multiple kinds of
decode.   As I recall for pci and pci-x at the hardware level the only
difference in between 32bit transactions and smaller ones is the state
of the byte-enable lines.

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

> Quoting Eric W. Biederman <[EMAIL PROTECTED]>:
> Subject: Re: SATA resume slowness, e1000 MSI warning
> 
> "Michael S. Tsirkin" <[EMAIL PROTECTED]> writes:
> 
> >> Rumor has it that some pci devices can't tolerate < 32bit accesses.
> >> Although I have never met one.
> >
> > hopefully not bridge devices?
> >
> >> The two factors together suggest that
> >> for generic code it probably makes sense to operate on 32bit
> >> quantities, and just to ignore the read-only portion.
> >
> > The code for regular devices seems to use 16-bit accesses, so
> > I think it's best to stay consistent. Or do you want to change this too?
> 
> If we are stomping rare probabilities we might as well change that too.
> The code to save pci-x state is relatively recent.  So it probably just
> hasn't met a problem device yet (assuming they exist).

OK I guess. I gather we assume writing read-only registers has no side effects?
Are there rumors circulating wrt to these?

-- 
MST
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

"Michael S. Tsirkin" <[EMAIL PROTECTED]> writes:

>> Rumor has it that some pci devices can't tolerate < 32bit accesses.
>> Although I have never met one.
>
> hopefully not bridge devices?
>
>> The two factors together suggest that
>> for generic code it probably makes sense to operate on 32bit
>> quantities, and just to ignore the read-only portion.
>
> The code for regular devices seems to use 16-bit accesses, so
> I think it's best to stay consistent. Or do you want to change this too?

If we are stomping rare probabilities we might as well change that too.
The code to save pci-x state is relatively recent.  So it probably just
hasn't met a problem device yet (assuming they exist).

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

> Rumor has it that some pci devices can't tolerate < 32bit accesses.
> Although I have never met one.

hopefully not bridge devices?

> The two factors together suggest that
> for generic code it probably makes sense to operate on 32bit
> quantities, and just to ignore the read-only portion.

The code for regular devices seems to use 16-bit accesses, so
I think it's best to stay consistent. Or do you want to change this too?

-- 
MST
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

"Michael S. Tsirkin" <[EMAIL PROTECTED]> writes:

>> Quoting Eric W. Biederman <[EMAIL PROTECTED]>:
>> Subject: Re: SATA resume slowness, e1000 MSI warning
>> 
>> "Michael S. Tsirkin" <[EMAIL PROTECTED]> writes:
>> 
>> >> The only case I can see which might trigger this is if we saved
>> >> pci-X state and then didn't restore it because we could not find
>> >> the capability on restore.
>> >
>> > Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
>> > regular devices and seem to ignore the fact that for bridge PCI-X
>> > capability has a different structure.
>> >
>> > Is this intentional? 
>> 
>> Probably not a such.  I don't think we have any drivers for bridge
>> devices so I don't think it matters.  It likely wouldn't hurt to fix
>> it just in case though.
>> 
>> Do any of the mellanox cards do anything with the bridge on the card?
>
> Yes but they do their own thing wrt saving/restoring registers.
> Look at drivers/infiniband/hw/mthca/mthca_reset.c
>
>> > If not, here's a patch to fix this. Warning: completely untested.
>> 
>> If you fix the offsets and diff this against my last fix (to never
>> free the buffer) I think your patch makes sense.
>
> Let's agree what the correct offsets are.
>
>> > PCI: restore bridge PCI-X capability registers after PM event
>> >
>> > Restore PCI-X bridge up/downstream capability registers
>> > after PM event.  This includes maxumum split transaction
>> > commitment limit which might be vital for PCI X.
>> >
>> > Signed-off-by: Michael S. Tsirkin <[EMAIL PROTECTED]>
>> >
>> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> > index df49530..4b788ef 100644
>> > --- a/drivers/pci/pci.c
>> > +++ b/drivers/pci/pci.c
>> > @@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
>> >if (pos <= 0)
>> >return 0;
>> >  
>> > -  save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
>> > + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
>> >if (!save_state) {
>> > -  dev_err(>dev, "Out of memory in pci_save_pcie_state\n");
>> > +  dev_err(>dev, "Out of memory in pci_save_pcix_state\n");
>> >return -ENOMEM;
>> >}
>> >cap = (u16 *)_state->data[0];
>> >  
>> > -  pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
>> > +  if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> 
>> This appears to be the proper test.
>> 
>> > + pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, [i++]);
>> > + pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, [i++]);
>> > +  } else
>> > +  pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
>> > +
>> >pci_add_saved_cap(dev, save_state);
>> >return 0;
>> >  }
>> > @@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev 
>> > *dev)
>> >return;
>> >cap = (u16 *)_state->data[0];
>> >  
>> > -  pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
>> > +  if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>> > + pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
>> > + pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
>> 
>> These look like the proper two registers to save.
>> 
>> > +  } else
>> > +  pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
>> >pci_remove_saved_cap(save_state);
>> >kfree(save_state);
>> >  }
>> > diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
>> > index f09cce2..fb7eefd 100644
>> > --- a/include/linux/pci_regs.h
>> > +++ b/include/linux/pci_regs.h
>> > @@ -332,6 +332,8 @@
>> > #define PCI_X_STATUS_SPL_ERR 0x2000 /* Rcvd Split Completion Error Msg
> */
>> >  #define  PCI_X_STATUS_266MHZ  0x4000  /* 266 MHz capable */
>> >  #define  PCI_X_STATUS_533MHZ  0x8000  /* 533 MHz capable */
>> > +#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction
> limit */
>> > +#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction
> limit */
>> 
>> Unless I am completely misreading the spec. While you have picked the
>> right register to save the offsets should

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

> Quoting Eric W. Biederman <[EMAIL PROTECTED]>:
> Subject: Re: SATA resume slowness, e1000 MSI warning
> 
> "Michael S. Tsirkin" <[EMAIL PROTECTED]> writes:
> 
> >> The only case I can see which might trigger this is if we saved
> >> pci-X state and then didn't restore it because we could not find
> >> the capability on restore.
> >
> > Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
> > regular devices and seem to ignore the fact that for bridge PCI-X
> > capability has a different structure.
> >
> > Is this intentional? 
> 
> Probably not a such.  I don't think we have any drivers for bridge
> devices so I don't think it matters.  It likely wouldn't hurt to fix
> it just in case though.
> 
> Do any of the mellanox cards do anything with the bridge on the card?

Yes but they do their own thing wrt saving/restoring registers.
Look at drivers/infiniband/hw/mthca/mthca_reset.c

> > If not, here's a patch to fix this. Warning: completely untested.
> 
> If you fix the offsets and diff this against my last fix (to never
> free the buffer) I think your patch makes sense.

Let's agree what the correct offsets are.

> > PCI: restore bridge PCI-X capability registers after PM event
> >
> > Restore PCI-X bridge up/downstream capability registers
> > after PM event.  This includes maxumum split transaction
> > commitment limit which might be vital for PCI X.
> >
> > Signed-off-by: Michael S. Tsirkin <[EMAIL PROTECTED]>
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index df49530..4b788ef 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
> > if (pos <= 0)
> > return 0;
> >  
> > -   save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
> > + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
> > if (!save_state) {
> > -   dev_err(>dev, "Out of memory in pci_save_pcie_state\n");
> > +   dev_err(>dev, "Out of memory in pci_save_pcix_state\n");
> > return -ENOMEM;
> > }
> > cap = (u16 *)_state->data[0];
> >  
> > -   pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
> > +   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> 
> This appears to be the proper test.
> 
> > + pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, [i++]);
> > + pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, [i++]);
> > +   } else
> > +   pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
> > +
> > pci_add_saved_cap(dev, save_state);
> > return 0;
> >  }
> > @@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
> > return;
> > cap = (u16 *)_state->data[0];
> >  
> > -   pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> > +   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > + pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
> > + pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
> 
> These look like the proper two registers to save.
> 
> > +   } else
> > +   pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> > pci_remove_saved_cap(save_state);
> > kfree(save_state);
> >  }
> > diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> > index f09cce2..fb7eefd 100644
> > --- a/include/linux/pci_regs.h
> > +++ b/include/linux/pci_regs.h
> > @@ -332,6 +332,8 @@
> >  #define PCI_X_STATUS_SPL_ERR 0x2000 /* Rcvd Split Completion Error Msg 
> > */
> >  #define  PCI_X_STATUS_266MHZ   0x4000  /* 266 MHz capable */
> >  #define  PCI_X_STATUS_533MHZ   0x8000  /* 533 MHz capable */
> > +#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction 
> > limit */
> > +#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction 
> > limit */
> 
> Unless I am completely misreading the spec. While you have picked the
> right register to save the offsets should be 0x08 and 0x0c or 8 and 12

No, the spec is written in terms of dwords (32 bit), we are storing words (16 
bits).
The data at offsets 8 and 12 is read-only split transaction capacity.
Split transaction limit starts at bit 16 so you need to add 2 to byte offset.

Right?


-- 
MST
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

"Michael S. Tsirkin" <[EMAIL PROTECTED]> writes:

>> The only case I can see which might trigger this is if we saved
>> pci-X state and then didn't restore it because we could not find
>> the capability on restore.
>
> Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
> regular devices and seem to ignore the fact that for bridge PCI-X
> capability has a different structure.
>
> Is this intentional? 

Probably not a such.  I don't think we have any drivers for bridge
devices so I don't think it matters.  It likely wouldn't hurt to fix
it just in case though.

Do any of the mellanox cards do anything with the bridge on the card?

> If not, here's a patch to fix this. Warning: completely untested.

If you fix the offsets and diff this against my last fix (to never
free the buffer) I think your patch makes sense.

> PCI: restore bridge PCI-X capability registers after PM event
>
> Restore PCI-X bridge up/downstream capability registers
> after PM event.  This includes maxumum split transaction
> commitment limit which might be vital for PCI X.
>
> Signed-off-by: Michael S. Tsirkin <[EMAIL PROTECTED]>
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index df49530..4b788ef 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
>   if (pos <= 0)
>   return 0;
>  
> - save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
> + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
>   if (!save_state) {
> - dev_err(>dev, "Out of memory in pci_save_pcie_state\n");
> + dev_err(>dev, "Out of memory in pci_save_pcix_state\n");
>   return -ENOMEM;
>   }
>   cap = (u16 *)_state->data[0];
>  
> - pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {

This appears to be the proper test.

> + pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, [i++]);
> + pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, [i++]);
> + } else
> + pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
> +
>   pci_add_saved_cap(dev, save_state);
>   return 0;
>  }
> @@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
>   return;
>   cap = (u16 *)_state->data[0];
>  
> - pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> + pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
> + pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);

These look like the proper two registers to save.

> + } else
> + pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
>   pci_remove_saved_cap(save_state);
>   kfree(save_state);
>  }
> diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
> index f09cce2..fb7eefd 100644
> --- a/include/linux/pci_regs.h
> +++ b/include/linux/pci_regs.h
> @@ -332,6 +332,8 @@
>  #define PCI_X_STATUS_SPL_ERR 0x2000 /* Rcvd Split Completion Error Msg */
>  #define  PCI_X_STATUS_266MHZ 0x4000  /* 266 MHz capable */
>  #define  PCI_X_STATUS_533MHZ 0x8000  /* 533 MHz capable */
> +#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction limit 
> */
> +#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction 
> limit */

Unless I am completely misreading the spec. While you have picked the
right register to save the offsets should be 0x08 and 0x0c or 8 and 12

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Michael S. Tsirkin [EMAIL PROTECTED] writes:

 The only case I can see which might trigger this is if we saved
 pci-X state and then didn't restore it because we could not find
 the capability on restore.

 Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
 regular devices and seem to ignore the fact that for bridge PCI-X
 capability has a different structure.

 Is this intentional? 

Probably not a such.  I don't think we have any drivers for bridge
devices so I don't think it matters.  It likely wouldn't hurt to fix
it just in case though.

Do any of the mellanox cards do anything with the bridge on the card?

 If not, here's a patch to fix this. Warning: completely untested.

If you fix the offsets and diff this against my last fix (to never
free the buffer) I think your patch makes sense.

 PCI: restore bridge PCI-X capability registers after PM event

 Restore PCI-X bridge up/downstream capability registers
 after PM event.  This includes maxumum split transaction
 commitment limit which might be vital for PCI X.

 Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]

 diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
 index df49530..4b788ef 100644
 --- a/drivers/pci/pci.c
 +++ b/drivers/pci/pci.c
 @@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
   if (pos = 0)
   return 0;
  
 - save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
 + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
   if (!save_state) {
 - dev_err(dev-dev, Out of memory in pci_save_pcie_state\n);
 + dev_err(dev-dev, Out of memory in pci_save_pcix_state\n);
   return -ENOMEM;
   }
   cap = (u16 *)save_state-data[0];
  
 - pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
 + if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {

This appears to be the proper test.

 + pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
 + pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
 + } else
 + pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
 +
   pci_add_saved_cap(dev, save_state);
   return 0;
  }
 @@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
   return;
   cap = (u16 *)save_state-data[0];
  
 - pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
 + if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 + pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
 + pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);

These look like the proper two registers to save.

 + } else
 + pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
   pci_remove_saved_cap(save_state);
   kfree(save_state);
  }
 diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
 index f09cce2..fb7eefd 100644
 --- a/include/linux/pci_regs.h
 +++ b/include/linux/pci_regs.h
 @@ -332,6 +332,8 @@
  #define PCI_X_STATUS_SPL_ERR 0x2000 /* Rcvd Split Completion Error Msg */
  #define  PCI_X_STATUS_266MHZ 0x4000  /* 266 MHz capable */
  #define  PCI_X_STATUS_533MHZ 0x8000  /* 533 MHz capable */
 +#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction limit 
 */
 +#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction 
 limit */

Unless I am completely misreading the spec. While you have picked the
right register to save the offsets should be 0x08 and 0x0c or 8 and 12

Eric
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

 Quoting Eric W. Biederman [EMAIL PROTECTED]:
 Subject: Re: SATA resume slowness, e1000 MSI warning
 
 Michael S. Tsirkin [EMAIL PROTECTED] writes:
 
  The only case I can see which might trigger this is if we saved
  pci-X state and then didn't restore it because we could not find
  the capability on restore.
 
  Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
  regular devices and seem to ignore the fact that for bridge PCI-X
  capability has a different structure.
 
  Is this intentional? 
 
 Probably not a such.  I don't think we have any drivers for bridge
 devices so I don't think it matters.  It likely wouldn't hurt to fix
 it just in case though.
 
 Do any of the mellanox cards do anything with the bridge on the card?

Yes but they do their own thing wrt saving/restoring registers.
Look at drivers/infiniband/hw/mthca/mthca_reset.c

  If not, here's a patch to fix this. Warning: completely untested.
 
 If you fix the offsets and diff this against my last fix (to never
 free the buffer) I think your patch makes sense.

Let's agree what the correct offsets are.

  PCI: restore bridge PCI-X capability registers after PM event
 
  Restore PCI-X bridge up/downstream capability registers
  after PM event.  This includes maxumum split transaction
  commitment limit which might be vital for PCI X.
 
  Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]
 
  diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
  index df49530..4b788ef 100644
  --- a/drivers/pci/pci.c
  +++ b/drivers/pci/pci.c
  @@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
  if (pos = 0)
  return 0;
   
  -   save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
  + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
  if (!save_state) {
  -   dev_err(dev-dev, Out of memory in pci_save_pcie_state\n);
  +   dev_err(dev-dev, Out of memory in pci_save_pcix_state\n);
  return -ENOMEM;
  }
  cap = (u16 *)save_state-data[0];
   
  -   pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
  +   if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 
 This appears to be the proper test.
 
  + pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
  + pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
  +   } else
  +   pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
  +
  pci_add_saved_cap(dev, save_state);
  return 0;
   }
  @@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
  return;
  cap = (u16 *)save_state-data[0];
   
  -   pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
  +   if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
  + pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
  + pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
 
 These look like the proper two registers to save.
 
  +   } else
  +   pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
  pci_remove_saved_cap(save_state);
  kfree(save_state);
   }
  diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
  index f09cce2..fb7eefd 100644
  --- a/include/linux/pci_regs.h
  +++ b/include/linux/pci_regs.h
  @@ -332,6 +332,8 @@
   #define PCI_X_STATUS_SPL_ERR 0x2000 /* Rcvd Split Completion Error Msg 
  */
   #define  PCI_X_STATUS_266MHZ   0x4000  /* 266 MHz capable */
   #define  PCI_X_STATUS_533MHZ   0x8000  /* 533 MHz capable */
  +#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction 
  limit */
  +#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction 
  limit */
 
 Unless I am completely misreading the spec. While you have picked the
 right register to save the offsets should be 0x08 and 0x0c or 8 and 12

No, the spec is written in terms of dwords (32 bit), we are storing words (16 
bits).
The data at offsets 8 and 12 is read-only split transaction capacity.
Split transaction limit starts at bit 16 so you need to add 2 to byte offset.

Right?


-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Michael S. Tsirkin [EMAIL PROTECTED] writes:

 Quoting Eric W. Biederman [EMAIL PROTECTED]:
 Subject: Re: SATA resume slowness, e1000 MSI warning
 
 Michael S. Tsirkin [EMAIL PROTECTED] writes:
 
  The only case I can see which might trigger this is if we saved
  pci-X state and then didn't restore it because we could not find
  the capability on restore.
 
  Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
  regular devices and seem to ignore the fact that for bridge PCI-X
  capability has a different structure.
 
  Is this intentional? 
 
 Probably not a such.  I don't think we have any drivers for bridge
 devices so I don't think it matters.  It likely wouldn't hurt to fix
 it just in case though.
 
 Do any of the mellanox cards do anything with the bridge on the card?

 Yes but they do their own thing wrt saving/restoring registers.
 Look at drivers/infiniband/hw/mthca/mthca_reset.c

  If not, here's a patch to fix this. Warning: completely untested.
 
 If you fix the offsets and diff this against my last fix (to never
 free the buffer) I think your patch makes sense.

 Let's agree what the correct offsets are.

  PCI: restore bridge PCI-X capability registers after PM event
 
  Restore PCI-X bridge up/downstream capability registers
  after PM event.  This includes maxumum split transaction
  commitment limit which might be vital for PCI X.
 
  Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]
 
  diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
  index df49530..4b788ef 100644
  --- a/drivers/pci/pci.c
  +++ b/drivers/pci/pci.c
  @@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
 if (pos = 0)
 return 0;
   
  -  save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
  + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
 if (!save_state) {
  -  dev_err(dev-dev, Out of memory in pci_save_pcie_state\n);
  +  dev_err(dev-dev, Out of memory in pci_save_pcix_state\n);
 return -ENOMEM;
 }
 cap = (u16 *)save_state-data[0];
   
  -  pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
  +  if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
 
 This appears to be the proper test.
 
  + pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
  + pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
  +  } else
  +  pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
  +
 pci_add_saved_cap(dev, save_state);
 return 0;
   }
  @@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev 
  *dev)
 return;
 cap = (u16 *)save_state-data[0];
   
  -  pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
  +  if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
  + pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, cap[i++]);
  + pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, cap[i++]);
 
 These look like the proper two registers to save.
 
  +  } else
  +  pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
 pci_remove_saved_cap(save_state);
 kfree(save_state);
   }
  diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
  index f09cce2..fb7eefd 100644
  --- a/include/linux/pci_regs.h
  +++ b/include/linux/pci_regs.h
  @@ -332,6 +332,8 @@
  #define PCI_X_STATUS_SPL_ERR 0x2000 /* Rcvd Split Completion Error Msg
 */
   #define  PCI_X_STATUS_266MHZ  0x4000  /* 266 MHz capable */
   #define  PCI_X_STATUS_533MHZ  0x8000  /* 533 MHz capable */
  +#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction
 limit */
  +#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction
 limit */
 
 Unless I am completely misreading the spec. While you have picked the
 right register to save the offsets should be 0x08 and 0x0c or 8 and 12

 No, the spec is written in terms of dwords (32 bit), we are storing words (16
 bits).
 The data at offsets 8 and 12 is read-only split transaction capacity.
 Split transaction limit starts at bit 16 so you need to add 2 to byte offset.

 Right?

From that perspective it makes sense.  So I will agree with the way you are
thinking the code works.

The read-only and the read-write part are all defined as part of the
same register so I didn't expect them to be separate.  And I hadn't
paid attention enough to see that the code was only saving 16bit
values.

Rumor has it that some pci devices can't tolerate  32bit accesses.
Although I have never met one.  The two factors together suggest that
for generic code it probably makes sense to operate on 32bit
quantities, and just to ignore the read-only portion.

Eric
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

 Rumor has it that some pci devices can't tolerate  32bit accesses.
 Although I have never met one.

hopefully not bridge devices?

 The two factors together suggest that
 for generic code it probably makes sense to operate on 32bit
 quantities, and just to ignore the read-only portion.

The code for regular devices seems to use 16-bit accesses, so
I think it's best to stay consistent. Or do you want to change this too?

-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Michael S. Tsirkin [EMAIL PROTECTED] writes:

 Rumor has it that some pci devices can't tolerate  32bit accesses.
 Although I have never met one.

 hopefully not bridge devices?

 The two factors together suggest that
 for generic code it probably makes sense to operate on 32bit
 quantities, and just to ignore the read-only portion.

 The code for regular devices seems to use 16-bit accesses, so
 I think it's best to stay consistent. Or do you want to change this too?

If we are stomping rare probabilities we might as well change that too.
The code to save pci-x state is relatively recent.  So it probably just
hasn't met a problem device yet (assuming they exist).

Eric
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

 Quoting Eric W. Biederman [EMAIL PROTECTED]:
 Subject: Re: SATA resume slowness, e1000 MSI warning
 
 Michael S. Tsirkin [EMAIL PROTECTED] writes:
 
  Rumor has it that some pci devices can't tolerate  32bit accesses.
  Although I have never met one.
 
  hopefully not bridge devices?
 
  The two factors together suggest that
  for generic code it probably makes sense to operate on 32bit
  quantities, and just to ignore the read-only portion.
 
  The code for regular devices seems to use 16-bit accesses, so
  I think it's best to stay consistent. Or do you want to change this too?
 
 If we are stomping rare probabilities we might as well change that too.
 The code to save pci-x state is relatively recent.  So it probably just
 hasn't met a problem device yet (assuming they exist).

OK I guess. I gather we assume writing read-only registers has no side effects?
Are there rumors circulating wrt to these?

-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Michael S. Tsirkin [EMAIL PROTECTED] writes:

 OK I guess. I gather we assume writing read-only registers has no side 
 effects?
 Are there rumors circulating wrt to these?

I haven't heard anything about that, and if we are writing the same value back
it should be pretty safe.

I have heard it asserted that at least one version of the pci spec
only required 32bit accesses to be supported by the hardware.  One of
these days I will have to look that and see if it is true.  I do know
it can be weird for hardware developers to support multiple kinds of
decode.   As I recall for pci and pci-x at the hardware level the only
difference in between 32bit transactions and smaller ones is the state
of the byte-enable lines.

Eric
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

 Quoting Eric W. Biederman [EMAIL PROTECTED]:
 Subject: Re: SATA resume slowness, e1000 MSI warning
 
 Michael S. Tsirkin [EMAIL PROTECTED] writes:
 
  OK I guess. I gather we assume writing read-only registers has no side 
  effects?
  Are there rumors circulating wrt to these?
 
 I haven't heard anything about that, and if we are writing the same value back
 it should be pretty safe.
 
 I have heard it asserted that at least one version of the pci spec
 only required 32bit accesses to be supported by the hardware.  One of
 these days I will have to look that and see if it is true.

Maybe. But surely before the PCI-X days.

 I do know
 it can be weird for hardware developers to support multiple kinds of
 decode.

Is this the only place where Linux uses 
pci_read_config_word/pci_read_config_dword?
I think such hardware will be pretty much DOA on all OS-es.  Why don't we wait
and see whether someone reports a broken config?

 As I recall for pci and pci-x at the hardware level the only
 difference in between 32bit transactions and smaller ones is the state
 of the byte-enable lines.

True, and same holds for PCI-Express.

So let's assume hardware implements RO correctly but ignores the BE bits -
nothing bad happens then, right?

-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

"Kok, Auke" <[EMAIL PROTECTED]> writes:

> Eric W. Biederman wrote:
>
> [CHOP]
>
>> Below is an additional set of warnings that should help debug this.
>> The old code just got lucky that it triggered a warning when this happens.
>
>
> I'm trying this patch together with the other 2 that you sent out a few days
> ago. I'm seeing some minor issues with this and lots of bogus warnings as far 
> as
> I can see.
>
> If I suspend/resume and unload e1000, then reinsert e1000.ko, I immediately 
> hit
> the WARN_ON at `msi.c:516: WARN_ON(!hlist_empty(>saved_cap_space));`
>
> I'm not sure that's useful debugging information. even though saved state 
> exists
> the module has been removed, so you might want to purge the state table when 
> the
> driver gets removed?
>
>
> anyway, back to testing.

Sorry I should have been clear.

With the last two patches I sent out:
pci: Repair pci_save/restore_state so we can restore one save many times.
msi: Safer state caching.

Those WARN_ON's are now totally bogus.  

In essence the WARN_ON's were testing to ensure pci_save_state and
pci_restore_state were paired.

The assumptions was that pci_restore_state would remove everything
from the list that pci_save_state placed on it.

When I reworked the code and removed the bogus pairing requirement it meant
that if we ever save state and have a pci-e or pci-x capability we will have
a state structure on the list until the kernel reboots. 

In summary:
pci_save_state and pci_restore_state were making a wrong assumption
about the world.  The WARN_ON patch tested to ensure the world matched
those functions.  When I fixed those functions to match the world the
WARN_ON's became completely bogus.

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Eric W. Biederman wrote:

[CHOP]


Below is an additional set of warnings that should help debug this.
The old code just got lucky that it triggered a warning when this happens.



I'm trying this patch together with the other 2 that you sent out a few days 
ago. I'm seeing some minor issues with this and lots of bogus warnings as far as 
I can see.


If I suspend/resume and unload e1000, then reinsert e1000.ko, I immediately hit 
the WARN_ON at `msi.c:516: WARN_ON(!hlist_empty(>saved_cap_space));`


I'm not sure that's useful debugging information. even though saved state exists 
the module has been removed, so you might want to purge the state table when the 
driver gets removed?



anyway, back to testing.

Auke



diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 01869b1..5113913 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -613,6 +613,7 @@ int pci_enable_msi(struct pci_dev* dev)
return -EINVAL;
 
 	WARN_ON(!!dev->msi_enabled);

+   WARN_ON(!hlist_empty(>saved_cap_space));
 
 	/* Check whether driver already requested for MSI-X irqs */

if (dev->msix_enabled) {
@@ -638,6 +639,8 @@ void pci_disable_msi(struct pci_dev* dev)
if (!dev->msi_enabled)
return;
 
+	WARN_ON(!hlist_empty(>saved_cap_space));

+
msi_set_enable(dev, 0);
pci_intx(dev, 1);   /* enable intx */
dev->msi_enabled = 0;
@@ -739,6 +742,7 @@ int pci_enable_msix(struct pci_dev* dev, struct msix_entry 
*entries, int nvec)
}
}
WARN_ON(!!dev->msix_enabled);
+   WARN_ON(!hlist_empty(>saved_cap_space));
 
 	/* Check whether driver already requested for MSI irq */

if (dev->msi_enabled) {
@@ -763,6 +767,8 @@ void pci_disable_msix(struct pci_dev* dev)
if (!dev->msix_enabled)
return;
 
+	WARN_ON(!hlist_empty(>saved_cap_space));

+
msix_set_enable(dev, 0);
pci_intx(dev, 1);   /* enable intx */
dev->msix_enabled = 0;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bd44a48..4418839 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -677,6 +677,7 @@ pci_restore_state(struct pci_dev *dev)
}
pci_restore_pcix_state(dev);
pci_restore_msi_state(dev);
+   WARN_ON(!hlist_empty(>saved_cap_space));
 
 	return 0;

 }

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Eric W. Biederman wrote:

[CHOP]


Below is an additional set of warnings that should help debug this.
The old code just got lucky that it triggered a warning when this happens.



I'm trying this patch together with the other 2 that you sent out a few days 
ago. I'm seeing some minor issues with this and lots of bogus warnings as far as 
I can see.


If I suspend/resume and unload e1000, then reinsert e1000.ko, I immediately hit 
the WARN_ON at `msi.c:516: WARN_ON(!hlist_empty(dev-saved_cap_space));`


I'm not sure that's useful debugging information. even though saved state exists 
the module has been removed, so you might want to purge the state table when the 
driver gets removed?



anyway, back to testing.

Auke



diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 01869b1..5113913 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -613,6 +613,7 @@ int pci_enable_msi(struct pci_dev* dev)
return -EINVAL;
 
 	WARN_ON(!!dev-msi_enabled);

+   WARN_ON(!hlist_empty(dev-saved_cap_space));
 
 	/* Check whether driver already requested for MSI-X irqs */

if (dev-msix_enabled) {
@@ -638,6 +639,8 @@ void pci_disable_msi(struct pci_dev* dev)
if (!dev-msi_enabled)
return;
 
+	WARN_ON(!hlist_empty(dev-saved_cap_space));

+
msi_set_enable(dev, 0);
pci_intx(dev, 1);   /* enable intx */
dev-msi_enabled = 0;
@@ -739,6 +742,7 @@ int pci_enable_msix(struct pci_dev* dev, struct msix_entry 
*entries, int nvec)
}
}
WARN_ON(!!dev-msix_enabled);
+   WARN_ON(!hlist_empty(dev-saved_cap_space));
 
 	/* Check whether driver already requested for MSI irq */

if (dev-msi_enabled) {
@@ -763,6 +767,8 @@ void pci_disable_msix(struct pci_dev* dev)
if (!dev-msix_enabled)
return;
 
+	WARN_ON(!hlist_empty(dev-saved_cap_space));

+
msix_set_enable(dev, 0);
pci_intx(dev, 1);   /* enable intx */
dev-msix_enabled = 0;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bd44a48..4418839 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -677,6 +677,7 @@ pci_restore_state(struct pci_dev *dev)
}
pci_restore_pcix_state(dev);
pci_restore_msi_state(dev);
+   WARN_ON(!hlist_empty(dev-saved_cap_space));
 
 	return 0;

 }

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Kok, Auke [EMAIL PROTECTED] writes:

 Eric W. Biederman wrote:

 [CHOP]

 Below is an additional set of warnings that should help debug this.
 The old code just got lucky that it triggered a warning when this happens.


 I'm trying this patch together with the other 2 that you sent out a few days
 ago. I'm seeing some minor issues with this and lots of bogus warnings as far 
 as
 I can see.

 If I suspend/resume and unload e1000, then reinsert e1000.ko, I immediately 
 hit
 the WARN_ON at `msi.c:516: WARN_ON(!hlist_empty(dev-saved_cap_space));`

 I'm not sure that's useful debugging information. even though saved state 
 exists
 the module has been removed, so you might want to purge the state table when 
 the
 driver gets removed?


 anyway, back to testing.

Sorry I should have been clear.

With the last two patches I sent out:
pci: Repair pci_save/restore_state so we can restore one save many times.
msi: Safer state caching.

Those WARN_ON's are now totally bogus.  

In essence the WARN_ON's were testing to ensure pci_save_state and
pci_restore_state were paired.

The assumptions was that pci_restore_state would remove everything
from the list that pci_save_state placed on it.

When I reworked the code and removed the bogus pairing requirement it meant
that if we ever save state and have a pci-e or pci-x capability we will have
a state structure on the list until the kernel reboots. 

In summary:
pci_save_state and pci_restore_state were making a wrong assumption
about the world.  The WARN_ON patch tested to ensure the world matched
those functions.  When I fixed those functions to match the world the
WARN_ON's became completely bogus.

Eric
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Jeff Garzik <[EMAIL PROTECTED]> writes:

> Eric W. Biederman wrote:
>> Until I get the best scenario I can come up with is a tg3 hardware bug
>> that doesn't renable the pci-X capability after a restore of power state.
>
>
> Speaking of tg3, make sure you are aware that the number of calls to 
> save-state
> functions may not match the number of calls to the restore-state functions.
> ISTR seeing some memleak bugs in PCI related to this.

Thanks that looks like the problem, multiple calls to save before one
call to restore when you have a pci-x capability would easily trigger
this problem.  I just surveyed a bunch of the pci_save_state and
pci_restore_state users and this appears to be a common idiom not just
a tg3 thing

It looks like when code was added to save/restore the msi capability
was added to pci_save/restore_state that an assumption was added that
pci_save_state and pci_restore state were only used for suspend and
only used in pairs.  There is even a partial bug fix that removed the
worst of the symptoms of that assumption from the msi code but failed
to recognize the core problem.

Now that we have code to work with pcie and pcix capabilities as well
as msi this problem is much easier to hit.

All of pci_save_state and pci_restore_state is going to have to be
restructured to fix this, and it is late in the game.  Ugh.
Oh well, better to fix it now 

At least I get my answer about if what pci_save_state is doing
is reasonable. It is not.  pci_save_state no longer supports being
used in conjunction with hardware reset and has become a
suspend/resume specific function.

Now I'm off to wite some patches to fix this.

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

> The only case I can see which might trigger this is if we saved
> pci-X state and then didn't restore it because we could not find
> the capability on restore.

Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
regular devices and seem to ignore the fact that for bridge PCI-X
capability has a different structure.

Is this intentional? If not, here's a patch to fix this.
Warning: completely untested.


PCI: restore bridge PCI-X capability registers after PM event

Restore PCI-X bridge up/downstream capability registers
after PM event.  This includes maxumum split transaction
commitment limit which might be vital for PCI X.

Signed-off-by: Michael S. Tsirkin <[EMAIL PROTECTED]>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index df49530..4b788ef 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
if (pos <= 0)
return 0;
 
-   save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
+   save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
if (!save_state) {
-   dev_err(>dev, "Out of memory in pci_save_pcie_state\n");
+   dev_err(>dev, "Out of memory in pci_save_pcix_state\n");
return -ENOMEM;
}
cap = (u16 *)_state->data[0];
 
-   pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
+   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+   pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, 
[i++]);
+   pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, 
[i++]);
+   } else
+   pci_read_config_word(dev, pos + PCI_X_CMD, [i++]);
+
pci_add_saved_cap(dev, save_state);
return 0;
 }
@@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
return;
cap = (u16 *)_state->data[0];
 
-   pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
+   if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+   pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, 
cap[i++]);
+   pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, 
cap[i++]);
+   } else
+   pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
pci_remove_saved_cap(save_state);
kfree(save_state);
 }
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index f09cce2..fb7eefd 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -332,6 +332,8 @@
 #define  PCI_X_STATUS_SPL_ERR  0x2000  /* Rcvd Split Completion Error 
Msg */
 #define  PCI_X_STATUS_266MHZ   0x4000  /* 266 MHz capable */
 #define  PCI_X_STATUS_533MHZ   0x8000  /* 533 MHz capable */
+#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction 
limit */
+#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction 
limit */
 
 /* PCI Express capability registers */
 


-- 
MST
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Jeff Garzik]

Eric W. Biederman wrote:

Until I get the best scenario I can come up with is a tg3 hardware bug
that doesn't renable the pci-X capability after a restore of power state.



Speaking of tg3, make sure you are aware that the number of calls to 
save-state functions may not match the number of calls to the 
restore-state functions.  ISTR seeing some memleak bugs in PCI related 
to this.


Jeff


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Jeff Garzik [EMAIL PROTECTED] writes:

 Eric W. Biederman wrote:
 Until I get the best scenario I can come up with is a tg3 hardware bug
 that doesn't renable the pci-X capability after a restore of power state.


 Speaking of tg3, make sure you are aware that the number of calls to 
 save-state
 functions may not match the number of calls to the restore-state functions.
 ISTR seeing some memleak bugs in PCI related to this.

Thanks that looks like the problem, multiple calls to save before one
call to restore when you have a pci-x capability would easily trigger
this problem.  I just surveyed a bunch of the pci_save_state and
pci_restore_state users and this appears to be a common idiom not just
a tg3 thing

It looks like when code was added to save/restore the msi capability
was added to pci_save/restore_state that an assumption was added that
pci_save_state and pci_restore state were only used for suspend and
only used in pairs.  There is even a partial bug fix that removed the
worst of the symptoms of that assumption from the msi code but failed
to recognize the core problem.

Now that we have code to work with pcie and pcix capabilities as well
as msi this problem is much easier to hit.

All of pci_save_state and pci_restore_state is going to have to be
restructured to fix this, and it is late in the game.  Ugh.
Oh well, better to fix it now 

At least I get my answer about if what pci_save_state is doing
is reasonable. It is not.  pci_save_state no longer supports being
used in conjunction with hardware reset and has become a
suspend/resume specific function.

Now I'm off to wite some patches to fix this.

Eric
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Jeff Garzik]

Eric W. Biederman wrote:

Until I get the best scenario I can come up with is a tg3 hardware bug
that doesn't renable the pci-X capability after a restore of power state.



Speaking of tg3, make sure you are aware that the number of calls to 
save-state functions may not match the number of calls to the 
restore-state functions.  ISTR seeing some memleak bugs in PCI related 
to this.


Jeff


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Michael S. Tsirkin]

 The only case I can see which might trigger this is if we saved
 pci-X state and then didn't restore it because we could not find
 the capability on restore.

Hmm. pci_save_pcix_state/pci_restore_pcix_state seem to only handle
regular devices and seem to ignore the fact that for bridge PCI-X
capability has a different structure.

Is this intentional? If not, here's a patch to fix this.
Warning: completely untested.


PCI: restore bridge PCI-X capability registers after PM event

Restore PCI-X bridge up/downstream capability registers
after PM event.  This includes maxumum split transaction
commitment limit which might be vital for PCI X.

Signed-off-by: Michael S. Tsirkin [EMAIL PROTECTED]

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index df49530..4b788ef 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -597,14 +597,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
if (pos = 0)
return 0;
 
-   save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
+   save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 2, GFP_KERNEL);
if (!save_state) {
-   dev_err(dev-dev, Out of memory in pci_save_pcie_state\n);
+   dev_err(dev-dev, Out of memory in pci_save_pcix_state\n);
return -ENOMEM;
}
cap = (u16 *)save_state-data[0];
 
-   pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
+   if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+   pci_read_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, 
cap[i++]);
+   pci_read_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, 
cap[i++]);
+   } else
+   pci_read_config_word(dev, pos + PCI_X_CMD, cap[i++]);
+
pci_add_saved_cap(dev, save_state);
return 0;
 }
@@ -621,7 +626,11 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
return;
cap = (u16 *)save_state-data[0];
 
-   pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
+   if (dev-hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+   pci_write_config_word(dev, pos + PCI_X_BRIDGE_UP_SPL_CTL, 
cap[i++]);
+   pci_write_config_word(dev, pos + PCI_X_BRIDGE_DN_SPL_CTL, 
cap[i++]);
+   } else
+   pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
pci_remove_saved_cap(save_state);
kfree(save_state);
 }
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index f09cce2..fb7eefd 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -332,6 +332,8 @@
 #define  PCI_X_STATUS_SPL_ERR  0x2000  /* Rcvd Split Completion Error 
Msg */
 #define  PCI_X_STATUS_266MHZ   0x4000  /* 266 MHz capable */
 #define  PCI_X_STATUS_533MHZ   0x8000  /* 533 MHz capable */
+#define PCI_X_BRIDGE_UP_SPL_CTL 10 /* PCI-X upstream split transaction 
limit */
+#define PCI_X_BRIDGE_DN_SPL_CTL 14 /* PCI-X downstream split transaction 
limit */
 
 /* PCI Express capability registers */
 


-- 
MST
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Andrew Morton <[EMAIL PROTECTED]> writes:

>
> That's:
>
> pci_restore_pcix_state(dev);
> pci_restore_msi_state(dev);
> WARN_ON(!hlist_empty(>saved_cap_space));
>
> return 0;

Hmm.  Either I am confused of I just found an unanticipated leak.

pci_restore_msi_state should be out of the picture as we don't yet
have ppc msi support and I don't think the g5 generation hardware
supported it either.

The only case I can see which might trigger this is if we saved
pci-X state and then didn't restore it because we could not find
the capability on restore.

Any chance you could walk that list and find the cap_nr of the remaining
element?  

Something like:
{
struct pci_cap_saved_state *tmp;
struct hlist_node *pos;

hlist_for_each_entry(tmp, pos, _dev->saved_cap_space, next)
printk(KERN_INFO "saved_cap: 0x%02x\n", tmp->cap_nr);
}

Until I get the best scenario I can come up with is a tg3 hardware bug
that doesn't renable the pci-X capability after a restore of power state.

Getting that cap_nr will at least allow me to be certain if I am dealing
with msi, pci-X or pci-e.

Unanticipated bugs aren't supposed to be this easy to find!

Eric

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Andrew Morton]

On Wed, 07 Mar 2007 12:28:11 -0700 [EMAIL PROTECTED] (Eric W. Biederman) wrote:

> Below is an additional set of warnings that should help debug this.
> The old code just got lucky that it triggered a warning when this happens.
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 01869b1..5113913 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -613,6 +613,7 @@ int pci_enable_msi(struct pci_dev* dev)
>   return -EINVAL;
>  
>   WARN_ON(!!dev->msi_enabled);
> + WARN_ON(!hlist_empty(>saved_cap_space));
>  
>   /* Check whether driver already requested for MSI-X irqs */
>   if (dev->msix_enabled) {
> @@ -638,6 +639,8 @@ void pci_disable_msi(struct pci_dev* dev)
>   if (!dev->msi_enabled)
>   return;
>  
> + WARN_ON(!hlist_empty(>saved_cap_space));
> +
>   msi_set_enable(dev, 0);
>   pci_intx(dev, 1);   /* enable intx */
>   dev->msi_enabled = 0;
> @@ -739,6 +742,7 @@ int pci_enable_msix(struct pci_dev* dev, struct 
> msix_entry *entries, int nvec)
>   }
>   }
>   WARN_ON(!!dev->msix_enabled);
> + WARN_ON(!hlist_empty(>saved_cap_space));
>  
>   /* Check whether driver already requested for MSI irq */
>   if (dev->msi_enabled) {
> @@ -763,6 +767,8 @@ void pci_disable_msix(struct pci_dev* dev)
>   if (!dev->msix_enabled)
>   return;
>  
> + WARN_ON(!hlist_empty(>saved_cap_space));
> +
>   msix_set_enable(dev, 0);
>   pci_intx(dev, 1);   /* enable intx */
>   dev->msix_enabled = 0;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index bd44a48..4418839 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -677,6 +677,7 @@ pci_restore_state(struct pci_dev *dev)
>   }
>   pci_restore_pcix_state(dev);
>   pci_restore_msi_state(dev);
> + WARN_ON(!hlist_empty(>saved_cap_space));
>  
>   return 0;
>  }

Got a hit on a powerpc g5:

PM: Writing back config space on device 0001:05:04.0 at offset 1 (was 2b0, 
writing 2b6)
[ cut here ]
Badness at drivers/pci/pci.c:679
Call Trace:
[C80F7410] [C0011EFC] .show_stack+0x50/0x1cc (unreliable)
[C80F74C0] [C01AD610] .report_bug+0xa0/0x110
[C80F7550] [C00256E4] .program_check_exception+0xb4/0x670
[C80F7630] [C00046F4] program_check_common+0xf4/0x100
--- Exception: 700 at .pci_restore_state+0x310/0x340
LR = .pci_restore_state+0x2e0/0x340
[C80F79D0] [C026A174] .tg3_chip_reset+0x19c/0xa04
[C80F7A90] [C026D948] .tg3_reset_hw+0xa4/0x2718
[C80F7BA0] [C0270030] .tg3_init_hw+0x74/0x94
[C80F7C30] [C0270BE0] .tg3_open+0x4c8/0x854
[C80F7CF0] [C03A74A4] .dev_open+0x100/0x12c
[C80F7D90] [C03BAEA8] .netpoll_setup+0x2dc/0x3ec
[C80F7E40] [C0283450] .init_netconsole+0x64/0x8c
[C80F7EC0] [C05C0BE4] .init+0x1d0/0x390
[C80F7F90] [C00271F8] .kernel_thread+0x4c/0x68
tg3: eth0: Link is up at 1000 Mbps, full duplex.
tg3: eth0: Flow control is on for TX and on for RX.
Scheduler bitmap error - bitmap being reconstructed..
netconsole: network logging started
Calling initcall 0xc06bd180: .macio_module_init+0x0/0x3c()

That's:

pci_restore_pcix_state(dev);
pci_restore_msi_state(dev);
WARN_ON(!hlist_empty(>saved_cap_space));

return 0;

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

"Kok, Auke" <[EMAIL PROTECTED]> writes:

> Kok, Auke wrote:
>> Eric W. Biederman wrote:

>>> This leaves with some basic questions.
>>> - Does it make sense for suspend/resume methods to request/free irqs?
>>> - Does it make sense for suspend/resume methods to allocate/free msi irqs?
>>> - Do we want pci_save/restore_cap to save/restore msi state?
>>>
>>> The path of least resistance is to just free the extra state and we
>>> are good.  I'm just not quite certain that is sane and it has been a
>>> long day.
>>
>> we used to have a lengthy e1000_pci_save|restore_state in our code, which is
>> now gone, so I'm all for that. A separate pci_save_pxie|msi(x)_state for 
>> every
>> driver seems completely unnecessary. I can't think of a use case where
>> saving+restoring everything hurts. That's what you want I presume.

I just want to understand why we have issues and to see if how we have
organized the suspend/resume path for dealing with msi irqs makes sense.

That is I haven't looked much at the suspend/resume path so I don't know it
well and I am afraid that your problem might be a symptom of a deeper
problem.

>> We currently free all irq's and msi before going into suspend in e1000, and I
>> think that is probably a good thing, somehow I can think of bad things
>> happening if we dont, but I admit that I haven't tried it without
>> alloc/free. We do this in e100 as well and it works.

Currently the irq code supports operation without the
free_irq/request_irq.  Since the numbers given are pure linux
abstractions things should it is really a matter of just
saving/restoring the appropriate state.

>> Another motivation would be to leave this up to the driver: if the driver
>> chooses to free/alloc interrupts because it makes sense, you probably would
>> want to keep that choice available. Devices that don't need this can skip the
>> alloc/free, but leave the choice open for others.
>
> ah, looking at the code in e1000 we do:
>
> _suspend:
>   pci_save_state();
>   free_irq()
>
> _resume:
>   pci_restore_state();
>   alloc_irq();
>
> I suppose that's not good either, and the major cause of the warning in the
> first place.

Yep. 

> Maybe I can rollback your latest patches and try to fix that mess by 
> postponing
> the pci_save_state until after we free'd the irq's.

Below is an additional set of warnings that should help debug this.
The old code just got lucky that it triggered a warning when this happens.

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 01869b1..5113913 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -613,6 +613,7 @@ int pci_enable_msi(struct pci_dev* dev)
return -EINVAL;
 
WARN_ON(!!dev->msi_enabled);
+   WARN_ON(!hlist_empty(>saved_cap_space));
 
/* Check whether driver already requested for MSI-X irqs */
if (dev->msix_enabled) {
@@ -638,6 +639,8 @@ void pci_disable_msi(struct pci_dev* dev)
if (!dev->msi_enabled)
return;
 
+   WARN_ON(!hlist_empty(>saved_cap_space));
+
msi_set_enable(dev, 0);
pci_intx(dev, 1);   /* enable intx */
dev->msi_enabled = 0;
@@ -739,6 +742,7 @@ int pci_enable_msix(struct pci_dev* dev, struct msix_entry 
*entries, int nvec)
}
}
WARN_ON(!!dev->msix_enabled);
+   WARN_ON(!hlist_empty(>saved_cap_space));
 
/* Check whether driver already requested for MSI irq */
if (dev->msi_enabled) {
@@ -763,6 +767,8 @@ void pci_disable_msix(struct pci_dev* dev)
if (!dev->msix_enabled)
return;
 
+   WARN_ON(!hlist_empty(>saved_cap_space));
+
msix_set_enable(dev, 0);
pci_intx(dev, 1);   /* enable intx */
dev->msix_enabled = 0;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bd44a48..4418839 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -677,6 +677,7 @@ pci_restore_state(struct pci_dev *dev)
}
pci_restore_pcix_state(dev);
pci_restore_msi_state(dev);
+   WARN_ON(!hlist_empty(>saved_cap_space));
 
return 0;
 }
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Kok, Auke wrote:

Eric W. Biederman wrote:

"Kok, Auke" <[EMAIL PROTECTED]> writes:


Ingo Molnar wrote:

* Kok, Auke <[EMAIL PROTECTED]> wrote:


BUG: at drivers/pci/msi.c:611 pci_enable_msi()

I would poke Eric Biederman(sp?) about this one.  Maybe its even solved by
the MSI-enable-related patch he posted in the past 24-48 hours.

I tried the 3-patch series "[PATCH 0/3] Basic msi bug fixes.." and they fix
this problem for me. Were you expecting the OOPS in the first place? [...]

the bug was the warning message (a WARN_ON()) above - not an oops. So that
warning message is gone in your testing?

yes.

Sorry for the slow delay.  I was out of town for my brothers wedding the last 
few
days.

I wasn't exactly expecting the WARN_ON to trigger.  What I fixed was
an inconsistency in handling our state bits.  Fixing that
inconsistency appears to have fixed the e1000 usage scenario mostly by
accident.

The basic issue is that pci_save_state saves the current msi state
along with other registers, and then the e1000 driver goes and
disables the msi irq after we have saved the irq state as on.

My code notices that the msi irq was disabled before restore time, so
it skips the restore.  However we now have a leak of the msi saved cap
because we are not freeing it. 


This leaves with some basic questions.
- Does it make sense for suspend/resume methods to request/free irqs?
- Does it make sense for suspend/resume methods to allocate/free msi irqs?
- Do we want pci_save/restore_cap to save/restore msi state?

The path of least resistance is to just free the extra state and we
are good.  I'm just not quite certain that is sane and it has been a
long day.


we used to have a lengthy e1000_pci_save|restore_state in our code, which is now 
gone, so I'm all for that. A separate pci_save_pxie|msi(x)_state for every 
driver seems completely unnecessary. I can't think of a use case where 
saving+restoring everything hurts. That's what you want I presume.


We currently free all irq's and msi before going into suspend in e1000, and I 
think that is probably a good thing, somehow I can think of bad things happening 
if we dont, but I admit that I haven't tried it without alloc/free. We do this 
in e100 as well and it works.


Another motivation would be to leave this up to the driver: if the driver 
chooses to free/alloc interrupts because it makes sense, you probably would want 
to keep that choice available. Devices that don't need this can skip the 
alloc/free, but leave the choice open for others.


ah, looking at the code in e1000 we do:

_suspend:
pci_save_state();
free_irq()

_resume:
pci_restore_state();
alloc_irq();

I suppose that's not good either, and the major cause of the warning in the 
first place.


Maybe I can rollback your latest patches and try to fix that mess by postponing 
the pci_save_state until after we free'd the irq's.


Auke
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Eric W. Biederman wrote:

"Kok, Auke" <[EMAIL PROTECTED]> writes:


Ingo Molnar wrote:

* Kok, Auke <[EMAIL PROTECTED]> wrote:


BUG: at drivers/pci/msi.c:611 pci_enable_msi()

I would poke Eric Biederman(sp?) about this one.  Maybe its even solved by
the MSI-enable-related patch he posted in the past 24-48 hours.

I tried the 3-patch series "[PATCH 0/3] Basic msi bug fixes.." and they fix
this problem for me. Were you expecting the OOPS in the first place? [...]

the bug was the warning message (a WARN_ON()) above - not an oops. So that
warning message is gone in your testing?

yes.


Sorry for the slow delay.  I was out of town for my brothers wedding the last 
few
days.

I wasn't exactly expecting the WARN_ON to trigger.  What I fixed was
an inconsistency in handling our state bits.  Fixing that
inconsistency appears to have fixed the e1000 usage scenario mostly by
accident.

The basic issue is that pci_save_state saves the current msi state
along with other registers, and then the e1000 driver goes and
disables the msi irq after we have saved the irq state as on.

My code notices that the msi irq was disabled before restore time, so
it skips the restore.  However we now have a leak of the msi saved cap
because we are not freeing it. 


This leaves with some basic questions.
- Does it make sense for suspend/resume methods to request/free irqs?
- Does it make sense for suspend/resume methods to allocate/free msi irqs?
- Do we want pci_save/restore_cap to save/restore msi state?

The path of least resistance is to just free the extra state and we
are good.  I'm just not quite certain that is sane and it has been a
long day.


we used to have a lengthy e1000_pci_save|restore_state in our code, which is now 
gone, so I'm all for that. A separate pci_save_pxie|msi(x)_state for every 
driver seems completely unnecessary. I can't think of a use case where 
saving+restoring everything hurts. That's what you want I presume.


We currently free all irq's and msi before going into suspend in e1000, and I 
think that is probably a good thing, somehow I can think of bad things happening 
if we dont, but I admit that I haven't tried it without alloc/free. We do this 
in e100 as well and it works.


Another motivation would be to leave this up to the driver: if the driver 
chooses to free/alloc interrupts because it makes sense, you probably would want 
to keep that choice available. Devices that don't need this can skip the 
alloc/free, but leave the choice open for others.


hth

Auke
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Eric W. Biederman wrote:

Kok, Auke [EMAIL PROTECTED] writes:


Ingo Molnar wrote:

* Kok, Auke [EMAIL PROTECTED] wrote:


BUG: at drivers/pci/msi.c:611 pci_enable_msi()

I would poke Eric Biederman(sp?) about this one.  Maybe its even solved by
the MSI-enable-related patch he posted in the past 24-48 hours.

I tried the 3-patch series [PATCH 0/3] Basic msi bug fixes.. and they fix
this problem for me. Were you expecting the OOPS in the first place? [...]

the bug was the warning message (a WARN_ON()) above - not an oops. So that
warning message is gone in your testing?

yes.


Sorry for the slow delay.  I was out of town for my brothers wedding the last 
few
days.

I wasn't exactly expecting the WARN_ON to trigger.  What I fixed was
an inconsistency in handling our state bits.  Fixing that
inconsistency appears to have fixed the e1000 usage scenario mostly by
accident.

The basic issue is that pci_save_state saves the current msi state
along with other registers, and then the e1000 driver goes and
disables the msi irq after we have saved the irq state as on.

My code notices that the msi irq was disabled before restore time, so
it skips the restore.  However we now have a leak of the msi saved cap
because we are not freeing it. 


This leaves with some basic questions.
- Does it make sense for suspend/resume methods to request/free irqs?
- Does it make sense for suspend/resume methods to allocate/free msi irqs?
- Do we want pci_save/restore_cap to save/restore msi state?

The path of least resistance is to just free the extra state and we
are good.  I'm just not quite certain that is sane and it has been a
long day.


we used to have a lengthy e1000_pci_save|restore_state in our code, which is now 
gone, so I'm all for that. A separate pci_save_pxie|msi(x)_state for every 
driver seems completely unnecessary. I can't think of a use case where 
saving+restoring everything hurts. That's what you want I presume.


We currently free all irq's and msi before going into suspend in e1000, and I 
think that is probably a good thing, somehow I can think of bad things happening 
if we dont, but I admit that I haven't tried it without alloc/free. We do this 
in e100 as well and it works.


Another motivation would be to leave this up to the driver: if the driver 
chooses to free/alloc interrupts because it makes sense, you probably would want 
to keep that choice available. Devices that don't need this can skip the 
alloc/free, but leave the choice open for others.


hth

Auke
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Kok, Auke wrote:

Eric W. Biederman wrote:

Kok, Auke [EMAIL PROTECTED] writes:


Ingo Molnar wrote:

* Kok, Auke [EMAIL PROTECTED] wrote:


BUG: at drivers/pci/msi.c:611 pci_enable_msi()

I would poke Eric Biederman(sp?) about this one.  Maybe its even solved by
the MSI-enable-related patch he posted in the past 24-48 hours.

I tried the 3-patch series [PATCH 0/3] Basic msi bug fixes.. and they fix
this problem for me. Were you expecting the OOPS in the first place? [...]

the bug was the warning message (a WARN_ON()) above - not an oops. So that
warning message is gone in your testing?

yes.

Sorry for the slow delay.  I was out of town for my brothers wedding the last 
few
days.

I wasn't exactly expecting the WARN_ON to trigger.  What I fixed was
an inconsistency in handling our state bits.  Fixing that
inconsistency appears to have fixed the e1000 usage scenario mostly by
accident.

The basic issue is that pci_save_state saves the current msi state
along with other registers, and then the e1000 driver goes and
disables the msi irq after we have saved the irq state as on.

My code notices that the msi irq was disabled before restore time, so
it skips the restore.  However we now have a leak of the msi saved cap
because we are not freeing it. 


This leaves with some basic questions.
- Does it make sense for suspend/resume methods to request/free irqs?
- Does it make sense for suspend/resume methods to allocate/free msi irqs?
- Do we want pci_save/restore_cap to save/restore msi state?

The path of least resistance is to just free the extra state and we
are good.  I'm just not quite certain that is sane and it has been a
long day.


we used to have a lengthy e1000_pci_save|restore_state in our code, which is now 
gone, so I'm all for that. A separate pci_save_pxie|msi(x)_state for every 
driver seems completely unnecessary. I can't think of a use case where 
saving+restoring everything hurts. That's what you want I presume.


We currently free all irq's and msi before going into suspend in e1000, and I 
think that is probably a good thing, somehow I can think of bad things happening 
if we dont, but I admit that I haven't tried it without alloc/free. We do this 
in e100 as well and it works.


Another motivation would be to leave this up to the driver: if the driver 
chooses to free/alloc interrupts because it makes sense, you probably would want 
to keep that choice available. Devices that don't need this can skip the 
alloc/free, but leave the choice open for others.


ah, looking at the code in e1000 we do:

_suspend:
pci_save_state();
free_irq()

_resume:
pci_restore_state();
alloc_irq();

I suppose that's not good either, and the major cause of the warning in the 
first place.


Maybe I can rollback your latest patches and try to fix that mess by postponing 
the pci_save_state until after we free'd the irq's.


Auke
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Kok, Auke [EMAIL PROTECTED] writes:

 Kok, Auke wrote:
 Eric W. Biederman wrote:

 This leaves with some basic questions.
 - Does it make sense for suspend/resume methods to request/free irqs?
 - Does it make sense for suspend/resume methods to allocate/free msi irqs?
 - Do we want pci_save/restore_cap to save/restore msi state?

 The path of least resistance is to just free the extra state and we
 are good.  I'm just not quite certain that is sane and it has been a
 long day.

 we used to have a lengthy e1000_pci_save|restore_state in our code, which is
 now gone, so I'm all for that. A separate pci_save_pxie|msi(x)_state for 
 every
 driver seems completely unnecessary. I can't think of a use case where
 saving+restoring everything hurts. That's what you want I presume.

I just want to understand why we have issues and to see if how we have
organized the suspend/resume path for dealing with msi irqs makes sense.

That is I haven't looked much at the suspend/resume path so I don't know it
well and I am afraid that your problem might be a symptom of a deeper
problem.

 We currently free all irq's and msi before going into suspend in e1000, and I
 think that is probably a good thing, somehow I can think of bad things
 happening if we dont, but I admit that I haven't tried it without
 alloc/free. We do this in e100 as well and it works.

Currently the irq code supports operation without the
free_irq/request_irq.  Since the numbers given are pure linux
abstractions things should it is really a matter of just
saving/restoring the appropriate state.

 Another motivation would be to leave this up to the driver: if the driver
 chooses to free/alloc interrupts because it makes sense, you probably would
 want to keep that choice available. Devices that don't need this can skip the
 alloc/free, but leave the choice open for others.

 ah, looking at the code in e1000 we do:

 _suspend:
   pci_save_state();
   free_irq()

 _resume:
   pci_restore_state();
   alloc_irq();

 I suppose that's not good either, and the major cause of the warning in the
 first place.

Yep. 

 Maybe I can rollback your latest patches and try to fix that mess by 
 postponing
 the pci_save_state until after we free'd the irq's.

Below is an additional set of warnings that should help debug this.
The old code just got lucky that it triggered a warning when this happens.

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 01869b1..5113913 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -613,6 +613,7 @@ int pci_enable_msi(struct pci_dev* dev)
return -EINVAL;
 
WARN_ON(!!dev-msi_enabled);
+   WARN_ON(!hlist_empty(dev-saved_cap_space));
 
/* Check whether driver already requested for MSI-X irqs */
if (dev-msix_enabled) {
@@ -638,6 +639,8 @@ void pci_disable_msi(struct pci_dev* dev)
if (!dev-msi_enabled)
return;
 
+   WARN_ON(!hlist_empty(dev-saved_cap_space));
+
msi_set_enable(dev, 0);
pci_intx(dev, 1);   /* enable intx */
dev-msi_enabled = 0;
@@ -739,6 +742,7 @@ int pci_enable_msix(struct pci_dev* dev, struct msix_entry 
*entries, int nvec)
}
}
WARN_ON(!!dev-msix_enabled);
+   WARN_ON(!hlist_empty(dev-saved_cap_space));
 
/* Check whether driver already requested for MSI irq */
if (dev-msi_enabled) {
@@ -763,6 +767,8 @@ void pci_disable_msix(struct pci_dev* dev)
if (!dev-msix_enabled)
return;
 
+   WARN_ON(!hlist_empty(dev-saved_cap_space));
+
msix_set_enable(dev, 0);
pci_intx(dev, 1);   /* enable intx */
dev-msix_enabled = 0;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bd44a48..4418839 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -677,6 +677,7 @@ pci_restore_state(struct pci_dev *dev)
}
pci_restore_pcix_state(dev);
pci_restore_msi_state(dev);
+   WARN_ON(!hlist_empty(dev-saved_cap_space));
 
return 0;
 }
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Andrew Morton]

On Wed, 07 Mar 2007 12:28:11 -0700 [EMAIL PROTECTED] (Eric W. Biederman) wrote:

 Below is an additional set of warnings that should help debug this.
 The old code just got lucky that it triggered a warning when this happens.
 
 diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
 index 01869b1..5113913 100644
 --- a/drivers/pci/msi.c
 +++ b/drivers/pci/msi.c
 @@ -613,6 +613,7 @@ int pci_enable_msi(struct pci_dev* dev)
   return -EINVAL;
  
   WARN_ON(!!dev-msi_enabled);
 + WARN_ON(!hlist_empty(dev-saved_cap_space));
  
   /* Check whether driver already requested for MSI-X irqs */
   if (dev-msix_enabled) {
 @@ -638,6 +639,8 @@ void pci_disable_msi(struct pci_dev* dev)
   if (!dev-msi_enabled)
   return;
  
 + WARN_ON(!hlist_empty(dev-saved_cap_space));
 +
   msi_set_enable(dev, 0);
   pci_intx(dev, 1);   /* enable intx */
   dev-msi_enabled = 0;
 @@ -739,6 +742,7 @@ int pci_enable_msix(struct pci_dev* dev, struct 
 msix_entry *entries, int nvec)
   }
   }
   WARN_ON(!!dev-msix_enabled);
 + WARN_ON(!hlist_empty(dev-saved_cap_space));
  
   /* Check whether driver already requested for MSI irq */
   if (dev-msi_enabled) {
 @@ -763,6 +767,8 @@ void pci_disable_msix(struct pci_dev* dev)
   if (!dev-msix_enabled)
   return;
  
 + WARN_ON(!hlist_empty(dev-saved_cap_space));
 +
   msix_set_enable(dev, 0);
   pci_intx(dev, 1);   /* enable intx */
   dev-msix_enabled = 0;
 diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
 index bd44a48..4418839 100644
 --- a/drivers/pci/pci.c
 +++ b/drivers/pci/pci.c
 @@ -677,6 +677,7 @@ pci_restore_state(struct pci_dev *dev)
   }
   pci_restore_pcix_state(dev);
   pci_restore_msi_state(dev);
 + WARN_ON(!hlist_empty(dev-saved_cap_space));
  
   return 0;
  }

Got a hit on a powerpc g5:

PM: Writing back config space on device 0001:05:04.0 at offset 1 (was 2b0, 
writing 2b6)
[ cut here ]
Badness at drivers/pci/pci.c:679
Call Trace:
[C80F7410] [C0011EFC] .show_stack+0x50/0x1cc (unreliable)
[C80F74C0] [C01AD610] .report_bug+0xa0/0x110
[C80F7550] [C00256E4] .program_check_exception+0xb4/0x670
[C80F7630] [C00046F4] program_check_common+0xf4/0x100
--- Exception: 700 at .pci_restore_state+0x310/0x340
LR = .pci_restore_state+0x2e0/0x340
[C80F79D0] [C026A174] .tg3_chip_reset+0x19c/0xa04
[C80F7A90] [C026D948] .tg3_reset_hw+0xa4/0x2718
[C80F7BA0] [C0270030] .tg3_init_hw+0x74/0x94
[C80F7C30] [C0270BE0] .tg3_open+0x4c8/0x854
[C80F7CF0] [C03A74A4] .dev_open+0x100/0x12c
[C80F7D90] [C03BAEA8] .netpoll_setup+0x2dc/0x3ec
[C80F7E40] [C0283450] .init_netconsole+0x64/0x8c
[C80F7EC0] [C05C0BE4] .init+0x1d0/0x390
[C80F7F90] [C00271F8] .kernel_thread+0x4c/0x68
tg3: eth0: Link is up at 1000 Mbps, full duplex.
tg3: eth0: Flow control is on for TX and on for RX.
Scheduler bitmap error - bitmap being reconstructed..
netconsole: network logging started
Calling initcall 0xc06bd180: .macio_module_init+0x0/0x3c()

That's:

pci_restore_pcix_state(dev);
pci_restore_msi_state(dev);
WARN_ON(!hlist_empty(dev-saved_cap_space));

return 0;

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Andrew Morton [EMAIL PROTECTED] writes:


 That's:

 pci_restore_pcix_state(dev);
 pci_restore_msi_state(dev);
 WARN_ON(!hlist_empty(dev-saved_cap_space));

 return 0;

Hmm.  Either I am confused of I just found an unanticipated leak.

pci_restore_msi_state should be out of the picture as we don't yet
have ppc msi support and I don't think the g5 generation hardware
supported it either.

The only case I can see which might trigger this is if we saved
pci-X state and then didn't restore it because we could not find
the capability on restore.

Any chance you could walk that list and find the cap_nr of the remaining
element?  

Something like:
{
struct pci_cap_saved_state *tmp;
struct hlist_node *pos;

hlist_for_each_entry(tmp, pos, pci_dev-saved_cap_space, next)
printk(KERN_INFO saved_cap: 0x%02x\n, tmp-cap_nr);
}

Until I get the best scenario I can come up with is a tg3 hardware bug
that doesn't renable the pci-X capability after a restore of power state.

Getting that cap_nr will at least allow me to be certain if I am dealing
with msi, pci-X or pci-e.

Unanticipated bugs aren't supposed to be this easy to find!

Eric

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

"Kok, Auke" <[EMAIL PROTECTED]> writes:

> Ingo Molnar wrote:
>> * Kok, Auke <[EMAIL PROTECTED]> wrote:
>>
> BUG: at drivers/pci/msi.c:611 pci_enable_msi()
>>
 I would poke Eric Biederman(sp?) about this one.  Maybe its even solved by
 the MSI-enable-related patch he posted in the past 24-48 hours.
>>> I tried the 3-patch series "[PATCH 0/3] Basic msi bug fixes.." and they fix
>>> this problem for me. Were you expecting the OOPS in the first place? [...]
>>
>> the bug was the warning message (a WARN_ON()) above - not an oops. So that
>> warning message is gone in your testing?
>
> yes.

Sorry for the slow delay.  I was out of town for my brothers wedding the last 
few
days.

I wasn't exactly expecting the WARN_ON to trigger.  What I fixed was
an inconsistency in handling our state bits.  Fixing that
inconsistency appears to have fixed the e1000 usage scenario mostly by
accident.

The basic issue is that pci_save_state saves the current msi state
along with other registers, and then the e1000 driver goes and
disables the msi irq after we have saved the irq state as on.

My code notices that the msi irq was disabled before restore time, so
it skips the restore.  However we now have a leak of the msi saved cap
because we are not freeing it. 

This leaves with some basic questions.
- Does it make sense for suspend/resume methods to request/free irqs?
- Does it make sense for suspend/resume methods to allocate/free msi irqs?
- Do we want pci_save/restore_cap to save/restore msi state?

The path of least resistance is to just free the extra state and we
are good.  I'm just not quite certain that is sane and it has been a
long day.

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Linus Torvalds wrote:


On Tue, 6 Mar 2007, Thomas Gleixner wrote:

SATA has another nice feature. Somehow there is an interrupt pending on
the SATA controller, which comes in somewhere in the middle of resume.
If it happens before the SATA code resumed, the SATA code ignores the
interrupt and the interrupt is disabled due to "nobody cared", which in
turn prevents SATA to ever become functional again. >


And if you don't want to do any of these things (or are unable to, because 
of some ordering constraint or bad design), then you simply need to 
unregister and re-register the interrupt handler over sleep.



Any idea on that one ?


Jeff, Auke, does this ring any bells?


For the e1000 issue, the problem is solved with Eric Biederman's 3-patch msi 
cleanups. You should have another message in your mailbox confirming that I 
tested his patches and the MSI warning for e1000 suspend-resume is gone with them.


Cheers,

Auke
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Linus Torvalds]

On Tue, 6 Mar 2007, Thomas Gleixner wrote:
> 
> SATA has another nice feature. Somehow there is an interrupt pending on
> the SATA controller, which comes in somewhere in the middle of resume.
> If it happens before the SATA code resumed, the SATA code ignores the
> interrupt and the interrupt is disabled due to "nobody cared", which in
> turn prevents SATA to ever become functional again.

Jeff - that sounds like a SATA bug.

If you have an interrupt handler registered, you'd better handle the 
interrupt regardless of whether you think the hardware might be gone or 
not.

It's generally *not* ok to do

if (device_offline())
return IRQ_NONE;

at the top of an interrupt handler. 

Of course, if you think the hardware is supposed to be quiescent, then the 
only thing you should do is generally just do the "shut up" operation (ie 
read status, write it back or whatever). You must generally *not* try to 
pass any data upwards (ie if the higher layers have told you to shut up, 
you may need to handle the hardware, but you must not involve the higher 
layers themselves any more, because they expect you to be quiet).

And if you cannot do that because you need to resume in order to have the 
status register mapped, then you need to have an "early_resume()" function 
which gets called *before* interrupts are enabled. That's what 
early-resume (and late-suspend) are designed for: doing things that happen 
very early in the resume sequence before everything is up.

And if you don't want to do any of these things (or are unable to, because 
of some ordering constraint or bad design), then you simply need to 
unregister and re-register the interrupt handler over sleep.

> Any idea on that one ?

Jeff, Auke, does this ring any bells?

Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Thomas Gleixner]

On Mon, 2007-03-05 at 11:11 +0100, Ingo Molnar wrote:
> the spin-up takes a few seconds here under suspend/resume simulation:
> 
>  | ata1: waiting for device to spin up (7 secs)
>  | Restarting tasks ... done.
> 
>  [5-10 seconds pass]
> 
>  | ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>  | ata1.00: configured for UDMA/100
>  | SCSI device sda: 156301488 512-byte hdwr sectors (80026 MB)
>  | sda: Write Protect is off
>  | sda: Mode Sense: 00 3a 00 00
>  | SCSI device sda: write cache: enabled, read cache: enabled, doesn't 
> support DPO or FUA
> 
> with real resume it takes even longer time - but i dont see where the 
> delays come from in that case - i suspect it's SATA.

SATA has another nice feature. Somehow there is an interrupt pending on
the SATA controller, which comes in somewhere in the middle of resume.
If it happens before the SATA code resumed, the SATA code ignores the
interrupt and the interrupt is disabled due to "nobody cared", which in
turn prevents SATA to ever become functional again.

Any idea on that one ?

tglx


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Ingo Molnar wrote:

* Kok, Auke <[EMAIL PROTECTED]> wrote:


BUG: at drivers/pci/msi.c:611 pci_enable_msi()


I would poke Eric Biederman(sp?) about this one.  Maybe its even 
solved by the MSI-enable-related patch he posted in the past 24-48 
hours.
I tried the 3-patch series "[PATCH 0/3] Basic msi bug fixes.." and 
they fix this problem for me. Were you expecting the OOPS in the first 
place? [...]


the bug was the warning message (a WARN_ON()) above - not an oops. So 
that warning message is gone in your testing?


yes.

Auke
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Ingo Molnar]

* Ingo Molnar <[EMAIL PROTECTED]> wrote:

> with real resume it takes even longer time - but i dont see where the 
> delays come from in that case - i suspect it's SATA.

update: Thomas' PIT/HPET resume-fix patch fixed the delay for me.

Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Ingo Molnar]

* Kok, Auke <[EMAIL PROTECTED]> wrote:

> >>BUG: at drivers/pci/msi.c:611 pci_enable_msi()

> >I would poke Eric Biederman(sp?) about this one.  Maybe its even 
> >solved by the MSI-enable-related patch he posted in the past 24-48 
> >hours.
> 
> I tried the 3-patch series "[PATCH 0/3] Basic msi bug fixes.." and 
> they fix this problem for me. Were you expecting the OOPS in the first 
> place? [...]

the bug was the warning message (a WARN_ON()) above - not an oops. So 
that warning message is gone in your testing?

Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Ingo Molnar]

* Kok, Auke [EMAIL PROTECTED] wrote:

 BUG: at drivers/pci/msi.c:611 pci_enable_msi()

 I would poke Eric Biederman(sp?) about this one.  Maybe its even 
 solved by the MSI-enable-related patch he posted in the past 24-48 
 hours.
 
 I tried the 3-patch series [PATCH 0/3] Basic msi bug fixes.. and 
 they fix this problem for me. Were you expecting the OOPS in the first 
 place? [...]

the bug was the warning message (a WARN_ON()) above - not an oops. So 
that warning message is gone in your testing?

Ingo
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Ingo Molnar]

* Ingo Molnar [EMAIL PROTECTED] wrote:

 with real resume it takes even longer time - but i dont see where the 
 delays come from in that case - i suspect it's SATA.

update: Thomas' PIT/HPET resume-fix patch fixed the delay for me.

Ingo
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Ingo Molnar wrote:

* Kok, Auke [EMAIL PROTECTED] wrote:


BUG: at drivers/pci/msi.c:611 pci_enable_msi()


I would poke Eric Biederman(sp?) about this one.  Maybe its even 
solved by the MSI-enable-related patch he posted in the past 24-48 
hours.
I tried the 3-patch series [PATCH 0/3] Basic msi bug fixes.. and 
they fix this problem for me. Were you expecting the OOPS in the first 
place? [...]


the bug was the warning message (a WARN_ON()) above - not an oops. So 
that warning message is gone in your testing?


yes.

Auke
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Thomas Gleixner]

On Mon, 2007-03-05 at 11:11 +0100, Ingo Molnar wrote:
 the spin-up takes a few seconds here under suspend/resume simulation:
 
  | ata1: waiting for device to spin up (7 secs)
  | Restarting tasks ... done.
 
  [5-10 seconds pass]
 
  | ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
  | ata1.00: configured for UDMA/100
  | SCSI device sda: 156301488 512-byte hdwr sectors (80026 MB)
  | sda: Write Protect is off
  | sda: Mode Sense: 00 3a 00 00
  | SCSI device sda: write cache: enabled, read cache: enabled, doesn't 
 support DPO or FUA
 
 with real resume it takes even longer time - but i dont see where the 
 delays come from in that case - i suspect it's SATA.

SATA has another nice feature. Somehow there is an interrupt pending on
the SATA controller, which comes in somewhere in the middle of resume.
If it happens before the SATA code resumed, the SATA code ignores the
interrupt and the interrupt is disabled due to nobody cared, which in
turn prevents SATA to ever become functional again.

Any idea on that one ?

tglx


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Linus Torvalds]

On Tue, 6 Mar 2007, Thomas Gleixner wrote:
 
 SATA has another nice feature. Somehow there is an interrupt pending on
 the SATA controller, which comes in somewhere in the middle of resume.
 If it happens before the SATA code resumed, the SATA code ignores the
 interrupt and the interrupt is disabled due to nobody cared, which in
 turn prevents SATA to ever become functional again.

Jeff - that sounds like a SATA bug.

If you have an interrupt handler registered, you'd better handle the 
interrupt regardless of whether you think the hardware might be gone or 
not.

It's generally *not* ok to do

if (device_offline())
return IRQ_NONE;

at the top of an interrupt handler. 

Of course, if you think the hardware is supposed to be quiescent, then the 
only thing you should do is generally just do the shut up operation (ie 
read status, write it back or whatever). You must generally *not* try to 
pass any data upwards (ie if the higher layers have told you to shut up, 
you may need to handle the hardware, but you must not involve the higher 
layers themselves any more, because they expect you to be quiet).

And if you cannot do that because you need to resume in order to have the 
status register mapped, then you need to have an early_resume() function 
which gets called *before* interrupts are enabled. That's what 
early-resume (and late-suspend) are designed for: doing things that happen 
very early in the resume sequence before everything is up.

And if you don't want to do any of these things (or are unable to, because 
of some ordering constraint or bad design), then you simply need to 
unregister and re-register the interrupt handler over sleep.

 Any idea on that one ?

Jeff, Auke, does this ring any bells?

Linus
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Linus Torvalds wrote:


On Tue, 6 Mar 2007, Thomas Gleixner wrote:

SATA has another nice feature. Somehow there is an interrupt pending on
the SATA controller, which comes in somewhere in the middle of resume.
If it happens before the SATA code resumed, the SATA code ignores the
interrupt and the interrupt is disabled due to nobody cared, which in
turn prevents SATA to ever become functional again. 


And if you don't want to do any of these things (or are unable to, because 
of some ordering constraint or bad design), then you simply need to 
unregister and re-register the interrupt handler over sleep.



Any idea on that one ?


Jeff, Auke, does this ring any bells?


For the e1000 issue, the problem is solved with Eric Biederman's 3-patch msi 
cleanups. You should have another message in your mailbox confirming that I 
tested his patches and the MSI warning for e1000 suspend-resume is gone with them.


Cheers,

Auke
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Eric W. Biederman]

Kok, Auke [EMAIL PROTECTED] writes:

 Ingo Molnar wrote:
 * Kok, Auke [EMAIL PROTECTED] wrote:

 BUG: at drivers/pci/msi.c:611 pci_enable_msi()

 I would poke Eric Biederman(sp?) about this one.  Maybe its even solved by
 the MSI-enable-related patch he posted in the past 24-48 hours.
 I tried the 3-patch series [PATCH 0/3] Basic msi bug fixes.. and they fix
 this problem for me. Were you expecting the OOPS in the first place? [...]

 the bug was the warning message (a WARN_ON()) above - not an oops. So that
 warning message is gone in your testing?

 yes.

Sorry for the slow delay.  I was out of town for my brothers wedding the last 
few
days.

I wasn't exactly expecting the WARN_ON to trigger.  What I fixed was
an inconsistency in handling our state bits.  Fixing that
inconsistency appears to have fixed the e1000 usage scenario mostly by
accident.

The basic issue is that pci_save_state saves the current msi state
along with other registers, and then the e1000 driver goes and
disables the msi irq after we have saved the irq state as on.

My code notices that the msi irq was disabled before restore time, so
it skips the restore.  However we now have a leak of the msi saved cap
because we are not freeing it. 

This leaves with some basic questions.
- Does it make sense for suspend/resume methods to request/free irqs?
- Does it make sense for suspend/resume methods to allocate/free msi irqs?
- Do we want pci_save/restore_cap to save/restore msi state?

The path of least resistance is to just free the extra state and we
are good.  I'm just not quite certain that is sane and it has been a
long day.

Eric
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Jeff Garzik wrote:

Ingo Molnar wrote:

i'm also getting this WARN_ON() from e1000:

BUG: at drivers/pci/msi.c:611 pci_enable_msi()
 [] show_trace_log_lvl+0x19/0x2e
 [] show_trace+0x12/0x14
 [] dump_stack+0x14/0x16
 [] pci_enable_msi+0x6d/0x203
 [] e1000_request_irq+0x2e/0xe2
 [] e1000_resume+0x7f/0xef
 [] pci_device_resume+0x1a/0x44
 [] resume_device+0xf7/0x16f
 [] dpm_resume+0x77/0xcb
 [] device_resume+0x3a/0x51
 [] enter_state+0x193/0x1bb
 [] state_store+0x81/0x97
 [] subsys_attr_store+0x20/0x25
 [] sysfs_write_file+0xce/0xf6
 [] vfs_write+0xb1/0x13a
 [] sys_write+0x3d/0x61
 [] syscall_call+0x7/0xb

seems harmless because it seems to work fine.


I would poke Eric Biederman(sp?) about this one.  Maybe its even solved 
by the MSI-enable-related patch he posted in the past 24-48 hours.



Eric, Linus,

I tried the 3-patch series "[PATCH 0/3] Basic msi bug fixes.." and they fix this 
problem for me. Were you expecting the OOPS in the first place? In any case, it 
survived several suspend/resume cycles on both enabled (irq alloc'd and enabled) 
and disabled devices (only initialized).


Jens Axboe was seeing the same problem, perhaps he can confirm the fix as well.

In any case, the patches have my blessing :)

Please add my:

  Signed-off-by: Auke Kok <[EMAIL PROTECTED]>


Cheers,

Auke
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Jeff Garzik]

Ingo Molnar wrote:

i'm also getting this WARN_ON() from e1000:

BUG: at drivers/pci/msi.c:611 pci_enable_msi()
 [] show_trace_log_lvl+0x19/0x2e
 [] show_trace+0x12/0x14
 [] dump_stack+0x14/0x16
 [] pci_enable_msi+0x6d/0x203
 [] e1000_request_irq+0x2e/0xe2
 [] e1000_resume+0x7f/0xef
 [] pci_device_resume+0x1a/0x44
 [] resume_device+0xf7/0x16f
 [] dpm_resume+0x77/0xcb
 [] device_resume+0x3a/0x51
 [] enter_state+0x193/0x1bb
 [] state_store+0x81/0x97
 [] subsys_attr_store+0x20/0x25
 [] sysfs_write_file+0xce/0xf6
 [] vfs_write+0xb1/0x13a
 [] sys_write+0x3d/0x61
 [] syscall_call+0x7/0xb

seems harmless because it seems to work fine.



I would poke Eric Biederman(sp?) about this one.  Maybe its even solved 
by the MSI-enable-related patch he posted in the past 24-48 hours.


Jeff


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

SATA resume slowness, e1000 MSI warning

[Ingo Molnar]

* Michael S. Tsirkin <[EMAIL PROTECTED]> wrote:

> > (Resume is very slow, because disks are not spinned up properly; and
> > there's something wrong with timers; console beeps take way too long).

> Pavel, I tried with your .config, and indeed the system came back to 
> life after 2-3 minutes after I press Fn/F4, indeed the issue seems to 
> be with the disk. It could be that the same takes place with my 
> original .config - maybe I just wasn't patient enough. I'll need to 
> re-test that.

the spin-up takes a few seconds here under suspend/resume simulation:

 | ata1: waiting for device to spin up (7 secs)
 | Restarting tasks ... done.

 [5-10 seconds pass]

 | ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
 | ata1.00: configured for UDMA/100
 | SCSI device sda: 156301488 512-byte hdwr sectors (80026 MB)
 | sda: Write Protect is off
 | sda: Mode Sense: 00 3a 00 00
 | SCSI device sda: write cache: enabled, read cache: enabled, doesn't support 
DPO or FUA

with real resume it takes even longer time - but i dont see where the 
delays come from in that case - i suspect it's SATA.

i'm also getting this WARN_ON() from e1000:

BUG: at drivers/pci/msi.c:611 pci_enable_msi()
 [] show_trace_log_lvl+0x19/0x2e
 [] show_trace+0x12/0x14
 [] dump_stack+0x14/0x16
 [] pci_enable_msi+0x6d/0x203
 [] e1000_request_irq+0x2e/0xe2
 [] e1000_resume+0x7f/0xef
 [] pci_device_resume+0x1a/0x44
 [] resume_device+0xf7/0x16f
 [] dpm_resume+0x77/0xcb
 [] device_resume+0x3a/0x51
 [] enter_state+0x193/0x1bb
 [] state_store+0x81/0x97
 [] subsys_attr_store+0x20/0x25
 [] sysfs_write_file+0xce/0xf6
 [] vfs_write+0xb1/0x13a
 [] sys_write+0x3d/0x61
 [] syscall_call+0x7/0xb

seems harmless because it seems to work fine.

Ingo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

SATA resume slowness, e1000 MSI warning

[Ingo Molnar]

* Michael S. Tsirkin [EMAIL PROTECTED] wrote:

  (Resume is very slow, because disks are not spinned up properly; and
  there's something wrong with timers; console beeps take way too long).

 Pavel, I tried with your .config, and indeed the system came back to 
 life after 2-3 minutes after I press Fn/F4, indeed the issue seems to 
 be with the disk. It could be that the same takes place with my 
 original .config - maybe I just wasn't patient enough. I'll need to 
 re-test that.

the spin-up takes a few seconds here under suspend/resume simulation:

 | ata1: waiting for device to spin up (7 secs)
 | Restarting tasks ... done.

 [5-10 seconds pass]

 | ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
 | ata1.00: configured for UDMA/100
 | SCSI device sda: 156301488 512-byte hdwr sectors (80026 MB)
 | sda: Write Protect is off
 | sda: Mode Sense: 00 3a 00 00
 | SCSI device sda: write cache: enabled, read cache: enabled, doesn't support 
DPO or FUA

with real resume it takes even longer time - but i dont see where the 
delays come from in that case - i suspect it's SATA.

i'm also getting this WARN_ON() from e1000:

BUG: at drivers/pci/msi.c:611 pci_enable_msi()
 [c01061bd] show_trace_log_lvl+0x19/0x2e
 [c01062b6] show_trace+0x12/0x14
 [c01062cc] dump_stack+0x14/0x16
 [c024fcc4] pci_enable_msi+0x6d/0x203
 [c02b709e] e1000_request_irq+0x2e/0xe2
 [c02bb742] e1000_resume+0x7f/0xef
 [c0249a68] pci_device_resume+0x1a/0x44
 [c02b39ec] resume_device+0xf7/0x16f
 [c02b3adb] dpm_resume+0x77/0xcb
 [c02b3b69] device_resume+0x3a/0x51
 [c014e669] enter_state+0x193/0x1bb
 [c014e712] state_store+0x81/0x97
 [c01b68bc] subsys_attr_store+0x20/0x25
 [c01b6feb] sysfs_write_file+0xce/0xf6
 [c017e16b] vfs_write+0xb1/0x13a
 [c017e899] sys_write+0x3d/0x61
 [c0105220] syscall_call+0x7/0xb

seems harmless because it seems to work fine.

Ingo
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Jeff Garzik]

Ingo Molnar wrote:

i'm also getting this WARN_ON() from e1000:

BUG: at drivers/pci/msi.c:611 pci_enable_msi()
 [c01061bd] show_trace_log_lvl+0x19/0x2e
 [c01062b6] show_trace+0x12/0x14
 [c01062cc] dump_stack+0x14/0x16
 [c024fcc4] pci_enable_msi+0x6d/0x203
 [c02b709e] e1000_request_irq+0x2e/0xe2
 [c02bb742] e1000_resume+0x7f/0xef
 [c0249a68] pci_device_resume+0x1a/0x44
 [c02b39ec] resume_device+0xf7/0x16f
 [c02b3adb] dpm_resume+0x77/0xcb
 [c02b3b69] device_resume+0x3a/0x51
 [c014e669] enter_state+0x193/0x1bb
 [c014e712] state_store+0x81/0x97
 [c01b68bc] subsys_attr_store+0x20/0x25
 [c01b6feb] sysfs_write_file+0xce/0xf6
 [c017e16b] vfs_write+0xb1/0x13a
 [c017e899] sys_write+0x3d/0x61
 [c0105220] syscall_call+0x7/0xb

seems harmless because it seems to work fine.



I would poke Eric Biederman(sp?) about this one.  Maybe its even solved 
by the MSI-enable-related patch he posted in the past 24-48 hours.


Jeff


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: SATA resume slowness, e1000 MSI warning

[Kok, Auke]

Jeff Garzik wrote:

Ingo Molnar wrote:

i'm also getting this WARN_ON() from e1000:

BUG: at drivers/pci/msi.c:611 pci_enable_msi()
 [c01061bd] show_trace_log_lvl+0x19/0x2e
 [c01062b6] show_trace+0x12/0x14
 [c01062cc] dump_stack+0x14/0x16
 [c024fcc4] pci_enable_msi+0x6d/0x203
 [c02b709e] e1000_request_irq+0x2e/0xe2
 [c02bb742] e1000_resume+0x7f/0xef
 [c0249a68] pci_device_resume+0x1a/0x44
 [c02b39ec] resume_device+0xf7/0x16f
 [c02b3adb] dpm_resume+0x77/0xcb
 [c02b3b69] device_resume+0x3a/0x51
 [c014e669] enter_state+0x193/0x1bb
 [c014e712] state_store+0x81/0x97
 [c01b68bc] subsys_attr_store+0x20/0x25
 [c01b6feb] sysfs_write_file+0xce/0xf6
 [c017e16b] vfs_write+0xb1/0x13a
 [c017e899] sys_write+0x3d/0x61
 [c0105220] syscall_call+0x7/0xb

seems harmless because it seems to work fine.


I would poke Eric Biederman(sp?) about this one.  Maybe its even solved 
by the MSI-enable-related patch he posted in the past 24-48 hours.



Eric, Linus,

I tried the 3-patch series [PATCH 0/3] Basic msi bug fixes.. and they fix this 
problem for me. Were you expecting the OOPS in the first place? In any case, it 
survived several suspend/resume cycles on both enabled (irq alloc'd and enabled) 
and disabled devices (only initialized).


Jens Axboe was seeing the same problem, perhaps he can confirm the fix as well.

In any case, the patches have my blessing :)

Please add my:

  Signed-off-by: Auke Kok [EMAIL PROTECTED]


Cheers,

Auke
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/