qla_wxyz pci_set_mwi question

qla_wxyz pci_set_mwi question

[Randy Dunlap]

Both scsi/qla2xxx/qla_init.c and scsi/qla4xxx/ql4_init.c use pci_set_mwi()
like this:


	mwi = 0;
	if (pci_set_mwi(ha->pdev))
		mwi = PCI_COMMAND_INVALIDATE;

and then go on to set the PCI_COMMAND "word" based on that value.

I don't quite get it.  pci_set_mwi() returns 0 for success or
negative for error... so if pci_set_mwi() fails, the qla init
code still tries to set the PCI_COMMAND_INVALIDATE bit in the
PCI_COMMAND word?  The logic looks inverted to me.  What am I
overlooking?

Thanks.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: qla_wxyz pci_set_mwi question

[Andrew Vasquez]

On Wed, 11 Apr 2007, Randy Dunlap wrote:

> Both scsi/qla2xxx/qla_init.c and scsi/qla4xxx/ql4_init.c use pci_set_mwi()
> like this:
> 
> 
> 	mwi = 0;
> 	if (pci_set_mwi(ha->pdev))
> 		mwi = PCI_COMMAND_INVALIDATE;
> 
> and then go on to set the PCI_COMMAND "word" based on that value.
> 
> I don't quite get it.  pci_set_mwi() returns 0 for success or
> negative for error... so if pci_set_mwi() fails, the qla init
> code still tries to set the PCI_COMMAND_INVALIDATE bit in the
> PCI_COMMAND word?  The logic looks inverted to me.  What am I
> overlooking?

No, the current logic doesn't make much sense at all, initialization
should not continue if MWI is not set... I'll queue-up something like
the patch below for our next batch-of-qla2xxx-updates.

Dave S., please be sure something similar is added for qla4xxx.

Thanks,
AV

---

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 98c01cd..a586e19 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -130,18 +130,17 @@ qla2x00_initialize_adapter(scsi_qla_host_t *ha)
 int
 qla2100_pci_config(scsi_qla_host_t *ha)
 {
-	uint16_t w, mwi;
+	uint16_t w;
 	uint32_t d;
 	unsigned long flags;
 	struct device_reg_2xxx __iomem *reg = &ha->iobase->isp;
 
 	pci_set_master(ha->pdev);
-	mwi = 0;
-	if (pci_set_mwi(ha->pdev))
-		mwi = PCI_COMMAND_INVALIDATE;
+	if (pci_set_mwi(ha->pdev) < 0)
+		return QLA_FUNCTION_FAILED;
 
 	pci_read_config_word(ha->pdev, PCI_COMMAND, &w);
-	w |= mwi | (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
+	w |= PCI_COMMAND_PARITY | PCI_COMMAND_SERR;
 	pci_write_config_word(ha->pdev, PCI_COMMAND, w);
 
 	/* Reset expansion ROM address decode enable */
@@ -166,19 +165,18 @@ qla2100_pci_config(scsi_qla_host_t *ha)
 int
 qla2300_pci_config(scsi_qla_host_t *ha)
 {
-	uint16_t	w, mwi;
+	uint16_t	w;
 	uint32_t	d;
 	unsigned long   flags = 0;
 	uint32_t	cnt;
 	struct device_reg_2xxx __iomem *reg = &ha->iobase->isp;
 
 	pci_set_master(ha->pdev);
-	mwi = 0;
-	if (pci_set_mwi(ha->pdev))
-		mwi = PCI_COMMAND_INVALIDATE;
+	if (pci_set_mwi(ha->pdev) < 0)
+		return QLA_FUNCTION_FAILED;
 
 	pci_read_config_word(ha->pdev, PCI_COMMAND, &w);
-	w |= mwi | (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
+	w |= PCI_COMMAND_PARITY | PCI_COMMAND_SERR;
 
 	if (IS_QLA2322(ha) || IS_QLA6322(ha))
 		w &= ~PCI_COMMAND_INTX_DISABLE;
@@ -253,19 +251,18 @@ qla2300_pci_config(scsi_qla_host_t *ha)
 int
 qla24xx_pci_config(scsi_qla_host_t *ha)
 {
-	uint16_t w, mwi;
+	uint16_t w;
 	uint32_t d;
 	unsigned long flags = 0;
 	struct device_reg_24xx __iomem *reg = &ha->iobase->isp24;
 	int pcix_cmd_reg, pcie_dctl_reg;
 
 	pci_set_master(ha->pdev);
-	mwi = 0;
-	if (pci_set_mwi(ha->pdev))
-		mwi = PCI_COMMAND_INVALIDATE;
+	if (pci_set_mwi(ha->pdev) < 0)
+		return QLA_FUNCTION_FAILED;
 
 	pci_read_config_word(ha->pdev, PCI_COMMAND, &w);
-	w |= mwi | (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
+	w |= PCI_COMMAND_PARITY | PCI_COMMAND_SERR;
 	w &= ~PCI_COMMAND_INTX_DISABLE;
 	pci_write_config_word(ha->pdev, PCI_COMMAND, w);
 

Re: qla_wxyz pci_set_mwi question

[Matthew Wilcox]

On Thu, Apr 12, 2007 at 10:20:38AM -0700, Andrew Vasquez wrote:
> No, the current logic doesn't make much sense at all, initialization
> should not continue if MWI is not set... I'll queue-up something like
> the patch below for our next batch-of-qla2xxx-updates.

Why should it fail?  If there's a platform which can't support a
cacheline size that the qla2xyz card can handle, it should be able to
happily fall back to doing plain writes instead of MWIs.  IMO, it should
just call pci_set_mwi() and ignore the result.

>  	pci_set_master(ha->pdev);
> -	mwi = 0;
> -	if (pci_set_mwi(ha->pdev))
> -		mwi = PCI_COMMAND_INVALIDATE;
> +	if (pci_set_mwi(ha->pdev) < 0)
> +		return QLA_FUNCTION_FAILED;
>  
>  	pci_read_config_word(ha->pdev, PCI_COMMAND, &w);
> -	w |= mwi | (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
> +	w |= PCI_COMMAND_PARITY | PCI_COMMAND_SERR;
>  	pci_write_config_word(ha->pdev, PCI_COMMAND, w);

However, setting the msi bit here was redundant.  pci_set_mwi() already
sets the PCI_COMMAND_INVALIDATE bit.

Re: qla_wxyz pci_set_mwi question

[Andrew Vasquez]

On Thu, 12 Apr 2007, Matthew Wilcox wrote:

> On Thu, Apr 12, 2007 at 10:20:38AM -0700, Andrew Vasquez wrote:
> > No, the current logic doesn't make much sense at all, initialization
> > should not continue if MWI is not set... I'll queue-up something like
> > the patch below for our next batch-of-qla2xxx-updates.
> 
> Why should it fail?  If there's a platform which can't support a
> cacheline size that the qla2xyz card can handle, it should be able to
> happily fall back to doing plain writes instead of MWIs.  IMO, it should
> just call pci_set_mwi() and ignore the result.

I believe there were some erratas on some ISP2xxx chips where MWI
needed to be set for proper operation.  I'll go back, verify and
update the patch accordingly.

> >  	pci_set_master(ha->pdev);
> > -	mwi = 0;
> > -	if (pci_set_mwi(ha->pdev))
> > -		mwi = PCI_COMMAND_INVALIDATE;
> > +	if (pci_set_mwi(ha->pdev) < 0)
> > +		return QLA_FUNCTION_FAILED;
> >  
> >  	pci_read_config_word(ha->pdev, PCI_COMMAND, &w);
> > -	w |= mwi | (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
> > +	w |= PCI_COMMAND_PARITY | PCI_COMMAND_SERR;
> >  	pci_write_config_word(ha->pdev, PCI_COMMAND, w);
> 
> However, setting the msi bit here was redundant.  pci_set_mwi() already
> sets the PCI_COMMAND_INVALIDATE bit.

Re: qla_wxyz pci_set_mwi question

[Matthew Wilcox]

On Thu, Apr 12, 2007 at 12:37:13PM -0700, Andrew Vasquez wrote:
> On Thu, 12 Apr 2007, Matthew Wilcox wrote:
> > Why should it fail?  If there's a platform which can't support a
> > cacheline size that the qla2xyz card can handle, it should be able to
> > happily fall back to doing plain writes instead of MWIs.  IMO, it should
> > just call pci_set_mwi() and ignore the result.
> 
> I believe there were some erratas on some ISP2xxx chips where MWI
> needed to be set for proper operation.  I'll go back, verify and
> update the patch accordingly.

Hmm.  The thing is that pci_set_mwi() returns success on machines where
MWI is disabled (currently only PPC64).  Perhaps it needs to fail
instead.

Re: qla_wxyz pci_set_mwi question

[Benjamin Herrenschmidt]

On Thu, 2007-04-12 at 14:04 -0600, Matthew Wilcox wrote:
> On Thu, Apr 12, 2007 at 12:37:13PM -0700, Andrew Vasquez wrote:
> > On Thu, 12 Apr 2007, Matthew Wilcox wrote:
> > > Why should it fail?  If there's a platform which can't support a
> > > cacheline size that the qla2xyz card can handle, it should be able to
> > > happily fall back to doing plain writes instead of MWIs.  IMO, it should
> > > just call pci_set_mwi() and ignore the result.
> > 
> > I believe there were some erratas on some ISP2xxx chips where MWI
> > needed to be set for proper operation.  I'll go back, verify and
> > update the patch accordingly.
> 
> Hmm.  The thing is that pci_set_mwi() returns success on machines where
> MWI is disabled (currently only PPC64).  Perhaps it needs to fail
> instead.

MWI isn't diabled on ppc64... or did I miss something ?

Ben.

Re: qla_wxyz pci_set_mwi question

[Randy Dunlap]

Benjamin Herrenschmidt wrote:
> On Thu, 2007-04-12 at 14:04 -0600, Matthew Wilcox wrote:
>> On Thu, Apr 12, 2007 at 12:37:13PM -0700, Andrew Vasquez wrote:
>>> On Thu, 12 Apr 2007, Matthew Wilcox wrote:
>>>> Why should it fail?  If there's a platform which can't support a
>>>> cacheline size that the qla2xyz card can handle, it should be able to
>>>> happily fall back to doing plain writes instead of MWIs.  IMO, it should
>>>> just call pci_set_mwi() and ignore the result.
>>> I believe there were some erratas on some ISP2xxx chips where MWI
>>> needed to be set for proper operation.  I'll go back, verify and
>>> update the patch accordingly.
>> Hmm.  The thing is that pci_set_mwi() returns success on machines where
>> MWI is disabled (currently only PPC64).  Perhaps it needs to fail
>> instead.
> 
> MWI isn't diabled on ppc64... or did I miss something ?
> 
> Ben.
> 

Willy was referring to this from include/asm-powerpc/pci.h:

#ifdef CONFIG_PPC64

/*
 * We want to avoid touching the cacheline size or MWI bit.
 * pSeries firmware sets the cacheline size (which is not the cpu cacheline
 * size in all cases) and hardware treats MWI the same as memory write.
 */
#define PCI_DISABLE_MWI


which makes pci_set_mwi() do nothing other than return 0;


-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: qla_wxyz pci_set_mwi question

[Benjamin Herrenschmidt]

> Willy was referring to this from include/asm-powerpc/pci.h:
> 
> #ifdef CONFIG_PPC64
> 
> /*
>  * We want to avoid touching the cacheline size or MWI bit.
>  * pSeries firmware sets the cacheline size (which is not the cpu cacheline
>  * size in all cases) and hardware treats MWI the same as memory write.
>  */
> #define PCI_DISABLE_MWI
> 
> 
> which makes pci_set_mwi() do nothing other than return 0;

Interesting... I think I missed that we had that bit for some time :-)

Well, I suppose that on pSeries and probably pmac too, the firmware will
set the MWI bit for us anyway, but that's a bit dodgy to apply that to
all ppc64... they aren't all pSeries.

I'll have to look into that again one of these days.

Cheers,
Ben.

Re: qla_wxyz pci_set_mwi question

[Andrew Vasquez]

On Thu, 12 Apr 2007, Matthew Wilcox wrote:

> On Thu, Apr 12, 2007 at 12:37:13PM -0700, Andrew Vasquez wrote:
> > On Thu, 12 Apr 2007, Matthew Wilcox wrote:
> > > Why should it fail?  If there's a platform which can't support a
> > > cacheline size that the qla2xyz card can handle, it should be able to
> > > happily fall back to doing plain writes instead of MWIs.  IMO, it should
> > > just call pci_set_mwi() and ignore the result.
> > 
> > I believe there were some erratas on some ISP2xxx chips where MWI
> > needed to be set for proper operation.  I'll go back, verify and
> > update the patch accordingly.
> 
> Hmm.  The thing is that pci_set_mwi() returns success on machines where
> MWI is disabled (currently only PPC64).  Perhaps it needs to fail
> instead.

Sorry for the latency on getting this one resolved...  So is Randy's
proposal for pci_try_set_mwi():

	http://article.gmane.org/gmane.linux.kernel/516349

going to be added in 2.6.22?  If so, I'd like to propose the following
to qla2xxx which handles the 2300 errata.

--

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 6ad1588..8b83e1c 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -130,18 +130,16 @@ qla2x00_initialize_adapter(scsi_qla_host_t *ha)
 int
 qla2100_pci_config(scsi_qla_host_t *ha)
 {
-	uint16_t w, mwi;
+	uint16_t w;
 	uint32_t d;
 	unsigned long flags;
 	struct device_reg_2xxx __iomem *reg = &ha->iobase->isp;
 
 	pci_set_master(ha->pdev);
-	mwi = 0;
-	if (pci_set_mwi(ha->pdev))
-		mwi = PCI_COMMAND_INVALIDATE;
+	pci_try_set_mwi(ha->pdev);
 
 	pci_read_config_word(ha->pdev, PCI_COMMAND, &w);
-	w |= mwi | (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
+	w |= (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
 	pci_write_config_word(ha->pdev, PCI_COMMAND, w);
 
 	/* Reset expansion ROM address decode enable */
@@ -166,22 +164,21 @@ qla2100_pci_config(scsi_qla_host_t *ha)
 int
 qla2300_pci_config(scsi_qla_host_t *ha)
 {
-	uint16_t	w, mwi;
+	uint16_t	w;
 	uint32_t	d;
 	unsigned long   flags = 0;
 	uint32_t	cnt;
 	struct device_reg_2xxx __iomem *reg = &ha->iobase->isp;
 
 	pci_set_master(ha->pdev);
-	mwi = 0;
-	if (pci_set_mwi(ha->pdev))
-		mwi = PCI_COMMAND_INVALIDATE;
+	pci_try_set_mwi(ha->pdev);
 
 	pci_read_config_word(ha->pdev, PCI_COMMAND, &w);
-	w |= mwi | (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
+	w |= (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
 
 	if (IS_QLA2322(ha) || IS_QLA6322(ha))
 		w &= ~PCI_COMMAND_INTX_DISABLE;
+	pci_write_config_word(ha->pdev, PCI_COMMAND, w);
 
 	/*
 	 * If this is a 2300 card and not 2312, reset the
@@ -210,7 +207,7 @@ qla2300_pci_config(scsi_qla_host_t *ha)
 		ha->fb_rev = RD_FB_CMD_REG(ha, reg);
 
 		if (ha->fb_rev == FPM_2300)
-			w &= ~PCI_COMMAND_INVALIDATE;
+			pci_clear_mwi(ha->pdev);
 
 		/* Deselect FPM registers. */
 		WRT_REG_WORD(&reg->ctrl_status, 0x0);
@@ -227,7 +224,6 @@ qla2300_pci_config(scsi_qla_host_t *ha)
 
 		spin_unlock_irqrestore(&ha->hardware_lock, flags);
 	}
-	pci_write_config_word(ha->pdev, PCI_COMMAND, w);
 
 	pci_write_config_byte(ha->pdev, PCI_LATENCY_TIMER, 0x80);
 
@@ -253,19 +249,17 @@ qla2300_pci_config(scsi_qla_host_t *ha)
 int
 qla24xx_pci_config(scsi_qla_host_t *ha)
 {
-	uint16_t w, mwi;
+	uint16_t w;
 	uint32_t d;
 	unsigned long flags = 0;
 	struct device_reg_24xx __iomem *reg = &ha->iobase->isp24;
 	int pcix_cmd_reg, pcie_dctl_reg;
 
 	pci_set_master(ha->pdev);
-	mwi = 0;
-	if (pci_set_mwi(ha->pdev))
-		mwi = PCI_COMMAND_INVALIDATE;
+	pci_try_set_mwi(ha->pdev);
 
 	pci_read_config_word(ha->pdev, PCI_COMMAND, &w);
-	w |= mwi | (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
+	w |= (PCI_COMMAND_PARITY | PCI_COMMAND_SERR);
 	w &= ~PCI_COMMAND_INTX_DISABLE;
 	pci_write_config_word(ha->pdev, PCI_COMMAND, w);