Linus Torvalds | 1 Jul 01:00 2009

Re: [BUG 2.6.31-rc1] HIGHMEM64G causes hang in PCI init on 32-bit x86


On Tue, 30 Jun 2009, Yinghai Lu wrote:
> 
> how about x = 0, y = 0x100?

Yeah, no. Peter's trick doesn't work. Good catch. 

I don't see any single-use trick then, and so it needs the whole statement 
expression mess.

		Linus
Linus Torvalds | 1 Jul 01:04 2009

Re: [BUG 2.6.31-rc1] HIGHMEM64G causes hang in PCI init on 32-bit x86


On Tue, 30 Jun 2009, Linus Torvalds wrote:
> 
> I don't see any single-use trick then, and so it needs the whole statement 
> expression mess.

Hmm. Does (((x)-1) | mask)+1) work?

I haven't thought it fully through, but that _should_ take care of the 
"already aligned" case, no?

		Linus
Yinghai Lu | 1 Jul 01:13 2009

Re: [BUG 2.6.31-rc1] HIGHMEM64G causes hang in PCI init on 32-bit x86

Linus Torvalds wrote:
> 
> On Tue, 30 Jun 2009, Linus Torvalds wrote:
>> I don't see any single-use trick then, and so it needs the whole statement 
>> expression mess.
> 
> Hmm. Does (((x)-1) | mask)+1) work?
> 
> I haven't thought it fully through, but that _should_ take care of the 
> "already aligned" case, no?

yes. that is right.

then how about
roundup(x,y)
round_up(x,y)

roundup doesn't need y is 2^n
but round_up does need y is 2^n, and only for x86

the name is confusing.

YH
H. Peter Anvin | 1 Jul 01:16 2009

Re: [BUG 2.6.31-rc1] HIGHMEM64G causes hang in PCI init on 32-bit x86

Linus Torvalds wrote:
> 
> On Tue, 30 Jun 2009, Yinghai Lu wrote:
>> how about x = 0, y = 0x100?
> 
> Yeah, no. Peter's trick doesn't work. Good catch. 
> 
> I don't see any single-use trick then, and so it needs the whole statement 
> expression mess.
> 

Yes, the | trick only works if you know you have a nonzero alignment pad
(or you want a full-block pad in the empty case.)

	-hpa

H. Peter Anvin | 1 Jul 01:19 2009

Re: [BUG 2.6.31-rc1] HIGHMEM64G causes hang in PCI init on 32-bit x86

Yinghai Lu wrote:
> Linus Torvalds wrote:
>> On Tue, 30 Jun 2009, Linus Torvalds wrote:
>>> I don't see any single-use trick then, and so it needs the whole statement 
>>> expression mess.
>> Hmm. Does (((x)-1) | mask)+1) work?
>>
>> I haven't thought it fully through, but that _should_ take care of the 
>> "already aligned" case, no?
> 
> yes. that is right.
> 
> then how about
> roundup(x,y)
> round_up(x,y)
> 
> roundup doesn't need y is 2^n
> but round_up does need y is 2^n, and only for x86
> 

We should definitely move whatever to global.  However, I do think it is
valuable to have something that can avoid divides even if the argument
is not necessarily a constant.

	-hpa
Yinghai Lu | 1 Jul 06:18 2009
Picon

pci_stub and kvm

[ 1966.343286]
[ 1966.343288] =======================================================
[ 1966.356756] [ INFO: possible circular locking dependency detected ]
[ 1966.356759] 2.6.31-rc1-tip-00978-g99123e5-dirty #438
[ 1966.356761] -------------------------------------------------------
[ 1966.356764] events/0/387 is trying to acquire lock:
[ 1966.356766]  (&kvm->lock){+.+.+.}, at: [<ffffffff8100af27>]
kvm_assigned_dev_interrupt_work_handler+0x42/0x13a
[ 1966.356786]
[ 1966.356787] but task is already holding lock:
[ 1966.356789]  (&match->interrupt_work){+.+...}, at:
[<ffffffff810986e9>] worker_thread+0x175/0x2f6
[ 1966.356797]
[ 1966.356798] which lock already depends on the new lock.
[ 1966.356799]
[ 1966.356800]
[ 1966.356801] the existing dependency chain (in reverse order) is:
[ 1966.356803]
[ 1966.356803] -> #1 (&match->interrupt_work){+.+...}:
[ 1966.356809]        [<ffffffff810b3bf6>] __lock_acquire+0x1396/0x1710
[ 1966.356817]        [<ffffffff810b403c>] lock_acquire+0xcc/0x104
[ 1966.356821]        [<ffffffff810994a8>] __cancel_work_timer+0x121/0x247
[ 1966.356825]        [<ffffffff8109962c>] cancel_work_sync+0x23/0x39
[ 1966.356828]        [<ffffffff8100b280>] kvm_deassign_irq+0xf1/0x183
[ 1966.356832]        [<ffffffff8100db6c>] kvm_vm_ioctl+0x8c8/0xc1a
[ 1966.356837]        [<ffffffff81156e56>] vfs_ioctl+0x3e/0xa3
[ 1966.356846]        [<ffffffff8115741c>] do_vfs_ioctl+0x4be/0x511
[ 1966.356850]        [<ffffffff811574c5>] sys_ioctl+0x56/0x8d
[ 1966.356854]        [<ffffffff81034fdb>] system_call_fastpath+0x16/0x1b
[ 1966.356860]        [<ffffffffffffffff>] 0xffffffffffffffff
(Continue reading)

Kenji Kaneshige | 1 Jul 06:34 2009

Re: pciehp resume handler - racy?

Alan Jenkins wrote:
> Hi,
> 
> I've been hacking on the PCI hotplug driver otherwise known as
> eeepc-laptop.  At one point I reliably triggered a race on resume.  In
> the hot-unplug case, it seemed that the resume handler would try to
> remove the PCI device at the same time as an acpi notification (run in a
> workqueue) tried to do the same thing.  The result was an OOPS.  My
> conclusion is that the PCI hotplug core does not protect against
> multiple simultaneous removals of the same device.

Though I don't know hotplug driver for eeepc-laptop at all, I guess
it doesn't use pci_hp_register(), which is for registering a hotplug
slot to pci hotplug core. The pci_hp_register() prevents a hot-plug
slot from being managed by multiple hotplug controller drivers at
the same time. Using pci_hp_register() would be one of the solution
for eeepc-laptop hotplug driver.

> 
> pciehp appears to have an analogous problem.  Assuming pciehp_force is
> set, the resume handler can hot-unplug the device.  The interrupt
> handler could try to hot-unplug the device at the same time.  Should the
> resume handler take the slot mutex to avoid this problem?
> 

Hot-plug operations for the same hotplug slot are serialized by
crit_sect mutex of struct controller in pciehp_enable_slot() and
pciehp_disable_slot(). So I don't think multiple hot-plug
operations for the same slot would be executed at the same time.

(Continue reading)

Kenji Kaneshige | 1 Jul 07:26 2009

Re: [PATCH resend 3/3] PCI: support Secondary Bus Reset

Hi,

I'm sorry for the very delayed comment. May I ask you about timing
parameters?

Yu Zhao wrote:
> PCI-to-PCI Bridge 1.2 specifies that the Secondary Bus Reset bit can
> force the assertion of RST# on the secondary interface, which can be
> used to reset all devices including subordinates under this bus. This
> can be used to reset a function if this function is the only device
> under this bus.
> 
> Reviewed-by: Kenji Kaneshige <kaneshige.kenji <at> jp.fujitsu.com>
> Signed-off-by: Yu Zhao <yu.zhao <at> intel.com>
> ---
>  drivers/pci/pci.c |   31 +++++++++++++++++++++++++++++++
>  1 files changed, 31 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 2e58acc..c56a4a0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
>  <at>  <at>  -2162,6 +2162,33  <at>  <at>  static int pci_pm_reset(struct pci_dev *dev, int probe)
>  	return 0;
>  }
>  
> +static int pci_parent_bus_reset(struct pci_dev *dev, int probe)
> +{
> +	u16 ctrl;
> +	struct pci_dev *pdev;
(Continue reading)

Yu Zhao | 1 Jul 08:30 2009
Picon

Re: [PATCH resend 3/3] PCI: support Secondary Bus Reset

Kenji Kaneshige wrote:
> Hi,
> 
> I'm sorry for the very delayed comment. May I ask you about timing
> parameters?

>> +	pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl);
>> +	ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
>> +	pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
>> +	msleep(100);
> 
> Why 100 msec here? Is this based on T_rst (1 msec)?

Yes, this is Trst.

>> +
>> +	ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
>> +	pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl);
>> +	msleep(100);
>> +
> 
> I think 100 msec here is not enough for PCI/PCI-X. I think T_rhfa
> is required here for PCI/PCI-X.

Yes, should be Trhfa here. PCI spec says "Device operational parameters 
at frequencies under 16 MHz may be guaranteed by design rather than by 
testing", so the Trhfa should be 2s (i.e. 2^25 clock / 16 MHz).

Will send a patch to update these values.

(Continue reading)

Avi Kivity | 1 Jul 09:49 2009
Picon

Re: pci_stub and kvm

On 07/01/2009 07:18 AM, Yinghai Lu wrote:
> [ 1966.343286]
> [ 1966.343288] =======================================================
> [ 1966.356756] [ INFO: possible circular locking dependency detected ]
> [ 1966.356759] 2.6.31-rc1-tip-00978-g99123e5-dirty #438
> [ 1966.356761] -------------------------------------------------------
> [ 1966.356764] events/0/387 is trying to acquire lock:
> [ 1966.356766]  (&kvm->lock){+.+.+.}, at: [<ffffffff8100af27>]
> kvm_assigned_dev_interrupt_work_handler+0x42/0x13a
> [ 1966.356786]
> [ 1966.356787] but task is already holding lock:
> [ 1966.356789]  (&match->interrupt_work){+.+...}, at:
> [<ffffffff810986e9>] worker_thread+0x175/0x2f6
> [ 1966.356797]
> [ 1966.356798] which lock already depends on the new lock.
> [ 1966.356799]
> [ 1966.356800]
> [ 1966.356801] the existing dependency chain (in reverse order) is:
> [ 1966.356803]
> [ 1966.356803] ->  #1 (&match->interrupt_work){+.+...}:
> [ 1966.356809]        [<ffffffff810b3bf6>] __lock_acquire+0x1396/0x1710
> [ 1966.356817]        [<ffffffff810b403c>] lock_acquire+0xcc/0x104
> [ 1966.356821]        [<ffffffff810994a8>] __cancel_work_timer+0x121/0x247
> [ 1966.356825]        [<ffffffff8109962c>] cancel_work_sync+0x23/0x39
> [ 1966.356828]        [<ffffffff8100b280>] kvm_deassign_irq+0xf1/0x183
> [ 1966.356832]        [<ffffffff8100db6c>] kvm_vm_ioctl+0x8c8/0xc1a
> [ 1966.356837]        [<ffffffff81156e56>] vfs_ioctl+0x3e/0xa3
> [ 1966.356846]        [<ffffffff8115741c>] do_vfs_ioctl+0x4be/0x511
> [ 1966.356850]        [<ffffffff811574c5>] sys_ioctl+0x56/0x8d
> [ 1966.356854]        [<ffffffff81034fdb>] system_call_fastpath+0x16/0x1b
(Continue reading)


Gmane