Skip to contentSkip to conversation

sg: remove unreachable code in SET_FORCE_LOW_DMA

PATCH In review Hannes Reinecke 2 participants 5 messages Feb 1, 2017

Subsystems (2)
SCSI SCSI SG driver
lore
Share & tools
lore

Hannes Reinecke

author
#0 lore raw

PATCH 1/1 sg: remove unreachable code in SET_FORCE_LOW_DMA

The low_dma value is always '1' in that branch, so the remaining 'if' statement can never be reached.

Signed-off-by Hannes Reinecke <[email protected]>
---
 drivers/scsi/sg.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
drivers/scsi/sg.c +2 -7
index dbe5b4b..652b934 100644
@@ -890,14 +890,9 @@ static int max_sectors_bytes(struct request_queue *q)
 		result = get_user(val, ip);
 		if (result)
 			return result;
-		if (val) {
+		if (val)
 			sfp->low_dma = 1;
-			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
-				val = (int) sfp->reserve.bufflen;
-				sg_remove_scat(sfp, &sfp->reserve);
-				sg_build_reserve(sfp, val);
-			}
-		} else {
+		else {
 			if (atomic_read(&sdp->detaching))
 				return -ENODEV;
 			sfp->low_dma = sdp->device->host->unchecked_isa_dma;
Signature · 1 line
1.8.5.6
#1 lore raw
Quoted 17 lines · Hannes Reinecke

The low_dma value is always '1' in that branch, so the remaining 'if' statement can never be reached.

Signed-off-by Hannes Reinecke <[email protected]>
---
 drivers/scsi/sg.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
drivers/scsi/sg.c +0 -0
index dbe5b4b..652b934 100644
@@ -890,14 +890,9 @@ static int max_sectors_bytes(struct request_queue *q)
 		result = get_user(val, ip);
 		if (result)
 			return result;

drivers/scsi/sg.c static int max_sectors_bytes(struct request_queue *q) · line 895

Quoted 10 lines
drivers/scsi/sg.c +2 -7
-		if (val) {
+		if (val)
 			sfp->low_dma = 1;
-			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
-				val = (int) sfp->reserve.bufflen;
-				sg_remove_scat(sfp, &sfp->reserve);
-				sg_build_reserve(sfp, val);
-			}
-		} else {
+		else {

I think the proper fix is to check sfp->low_dma for 0 before updating it, at least that seems to be the intent here.

#2 lore raw
Quoted 2 lines · Christoph Hellwig

I think the proper fix is to check sfp->low_dma for 0 before updating it, at least that seems to be the intent here.

Looking at this code a bit more I think the actual, real proper fix is to remove the SET_FORCE_LOW_DMA entirely (except maybe the flag for SG_GET_LOW_DMA and procfs output). The block layer already does bounce buffering if the DMA addressing constraints are not met, so all this should just go away.

Btw, any rason Doug isn't on Cc on these sg.c patches?

author
#3 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 2 lines · Christoph Hellwig

I think the proper fix is to check sfp->low_dma for 0 before updating it, at least that seems to be the intent here.

Looking at this code a bit more I think the actual, real proper fix is to remove the SET_FORCE_LOW_DMA entirely (except maybe the flag for SG_GET_LOW_DMA and procfs output). The block layer already does bounce buffering if the DMA addressing constraints are not met, so all this should just go away.

Oh, I'm more than happy to do so.

Quoted 2 lines

Btw, any rason Doug isn't on Cc on these sg.c patches?

None. Oversight from my side.

Will be sending a v2 with those two patches rolled into one patchset.

Cheers,

Hannes

Signature · 5 lines
Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
author
#4 lore raw
In reply to Christoph Hellwig
Quoted 23 lines · Christoph Hellwig
Quoted 22 lines · Hannes Reinecke

The low_dma value is always '1' in that branch, so the remaining 'if' statement can never be reached.

Signed-off-by Hannes Reinecke <[email protected]>
---
 drivers/scsi/sg.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
drivers/scsi/sg.c +1 -3
index dbe5b4b..652b934 100644
@@ -890,14 +890,9 @@ static int max_sectors_bytes(struct request_queue *q)
 		result = get_user(val, ip);
 		if (result)
 			return result;
-		if (val) {
+		if (val)
 			sfp->low_dma = 1;
-			if ((0 == sfp->low_dma) && (0 == sg_res_in_use(sfp))) {
-				val = (int) sfp->reserve.bufflen;
Quoted 9 lines

drivers/scsi/sg.c static int max_sectors_bytes(struct request_queue *q) · line 895

Quoted 5 lines
drivers/scsi/sg.c +1 -4
-				sg_remove_scat(sfp, &sfp->reserve);
-				sg_build_reserve(sfp, val);
-			}
-		} else {
+		else {

I think the proper fix is to check sfp->low_dma for 0 before updating it, at least that seems to be the intent here.

May. Or may not.

But this code path has _never_ been taken for the last ten years; this particular code predates the initial git check-in from Linus. Where would be the point of enabling it now?
(Especially given all the issues we're having with the sg driver ...)

Cheers,

Hannes

Signature · 5 lines
Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)