Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl
The current way isn't safe for chained sgl, so use sg helper to
operate sgl.
Cc: de...@driverdev.osuosl.org
Cc: Greg Kroah-Hartman
Signed-off-by: Ming Lei
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
