ips.c warnings

ips.c warnings

[Andrew Morton]

Never seen this before.

drivers/scsi/ips.c: In function `ips_scmd_buf_write':
drivers/scsi/ips.c:3665: warning: null argument where non-null required (arg 1)
drivers/scsi/ips.c: In function `ips_scmd_buf_read':
drivers/scsi/ips.c:3697: warning: null argument where non-null required (arg 2)
drivers/scsi/ips.c: In function `ips_register_scsi':

Due to

                        memcpy(IPS_SG_ADDRESS(&sg[i]), &cdata[xfer_cnt],
                               min_cnt);

I guess the compiler is saying that if IPS_SG_ADDRESS indeed evaluates to
NULL (as it is designed to do), we have an oops.

That's an x86_64 allmodconfig build, btw.

Re: ips.c warnings

[James Bottomley]

On Sun, 2005-10-16 at 14:08 -0700, Andrew Morton wrote:
> drivers/scsi/ips.c: In function `ips_scmd_buf_write':
> drivers/scsi/ips.c:3665: warning: null argument where non-null required (arg 1)
> drivers/scsi/ips.c: In function `ips_scmd_buf_read':
> drivers/scsi/ips.c:3697: warning: null argument where non-null required (arg 2)
> drivers/scsi/ips.c: In function `ips_register_scsi':
> 
> Due to
> 
>                         memcpy(IPS_SG_ADDRESS(&sg[i]), &cdata[xfer_cnt],
>                                min_cnt);
> 
> I guess the compiler is saying that if IPS_SG_ADDRESS indeed evaluates to
> NULL (as it is designed to do), we have an oops.

That's weird ... the compiler can't possibly be in a position to make
that judgement call.  We have lots of places where we return null if
something goes wrong and the kernel oopses.  The compiler certainly
isn't warning about all of them.

James

Re: ips.c warnings

[Andrew Morton]

James Bottomley <James.Bottomley@SteelEye.com> wrote:
>
> On Sun, 2005-10-16 at 14:08 -0700, Andrew Morton wrote:
>  > drivers/scsi/ips.c: In function `ips_scmd_buf_write':
>  > drivers/scsi/ips.c:3665: warning: null argument where non-null required (arg 1)
>  > drivers/scsi/ips.c: In function `ips_scmd_buf_read':
>  > drivers/scsi/ips.c:3697: warning: null argument where non-null required (arg 2)
>  > drivers/scsi/ips.c: In function `ips_register_scsi':
>  > 
>  > Due to
>  > 
>  >                         memcpy(IPS_SG_ADDRESS(&sg[i]), &cdata[xfer_cnt],
>  >                                min_cnt);
>  > 
>  > I guess the compiler is saying that if IPS_SG_ADDRESS indeed evaluates to
>  > NULL (as it is designed to do), we have an oops.
> 
>  That's weird ... the compiler can't possibly be in a position to make
>  that judgement call.  We have lots of places where we return null if
>  something goes wrong and the kernel oopses.  The compiler certainly
>  isn't warning about all of them.

Well I guess the difference is that the compiler can _see_ that
IPS_SG_ADDRESS() might return NULL:

#define IPS_SG_ADDRESS(sg)      (page_address((sg)->page) ? \
                                  page_address((sg)->page)+(sg)->offset : NULL)

What's the point in this expression anyway?  Why not just assume that
page_address() returns non-NULL?

Re: ips.c warnings

[James Bottomley]

On Sun, 2005-10-16 at 15:57 -0700, Andrew Morton wrote:
> James Bottomley <James.Bottomley@SteelEye.com> wrote:
> >  That's weird ... the compiler can't possibly be in a position to make
> >  that judgement call.  We have lots of places where we return null if
> >  something goes wrong and the kernel oopses.  The compiler certainly
> >  isn't warning about all of them.
> 
> Well I guess the difference is that the compiler can _see_ that
> IPS_SG_ADDRESS() might return NULL:
> 
> #define IPS_SG_ADDRESS(sg)      (page_address((sg)->page) ? \
>                                   page_address((sg)->page)+(sg)->offset : NULL)
> 
> What's the point in this expression anyway?  Why not just assume that
> page_address() returns non-NULL?

Actually, what it should be doing is a kmap/kunmap on page instead of
assuming the page has an address ...  hopefully the IPS people can fix
it.

James

Re: ips.c warnings

[Andrew Morton]

James Bottomley <James.Bottomley@SteelEye.com> wrote:
>
> On Sun, 2005-10-16 at 15:57 -0700, Andrew Morton wrote:
> > James Bottomley <James.Bottomley@SteelEye.com> wrote:
> > >  That's weird ... the compiler can't possibly be in a position to make
> > >  that judgement call.  We have lots of places where we return null if
> > >  something goes wrong and the kernel oopses.  The compiler certainly
> > >  isn't warning about all of them.
> > 
> > Well I guess the difference is that the compiler can _see_ that
> > IPS_SG_ADDRESS() might return NULL:
> > 
> > #define IPS_SG_ADDRESS(sg)      (page_address((sg)->page) ? \
> >                                   page_address((sg)->page)+(sg)->offset : NULL)
> > 
> > What's the point in this expression anyway?  Why not just assume that
> > page_address() returns non-NULL?
> 
> Actually, what it should be doing is a kmap/kunmap on page instead of
> assuming the page has an address ...  hopefully the IPS people can fix
> it.
> 

(cc's the IPS people)

ips.c warnings

[Jack Hammer]

This patch ( applied to 2.6.14-rc4-mm1 ) keeps all the ips code intact for both 2.4 and 2.6 kernels, and should eliminate the compiler warnings ( in 2.6 ) reported by Andrew.

I did not use kmap() because these routines can be called from the interrupt handler context and kmap() can sleep.


--- 2.6.14-rc4/drivers/scsi/ips.c	Thu Oct 20 11:26:50 2005
+++ devel/drivers/scsi/ips.c	Thu Oct 20 11:33:39 2005
@@ -220,13 +220,15 @@
 #include <linux/blk.h>
 #include "sd.h"
 #define IPS_SG_ADDRESS(sg)       ((sg)->address)
+#define IPS_CHECK_SG_ADDRESS(sg) ((sg)->address)
 #define IPS_LOCK_SAVE(lock,flags) spin_lock_irqsave(&io_request_lock,flags)
 #define IPS_UNLOCK_RESTORE(lock,flags) spin_unlock_irqrestore(&io_request_lock,flags)
 #ifndef __devexit_p
 #define __devexit_p(x) x
 #endif
 #else
-#define IPS_SG_ADDRESS(sg)      (page_address((sg)->page) ? \
+#define IPS_SG_ADDRESS(sg)       page_address((sg)->page)+(sg)->offset
+#define IPS_CHECK_SG_ADDRESS(sg) (page_address((sg)->page) ? \
                                    page_address((sg)->page)+(sg)->offset : NULL)
 #define IPS_LOCK_SAVE(lock,flags) do{spin_lock(lock);(void)flags;}while(0)
 #define IPS_UNLOCK_RESTORE(lock,flags) do{spin_unlock(lock);(void)flags;}while(0)
@@ -3659,7 +3661,7 @@
 		struct scatterlist *sg = scmd->request_buffer;
 		for (i = 0, xfer_cnt = 0;
 		     (i < scmd->use_sg) && (xfer_cnt < count); i++) {
-			if (!IPS_SG_ADDRESS(&sg[i]))
+			if (!IPS_CHECK_SG_ADDRESS(&sg[i]))
 				return;
 			min_cnt = min(count - xfer_cnt, sg[i].length);
 			memcpy(IPS_SG_ADDRESS(&sg[i]), &cdata[xfer_cnt],
@@ -3691,7 +3693,7 @@
 		struct scatterlist *sg = scmd->request_buffer;
 		for (i = 0, xfer_cnt = 0;
 		     (i < scmd->use_sg) && (xfer_cnt < count); i++) {
-			if (!IPS_SG_ADDRESS(&sg[i]))
+			if (!IPS_CHECK_SG_ADDRESS(&sg[i]))
 				return;
 			min_cnt = min(count - xfer_cnt, sg[i].length);
 			memcpy(&cdata[xfer_cnt], IPS_SG_ADDRESS(&sg[i]),

Re: ips.c warnings

[Matthew Wilcox]

On Thu, Oct 20, 2005 at 12:59:54PM -0400, Jack Hammer wrote:
> I did not use kmap() because these routines can be called from the interrupt handler context and kmap() can sleep.

Is there a reason you can't use kmap_atomic() then?

RE: ips.c warnings

[Hammer, Jack]

I have no problem trying kmap_atomic() as long as there's a sound,
technical reason for changing code that's worked well for years.

What are the advantages/reasons of changing to use kmap_atomic() ?   It
appears very few SCSI drivers use it.


-----Original Message-----
From: linux-scsi-owner@vger.kernel.org
[mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Matthew Wilcox
Sent: Thursday, October 20, 2005 1:06 PM
To: Hammer, Jack
Cc: akmpm@osdl.org; James.Bottomley@steeleye.com;
linux-scsi@vger.kernel.org; IpsLinux
Subject: Re: ips.c warnings

On Thu, Oct 20, 2005 at 12:59:54PM -0400, Jack Hammer wrote:
> I did not use kmap() because these routines can be called from the
interrupt handler context and kmap() can sleep.

Is there a reason you can't use kmap_atomic() then?
-
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: ips.c warnings

[Christoph Hellwig]

On Thu, Oct 20, 2005 at 01:25:28PM -0400, Hammer, Jack wrote:
> 
> I have no problem trying kmap_atomic() as long as there's a sound,
> technical reason for changing code that's worked well for years.
> 
> What are the advantages/reasons of changing to use kmap_atomic() ?   It
> appears very few SCSI drivers use it.

I think the real question is: what are these checks for.  You should never
get an SG list with a NULL struct page or an SGL entry that points to address
0 from the higher level code, and no other drivers checks for that condition.

RE: ips.c warnings

[IpsLinux]

Sounds good to me.  That's easy enough to do.

I was just hesitant to take out an error check ( in code I didn't
originate ) without a 100% guarantee that it's OK without it.

I'll send a new patch and totally eliminate the checks and extra macros
...

Thanks.



-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org] 
Sent: Thursday, October 20, 2005 1:29 PM
To: Hammer, Jack
Cc: Matthew Wilcox; akmpm@osdl.org; James.Bottomley@steeleye.com;
linux-scsi@vger.kernel.org; IpsLinux
Subject: Re: ips.c warnings

On Thu, Oct 20, 2005 at 01:25:28PM -0400, Hammer, Jack wrote:
> 
> I have no problem trying kmap_atomic() as long as there's a sound, 
> technical reason for changing code that's worked well for years.
> 
> What are the advantages/reasons of changing to use kmap_atomic() ?
It
> appears very few SCSI drivers use it.

I think the real question is: what are these checks for.  You should
never get an SG list with a NULL struct page or an SGL entry that points
to address 0 from the higher level code, and no other drivers checks for
that condition.

RE: ips.c warnings

[Salyzyn, Mark]

Jack, do not forget legacy kernels. Christoph is notorious for dealing
only with latest kernel only.

Christoph, has this always been true? I believe it to be, but just in
case ...

Sincerely -- Mark Salyzyn


-----Original Message-----
From: linux-scsi-owner@vger.kernel.org
[mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of IpsLinux
Sent: Thursday, October 20, 2005 1:33 PM
To: Christoph Hellwig
Cc: Matthew Wilcox; akmpm@osdl.org; James.Bottomley@steeleye.com;
linux-scsi@vger.kernel.org; IpsLinux
Subject: RE: ips.c warnings


Sounds good to me.  That's easy enough to do.

I was just hesitant to take out an error check ( in code I didn't
originate ) without a 100% guarantee that it's OK without it.

I'll send a new patch and totally eliminate the checks and extra macros
...

Thanks.



-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org] 
Sent: Thursday, October 20, 2005 1:29 PM
To: Hammer, Jack
Cc: Matthew Wilcox; akmpm@osdl.org; James.Bottomley@steeleye.com;
linux-scsi@vger.kernel.org; IpsLinux
Subject: Re: ips.c warnings

On Thu, Oct 20, 2005 at 01:25:28PM -0400, Hammer, Jack wrote:
> 
> I have no problem trying kmap_atomic() as long as there's a sound,
> technical reason for changing code that's worked well for years.
> 
> What are the advantages/reasons of changing to use kmap_atomic() ?
It
> appears very few SCSI drivers use it.

I think the real question is: what are these checks for.  You should
never get an SG list with a NULL struct page or an SGL entry that points
to address 0 from the higher level code, and no other drivers checks for
that condition.



-
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: ips.c warnings

[Christoph Hellwig]

On Thu, Oct 20, 2005 at 01:38:30PM -0400, Salyzyn, Mark wrote:
> Jack, do not forget legacy kernels. Christoph is notorious for dealing
> only with latest kernel only.
> 
> Christoph, has this always been true? I believe it to be, but just in
> case ...

For mainline this was always true.  OTOH Adaptec is notorious for supporting
very odd non-standard kernels, so who knows ;-)

Re: ips.c warnings

[James Bottomley]

On Thu, 2005-10-20 at 18:29 +0100, Christoph Hellwig wrote:
> I think the real question is: what are these checks for.  You should never
> get an SG list with a NULL struct page or an SGL entry that points to address
> 0 from the higher level code, and no other drivers checks for that condition.

Actually, no, that's not the problem.

The macro is taking page_address((sgl)->page).  That can be null if the
page isn't mapped into the kernel.  According to my reading of the code
it's doing this for some type of buffering reasons which require an
in-kernel memcpy().  If it finds page_address() to be NULL then it
simply doesn't do the operation.  Scatter/Gather lists, by and large,
come from a user so this operation should be failing all the time in any
x86 machine that has > 1GB of memory, I would have thought.

That's why I advocated using kmap (or kmap_atomic) because you need to
make the page exist in the kernel while you do the copy (and then unmap
it afterwards).  We've been having to add this operation to quite a lot
of raid drivers recently because of the changes to SCSI that make all
operations go via block layer scatter/gather lists.

James

RE: ips.c warnings

[IpsLinux]

OK, I'm convinced. I will re-do the ips patch, eliminate the error
checking, and use kmap_atomic().

The patch should be small, but since this is in the main I/O path, the
skeptic in me says to stress the changes I make thoroughly under both
the 2.4 and 2.6 kernels. So it may be a couple days before I post an
updated patch.

 

-----Original Message-----
From: Christoph Hellwig [mailto:hch@infradead.org] 
Sent: Thursday, October 20, 2005 1:40 PM
To: Salyzyn, Mark
Cc: IpsLinux; Christoph Hellwig; Matthew Wilcox; akmpm@osdl.org;
James.Bottomley@steeleye.com; linux-scsi@vger.kernel.org
Subject: Re: ips.c warnings

On Thu, Oct 20, 2005 at 01:38:30PM -0400, Salyzyn, Mark wrote:
> Jack, do not forget legacy kernels. Christoph is notorious for dealing

> only with latest kernel only.
> 
> Christoph, has this always been true? I believe it to be, but just in 
> case ...

For mainline this was always true.  OTOH Adaptec is notorious for
supporting very odd non-standard kernels, so who knows ;-)