[PATCH] fix idiocy in asd_init_lseq_mdp()

[PATCH] fix idiocy in asd_init_lseq_mdp()

[Al Viro]

To whoever had written that code:

a) priority of >> is higher than that of &
b) priority of typecast is higher than that of any binary operator
c) learn the fscking C

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/scsi/aic94xx/aic94xx_seq.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_seq.c b/drivers/scsi/aic94xx/aic94xx_seq.c
index d9b6da5..56e4b3b 100644
--- a/drivers/scsi/aic94xx/aic94xx_seq.c
+++ b/drivers/scsi/aic94xx/aic94xx_seq.c
@@ -764,7 +764,7 @@ static void asd_init_lseq_mdp(struct asd
 	asd_write_reg_word(asd_ha, LmSEQ_FIRST_INV_SCB_SITE(lseq),
 			   (u16)last_scb_site_no+1);
 	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq),
-			    (u16) LmM0INTEN_MASK & 0xFFFF0000 >> 16);
+			    (u16) ((LmM0INTEN_MASK & 0xFFFF0000) >> 16));
 	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq) + 2,
 			    (u16) LmM0INTEN_MASK & 0xFFFF);
 	asd_write_reg_byte(asd_ha, LmSEQ_LINK_RST_FRM_LEN(lseq), 0);
-- 
1.4.2.GIT

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Luben Tuikov]

--- Al Viro <viro@ftp.linux.org.uk> wrote:
> To whoever had written that code:
> 
> a) priority of >> is higher than that of &
> b) priority of typecast is higher than that of any binary operator
> c) learn the fscking C

"whoever" would be Bottomley or someone at LTC.

My code in that spot has:
asd_write_reg_dword(asd_ha, LmSEQ_INTEN_SAVE(lseq),
			    (u32) LmM0INTEN_MASK);

> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/scsi/aic94xx/aic94xx_seq.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/aic94xx/aic94xx_seq.c b/drivers/scsi/aic94xx/aic94xx_seq.c
> index d9b6da5..56e4b3b 100644
> --- a/drivers/scsi/aic94xx/aic94xx_seq.c
> +++ b/drivers/scsi/aic94xx/aic94xx_seq.c
> @@ -764,7 +764,7 @@ static void asd_init_lseq_mdp(struct asd
>  	asd_write_reg_word(asd_ha, LmSEQ_FIRST_INV_SCB_SITE(lseq),
>  			   (u16)last_scb_site_no+1);
>  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq),
> -			    (u16) LmM0INTEN_MASK & 0xFFFF0000 >> 16);
> +			    (u16) ((LmM0INTEN_MASK & 0xFFFF0000) >> 16));
>  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq) + 2,
>  			    (u16) LmM0INTEN_MASK & 0xFFFF);
>  	asd_write_reg_byte(asd_ha, LmSEQ_LINK_RST_FRM_LEN(lseq), 0);
> -- 
> 1.4.2.GIT
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Luben Tuikov]

--- Al Viro <viro@ftp.linux.org.uk> wrote:
> To whoever had written that code:
> 
> a) priority of >> is higher than that of &
> b) priority of typecast is higher than that of any binary operator
> c) learn the fscking C

"whoever" would be Bottomley or someone at LTC.

My code in that spot has:
     asd_write_reg_dword(asd_ha, LmSEQ_INTEN_SAVE(lseq),
	        		    (u32) LmM0INTEN_MASK);

    Luben

> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/scsi/aic94xx/aic94xx_seq.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/aic94xx/aic94xx_seq.c b/drivers/scsi/aic94xx/aic94xx_seq.c
> index d9b6da5..56e4b3b 100644
> --- a/drivers/scsi/aic94xx/aic94xx_seq.c
> +++ b/drivers/scsi/aic94xx/aic94xx_seq.c
> @@ -764,7 +764,7 @@ static void asd_init_lseq_mdp(struct asd
>  	asd_write_reg_word(asd_ha, LmSEQ_FIRST_INV_SCB_SITE(lseq),
>  			   (u16)last_scb_site_no+1);
>  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq),
> -			    (u16) LmM0INTEN_MASK & 0xFFFF0000 >> 16);
> +			    (u16) ((LmM0INTEN_MASK & 0xFFFF0000) >> 16));
>  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq) + 2,
>  			    (u16) LmM0INTEN_MASK & 0xFFFF);
>  	asd_write_reg_byte(asd_ha, LmSEQ_LINK_RST_FRM_LEN(lseq), 0);
> -- 
> 1.4.2.GIT
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Douglas Gilbert]

Al Viro wrote:
> To whoever had written that code:
> 
> a) priority of >> is higher than that of &
> b) priority of typecast is higher than that of any binary operator
> c) learn the fscking C

Al,
On the assumption that you have hardware that uses this
driver, did you notice any improvement with your patch
applied?

Several of us have reported a degenerate mode, that
I term as "tmf timeout", in which a aic94xx based card
becomes inoperable. Alas, the same hardware running another
OS does not exhibit that problem (or at least not as much).

> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  drivers/scsi/aic94xx/aic94xx_seq.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/aic94xx/aic94xx_seq.c b/drivers/scsi/aic94xx/aic94xx_seq.c
> index d9b6da5..56e4b3b 100644
> --- a/drivers/scsi/aic94xx/aic94xx_seq.c
> +++ b/drivers/scsi/aic94xx/aic94xx_seq.c
> @@ -764,7 +764,7 @@ static void asd_init_lseq_mdp(struct asd
>  	asd_write_reg_word(asd_ha, LmSEQ_FIRST_INV_SCB_SITE(lseq),
>  			   (u16)last_scb_site_no+1);
>  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq),
> -			    (u16) LmM0INTEN_MASK & 0xFFFF0000 >> 16);
> +			    (u16) ((LmM0INTEN_MASK & 0xFFFF0000) >> 16));
>  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq) + 2,
>  			    (u16) LmM0INTEN_MASK & 0xFFFF);
>  	asd_write_reg_byte(asd_ha, LmSEQ_LINK_RST_FRM_LEN(lseq), 0);

BTW Luben was pointing out that the call you patched
and the following call can be combined into a less
trouble prone asd_write_reg_dword() call.

Doug Gilbert

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Al Viro]

> Several of us have reported a degenerate mode, that
> I term as "tmf timeout", in which a aic94xx based card
> becomes inoperable. Alas, the same hardware running another
> OS does not exhibit that problem (or at least not as much).

> >  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq),
> > -			    (u16) LmM0INTEN_MASK & 0xFFFF0000 >> 16);
> > +			    (u16) ((LmM0INTEN_MASK & 0xFFFF0000) >> 16));
> >  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq) + 2,
> >  			    (u16) LmM0INTEN_MASK & 0xFFFF);
> >  	asd_write_reg_byte(asd_ha, LmSEQ_LINK_RST_FRM_LEN(lseq), 0);
 
> BTW Luben was pointing out that the call you patched
> and the following call can be combined into a less
> trouble prone asd_write_reg_dword() call.

In that case there's another bug - we should write upper 16 bits to
addr + 2, not the lower ones.

IOW, the old code was
	broken attempt to write upper 16 bits to addr (ends up writing _lower_
16 bits)
	writing lower 16 bits to addr + 2

With this patch we get the first call do what it clearly intended to do
(unless it's a deliberate obfuscation from hell).  _IF_ we really want
to write the damn thing little-endian, the order should be reverted on
top of that.  I.e.
  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq),
  			    (u16) LmM0INTEN_MASK & 0xFFFF);
  	asd_write_reg_word(asd_ha, LmSEQ_INTEN_SAVE(lseq) + 2,
			    (u16) ((LmM0INTEN_MASK & 0xFFFF0000) >> 16));
or, indeed, asd_write_reg_dword().

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Luben Tuikov]

--- Douglas Gilbert <dougg@torque.net> wrote:
> Al Viro wrote:
> > To whoever had written that code:
> > 
> > a) priority of >> is higher than that of &
> > b) priority of typecast is higher than that of any binary operator
> > c) learn the fscking C
[...]
> BTW Luben was pointing out that the call you patched
> and the following call can be combined into a less
> trouble prone asd_write_reg_dword() call.

More than that -- I looked at the history of that
file/line and the code as I had written it _never_ had
that broken cast and shift mess.

"Someone" changed that after I submitted the code to lkml/lsml.

    Luben

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Al Viro]

On Mon, Sep 25, 2006 at 10:16:34AM -0700, Luben Tuikov wrote:
> --- Douglas Gilbert <dougg@torque.net> wrote:
> > Al Viro wrote:
> > > To whoever had written that code:
> > > 
> > > a) priority of >> is higher than that of &
> > > b) priority of typecast is higher than that of any binary operator
> > > c) learn the fscking C
> [...]
> > BTW Luben was pointing out that the call you patched
> > and the following call can be combined into a less
> > trouble prone asd_write_reg_dword() call.
> 
> More than that -- I looked at the history of that
> file/line and the code as I had written it _never_ had
> that broken cast and shift mess.

Far more interesting question: where does the hardware expect to see the
upper 16 bits of that 32bit value?  Which one it is - LmSEQ_INTEN_SAVE(lseq)
ori LmSEQ_INTEN_SAVE(lseq) + 2?

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[James Bottomley]

On Mon, 2006-09-25 at 18:39 +0100, Al Viro wrote:
> Far more interesting question: where does the hardware expect to see
> the
> upper 16 bits of that 32bit value?  Which one it is -
> LmSEQ_INTEN_SAVE(lseq)
> ori LmSEQ_INTEN_SAVE(lseq) + 2?

I don't honestly know.  The change was made as part of a slew of changes
by Robert Tarte at Adaptec to make the driver run on Big Endian
platforms.  I've copied Jack Hammer who's now looking after it in the
hope that he can enlighten us.

James

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Mike Anderson]

James Bottomley <James.Bottomley@SteelEye.com> wrote:
> On Mon, 2006-09-25 at 18:39 +0100, Al Viro wrote:
> > Far more interesting question: where does the hardware expect to see
> > the
> > upper 16 bits of that 32bit value?  Which one it is -
> > LmSEQ_INTEN_SAVE(lseq)
> > ori LmSEQ_INTEN_SAVE(lseq) + 2?
> 
> I don't honestly know.  The change was made as part of a slew of changes
> by Robert Tarte at Adaptec to make the driver run on Big Endian
> platforms.  I've copied Jack Hammer who's now looking after it in the
> hope that he can enlighten us.

This was not Rob. I sent this bad code out in a roll up of support for
non-x86 systems (and bad process for not running sparse on the
patch which passed the buck onto someone else to find).

I think it might have been for an IA64 offset issue someone was seeing. I
cannot find the original mail on the issue in my mail archives.

I will try and track down a IA64 system to see if we can verify this is
really needed. If not we should revert back to the original dword
implementation. 

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Luben Tuikov]

--- James Bottomley <James.Bottomley@SteelEye.com> wrote:
> On Mon, 2006-09-25 at 18:39 +0100, Al Viro wrote:
> > Far more interesting question: where does the hardware expect to see
> > the
> > upper 16 bits of that 32bit value?  Which one it is -
> > LmSEQ_INTEN_SAVE(lseq)
> > ori LmSEQ_INTEN_SAVE(lseq) + 2?
> 
> I don't honestly know.  The change was made as part of a slew of changes
> by Robert Tarte at Adaptec to make the driver run on Big Endian
> platforms.  I've copied Jack Hammer who's now looking after it in the
> hope that he can enlighten us.

Al,

I can see that you addressed your message to me, but Bottomley has
stepped in to answer.  I can also see that Bottomley is looking
for an answer from Jack.  To save an off the list correspondence,

I'll go ahead and answer your question addressed to me.

LSEQ_INTEN_SAVE is a 32 bit Little-Endian storage, thus
your original, first email on this subject is correct, and your
supposition that if the storage is 32 bit LE, then my version
is correct, is in itself correct.

No such "changes" (in the HW page writing area) are necessary in order
to make the code run in BE platforms.  My version of my code
(NOT Bottomley's version of my code) has been extensively tested on
BE (PowerPC) platforms, and is working properly.

The version as seen in my code:
	asd_write_reg_dword(asd_ha, LmSEQ_INTEN_SAVE(lseq),
			    (u32) LmM0INTEN_MASK);
is the correct way.

Good luck!
    Luben
P.S. Git tells me that I needed to change two lines only,
in order to make the code run on BE platforms, the git commit
dates Wednesday Sept 28, 2005.  That commit is present on
the GIT repo, then present on http://linux.adaptec.com/sas/.

RE: [PATCH] fix idiocy in asd_init_lseq_mdp()

[Hammer, Jack]

James,

asd_write_reg_dword() is the correct implementation for writing the
LmM0INTEN_MASK to the LmSEQ_INTEN_SAVE register.

Jack 



-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
Sent: Monday, September 25, 2006 1:43 PM
To: Hammer, Jack; Al Viro
Cc: Luben Tuikov; dougg@torque.net; linux-scsi@vger.kernel.org;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fix idiocy in asd_init_lseq_mdp()

On Mon, 2006-09-25 at 18:39 +0100, Al Viro wrote:
> Far more interesting question: where does the hardware expect to see 
> the upper 16 bits of that 32bit value?  Which one it is -
> LmSEQ_INTEN_SAVE(lseq)
> ori LmSEQ_INTEN_SAVE(lseq) + 2?

I don't honestly know.  The change was made as part of a slew of changes
by Robert Tarte at Adaptec to make the driver run on Big Endian
platforms.  I've copied Jack Hammer who's now looking after it in the
hope that he can enlighten us.

James