[PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl

[PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl

[Ming Lei]

The current way isn't safe for chained sgl, so use sg helper to
operate sgl.

Cc: devel@driverdev.osuosl.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/staging/unisys/visorhba/visorhba_main.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c b/drivers/staging/unisys/visorhba/visorhba_main.c
index 2dad36a05518..dd979ee4dcf1 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -871,12 +871,11 @@ static void do_scsi_nolinuxstat(struct uiscmdrsp *cmdrsp,
 			return;
 		}
 
-		sg = scsi_sglist(scsicmd);
-		for (i = 0; i < scsi_sg_count(scsicmd); i++) {
-			this_page_orig = kmap_atomic(sg_page(sg + i));
+		scsi_for_each_sg(scsicmd, sg, scsi_sg_count(scsicmd), i) {
+			this_page_orig = kmap_atomic(sg_page(sg));
 			this_page = (void *)((unsigned long)this_page_orig |
-					     sg[i].offset);
-			memcpy(this_page, buf + bufind, sg[i].length);
+					     sg->offset);
+			memcpy(this_page, buf + bufind, sg->length);
 			kunmap_atomic(this_page_orig);
 		}
 		kfree(buf);
-- 
2.20.1

Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl

[Greg Kroah-Hartman]

On Thu, Jun 13, 2019 at 03:13:28PM +0800, Ming Lei wrote:
> The current way isn't safe for chained sgl, so use sg helper to
> operate sgl.

I can not make any sense out of this changelog.

What "isn't safe"?  What is a "sgl"?

Can this be applied "out of order"?

confused,

greg k-h

Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl

[Ming Lei]

On Thu, Jun 13, 2019 at 11:52:14AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 13, 2019 at 03:13:28PM +0800, Ming Lei wrote:
> > The current way isn't safe for chained sgl, so use sg helper to
> > operate sgl.
> 
> I can not make any sense out of this changelog.
> 
> What "isn't safe"?  What is a "sgl"?

sgl is 'scatterlist' in kernel, and several linear sgl can be chained
together, so accessing the sgl in linear way may see a chained sg, which
is like a link pointer, then may cause trouble for driver.

> 
> Can this be applied "out of order"?

Yes, there isn't any dependency among the 15 patches.


Thanks,
Ming

Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl

[Greg Kroah-Hartman]

On Thu, Jun 13, 2019 at 06:04:11PM +0800, Ming Lei wrote:
> On Thu, Jun 13, 2019 at 11:52:14AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 13, 2019 at 03:13:28PM +0800, Ming Lei wrote:
> > > The current way isn't safe for chained sgl, so use sg helper to
> > > operate sgl.
> > 
> > I can not make any sense out of this changelog.
> > 
> > What "isn't safe"?  What is a "sgl"?
> 
> sgl is 'scatterlist' in kernel, and several linear sgl can be chained
> together, so accessing the sgl in linear way may see a chained sg, which
> is like a link pointer, then may cause trouble for driver.

What kind of "trouble"?  Is this a bug fix that needs to be backported
to stable kernels?  How can this be triggered?

> > Can this be applied "out of order"?
> 
> Yes, there isn't any dependency among the 15 patches.

Then perhaps you shouldn't send a numbered patch series with different
patches sent to different maintainers, it just causes confusion :)

thanks,

greg k-h

Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl

[Dan Carpenter]

On Thu, Jun 13, 2019 at 06:04:11PM +0800, Ming Lei wrote:
> On Thu, Jun 13, 2019 at 11:52:14AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 13, 2019 at 03:13:28PM +0800, Ming Lei wrote:
> > > The current way isn't safe for chained sgl, so use sg helper to
> > > operate sgl.
> > 
> > I can not make any sense out of this changelog.
> > 
> > What "isn't safe"?  What is a "sgl"?
> 
> sgl is 'scatterlist' in kernel, and several linear sgl can be chained
> together, so accessing the sgl in linear way may see a chained sg, which
> is like a link pointer, then may cause trouble for driver.
> 

So from a user perspective it results in an Oops?  It would be really
cool if you had the copy of the Oops btw so people could grep the git
history for it.

(You need to resend with the improved commit message).

regards,
dan carpenter

Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl

[James Bottomley]

On Thu, 2019-06-13 at 12:16 +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 13, 2019 at 06:04:11PM +0800, Ming Lei wrote:
> > On Thu, Jun 13, 2019 at 11:52:14AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 13, 2019 at 03:13:28PM +0800, Ming Lei wrote:
> > > > The current way isn't safe for chained sgl, so use sg helper to
> > > > operate sgl.
> > > 
> > > I can not make any sense out of this changelog.
> > > 
> > > What "isn't safe"?  What is a "sgl"?
> > 
> > sgl is 'scatterlist' in kernel, and several linear sgl can be
> > chained together, so accessing the sgl in linear way may see a
> > chained sg, which is like a link pointer, then may cause trouble
> > for driver.
> 
> What kind of "trouble"?  Is this a bug fix that needs to be
> backported to stable kernels?  How can this be triggered?

OK, stop.  I haven't seen the commit log since the original hasn't
appeared on the list yet, but the changelog needs to say something like
this to prevent questions like the above, as I asked in the last
review.  Please make it something like this:

---
Scsi MQ makes a large static allocation for the first scatter gather
list chunk for the driver to use.  This is a performance headache we'd
like to fix by reducing the size of the allocation to a 2 element
array.  Doing this will break the current guarantee that any driver
using SG_ALL doesn't need to use the scatterlist iterators and can get
away with directly dereferencing the array.  Thus we need to update all
drivers to use the scatterlist iterators and remove direct indexing of
the scatterlist array before reducing the initial scatterlist
allocation size in SCSI.
---

James