linux-kernel - Re: [PATCH v4]: PCI Error Recovery: Symbios SCSI device driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20061102035722.GA31830@parisc-linux.org>
Date:	Wed, 1 Nov 2006 20:57:22 -0700
From:	Matthew Wilcox <matthew@....cx>
To:	Linas Vepstas <linas@...tin.ibm.com>
Cc:	linux-scsi@...r.kernel.org, linux-pci@...ey.karlin.mff.cuni.cz,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4]: PCI Error Recovery: Symbios SCSI device driver

On Wed, Nov 01, 2006 at 07:19:37PM -0600, Linas Vepstas wrote:
> @@ -657,6 +657,10 @@ static irqreturn_t sym53c8xx_intr(int ir
>  	unsigned long flags;
>  	struct sym_hcb *np = (struct sym_hcb *)dev_id;
>  
> +	/* Avoid spinloop trying to handle interrupts on frozen device */
> +	if (pci_channel_offline(np->s.device))
> +		return IRQ_HANDLED;
> +
>  	if (DEBUG_FLAGS & DEBUG_TINY) printf_debug ("[");
>  
>  	spin_lock_irqsave(np->s.host->host_lock, flags);

Just wondering ... should we really be returning HANDLED?  What if the
IRQ is shared?  Will the hardware de-assert the level interrupt when it
puts the device in reset (ie is this a transitory glitch?), or do we
have to cope with a screaming interrupt?

> +#define WAIT_FOR_PCI_RECOVERY	35
> +	if (pci_channel_offline(np->s.device))
> +	{

I prefer if () {

> +static pci_ers_result_t sym2_io_slot_reset (struct pci_dev *pdev)
> +{
> +	struct sym_hcb *np = pci_get_drvdata(pdev);
> +
> +	printk(KERN_INFO "%s: recovering from a PCI slot reset\n",
> +	          sym_name(np));
> +
> +	if (pci_enable_device(pdev)) {
> +		printk(KERN_ERR "%s: Unable to enable afer PCI reset\n",
> +		        sym_name(np));
> +		return PCI_ERS_RESULT_DISCONNECT;
> +	}
> +
> +	pci_set_master(pdev);
> +	enable_irq(pdev->irq);

Hm.  If we need to call pci_set_master, then we're also going to need to
call pci_set_mwi (if appropriate) which is currently done in
sym_set_workarounds().  Except you don't have a sym_device, or a
sym_chip around at this point.  Bother.  Need to do some refactoring to
take care of that.

> +		/* Prevent deadlock waiting on a condition that may never clear. */
> +		if (unlikely(sist == 0xffff && dstat == 0xff)) {
> +			if (unlikely(pci_channel_offline(np->s.device)))
> +				return;
> +		}

I like the first unlikely ... but I'd drop the second one.  If they are
both ffff ff, I'd say it's quite likely ;-)  Anyway, the first unlikely
is good enough a hint to GCC, IMO.

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